googleapis / googleapis/google-cloud-cpp

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

Đang mở
#16,248 1 bình luận 0 reaction 1 người được giao Được @scotthart nhận Xem trên GitHub
Ngôn ngữ chính
C++
Star
659
Fork
462
Merge trung bình
1 ngày 2 giờ
Pull request đã merge (30 ngày)
89

Mô tả

## 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.

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.