Skip to content

Commit 21fe8cb

Browse files
committed
fix(qwp): interrupted drainer-pool close leaves drainers busy-spinning at 100% CPU
BackgroundDrainerPool.close()'s InterruptedException arm called only executor.shutdownNow() -- never requestStop() on the active drainers. The pre-shutdown split-stop sweep deliberately spares actively-draining drainers (ackedFsn >= 0), and the graceful-timeout sweep never runs when awaitTermination throws instead of returning false, so on this path an actively-draining drainer kept stopRequested == false while shutdownNow's interrupt turned every LockSupport.parkNanos in its poll/backoff loops into an immediate return (park does not clear the status): a 100% CPU busy-spin for as long as the outage lasted, with the slot's on-disk lock pinned against re-adoption. Fixed in two layers: - pool: the interrupted arm sweeps active -> requestStop() before shutdownNow -- the authoritative signal, synchronous with close(); - drainer: the park loops fold a pending interrupt into stopRequested (stopRequestedOrInterrupted), so ANY interrupt reaching a drainer thread without the flag routes through the normal STOPPED exit instead of the spin -- the pairing is no longer caller discipline. The status is deliberately left set (isInterrupted, not Thread.interrupted): the interrupted-teardown delegation protocol depends on loop.close()'s latch await throwing under a wedged I/O thread (BackgroundDrainerInterruptedTeardownTest). A drainer cut down mid-drain exits STOPPED with its unacked rows still in SF -- re-adopted by the next orphan scan, never dropped -- matching the graceful-timeout sweep's semantics. Both regression tests are red pre-fix: the pool test fails the synchronous stop-signal pin after an interrupted close(), and the drainer test joins out after 10s against a spinning connect-retry loop.
1 parent 0277a32 commit 21fe8cb

4 files changed

Lines changed: 437 additions & 3 deletions

