Skip to content

Commit ae504a3

Browse files
author
opencode
committed
fix(pooling): avoid PoolEntry/ConnectionImpl deadlock by calling client.close()/emit outside monitor
The previous attempt to fix race conditions in PoolEntry made shutdown(), close() and handleConnectError() synchronized on the PoolEntry monitor while they invoked client.close() (which acquires the ConnectionImpl monitor) and emitted pool events. The Netty close-event path runs the reverse order: ConnectionImpl.onChannelClose() (synchronized on ConnectionImpl) calls back into PoolEntry.handleConnectError(), which then blocks on the PoolEntry monitor. This produced an ABBA deadlock between the HashedWheelTimer worker and the Netty event loop, which manifested as a permanent hang in DistributingRoundRobinBalancerTest (the @timeout(25) SameThread variant cannot interrupt CompletableFuture.join()). The thread dump of the hung JVM confirmed: - 'pool-N-thread-1' BLOCKED in ConnectionImpl.closeChannel, holding PoolEntry - 'multiThreadIoEventLoopGroup-2-1' BLOCKED in PoolEntry.handleConnectError, holding ConnectionImpl Fix: narrow the entry-monitor critical sections to field mutations only. client.close() and emit(...) are now called outside the monitor, breaking the lock-ordering cycle. Also fix a race in internalConnect(): assign connectFuture to the local chain BEFORE attaching whenComplete, so an inline failure completing before the method returns cannot have its nulling of connectFuture in handleConnectError() overwritten by a stale failed future. Verified locally: - DistributingRoundRobinBalancerTest: 5/5 on 2.11.8, 5/5 on 3.5.0, no hangs - testDistributingRoundRobinStartWithStuckNodeA (reproducer): 5/5 stable - ConnectionPoolReconnectsTest + ConnectionPoolHeartbeatTest: 5/5 on 2.11.8, 3/3 on 3.5.0 - IProtoClientPoolTest + ConnectionPoolTest (unit): 32/32
1 parent c527f09 commit ae504a3

1 file changed

Lines changed: 36 additions & 12 deletions

File tree

tarantool-pooling/src/main/java/io/tarantool/pool/PoolEntry.java

Lines changed: 36 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -345,15 +345,25 @@ public boolean isLocked() {
345345
}
346346

347347
/** Closes client and stops heartbeat and reconnect tasks if started. */
348-
public synchronized void close() {
348+
public void close() {
349349
stopReconnectTask();
350350
shutdown();
351351
}
352352

353-
/** Closes client and stops heartbeat task is started. */
354-
public synchronized void shutdown() {
355-
connectFuture = null;
356-
stopHeartbeat();
353+
/**
354+
* Closes the underlying client and stops the heartbeat task.
355+
*
356+
* <p>Performs field mutations under the entry monitor, then releases it before calling {@code
357+
* client.close()} (which acquires the {@code ConnectionImpl} monitor) and emitting the close
358+
* event. Holding the entry monitor across either of those calls would create an ABBA deadlock
359+
* with the Netty close-callback path, which takes the {@code ConnectionImpl} monitor first and
360+
* then re-enters {@link #handleConnectError(Object, Throwable)} on the entry.
361+
*/
362+
public void shutdown() {
363+
synchronized (this) {
364+
connectFuture = null;
365+
stopHeartbeat();
366+
}
357367
try {
358368
client.close();
359369
} catch (Exception e) {
@@ -422,6 +432,11 @@ public void stopHeartbeat() {
422432
/**
423433
* Internal method used by reconnect task and public connect.
424434
*
435+
* <p>{@code connectFuture} is assigned to the local chain <em>before</em> {@code whenComplete} is
436+
* attached, so that an inline failure completing before this method returns cannot have its
437+
* nulling of {@code connectFuture} in {@link #handleConnectError(Object, Throwable)} overwritten
438+
* by a stale failed future.
439+
*
425440
* @return {@link java.util.concurrent.CompletableFuture} with client
426441
*/
427442
private synchronized CompletableFuture<IProtoClient> internalConnect() {
@@ -433,7 +448,7 @@ private synchronized CompletableFuture<IProtoClient> internalConnect() {
433448
CompletableFuture<?> future =
434449
client.connect(group.getAddress(), connectTimeout, gracefulShutdown);
435450
String user = group.getUser();
436-
connectFuture =
451+
CompletableFuture<IProtoClient> cf =
437452
future
438453
.thenCompose(
439454
greeting -> {
@@ -443,8 +458,9 @@ private synchronized CompletableFuture<IProtoClient> internalConnect() {
443458
}
444459
return client.ping(firstPingOpts);
445460
})
446-
.thenApply(r -> client)
447-
.whenComplete(this::onConnectComplete);
461+
.thenApply(r -> client);
462+
connectFuture = cf;
463+
cf.whenComplete(this::onConnectComplete);
448464
return connectFuture;
449465
}
450466

@@ -477,19 +493,25 @@ private void onConnectComplete(Object r, Throwable exc) {
477493
/**
478494
* Handler for connection close.
479495
*
496+
* <p>Only the {@code connectFuture} reset is performed under the entry monitor. The listener
497+
* emit, {@link #lock()}, {@link #shutdown()} and {@link #connectAfter()} calls are made outside
498+
* the monitor; {@code shutdown()} will itself acquire {@code ConnectionImpl} via {@code
499+
* client.close()}, and the close-event path takes the same monitor — holding the entry monitor
500+
* across them would deadlock.
501+
*
480502
* @param r connection instance
481503
* @param exc exception which led to connection close
482504
*/
483505
private void handleConnectError(Object r, Throwable exc) {
484506
if (exc == null) {
485507
return;
486508
}
509+
Throwable failure = exc.getCause() != null ? exc.getCause() : exc;
487510
synchronized (this) {
488-
Throwable failure = exc.getCause() != null ? exc.getCause() : exc;
489511
connectFuture = null;
490-
log.error("connect error {}/{}: {}", tag, index, failure.toString());
491-
emit(listener -> listener.onConnectionFailed(tag, index, failure));
492512
}
513+
log.error("connect error {}/{}: {}", tag, index, failure.toString());
514+
emit(listener -> listener.onConnectionFailed(tag, index, failure));
493515
lock();
494516
shutdown();
495517
connectAfter();
@@ -500,15 +522,17 @@ private void connectAfter() {
500522
synchronized (this) {
501523
log.info("reconnect {}/{} after {} ms", tag, index, reconnectAfter);
502524
if (reconnectTask != null) {
525+
// existing task is being replaced; the existing increment in `reconnecting` carries over
526+
// to the new task, so no counter change is needed here.
503527
reconnectTask.cancel();
504528
} else {
505529
reconnecting.incrementAndGet();
506530
}
507531
reconnectTask =
508532
timerService.newTimeout(
509533
timeout -> internalConnect(), reconnectAfter, TimeUnit.MILLISECONDS);
510-
emit(listener -> listener.onReconnectScheduled(tag, index, reconnectAfter));
511534
}
535+
emit(listener -> listener.onReconnectScheduled(tag, index, reconnectAfter));
512536
}
513537

514538
/**

0 commit comments

Comments
 (0)