Skip to content

Commit 3a33cf2

Browse files
committed
Add deterministic regression test for the busy-worker shutdown-drop fix
df6f7ca (while (!shuttingDown) -> while (true) in QueryWorker.runLoop) repaired the busy-worker path: a worker returning from runOn() with current already re-armed by a reused lease's submit() must strand and signal that job before exiting on shutdown. That fix shipped without a deterministic regression test -- the only adjacent test, testShutdownRacingDispatchMustNotStrandCaller, drives only the parked-worker branch and stays green with df6f7ca reverted. Reproducing the busy path needs the worker mid-runOn() when current is re-dispatched and shuttingDown flips. QueryWorker and QueryImpl are final and QwpQueryClient has no test seam, so the only race-free pause point is a test-only barrier. Add a volatile busyWorkerTestHook (null in production; the sole cost is a null check) invoked on the worker thread right after a job returns from runOn(), and a new test that installs it to set current=q2 and shuttingDown=true at exactly that window. The new test fails on the reverted fix ("q2 never signalled") and passes on the fix; the old parked test passes either way, confirming it never guarded this path.
1 parent e77b4fc commit 3a33cf2

2 files changed

Lines changed: 120 additions & 0 deletions

File tree

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,17 @@ public final class QueryWorker {
5757
private final ReentrantLock signalLock = new ReentrantLock();
5858
private final Thread thread;
5959
private volatile QueryImpl current;
60+
// Test-only deterministic barrier for the busy-worker shutdown-drop race
61+
// fixed in df6f7ca (while (!shuttingDown) -> while (true)). Null in
62+
// production -- the only cost is the null check in runLoop(). A regression
63+
// test installs a hook that runs ON THE WORKER THREAD right after a job
64+
// returns from runOn() and before the loop re-enters the strand check, to
65+
// re-arm current with a re-dispatched job and flip shuttingDown -- exactly
66+
// the window where the old top-of-loop check dropped a pending job. The
67+
// classes involved (QueryWorker, QueryImpl) are final and QwpQueryClient
68+
// has no test seam, so this is the only race-free reproduction point. See
69+
// QueryWorkerTest.testBusyWorkerShutdownStrandsReDispatchedCurrent.
70+
volatile Runnable busyWorkerTestHook;
6071
// Monotonic lease id. Mutated only under the QueryClientPool lock
6172
// (bumped once in acquire() when the worker is handed out and once in
6273
// release() when it is returned), so successive borrows of the same
@@ -319,6 +330,12 @@ private void runLoop() {
319330
} catch (Throwable t) {
320331
q.signalUnexpected(t);
321332
}
333+
// Test-only barrier: deterministically reproduce the busy-worker
334+
// shutdown-drop race (df6f7ca) at its exact site. Null in production.
335+
Runnable hook = busyWorkerTestHook;
336+
if (hook != null) {
337+
hook.run();
338+
}
322339
}
323340
}
324341
}

core/src/test/java/io/questdb/client/test/impl/QueryWorkerTest.java

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
import java.lang.reflect.InvocationTargetException;
3737
import java.lang.reflect.Method;
3838
import java.util.concurrent.TimeUnit;
39+
import java.util.concurrent.atomic.AtomicBoolean;
3940
import java.util.concurrent.locks.Condition;
4041
import java.util.concurrent.locks.ReentrantLock;
4142

@@ -175,6 +176,108 @@ public void testShutdownRacingDispatchMustNotStrandCaller() throws Exception {
175176
unexpectedF.get(queryImpl));
176177
}
177178

