fix(bigtable): bound SessionPoolImpl lock to prevent pod-wide wedge#13890
Merged
Conversation
Pods running the vRPC session pool intermittently went permanently wedged
(recurring ~7-9h): in-flight requests piled up, RED fired, and the pod shed
100% of traffic with no recovery until restart.
Root cause (from a production thread dump, 2026-07-23):
- SessionPoolImpl guarded all of its state with a single `synchronized(this)`
intrinsic monitor.
- The dump showed that monitor with NO owner but 39 waiters
(22 onVRpcComplete, 8 onSessionGoAway, 6 onSessionReady, plus watchdog /
cancel / deadline threads) — the classic signature of an *orphaned monitor*:
a thread died while holding the lock (e.g. a native-thread OOM,
"unable to create native thread", mid-critical-section) so the monitor was
never released.
- The shim probes the pool on EVERY rpc via hasSession()/newCall(), both
synchronized. Once the monitor was orphaned, every incoming request blocked
on it forever. An intrinsic monitor has no timeout, so this was unrecoverable.
Note on the native-thread OOM: it is transient/recoverable on its own (unlike a
heap OOM). The damage that persisted was the orphaned lock it left behind — that
is what turned a momentary resource blip into a permanent, pod-wide wedge.
Fix: replace the intrinsic monitor with an explicit ReentrantLock (`poolLock`)
and use a bounded tryLock on the request-serving hot path
(HOT_PATH_LOCK_TIMEOUT_MILLIS = 1000). If the lock can't be acquired in time the
caller degrades instead of blocking forever:
- newCall() -> returns a PendingVRpc (start() re-attempts, then fast-fails)
- hasSession() -> returns false
- PendingVRpc.start() -> fast-fails the vRPC with an uncommitted UNAVAILABLE,
so gax can still retry / fall back to the classic client
- PendingVRpc.cancel() -> also uses the bounded acquisition. cancel() runs on a
callback (op-executor) thread; on the old code its unbounded lock() would
park that thread on the wedged lock — the exact pile-up seen in the dump.
The eager pendingRpcs.remove() is only an optimization; drainTo()'s
isCancelled guard is the correctness backstop, so skipping it on timeout is
safe.
This converts a permanent hang into a bounded, retriable fast-fail, so the pod
sheds a wedged pool's traffic transiently and recovers automatically once the
lock frees.
tryAcquireHotPathLock() first does a non-interruptible zero-arg tryLock() before
the timed wait: the timed tryLock(timeout) throws InterruptedException
immediately if the caller already has its interrupt flag set (even when the lock
is free), which the old `synchronized` never observed. Without the fast path a
pre-existing interrupt on a reused worker thread would spuriously shed traffic on
a healthy pool.
Watchdog: its lock field changes Object -> Lock and is now constructed with the
pool's poolLock; forceClose runs outside the lock (snapshot-then-release), and it
never nests poolLock with scheduleLock.
Tests:
- orphanedPoolLock_hotPathDegradesInsteadOfHangingForever: simulates the dead
owner (a thread that grabs poolLock and never releases) and asserts
hasSession/newCall/start/cancel all degrade bounded, then the pool recovers
once the lock frees. Fails by timeout on the old synchronized design.
- healthyPool_preExistingInterrupt_doesNotSpuriouslyDegrade: guards the
zero-arg fast path — an already-interrupted caller must still be served.
- WatchdogTest updated for the Lock-typed constructor.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request replaces the intrinsic synchronized monitor in SessionPoolImpl with an explicit ReentrantLock to prevent threads from hanging indefinitely if the lock becomes wedged or orphaned. Bounded lock acquisition is introduced on the request-serving hot path, allowing the pool to degrade gracefully and fast-fail instead of blocking forever. Additionally, comprehensive unit tests are added to verify the behavior under wedged lock conditions and to ensure that pre-existing thread interrupts do not spuriously degrade a healthy pool. As there are no review comments, no further feedback is provided.
mutianf
enabled auto-merge (squash)
July 24, 2026 18:53
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prevent the session pool from wedged forever from a orphaned monitor.
Using a reentry lock so we can set a timeout when trying to aquire the lock. if it failed from the timeout, the pool would return unavailable, and fallback to the classic path, preventing traffic getting rejected forever.
We should implement self healing session pool in a follow up PR.