Skip to content

fix(bigtable): don't notify config listeners while holding the manager lock - #14294

Merged
mutianf merged 1 commit into
googleapis:mainfrom
mutianf:fix-config-manager-listener-deadlock
Sep 8, 2026
Merged

mutianf merged 1 commit into
googleapis:mainfrom
mutianf:fix-config-manager-listener-deadlock

Conversation

@mutianf

@mutianf mutianf commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Fixes #14177
Fixes #13903

The bug

ClientConfigurationManager.sendRequestWithRetries() was synchronized and registered its whenComplete() continuation while still holding the manager's monitor.

When the GetClientConfiguration response lands before the continuation is registered, CompletableFuture runs that continuation inline on the registering thread. So setClientConfiguration() -> notifyListeners() invoked alien listener code under the monitor. Any thread that holds a listener's own lock and then calls a synchronized manager method (e.g. getClientConfiguration()) closes the cycle and deadlocks.

Both linked issues are this same bug: in each CI log the output stops immediately after testSysPropOverrideSessionLoadZero's Local client config override: session_configuration { } line — i.e. entering testDeadlockPrevention — and the job then hangs until it is cancelled hours later (~6h in #13903, ~100min in #14177).

Evidence:

  • Deterministic reproduction. ThreadMXBean.findDeadlockedThreads() returns two threads; the main thread shows - locked ClientConfigurationManager@... at depth 11 inside sendRequestWithRetries while blocked acquiring the listener's lock, and the other thread is BLOCKED on the manager monitor inside getClientConfiguration().
  • Occurs naturally. Over a real loopback gRPC channel under CPU contention, 4 / 2000 iterations notified listeners with Thread.holdsLock(manager) == true, clustered during JIT warmup.

Why it hangs forever instead of timing out

The class already had @Timeout(60s), but JUnit 5's default SAME_THREAD mode enforces timeouts by interrupting the test thread, and a thread blocked on monitor entry cannot be interrupted. The timeout was a no-op, so the deadlock wedged the whole surefire JVM until the CI job itself was cancelled.

The fix

Narrow the lock so it covers only sendRequest(); completing the future and registering the continuation now happen outside it.

This does not widen any race. The continuation was already required to be correct without the lock, since it runs lock-free whenever the response arrives on a gRPC thread — setClientConfiguration(), handleFailedFetch() and notifyListeners() each take the monitor themselves in narrow blocks. The change removes an accidental lock hold, not a required one.

Test changes

  • New deterministic regression test. testDeadlockPrevention relies on a real RPC and only hits the bad interleaving by luck. The new test installs a ClientInterceptor that answers GetClientConfiguration synchronously on the calling thread, so the future is always already complete when the continuation is registered, and asserts Thread.holdsLock(manager) is false in the callback. With the product change reverted it fails every run.
  • @Timeout now uses ThreadMode.SEPARATE_THREAD for this class, so a future regression fails in seconds instead of wedging the runner. Verified: against the unfixed manager, a deadlocking test now fails in ~5s with TimeoutException rather than hanging. Scoped to this class rather than set repo-wide, to avoid changing execution semantics for unrelated tests.
  • Re-enabled testDeadlockPrevention, which had been @Disabled for Bigtable flaky test - ClientConfigurationManagerTest#testDeadlockPrevention #13903.

Verification

  • mvn -pl google-cloud-bigtable test: 2446 tests, 0 failures, 0 errors, 1 skipped; ClientConfigurationManagerTest 11/11.
  • mvn -pl google-cloud-bigtable fmt:check: clean (644 files, 0 non-complying).
  • Negative control: reverting the product change makes the new test fail deterministically.

…r lock

ClientConfigurationManager.sendRequestWithRetries() was synchronized and
registered its whenComplete() continuation while still holding the manager's
monitor. When the GetClientConfiguration response lands before the continuation
is registered, CompletableFuture runs the continuation inline on the registering
thread, so setClientConfiguration() -> notifyListeners() invoked alien listener
code under the monitor. Any thread that holds a listener's own lock and then
calls a synchronized manager method (e.g. getClientConfiguration()) closes the
cycle and deadlocks.

Narrow the lock so it covers only sendRequest(); completing the future and
registering the continuation now happen outside it. The continuation was already
required to be correct without the lock, since it runs lock-free whenever the
response arrives on a gRPC thread -- setClientConfiguration(), handleFailedFetch()
and notifyListeners() each take the monitor themselves in narrow blocks.

On the test side:

- Add a deterministic regression test. testDeadlockPrevention relies on a real
  RPC and only hits the bad interleaving by luck; the new test installs a
  ClientInterceptor that answers GetClientConfiguration synchronously on the
  calling thread, so the future is always already complete when the continuation
  is registered, and asserts Thread.holdsLock(manager) is false in the callback.
  With the product change reverted it fails every run.

