Skip to content

Commit 720d05f

Browse files
committed
fix(qwp): foreground-priority connect-walk lock; stop connect-phase drainers immediately on close (M11)
Three changes addressing M11 (close/reconnect latency under outage): 1. buildAndConnect no longer holds the sender monitor across network I/O. A dedicated ReentrantLock guards the connect walk (shared hostTracker round/health state, round counters, foreground lifecycle commits). Foreground walks use lock() and can only ever wait behind another foreground walk; background drainer walks use tryLock() and treat contention as a transport-shaped transient, deferring the sweep into their existing capped-backoff retry (Invariant B). The foreground's reconnect and close() paths can no longer queue behind a drainer's endpoint walk (previously up to endpoints x ~30s on an unfair monitor). 2. BackgroundDrainerPool.close() splits its stop policy: drainers that never started draining (still in connect-retry, ackedFsn < 0) are stop-signaled before the graceful window, exiting within ~50ms; only actively-draining drainers get the 2.5s grace. Cuts close() latency during an outage from ~3s to roughly one stop-check park chunk. 3. Documents the worst-case latency budgets: connect-walk lock hold formula on buildAndConnect, close() budget on QwpWebSocketSender, and a close-latency note on the drainOrphans builder javadoc.
1 parent 514e789 commit 720d05f

4 files changed

Lines changed: 149 additions & 15 deletions

File tree

