Skip to content

Commit 4340371

Browse files
committed
fix(qwp): failed-stop teardown protocol -- never free the engine or client under a live I/O thread (C5 SEGV)
Completes the C5 fix. The landed discard-when-stopped commit removed the routine post-teardown engine access; this eliminates the SEGV class on the close path by construction: every native free is either ordered after the I/O thread's last access via the shutdown latch, or performed by the I/O thread itself. 1. CursorWebSocketSendLoop.close(): an interrupted latch await with the thread still live (latch count nonzero) now re-asserts the interrupt and THROWS instead of falling through -- falling through both closed the client's native buffers under a possibly mid-send thread and let owners unmap the engine under it. A concurrent latch-zero race is detected and proceeds as a normal stop. ioThread stays set on the throw, so a duplicate close() re-signals rather than silently succeeding. 2. delegateEngineClose() + delegated close in ioLoop's finally: on a failed stop the owner hands engine.close() to the I/O thread's exit path, which runs it strictly after the thread's last engine access and after the latch countdown. Store/load handshake (flag write then latch read vs countdown then flag read) guarantees no missed close under volatile SC; engine.close() is synchronized and idempotent, so a double close from both sides is benign. Unlike a deliberate leak, this releases the mapping and slot lock as soon as the stuck wire call resolves (bounded by OS timeouts) -- deferred, never abandoned. 3. BackgroundDrainer.run() finally: on a failed loop stop, skip the client close (the thread may be mid-send on it; its exit path closes the loop's current client) and delegate the engine close, falling back to closing locally when the thread exited in the interim. The mid-drain capability-gap recycle bails out on a failed stop instead of opening a new wire session against an engine the old thread still uses; the finally re-signals and applies the same protocol. 4. QwpWebSocketSender.close(): the ioThreadStopped guard -- now actually armed by close()'s throw, which the swallow previously made unreachable -- keeps leaking the microbatch buffers and client deliberately, but delegates the owned engine close instead of leaking the mapping and slot flock forever. slotLockReleased stays false: the pool must not reuse the slot until the delegated close has actually run. Turns BackgroundDrainerInterruptedTeardownTest green; the C5 leak test and close tests stay green. Regression net: sf/cursor + qwp/client + impl + http/client (1249) and the sf integration package (incl. BackgroundDrainerEndToEndTest, OrphanScanIntegrationTest) -- all green. Residual analysis: with the connect racing close(), a client obtained after running flips false is discarded on the I/O thread; a swapClient already in flight completes against an engine that is now guaranteed still mapped (owners cannot unmap while the latch is up and the thread is live). No close-path free can race the I/O thread anymore.
1 parent c7419f4 commit 4340371

3 files changed

Lines changed: 143 additions & 8 deletions

File tree

core/src/main/java/io/questdb/client/cutlass/qwp/client/QwpWebSocketSender.java

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1062,6 +1062,27 @@ public void close() {
10621062
// The I/O thread may still be using the socket and microbatch
10631063
// buffers. Freeing them would risk SIGSEGV.
10641064
LOG.error("I/O thread is still running, leaking WebSocket client and microbatch buffers");
1065+
// The engine, however, need not leak: delegate its close to
1066+
// the I/O thread's exit path, which runs it strictly after
1067+
// the thread's last engine access — the mapping and slot
1068+
// lock release as soon as the stuck wire call resolves
1069+
// (bounded by OS timeouts). slotLockReleased intentionally
1070+
// stays false: the lock is released only when the delegated
1071+
// close actually runs, so the pool must not reuse the slot
1072+
// meanwhile. A false return means the thread exited between
1073+
// the failed close() and now — then closing here is safe.
1074+
if (ownsCursorEngine && cursorEngine != null && cursorSendLoop != null
1075+
&& !cursorSendLoop.delegateEngineClose()) {
1076+
try {
1077+
cursorEngine.close();
1078+
} catch (Throwable t) {
1079+
LOG.error("Error closing owned CursorSendEngine: {}", String.valueOf(t));
1080+
terminalError = captureCloseError(terminalError, t);
1081+
}
1082+
cursorEngine = null;
1083+
ownsCursorEngine = false;
1084+
slotLockReleased = true;
1085+
}
10651086
rethrowTerminal(terminalError);
10661087
return;
10671088
}

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

