TCP M: connection pool — concurrent operations over one client - #560
TCP M: connection pool — concurrent operations over one client#560alex-clickhouse wants to merge 15 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Introduces concurrent TCP operations through a configurable, bounded connection pool.
Changes:
- Replaces the single-connection source with pooled connection leasing.
- Adds lifetime, idle-timeout, reuse-policy, and teardown handling.
- Adds extensive pool, socket, configuration, and integration tests.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
ClickHouseTcpConnection.cs |
Adds reuse validation and transport abort. |
ClickHouseBinaryReader.cs |
Exposes buffered-byte count. |
SingleConnectionSource.cs |
Removes serialized connection source. |
PooledConnection.cs |
Adds pooled connection bookkeeping. |
IConnectionSource.cs |
Updates leasing contract documentation. |
IConnectionFactory.cs |
Adds timeout-aware connection factory. |
ConnectionPool.cs |
Implements pooling, pruning, queueing, and disposal. |
ClickHouseTcpPoolReusePolicy.cs |
Adds FIFO/LIFO policies. |
ClickHouseTcpConnectionStringBuilder.cs |
Adds pool settings. |
ClickHouseTcpClientOptions.cs |
Adds pool options and validation. |
ClickHouseTcpClient.cs |
Switches the client to pooling. |
PoolTestDoubles.cs |
Adds deterministic pool test infrastructure. |
ClickHouseTcpConnectionTests.cs |
Tests connection reuse checks. |
ConnectionPoolIntegrationTests.cs |
Tests concurrent server operations. |
TcpConnectionFactoryTests.cs |
Tests dialing and socket liveness. |
SingleConnectionSourceTests.cs |
Removes obsolete source tests. |
ConnectionPoolTests.cs |
Covers pool lifecycle and concurrency. |
ClickHouseTcpConnectionStringBuilderTests.cs |
Tests pool setting parsing. |
ClickHouseTcpClientOptionsTests.cs |
Tests pool defaults and validation. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
ClickHouse.Driver.Tcp/Client/ConnectionPool.cs:306
- This indexes
idle[0]even when the lifetime pass just removed every idle connection butleased.Countremains above the floor. The timer wrapper swallows the resulting exception beforeCloseAll(reaped), so those removed connections are never closed. Guardidle.Count > 0before indexing.
while (options.IdleTimeout > TimeSpan.Zero
&& idle.Count + leased.Count > options.MinPoolSize
&& idle[0].IdleFor >= options.IdleTimeout)
ClickHouse.Driver.Tcp/Client/ClickHouseTcpConnectionStringBuilder.cs:95
- This public description says the floor is a count of idle connections, but the pool and
ClickHouseTcpClientOptions.MinPoolSizecount idle plus in-use connections. That distinction changes capacity planning under steady load; align this connection-string API documentation with the implemented semantics.
/// <summary>The number of idle connections kept rather than closed for inactivity. Defaults to 0.</summary>
4e8b63c to
f448156
Compare
54db672 to
d25b41d
Compare
d25b41d to
c228e1c
Compare
aaed69b to
4986a48
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4986a48. Configure here.
3737d23 to
f09215c
Compare
f09215c to
0f3e89e
Compare
0f3e89e to
b8d4979
Compare
b8d4979 to
8abcae2
Compare
5835516 to
ec35400
Compare
ec35400 to
4db59dd
Compare
Epic M. ConnectionPool replaces the interim SingleConnectionSource behind the same IConnectionSource seam, so a shared client now runs operations concurrently instead of serializing them onto one connection. A checkout takes a slot (SemaphoreSlim, MaxPoolSize), then either reuses an idle connection or opens one. Reuse is checked client-side every time: Ready, nothing left in the read buffer, and a non-blocking socket poll. Age is a correctness limit that overrides MinPoolSize; idleness is only a resource limit, so the sweep applies it and a checkout does not. M12 caps each operation's max_execution_time at the connection's remaining life, so the server ends a long query before the pool retires the connection under it. A connection with less than the retirement floor left is retired at checkout instead: the naive subtraction would derive a value of 0, which ClickHouse reads as no limit at all. Co-Authored-By: Claude <noreply@anthropic.com>
Drop the lifetime-derived max_execution_time. Its premise does not hold: the pool reaches a connection only through the idle set, which a checkout removes it from, so a running query is never interrupted for age — as in clickhouse-go, whose ConnMaxLifetime works the same way. What the cap bought was a bound on lifetime overshoot; what it cost was a query capped at 10s whenever a checkout landed near a connection's end of life, a hard ~30min ceiling on every query by default, and total failure against a profile that constrains the setting, which ClickHouse rejects rather than clamps. The "like in Go" precedent describes something else: Go derives the value from the caller's context deadline, and adds five seconds rather than subtracting them. Also from the review: - Track the leased connections. Nothing else can reach a connection whose lease is never disposed, so disposal used to leave that socket open for good; it now closes the stragglers once the drain deadline passes. - Release the pool slot in a finally, so a throwing teardown cannot shrink the pool by one for the rest of the process. - Swallow exceptions in the sweep timer's callback, which would otherwise fault a thread-pool thread rather than fail a sweep. - Stamp the idle clock under the lock, so the idle list really is ordered by return time as the sweep's early exit assumes. - Reject a timeout past what an int millisecond count can hold, at construction rather than from inside every operation. - Drop the second idle drain in DisposeAsync: nothing can land after the first, and the comment claiming otherwise obscured why. - Correct three doc comments that described behaviour the code does not have, including a read deadline that is not implemented yet. Tests: a concurrency stress case over the cap, the socket-poll branch of IsReusable over a real loopback socket, and the sweep guard. Co-Authored-By: Claude <noreply@anthropic.com>
Three fixes in the teardown paths. A close that throws no longer takes anything else down with it. Teardown is swallowed once, in PooledConnection.Close, rather than at each of the four sites that discard a connection: the batch being closed is emptied from its set first, so an escaping exception used to strand every connection after the failing one with nothing able to reach them — and on the return and checkout paths it surfaced as a failure of an unrelated caller's operation. Disposal aborts a straggler's transport instead of terminating it. Terminate is documented as unsafe to call concurrently with an operation, and it is: state and the reader/writer disposed flags are plain fields with check-then-set, so racing the operation's own unwinding could return one pooled buffer twice and hand the same memory to two callers. AbortTransport closes the socket only — which is what frees an operation parked on a dead read — and leaves the buffers to the operation itself. A rent that started before disposal can no longer add to the leased set after the drain has emptied it: opening a connection takes up to DialTimeout, so the check now happens under the same lock as the add. Also: docs for the disposal behaviour on the client, the pool and the lease; a test comment that said 200 checkouts for a loop of 800; tests for a throwing close on each path, the exact lifetime boundary, and a timeout of exactly int.MaxValue milliseconds. Co-Authored-By: Claude <noreply@anthropic.com>
CHTCP0001 is an error, not a warning, so referencing ClickHouseTcpClient from the pool to build an ObjectDisposedException message failed the Release build that CI runs. Debug did not catch it. The message should still name the type the caller holds rather than the internal pool, so the name is now a literal. Co-Authored-By: Claude <noreply@anthropic.com>
The floor used to decay. Nothing refilled it, so a pool that had been reaped down sat below MinPoolSize until traffic happened to grow it back, and after MaxConnectionLifetime of inactivity it was empty regardless. The sweep now opens connections to restore it. MinPoolSize counts open connections, in use as well as idle: the name says pool size, and counting only the spare ones would over-provision a pool under steady light load. The top-up takes a permit per connection exactly as a checkout does, which is what keeps the floor inside MaxPoolSize, and takes it with a zero timeout so it never queues ahead of a real caller. One runs at a time, or two sweeps race to fill the same gap. A failed dial ends the round quietly, since nobody is waiting on it, and the next sweep tries again. Disposal cancels a dial in flight rather than letting it hold a permit the drain is waiting for. No jitter on connection lifetime, matching Npgsql, database/sql and clickhouse-go; HikariCP is the outlier. Recorded in the TODO with the herd behaviour it accepts, so it reads as a choice rather than an oversight. Also conform to the house style the earlier commits missed: no collection expressions and no parenthesized `is not (A or B)` patterns, neither of which appears elsewhere in this library, and both of which were adding StyleCop warnings. Co-Authored-By: Claude <noreply@anthropic.com>
The top-up is a third party taking permits, so MaxPoolSize now depends on it playing by the same rules as a checkout. The existing stress case runs without a floor, so it never exercised that. Co-Authored-By: Claude <noreply@anthropic.com>
Terminate returned early on an already-terminated connection, so after AbortTransport set the state the operation's own Terminate was a no-op and the reader and writer buffers were always left to the GC — not only when the operation never unwound, as the comment claimed. Fix rather than document. The two Dispose methods now guard with an interlocked exchange, so releasing a pooled buffer is exactly-once even when two teardown paths race, and Terminate drops its early return: every step is idempotent, and the release has to run even when the state was set elsewhere. AbortTransport still never touches the buffers, since a buffer a pending read points at must not go back to the pool; the operation releases them as it unwinds, which is when the I/O has actually stopped. Prove the abort path with a real operation, which the previous test did not: a loopback listener completes the handshake then goes silent, a query blocks on the reply, and disposal past PoolTimeout has to release it. The assertion uses Task.WhenAny rather than WaitAsync, whose own timeout would have read as success. Co-Authored-By: Claude <noreply@anthropic.com>
The interlocked guard those two just gained had no test. Repeat disposal and sixteen threads disposing one writer at once are the observable half of it; the accounting itself is not observable, since ArrayPool does not detect a duplicate return, and the comment says so rather than implying the tests prove more than they do. Co-Authored-By: Claude <noreply@anthropic.com>
Two holes the permit accounting left open, both reported on the PR. A permit does not cover a connection for its whole life: an idle one holds none. The cap therefore also needs the dials in flight to be counted, and `BelowFloor` counted only the connections the pool already held. With min=max=3, a top-up that finished one connection while two checkouts were still dialing saw one connection against a floor of three and opened two more — five open against a cap of three. Both paths now reserve a slot in `opening` under the same lock that decides they may dial, and give it back under the same lock that files the connection. A checkout that was still dialing was also the one connection disposal could not reach: not in `leased`, so the abort at the end of the drain did not see it, and the dial observed only the caller's token. Disposal waited out `PoolTimeout`, found no straggler, and returned while the socket lived on for up to `DialTimeout`. Checkout dials now run on the shutdown token as well, reported as `ObjectDisposedException`; a caller's own cancellation still surfaces as a cancellation. Both tests fail on the previous commit: the cap test opens three connections where one was allowed, and the disposal test leaves its caller in a dial that never ends. Co-Authored-By: Claude <noreply@anthropic.com>
Coverage found both open paths in ConnectionPool: a checkout and a top-up whose dial finishes after disposal already emptied the sets. Neither connection is reachable by anything else, so each path has to close its own — which is the whole reason the disposed check shares a lock with the add. The dial these need is one that ran to completion before the cancellation arrived, so the fake factory grows an `IgnoresCancellation` seam. Without it the handshake observes the token and no connection is ever produced, which is the case the pool already handles trivially. Co-Authored-By: Claude <noreply@anthropic.com>
The previous commit counted dials in flight but not the other window of the same shape. A checkout lifts its connection out of `idle` before it records it in `leased`, so for that moment the connection is in neither set: with min=max=3, one connection leased and two idle, a checkout that takes one leaves the count reading 2, and a top-up dials into the gap for a fourth against a cap of 3. So the slot is now taken for the whole checkout — from the permit to the entry in `leased` — rather than only around a dial. A reused connection is then counted while it is off the books, and a checkout that has not yet decided whether to reuse or dial is counted before it holds anything at all. That makes the figure an upper bound rather than a census, which is the safe direction: a top-up declines to dial rather than overshooting. `opening` becomes `pending` and `TotalConnections` becomes `AccountedConnections`, since neither name was true any more. Reaching that window from a test needs a seam inside the checkout, and the clock is one: the checkout reads it to test the connection's age, between the two sets. `ControlledTimeProvider` grows a one-shot hook, and the test sweeps at exactly that moment. It opens three connections against a cap of two on the previous commit. Also from the review: - The cancellation test claimed to prove the reserved slot came back and proved only the permit, because it ran with no floor to observe it. Deleting the give-back left the whole suite green; it now runs with a floor of one and fails. - `BelowFloor` is back in the top-up loop condition, so a round with nothing to do no longer takes and releases a permit. - Documented why the sweep's trim counts differently from the floor, and that a dial deadline and a disposal landing together can be reported as each other. - Two tests disposed their pool only on the happy path. Co-Authored-By: Claude <noreply@anthropic.com>
The trim's floor test counts the connections that are out, so it passes with an empty idle set whenever more are leased than the floor asks for — and the trim then read `idle[0]`. With the default floor of zero that is any sweep at all while one connection is checked out, which is the commonest state a sweep can land in. On the timer `SweepQuietly` swallowed it, so the sweep silently stopped trimming and stopped holding the floor for the rest of the pool's life. Worse, the connections already reaped for age had left the idle set by then, and the throw skipped the close that follows the lock, leaving them open with nothing able to reach them. Tests missed it because every sweep case either had something idle or a floor at least as large as the number of leases. Also, concurrent disposal now shares one teardown. The second caller saw the flag and returned at once, reporting a pool whose connections were still open — and might yet be aborted by the first caller — as closed. And `MinPoolSize` on the connection-string builder described itself as a count of idle connections, which it never was. Co-Authored-By: Claude <noreply@anthropic.com>
An idle connection is what a proxy or load balancer between client and server drops on its own schedule, and such a drop can arrive without a FIN. IsReusable then still says yes, and the operation sent over that connection stalls until TCP gives up. So idleness now bars a checkout exactly as age does. Both limits go through one predicate, read at checkout, on return, and by the sweep. Neither limit yields to MinPoolSize any more. Reaping only to the floor while a checkout refuses an expired connection is the worst of both: the pool holds MinPoolSize sockets nobody can use, and the next burst still pays connect and handshake. The sweep now reaps everything expired and the top-up re-dials, which makes it a floor of fresh connections. The cost is MinPoolSize dials per IdleTimeout on a pool carrying no traffic, which the MinPoolSize doc now states with an example, since the two knobs multiply. Two consequences: - The idle clock must stop while a connection is out. It used to keep running, which was harmless when only the sweep read it, since the sweep walks the idle set. Once the return path reads it, a query longer than IdleTimeout would report its own duration as idleness and retire the connection that had just run it. - The sweep is one indexed walk over the idle set. That removes the two-pass structure behind the idle[0] bug rather than only fixing it. The return path asks IsReusable, which contains its old State == Ready test, so a connection that comes back out of step with the server is closed at once instead of at the next checkout. Every test that pins this fails when the change is reverted, checked by reverting each half in turn: the two checks, the idle clock, and the floor override. Two are integration tests, since a hand-held clock's timers do nothing: that an over-idle connection is retired end to end without a test calling Sweep itself, and that execute, insert, query and stream each leave a connection the pool keeps. Co-Authored-By: Claude <noreply@anthropic.com>
The sweep allocated a List each time it retired a connection. It now fills a reusable field. A plain field is not safe by itself, because the sweep closes sockets outside the lock: two sweeps that overlap would share the buffer, and the second would clear it while the first still read it. Timer callbacks can overlap, because a close can take longer than the period. So a sweep now admits one caller at a time, with the same Interlocked guard the top-up uses. That guard also publishes the clear to the next sweep. A new test re-enters the sweep from inside a close; without the guard it fails with "Collection was modified". SweepInterval is a new option. Null, the default, derives the period as before: a quarter of the shortest limit in force, held between 1 and 30 seconds. An explicit value replaces that period and is used unclamped, because a caller who sets it is overriding a derivation that does not suit their workload. It must be positive; null is how to go back to deriving. Nothing-to-sweep still wins over the option: with both limits off and no floor the pool creates no timer at all, which keeps an undisposed client collectable. The divisor is now a named constant, and the derivation reads top to bottom. The XML comments in the pool files were too long and too indirect. This states the same facts in fewer words. Only two invariants keep their length: why OnRented can write outside the lock, and why the disposed check belongs inside it. Co-Authored-By: Claude <noreply@anthropic.com>
4db59dd to
358c966
Compare

