Skip to content

Commit 1347025

Browse files
dkasimovskiyclaude
andcommitted
refactor(pooling): make connectFuture and reconnectTask atomic in PoolEntry
Replace the entry-monitor `synchronized` blocks that guarded `connectFuture` and `reconnectTask` with `AtomicReference` and CAS/`getAndSet` operations. The four `synchronized (this)` blocks in `internalConnect()`, `shutdown()`, `handleConnectError()`, and `connectAfter()` collapse to atomic field operations; the `synchronized` keyword on `stopReconnectTask()` is dropped. Behaviour preserved: * `internalConnect()` fast-path check, then `compareAndSet(null, cf)` — the loser's wasted build matches the previous double-checked-locking race; the loser's `cf.whenComplete` is not registered, same as before. * `connectAfter()` schedules the new task outside the lock and uses `getAndSet` to atomically swap it in, incrementing `reconnecting` only on the first install and cancelling the displaced task otherwise. * `stopReconnectTask()` and the `connectFuture.set(null)` sites become lock-free. ABBA deadlock reasoning in `shutdown()` JavaDoc is reframed: with no entry monitor to release, the field mutations are atomic/volatile and `client.close()` is just a method call that the Netty close-callback path is free to interleave with. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 3e37768 commit 1347025

1 file changed

Lines changed: 37 additions & 41 deletions

File tree

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

Lines changed: 37 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import java.util.concurrent.TimeUnit;
1111
import java.util.concurrent.atomic.AtomicBoolean;
1212
import java.util.concurrent.atomic.AtomicInteger;
13+
import java.util.concurrent.atomic.AtomicReference;
1314
import java.util.function.BiFunction;
1415
import java.util.function.Consumer;
1516

