Skip to content

Commit dd0c547

Browse files
committed
docs(qwp): fix load-bearing concurrency comments that contradict the code
Five doc sites documented behavior the code deliberately does not have; each invited a future 'fix' that would reintroduce the hazard the quiescence work eliminated. Comment-only change. - CursorSendEngine.closeCompleted field doc: carve the failed-flock- release case out of the retry sentence. A retried close() exits at the consumed terminalCleanupClaimed CAS and never calls SlotLock.release() again — deliberate, pinned by testUnconfirmedFlockReleaseKeepsCloseIncomplete. - CursorSendEngine.finishClose javadoc: 'must hold the engine monitor' was false for completeDeferredClose (deliberately monitor-free to avoid the join livelock). State the real contract: monitor (close path) OR worker exit path; in all cases the CAS must be won. - CursorSendEngine.isCloseCompleted javadoc: admit the third, unrecoverable state — failed flock release never flips; only process exit frees the flock. - SegmentManager.isWorkerReaped javadoc: the fall-through reap nulls workerThread while the thread may still be running deferred engine cleanups; the engine-side CAS, not this predicate, is the exclusion. - SegmentManager.serviceRing0 trim comment: narrow 'stale snapshots' to mid-pass-deregistered entries — the claim gate makes the trim block unreachable for pre-pass-deregistered entries. - SenderPool reclaimSlot/retireLease: 'retired permanently' contradicted retiredSlots.add + reprobeRetiredSlots recovery three lines down. - QwpWebSocketSender post-guard comment: the incomplete-close branch is not only the owned-manager handoff; document the no-handoff cases (shared manager, failed handoff registration, failed flock release) where the re-probe never flips.
1 parent 79e0847 commit dd0c547

4 files changed

Lines changed: 61 additions & 25 deletions