- Run this class's @timeout in ThreadMode.SEPARATE_THREAD. This is not cosmetic:
  the default SAME_THREAD mode enforces timeouts by interrupting the test thread,
  and a thread blocked on monitor entry cannot be interrupted, which is why these
  hangs wedged the surefire JVM until the CI job itself was cancelled hours later
  instead of failing.

- Re-enable testDeadlockPrevention, disabled for googleapis#13903.

Fixes googleapis#14177
Fixes googleapis#13903
@mutianf
mutianf requested review from a team as code owners September 8, 2026 20:35

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors ClientConfigurationManager to avoid deadlocks by narrowing the synchronized block in sendRequestWithRetries. This ensures that the manager's monitor lock is not held when completing futures or notifying listeners. Additionally, the test suite has been updated to re-enable the deadlock prevention test, configure the test timeout to run in a separate thread to prevent silent hangs, and add a new test specifically verifying that listeners are notified without holding the manager lock. There are no review comments to evaluate, so no feedback is provided.

@mutianf
mutianf merged commit 4426ccd into googleapis:main Sep 8, 2026
208 checks passed
@mutianf
mutianf deleted the fix-config-manager-listener-deadlock branch September 8, 2026 21:50
blakeli0 pushed a commit that referenced this pull request Sep 23, 2026
🤖 I have created a release *beep* *boop*
---


<details><summary>1.92.0</summary>

##
[1.92.0](v1.91.0...v1.92.0)
(2026-09-23)


### Features

