Skip to content

Commit d6bdbdf

Browse files
committed
Fix vacuous broken-slot assertion in SenderPoolTest
testBrokenSenderIsNotReturnedToPool asserted assertNotSame(first, second) on the two PooledSender wrappers. SenderPool.borrow() allocates a fresh PooledSender on every call, so that comparison is unconditionally true and proves nothing: it stays green whether or not the broken slot was discarded. With the discard logic reverted the test failed only incidentally, when second.close() re-threw on the recycled broken delegate, masking the real intent. Compare the underlying SenderSlot instead via the existing slotOf() helper, mirroring testBorrowReturnRecyclesSameDecorator. The pool recycles slots, not wrappers, so a broken slot leaking back to the next borrower now surfaces as the same slot and fails the assertion directly. The finally swallows the incidental close() exception so the assertion result is what surfaces. Verified by injecting the bug (giveBack instead of discardBroken on flush failure): the corrected assertion fails as a clean Failure at the assertion line, and passes with correct code. Also document the QueryWorker lost-dispatch coverage boundary in QueryWorkerTest: the single-flight-reuse race fix has no deterministic unit reproduction here because it needs the worker mid-runOn(client) when the user thread re-dispatches, which requires a live query client. That regression is guarded end-to-end by QuestDBFacadeE2ETest.testSustainedMixedConcurrency in the parent repo; testShutdownRacingDispatchMustNotStrandCaller covers only the adjacent shutdown branch.
1 parent 21becac commit d6bdbdf

2 files changed

Lines changed: 38 additions & 9 deletions

File tree

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,22 @@
3939
import java.util.concurrent.locks.Condition;
4040
import java.util.concurrent.locks.ReentrantLock;
4141

42+
/**
43+
* Unit tests for {@link QueryWorker}.
44+
* <p>
45+
* Coverage boundary: the lost-dispatch fix for the single-flight-reuse race
46+
* (clearing {@code current} under {@code signalLock} at the moment of
47+
* consumption rather than in a post-{@code runOn()} finally) has no
48+
* deterministic unit reproduction here. Reproducing the clobber needs the
49+
* worker to be mid-{@code runOn(client)} when the user thread re-dispatches on
50+
* the same lease, which requires a live query client to drive
51+
* {@code client.execute(...)} to its terminal callback. That regression is
52+
* guarded end-to-end by {@code QuestDBFacadeE2ETest.testSustainedMixedConcurrency}
53+
* in the parent questdb repo (more threads than pool slots, repeated
54+
* submit/await per lease). {@link #testShutdownRacingDispatchMustNotStrandCaller()}
55+
* below covers the adjacent but distinct shutdown-vs-dispatch branch only --
56+
* reverting the lost-dispatch hunk would not fail it.
57+
*/
4258
public class QueryWorkerTest {
4359

4460
/**

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

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -74,15 +74,16 @@ private static Object slotOf(Sender pooledWrapper) throws Exception {
7474
}
7575

7676
@Test
77-
public void testBrokenSenderIsNotReturnedToPool() {
77+
public void testBrokenSenderIsNotReturnedToPool() throws Exception {
7878
// Borrowing, buffering a row, and then closing forces flush() against
79-
// the unreachable address, which throws. The broken wrapper must not
80-
// be returned to the pool: its delegate's buffer still holds the
81-
// failed row, and on transports with terminal-failure semantics the
82-
// delegate is also unusable. Either way, the next borrower must get
83-
// a fresh wrapper.
79+
// the unreachable address, which throws. The broken slot must not be
80+
// returned to the pool: its delegate's buffer still holds the failed
81+
// row, and on transports with terminal-failure semantics the delegate
82+
// is also unusable. Either way, the next borrower must get a fresh
83+
// slot, not the broken one.
8484
try (SenderPool pool = new SenderPool(DEAD_HTTP_CONFIG, 1, 1, 1_000, Long.MAX_VALUE, Long.MAX_VALUE)) {
8585
Sender first = pool.borrow();
86+
Object firstSlot = slotOf(first);
8687
first.table("t").longColumn("v", 1).atNow();
8788
try {
8889
first.close();
@@ -92,11 +93,23 @@ public void testBrokenSenderIsNotReturnedToPool() {
9293
}
9394
Sender second = pool.borrow();
9495
try {
95-
Assert.assertNotSame("broken sender must not be handed back to next borrower",
96-
first, second);
96+
// borrow() always hands out a FRESH PooledSender wrapper, so
97+
// assertNotSame(first, second) on the wrappers is vacuously
98+
// true and proves nothing -- it stays true whether or not the
99+
// broken slot was discarded. What the pool recycles is the
100+
// underlying slot, so a broken slot leaking back to the next
101+
// borrower shows up as the SAME slot. Assert the slot differs.
102+
Assert.assertNotSame("broken slot must not be handed back to next borrower",
103+
firstSlot, slotOf(second));
97104
} finally {
98-
if (second != first) {
105+
// On the failing path (broken slot recycled) second.close()
106+
// re-throws, since its delegate's buffer still holds the
107+
// failed row; swallow it so the assertion above is what
108+
// surfaces rather than this incidental close() failure.
109+
try {
99110
second.close();
111+
} catch (LineSenderException ignored) {
112+
// expected only when the regression is present
100113
}
101114
}
102115
}

0 commit comments

Comments
 (0)