File tree

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

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1272,13 +1272,21 @@ public void close() {
12721272
ownsCursorEngine = false;
12731273
slotLockReleased = true;
12741274
} else {
1275-
// The manager worker did not quiesce. Preserve ownership
1276-
// and report the retained flock so pools retire this slot.
1277-
// Repeated Sender.close() calls remain no-ops by contract.
1278-
// Engine cleanup was handed to the worker's exit path
1279-
// (owned manager); the getter re-probes the retained
1280-
// engine so the pool can reclaim the slot once cleanup
1281-
// actually completes.
1275+
// Engine close() could not confirm a released flock.
1276+
// Preserve ownership and report the retained flock so
1277+
// pools retire this slot. Repeated Sender.close() calls
1278+
// remain no-ops by contract. Recovery depends on WHY the
1279+
// close is incomplete: when cleanup was handed to the
1280+
// worker's exit path (owned manager — the only shipping
1281+
// configuration), isSlotLockReleased()'s re-probe of the
1282+
// retained engine flips once that cleanup completes.
1283+
// When there was no handoff, the re-probe never flips
1284+
// and the slot stays retired until process exit: a
1285+
// shared-manager engine or a failed handoff registration
1286+
// needs a retried engine.close() that this one-shot
1287+
// sender never issues, and a failed flock release has
1288+
// consumed the engine's cleanup claim, so nothing can
1289+
// re-run the release.
12821290
slotLockReleased = false;
12831291
retainedEngine = engine;
12841292
}

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

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -124,9 +124,16 @@ public final class CursorSendEngine implements QuietCloseable {
124124
// SenderPool.reprobeRetiredSlots), so publishing before the release
125125
// would let a replacement engine's SlotLock.acquire collide with the
126126
// still-open fd. Stays false when a close attempt could not confirm
127-
// manager-worker quiescence (or the flock release itself failed) and had
128-
// to leak the ring/watermark/slot lock — in that case a later close()
129-
// call retries the cleanup (the worker may have exited by then).
127+
// manager-worker quiescence and had to leak the ring/watermark/slot
128+
// lock — in that case a later close() call retries the cleanup (the
129+
// worker may have exited by then). Also stays false — permanently —
130+
// when finishClose() ran but the flock release itself failed: the
131+
// terminalCleanupClaimed CAS is consumed, so a retried close() exits at
132+
// the CAS without re-running the cleanup or calling SlotLock.release()
133+
// again. That non-retry is deliberate (a retry would double-close the
134+
// ring/watermark and double-release the flock; pinned by
135+
// testUnconfirmedFlockReleaseKeepsCloseIncomplete) — the kernel drops
136+
// the flock at process exit.
130137
// volatile: latched by finishClose(), but read lock-free by
131138
// isCloseCompleted() from pool threads re-probing a retired slot (see the
132139
// getter for why it must not synchronize).
@@ -674,7 +681,10 @@ public synchronized void close() {
674681
* {@code closeCompleted} (via {@code isSlotLockReleased()}), so it must
675682
* never be visible while the flock fd is still open, or a replacement
676683
* engine races the release and fails acquisition on a live slot.
677-
* The caller must hold the engine monitor and
684+
* The caller must either hold the engine monitor (the close() path) or
685+
* run on the manager worker's exit path ({@link #completeDeferredClose},
686+
* which deliberately takes no monitor — adding one would recreate the
687+
* join livelock the CAS exists to avoid); in all cases the caller
678688
* must have established that the manager worker can no longer touch the
679689
* slot directory (reaped, provably exited, or running this on its own
680690
* exit path) AND have won the {@link #terminalCleanupClaimed} CAS — the
@@ -796,11 +806,14 @@ private void completeDeferredClose(boolean fullyDrained) {
796806
* <b>confirmed</b> release of the SF slot lock — the flip is published
797807
* strictly after the flock fd is closed, so observing {@code true}
798808
* guarantees the slot dir is acquirable by a replacement engine. A false
799-
* value after close means manager-worker quiescence could not be
800-
* confirmed (or the flock release itself failed) and the
801-
* worker-reachable resources were retained deliberately — either handed to the worker's exit path (owned manager),
802-
* which flips this to true the moment the worker's in-flight pass
803-
* finishes, or leaked until a retried close() (shared manager). Owners
809+
* value after close means the worker-reachable resources were retained
810+
* deliberately, for one of three reasons: cleanup was handed to the
811+
* worker's exit path (owned manager), which flips this to true the
812+
* moment the worker's in-flight pass finishes; cleanup was leaked until
813+
* a retried close() (shared manager, or a failed handoff registration);
814+
* or the flock release itself failed — that last case never flips: the
815+
* {@link #terminalCleanupClaimed} CAS is consumed, so no retry re-runs
816+
* the release, and only process exit frees the flock. Owners
804817
* must not reuse the slot while this is false; pools may re-probe it to
805818
* recover a retired slot's capacity once it flips.
806819
* <p>

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

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -376,9 +376,16 @@ public boolean awaitRingQuiescence(SegmentRing ring) {
376376
}
377377

378378
/**
379-
* True when no manager worker thread can be running: either
380-
* {@link #start()} was never called, or a {@link #close()} confirmed
381-
* worker termination and reaped the thread. Owners use this as a
379+
* True when the manager worker can no longer run a service pass: either
380+
* {@link #start()} was never called, or a {@link #close()} reaped the
381+
* thread — including the fall-through reap that observes
382+
* {@code workerLoopExited} while the thread is still alive. In that case
383+
* the worker may still be RUNNING its deferred engine cleanups
384+
* (see {@link #deferUntilWorkerExit}: munmap/unlink/flock-release on the
385+
* exit path) when this flips true, so this predicate is NOT the
386+
* exclusion between a retried engine close() and the worker's deferred
387+
* cleanup — the engine-side {@code terminalCleanupClaimed} CAS is.
388+
* Owners use this as a
382389
* stronger fallback barrier when {@link #awaitRingQuiescence(SegmentRing)}
383390
* times out but a subsequent {@code close()} join succeeded.
384391
*/
@@ -742,7 +749,10 @@ private void serviceRing0(RingEntry e) {
742749
// The watermark write and totalBytes commit are registration-gated
743750
// under `lock` so stale worker snapshots cannot touch the
744751
// engine-owned watermark or mutate accounting after deregister()
745-
// returns. drainTrimmable still runs for stale snapshots: it
752+
// returns. drainTrimmable still runs for entries deregistered
753+
// MID-pass (entries deregistered before the pass started never
754+
// get here — the registration/in-service claim at the top of
755+
// serviceRing skips them entirely): it
746756
// transfers ownership of fully-acked sealed segments to this
747757
// worker, preserving the old close + unlink behavior.
748758
//

core/src/main/java/io/questdb/client/impl/SenderPool.java

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1062,7 +1062,9 @@ private void retireLease(PooledSender ps, String context) {
10621062
pendingLeaseTeardowns--;
10631063
if (reserved) {
10641064
// Free the index only when the flock was released; a slot
1065-
// left locked is retired permanently.
1065+
// left locked is retired into retiredSlots, recoverable
1066+
// by reprobeRetiredSlots() if the deferred cleanup drops
1067+
// the flock later.
10661068
reclaimSlot(s, context);
10671069
}
10681070
slotReleased.signalAll();
@@ -1350,10 +1352,13 @@ private static boolean flockReleased(SenderSlot s) {
13501352
* Reclaims one SF slot after its delegate's {@code close()} has been
13511353
* attempted. When the flock was released the index returns to the free
13521354
* set; when {@code close()} returned with the flock still held because an
1353-
* I/O or manager worker did not stop, the slot is retired permanently --
1354-
* {@code leakedSlots++} and {@code slotInUse[idx]} stays set -- so the cap
1355-
* math accounts for the lost capacity and no later borrow ever reuses the
1356-
* still-locked dir. Either way {@code closingSlots} is decremented.
1355+
* I/O or manager worker did not stop, the slot is retired --
1356+
* {@code leakedSlots++}, {@code slotInUse[idx]} stays set, and the sender
1357+
* joins {@code retiredSlots} -- so the cap math accounts for the lost
1358+
* capacity and no borrow reuses the still-locked dir unless
1359+
* {@link #reprobeRetiredSlots} later observes the deferred cleanup's
1360+
* release and recovers the index. Either way {@code closingSlots} is
1361+
* decremented.
13571362
* <p>
13581363
* Caller must hold {@code lock} and is responsible for signalling waiters
13591364
* (only the free path admits a new creation). Shared by

0 commit comments

Comments
 (0)