@@ -144,7 +145,8 @@ final class PoolEntry {
144145
* <p>It will be returned to all out clients wanting to obtain this client. It is recreated only
145146
* if client is reconnected.
146147
*/
147-
private CompletableFuture<IProtoClient> connectFuture;
148+
private final AtomicReference<CompletableFuture<IProtoClient>> connectFuture =
149+
new AtomicReference<>();
148150

149151
/** Last heartbeat state/event. */
150152
private volatile HeartbeatEvent lastHeartbeatEvent;
@@ -153,7 +155,7 @@ final class PoolEntry {
153155
private volatile Timeout heartbeatTask;
154156

155157
/** Reconnection task. */
156-
private Timeout reconnectTask;
158+
private final AtomicReference<Timeout> reconnectTask = new AtomicReference<>();
157159

158160
/** Flag signaling if heartbeat started or not. */
159161
private volatile boolean isHeartbeatStarted;
@@ -385,9 +387,9 @@ public void close() {
385387
/**
386388
* Closes the underlying client and stops the heartbeat task.
387389
*
388-
* <p>Performs field mutations under the entry monitor, then releases it before calling {@code
389-
* client.close()} (which acquires the {@code ConnectionImpl} monitor) and emitting the close
390-
* event. Holding the entry monitor across either of those calls would create an ABBA deadlock
390+
* <p>Field mutations on {@link #connectFuture} and the heartbeat task are atomic/volatile, so
391+
* {@code client.close()} (which acquires the {@code ConnectionImpl} monitor) and the close-event
392+
* emit run without holding the entry monitor — keeping the call lock-free avoids an ABBA deadlock
391393
* with the Netty close-callback path, which takes the {@code ConnectionImpl} monitor first and
392394
* then re-enters {@link #handleConnectError(Object, Throwable)} on the entry.
393395
*
@@ -410,10 +412,8 @@ public void close() {
410412
* method for the same entry.
411413
*/
412414
public void shutdown() {
413-
synchronized (this) {
414-
connectFuture = null;
415-
stopHeartbeat();
416-
}
415+
connectFuture.set(null);
416+
stopHeartbeat();
417417
try {
418418
client.close();
419419
} catch (Exception e) {
@@ -479,16 +479,18 @@ public void stopHeartbeat() {
479479
/**
480480
* Internal method used by reconnect task and public connect.
481481
*
482-
* <p>See {@link #shutdown()} for the monitor-ordering reasoning; {@code client.connect()} runs
483-
* outside the entry monitor for the same reason.
482+
* <p>{@code client.connect()} runs outside any entry lock for the same reason as in {@link
483+
* #shutdown()} — see there for the monitor-ordering reasoning. The {@link
484+
* AtomicReference#compareAndSet} on {@link #connectFuture} atomically installs the freshly built
485+
* future or returns the winner's future to the loser, so only one {@code
486+
* whenComplete(onConnectComplete)} is registered per in-flight connect.
484487
*
485488
* @return {@link java.util.concurrent.CompletableFuture} with client
486489
*/
487490
private CompletableFuture<IProtoClient> internalConnect() {
488-
synchronized (this) {
489-
if (connectFuture != null) {
490-
return connectFuture;
491-
}
491+
CompletableFuture<IProtoClient> existing = connectFuture.get();
492+
if (existing != null) {
493+
return existing;
492494
}
493495
log.info("connect {}/{}", tag, index);
494496
LongTaskTimer.Sample timer = startTimer(connectTime);
@@ -506,15 +508,12 @@ private CompletableFuture<IProtoClient> internalConnect() {
506508
return client.ping(firstPingOpts);
507509
})
508510
.thenApply(r -> client);
509-
synchronized (this) {
510-
if (connectFuture != null) {
511-
return connectFuture;
512-
}
513-
connectFuture = cf;
511+
if (connectFuture.compareAndSet(null, cf)) {
514512
isShutdown.set(false);
513+
cf.whenComplete(this::onConnectComplete);
514+
return cf;
515515
}
516-
cf.whenComplete(this::onConnectComplete);
517-
return cf;
516+
return connectFuture.get();
518517
}
519518

520519
/**
@@ -554,9 +553,7 @@ private void handleConnectError(Object r, Throwable exc) {
554553
return;
555554
}
556555
Throwable failure = exc.getCause() != null ? exc.getCause() : exc;
557-
synchronized (this) {
558-
connectFuture = null;
559-
}
556+
connectFuture.set(null);
560557
log.error("connect error {}/{}: {}", tag, index, failure.toString());
561558
emit(listener -> listener.onConnectionFailed(tag, index, failure));
562559
lock();
@@ -566,18 +563,17 @@ private void handleConnectError(Object r, Throwable exc) {
566563

567564
/** Reconnect task scheduler. */
568565
private void connectAfter() {
569-
synchronized (this) {
570-
log.info("reconnect {}/{} after {} ms", tag, index, reconnectAfter);
571-
if (reconnectTask != null) {
572-
// existing task is being replaced; the existing increment in `reconnecting` carries over
573-
// to the new task, so no counter change is needed here.
574-
reconnectTask.cancel();
575-
} else {
576-
reconnecting.incrementAndGet();
577-
}
578-
reconnectTask =
579-
timerService.newTimeout(
580-
timeout -> internalConnect(), reconnectAfter, TimeUnit.MILLISECONDS);
566+
log.info("reconnect {}/{} after {} ms", tag, index, reconnectAfter);
567+
Timeout newTask =
568+
timerService.newTimeout(
569+
timeout -> internalConnect(), reconnectAfter, TimeUnit.MILLISECONDS);
570+
Timeout old = reconnectTask.getAndSet(newTask);
571+
if (old == null) {
572+
reconnecting.incrementAndGet();
573+
} else {
574+
// existing task is being replaced; the existing increment in `reconnecting` carries over
575+
// to the new task, so no counter change is needed here.
576+
old.cancel();
581577
}
582578
emit(listener -> listener.onReconnectScheduled(tag, index, reconnectAfter));
583579
}
@@ -750,11 +746,11 @@ private void incHeartbeatCounters(int fail) {
750746
}
751747

752748
/** Stops reconnecting task if it is active. */
753-
private synchronized void stopReconnectTask() {
754-
if (reconnectTask != null) {
749+
private void stopReconnectTask() {
750+
Timeout existing = reconnectTask.getAndSet(null);
751+
if (existing != null) {
755752
reconnecting.decrementAndGet();
756-
reconnectTask.cancel();
757-
reconnectTask = null;
753+
existing.cancel();
758754
}
759755
}
760756

0 commit comments

Comments
 (0)