Stacked on #559 (
tcp/epic-n9-poco-write) — review that one first; this PR's diff is the pool only.TCP epic M: a real connection pool.
ConnectionPoolreplaces the interimSingleConnectionSourcebehind the sameIConnectionSourceseam, so the client changed by one line — but the behaviour it exposes is new: a shared client now runs operations concurrently instead of serializing them onto one connection.Knobs
On
ClickHouseTcpClientOptionsand the connection-string builder.TimeSpan.Zerodisables the two limits.SweepIntervalis the one knob whose absence means something rather than nothing.MaxPoolSizeMinPoolSizePoolTimeoutMaxConnectionLifetimeIdleTimeoutSweepIntervalPoolReusePolicyLifoDecisions worth a look
Validation is always on, client-side, with no knob. The design sketch had a
ValidateOnBorrowswitch, but a switch to turn off correctness is only a way to get corruption.ClickHouseTcpConnection.IsReusableis Ready + nothing in our read buffer + a non-blockingSocket.Poll(SelectRead); both buffers have to be checked, since the poll sees the kernel's andBufferedBytessees ours. A readable idle socket means either the peer closed or bytes are waiting that the last operation did not consume — neither allows reuse. No Ping option: a round-trip per checkout is the wrong default and the wrong opt-in.Age and idleness are the same kind of limit. Both are read at checkout, on return, and by the sweep, through one predicate, and both override
MinPoolSize.The sweep period is derived, with an override. A quarter of the shortest limit in force, held between 1 and 30 seconds, so the delay in noticing an expiry stays proportional to the limit being enforced — one fixed period cannot do that for limits that range from seconds to hours. At the defaults that is 30s, so a flat 5s would be six times the wake-ups for nothing, while a 2s
IdleTimeoutwould be enforced three and a half times late.SweepIntervaloverrides it for a workload the derivation does not suit, and is used unclamped: silently changing an explicit value is the failure mode the knob exists to avoid. Zero is invalid rather than meaning "no sweep" — null is how to go back to deriving — because zero alongside aMinPoolSizewould be a floor nothing ever fills, the sweep being the only thing that tops it up. And nothing-to-sweep still wins over the override: with both limits off and no floor, no timer is created at all, which is what keeps an undisposed client collectable.Idleness started as a resource limit only — swept, honouring the floor, never barring a checkout — on the reasoning that
IdleTimeoutexists to release sockets nobody is using, not to judge whether one works. That reasoning misses what actually happens to an idle connection: a proxy or load balancer between client and server drops it on its own schedule, and such a drop can arrive without a FIN, in which caseIsReusablestill says yes and the operation sent over it stalls until TCP gives up — on Linux, about fifteen minutes. So idleness is a liveness limit as much as a resource one, and the docs now say to set it below the shortest idle timeout on the path to the server.Two consequences worth spelling out.
Neither limit yields to
MinPoolSize, and that is what makes the floor useful. Reaping only to the floor while a checkout refuses an expired connection is the worst of both: the pool holdsMinPoolSizesockets nobody can use, and the next burst still pays connect and handshake. Reaping everything expired and letting the top-up re-dial makes it a floor of fresh connections. The cost isMinPoolSizedials perIdleTimeouton a pool carrying no traffic at all — with the defaults, none, the floor being 0, but the two knobs multiply, so a floor of 10 against a 5-second idle limit is 10 connects and handshakes every 5 seconds from an application issuing no queries. TheMinPoolSizedoc now says so with that example, and a test pins the rotation over two sweeps. Npgsql and HikariCP both stop at the floor instead, and HikariCP then needs a separatekeepaliveTimeto validate what it kept; one limit does both here.The idle clock has to stop while a connection is out. It used to keep running, which was harmless when only the sweep read it — the sweep walks the idle set, so the value was meaningless for a leased connection either way. Once the return path reads it, that stops being harmless: a query longer than
IdleTimeoutwould report its own duration as idleness and retire the connection that had just run it successfully. With the 5-minute default, every query over five minutes.PooledConnectionnow tracks whether it is out and reports zero idleness while it is.That also simplified the sweep to one indexed walk over the idle set, which removed the two-pass structure behind the
idle[0]bug below rather than only fixing it.MinPoolSizeis held, not just respected. The sweep tops the pool back up to the floor after reaping, so it does not decay — without that, nothing refilled it and afterMaxConnectionLifetimeof inactivity the pool was empty whatever the floor said. It counts open connections, in use as well as idle: the name says pool size, and counting only spare ones would over-provision a pool under steady light load. The top-up takes a permit per connection exactly as a checkout does, which is what keeps the floor insideMaxPoolSize, and takes it with a zero timeout so it never queues ahead of a real caller. One runs at a time; a failed dial ends the round quietly and the next sweep retries; disposal cancels a dial in flight. What is still M10 is the eager half — filling the floor at construction, so the very first burst does not pay connect and handshake.No jitter on connection lifetime. Connections opened in one burst age out together, so they are reaped together and re-dialled together; across a fleet deployed at once, every instance rotates at the same time. Npgsql, Go's
database/sqland clickhouse-go all accept this and HikariCP is the outlier that jitters. Matching the majority, and recorded in the TODO as a choice rather than an oversight — worth revisiting if reconnect spikes show up against an expensive auth path, where the herd actually costs something.No retry counter for M9. The checkout loop discards unusable idle candidates until one passes or the set is empty, then dials once. A failure to dial that connection is reported rather than retried, so a server that is down is reported as down instead of being hidden behind
PoolTimeout.Waiters are not ordered, and the pool says so.
SemaphoreSlimmakes no promise and a fair queue is not worth building here: two operations that overlap on different connections have no ordering at the server either.🤖 Generated with Claude Code