core/src/main/java/io/questdb/client/Sender.java

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1840,6 +1840,16 @@ public LineSenderBuilder drainerListener(
18401840
* Slots flagged with the {@code .failed} sentinel are skipped
18411841
* (manual reset required), and the foreground sender's own slot is
18421842
* never adopted.
1843+
* <p>
1844+
* Close-latency note: {@code close()} stops adopted drainers. A
1845+
* drainer still connecting (e.g. during an outage) is stop-signaled
1846+
* immediately and exits within ~50ms; a drainer actively replaying
1847+
* frames is given a ~2.5s grace window to finish, plus a 0.5s stop
1848+
* window — so {@code close()} may take up to ~3s while orphan
1849+
* drainers are in flight (and a drainer parked in a blocking native
1850+
* connect is abandoned to exit on its own daemon thread).
1851+
* Un-drained slots stay on disk and are re-adopted by the next
1852+
* sender that enables {@code drain_orphans}.
18431853
*/
18441854
public LineSenderBuilder drainOrphans(boolean enabled) {
18451855
if (protocol != PARAMETER_NOT_SET_EXPLICITLY && protocol != PROTOCOL_WEBSOCKET) {

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

Lines changed: 91 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@
7373
import java.util.List;
7474
import java.util.concurrent.TimeUnit;
7575
import java.util.concurrent.atomic.AtomicReference;
76+
import java.util.concurrent.locks.ReentrantLock;
7677

7778
/**
7879
* QWP v1 WebSocket client sender for streaming data to QuestDB.
@@ -152,6 +153,16 @@ public class QwpWebSocketSender implements Sender {
152153
private final List<Endpoint> endpoints;
153154
// Global symbol dictionary for delta encoding
154155
private final GlobalSymbolDictionary globalSymbolDictionary;
156+
// Guards the connect walk (see buildAndConnect): the shared
157+
// QwpHostHealthTracker round/health state, roundSeq,
158+
// roundConnectAttemptSeq, and the foreground lifecycle commits
159+
// (currentEndpointIdx, hasEverConnected, cap-derived sizing). A
160+
// dedicated lock rather than the sender monitor so that (a) unrelated
161+
// synchronized sender methods (setDrainerListener, startOrphanDrainers)
162+
// never contend with a connect walk's network I/O, and (b) background
163+
// walks can use tryLock to yield to the foreground instead of queuing
164+
// on an unfair monitor.
165+
private final ReentrantLock connectWalkLock = new ReentrantLock();
155166
private final QwpHostHealthTracker hostTracker;
156167
private final CharSequenceObjHashMap<QwpTableBuffer> tableBuffers;
157168
// null means plain text (no TLS)
@@ -943,6 +954,30 @@ public QwpWebSocketSender charColumn(CharSequence columnName, char value) {
943954
return this;
944955
}
945956

957+
/**
958+
* Closes the sender: flushes user-thread state into the engine, drains
959+
* acked data within {@code close_flush_timeout}, stops the I/O loop,
960+
* closes the orphan-drainer pool, and frees buffers.
961+
* <p>
962+
* Worst-case latency budget (dominant contributors, sequential):
963+
* <ul>
964+
* <li>bounded drain: up to {@code close_flush_timeout} when the server
965+
* is slow or unreachable ({@code <= 0} opts out);</li>
966+
* <li>I/O loop stop: the shutdown-latch await is untimed, but the loop
967+
* exits promptly unless the I/O thread sits inside a blocking
968+
* native connect — bounded by {@code connect_timeout}, or by the
969+
* OS SYN-retry deadline (60-130s on Linux) when the default
970+
* {@code 0} is in effect. Background drainer walks never delay
971+
* this stop: they yield the connect-walk lock to the foreground
972+
* (see {@link #buildAndConnect});</li>
973+
* <li>drainer pool: drainers still in their connect-retry phase are
974+
* stop-signaled immediately (exit within ~50ms); drainers actively
975+
* replaying frames get a 2.5s grace window plus a 0.5s stop window
976+
* — worst case ~3s when a drainer sits in a blocking native
977+
* connect (15s background deadline) and must be abandoned to exit
978+
* on its own.</li>
979+
* </ul>
980+
*/
946981
@Override
947982
public void close() {
948983
if (!closed) {
@@ -2524,7 +2559,62 @@ private WebSocketClient newWebSocketClient() {
25242559
: WebSocketClientFactory.newPlainTextInstance();
25252560
}
25262561

2527-
private synchronized WebSocketClient buildAndConnect(ReconnectSupplier ctx) {
2562+
/**
2563+
* Multi-endpoint connect walk shared by the foreground sender and the
2564+
* background orphan drainers. One invocation walks every endpoint of the
2565+
* current round, performing a TCP/TLS connect plus a WebSocket upgrade
2566+
* per endpoint, all while holding {@link #connectWalkLock} (the lock
2567+
* protects the shared {@link QwpHostHealthTracker} round/health state,
2568+
* the round counters, and the foreground lifecycle commits).
2569+
* <p>
2570+
* Worst-case lock hold per walk is
2571+
* {@code endpoints x (connect timeout + upgrade timeout)}:
2572+
* <ul>
2573+
* <li>foreground walk: {@code connect_timeout} verbatim -- the default
2574+
* {@code 0} keeps the untimed native connect, bounded only by the
2575+
* OS SYN-retry deadline (60-130s per endpoint on Linux) -- plus
2576+
* {@code auth_timeout_ms} (default 15s) for the upgrade;</li>
2577+
* <li>background walk: 15s connect fallback
2578+
* ({@link #DEFAULT_BACKGROUND_CONNECT_TIMEOUT_MS}) plus
2579+
* {@code auth_timeout_ms} -- see
2580+
* {@link #effectiveConnectTimeoutMs(boolean, int)}.</li>
2581+
* </ul>
2582+
* <p>
2583+
* Lock policy -- the foreground has absolute priority. Foreground walks
2584+
* (the producer's initial connect and the I/O loop's reconnects) block
2585+
* on {@code lock()} and can therefore only ever wait behind another
2586+
* foreground walk. Background walks use {@code tryLock()} and treat
2587+
* contention as a transport-shaped transient: the thrown
2588+
* {@link LineSenderException} lands in the drainer's indefinite
2589+
* capped-backoff retry (Invariant B) -- both in
2590+
* {@code BackgroundDrainer.connectWithDurableAckRetry()} and in
2591+
* {@code CursorWebSocketSendLoop.connectLoop}'s Throwable branch -- so a
2592+
* drainer sweep is deferred, never failed, and the foreground's
2593+
* reconnect and {@code close()} paths are never queued behind a
2594+
* drainer's endpoint walk.
2595+
*/
2596+
private WebSocketClient buildAndConnect(ReconnectSupplier ctx) {
2597+
if (ctx.isBackground()) {
2598+
if (!connectWalkLock.tryLock()) {
2599+
// Transport-shaped on purpose: every drainer retry loop
2600+
// treats an untyped connect failure as transient and backs
2601+
// off (Invariant B) -- the sweep is deferred, never failed,
2602+
// and no .failed sentinel can result from lock contention.
2603+
throw new LineSenderException(
2604+
"connect walk lock is busy (another connect walk is in progress); "
2605+
+ "background drainer will retry after backoff");
2606+
}
2607+
} else {
2608+
connectWalkLock.lock();
2609+
}
2610+
try {
2611+
return connectWalk(ctx);
2612+
} finally {
2613+
connectWalkLock.unlock();
2614+
}
2615+
}
2616+
2617+
private WebSocketClient connectWalk(ReconnectSupplier ctx) {
25282618
// Background (drainer) factories share this connect walk -- endpoint
25292619
// list, hostTracker health and round state -- but must stay INVISIBLE
25302620
// in the foreground sender's observable state. SenderConnectionEvents

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

Lines changed: 42 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -46,10 +46,15 @@
4646
* (no orphans submitted) costs one core thread; submitted-and-finished
4747
* drainers are GC'd after they complete.
4848
* <p>
49-
* Closing the pool requests every still-running drainer to stop and
50-
* waits up to a few seconds for them to exit cleanly. Drainers that
51-
* don't exit in time are left to finish on their own — the pool's
52-
* underlying executor uses daemon threads so they don't block JVM exit.
49+
* Closing the pool uses a split stop policy: drainers that never started
50+
* draining (still inside their connect-retry loop — e.g. the cluster is
51+
* unreachable) are stop-signaled immediately, because no grace window can
52+
* help them finish; drainers actively replaying frames get a graceful
53+
* window to reach {@code acked >= target} before being signaled. Drainers
54+
* that don't exit in time (typically parked in a blocking native connect
55+
* that neither unpark nor interrupt cancels) are left to finish on their
56+
* own — the pool's underlying executor uses daemon threads so they don't
57+
* block JVM exit.
5358
*/
5459
public final class BackgroundDrainerPool implements QuietCloseable {
5560

@@ -66,9 +71,12 @@ public final class BackgroundDrainerPool implements QuietCloseable {
6671
// either lands before close (and close waits for it to finish) or
6772
// sees the closed bit and throws.
6873
private static final int CLOSED_BIT = Integer.MIN_VALUE;
69-
// Time we let drainers finish their drain naturally before signaling
70-
// stop. awaitTermination returns as soon as the last drainer exits,
71-
// so this only matters when something is genuinely stuck.
74+
// Time we let ACTIVELY DRAINING drainers finish naturally before
75+
// signaling stop. Connect-phase drainers are stop-signaled before this
76+
// window even starts (see close()), so during an outage — when no
77+
// drainer can be draining — close() does not pay this in full.
78+
// awaitTermination returns as soon as the last drainer exits, so this
79+
// only matters when something is genuinely stuck.
7280
private static final long GRACEFUL_DRAIN_MILLIS = 2_500L;
7381
private static final Logger LOG = LoggerFactory.getLogger(BackgroundDrainerPool.class);
7482
// After signaling stop, give drainers a brief window to unwind cleanly
@@ -125,11 +133,33 @@ public void close() {
125133
while (state.get() != CLOSED_BIT) {
126134
Compat.onSpinWait();
127135
}
128-
// Reject new tasks but let in-flight drainers finish their drain
129-
// naturally. Without this grace window a drainer that's seconds
130-
// away from acked >= target gets requestStop()'d and exits as
131-
// STOPPED — its engine.close() then sees fullyDrained=false and
132-
// leaves the slot's .sfa files behind, defeating drain_orphans.
136+
// Split stop policy. The graceful window below exists so a drainer
137+
// that is seconds away from acked >= target is not cut down
138+
// mid-drain (its engine.close() would see fullyDrained=false and
139+
// leave the slot's .sfa files behind, defeating drain_orphans). A
140+
// drainer that never started draining — still inside its
141+
// connect-retry loop, e.g. the cluster is unreachable and
142+
// Invariant B retries forever — cannot possibly use that window
143+
// productively, so stop it NOW: it wakes from its backoff park
144+
// within ~50ms (STOP_CHECK_PARK_CHUNK_NANOS) and exits as STOPPED,
145+
// cutting close() latency during an outage from
146+
// GRACEFUL_DRAIN_MILLIS + STOP_GRACE_MILLIS (~3s) to roughly one
147+
// stop-check park chunk. ackedFsn stays -1 until the drain loop's
148+
// first poll, so `< 0` discriminates "never connected/started
149+
// draining" from "actively draining"; the moments-wide race with a
150+
// just-connected drainer is benign — it exits as STOPPED and the
151+
// slot is re-adopted by the next scan. A drainer parked inside a
152+
// blocking native connect ignores the stop until its background
153+
// connect deadline resolves; that one still burns the full grace +
154+
// stop windows below and is then abandoned to exit on its own
155+
// (daemon thread).
156+
for (BackgroundDrainer d : active) {
157+
if (d.outcome() == BackgroundDrainer.DrainOutcome.PENDING && d.getAckedFsn() < 0) {
158+
d.requestStop();
159+
}
160+
}
161+
// Reject new tasks but let actively-draining drainers finish
162+
// naturally.
133163
executor.shutdown();
134164
try {
135165
if (!executor.awaitTermination(GRACEFUL_DRAIN_MILLIS, TimeUnit.MILLISECONDS)) {

core/src/test/java/io/questdb/client/test/cutlass/qwp/client/QwpWebSocketSenderJvmErrorCleanupTest.java

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -151,11 +151,15 @@ public void testExceptionPathStillClosesAndWalksAllEndpoints() throws Exception
151151
* Bypasses the real constructor -- no wire client, engine or dispatcher
152152
* needed. The connect walk dereferences only the fields wired below plus
153153
* primitives whose zero-defaults are valid here (field initializers do
154-
* not run under {@code Unsafe.allocateInstance}).
154+
* not run under {@code Unsafe.allocateInstance}), plus the connect-walk
155+
* lock, which buildAndConnect acquires unconditionally and is therefore
156+
* wired here.
155157
*/
156158
private static QwpWebSocketSender newBareSender() throws Exception {
157-
return (QwpWebSocketSender) Unsafe.getUnsafe()
159+
QwpWebSocketSender sender = (QwpWebSocketSender) Unsafe.getUnsafe()
158160
.allocateInstance(QwpWebSocketSender.class);
161+
setField(sender, "connectWalkLock", new java.util.concurrent.locks.ReentrantLock());
162+
return sender;
159163
}
160164

161165
private static QwpHostHealthTracker wireEndpoints(QwpWebSocketSender sender,

0 commit comments

Comments
 (0)