Reclaim emancipated connections in ChannelDbConnectionPool - #4490
Conversation
There was a problem hiding this comment.
Pull request overview
Closes the remaining behavioral and observability parity gaps between ChannelDbConnectionPool (V2) and WaitHandleDbConnectionPool (V1), primarily around reclaiming emancipated connections, emitting pool metrics, reporting Count consistently, and allowing async opens to complete synchronously on a warm pool when safe.
Changes:
- Add an emancipated-connection reclamation sweep on the V2 slow acquisition path to prevent permanent pool-slot leaks at
MaxPoolSize. - Wire up V2 pool metrics (pooled/free/active connections and connect/disconnect counters) and align
Countsemantics with V1 via a new tracked connection count. - Improve V2 async acquisition parity by attempting a non-blocking idle-channel hit before enqueuing work (excluding transactional requests), and update tests to run under both pool versions with proper isolation.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs | Fix test reliability under new synchronous-completion behavior and prevent GC-induced “emancipated” reclamation from invalidating pool-exhaustion tests. |
| src/Microsoft.Data.SqlClient/tests/ManualTests/TracingTests/MetricsTest.cs | Parameterize pooled-connection metrics validation across V1/V2 via a pool-version scope helper. |
| src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ParameterTest/TvpTest.cs | Fix Task.Factory.StartNew async-lambda misuse by Unwrap() so failures/timeout behavior are observed correctly. |
| src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectivityTests/ConnectivityTest.cs | Parameterize resiliency test across pool versions with isolation via the new scope helper. |
| src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/ConnectionPoolTest.cs | Run emancipated-reclaim and max-pool-wait tests under both pool versions using a shared provider and pool-version scope. |
| src/Microsoft.Data.SqlClient/tests/Common/ConnectionPoolVersionScope.cs | New RAII helper to toggle UseConnectionPoolV2 and clear pools on entry/exit to prevent cross-test implementation leakage. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IdleConnectionChannel.cs | Emit free-connection metric transitions at the idle-channel choke point. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs | Track actual connection count (distinct from reservations) and add a best-effort snapshot API for infrequent bookkeeping. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs | Align Count with V1, add idle-channel async fast path, add emancipated reclamation sweep, and wire up pooled/soft/hard metric emission. |
04b8001 to
ca8079b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs: Generated file
Suppressed comments (1)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs:1342
- ReclaimEmancipatedConnections logs reclaimed connections but does not emit the corresponding metrics counter. WaitHandleDbConnectionPool.ReclaimEmancipatedObjects calls SqlClientDiagnostics.Metrics.ReclaimedConnectionRequest() for each reclaimed connection; missing this in ChannelDbConnectionPool means reclaimed-connection metrics remain incorrect under V2.
SqlClientEventSource.Log.TryPoolerTraceEvent(
"<prov.DbConnectionPool.ReclaimEmancipatedObjects|RES|CPOOL> {0}, Connection {1}, Reclaiming.",
Id,
connection.ObjectID);
connection.DetachCurrentTransactionIfEnded();
DeactivateAndRouteConnection(connection);
| // Use 3-phase synchronization so task1 gets AND returns before task2 requests. | ||
| // This ensures the connection is back in the transacted pool for task2 to reuse. | ||
| using var task1Returned = new ManualResetEventSlim(false); | ||
| using var task2Done = new ManualResetEventSlim(false); |
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| namespace Microsoft.Data.SqlClient.Tests.Common; | ||
|
|
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>
ca8079b to
6ccd63d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs:1457
- ChannelDbConnectionPool's emancipated-connection sweep reclaims connections but doesn't record the corresponding metrics event. WaitHandleDbConnectionPool increments
SqlClientDiagnostics.Metrics.ReclaimedConnectionRequest()for each reclaimed connection (WaitHandleDbConnectionPool.cs:1534), so the channel pool will under-report reclaimed connections after this change.
connection.DetachCurrentTransactionIfEnded();
DeactivateAndRouteConnection(connection);
src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs:258
- These tests now keep the pool-filling owners alive to avoid emancipation/reclamation affecting the pool-exhaustion behavior, but
GetConnectionAsyncMaxPoolSize_ShouldReuseAfterConnectionReleasedin the same file still fills the pool withnew SqlConnection()instances that aren't rooted. With reclamation implemented, that test can become GC-sensitive/flaky for the same reason.
// The owning connections must stay reachable for the duration of the test. If they were
// collected, their internal connections would become emancipated and the pool would be
// entitled to reclaim them, which would defeat the pool-exhaustion this test relies on.
List<SqlConnection> owningConnections = new();
Stacked on #4487 (
dev/automation/channel-pool-transactions). Review only the top commit; the rest is #4487.The remaining parity fixes I found (metrics,
Countsemantics, async idle fast path) are stacked on top of this one in a follow-up PR, so this one stays focused on reclamation.Why
I ran the whole test suite against both pool implementations and diffed the results, plus built a standalone differential harness that exercises pool semantics the suite doesn't cover. This was the most serious of the gaps it turned up.
A
SqlConnectionthat 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.WaitHandleDbConnectionPoolsweeps for these before waiting for a free connection;ChannelDbConnectionPooldid not. So an emancipated connection permanently occupied a pool slot, and atMaxPoolSizeevery subsequentOpentimed out — forever, not just once. Leaking a single connection was enough to eventually wedge the whole pool.What
GetInternalConnectionnow performs the same sweep just before parking on the idle channel. It's deliberately confined to the slow path: it's O(MaxPoolSize) and allocates a snapshot, so it must not run on the hot acquire path.A couple of details worth calling out for review:
Monitor.TryEnterrather thanEnter.IsEmancipatedhas to be read under that lock to avoid racingPrePush/PostPop, but a connection that is currently locked is being actively handed out or returned and therefore isn't emancipated anyway — so skipping it costs nothing and keeps the sweep from blocking the caller.PrePushhappens under the lock. Deactivation can make server round trips, so it's deferred until all locks are released.ReturnInternalConnectionintoDeactivateAndRouteConnectionso reclamation can share it. Reclamation can't just callReturnInternalConnection, because it has already done thePrePushand there's no owning object left to validate against.Tests
ConnectionPoolVersionScopehelper. It flips the switch and clears all pools on entry and exit — the clearing matters because a pool binds to its implementation at creation time, so without it V2 pools leak into later tests.ReclaimEmancipatedOnOpenTestandMaxPoolWaitForConnectionTestby pool version.ReclaimEmancipatedOnOpenTestfails againstChannelDbConnectionPoolwithout this fix and passes with it.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 also what they actually meant.Verification
Ran all three suites under both pools on net9.0/managed SNI against SQL Server. With the full stack applied the failure sets are identical between V1 and V2 apart from
TestDefaultAppContextSwitchValues, which necessarily fails when the switch is globally on.For this PR in isolation, the pool, transaction, resiliency and metrics test classes all pass with the switch in its default (off) state, and
ReclaimEmancipatedOnOpenTestpasses explicitly under both pool versions.The pre-existing failures on my box are environmental (no MSDTC, no SQL CLR/UDT, Windows-only CNG/CSP and named pipe tests).
Checklist