179+
/**
180+
* Busy-worker variant of the shutdown-drop race fixed in df6f7ca
181+
* ({@code while (!shuttingDown)} -> {@code while (true)} in
182+
* {@link QueryWorker}'s run loop). Unlike
183+
* {@link #testShutdownRacingDispatchMustNotStrandCaller()} -- which only
184+
* drives the PARKED-worker branch (worker blocked in
185+
* {@code awaitUninterruptibly} before {@code shuttingDown} flips) and stays
186+
* green even with the fix reverted -- this test forces the worker THROUGH a
187+
* job's {@code runOn()} and then, on the worker thread at the exact instant
188+
* that job returns, reproduces a reused lease re-dispatching
189+
* ({@code current = q2}) racing a shutdown ({@code shuttingDown = true}),
190+
* both set before the loop re-enters the strand check.
191+
* <p>
192+
* With the fix the loop re-enters the {@code signalLock} block, observes
193+
* {@code shuttingDown}, and strands q2 (signalling its caller). With the bug
194+
* the loop exits at the top without re-reading {@code current}, so q2 is
195+
* dropped -- never run, never signalled -- and its caller's
196+
* {@code Completion.await()} would hang forever. The assertion on
197+
* {@code q2.done} fails if the fix is reverted.
198+
* <p>
199+
* The interleaving is made deterministic with a test-only worker-thread
200+
* barrier ({@code QueryWorker.busyWorkerTestHook}) instead of a sleep:
201+
* {@link QueryWorker} and {@code QueryImpl} are final and
202+
* {@code QwpQueryClient} has no test seam, so pausing between
203+
* {@code runOn()} and the loop check is the only race-free reproduction.
204+
* {@code client}/{@code pool} are null -- {@code q1.runOn(null)} throws an
205+
* NPE that {@code runLoop} catches and turns into q1's terminal signal, a
206+
* fast stand-in for a real job returning from {@code runOn()}.
207+
*/
208+
@Test(timeout = 30_000)
209+
public void testBusyWorkerShutdownStrandsReDispatchedCurrent() throws Exception {
210+
Class<?> queryImplClass = Class.forName("io.questdb.client.impl.QueryImpl");
211+
212+
Field lockF = QueryWorker.class.getDeclaredField("signalLock");
213+
Field currentF = QueryWorker.class.getDeclaredField("current");
214+
Field shuttingF = QueryWorker.class.getDeclaredField("shuttingDown");
215+
Field threadF = QueryWorker.class.getDeclaredField("thread");
216+
Field hookF = QueryWorker.class.getDeclaredField("busyWorkerTestHook");
217+
for (Field f : new Field[]{lockF, currentF, shuttingF, threadF, hookF}) {
218+
f.setAccessible(true);
219+
}
220+
221+
Field doneF = queryImplClass.getDeclaredField("done");
222+
Field unexpectedF = queryImplClass.getDeclaredField("unexpectedError");
223+
doneF.setAccessible(true);
224+
unexpectedF.setAccessible(true);
225+
226+
// client == null: q1.runOn(null) throws NPE, which runLoop catches and
227+
// turns into q1's terminal signal -- a fast, deterministic stand-in for
228+
// a real job returning from runOn(). pool == null is never touched here.
229+
QueryWorker worker = new QueryWorker(null, null, 0);
230+
231+
Constructor<?> ctor = queryImplClass.getDeclaredConstructor(QueryWorker.class);
232+
ctor.setAccessible(true);
233+
Object q1 = ctor.newInstance(new Object[]{worker});
234+
Object q2 = ctor.newInstance(new Object[]{worker});
235+
doneF.setBoolean(q1, false);
236+
doneF.setBoolean(q2, false);
237+
238+
ReentrantLock lock = (ReentrantLock) lockF.get(worker);
239+
AtomicBoolean fired = new AtomicBoolean(false);
240+
241+
// The busy-worker barrier: the FIRST time the worker returns from a
242+
// job's runOn(), simulate submit() -> dispatch() re-arming current with
243+
// q2 while shutdown() flips shuttingDown -- both set, under signalLock,
244+
// before the loop re-checks. Runs on the worker thread.
245+
Runnable hook = () -> {
246+
if (fired.compareAndSet(false, true)) {
247+
lock.lock();
248+
try {
249+
currentF.set(worker, q2);
250+
shuttingF.setBoolean(worker, true);
251+
} catch (IllegalAccessException e) {
252+
throw new RuntimeException(e);
253+
} finally {
254+
lock.unlock();
255+
}
256+
}
257+
};
258+
hookF.set(worker, hook);
259+
260+
// Pre-arm current with q1 so the worker consumes it immediately on
261+
// start (no need to wait for the await park); start() establishes the
262+
// happens-before that publishes current and the hook to the worker.
263+
currentF.set(worker, q1);
264+
265+
Thread t = (Thread) threadF.get(worker);
266+
t.start();
267+
268+
t.join(5_000);
269+
Assert.assertFalse("worker thread must exit after shuttingDown=true", t.isAlive());
270+
271+
Assert.assertTrue(
272+
"BUG (df6f7ca regressed): the busy worker returned from runOn() with a "
273+
+ "re-dispatched current!=null and shuttingDown=true, then exited the loop "
274+
+ "without stranding it. q2 was never signalled; its caller's await() hangs "
275+
+ "forever.",
276+
doneF.getBoolean(q2));
277+
Assert.assertNotNull("the stranded busy-path job must record the closed-handle error",
278+
unexpectedF.get(q2));
279+
}
280+
178281
/**
179282
* Result handlers (onBatch/onEnd/onError) run inline on the worker's
180283
* dispatch thread. The blocking lease ops -- {@code close()} and the two

0 commit comments

Comments
 (0)