Lines changed: 49 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -534,7 +534,21 @@ public void run() {
534534
slotPath, t.getMessage());
535535
try {
536536
loop.close();
537-
} catch (Throwable ignored) {
537+
} catch (Throwable closeFailure) {
538+
// Interrupted shutdown mid-recycle (pool
539+
// shutdownNow): the old I/O thread is still
540+
// alive, so opening a new wire session against
541+
// the same engine would race its exit — and
542+
// closing the client under a possibly mid-send
543+
// thread risks SEGV. Bail out; the finally
544+
// re-runs loop.close(), which re-signals the
545+
// failed stop and routes client/engine
546+
// teardown to the delegation protocol there.
547+
LOG.warn("drainer slot {}: stop requested mid-recycle and the "
548+
+ "I/O thread did not stop ({}); abandoning recycle",
549+
slotPath, closeFailure.getMessage());
550+
outcome = stopRequested ? DrainOutcome.STOPPED : DrainOutcome.FAILED;
551+
return;
538552
}
539553
try {
540554
client.close();
@@ -584,23 +598,51 @@ public void run() {
584598
}
585599
outcome = DrainOutcome.FAILED;
586600
} finally {
601+
boolean ioThreadStopped = true;
587602
if (loop != null) {
588603
try {
589604
loop.close();
590-
} catch (Throwable ignored) {
605+
} catch (Throwable e) {
606+
// The loop's I/O thread would not stop — close() was
607+
// interrupted (the pool's shutdownNow path) while the
608+
// thread sat in a blocking native connect/send that
609+
// neither unpark nor interrupt cancels. Freeing the
610+
// client's buffers or unmapping the engine now would
611+
// race the live thread (C5 SEGV); both are delegated to
612+
// the thread's own exit path below.
613+
ioThreadStopped = false;
614+
LOG.warn("drainer slot {}: I/O thread did not stop during close ({}); "
615+
+ "delegating client/engine teardown to its exit path",
616+
slotPath, e.getMessage());
591617
}
592618
}
593-
if (client != null) {
619+
if (client != null && ioThreadStopped) {
620+
// Skipped on a failed stop: the thread may be mid-send on
621+
// this very client; ioLoop's finally closes the loop's
622+
// current client (this one, unless a reconnect swapped it —
623+
// in which case swapClient already closed this reference).
594624
try {
595625
client.close();
596626
} catch (Throwable ignored) {
597627
}
598628
}
599629
if (engine != null) {
600-
try {
601-
// engine.close() releases the slot lock too.
602-
engine.close();
603-
} catch (Throwable ignored) {
630+
// Failed-stop hand-off: delegateEngineClose() makes the I/O
631+
// thread run engine.close() strictly after its last engine
632+
// access, releasing the slot lock as soon as the stuck wire
633+
// call resolves — deferred teardown, never abandoned. The
634+
// false return covers the race where the thread exited
635+
// between the failed close() and now: then it is safe (and
636+
// necessary) to close the engine here.
637+
if (ioThreadStopped || !loop.delegateEngineClose()) {
638+
try {
639+
// engine.close() releases the slot lock too.
640+
engine.close();
641+
} catch (Throwable ignored) {
642+
}
643+
} else {
644+
LOG.warn("drainer slot {}: engine close delegated to the I/O thread; "
645+
+ "slot lock releases when it exits", slotPath);
604646
}
605647
}
606648
// Don't let a later requestStop() unpark an unrelated task that

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

Lines changed: 73 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,12 @@ public final class CursorWebSocketSendLoop implements QuietCloseable {
217217
// terminalError: the only writer runs on the I/O thread under the same
218218
// first-writer-wins latch.
219219
private volatile QwpDurableAckMismatchException capabilityGapTerminal;
220+
// Failed-stop hand-off flag: set by delegateEngineClose() when an owner's
221+
// close() could not stop the I/O thread and the engine close is therefore
222+
// performed by the I/O thread's exit path. Write-once, owner thread only;
223+
// read by the I/O thread strictly after its shutdown-latch countdown (see
224+
// the handshake contract on delegateEngineClose).
225+
private volatile boolean engineCloseDelegated;
220226
// The latched terminal failure — THE exception every checkError() call
221227
// rethrows. Write-once for the loop's lifetime: the only writer is
222228
// recordFatal on the I/O thread (first-writer-wins). The whole
@@ -576,8 +582,31 @@ public synchronized void close() {
576582
if (t.isAlive()) {
577583
try {
578584
shutdownLatch.await();
579-
} catch (InterruptedException ignored) {
585+
} catch (InterruptedException e) {
586+
// Re-assert the flag for the caller's stack, then decide.
587+
// If the I/O thread has genuinely not exited (latch still
588+
// up — it may be inside a blocking native connect/send
589+
// that neither unpark nor interrupt cancels), touching the
590+
// client here would free native buffers under a possibly
591+
// mid-send thread, and returning quietly would let the
592+
// owner unmap the engine under it (C5 SEGV). Signal the
593+
// failed stop loudly instead: QwpWebSocketSender.close()
594+
// keys its ioThreadStopped guard on this throw, and
595+
// BackgroundDrainer switches to delegateEngineClose().
596+
// The I/O thread's own exit path (ioLoop's finally)
597+
// disposes of the client either way. ioThread stays set,
598+
// so a duplicate close() re-signals rather than silently
599+
// succeeding against a still-live thread.
580600
Thread.currentThread().interrupt();
601+
if (shutdownLatch.getCount() != 0L) {
602+
throw new LineSenderException(
603+
"cursor I/O thread did not stop: close() was interrupted "
604+
+ "while awaiting shutdown; client/engine teardown "
605+
+ "is delegated to the I/O thread's exit path");
606+
}
607+
// Latch hit zero concurrently with the interrupt: the
608+
// thread is past its last client/engine access — proceed
609+
// with normal teardown.
581610
}
582611
}
583612
ioThread = null;
@@ -602,6 +631,34 @@ public synchronized void close() {
602631
}
603632
}
604633

634+
/**
635+
* Failed-stop hand-off for the engine. Called by an owner whose
636+
* {@link #close()} threw because the I/O thread would not stop: the owner
637+
* must not free the engine (munmap/Unsafe.free of segment memory) while
638+
* the thread may still touch it with raw {@code Unsafe} reads. Setting
639+
* the delegation flag makes the I/O thread run {@code engine.close()} on
640+
* its exit path, strictly after its last engine access and after the
641+
* shutdown-latch countdown — releasing the slot lock as soon as the
642+
* stuck wire call resolves (bounded by OS timeouts) instead of leaking
643+
* the mapping and lock forever.
644+
* <p>
645+
* Returns {@code true} when the I/O thread is still live and has adopted
646+
* the engine close; {@code false} when the thread has already exited —
647+
* the caller must close the engine itself.
648+
* <p>
649+
* Memory model — the classic store/load handshake: this method writes the
650+
* volatile flag, then reads the latch count; the exit path counts the
651+
* latch down, then reads the flag. Under the sequential consistency of
652+
* volatile (and AQS latch state) accesses, if this method observes the
653+
* latch still up, the exit path is guaranteed to observe the flag — no
654+
* missed close. If both sides act, {@link CursorSendEngine#close()} is
655+
* synchronized and idempotent, so the double close is benign.
656+
*/
657+
public boolean delegateEngineClose() {
658+
engineCloseDelegated = true;
659+
return shutdownLatch.getCount() != 0L;
660+
}
661+
605662
/**
606663
* Typed server-rejection payload of the latched terminal error, or
607664
* {@code null} when the loop latched a wire-level failure (or nothing).
@@ -1232,6 +1289,21 @@ private void ioLoop() {
12321289
}
12331290
}
12341291
shutdownLatch.countDown();
1292+
// Failed-stop hand-off (see delegateEngineClose): the owner could
1293+
// not free the engine safely while this thread was alive, so the
1294+
// engine close — and with it the slot-lock release — happens
1295+
// here, strictly after this thread's last engine access. The flag
1296+
// is read only after the countDown: the store/load pairing with
1297+
// delegateEngineClose's flag-write-then-latch-read guarantees
1298+
// either this branch or the owner's fallback runs (or both —
1299+
// engine.close() is idempotent).
1300+
if (engineCloseDelegated) {
1301+
try {
1302+
engine.close();
1303+
} catch (Throwable ignored) {
1304+
// best-effort
1305+
}
1306+
}
12351307
}
12361308
}
12371309

0 commit comments

Comments
 (0)