From 6ccd63d898b80e88f602477e8541486baff76958 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 4 Aug 2026 15:17:25 -0700 Subject: [PATCH] Reclaim emancipated connections in ChannelDbConnectionPool 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> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 87 +++++++++++++++++++ .../ConnectionPool/ConnectionPoolSlots.cs | 23 +++++ .../Common/ConnectionPoolVersionScope.cs | 62 +++++++++++++ .../ConnectionPoolTest/ConnectionPoolTest.cs | 15 ++-- .../ChannelDbConnectionPoolTest.cs | 31 ++++++- 5 files changed, 209 insertions(+), 9 deletions(-) create mode 100644 src/Microsoft.Data.SqlClient/tests/Common/ConnectionPoolVersionScope.cs diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 9f75c50c1a..d0aaf0dad6 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -3,6 +3,7 @@ // See the LICENSE file in the project root for more information. using System; using System.Collections.Concurrent; +using System.Collections.Generic; using System.Collections.ObjectModel; using System.Data.Common; using System.Diagnostics; @@ -498,6 +499,19 @@ public void ReturnInternalConnection(DbConnectionInternal connection, DbConnecti { ValidateOwnershipAndSetPoolingState(connection, owningObject); + DeactivateAndRouteConnection(connection); + } + + /// + /// Deactivates a connection that is already marked as owned by the pool (via + /// ) and routes it to the idle channel, the + /// transacted pool, stasis, or destruction as appropriate. Shared by the normal return path + /// and by emancipated connection reclamation, which has already performed the + /// PrePush itself and must not re-validate ownership. + /// + /// The connection to deactivate and route. + private void DeactivateAndRouteConnection(DbConnectionInternal connection) + { SqlClientEventSource.Log.TryPoolerTraceEvent( " {0}, Connection {1}, Deactivating.", Id, @@ -1340,6 +1354,18 @@ private async Task GetInternalConnection( cancellationToken, timeout); + // Before parking on the idle channel (potentially for the full timeout), sweep + // for connections whose owning SqlConnection was garbage collected without ever + // being closed or disposed. Those "emancipated" connections still occupy pool + // 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. + if (connection is null && ReclaimEmancipatedConnections()) + { + connection = GetIdleConnection(); + } + // If we're at max capacity and couldn't open a connection. Block on the idle channel with a // timeout. Note that Channels guarantee fair FIFO behavior to callers of ReadAsync // (first-come, first-served), which is crucial to us. @@ -1373,6 +1399,67 @@ private async Task GetInternalConnection( return connection; } + /// + /// Reclaims connections whose owning has been garbage collected + /// without being closed or disposed. Such connections are still tracked by the pool but can + /// never be returned by their owner, so without this sweep they would leak pool slots. + /// + /// True if at least one connection was reclaimed; otherwise, false. + private bool ReclaimEmancipatedConnections() + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}", Id); + + List? reclaimed = null; + + foreach (DbConnectionInternal connection in _connectionSlots.Snapshot()) + { + // TryEnter rather than Enter: IsEmancipated must be read under the connection 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. Skipping + // it keeps this sweep from blocking the caller. + bool locked = false; + try + { + Monitor.TryEnter(connection, ref locked); + + if (locked && connection.IsEmancipated) + { + // Do as little as possible under the lock: just claim the connection for the + // pool and defer deactivation (which can make server round trips) until the + // lock is released. + connection.PrePush(null); + (reclaimed ??= new List()).Add(connection); + } + } + finally + { + if (locked) + { + Monitor.Exit(connection); + } + } + } + + if (reclaimed is null) + { + return false; + } + + foreach (DbConnectionInternal connection in reclaimed) + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, Reclaiming.", + Id, + connection.ObjectID); + + connection.DetachCurrentTransactionIfEnded(); + DeactivateAndRouteConnection(connection); + } + + return true; + } + /// /// Performs a blocking synchronous read from the idle connection channel. /// diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs index c9d268fd29..27a39df22b 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs @@ -3,6 +3,7 @@ // See the LICENSE file in the project root for more information. using System; +using System.Collections.Generic; using System.Diagnostics; using System.Threading; using Microsoft.Data.ProviderBase; @@ -190,6 +191,28 @@ internal bool TryReplace(DbConnectionInternal oldConnection, DbConnectionInterna return false; } + /// + /// Returns a point-in-time snapshot of the connections currently tracked by this collection. + /// The snapshot is best-effort: connections may be added or removed while it is being taken, + /// so callers must tolerate entries that have since left the pool. Intended for infrequent + /// bookkeeping passes (e.g. reclaiming emancipated connections), not for hot paths. + /// + internal List Snapshot() + { + List snapshot = new(_connections.Length); + + for (int i = 0; i < _connections.Length; i++) + { + DbConnectionInternal? connection = Volatile.Read(ref _connections[i]); + if (connection is not null) + { + snapshot.Add(connection); + } + } + + return snapshot; + } + /// /// Attempts to reserve a spot in the collection. /// diff --git a/src/Microsoft.Data.SqlClient/tests/Common/ConnectionPoolVersionScope.cs b/src/Microsoft.Data.SqlClient/tests/Common/ConnectionPoolVersionScope.cs new file mode 100644 index 0000000000..73cf652a71 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/Common/ConnectionPoolVersionScope.cs @@ -0,0 +1,62 @@ +// 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; + +/// +/// Selects the connection pool implementation (WaitHandleDbConnectionPool or +/// ChannelDbConnectionPool) for the duration of a test. +/// +/// A pool is bound to an implementation when it is created, so simply flipping the +/// UseConnectionPoolV2 switch is not enough: pools created before the switch was flipped +/// keep their original implementation, and pools created inside the scope would otherwise outlive +/// it and leak the chosen implementation into unrelated tests. This scope therefore clears all +/// pools both on entry and on exit. +/// +/// This follows the RAII pattern; construct it at the start of a test and dispose it at the end. +/// Like , it manipulates global state and enforces a +/// single-instance policy, so it must not be held for longer than necessary. +/// +public sealed class ConnectionPoolVersionScope : IDisposable +{ + private readonly LocalAppContextSwitchesHelper _switches; + + /// + /// Clears all existing pools and selects the requested pool implementation. + /// + /// + /// True to use ChannelDbConnectionPool; false to use WaitHandleDbConnectionPool. + /// + public ConnectionPoolVersionScope(bool usePoolV2) + { + _switches = new LocalAppContextSwitchesHelper(); + + try + { + SqlConnection.ClearAllPools(); + _switches.UseConnectionPoolV2 = usePoolV2; + } + catch + { + _switches.Dispose(); + throw; + } + } + + /// + /// Clears all pools created under the selected implementation and restores the original + /// switch values. + /// + public void Dispose() + { + try + { + SqlConnection.ClearAllPools(); + } + finally + { + _switches.Dispose(); + } + } +} diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/ConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/ConnectionPoolTest.cs index 4b63ff655f..a3ae028a5d 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/ConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/ConnectionPoolTest.cs @@ -150,8 +150,7 @@ public static void AccessTokenConnectionPoolingTest() [ClassData(typeof(ConnectionPoolConnectionStringAndPoolVersionProvider))] public static void ClearAllPoolsTest(string connectionString, bool usePoolV2) { - using LocalAppContextSwitchesHelper switchesHelper = new(); - switchesHelper.UseConnectionPoolV2 = usePoolV2; + using ConnectionPoolVersionScope poolVersion = new(usePoolV2); SqlConnection.ClearAllPools(); Assert.True(0 == ConnectionPoolWrapper.AllConnectionPools().Length, "Pools exist after clearing all pools"); @@ -178,9 +177,11 @@ public static void ClearAllPoolsTest(string connectionString, bool usePoolV2) /// NOTE: 'emancipated' means that the internal connection's SqlConnection has fallen out of scope and has no references, but was not explicitly disposed\closed /// [ConditionalTheory(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] - [ClassData(typeof(ConnectionPoolConnectionStringProvider))] - public static void ReclaimEmancipatedOnOpenTest(string connectionString) + [ClassData(typeof(ConnectionPoolConnectionStringAndPoolVersionProvider))] + public static void ReclaimEmancipatedOnOpenTest(string connectionString, bool usePoolV2) { + using ConnectionPoolVersionScope poolVersion = new(usePoolV2); + string newConnectionString = (new SqlConnectionStringBuilder(connectionString) { MaxPoolSize = 1 }).ConnectionString; SqlConnection.ClearAllPools(); @@ -205,9 +206,11 @@ public static void ReclaimEmancipatedOnOpenTest(string connectionString) /// Tests if, when max pool size is reached, Open() will block until a connection becomes available /// [ConditionalTheory(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] - [ClassData(typeof(ConnectionPoolConnectionStringProvider))] - public static void MaxPoolWaitForConnectionTest(string connectionString) + [ClassData(typeof(ConnectionPoolConnectionStringAndPoolVersionProvider))] + public static void MaxPoolWaitForConnectionTest(string connectionString, bool usePoolV2) { + using ConnectionPoolVersionScope poolVersion = new(usePoolV2); + string newConnectionString = (new SqlConnectionStringBuilder(connectionString) { MaxPoolSize = 1 }).ConnectionString; SqlConnection.ClearAllPools(); diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index 228e949cd0..e352eecdbe 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Concurrent; +using System.Collections.Generic; using System.Data.Common; using System.Threading; using System.Threading.RateLimiting; @@ -251,10 +252,16 @@ public async Task GetConnectionMaxPoolSize_ShouldReuseAfterConnectionReleased() out DbConnectionInternal? firstConnection ); + // 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 owningConnections = new(); for (int i = 1; i < pool.PoolGroupOptions.MaxPoolSize; i++) { + SqlConnection owningConnection = new(); + owningConnections.Add(owningConnection); var completed = pool.TryGetConnection( - new SqlConnection(), + owningConnection, taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? internalConnection @@ -281,6 +288,8 @@ out DbConnectionInternal? extraConnection // Assert Assert.Equal(firstConnection, extraConnection); + + GC.KeepAlive(owningConnections); } /// @@ -350,10 +359,16 @@ public async Task GetConnectionMaxPoolSize_ShouldRespectOrderOfRequest() out DbConnectionInternal? firstConnection ); + // 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 owningConnections = new(); for (int i = 1; i < pool.PoolGroupOptions.MaxPoolSize; i++) { + SqlConnection owningConnection = new(); + owningConnections.Add(owningConnection); var completed = pool.TryGetConnection( - new SqlConnection(), + owningConnection, taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? internalConnection @@ -403,6 +418,8 @@ out DbConnectionInternal? failedConnection // Assert Assert.Equal(firstConnection, recycledConnection); await Assert.ThrowsAsync(async () => await failedTask); + + GC.KeepAlive(owningConnections); } /// @@ -424,10 +441,16 @@ public async Task GetConnectionAsyncMaxPoolSize_ShouldRespectOrderOfRequest() out DbConnectionInternal? firstConnection ); + // 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 owningConnections = new(); for (int i = 1; i < pool.PoolGroupOptions.MaxPoolSize; i++) { + SqlConnection owningConnection = new(); + owningConnections.Add(owningConnection); var completed = pool.TryGetConnection( - new SqlConnection(), + owningConnection, taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? internalConnection @@ -465,6 +488,8 @@ out DbConnectionInternal? failedConnection // Assert Assert.Equal(firstConnection, recycledConnection); await Assert.ThrowsAsync(async () => failedConnection = await failedCompletionSource.Task); + + GC.KeepAlive(owningConnections); } ///