Skip to content

Commit 4426ccd

Browse files
authored
fix(bigtable): don't notify config listeners while holding the manager lock (#14294)
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 #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.
1 parent a03ee10 commit 4426ccd

2 files changed

Lines changed: 127 additions & 6 deletions

File tree

‎java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/util/ClientConfigurationManager.java‎

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -286,14 +286,29 @@ private CompletableFuture<ClientConfiguration> startPolling() {
286286
return result;
287287
}
288288

289-
private synchronized void sendRequestWithRetries(
289+
private void sendRequestWithRetries(
290290
int attemptCount, CompletableFuture<ClientConfiguration> finalResult) {
291-
if (closing) {
291+
// Nothing after the synchronized block below may run while holding this monitor, because both
292+
// completing finalResult and registering the continuation can run alien code inline on the
293+
// current thread: completing the future runs whatever the caller of start() chained onto it,
294+
// and the continuation reaches registered ConfigListeners through setClientConfiguration() ->
295+
// notifyListeners(). Notifying a listener under this monitor deadlocks against any thread that
296+
// holds the listener's own lock and calls a synchronized method such as
297+
// getClientConfiguration().
298+
@Nullable CompletableFuture<ClientConfiguration> currentRequest = null;
299+
synchronized (this) {
300+
if (!closing) {
301+
// Note that this only starts the call: the response can land before whenComplete() is
302+
// registered below, in which case the continuation runs inline on this thread.
303+
currentRequest = sendRequest();
304+
}
305+
}
306+
307+
if (currentRequest == null) {
292308
finalResult.completeExceptionally(new RuntimeException("Client is closing"));
293309
return;
294310
}
295311

296-
CompletableFuture<ClientConfiguration> currentRequest = sendRequest();
297312
// We only schedule the next poll after successfully getting a client config
298313
// from the server.
299314
@SuppressWarnings("UnusedVariable")

‎java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/util/ClientConfigurationManagerTest.java‎

Lines changed: 109 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -63,13 +63,15 @@
6363
import java.util.concurrent.ScheduledExecutorService;
6464
import java.util.concurrent.ScheduledFuture;
6565
import java.util.concurrent.TimeUnit;
66+
import java.util.concurrent.atomic.AtomicBoolean;
6667
import java.util.concurrent.atomic.AtomicInteger;
6768
import java.util.concurrent.atomic.AtomicReference;
69+
import javax.annotation.Nullable;
6870
import org.junit.jupiter.api.AfterEach;
6971
import org.junit.jupiter.api.BeforeEach;
70-
import org.junit.jupiter.api.Disabled;
7172
import org.junit.jupiter.api.Test;
7273
import org.junit.jupiter.api.Timeout;
74+
import org.junit.jupiter.api.Timeout.ThreadMode;
7375
import org.junit.jupiter.api.extension.ExtendWith;
7476
import org.mockito.ArgumentCaptor;
7577
import org.mockito.Mock;
@@ -79,7 +81,12 @@
7981
@ExtendWith(MockitoExtension.class)
8082
// Backstop against a wedged blocking call (e.g. manager.start().get()) hanging the CI runner
8183
// indefinitely. If any test exceeds this, fail fast with a diagnosable timeout instead.
82-
@Timeout(value = 60, unit = TimeUnit.SECONDS)
84+
//
85+
// SEPARATE_THREAD is required, not cosmetic: the default SAME_THREAD mode enforces the timeout by
86+
// interrupting the test thread, and a thread blocked on monitor entry (`synchronized`) cannot be
87+
// interrupted. A deadlock like the one testDeadlockPrevention guards against would therefore
88+
// silently wedge the whole surefire JVM until the CI job's own multi-hour timeout.
89+
@Timeout(value = 60, unit = TimeUnit.SECONDS, threadMode = ThreadMode.SEPARATE_THREAD)
8390
class ClientConfigurationManagerTest {
8491
private static final FeatureFlags FEATURE_FLAGS = FeatureFlags.getDefaultInstance();
8592

@@ -380,7 +387,63 @@ void testSysPropOverrideSessionLoadZero() throws Exception {
380387
assertThat(initialConfig).isEqualTo(service.config.get());
381388
}
382389

383-
@Disabled("https://github.com/googleapis/google-cloud-java/issues/13903")
390+
/**
391+
* The manager must never invoke a listener while holding its own monitor, no matter which thread
392+
* ends up running the config-fetch continuation. See {@link #testDeadlockPrevention()} for the
393+
* deadlock this prevents.
394+
*
395+
* <p>Where testDeadlockPrevention relies on a real RPC and so only catches the bad interleaving
396+
* by luck, this test pins it: the channel answers GetClientConfiguration synchronously on the
397+
* calling thread, so the config future is always already complete by the time
398+
* sendRequestWithRetries() registers its continuation, and the continuation always runs inline on
399+
* the polling thread.
400+
*/
401+
@Test
402+
void listenersAreNotifiedWithoutHoldingTheManagerLock() throws Exception {
403+
manager.close();
404+
405+
ChannelProviders.ChannelProvider synchronousProvider =
406+
new ForwardingChannelProvider(channelProvider) {
407+
@Override
408+
public ManagedChannelBuilder<?> newChannelBuilder() {
409+
return super.newChannelBuilder().intercept(new SynchronousConfigInterceptor(service));
410+
}
411+
};
412+
manager =
413+
new ClientConfigurationManager(
414+
FEATURE_FLAGS, CLIENT_INFO, synchronousProvider, noopDebugTracer, mockExecutor);
415+
416+
manager.start().get();
417+
418+
ArgumentCaptor<Runnable> runnableCaptor = ArgumentCaptor.forClass(Runnable.class);
419+
verify(mockExecutor, times(1)).schedule(runnableCaptor.capture(), anyLong(), any());
420+
421+
ClientConfigurationManager notifyingManager = manager;
422+
AtomicInteger notifications = new AtomicInteger();
423+
AtomicBoolean lockHeldDuringCallback = new AtomicBoolean();
424+
manager.addListener(
425+
ClientConfiguration::getSessionConfiguration,
426+
newValue -> {
427+
notifications.incrementAndGet();
428+
lockHeldDuringCallback.set(Thread.holdsLock(notifyingManager));
429+
});
430+
431+
// Change the watched section so the listener actually fires, then poll.
432+
ClientConfiguration.Builder builder = service.config.get().toBuilder();
433+
builder
434+
.getSessionConfigurationBuilder()
435+
.getSessionPoolConfigurationBuilder()
436+
.setLoadBalancingOptions(
437+
LoadBalancingOptions.newBuilder()
438+
.setLeastInFlight(LeastInFlight.newBuilder().setRandomSubsetSize(30)));
439+
service.config.set(builder.build());
440+
441+
runnableCaptor.getValue().run();
442+
443+
assertThat(notifications.get()).isEqualTo(1);
444+
assertThat(lockHeldDuringCallback.get()).isFalse();
445+
}
446+
384447
@Test
385448
void testDeadlockPrevention() throws Exception {
386449
// Initialize the manager and fetch the initial config to schedule polling.
@@ -552,6 +615,49 @@ public void getClientConfiguration(
552615
}
553616
}
554617

618+
/**
619+
* Answers GetClientConfiguration from {@link FakeConfigService#config} synchronously, on the
620+
* thread that issued the call, without touching the network. Used to make the "response already
621+
* delivered before the caller registers its continuation" interleaving deterministic.
622+
*/
623+
private static class SynchronousConfigInterceptor implements ClientInterceptor {
624+
private final FakeConfigService service;
625+
626+
SynchronousConfigInterceptor(FakeConfigService service) {
627+
this.service = service;
628+
}
629+
630+
@Override
631+
public <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall(
632+
MethodDescriptor<ReqT, RespT> methodDescriptor, CallOptions callOptions, Channel channel) {
633+
return new ClientCall<ReqT, RespT>() {
634+
private Listener<RespT> responseListener;
635+
636+
@Override
637+
public void start(Listener<RespT> responseListener, Metadata headers) {
638+
this.responseListener = responseListener;
639+
}
640+
641+
@Override
642+
public void request(int numMessages) {}
643+
644+
@Override
645+
public void cancel(@Nullable String message, @Nullable Throwable cause) {}
646+
647+
@Override
648+
public void halfClose() {
649+
@SuppressWarnings("unchecked")
650+
RespT response = (RespT) service.config.get();
651+
responseListener.onMessage(response);
652+
responseListener.onClose(Status.OK, new Metadata());
653+
}
654+
655+
@Override
656+
public void sendMessage(ReqT message) {}
657+
};
658+
}
659+
}
660+
555661
private static class OutstandingRpcCounter implements ClientInterceptor {
556662
private int numOutstandingRpcs = 0;
557663
private final Object lock = new Object();

0 commit comments

Comments
 (0)