googleapis / googleapis/google-cloud-cpp

Pub/Sub SubscriptionLeaseManagement self-deadlock: StartRefreshTimer .then() continuation runs inline under mu_ (bigtable #4327 fix never ported)

Offen
#16,248 1 Kommentar 0 Reaktionen 1 zugewiesene Person Beansprucht von @scotthart Auf GitHub ansehen
Vorherrschende Sprache
C++
Sterne
659
Forks
462
Ø Merge
1 T. 2 Std.
Gemergte PRs (30 T.)
89

Beschreibung

## Summary

A Pub/Sub streaming-pull subscriber can permanently **self-deadlock** inside `SubscriptionLeaseManagement`. In `google/cloud/pubsub/internal/subscription_lease_management.cc`, `StartRefreshTimer()`'s `.then()` continuation can run **synchronously / inline** when the timer future is already satisfied (the deadline has already passed, or a completion-queue thread wins the satisfaction race) — while `mu_` is still held on the calling stack. The continuation (`OnRefreshTimer` → `RefreshMessageLeases`) then re-acquires the same **non-recursive** `std::mutex mu_` → self-deadlock. The subscription freezes at the lease-management mutex until the process restarts.

## Observed failure (thread state at the deadlock)

- A thread is executing `OnRefreshTimer` **holding** the `SubscriptionLeaseManagement` mutex. It calls `StartRefreshTimer`, which sets up a `.then()` future callback. That future fires **inline** (while the lock is still live on the stack), re-enters `OnRefreshTimer`, and tries to re-acquire the same non-recursive `std::mutex` → it deadlocks itself.
- The ack thread and the `OnRead` thread are both blocked behind that mutex. The TCP connection is alive (`OnRead` has a live message in hand) and the callback thread is running, yet the **entire subscription is frozen** at the lease-management mutex.
- It is **per-subscription** — sibling subscriptions on the same client keep running — and there is **no self-recovery**. Low-traffic / quiet subscriptions are the most exposed (the timer is more likely to be already satisfied when re-armed).

## Affected versions

`subscription_lease_management.cc` is **byte-identical from v2.28.0 through current `main`** (verified by direct diff). Its commit history shows it was **last touched in March 2024**, and no commit has ever addressed the inline-continuation / mutex re-entrancy. The latest release, **v3.6.0 (2026-06-18)**, and all 2026 release notes contain **no** Pub/Sub subscriber / lease / timer fix. So upgrading does not resolve it.

## Precedent — the identical bug was fixed in Bigtable but never ported here

The same re-entrancy class was reported for `bigtable::MutationBatcher` in **#4083** and fixed by **#4327**: `AsyncBulkApply`'s `.then()` continuation could execute synchronously/inline when the operation completed quickly while the `MutationBatcher` mutex was held; the continuation (`OnBulkApplyDone`) re-acquired the same non-recursive mutex → self-deadlock. The fix (quoting the PR) *"schedules the continuation on the completion queue explicitly"* — i.e. the `.then()` body just calls `RunAsync()` so the real work never runs inline under the lock.

That fix was **never ported to Pub/Sub's `SubscriptionLeaseManagement`**, which has the same inline-`.then()`-under-lock shape.

## Proposed fix

Apply the same pattern as #4327: dispatch the refresh-timer continuation via `cq.RunAsync(...)` so `OnRefreshTimer` always runs on a completion-queue thread with **no locks held**, never inline under `mu_`.

We have been running exactly this as a local patch to the v2.28.0 recipe and it resolves the deadlock. Happy to open a PR (under the CLA) if that's welcome.

Beitragsleitfaden

Beitragsleitfaden öffnen

Bewertung

Dieses Issue wurde noch nicht bewertet.

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.