Skip to content

Commit 2e0719d

Browse files
committed
fix(qwp): I/O thread disposes its client on exit; a connect racing close() discards instead of installing (C5)
Root cause of the C5 leak: ownership hand-off between close() and an in-flight connect attempt was broken. close() swallows the InterruptedException from shutdownLatch.await() (routine under BackgroundDrainerPool.close()'s shutdownNow escalation, now that Invariant-B drainers retry outages indefinitely) and returns while the I/O thread is still inside a blocking native connect (connect_timeout defaults to 0 = OS timeout; neither unpark nor interrupt cancels connect(2)). Its client read saw the pre-connect field; a subsequently succeeding reconnect() then installed a live client via swapClient into the abandoned loop, and no code path ever closed it -- native socket, fds and buffers leaked for the life of the process. Fix, in two layers, both on the I/O thread (close() stays interrupt-responsive; the re-await alternative would block user close() calls for up to the OS connect timeout): 1. connectLoop: a client obtained after running flipped false is closed and discarded instead of installed. This also stops the routine post-teardown engine access via swapClient -> positionCursorAt -> Unsafe reads against segments the drainer's finally may have already unmapped (the SEGV half of C5). 2. ioLoop's finally: the I/O thread disposes whatever client it holds before counting down the shutdown latch -- the airtight backstop for running flipping false between connectLoop's check and swapClient completing. Duplicate closes (loop.close()'s own, owners' stale references) remain no-ops: WebSocketClient.close() is idempotent. Turns CursorWebSocketSendLoopInterruptedCloseLeakTest green. Regression net: sf/cursor (309), qwp/client (895), impl + http/client (146) -- all green. Residual (out of scope here): the drainer-side teardown-overlap window -- interrupt landing in the nanoseconds between the running check and swapClient while the engine unmaps -- needs the ioThreadStopped-style guard in BackgroundDrainer plus a close() that signals a failed stop; tracked as the SEGV half of the C5 review finding.
1 parent f6dfb32 commit 2e0719d

1 file changed

Lines changed: 45 additions & 3 deletions

File tree

core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java

Lines changed: 45 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -586,9 +586,11 @@ public synchronized void close() {
586586
// replaced the original (and closed it); the owner only retains
587587
// the stale pre-reconnect reference. Without closing the live
588588
// client here, its native socket and fds leak past sender.close()
589-
// every time the loop reconnected at least once. close() is
590-
// idempotent, so the owner's duplicate close on its stale
591-
// reference is still safe.
589+
// every time the loop reconnected at least once. ioLoop's finally
590+
// also closes the current client on I/O-thread exit, so this read
591+
// matters chiefly when the loop never started (SYNC construction,
592+
// close() before start()) — and doubles as a safety net. close()
593+
// is idempotent, so duplicate closes on any path are safe.
592594
WebSocketClient c = client;
593595
if (c != null) {
594596
try {
@@ -898,6 +900,27 @@ private void connectLoop(Throwable initial, String phase) {
898900
try {
899901
WebSocketClient newClient = reconnectFactory.reconnect();
900902
if (newClient != null) {
903+
if (!running) {
904+
// close() ran while this connect attempt was in
905+
// flight. Its latch await may have been interrupted
906+
// (BackgroundDrainerPool.close()'s shutdownNow path)
907+
// and returned already — the owner's teardown,
908+
// including the engine unmap in BackgroundDrainer's
909+
// finally, can be complete. Installing the client now
910+
// would (a) touch engine memory via positionCursorAt
911+
// after a possible unmap and (b) abandon a live socket
912+
// in a loop nothing will revisit — close() has run,
913+
// its client read saw the pre-connect field. The
914+
// attempt owns the client until it is installed, so
915+
// dispose of it here, on the I/O thread, and exit
916+
// through the quiet stopped path below.
917+
try {
918+
newClient.close();
919+
} catch (Throwable ignored) {
920+
// best-effort
921+
}
922+
break;
923+
}
901924
swapClient(newClient);
902925
totalReconnects.incrementAndGet();
903926
long elapsedMs = (System.nanoTime() - outageStartNanos) / 1_000_000L;
@@ -1189,6 +1212,25 @@ private void ioLoop() {
11891212
}
11901213
fail(t);
11911214
} finally {
1215+
// Last act of the I/O thread: dispose of whatever client it
1216+
// holds. This is the airtight half of the close()-vs-reconnect
1217+
// race — when close()'s latch await is interrupted (drainer pool
1218+
// shutdownNow), close() returns before this thread has exited,
1219+
// and its own client close saw the pre-reconnect field. A client
1220+
// swapped in by the tail of an in-flight connect attempt (running
1221+
// flipped false between connectLoop's check and swapClient) would
1222+
// be abandoned live without this. Runs BEFORE the latch countdown
1223+
// so a non-interrupted close() observes a fully disposed loop.
1224+
// Duplicate closes — loop.close()'s own, owners' stale references
1225+
// — stay safe: WebSocketClient.close() is idempotent.
1226+
WebSocketClient c = client;
1227+
if (c != null) {
1228+
try {
1229+
c.close();
1230+
} catch (Throwable ignored) {
1231+
// best-effort
1232+
}
1233+
}
11921234
shutdownLatch.countDown();
11931235
}
11941236
}

0 commit comments

Comments
 (0)