Skip to content

Commit 53bab20

Browse files
committed
fix(qwp): move closed-mid-creation delegate teardown outside the pool lock
When borrow()/acquire() re-locks after building a fresh delegate and finds the pool closed, both pools tore the delegate down while holding the pool lock. A delegate close() can block for seconds (5s ack drain, ~3s drainer- pool wind-down, 5s dispatch-thread join) or longer (unbounded I/O-thread latch await behind an OS-level connect when connect_timeout=0), stalling close(), giveBack/retireLease, reapIdle and watchdog cancels behind it -- violating retireLease's close-outside-the-lock contract on the ingest side and cancelIfCurrent's held-only-briefly contract on the egress side. SenderPool now mirrors retireLease: accounting under the lock (move the SF slot reservation from inFlightCreations to closingSlots, bump pendingLeaseTeardowns so close()'s outstanding-teardown wait sees the in-flight close), delegate close outside, re-lock to reclaim the slot and signal. QueryClientPool simply unlocks before shutdown(): the fresh worker never entered `all`, so close()'s snapshot loop cannot race the teardown. RED/GREEN regression tests park a fake delegate mid-close and prove the pool lock stays free (probe close() completes), close() still waits for the teardown to finish, and the closed-mid-creation worker teardown leaks no native scratch.
1 parent 2a9b4cd commit 53bab20

4 files changed

Lines changed: 335 additions & 2 deletions

File tree

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -255,6 +255,16 @@ public QueryWorker acquire() {
255255
lock.lock();
256256
inFlightCreations--;
257257
if (closed) {
258+
// Pool was closed mid-creation -- tear the fresh worker
259+
// down rather than leaking it, but OUTSIDE the lock:
260+
// shutdown() joins the dispatch thread for up to
261+
// SHUTDOWN_JOIN_MILLIS, and close()/release()/discard()/
262+
// cancelIfCurrent() all contend on this lock (whose
263+
// contract is "held only briefly"). The accounting above
264+
// already ran under the lock, and the worker never
265+
// entered `all`, so close()'s snapshot loop cannot race
266+
// this teardown.
267+
lock.unlock();
258268
try {
259269
created.shutdown();
260270
} catch (Throwable ignored) {

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

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -726,13 +726,41 @@ public PooledSender borrow() {
726726
if (closed) {
727727
// Pool was closed mid-creation -- destroy the new connection
728728
// rather than leaking it. Other waiters have been signaled
729-
// by close() already.
730-
freeSlotIndex(slotIndex);
729+
// by close() already. The delegate is closed OUTSIDE the
730+
// lock (mirroring retireLease): its close() can block for
731+
// seconds (bounded ack drain, drainer-pool wind-down) or
732+
// longer (unbounded I/O-thread latch await behind an
733+
// OS-level connect), which held here would stall close(),
734+
// giveBack/retireLease and reapIdle behind the pool lock.
735+
// Accounting first, under the lock: for an SF slot the
736+
// index reservation moves from inFlightCreations to
737+
// closingSlots until the close below releases the flock,
738+
// and pendingLeaseTeardowns keeps the out-of-lock close
739+
// visible to close()'s outstanding-teardown wait.
740+
boolean reserved = created.slotIndex() >= 0;
741+
if (reserved) {
742+
closingSlots++;
743+
}
744+
pendingLeaseTeardowns++;
745+
lock.unlock();
731746
try {
732747
created.delegate().close();
733748
} catch (Throwable ignored) {
734749
// Best-effort: an Error (e.g. -ea AssertionError)
735750
// from teardown must not mask the closed-pool signal.
751+
} finally {
752+
// Re-lock to reclaim the SF slot index and signal a
753+
// close() waiting on this teardown. MUST run even if
754+
// the delegate close threw, otherwise the slot stays
755+
// reserved forever and close() waits out its full
756+
// budget on a teardown that already happened.
757+
lock.lock();
758+
pendingLeaseTeardowns--;
759+
if (reserved) {
760+
reclaimSlot(created, " after closed-mid-creation teardown");
761+
}
762+
slotReleased.signalAll();
763+
lock.unlock();
736764
}
737765
throw new LineSenderException("QuestDB handle is closed");
738766
}

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

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,10 @@
3434
import org.junit.Assert;
3535
import org.junit.Test;
3636

37+
import java.util.concurrent.CountDownLatch;
38+
import java.util.concurrent.TimeUnit;
3739
import java.util.concurrent.atomic.AtomicInteger;
40+
import java.util.concurrent.atomic.AtomicReference;
3841
import java.util.function.Consumer;
3942

4043
// Error-safety of the three QueryClientPool creation paths the teardown-hardening
@@ -221,6 +224,75 @@ public void preWarmDoesNotLeakNativeScratchOnErrorFromStart() throws Exception {
221224
});
222225
}
223226

227+
// Site: acquire()'s closed-mid-creation branch. When acquire() re-locks
228+
// after building a fresh worker and finds the pool closed, it must tear the
229+
// worker down on its own thread -- outside the pool lock (shutdown() joins
230+
// the dispatch thread for up to SHUTDOWN_JOIN_MILLIS; cancelIfCurrent's
231+
// contract is that this lock is "held only briefly") -- and still reclaim
232+
// everything: the client's NATIVE_DEFAULT scratch and the creation slot.
233+
// RED (teardown dropped in the restructure): the scratch leaks and the
234+
// baseline assertion fails.
235+
// GREEN: shutdown() runs after the lock is released -> no leak, accounting
236+
// restored, acquire() surfaces the closed pool.
237+
@Test(timeout = 30_000)
238+
public void closedMidCreationTearsDownFreshWorkerWithoutLeak() throws Exception {
239+
TestUtils.assertMemoryLeak(() -> {
240+
CountDownLatch inConnect = new CountDownLatch(1);
241+
CountDownLatch releaseConnect = new CountDownLatch(1);
242+
Consumer<QwpQueryClient> connectHook = client -> {
243+
inConnect.countDown();
244+
try {
245+
if (!releaseConnect.await(10, TimeUnit.SECONDS)) {
246+
throw new IllegalStateException("test never released the parked connect");
247+
}
248+
} catch (InterruptedException e) {
249+
Thread.currentThread().interrupt();
250+
throw new IllegalStateException("interrupted in parked connect", e);
251+
}
252+
};
253+
// No-op startHook: the dispatch thread stays unstarted, so the
254+
// closed-mid-creation shutdown()'s interrupt+join is instant.
255+
QueryClientPool pool = newPool(CFG, 0, 1, 10_000, connectHook, w -> {
256+
});
257+
try {
258+
long baseline = Unsafe.getMemUsedByTag(MemoryTag.NATIVE_DEFAULT);
259+
AtomicReference<Throwable> acquireOutcome = new AtomicReference<>();
260+
Thread acquirer = new Thread(() -> {
261+
try {
262+
pool.acquire();
263+
} catch (Throwable t) {
264+
acquireOutcome.set(t);
265+
}
266+
}, "mid-creation-acquirer");
267+
acquirer.start();
268+
Assert.assertTrue("acquirer never reached connect()",
269+
inConnect.await(10, TimeUnit.SECONDS));
270+
271+
// Close the pool while the worker build is in flight (the fresh
272+
// worker never entered `all`, so close()'s snapshot skips it),
273+
// then let the build finish: acquire() must observe `closed`,
274+
// tear the worker down on its own thread and throw.
275+
pool.close();
276+
releaseConnect.countDown();
277+
acquirer.join(TimeUnit.SECONDS.toMillis(10));
278+
Assert.assertFalse("acquirer did not finish", acquirer.isAlive());
279+
280+
Assert.assertTrue(
281+
"acquire() must surface the closed pool, got: " + acquireOutcome.get(),
282+
acquireOutcome.get() instanceof QueryException
283+
&& String.valueOf(acquireOutcome.get().getMessage()).contains("closed"));
284+
long after = Unsafe.getMemUsedByTag(MemoryTag.NATIVE_DEFAULT);
285+
Assert.assertEquals(
286+
"closed-mid-creation acquire() leaked the fresh worker's NATIVE_DEFAULT scratch",
287+
baseline, after);
288+
Assert.assertEquals("in-flight creation accounting must be restored",
289+
0, inFlightCreations(pool));
290+
} finally {
291+
pool.close();
292+
}
293+
});
294+
}
295+
224296
private static Consumer<QwpQueryClient> alwaysThrow() {
225297
return client -> {
226298
throw new AssertionError("injected native connect failure");

0 commit comments

Comments
 (0)