Reclaim emancipated connections, including while callers are parked - #4529
Reclaim emancipated connections, including while callers are parked#4529mdaigle wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a demand-driven background reclaim timer to ChannelDbConnectionPool so emancipated (GC-orphaned) connections can be swept and returned to the idle channel while all callers are blocked waiting, preventing pool saturation/timeouts in the “everyone parked” scenario described in #4490.
Changes:
- Introduces
PoolReclaimer, a one-shot re-arming timer that sweeps for emancipated connections only while callers are parked. - Wires reclaim registration around the idle-channel wait and disposes the reclaimer during pool shutdown before draining.
- Adds
ChannelDbConnectionPoolReclaimTimerTestunit tests to cover timer arming/disarming and an end-to-end “GC after parking” wake-up scenario.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs |
New unit tests validating PoolReclaimer behavior and the end-to-end “park then GC then wake” scenario. |
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs |
New demand-driven timer component that runs background sweeps while callers are parked. |
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs |
Integrates PoolReclaimer creation, parked-wait registration, internalizes ReclaimEmancipatedConnections, and disposes the reclaimer during shutdown. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ThreadPool.GetMinThreads(out int workerThreads, out int completionPortThreads); | ||
| int desired = Environment.ProcessorCount * 4; | ||
| if (workerThreads < desired) | ||
| { | ||
| ThreadPool.SetMinThreads(desired, completionPortThreads); | ||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs:124
- ThreadPool.SetMinThreads returns a bool indicating whether the new minimum was accepted. Since this helper is used to avoid test timeouts, assert on the return value so failures are diagnosed explicitly rather than showing up as flaky hangs/timeouts later.
if (workerThreads < desired)
{
ThreadPool.SetMinThreads(desired, completionPortThreads);
}
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs:163
- EnterParkedWait increments _parkedWaiters before checking _disposed. If the pool is shutting down and a caller races in after disposal, this can leave ParkedWaiters artificially inflated even though the timer can never be armed again.
_parkedWaiters++;
if (_armed || _disposed)
{
return;
A SqlConnection that is garbage collected without ever being closed or disposed leaves its internal connection "emancipated": still tracked by the pool, but with no owner that could ever return it. WaitHandleDbConnectionPool sweeps for these before waiting for a free connection; ChannelDbConnectionPool did not, so an emancipated connection permanently occupied a pool slot. At MaxPoolSize that meant every subsequent Open timed out -- forever, not just once. GetInternalConnection now performs the same sweep just before parking on the idle channel. This is deliberately confined to the slow path: it is O(MaxPoolSize) and allocates a snapshot, so it must not run on the hot acquire path. The sweep takes the connection lock with Monitor.TryEnter rather than Enter. IsEmancipated has to be read under that lock to avoid racing PrePush/PostPop, but a connection that is currently locked is being actively handed out or returned and therefore is not emancipated anyway, so skipping it costs nothing and keeps the sweep from blocking the caller. Only PrePush happens under the lock; deactivation, which can make server round trips, is deferred until all locks are released. Deactivating and routing a returned connection is now factored out of ReturnInternalConnection into DeactivateAndRouteConnection so reclamation can share it. Reclamation must not go through ReturnInternalConnection itself because it has already performed the PrePush and there is no owning object left to validate against. Tests: - Added ConnectionPoolVersionScope, which flips the pool version switch and clears all pools on both entry and exit. Clearing is required because a pool binds to its implementation at creation time, so without it pools leak across tests. - Parameterized ReclaimEmancipatedOnOpenTest and MaxPoolWaitForConnectionTest by pool version. ReclaimEmancipatedOnOpenTest fails against ChannelDbConnectionPool without this fix. - Three pool-exhaustion unit tests let their owning SqlConnections go out of scope, so reclamation could legitimately hand the "should time out" waiter a connection. They now keep the owners alive, which is what they meant anyway. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
ChannelDbConnectionPool only reclaimed emancipated connections inline, on the caller's own thread, immediately before parking on the idle channel. A connection becomes emancipated when its owning SqlConnection is collected without being closed, and that can only be observed after a GC. If the GC lands after the caller has already parked, nothing sweeps again: every caller is blocked on the channel, so the pool stays saturated until an unrelated caller arrives to run its own inline sweep. With all callers parked, that never happens and they all fail on their connect timeout. The sweep cannot simply be retried on the parked caller's thread. Channels guarantee FIFO delivery to ReadAsync callers, which the pool relies on for fairness, so a caller that cancelled its read to re-sweep would rejoin at the back of the queue behind callers that arrived later. The trigger has to come from off-thread. PoolReclaimer adds a demand-driven timer for that. Callers register around their parked wait, the timer arms on the first registration and disarms on the last, so a pool that is not blocking pays nothing beyond the ~100 bytes of a disarmed timer, which is not in the timer queue's list and does not lengthen any tick. It is separate from PoolPruner rather than folded into it: a merged timer would have to run at the faster of the two cadences, and the prune interval is derived from the idle timeout and stretches to 288s at the default, so merging would multiply prune-driven ticks by ~28x. The reclaimer is built for every pool configuration, unlike the pruner, which is null for a fixed-size pool or a zero idle timeout. A connection can leak in any configuration, and a fixed-size pool is where a leaked slot hurts most. The sweep runs one-shot and re-arms at the end of each callback so a slow sweep cannot overlap the next, sweeps outside its lock because reclamation can make server round trips, and swallows exceptions because a throwing timer callback would tear down the process. The timer is created via ADP.UnsafeCreateTimer so it does not capture the execution context of whichever caller happens to park first, which would otherwise pin that caller's async locals for the lifetime of the pool. The one-second cadence is far tighter than the legacy pool's background reclaim, which rides a randomized 2-4 minute cleanup wait and is too slow to rescue a caller inside a 15 second connect timeout. Sweeping only while callers are parked is what makes the tighter cadence affordable. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
ConnectionPoolSlots.Snapshot copied every occupied slot into a new List sized to the pool's full capacity, and the reclaim sweep was its only caller. The copy bought nothing: the backing array has a fixed capacity and is never reallocated, and the snapshot was just a sequence of the same per-slot volatile reads an in-place walk performs, so it offered no consistency guarantee beyond the best-effort one already documented. What it did cost was a list per sweep even when nothing was emancipated, roughly 800 bytes at the default MaxPoolSize of 100. That was tolerable when the sweep only ran inline on a caller about to park. It is less so now that the reclaim timer sweeps once a second for as long as any caller is parked, which is exactly when the pool is under pressure. Replaced with a struct enumerator so the sweep's foreach allocates nothing and the backing array stays private to the collection. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Rebasing onto the pool instrumentation work makes two things available that the reclaim path was missing. The metrics seam now exists, so the sweep emits ReclaimedConnectionRequest the way WaitHandleDbConnectionPool does. Previously the number-of-reclaimed-connections counter always read zero under pool V2, which made a leaking application look healthy on exactly the counter that would have identified it. The trace call sites also predate the new pool trace message format, so they still used the legacy prov-prefixed form. Converted the sweep, reclaimer and shutdown messages to match their neighbours. Adds a parity test asserting both pool implementations report identical counters when a leaked connection is reclaimed. It drives the sweep the same way for both, by capping the pool at one connection, leaking it and requesting another, so it asserts observable behaviour rather than an internal entry point. The counters agree exactly, including a shared quirk now pinned by the test: reclamation emits no soft disconnect, so activeSoftConnections drifts up by one for every leaked connection. Also makes the parked-caller test deterministic. It relied on the leaked owner surviving until the second caller parked, but any test running in parallel could collect it first, in which case the caller's inline sweep succeeded and it never parked. The owner is now rooted in a GCHandle that the test frees at the exact point it wants emancipation to become observable. A local cannot express that: in a Debug build its stack slot roots the object for the rest of the method even once it is assigned null. That also removes the thread-pool headroom workaround added earlier, which was aimed at a starvation theory the diagnostics disproved. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
8e2ec53 to
513c4d3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs:233
- DbConnectionInternal.IsEmancipated should be read under a lock on the connection to avoid races with PrePush/PostPop. This assertion reads it without locking.
Assert.Equal(0, pool.IdleCount);
Assert.True(connection.IsEmancipated);
}
src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs:246
- DbConnectionInternal.IsEmancipated is documented as requiring the connection lock to avoid races. This assertion reads it without locking, which can make the test nondeterministic if other pool activity overlaps.
DbConnectionInternal connection = CheckOutAndAbandonOwner(pool);
CollectAbandonedOwners();
Assert.True(connection.IsEmancipated);
Assert.Equal(0, pool.IdleCount);
src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs:325
- DbConnectionInternal.IsEmancipated should be read while holding a lock on the internal connection (per its implementation notes) to avoid races with pool bookkeeping. This assertion reads it without locking.
CollectAbandonedOwners();
Assert.True(leaked.IsEmancipated);
src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs:453
- DbConnectionInternal.IsEmancipated is documented as only safe to read while holding a lock on the connection (it can race PrePush/PostPop). This assertion reads it without locking, which can make the test flaky under concurrent pool activity (e.g., background cleanup/reclaim).
Assert.True(leaked.IsEmancipated);
src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs:226
- DbConnectionInternal.IsEmancipated is only safe to read while holding a lock on the connection (per its implementation notes). This test asserts it without locking, which can race with pool reclaim/return code paths and become flaky.
This issue also appears in the following locations of the same file:
- line 231
- line 243
- line 323
DbConnectionInternal connection = CheckOutAndAbandonOwner(pool);
CollectAbandonedOwners();
Assert.True(connection.IsEmancipated);
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs:1507
- The comment says the emancipated-connection sweep "allocates a snapshot", but ReclaimEmancipatedConnections now walks ConnectionPoolSlots via a struct enumerator and only allocates a List when it actually finds emancipated connections. This is now misleading about the sweep’s allocation behavior.
// This is deliberately confined to the slow path: it is O(MaxPoolSize) and
// allocates a snapshot, so it must not run on the hot acquire path.
cheenamalhotra
left a comment
There was a problem hiding this comment.
Design rationale is sound and the parity with WaitHandleDbConnectionPool is right. A few things to address before this goes in — see inline. The Shutdown one and the perf question are the two I'd like resolved; the rest are quick.
Also: please file tracked issues for the four "deliberately out of scope" items (especially reclaim-before-physical-connect and reclaim in Clear()), and one for the activeSoftConnections drift the parity test pins. A PR body isn't a backlog.
| // after the drain below has already passed it. | ||
| try | ||
| { | ||
| Reclaimer.Dispose(); |
There was a problem hiding this comment.
ITimer.Dispose() doesn't wait for an in-flight callback, so this doesn't close the race the comment claims to close. A sweep already past the IsRunning check in OnSweepCallback keeps running and can TryWrite a reclaimed connection into a channel the drain below has already passed — that connection is then never disposed.
Either make it true (re-check state before routing in the reclaim path, or drain again after disposing) or soften the comment to "narrows the window".
| // slots, so at MaxPoolSize every subsequent request would otherwise time out | ||
| // forever. WaitHandleDbConnectionPool performs the same sweep before waiting. | ||
| // This is deliberately confined to the slow path: it is O(MaxPoolSize) and | ||
| // allocates a snapshot, so it must not run on the hot acquire path. |
There was a problem hiding this comment.
Stale — it no longer allocates a snapshot after the enumerator change in this same PR.
| internal bool ReclaimEmancipatedConnections() | ||
| { | ||
| SqlClientEventSource.Log.TryPoolerTraceEvent( | ||
| "ChannelDbConnectionPool.ReclaimEmancipatedConnections | INFO | {0}, Sweeping for emancipated connections.", Id); |
There was a problem hiding this comment.
This fires once per second per blocked pool even when the sweep finds nothing. Move it below the reclaimed is null check, or fold it into a single summary trace that reports the count.
| bool locked = false; | ||
| try | ||
| { | ||
| Monitor.TryEnter(connection, ref locked); |
There was a problem hiding this comment.
Legacy does this scan under lock (_objectList); this walk takes no collection-level lock, so a slot can be removed or replaced concurrently. I believe it's safe today (an emancipated connection is checked out, so nothing else removes it), but please state that reasoning here — it's the kind of invariant that quietly stops being true.
| /// interval than the legacy background cadence while doing strictly less work when idle. | ||
| /// </para> | ||
| /// </summary> | ||
| internal static readonly TimeSpan SweepInterval = TimeSpan.FromSeconds(1); |
There was a problem hiding this comment.
For a busy app sitting at MaxPoolSize, "someone is parked" is steady state, not an exception — so this is a permanent 1/sec O(MaxPoolSize) walk doing Monitor.TryEnter on up to 100 connections whose monitors the hot path is also taking in PrePush/PostPop. TryEnter won't block anyone, but it does contend on those sync blocks.
The PR lists the ConnectionPoolChurnRunner MaxPoolSize parameter as out of scope; I'd rather it were in scope here, or at minimum post a measurement on a saturated pool. This is the one change that could regress healthy applications in order to help leaky ones.
| { | ||
| lock (_lock) | ||
| { | ||
| if (_parkedWaiters > 0) |
There was a problem hiding this comment.
This silently swallows an unbalanced ExitParkedWait. Add a Debug.Assert(_parkedWaiters > 0) so a missing EnterParkedWait surfaces in test runs instead of just under-counting.
|
|
||
| lock (_lock) | ||
| { | ||
| // Re-check rather than re-arming unconditionally: ExitParkedWait may have disarmed |
There was a problem hiding this comment.
The channel has synchronous completions, so DeactivateAndRouteConnection inside the sweep can inline a parked caller's continuation on the timer thread. Since the re-arm below happens only after the sweep returns, a slow continuation delays the next sweep. Worth a comment.
| /// reclaims the connection and wakes the caller. | ||
| /// </summary> | ||
| [Fact] | ||
| public async Task ParkedCaller_IsWokenBySweep_WhenConnectionIsEmancipatedAfterParking() |
There was a problem hiding this comment.
Please cover the sync path too. This is the test that proves the feature works, and it only exercises ReadAsync — repo guidance is explicit that sync and async both need coverage, and here they're genuinely different code paths.
Related: on the sync path EnterParkedWait is called before ReadChannelSyncOverAsync takes the process-wide sync-over-async semaphore, so a caller counts as "parked" while it's still queued for that semaphore rather than on the idle channel. Harmless (it only arms the timer early), but call it out in a comment so nobody reads ParkedWaiters as an exact channel-waiter count.
Layer 2 of the pool V2 stack:
main-> #4504 -> #4529 -> #4537. Supersedes #4490, whose commit is the first of the four here.The gap
ChannelDbConnectionPoolnever reclaimed emancipated connections. A connection becomes emancipated when its owningSqlConnectionis collected without being closed.WaitHandleDbConnectionPoolsweeps for these in four places; the channel pool did so in none, so a leaked connection held its pool slot forever.The first commit adds the sweep on the caller's own thread, right before it parks on the idle channel. That alone leaves a hole: emancipation only becomes observable after a GC, and if that GC lands after the caller has parked, nothing sweeps again. Every caller is blocked on the channel, so the pool stays saturated until an unrelated caller arrives and runs its own inline sweep. With all callers parked, that never happens and they all fail on their connect timeout.
Why not sweep on the parked caller's thread
Channels guarantee FIFO delivery to
ReadAsynccallers, which the pool relies on for fairness. A caller that cancelled its read to re-sweep would rejoin at the back of the queue behind callers that arrived later. So the legacy pattern (bounded wait, wake, re-sweep, re-wait) does not port, and the trigger has to come from off-thread.Why not fold this into
PoolPrunerA merged timer has to run at the faster of the two cadences. The prune interval is derived from the idle timeout and stretches to 288s at the default, so a 1s maintenance tick would multiply prune-driven ticks by ~28x. Timer count is nearly free; armed ticks are what cost. A disarmed timer is ~100 bytes and is not in the timer queue's list, so it does not lengthen any tick.
PoolPruneralso has three coverage holes for this purpose: it is null whenMinPoolSize >= MaxPoolSizeorIdleTimeout == 0, and it disarms whenCount <= MinPoolSize. Those are exactly the configurations where a leaked slot hurts most.What this adds
PoolReclaimer: a demand-driven timer. Callers register around their parked wait, it arms on the first registration and disarms on the last, so a pool that is not blocking pays nothing. It is constructed for every pool configuration.ADP.UnsafeCreateTimerso it does not capture the execution context of whichever caller happens to park first, which would otherwise pin that caller's async locals for the lifetime of the pool.Shutdown, before the drain, so an in-flight sweep cannot route a connection back into a channel the drain has already passed.Cadence
1s. The legacy pool's background reclaim rides a randomized 2-4 minute cleanup wait, which is too slow to rescue a caller inside a 15s connect timeout; legacy effectively depends on its inline sweeps instead. Sweeping only while callers are parked is what makes the tighter cadence affordable, since an idle pool does no work at all.
Allocation
ConnectionPoolSlots.Snapshot()copied every occupied slot into aListsized to the pool's full capacity. The copy bought nothing: the backing array is fixed-capacity and never reallocated, and the snapshot was just the same per-slot volatile reads an in-place walk does, so it gave no consistency beyond the best-effort one already documented. It cost a list per sweep even when nothing was emancipated, ~800 bytes at the defaultMaxPoolSize=100. Tolerable when the sweep only ran inline; less so at once a second while callers are parked. Replaced with a struct enumerator, so the sweep allocates nothing and the backing array stays private.Metrics and traces
Now that this sits on #4504, the metrics seam exists, so the sweep emits
ReclaimedConnectionRequestthe way the legacy pool does. Previouslynumber-of-reclaimed-connectionsalways read zero under pool V2, which made a leaking application look healthy on exactly the counter that would have identified it. The reclaim trace call sites are also converted from the legacyprov-prefixed form to #4504's new pool trace message format.Tests
ChannelDbConnectionPoolReclaimTimerTest: construction across pool configurations, arm/disarm transitions, re-arm after a full drain, the disarmed no-op path, shutdown disposal, and an end-to-end test where a caller parks, the owner is collected only afterward, and the sweep wakes it. Verified to fail without the fix.DbConnectionPoolInstrumentationTestasserting both pool implementations report identical counters when a leaked connection is reclaimed. It drives the sweep the same way for both, by capping the pool at one connection, leaking it and requesting another, so it asserts observable behavior rather than an internal entry point.That parity test pins one shared quirk worth calling out in review: reclamation emits no soft disconnect, so
activeSoftConnectionsdrifts up by one for every leaked connection. That is long-standingWaitHandlebehavior, not something introduced here, and both pools now agree on it exactly.Validation
ConnectionPoolfilter: 343 passed, run 8 consecutive times with no failures. Only net9.0 and net8.0 were built locally; net462 was not.Two pre-existing local issues are worth knowing about when validating this, both of which reproduce with these changes reverted:
SimulatedServerTests.ConnectionEnhancedRoutingTests.ServerDoesNotRoutehangs the full unit suite on macOS, andConnectionTests.IntegratedAuthConnectionTestfails with an SSPI error.Deliberately out of scope
MaxPoolSize. Today a leaky app onMaxPoolSize=100pays 99 extra physical connects before the first sweep.Clear(). It only drains the idle channel, so a leaked connection keeps its slot.MaxPoolSizeparameter onConnectionPoolChurnRunnerto guard hot-path allocations, and a leak-recovery runner measuring time-to-open atMaxPoolSizeafter leaking connections.