Skip to content

Commit dde3c15

Browse files
committed
Deflake close interrupt lifecycle tests
The lifecycle tests inferred that close had left its creation wait whenever ReentrantLock.hasWaiters() briefly returned false. Interrupt delivery moves a condition waiter to the lock queue while awaitNanos() reacquires the lock, so the observer could mistake that transition for deadline expiry. Add test-only retry hooks that fire only after each pool catches an interrupt and confirms the original deadline still permits another wait. The tests now send each next interrupt after that acknowledgement, preventing coalescing and removing the transient queue-membership check while retaining deadline-restart coverage.
1 parent 9a93fe3 commit dde3c15

3 files changed

Lines changed: 106 additions & 80 deletions

File tree

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,11 @@ public final class QueryClientPool implements AutoCloseable {
9797
// DEFAULT_CLOSE_QUERY_TIMEOUT_MILLIS. Volatile because QuestDBImpl sets it
9898
// once at build time on a different thread than the borrowers that read it.
9999
private volatile long closeQueryTimeoutMillis = DEFAULT_CLOSE_QUERY_TIMEOUT_MILLIS;
100+
// Test seam invoked after close() handles an interrupt and confirms the
101+
// original creation-wait deadline still permits another wait. Null in
102+
// production; lifecycle tests use it to acknowledge distinct retries
103+
// without inspecting transient Condition queue membership.
104+
private volatile Runnable creationWaitRetryHook;
100105
private int inFlightCreations;
101106

102107
public QueryClientPool(
@@ -340,12 +345,20 @@ public void close() {
340345
long creationRemainingNanos = creationWaitNanos;
341346
boolean creationWaitInterrupted = false;
342347
while (inFlightCreations > 0 && creationRemainingNanos > 0) {
348+
boolean isRetryingAfterInterrupt = false;
343349
try {
344350
creationFinished.awaitNanos(creationRemainingNanos);
345351
} catch (InterruptedException e) {
346352
creationWaitInterrupted = true;
353+
isRetryingAfterInterrupt = true;
347354
}
348355
creationRemainingNanos = creationWaitDeadlineNanos - System.nanoTime();
356+
if (isRetryingAfterInterrupt && inFlightCreations > 0 && creationRemainingNanos > 0) {
357+
Runnable hook = creationWaitRetryHook;
358+
if (hook != null) {
359+
hook.run();
360+
}
361+
}
349362
}
350363
if (creationWaitInterrupted) {
351364
Thread.currentThread().interrupt();
@@ -557,6 +570,11 @@ public boolean isClosedForTesting() {
557570
return closed;
558571
}
559572

573+
@TestOnly
574+
public void setCreationWaitRetryHookForTesting(Runnable hook) {
575+
this.creationWaitRetryHook = hook;
576+
}
577+
560578
private QueryWorker createUnlocked() {
561579
QwpQueryClient client = QwpQueryClient.fromConfig(configurationString);
562580
try {

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,11 @@ public final class SenderPool implements AutoCloseable {
222222
// production; regression tests release a retired slot here to prove that
223223
// the terminal pass re-probes returned capacity before throwing.
224224
private volatile Runnable borrowWaitExpiredHook;
225+
// Test seam invoked after close() handles an interrupt and confirms the
226+
// original creation-wait deadline still permits another wait. Null in
227+
// production; lifecycle tests use it to acknowledge distinct retries
228+
// without inspecting transient Condition queue membership.
229+
private volatile Runnable creationWaitRetryHook;
225230
// Slots removed from `all` whose delegate is still releasing its flock.
226231
// They keep reserving capacity (and their slotInUse mark) until the
227232
// flock drops, so the cap check and the slot allocator stay consistent
@@ -1410,6 +1415,11 @@ public void setBorrowWaitExpiredHook(Runnable hook) {
14101415
this.borrowWaitExpiredHook = hook;
14111416
}
14121417

1418+
@TestOnly
1419+
public void setCreationWaitRetryHookForTesting(Runnable hook) {
1420+
this.creationWaitRetryHook = hook;
1421+
}
1422+
14131423
/**
14141424
* Raises the shutdown signal early -- without tearing down live delegates --
14151425
* so an in-flight startup-recovery step stops promptly between slots. Direct
@@ -1484,12 +1494,20 @@ public void close() {
14841494
long creationRemainingNanos = creationWaitNanos;
14851495
boolean creationWaitInterrupted = false;
14861496
while (inFlightCreations > 0 && creationRemainingNanos > 0) {
1497+
boolean isRetryingAfterInterrupt = false;
14871498
try {
14881499
creationFinished.awaitNanos(creationRemainingNanos);
14891500
} catch (InterruptedException e) {
14901501
creationWaitInterrupted = true;
1502+
isRetryingAfterInterrupt = true;
14911503
}
14921504
creationRemainingNanos = creationWaitDeadlineNanos - System.nanoTime();
1505+
if (isRetryingAfterInterrupt && inFlightCreations > 0 && creationRemainingNanos > 0) {
1506+
Runnable hook = creationWaitRetryHook;
1507+
if (hook != null) {
1508+
hook.run();
1509+
}
1510+
}
14931511
}
14941512
if (creationWaitInterrupted) {
14951513
Thread.currentThread().interrupt();

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

Lines changed: 70 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -39,11 +39,11 @@
3939

4040
import java.lang.reflect.Proxy;
4141
import java.util.concurrent.CountDownLatch;
42+
import java.util.concurrent.Semaphore;
4243
import java.util.concurrent.TimeUnit;
4344
import java.util.concurrent.atomic.AtomicBoolean;
4445
import java.util.concurrent.atomic.AtomicInteger;
4546
import java.util.concurrent.atomic.AtomicReference;
46-
import java.util.function.BooleanSupplier;
4747
import java.util.function.Consumer;
4848
import java.util.function.IntFunction;
4949

@@ -186,18 +186,21 @@ public void facadeCloseIsBoundedUnderRepeatedInterruptsDuringQueryCreation() thr
186186
inCreation.countDown();
187187
awaitOrFail(releaseCreation, "test never released query creation");
188188
};
189-
// A 1s creation-wait budget, not 100ms: the interrupt storm below must land at least
190-
// twice inside this window for the deadline-restart property to be exercised at all,
191-
// and a freshly started, yielding interrupter thread is not guaranteed two scheduler
192-
// quanta within 100ms on a saturated CI agent (observed on hosted 3-core mac agents,
193-
// where the post-join count assert failed with the product deadline honored exactly).
189+
// A 1s creation-wait budget gives the closer scheduler margin. Each next interrupt
190+
// is sent only after the pool acknowledges that it caught the previous one and will
191+
// retry against the original deadline, so interrupts cannot coalesce.
194192
QuestDBImpl db = newQuestDB(
195193
SENDER_CFG, 0, 0, 1000, slotIndex -> fakeSender(null, null, null), connectHook);
196194
QueryClientPool pool = db.getQueryPoolForTesting();
197195
AtomicReference<Throwable> borrowOutcome = new AtomicReference<>();
198-
AtomicBoolean closeReturnedInterrupted = new AtomicBoolean();
199-
AtomicBoolean keepInterrupting = new AtomicBoolean(true);
200-
AtomicInteger interruptCount = new AtomicInteger();
196+
AtomicBoolean isCloseComplete = new AtomicBoolean();
197+
AtomicBoolean isCloseReturnedInterrupted = new AtomicBoolean();
198+
AtomicInteger interruptRetryCount = new AtomicInteger();
199+
Semaphore closeProgress = new Semaphore(0);
200+
pool.setCreationWaitRetryHookForTesting(() -> {
201+
interruptRetryCount.incrementAndGet();
202+
closeProgress.release();
203+
});
201204
Thread borrower = new Thread(() -> {
202205
try {
203206
db.borrowQuery();
@@ -206,16 +209,14 @@ public void facadeCloseIsBoundedUnderRepeatedInterruptsDuringQueryCreation() thr
206209
}
207210
}, "interrupted-query-borrower");
208211
Thread closer = new Thread(() -> {
209-
db.close();
210-
closeReturnedInterrupted.set(Thread.currentThread().isInterrupted());
211-
}, "interrupted-query-closer");
212-
Thread interrupter = new Thread(() -> {
213-
while (keepInterrupting.get()) {
214-
interruptCount.incrementAndGet();
215-
closer.interrupt();
216-
Thread.yield();
212+
try {
213+
db.close();
214+
isCloseReturnedInterrupted.set(Thread.currentThread().isInterrupted());
215+
} finally {
216+
isCloseComplete.set(true);
217+
closeProgress.release();
217218
}
218-
}, "query-close-interrupter");
219+
}, "interrupted-query-closer");
219220
long nativeBaseline = Unsafe.getMemUsedByTag(MemoryTag.NATIVE_DEFAULT);
220221
try {
221222
borrower.start();
@@ -226,16 +227,15 @@ public void facadeCloseIsBoundedUnderRepeatedInterruptsDuringQueryCreation() thr
226227
closer.start();
227228
awaitCreationWaiter(pool,
228229
"facade close did not wait while query construction was internally owned");
229-
interrupter.start();
230-
awaitRepeatedInterrupts(interruptCount, pool::hasCreationWaiterForTesting,
231-
"query close left its creation wait before the interrupt storm landed twice");
232-
closer.join(TimeUnit.SECONDS.toMillis(5));
233-
Assert.assertFalse(
230+
Assert.assertTrue(
234231
"repeated interrupts restarted the query creation-wait deadline",
235-
closer.isAlive());
236-
Assert.assertTrue("test did not repeatedly interrupt query close", interruptCount.get() > 1);
232+
interruptUntilClose(closer, closeProgress, isCloseComplete));
233+
closer.join(TimeUnit.SECONDS.toMillis(5));
234+
Assert.assertFalse("facade query closer did not terminate", closer.isAlive());
235+
Assert.assertTrue("test did not observe repeated query close interrupt retries",
236+
interruptRetryCount.get() > 1);
237237
Assert.assertTrue("facade close must restore query closer interruption",
238-
closeReturnedInterrupted.get());
238+
isCloseReturnedInterrupted.get());
239239
Assert.assertEquals(
240240
"close must retain late-completion cleanup ownership",
241241
1, pool.inFlightCreations());
@@ -252,9 +252,7 @@ public void facadeCloseIsBoundedUnderRepeatedInterruptsDuringQueryCreation() thr
252252
borrowOutcome.get() instanceof QueryException
253253
&& String.valueOf(borrowOutcome.get().getMessage()).contains("closed"));
254254
} finally {
255-
keepInterrupting.set(false);
256255
releaseCreation.countDown();
257-
interrupter.join(TimeUnit.SECONDS.toMillis(10));
258256
db.close();
259257
borrower.join(TimeUnit.SECONDS.toMillis(10));
260258
closer.join(TimeUnit.SECONDS.toMillis(10));
@@ -275,15 +273,20 @@ public void facadeCloseIsBoundedUnderRepeatedInterruptsDuringSenderCreation() th
275273
};
276274
String senderConfig = "ws::addr=localhost:1;sf_dir="
277275
+ System.getProperty("java.io.tmpdir") + "/qdb-interrupted-pool-" + System.nanoTime() + ";";
278-
// 1s creation-wait budget for the same reason as the query-interrupt test above: the
279-
// interrupt storm must land at least twice inside the window even on a saturated agent.
276+
// 1s creation-wait budget for the same acknowledged-interrupt retry protocol as the
277+
// query test above.
280278
QuestDBImpl db = newQuestDB(senderConfig, 0, 0, 1000, senderFactory, client -> {
281279
});
282280
SenderPool pool = db.getSenderPoolForTesting();
283281
AtomicReference<Throwable> borrowOutcome = new AtomicReference<>();
284-
AtomicBoolean closeReturnedInterrupted = new AtomicBoolean();
285-
AtomicBoolean keepInterrupting = new AtomicBoolean(true);
286-
AtomicInteger interruptCount = new AtomicInteger();
282+
AtomicBoolean isCloseComplete = new AtomicBoolean();
283+
AtomicBoolean isCloseReturnedInterrupted = new AtomicBoolean();
284+
AtomicInteger interruptRetryCount = new AtomicInteger();
285+
Semaphore closeProgress = new Semaphore(0);
286+
pool.setCreationWaitRetryHookForTesting(() -> {
287+
interruptRetryCount.incrementAndGet();
288+
closeProgress.release();
289+
});
287290
Thread borrower = new Thread(() -> {
288291
try {
289292
db.borrowSender();
@@ -292,16 +295,14 @@ public void facadeCloseIsBoundedUnderRepeatedInterruptsDuringSenderCreation() th
292295
}
293296
}, "interrupted-sender-borrower");
294297
Thread closer = new Thread(() -> {
295-
db.close();
296-
closeReturnedInterrupted.set(Thread.currentThread().isInterrupted());
297-
}, "interrupted-sender-closer");
298-
Thread interrupter = new Thread(() -> {
299-
while (keepInterrupting.get()) {
300-
interruptCount.incrementAndGet();
301-
closer.interrupt();
302-
Thread.yield();
298+
try {
299+
db.close();
300+
isCloseReturnedInterrupted.set(Thread.currentThread().isInterrupted());
301+
} finally {
302+
isCloseComplete.set(true);
303+
closeProgress.release();
303304
}
304-
}, "sender-close-interrupter");
305+
}, "interrupted-sender-closer");
305306
try {
306307
borrower.start();
307308
Assert.assertTrue("sender borrow never reached construction",
@@ -313,16 +314,15 @@ public void facadeCloseIsBoundedUnderRepeatedInterruptsDuringSenderCreation() th
313314
closer.start();
314315
awaitCreationWaiter(pool,
315316
"facade close did not wait while sender construction was internally owned");
316-
interrupter.start();
317-
awaitRepeatedInterrupts(interruptCount, pool::hasCreationWaiterForTesting,
318-
"sender close left its creation wait before the interrupt storm landed twice");
319-
closer.join(TimeUnit.SECONDS.toMillis(5));
320-
Assert.assertFalse(
317+
Assert.assertTrue(
321318
"repeated interrupts restarted the sender creation-wait deadline",
322-
closer.isAlive());
323-
Assert.assertTrue("test did not repeatedly interrupt sender close", interruptCount.get() > 1);
319+
interruptUntilClose(closer, closeProgress, isCloseComplete));
320+
closer.join(TimeUnit.SECONDS.toMillis(5));
321+
Assert.assertFalse("facade sender closer did not terminate", closer.isAlive());
322+
Assert.assertTrue("test did not observe repeated sender close interrupt retries",
323+
interruptRetryCount.get() > 1);
324324
Assert.assertTrue("facade close must restore sender closer interruption",
325-
closeReturnedInterrupted.get());
325+
isCloseReturnedInterrupted.get());
326326
Assert.assertEquals(
327327
"close must retain late-completion cleanup ownership",
328328
1, pool.getInFlightCreationsForTesting());
@@ -341,9 +341,7 @@ public void facadeCloseIsBoundedUnderRepeatedInterruptsDuringSenderCreation() th
341341
borrowOutcome.get() instanceof LineSenderException
342342
&& String.valueOf(borrowOutcome.get().getMessage()).contains("closed"));
343343
} finally {
344-
keepInterrupting.set(false);
345344
releaseCreation.countDown();
346-
interrupter.join(TimeUnit.SECONDS.toMillis(10));
347345
db.close();
348346
borrower.join(TimeUnit.SECONDS.toMillis(10));
349347
closer.join(TimeUnit.SECONDS.toMillis(10));
@@ -529,34 +527,6 @@ private static void awaitCreationWaiter(SenderPool pool, String message) {
529527
Assert.fail(message);
530528
}
531529

532-
/**
533-
* Holds the test until the interrupt storm has landed at least twice while the facade close is
534-
* still inside its bounded creation wait. The deadline-restart property is only exercised by
535-
* interrupts that arrive during that wait, and the scheduler owes the interrupter thread
536-
* nothing: with a post-join count assert alone, the run races the close budget against thread
537-
* scheduling and can fail with the product invariant intact. Failing here instead separates
538-
* "interrupter starved before the budget expired" from a genuine deadline bug.
539-
*/
540-
private static void awaitRepeatedInterrupts(
541-
AtomicInteger interruptCount,
542-
BooleanSupplier closerStillWaiting,
543-
String message
544-
) {
545-
long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(10);
546-
while (System.nanoTime() < deadline) {
547-
// Count first: two interrupts observed while polling means the storm landed no matter
548-
// how quickly the wait ends afterwards, so a budget expiry seen next is not a failure.
549-
if (interruptCount.get() > 1) {
550-
return;
551-
}
552-
if (!closerStillWaiting.getAsBoolean()) {
553-
Assert.fail(message + "; interrupts landed: " + interruptCount.get());
554-
}
555-
Thread.yield();
556-
}
557-
Assert.fail(message + "; interrupts landed: " + interruptCount.get());
558-
}
559-
560530
private static void awaitOrFail(CountDownLatch latch, String message) {
561531
try {
562532
if (!latch.await(10, TimeUnit.SECONDS)) {
@@ -610,6 +580,26 @@ private static Sender fakeSender(
610580
});
611581
}
612582

583+
private static boolean interruptUntilClose(
584+
Thread closer,
585+
Semaphore closeProgress,
586+
AtomicBoolean isCloseComplete
587+
) throws InterruptedException {
588+
long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(5);
589+
closer.interrupt();
590+
while (!isCloseComplete.get()) {
591+
long remainingNanos = deadline - System.nanoTime();
592+
if (remainingNanos <= 0
593+
|| !closeProgress.tryAcquire(remainingNanos, TimeUnit.NANOSECONDS)) {
594+
return false;
595+
}
596+
if (!isCloseComplete.get()) {
597+
closer.interrupt();
598+
}
599+
}
600+
return true;
601+
}
602+
613603
private static QuestDBImpl newQuestDB(
614604
int senderMin,
615605
int queryMin,

0 commit comments

Comments
 (0)