Skip to content

Commit f39e846

Browse files
committed
Fix M1: stale pooled handle can corrupt the pool under double-close/use-after-close
A pooled Query/Sender handle was the reused per-slot object itself, guarded only by a non-volatile in-use/borrowed flag. Once a worker/slot was released and re-borrowed, that flag flips back to "live", so a stale handle's close()/cancel()/write would leak into a *different* borrow: a duplicate close double-released the worker/slot (enqueued twice -> two concurrent borrowers on one non-thread-safe client/delegate), and a cached Completion.cancel() or stale write hit whatever borrow now owns it. Idempotent close() and no-op cancel() are documented contracts, so this was reachable from contract-legal code, not just misuse, with pool-wide blast radius and no -ea guard. Fix: give every borrow its own immutable generation, stamped under the pool lock when the worker/slot is handed out and bumped again when it is returned. The reused state stays on the slot; callers get a thin per-borrow handle that carries the generation and validates it on every operation: - close()/cancel() are no-ops on a stale generation (idempotency preserved), - submit()/data writes throw, - release/giveBack/discardBroken re-check the generation under the pool lock so a worker/slot can never be enqueued twice, plus an -ea assert that it is not already in the available deque. Egress: QueryImpl stops being the user-facing Query; new QueryLease wraps it. Ingest: new SenderSlot is the reused slot; PooledSender becomes the per-borrow wrapper (keeps the public name, so borrow() still returns it). The per-submit path stays allocation-free; only the small lease handle is created per borrow (routinely scalar-replaced under try-with-resources). Adds QueryLeaseGenerationTest and SenderLeaseGenerationTest covering the double-release and cross-borrow cancel/write paths; updates the white-box tests to the new shapes. Full core suite green under -ea (the lone failure is the unrelated pre-existing FilesTest M2, which fails identically on master).
1 parent 65e4912 commit f39e846

15 files changed

Lines changed: 937 additions & 311 deletions

core/src/main/java/io/questdb/client/Query.java

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,11 @@
3636
* {@link #close()} it to release the client back to the pool (typically via
3737
* try-with-resources).
3838
* <p>
39-
* Allocation: zero at steady state -- {@code borrowQuery()} returns a
40-
* pre-allocated handle bound to the leased pool slot, and the
41-
* {@link Completion} is a field on it reused across submits.
39+
* Allocation: the per-submit path is allocation-free -- the heavy query state
40+
* is pre-allocated on the leased pool slot and reused, and {@link #submit()}
41+
* returns this same handle as its {@link Completion}. {@code borrowQuery()}
42+
* creates one small lease handle per borrow (often scalar-replaced by the JIT
43+
* when used with try-with-resources).
4244
* <p>
4345
* Lifecycle: configure with {@link #sql}, optional {@link #binds}, and
4446
* {@link #handler}, then call {@link #submit()} to obtain a {@link Completion}
@@ -83,10 +85,10 @@ public interface Query extends Closeable {
8385
Query sql(CharSequence sql);
8486

8587
/**
86-
* Submits the query for execution on the leased client. Returns the
87-
* {@link Completion} field cached on this handle; never allocates. The
88-
* handle is single-flight: {@code await()} the returned Completion before
89-
* the next {@code submit()}.
88+
* Submits the query for execution on the leased client. Returns this handle
89+
* as its own {@link Completion}; never allocates. The handle is
90+
* single-flight: {@code await()} the returned Completion before the next
91+
* {@code submit()}.
9092
*
9193
* @return the single-flight Completion bound to this Query handle
9294
*/

0 commit comments

Comments
 (0)