File tree

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

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -435,7 +435,7 @@ public WebSocketClient connectWithDurableAckRetry() {
435435
if (sleepMillis > 0L && !stopRequested) {
436436
long parkDeadlineNanos = System.nanoTime() + sleepMillis * 1_000_000L;
437437
long remaining;
438-
while (!stopRequested
438+
while (!stopRequestedOrInterrupted()
439439
&& (remaining = parkDeadlineNanos - System.nanoTime()) > 0L) {
440440
LockSupport.parkNanos(Math.min(remaining, STOP_CHECK_PARK_CHUNK_NANOS));
441441
}
@@ -480,6 +480,37 @@ public boolean isStopRequested() {
480480
return stopRequested;
481481
}
482482

483+
/**
484+
* Stop check for the runner thread's park loops that also folds a
485+
* pending thread interrupt into the stop protocol. The pool delivers
486+
* cancellation as an interrupt ({@code shutdownNow}) and pairs it with a
487+
* {@link #requestStop()} sweep — but that pairing is caller discipline.
488+
* An interrupt arriving WITHOUT the flag would otherwise be pathological:
489+
* every wait here is a {@code LockSupport.parkNanos}, which returns
490+
* immediately while the status is pending and never clears it, so the
491+
* backoff/poll loops would degrade into a 100% CPU busy-spin (for as
492+
* long as an outage lasts) with the slot lock pinned. Mapping the
493+
* interrupt onto {@code stopRequested} routes them through the normal
494+
* STOPPED exit instead.
495+
* <p>
496+
* The status is deliberately left set ({@code isInterrupted()}, not
497+
* {@code Thread.interrupted()}): the teardown in {@link #run}'s finally
498+
* relies on it — {@code loop.close()}'s latch await must throw rather
499+
* than block under a wedged I/O thread, routing engine teardown through
500+
* the delegation protocol (pinned by
501+
* {@code BackgroundDrainerInterruptedTeardownTest}).
502+
* <p>
503+
* Called on the runner thread only. The unsynchronized check-then-set is
504+
* safe against a concurrent {@link #requestStop()}: both writers only
505+
* ever transition {@code stopRequested} false→true.
506+
*/
507+
private boolean stopRequestedOrInterrupted() {
508+
if (!stopRequested && Thread.currentThread().isInterrupted()) {
509+
stopRequested = true;
510+
}
511+
return stopRequested;
512+
}
513+
483514
@Override
484515
public void run() {
485516
runnerThread = Thread.currentThread();
@@ -539,7 +570,7 @@ public void run() {
539570
maxHeadFrameRejections);
540571
loop.start();
541572

542-
while (!stopRequested) {
573+
while (!stopRequestedOrInterrupted()) {
543574
long acked = engine.ackedFsn();
544575
this.ackedFsn = acked;
545576
if (acked >= target) {

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

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,11 @@
5454
* that don't exit in time (typically parked in a blocking native connect
5555
* that neither unpark nor interrupt cancels) are left to finish on their
5656
* own — the pool's underlying executor uses daemon threads so they don't
57-
* block JVM exit.
57+
* block JVM exit. An interrupted {@code close()} skips the graceful
58+
* window: every active drainer is stop-signaled immediately, then the
59+
* executor is shut down hard. A drainer cut down mid-drain exits STOPPED
60+
* with its unacked rows still in SF — re-adopted by the next orphan scan,
61+
* never dropped.
5862
*/
5963
public final class BackgroundDrainerPool implements QuietCloseable {
6064

@@ -177,6 +181,22 @@ public void close() {
177181
}
178182
} catch (InterruptedException e) {
179183
Thread.currentThread().interrupt();
184+
// Signal stop BEFORE shutdownNow's interrupts land. Actively-
185+
// draining drainers were spared by the split-stop above, and the
186+
// graceful-timeout sweep never runs on this path -- without this
187+
// sweep they would rely solely on the drainer-side fallback that
188+
// folds a pending interrupt into stopRequested
189+
// (BackgroundDrainer.stopRequestedOrInterrupted). This sweep is
190+
// the authoritative signal: it is synchronous with close() (the
191+
// caller observes fully stop-signaled drainers on return) and
192+
// covers a drainer that is between park-loop checks when the
193+
// interrupt lands. Cutting a mid-drain drainer short here is
194+
// deliberate -- an interrupted close() means "leave now", and
195+
// its unacked rows stay in SF for the next orphan scan (no data
196+
// loss), exactly like the graceful-timeout sweep.
197+
for (BackgroundDrainer d : active) {
198+
d.requestStop();
199+
}
180200
executor.shutdownNow();
181201
}
182202
}
Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,157 @@
1+
/*+*****************************************************************************
2+
* ___ _ ____ ____
3+
* / _ \ _ _ ___ ___| |_| _ \| __ )
4+
* | | | | | | |/ _ \/ __| __| | | | _ \
5+
* | |_| | |_| | __/\__ \ |_| |_| | |_) |
6+
* \__\_\\__,_|\___||___/\__|____/|____/
7+
*
8+
* Copyright (c) 2014-2019 Appsicle
9+
* Copyright (c) 2019-2026 QuestDB
10+
*
11+
* Licensed under the Apache License, Version 2.0 (the "License");
12+
* you may not use this file except in compliance with the License.
13+
* You may obtain a copy of the License at
14+
*
15+
* http://www.apache.org/licenses/LICENSE-2.0
16+
*
17+
* Unless required by applicable law or agreed to in writing, software
18+
* distributed under the License is distributed on an "AS IS" BASIS,
19+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
20+
* See the License for the specific language governing permissions and
21+
* limitations under the License.
22+
*
23+
******************************************************************************/
24+
25+
package io.questdb.client.test.cutlass.qwp.client.sf.cursor;
26+
27+
import io.questdb.client.cutlass.qwp.client.sf.cursor.BackgroundDrainer;
28+
import io.questdb.client.cutlass.qwp.client.sf.cursor.CursorSendEngine;
29+
import io.questdb.client.std.Files;
30+
import io.questdb.client.std.MemoryTag;
31+
import io.questdb.client.std.Unsafe;
32+
import org.junit.After;
33+
import org.junit.Assert;
34+
import org.junit.Before;
35+
import org.junit.Test;
36+
37+
import java.io.IOException;
38+
import java.nio.file.Paths;
39+
import java.util.concurrent.CountDownLatch;
40+
import java.util.concurrent.TimeUnit;
41+
42+
/**
43+
* Pins the drainer-side half of the interrupted-shutdown hardening: a bare
44+
* thread interrupt — no {@link BackgroundDrainer#requestStop()} — must act
45+
* as a stop signal, not degrade the drainer into a busy-spin.
46+
* <p>
47+
* {@code BackgroundDrainerPool.close()} pairs every executor interrupt
48+
* ({@code shutdownNow}) with a {@code requestStop()} sweep, but that pairing
49+
* is caller discipline: any interrupt reaching a drainer thread without the
50+
* flag (a future shutdownNow call site, a stray interrupt on the pool's
51+
* threads) would otherwise hit loops whose only wait primitive is
52+
* {@code LockSupport.parkNanos} — which returns immediately while the
53+
* interrupt status is pending and never clears it. With the status pending
54+
* and {@code stopRequested} false, the backoff/poll loops spin at 100% CPU
55+
* for as long as the outage lasts, holding the slot lock. The drainer
56+
* therefore folds a pending interrupt into {@code stopRequested} at its
57+
* park sites (status deliberately left set — the interrupted-teardown
58+
* delegation protocol depends on it, see
59+
* {@code BackgroundDrainerInterruptedTeardownTest}).
60+
*/
61+
public class BackgroundDrainerInterruptIsStopSignalTest {
62+
63+
private static final long SEGMENT_BYTES = 1L << 20;
64+
65+
private String tmpDir;
66+
67+
@Before
68+
public void setUp() {
69+
tmpDir = Paths.get(System.getProperty("java.io.tmpdir"),
70+
"qdb-int-stop-" + System.nanoTime()).toString();
71+
Assert.assertEquals(0, Files.mkdir(tmpDir, Files.DIR_MODE_DEFAULT));
72+
}
73+
74+
@After
75+
public void tearDown() {
76+
if (tmpDir == null) return;
77+
long find = Files.findFirst(tmpDir);
78+
if (find > 0) {
79+
try {
80+
int rc = 1;
81+
while (rc > 0) {
82+
String name = Files.utf8ToString(Files.findName(find));
83+
if (name != null && !".".equals(name) && !"..".equals(name)) {
84+
Files.remove(tmpDir + "/" + name);
85+
}
86+
rc = Files.findNext(find);
87+
}
88+
} finally {
89+
Files.findClose(find);
90+
}
91+
}
92+
Files.remove(tmpDir);
93+
}
94+
95+
@Test(timeout = 60_000)
96+
public void bareInterruptStopsConnectPhaseDrainer() throws Exception {
97+
// Slot with one published, unacked frame so the drainer opens a real
98+
// engine (takes the slot lock) and enters its connect-retry loop.
99+
long buf = Unsafe.malloc(16, MemoryTag.NATIVE_DEFAULT);
100+
try {
101+
CursorSendEngine prep = new CursorSendEngine(tmpDir, SEGMENT_BYTES);
102+
try {
103+
for (int i = 0; i < 16; i++) {
104+
Unsafe.getUnsafe().putByte(buf + i, (byte) i);
105+
}
106+
Assert.assertEquals(0L, prep.appendBlocking(buf, 16));
107+
} finally {
108+
prep.close();
109+
}
110+
} finally {
111+
Unsafe.free(buf, 16, MemoryTag.NATIVE_DEFAULT);
112+
}
113+
114+
// Unreachable cluster: plain transport failures retried indefinitely
115+
// under Invariant B -- absent a stop signal the drainer never exits.
116+
final CountDownLatch firstAttempt = new CountDownLatch(1);
117+
final BackgroundDrainer drainer = new BackgroundDrainer(
118+
tmpDir, SEGMENT_BYTES, Long.MAX_VALUE,
119+
() -> {
120+
firstAttempt.countDown();
121+
throw new IOException("connection refused (test)");
122+
},
123+
5_000L, 1L, 10L, false, 0L);
124+
125+
Thread runner = new Thread(drainer::run, "drainer-runner");
126+
runner.setDaemon(true);
127+
runner.start();
128+
try {
129+
Assert.assertTrue("drainer never reached its connect loop",
130+
firstAttempt.await(10, TimeUnit.SECONDS));
131+
132+
// Bare interrupt -- deliberately NOT paired with requestStop().
133+
runner.interrupt();
134+
135+
// The mapping must fold the pending status into stopRequested and
136+
// exit STOPPED. Pre-hardening: parks are no-ops, the flag never
137+
// rises, outcome stays PENDING while the loop spins -- this join
138+
// times out.
139+
runner.join(10_000L);
140+
Assert.assertFalse(
141+
"a bare interrupt did not stop the drainer: with the status pending "
142+
+ "every parkNanos is an immediate return, so the connect-retry "
143+
+ "loop busy-spins at 100% CPU holding the slot lock",
144+
runner.isAlive());
145+
Assert.assertEquals("interrupt must map onto the normal STOPPED exit",
146+
BackgroundDrainer.DrainOutcome.STOPPED, drainer.outcome());
147+
Assert.assertTrue("interrupt must be folded into stopRequested",
148+
drainer.isStopRequested());
149+
// Engine closed on the way out: the slot lock must be free.
150+
new CursorSendEngine(tmpDir, SEGMENT_BYTES).close();
151+
} finally {
152+
// Red-run cleanup: unhook a still-spinning drainer so a failing
153+
// assertion doesn't leave a hot daemon thread behind.
154+
drainer.requestStop();
155+
}
156+
}
157+
}

0 commit comments

Comments
 (0)