fix(bigtable): don't notify config listeners while holding the manager lock - #14294
Merged
mutianf merged 1 commit intoSep 8, 2026
Merged
Conversation
…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
Contributor
There was a problem hiding this comment.
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.
nimf
approved these changes
Sep 8, 2026
This was referenced Sep 22, 2026
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>
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
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.
Fixes #14177
Fixes #13903
The bug
ClientConfigurationManager.sendRequestWithRetries()wassynchronizedand registered itswhenComplete()continuation while still holding the manager's monitor.When the
GetClientConfigurationresponse lands before the continuation is registered,CompletableFutureruns that continuation inline on the registering thread. SosetClientConfiguration()->notifyListeners()invoked alien listener code under the monitor. Any thread that holds a listener's own lock and then calls asynchronizedmanager 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'sLocal client config override: session_configuration { }line — i.e. enteringtestDeadlockPrevention— and the job then hangs until it is cancelled hours later (~6h in #13903, ~100min in #14177).Evidence:
ThreadMXBean.findDeadlockedThreads()returns two threads; the main thread shows- locked ClientConfigurationManager@... at depth 11insidesendRequestWithRetrieswhile blocked acquiring the listener's lock, and the other thread isBLOCKEDon the manager monitor insidegetClientConfiguration().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 defaultSAME_THREADmode 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()andnotifyListeners()each take the monitor themselves in narrow blocks. The change removes an accidental lock hold, not a required one.Test changes
testDeadlockPreventionrelies on a real RPC and only hits the bad interleaving by luck. The new test installs aClientInterceptorthat answersGetClientConfigurationsynchronously on the calling thread, so the future is always already complete when the continuation is registered, and assertsThread.holdsLock(manager)isfalsein the callback. With the product change reverted it fails every run.@Timeoutnow usesThreadMode.SEPARATE_THREADfor 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 withTimeoutExceptionrather than hanging. Scoped to this class rather than set repo-wide, to avoid changing execution semantics for unrelated tests.testDeadlockPrevention, which had been@Disabledfor Bigtable flaky test - ClientConfigurationManagerTest#testDeadlockPrevention #13903.Verification
mvn -pl google-cloud-bigtable test: 2446 tests, 0 failures, 0 errors, 1 skipped;ClientConfigurationManagerTest11/11.mvn -pl google-cloud-bigtable fmt:check: clean (644 files, 0 non-complying).