* **bigquery-jdbc:** add `EnableTimestampPicos` connection property and
its plumbing
([#14284](#14284))
([b4aa5ac](b4aa5ac))
* **bigquery-jdbc:** implement picosecond temporal math and formatting
engine
([#14286](#14286))
([2a9612a](2a9612a))
* **bigquery-jdbc:** support picosecond in REST JSON path and nested
types
([#14334](#14334))
([15ffe4a](15ffe4a))
* **bigquery-jdbc:** support picosecond in `PreparedStatement`
parameters and batching
([#14373](#14373))
([c1aac66](c1aac66))
* **bigquery-jdbc:** support picosecond timestamp in `ResultSetMetaData`
and `DatabaseMetaData`
([#14358](#14358))
([43acdd3](43acdd3))
* **bigquery-jdbc:** support picosecond timestamps in Arrow Storage Read
API and nested types
([#14332](#14332))
([b5d9aca](b5d9aca))
* **bigquery-jdbc:** support qualified project delimiter in
`DefaultDataset` property
([#14240](#14240))
([6e8d6c8](6e8d6c8))
* **bigquery:** accelerate row-based query() with Arrow wire format
([#14405](#14405))
([8d12a8f](8d12a8f))
* **bigquery:** add ArrowDeserializer helper utility
([#13943](#13943))
([d9a298b](d9a298b))
* **bigquery:** add ArrowQueryPageFetcher for Arrow query result
pagination
([#14404](#14404))
([615409f](615409f))
* **bigquery:** add ArrowQueryResult and ArrowQueryResultImpl for Arrow
result streaming
([#13944](#13944))
([a62fdf8](a62fdf8))
* **bigquery:** add Storage Read API slow-path fallback for row-based
query()
([#14409](#14409))
([26e568a](26e568a))
* **bigquery:** add zero-copy queryArrow API for Arrow VectorSchemaRoot
streaming
([#14402](#14402))
([b44ffe8](b44ffe8))
* **bigquery:** make BigQuery AutoCloseable with default no-op close
method
([#14434](#14434))
([00bf3de](00bf3de))
* **firestore:** add support for BSON types
([#13189](#13189))
([8a123d9](8a123d9))
* **gax:** add ApiCallContext and request-level settings overloads to
ResumableUploadCallable
([#14251](#14251))
([e8cbd42](e8cbd42))
* **gax:** add globalTimeout settings field to
ResumableUploadCallSettings
([#14253](#14253))
([438cda6](438cda6))
* **gax:** add resumable upload error classification and retry algorithm
([#14419](#14419))
([b70396d](b70396d))
* **gax:** add ResumableUploadCallable creation to Callables and
HttpJsonCallableFactory
([#14242](#14242))
([7de24de](7de24de))
* **gax:** implement baseline Callable and Future for resumable uploads
([#14241](#14241))
([5a54db9](5a54db9))
* **generator:** add model flag and allowlist parser for resumable
upload RPCs
([#14317](#14317))
([acc1856](acc1856))
* **generator:** emit resumable upload client surface
([#14319](#14319))
([a9fed00](a9fed00))
* **generator:** emit resumable upload settings and HttpJson upload stub
([#14321](#14321))
([c122474](c122474))
* **generator:** enable resumable upload generation for showcase
([#14325](#14325))
([f9ebd79](f9ebd79))
* **generator:** switch resumable upload specialized stubs to package
private
([#14471](#14471))
([0d4e875](0d4e875))
* **generator:** wire transport stub delegation to resumable upload
stubs
([#14322](#14322))
([cc4b980](cc4b980))
* **google/cloud/backupdr/v1beta:** add backupdr
([#14410](#14410))
([a4a47da](a4a47da))
* **google/cloud/networkservices/v1beta1:** add networkservices
([#14407](#14407))
([21c4955](21c4955))
* **pubsub:** add publish telemetry headers for publish attempt
observability
([#14338](#14338))
([c167ab8](c167ab8))
* **pubsub:** implement publish hedging to reduce tail latency
([#13735](#13735))
([b302615](b302615))
* **spanner:** Support dynamic TLS certificate and key rotation for
Spanner Omni
([#14456](#14456))
([ffc745c](ffc745c))
* **storage/control:** add delete folder recursive sample
([#13642](#13642))
([f4b1b46](f4b1b46))
* **storage/control:** add delete folder recursive sample
([#14397](#14397))
([2c01d55](2c01d55))


### Bug Fixes

* **auth:** restore transportFactory upon deserialization in
InternalAwsSecurityCredentialsSupplier
([#14340](#14340))
([beea42f](beea42f))
* **bigquery-jdbc:** ensure row ordering in PCNT IT
([#14330](#14330))
([a16f048](a16f048))
* **bigquery-jdbc:** fix htapi fallback due to permission logic
([#14418](#14418))
([21e6dc8](21e6dc8))
* **bigquery-jdbc:** fix Timestamp assertions
([#14290](#14290))
([533ba14](533ba14))
* **bigquery-jdbc:** handle null parameters in Storage Write API bulk
inserts
([#14270](#14270))
([dd2c41a](dd2c41a)),
refs
[#14066](#14066)
* **bigquery-jdbc:** handle SQL NULLs in ResultSet primitive getters
([#14383](#14383))
([8e464fe](8e464fe)),
refs
[#14371](#14371)
* **bigquery:** default Arrow pagination stream location to US instead
of global
([#14458](#14458))
([2775eb1](2775eb1))
* **bigquery:** preserve page token and paginate correctly in Arrow
query when maxResults is set
([#14469](#14469))
([f5601f4](f5601f4))
* **bigquery:** use first page row count for Arrow query pagination
offset
([#14466](#14466))
([9d10dd0](9d10dd0))
* **bigtable:** don't notify config listeners while holding the manager
lock
([#14294](#14294))
([4426ccd](4426ccd))
* **bigtable:** fall back to classic path when per-RPC CallCredentials
are set on session path
([#14477](#14477))
([57bacb0](57bacb0))
* **bigtable:** fix abnormal session closures and scale-up in session
pool
([#14431](#14431))
([6361ecd](6361ecd))
* **biqguery:** fix undeclared QueryParameter wiring in QueryStatistics
([#14401](#14401))
([64cf1d3](64cf1d3))
* **bom:** restore google-cloud-spanner-jdbc to libraries-bom
([#14362](#14362))
([bc7be5e](bc7be5e)),
refs
[#14347](#14347)
* **spanner:** honor maxAttempts and totalTimeout in streaming resume
loop
([#14370](#14370))
([305f47d](305f47d))
* **spanner:** only set snapshot isolation read timestamp for SI or
optimistic txns in CloudClientExecutor
([#14346](#14346))
([54c0d0f](54c0d0f))
* **spanner:** prevent statement cancellation race in
AbstractBaseUnitOfWork
([#14283](#14283))
([d9a8eef](d9a8eef))
* **spanner:** re-enable ITInstanceAdminTest on cloud-devel and
cloud-staging
([#14281](#14281))
([89a8268](89a8268))


### Performance Improvements

* **spanner:** stop re-parsing the request id on every RPC
([#14353](#14353))
([46108f4](46108f4))


### Documentation

* Add a Http/Json Post-Quantum Cryptography Guide
([#13963](#13963))
([fcc65b0](fcc65b0))
* **bigquery:** add QueryArrow code sample and document JDK 17+ JVM
requirements
([#14437](#14437))
([bd363f6](bd363f6))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[java-bigtable] BigTable test hangs Bigtable flaky test - ClientConfigurationManagerTest#testDeadlockPrevention

2 participants