From a94e1b9cbeb4042b3565a2698b3b2976b3da7aa4 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 4 Aug 2026 15:17:25 -0700 Subject: [PATCH 01/27] 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 09fe3a1c2d..73a2d9db31 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; @@ -546,6 +547,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( "ChannelDbConnectionPool.ReturnInternalConnection | INFO | {0}, Connection {1}, Deactivating.", Id, @@ -1480,6 +1494,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. @@ -1526,6 +1552,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 b9465ec93c..7927a65c96 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; @@ -204,6 +205,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 c545685c9e..65d67c7c8e 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; @@ -252,10 +253,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 @@ -282,6 +289,8 @@ out DbConnectionInternal? extraConnection // Assert Assert.Equal(firstConnection, extraConnection); + + GC.KeepAlive(owningConnections); } /// @@ -351,10 +360,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 @@ -404,6 +419,8 @@ out DbConnectionInternal? failedConnection // Assert Assert.Equal(firstConnection, recycledConnection); await Assert.ThrowsAsync(async () => await failedTask); + + GC.KeepAlive(owningConnections); } /// @@ -425,10 +442,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 @@ -466,6 +489,8 @@ out DbConnectionInternal? failedConnection // Assert Assert.Equal(firstConnection, recycledConnection); await Assert.ThrowsAsync(async () => failedConnection = await failedCompletionSource.Task); + + GC.KeepAlive(owningConnections); } /// From 6ea76fe3a44671e47d5cd03f3eda224f6b040a58 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 11 Aug 2026 10:43:01 -0700 Subject: [PATCH 02/27] Sweep for emancipated connections while callers are parked 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> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 54 ++- .../SqlClient/ConnectionPool/PoolReclaimer.cs | 255 ++++++++++++++ ...ChannelDbConnectionPoolReclaimTimerTest.cs | 332 ++++++++++++++++++ 3 files changed, 634 insertions(+), 7 deletions(-) create mode 100644 src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs create mode 100644 src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.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 73a2d9db31..5655a9c6d6 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 @@ -202,6 +202,12 @@ internal ChannelDbConnectionPool( Pruner = new PoolPruner(this, PoolGroupOptions.IdleTimeout); } + // Reclamation, unlike pruning, applies to every pool configuration: a caller can leak a + // connection whatever the pool's sizing or idle timeout, and a fixed-size pool is the + // case where a leaked slot hurts most. The timer inside is created disarmed and only + // runs while callers are parked, so an unblocked pool pays nothing for it. + Reclaimer = new PoolReclaimer(this, _timeProvider); + State = Running; SqlClientEventSource.Log.TryPoolerTraceEvent( @@ -779,6 +785,19 @@ public void Shutdown() "ChannelDbConnectionPool.Shutdown | INFO | {0}, Pruner.Dispose threw, continuing shutdown: {1}", Id, ex); } + // Stop the background reclaim sweep before draining, for the same reason as the pruning + // timer: an in-flight sweep must not route a reclaimed connection back into the channel + // after the drain below has already passed it. + try + { + Reclaimer.Dispose(); + } + catch (Exception ex) + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Reclaimer.Dispose threw, continuing shutdown: {1}", Id, ex); + } + // Dispose the error state so its exit timer is released. Otherwise a timer scheduled // during the blocking period would keep this pool reachable and continue firing // callbacks/logging after shutdown. @@ -1509,13 +1528,25 @@ private async Task GetInternalConnection( // 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. - if (async) - { - connection ??= await _idleChannel.ReadAsync(cancellationToken).ConfigureAwait(false); - } - else + // + // That FIFO guarantee is also why the reclaim sweep above cannot simply be retried + // on this thread while we wait: cancelling the read to re-sweep would send us to + // the back of the queue behind callers that arrived later. Instead we register + // with the reclaimer, which sweeps on a timer for as long as anyone is parked and + // routes anything it reclaims back through this channel, waking us here. + if (connection is null) { - connection ??= ReadChannelSyncOverAsync(cancellationToken); + Reclaimer.EnterParkedWait(); + try + { + connection = async + ? await _idleChannel.ReadAsync(cancellationToken).ConfigureAwait(false) + : ReadChannelSyncOverAsync(cancellationToken); + } + finally + { + Reclaimer.ExitParkedWait(); + } } } catch (OperationCanceledException) @@ -1558,7 +1589,7 @@ private async Task GetInternalConnection( /// 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() + internal bool ReclaimEmancipatedConnections() { SqlClientEventSource.Log.TryPoolerTraceEvent( " {0}", Id); @@ -2000,5 +2031,14 @@ internal void PruneConnections(int count) pruned); } #endregion + + #region Reclamation + /// + /// Drives background sweeps for emancipated connections while callers are parked on the idle + /// channel. Unlike this is always present, because a connection can leak + /// in any pool configuration, including a fixed-size one. + /// + internal PoolReclaimer Reclaimer { get; } + #endregion } } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs new file mode 100644 index 0000000000..71f5e21c2a --- /dev/null +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs @@ -0,0 +1,255 @@ +// 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. +using System; +using System.Threading; +using Microsoft.Data.Common; +using Microsoft.Data.SqlClient.Internal; + +#nullable enable + +namespace Microsoft.Data.SqlClient.ConnectionPool +{ + /// + /// Drives background sweeps for emancipated connections in + /// while callers are parked waiting for a connection. + /// + /// A connection becomes emancipated when its owning + /// is garbage collected without ever being closed or disposed. The pool sweeps for these inline + /// just before a caller parks on the idle channel, but that single sweep is not enough: + /// emancipation only becomes observable once the garbage collector has collected the owner, which + /// routinely happens after the caller has already parked. Without a background sweep nothing would + /// then reclaim the connection, and every parked caller would wait out its full timeout even + /// though a pool slot was recoverable the whole time. + /// + /// + /// The sweep cannot run on the parked caller's own thread. + /// relies on the FIFO ordering that Channel guarantees to readers, so a parked caller that + /// cancelled its read in order to re-sweep would lose its place in line to callers that arrived + /// later. The sweep therefore runs on a timer callback instead. + /// + /// + /// The timer is demand-driven: it is armed when the first caller parks and disarmed when the last + /// one leaves, so a pool with no blocked callers never wakes the process. This mirrors the + /// approach taken for the connection factory's pruning timer (issue #1881). + /// + /// + internal sealed class PoolReclaimer : IDisposable + { + /// + /// Interval between sweeps while at least one caller is parked. + /// + /// Chosen relative to the connect timeout rather than to any pool sizing option: the value + /// bounds how long a parked caller can wait beyond the point at which its connection became + /// reclaimable, so it only has to be small compared to the default 15 second connect timeout. + /// It is not worth going lower, because emancipation only becomes observable after a garbage + /// collection and sweeping faster than the collector produces new information just burns CPU. + /// + /// + /// For reference, reclaims on its cleanup timer, + /// which runs on a 2-4 minute randomized schedule by default. That is far too slow to rescue + /// a caller inside its connect timeout, so the legacy pool depends on its own inline sweeps + /// instead. Sweeping only while callers are parked lets this pool afford a much tighter + /// interval than the legacy background cadence while doing strictly less work when idle. + /// + /// + internal static readonly TimeSpan SweepInterval = TimeSpan.FromSeconds(1); + + /// + /// The owning connection pool that is swept for emancipated connections. + /// + private readonly ChannelDbConnectionPool _pool; + + /// + /// One-shot timer that triggers a sweep. Re-armed at the end of each callback rather than + /// being created as a periodic timer, so a sweep that runs longer than + /// can never overlap with the next one. + /// + private readonly ITimer _timer; + + /// + /// Guards every state transition of this instance, including all + /// calls. + /// + private readonly object _lock = new(); + + /// + /// Number of callers currently parked waiting on the pool's idle channel. The timer is armed + /// exactly while this is non-zero. + /// + private int _parkedWaiters; + + /// + /// Whether the timer is currently armed. Tracked explicitly rather than being inferred from + /// so an in-flight callback can tell that it was disarmed while + /// it was running and decline to re-arm itself. + /// + private bool _armed; + + /// + /// Whether has run. Once set, the timer is never armed again. + /// + private bool _disposed; + + /// + /// Creates a reclaimer for the given pool. The timer is created disarmed; it is armed on + /// demand by . + /// + /// The owning connection pool. + /// + /// Time source used to create the timer. Tests inject a fake provider so sweeps can be driven + /// deterministically without real waits. + /// + internal PoolReclaimer(ChannelDbConnectionPool pool, TimeProvider timeProvider) + { + _pool = pool; + + // The execution context is deliberately not captured. The timer is armed by whichever + // caller happens to park first, and capturing that caller's context would pin its async + // locals (and any impersonation context) for as long as this pool-scoped timer lives. + _timer = ADP.UnsafeCreateTimer( + timeProvider, + static reclaimer => reclaimer.OnSweepCallback(), + this, + Timeout.InfiniteTimeSpan, + Timeout.InfiniteTimeSpan); + } + + #region Internal test surface + + /// Whether the sweep timer is currently armed. Exposed for unit tests. + internal bool IsTimerEnabled + { + get + { + lock (_lock) + { + return _armed; + } + } + } + + /// Number of callers currently parked. Exposed for unit tests. + internal int ParkedWaiters + { + get + { + lock (_lock) + { + return _parkedWaiters; + } + } + } + + #endregion + + /// + /// Registers a caller that is about to park on the pool's idle channel, arming the sweep + /// timer if this is the only parked caller. Must be paired with + /// in a finally block so a cancelled or timed out wait still releases its registration. + /// + internal void EnterParkedWait() + { + lock (_lock) + { + _parkedWaiters++; + + if (_armed || _disposed) + { + return; + } + + _armed = true; + _timer.Change(SweepInterval, Timeout.InfiniteTimeSpan); + + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, sweep timer started", _pool.Id); + } + } + + /// + /// Unregisters a caller that is no longer parked, disarming the sweep timer once the last + /// one leaves so an unblocked pool does not keep waking the process. + /// + internal void ExitParkedWait() + { + lock (_lock) + { + if (_parkedWaiters > 0) + { + _parkedWaiters--; + } + + if (_parkedWaiters != 0 || !_armed) + { + return; + } + + _armed = false; + _timer.Change(Timeout.InfiniteTimeSpan, Timeout.InfiniteTimeSpan); + + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, sweep timer stopped, no parked waiters", _pool.Id); + } + } + + /// + /// Timer callback that sweeps the pool for emancipated connections and re-arms itself while + /// callers remain parked. Reclaimed connections are routed back to the idle channel, which + /// wakes a parked caller. + /// + internal void OnSweepCallback() + { + lock (_lock) + { + // Disarmed (or disposed) after this callback was already scheduled. + if (!_armed || _disposed) + { + return; + } + } + + // Sweep outside the lock: reclamation deactivates connections, which can make server + // round trips, and must not block EnterParkedWait/ExitParkedWait on the hot path. + try + { + if (_pool.IsRunning) + { + _pool.ReclaimEmancipatedConnections(); + } + } + catch (Exception ex) + { + // A timer callback must never throw: an unhandled exception on the timer's thread + // would tear down the process. A failed sweep is not fatal to the pool, so trace it + // and let the next sweep try again. + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, sweep threw, continuing: {1}", _pool.Id, ex); + } + + lock (_lock) + { + // Re-check rather than re-arming unconditionally: ExitParkedWait may have disarmed + // the timer while the sweep above was running, in which case there is no longer + // anyone to wake and re-arming would resurrect a timer that is meant to be idle. + if (_armed && !_disposed) + { + _timer.Change(SweepInterval, Timeout.InfiniteTimeSpan); + } + } + } + + /// + /// Stops the sweep timer and releases resources. Safe to call multiple times. + /// + public void Dispose() + { + lock (_lock) + { + _disposed = true; + _armed = false; + _timer.Dispose(); + } + } + } +} diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs new file mode 100644 index 0000000000..68864221f9 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs @@ -0,0 +1,332 @@ +// 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. + +using System; +using System.Diagnostics; +using System.Runtime.CompilerServices; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Data.Common.ConnectionString; +using Microsoft.Data.ProviderBase; +using Microsoft.Data.SqlClient.ConnectionPool; +using Microsoft.Extensions.Time.Testing; +using Xunit; + +namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool +{ + /// + /// Unit tests for , the background sweep that reclaims emancipated + /// connections in while callers are parked on the idle + /// channel. + /// + public class ChannelDbConnectionPoolReclaimTimerTest + { + private static readonly SqlConnectionFactory ConnectionFactory = + new ChannelDbConnectionPoolTest.SuccessfulSqlConnectionFactory(); + + #region Helpers + + private static ChannelDbConnectionPool ConstructPool( + int minPoolSize = 0, + int maxPoolSize = 50, + int idleTimeout = 0, + TimeProvider? timeProvider = null) + { + var poolGroupOptions = new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: minPoolSize, + maxPoolSize: maxPoolSize, + creationTimeout: 15, + loadBalanceTimeout: 0, + hasTransactionAffinity: true, + idleTimeout: idleTimeout + ); + var dbConnectionPoolGroup = new DbConnectionPoolGroup( + new SqlConnectionOptions("Data Source=localhost;"), + new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), + poolGroupOptions + ); + return new ChannelDbConnectionPool( + ConnectionFactory, + dbConnectionPoolGroup, + DbConnectionPoolIdentity.NoIdentity, + new DbConnectionPoolProviderInfo(), + connectionCreationRateLimiter: null, + timeProvider: timeProvider + ); + } + + /// + /// Checks out a connection and abandons its owning , returning + /// only the internal connection. Marked so the + /// owner's stack slot is guaranteed to be gone when the caller collects: in Debug builds + /// locals stay alive to the end of their enclosing method, so the owner has to be confined + /// to a frame that has already been popped. + /// + [MethodImpl(MethodImplOptions.NoInlining)] + private static DbConnectionInternal CheckOutAndAbandonOwner(ChannelDbConnectionPool pool) + { + SqlConnection owner = new(); + bool completed = pool.TryGetConnection( + owner, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? connection); + + Assert.True(completed); + Assert.NotNull(connection); + return connection!; + } + + /// + /// Forces collection of an abandoned owner so its connection becomes emancipated. + /// + private static void CollectAbandonedOwners() + { + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + } + + /// + /// Spins until holds, failing the test if it does not happen + /// within a generous timeout. Used to observe a background caller reaching its parked wait, + /// which is inherently a cross-thread transition and cannot be awaited directly. + /// + private static void WaitFor(Func condition, string because) + { + Stopwatch stopwatch = Stopwatch.StartNew(); + while (!condition()) + { + Assert.True(stopwatch.Elapsed < TimeSpan.FromSeconds(30), because); + Thread.Sleep(10); + } + } + + /// + /// Guarantees the thread pool can start a queued work item promptly. + /// dispatches its async path through + /// , so a test that needs a caller to actually reach its parked + /// wait depends on a worker being available. Other tests in this assembly block pool threads + /// on sockets and sync-over-async waits, and the pool's own thread injection only adds + /// threads at roughly one per second, which is slow enough to time this test out. + /// The raise is deliberately monotonic and never restored: lowering it again would pull the + /// floor out from under tests running in parallel with this one. + /// + private static void EnsureThreadPoolHeadroom() + { + ThreadPool.GetMinThreads(out int workerThreads, out int completionPortThreads); + int desired = Environment.ProcessorCount * 4; + if (workerThreads < desired) + { + ThreadPool.SetMinThreads(desired, completionPortThreads); + } + } + + #endregion + + /// + /// The reclaimer must exist for every pool configuration. A connection can be leaked + /// regardless of sizing or idle timeout, and a fixed-size pool (where is deliberately null) is the configuration where + /// a permanently occupied slot hurts most. + /// + [Theory] + [InlineData(0, 50, 300)] // Growable pool with idle reclamation: pruner also present. + [InlineData(5, 5, 300)] // Fixed-size pool: no pruner. + [InlineData(0, 50, 0)] // Idle reclamation disabled: no pruner. + public void Reclaimer_IsConstructedForEveryPoolConfiguration(int minPoolSize, int maxPoolSize, int idleTimeout) + { + var pool = ConstructPool(minPoolSize: minPoolSize, maxPoolSize: maxPoolSize, idleTimeout: idleTimeout); + + Assert.NotNull(pool.Reclaimer); + Assert.False(pool.Reclaimer.IsTimerEnabled); + Assert.Equal(0, pool.Reclaimer.ParkedWaiters); + } + + /// + /// The timer is demand-driven: armed by the first parked caller and disarmed by the last one + /// to leave, so a pool with no blocked callers never wakes the process. + /// + [Fact] + public void EnterAndExitParkedWait_ArmsAndDisarmsTimer() + { + var pool = ConstructPool(); + PoolReclaimer reclaimer = pool.Reclaimer; + + reclaimer.EnterParkedWait(); + Assert.True(reclaimer.IsTimerEnabled); + Assert.Equal(1, reclaimer.ParkedWaiters); + + reclaimer.ExitParkedWait(); + Assert.False(reclaimer.IsTimerEnabled); + Assert.Equal(0, reclaimer.ParkedWaiters); + } + + /// + /// With several callers parked, the timer stays armed until the last one leaves. Disarming + /// on the first departure would strand the callers still waiting. + /// + [Fact] + public void ExitParkedWait_WithOtherWaitersRemaining_KeepsTimerArmed() + { + var pool = ConstructPool(); + PoolReclaimer reclaimer = pool.Reclaimer; + + reclaimer.EnterParkedWait(); + reclaimer.EnterParkedWait(); + reclaimer.EnterParkedWait(); + Assert.Equal(3, reclaimer.ParkedWaiters); + + reclaimer.ExitParkedWait(); + Assert.True(reclaimer.IsTimerEnabled); + reclaimer.ExitParkedWait(); + Assert.True(reclaimer.IsTimerEnabled); + + reclaimer.ExitParkedWait(); + Assert.False(reclaimer.IsTimerEnabled); + } + + /// + /// After the timer has been disarmed and re-armed, it must still fire. This guards the + /// one-shot re-arm bookkeeping, where losing the armed flag would silently stop all + /// subsequent sweeps. + /// + [Fact] + public void EnterParkedWait_AfterFullDrain_ReArmsTimer() + { + var pool = ConstructPool(); + PoolReclaimer reclaimer = pool.Reclaimer; + + reclaimer.EnterParkedWait(); + reclaimer.ExitParkedWait(); + Assert.False(reclaimer.IsTimerEnabled); + + reclaimer.EnterParkedWait(); + Assert.True(reclaimer.IsTimerEnabled); + } + + /// + /// A sweep callback that was already scheduled when the last caller left must not run. The + /// pool is no longer blocked, so there is nobody to wake and no reason to pay for the sweep. + /// + [Fact] + public void OnSweepCallback_WhenDisarmed_DoesNotSweep() + { + var pool = ConstructPool(maxPoolSize: 1); + DbConnectionInternal connection = CheckOutAndAbandonOwner(pool); + CollectAbandonedOwners(); + Assert.True(connection.IsEmancipated); + + // Never armed, so this stands in for a callback that raced with the last exit. + pool.Reclaimer.OnSweepCallback(); + + Assert.Equal(0, pool.IdleCount); + Assert.True(connection.IsEmancipated); + } + + /// + /// A sweep reclaims an emancipated connection and routes it back to the idle channel, which + /// is what makes it visible to a parked caller. + /// + [Fact] + public void OnSweepCallback_WhenArmed_ReclaimsEmancipatedConnection() + { + var pool = ConstructPool(maxPoolSize: 1); + DbConnectionInternal connection = CheckOutAndAbandonOwner(pool); + CollectAbandonedOwners(); + Assert.True(connection.IsEmancipated); + Assert.Equal(0, pool.IdleCount); + + pool.Reclaimer.EnterParkedWait(); + try + { + pool.Reclaimer.OnSweepCallback(); + } + finally + { + pool.Reclaimer.ExitParkedWait(); + } + + Assert.Equal(1, pool.IdleCount); + } + + /// + /// Shutting the pool down releases the timer. Otherwise a scheduled sweep would keep the + /// pool reachable and could route a connection back into the channel after the shutdown + /// drain had already passed it. + /// + [Fact] + public void Shutdown_DisposesReclaimer() + { + var pool = ConstructPool(); + pool.Reclaimer.EnterParkedWait(); + Assert.True(pool.Reclaimer.IsTimerEnabled); + + pool.Shutdown(); + + Assert.False(pool.Reclaimer.IsTimerEnabled); + + // Arming after disposal must be a no-op rather than resurrecting the timer. + pool.Reclaimer.EnterParkedWait(); + Assert.False(pool.Reclaimer.IsTimerEnabled); + } + + /// + /// End-to-end coverage of the gap this feature closes. A caller parks on an exhausted pool, + /// and only afterwards does the owner of the sole connection become collectable. The inline + /// sweep the caller performed before parking could not have seen that, so without a + /// background sweep the caller would wait out its entire timeout even though a slot was + /// recoverable. Advancing the injected fires the sweep, which + /// reclaims the connection and wakes the caller. + /// + [Fact] + public async Task ParkedCaller_IsWokenBySweep_WhenConnectionIsEmancipatedAfterParking() + { + EnsureThreadPoolHeadroom(); + + var fakeTime = new FakeTimeProvider(); + var pool = ConstructPool(maxPoolSize: 1, timeProvider: fakeTime); + + // Occupy the pool's only slot and abandon the owner. The connection is not yet + // collected, so it is not yet emancipated. + DbConnectionInternal leaked = CheckOutAndAbandonOwner(pool); + Assert.Equal(1, pool.Count); + + // A second caller finds the pool exhausted and parks. Its inline sweep runs before the + // collection below, so it finds nothing. The async path is used deliberately: the sync + // path first takes a process-wide sync-over-async semaphore, so a sync caller could sit + // behind unrelated tests rather than on the idle channel this test is exercising. + SqlConnection waitingOwner = new(); + TaskCompletionSource parked = new(); + pool.TryGetConnection( + waitingOwner, + parked, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(30)), + out DbConnectionInternal? immediate); + + Assert.Null(immediate); + WaitFor(() => pool.Reclaimer.ParkedWaiters == 1, "the second caller should park on the idle channel"); + Assert.True(pool.Reclaimer.IsTimerEnabled); + Assert.False(parked.Task.IsCompleted); + + // Only now does the abandoned owner become collectable, which is precisely the case the + // caller's own inline sweep cannot cover. + CollectAbandonedOwners(); + Assert.True(leaked.IsEmancipated); + + // FakeTimeProvider invokes timer callbacks synchronously on Advance. + fakeTime.Advance(PoolReclaimer.SweepInterval); + + DbConnectionInternal? result = await parked.Task; + + Assert.NotNull(result); + Assert.Equal(leaked, result); + Assert.Equal(0, pool.Reclaimer.ParkedWaiters); + Assert.False(pool.Reclaimer.IsTimerEnabled); + + GC.KeepAlive(waitingOwner); + } + } +} From 151b4e0145bcc8bfcd5c959af232063e7be8a523 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 12 Aug 2026 11:14:12 -0700 Subject: [PATCH 03/27] Walk pool slots without allocating a snapshot list 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> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 2 +- .../ConnectionPool/ConnectionPoolSlots.cs | 49 ++++++++++++++----- 2 files changed, 37 insertions(+), 14 deletions(-) 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 5655a9c6d6..85cbbba3f2 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 @@ -1596,7 +1596,7 @@ internal bool ReclaimEmancipatedConnections() List? reclaimed = null; - foreach (DbConnectionInternal connection in _connectionSlots.Snapshot()) + foreach (DbConnectionInternal connection in _connectionSlots) { // 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 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 7927a65c96..66dc1aae1b 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,7 +3,6 @@ // 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; @@ -206,25 +205,49 @@ internal bool TryReplace(DbConnectionInternal oldConnection, DbConnectionInterna } /// - /// 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. + /// Enumerates the connections currently tracked by this collection without allocating. + /// The enumeration is best-effort: connections may be added or removed while it is in + /// progress, so callers must tolerate entries that have since left the pool, and an entry + /// added during the walk may or may not be seen. This is safe because the backing array has + /// a fixed capacity and is never reallocated; each slot is read individually. Intended for + /// infrequent bookkeeping passes (e.g. reclaiming emancipated connections), not for hot + /// paths. /// - internal List Snapshot() + public Enumerator GetEnumerator() => new(_connections); + + /// + /// A non-allocating enumerator over the occupied slots of a . + /// Declared as a mutable struct and returned by value from so a + /// foreach over the collection allocates nothing; do not copy it into a local. + /// + public struct Enumerator { - List snapshot = new(_connections.Length); + private readonly DbConnectionInternal?[] _connections; + private int _index; - for (int i = 0; i < _connections.Length; i++) + internal Enumerator(DbConnectionInternal?[] connections) + { + _connections = connections; + _index = -1; + Current = null!; + } + + public DbConnectionInternal Current { get; private set; } + + public bool MoveNext() { - DbConnectionInternal? connection = Volatile.Read(ref _connections[i]); - if (connection is not null) + while (++_index < _connections.Length) { - snapshot.Add(connection); + DbConnectionInternal? connection = Volatile.Read(ref _connections[_index]); + if (connection is not null) + { + Current = connection; + return true; + } } - } - return snapshot; + return false; + } } /// From 6cf4e79848783700a8a49940c5aae75551f2ea49 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 13 Aug 2026 14:56:35 -0700 Subject: [PATCH 04/27] Count reclaimed connections and align reclaim traces 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> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 10 ++- .../SqlClient/ConnectionPool/PoolReclaimer.cs | 6 +- ...ChannelDbConnectionPoolReclaimTimerTest.cs | 57 +++++++------ .../DbConnectionPoolInstrumentationTest.cs | 80 +++++++++++++++++++ 4 files changed, 122 insertions(+), 31 deletions(-) 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 85cbbba3f2..f8a10a2532 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 @@ -795,7 +795,7 @@ public void Shutdown() catch (Exception ex) { SqlClientEventSource.Log.TryPoolerTraceEvent( - " {0}, Reclaimer.Dispose threw, continuing shutdown: {1}", Id, ex); + "ChannelDbConnectionPool.Shutdown | INFO | {0}, Reclaimer.Dispose threw, continuing shutdown: {1}", Id, ex); } // Dispose the error state so its exit timer is released. Otherwise a timer scheduled @@ -1592,7 +1592,7 @@ private async Task GetInternalConnection( internal bool ReclaimEmancipatedConnections() { SqlClientEventSource.Log.TryPoolerTraceEvent( - " {0}", Id); + "ChannelDbConnectionPool.ReclaimEmancipatedConnections | INFO | {0}, Sweeping for emancipated connections.", Id); List? reclaimed = null; @@ -1633,10 +1633,14 @@ internal bool ReclaimEmancipatedConnections() foreach (DbConnectionInternal connection in reclaimed) { SqlClientEventSource.Log.TryPoolerTraceEvent( - " {0}, Connection {1}, Reclaiming.", + "ChannelDbConnectionPool.ReclaimEmancipatedConnections | INFO | {0}, Connection {1}, Reclaiming.", Id, connection.ObjectID); + // Matches WaitHandleDbConnectionPool so the number-of-reclaimed-connections counter + // is meaningful under pool V2 as well; it previously always read zero here. + Metrics.ReclaimedConnectionRequest(); + connection.DetachCurrentTransactionIfEnded(); DeactivateAndRouteConnection(connection); } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs index 71f5e21c2a..df0ebdb1c1 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs @@ -163,7 +163,7 @@ internal void EnterParkedWait() _timer.Change(SweepInterval, Timeout.InfiniteTimeSpan); SqlClientEventSource.Log.TryPoolerTraceEvent( - " {0}, sweep timer started", _pool.Id); + "PoolReclaimer.EnterParkedWait | INFO | {0}, Sweep timer started.", _pool.Id); } } @@ -189,7 +189,7 @@ internal void ExitParkedWait() _timer.Change(Timeout.InfiniteTimeSpan, Timeout.InfiniteTimeSpan); SqlClientEventSource.Log.TryPoolerTraceEvent( - " {0}, sweep timer stopped, no parked waiters", _pool.Id); + "PoolReclaimer.ExitParkedWait | INFO | {0}, Sweep timer stopped, no parked waiters.", _pool.Id); } } @@ -224,7 +224,7 @@ internal void OnSweepCallback() // would tear down the process. A failed sweep is not fatal to the pool, so trace it // and let the next sweep try again. SqlClientEventSource.Log.TryPoolerTraceEvent( - " {0}, sweep threw, continuing: {1}", _pool.Id, ex); + "PoolReclaimer.OnSweepCallback | ERR | {0}, Sweep threw, continuing: {1}.", _pool.Id, ex); } lock (_lock) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs index 68864221f9..4053be68e5 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs @@ -5,6 +5,7 @@ using System; using System.Diagnostics; using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; using System.Threading; using System.Threading.Tasks; using Microsoft.Data.Common.ConnectionString; @@ -79,6 +80,31 @@ private static DbConnectionInternal CheckOutAndAbandonOwner(ChannelDbConnectionP return connection!; } + /// + /// Checks out a connection and roots its owner in a rather than a + /// local. The owner stays alive until the caller frees the returned handle, which gives the + /// test exact control over when the connection becomes emancipated. A local cannot do this: + /// in a Debug build its stack slot roots the object for the rest of the enclosing method + /// even after it is assigned null, so the collection would never happen. The checkout runs + /// in its own non-inlined frame so no slot in the caller's frame ever holds the owner. + /// + [MethodImpl(MethodImplOptions.NoInlining)] + private static DbConnectionInternal CheckOutAndRootOwner(ChannelDbConnectionPool pool, out GCHandle ownerRoot) + { + SqlConnection owner = new(); + ownerRoot = GCHandle.Alloc(owner); + + bool completed = pool.TryGetConnection( + owner, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? connection); + + Assert.True(completed); + Assert.NotNull(connection); + return connection!; + } + /// /// Forces collection of an abandoned owner so its connection becomes emancipated. /// @@ -104,26 +130,6 @@ private static void WaitFor(Func condition, string because) } } - /// - /// Guarantees the thread pool can start a queued work item promptly. - /// dispatches its async path through - /// , so a test that needs a caller to actually reach its parked - /// wait depends on a worker being available. Other tests in this assembly block pool threads - /// on sockets and sync-over-async waits, and the pool's own thread injection only adds - /// threads at roughly one per second, which is slow enough to time this test out. - /// The raise is deliberately monotonic and never restored: lowering it again would pull the - /// floor out from under tests running in parallel with this one. - /// - private static void EnsureThreadPoolHeadroom() - { - ThreadPool.GetMinThreads(out int workerThreads, out int completionPortThreads); - int desired = Environment.ProcessorCount * 4; - if (workerThreads < desired) - { - ThreadPool.SetMinThreads(desired, completionPortThreads); - } - } - #endregion /// @@ -284,14 +290,14 @@ public void Shutdown_DisposesReclaimer() [Fact] public async Task ParkedCaller_IsWokenBySweep_WhenConnectionIsEmancipatedAfterParking() { - EnsureThreadPoolHeadroom(); - var fakeTime = new FakeTimeProvider(); var pool = ConstructPool(maxPoolSize: 1, timeProvider: fakeTime); - // Occupy the pool's only slot and abandon the owner. The connection is not yet - // collected, so it is not yet emancipated. - DbConnectionInternal leaked = CheckOutAndAbandonOwner(pool); + // Occupy the pool's only slot. The owner is deliberately kept rooted until after the + // second caller has parked: a collection triggered by a test running in parallel would + // otherwise emancipate it early, letting the second caller's inline sweep succeed so it + // never parks and the case under test never arises. + DbConnectionInternal leaked = CheckOutAndRootOwner(pool, out GCHandle leakedOwnerRoot); Assert.Equal(1, pool.Count); // A second caller finds the pool exhausted and parks. Its inline sweep runs before the @@ -313,6 +319,7 @@ public async Task ParkedCaller_IsWokenBySweep_WhenConnectionIsEmancipatedAfterPa // Only now does the abandoned owner become collectable, which is precisely the case the // caller's own inline sweep cannot cover. + leakedOwnerRoot.Free(); CollectAbandonedOwners(); Assert.True(leaked.IsEmancipated); diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs index 25d98b351e..c2611dce9d 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs @@ -6,6 +6,7 @@ using System.Collections.Generic; using System.Data.Common; using System.Linq; +using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; using System.Transactions; @@ -733,6 +734,85 @@ public void IdleSweepOfTransactionRoot_CountsStasisExitOnReturnToPool() stasisConnections: 0); } + /// + /// Verifies that a connection whose owner was collected without being closed is reclaimed + /// and counted, in both pool implementations. + /// + /// + /// The sweep is triggered the same way for both pools rather than by calling their internal + /// reclaim methods: the pool is capped at a single connection, that connection is leaked, + /// and a second caller then requests one. Each implementation reaches its own reclaim path + /// from that saturated request, so the test asserts observable behavior rather than a + /// specific internal entry point. + /// + [Theory] + [InlineData(PoolImplementation.WaitHandle)] + [InlineData(PoolImplementation.Channel)] + public void EmancipatedConnection_IsReclaimedAndCounted(PoolImplementation implementation) + { + // Arrange + FakeSqlClientMetrics metrics = new(); + IDbConnectionPool pool = ConstructPool( + implementation, + metrics, + new SuccessfulSqlConnectionFactory(metrics), + maxPoolSize: 1); + + DbConnectionInternal leaked = CheckOutAndAbandonOwner(pool); + AssertCounters( + metrics, + hardConnects: 1, + softConnects: 1, + pooledConnections: 1, + activeConnections: 1); + + // The owner is only collectable now that the helper's frame has been popped. + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + Assert.True(leaked.IsEmancipated); + + // Act - the pool's only slot is occupied by a connection nobody can return, so this + // request can only be served by reclaiming it. + Assert.True(pool.TryGetConnection( + new SqlConnection(), + null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? reclaimed)); + + // Assert - the same physical connection was handed out again, with no second connect. + // softDisconnects stays at zero: reclamation never balances the leaked checkout, so + // activeSoftConnections drifts up by one per leaked connection. That is long-standing + // WaitHandle behavior, asserted here to pin the two implementations to the same numbers. + Assert.Same(leaked, reclaimed); + AssertCounters( + metrics, + hardConnects: 1, + softConnects: 2, + softDisconnects: 0, + pooledConnections: 1, + reclaimedConnections: 1, + activeConnections: 1); + } + + /// + /// Checks out a connection and drops the only reference to its owner. Kept in its own + /// non-inlined method so the owner becomes collectable as soon as this frame is popped; a + /// local in the calling test would stay rooted until the end of that method in a debug build. + /// + [MethodImpl(MethodImplOptions.NoInlining)] + private static DbConnectionInternal CheckOutAndAbandonOwner(IDbConnectionPool pool) + { + SqlConnection owner = new(); + Assert.True(pool.TryGetConnection( + owner, + null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? connection)); + Assert.NotNull(connection); + return connection!; + } + #region Test classes /// From 86eed4f149410ba52fb8a2b0c09e66cabf19f82a Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 19 Aug 2026 11:59:17 -0700 Subject: [PATCH 05/27] Address review feedback on reclaim sweep Correct the Shutdown comment: ITimer.Dispose does not wait for an in-flight callback, so disposing the reclaimer only narrows the window. What actually prevents a late sweep from stranding a connection is the ordering below it, so name that instead. Drop the per-sweep entry trace and the per-connection reclaim trace in favor of a single summary trace carrying the count, emitted only when something was reclaimed. The sweep runs once per second for as long as any caller is parked, so an entry trace floods the stream of a pool that is merely busy. Document why the slot walk is safe without a collection-level lock, unlike the legacy pool's scan under lock (_objectList). Assert that ExitParkedWait is balanced, so a park site missing its EnterParkedWait surfaces in test runs rather than silently under-counting. Note that a reclaimed connection can inline a woken caller's continuation on the timer thread, delaying the next sweep, and that ParkedWaiters counts sync callers still queued for the sync-over-async semaphore. Cover the sync park path end to end. It blocks the calling thread inside ReadChannelSyncOverAsync rather than awaiting the channel, so it is a genuinely different path from ReadAsync. Both end-to-end tests were verified to fail with the sweep stubbed out. Also remove a stale comment claiming the sweep allocates a snapshot. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 43 +++++++---- .../SqlClient/ConnectionPool/PoolReclaimer.cs | 27 ++++++- ...ChannelDbConnectionPoolReclaimTimerTest.cs | 77 +++++++++++++++++++ 3 files changed, 133 insertions(+), 14 deletions(-) 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 f8a10a2532..6786d4d80c 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 @@ -785,9 +785,15 @@ public void Shutdown() "ChannelDbConnectionPool.Shutdown | INFO | {0}, Pruner.Dispose threw, continuing shutdown: {1}", Id, ex); } - // Stop the background reclaim sweep before draining, for the same reason as the pruning - // timer: an in-flight sweep must not route a reclaimed connection back into the channel - // after the drain below has already passed it. + // Stop the background reclaim sweep before draining. This only narrows the window: + // ITimer.Dispose does not wait for a callback that is already running, so a sweep can + // still be in flight below. What actually guarantees a late-reclaimed connection is + // disposed rather than stranded is the ordering further down: _idleChannel.Complete() + // runs before the drain, after which TryWrite fails and PutConnectionInIdleChannel + // destroys the connection instead of pooling it. Any write that did succeed necessarily + // happened before Complete(), so the final unbounded drain mops it up. The State check + // in DeactivateAndRouteConnection catches most late sweeps earlier, but State is not + // synchronized against this write, so it cannot be relied on alone. try { Reclaimer.Dispose(); @@ -1518,8 +1524,8 @@ private async Task GetInternalConnection( // 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. + // This is deliberately confined to the slow path: it is O(MaxPoolSize), so it + // must not run on the hot acquire path. if (connection is null && ReclaimEmancipatedConnections()) { connection = GetIdleConnection(); @@ -1591,11 +1597,17 @@ private async Task GetInternalConnection( /// True if at least one connection was reclaimed; otherwise, false. internal bool ReclaimEmancipatedConnections() { - SqlClientEventSource.Log.TryPoolerTraceEvent( - "ChannelDbConnectionPool.ReclaimEmancipatedConnections | INFO | {0}, Sweeping for emancipated connections.", Id); - List? reclaimed = null; + // Unlike WaitHandleDbConnectionPool, which scans under lock (_objectList), this walk + // takes no collection-level lock: the enumerator reads each slot individually, so it can + // observe a slot that was concurrently emptied or refilled. That is safe because of what + // this sweep is looking for. A connection is only emancipated while it is checked out, + // and a checked-out connection is not in the idle channel, so neither the pruner nor + // Clear can remove it underneath us. The only code that can claim it is another sweep, + // which is excluded by the per-connection lock below. A concurrently replaced slot can + // therefore cost this sweep a miss, never a connection resurrected after removal, and a + // miss is picked up by the next sweep. foreach (DbConnectionInternal connection in _connectionSlots) { // TryEnter rather than Enter: IsEmancipated must be read under the connection lock to @@ -1627,16 +1639,21 @@ internal bool ReclaimEmancipatedConnections() if (reclaimed is null) { + // Deliberately silent. This runs once per second for as long as any caller is + // parked, so tracing an empty sweep would flood the trace stream of a pool that is + // merely busy. A sweep that finds nothing is reported by its absence. return false; } + // One summary trace rather than the entry trace plus a line per connection that + // WaitHandleDbConnectionPool emits. + SqlClientEventSource.Log.TryPoolerTraceEvent( + "ChannelDbConnectionPool.ReclaimEmancipatedConnections | INFO | {0}, Reclaimed {1} emancipated connection(s).", + Id, + reclaimed.Count); + foreach (DbConnectionInternal connection in reclaimed) { - SqlClientEventSource.Log.TryPoolerTraceEvent( - "ChannelDbConnectionPool.ReclaimEmancipatedConnections | INFO | {0}, Connection {1}, Reclaiming.", - Id, - connection.ObjectID); - // Matches WaitHandleDbConnectionPool so the number-of-reclaimed-connections counter // is meaningful under pool V2 as well; it previously always read zero here. Metrics.ReclaimedConnectionRequest(); diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs index df0ebdb1c1..f3a314f485 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. using System; +using System.Diagnostics; using System.Threading; using Microsoft.Data.Common; using Microsoft.Data.SqlClient.Internal; @@ -129,7 +130,17 @@ internal bool IsTimerEnabled } } - /// Number of callers currently parked. Exposed for unit tests. + /// + /// Number of callers currently registered as parked. Exposed for unit tests. + /// + /// Not an exact count of callers sitting on the idle channel. On the synchronous path + /// registration happens before ReadChannelSyncOverAsync takes the process-wide + /// sync-over-async semaphore, so a caller counts as parked while it is still queued for that + /// semaphore. The effect is only that the timer arms slightly early, which is harmless: the + /// caller is blocked either way, and a sweep that runs before it reaches the channel simply + /// finds nothing. + /// + /// internal int ParkedWaiters { get @@ -175,6 +186,12 @@ internal void ExitParkedWait() { lock (_lock) { + // An unbalanced call means some park site is missing its EnterParkedWait, which + // would leave the timer armed forever (or, if it under-counted the other way, stop + // sweeping while a caller is still parked). Clamped rather than thrown in release + // builds: a mis-paired registration is not worth failing a connection attempt over. + Debug.Assert(_parkedWaiters > 0, "ExitParkedWait called without a matching EnterParkedWait."); + if (_parkedWaiters > 0) { _parkedWaiters--; @@ -211,6 +228,14 @@ internal void OnSweepCallback() // Sweep outside the lock: reclamation deactivates connections, which can make server // round trips, and must not block EnterParkedWait/ExitParkedWait on the hot path. + // + // This can run for longer than the sweep itself. Routing a reclaimed connection into + // the idle channel completes a parked caller's ReadAsync, and channel completions are + // synchronous, so that caller's continuation can be inlined onto this timer thread. + // Because the re-arm below happens only after this returns, a slow continuation delays + // the next sweep. Acceptable: the delay is bounded by the work a woken caller does + // before its next await, and delaying a sweep only postpones reclaiming a connection + // that is already reclaimable. It does not drop one. try { if (_pool.IsRunning) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs index 4053be68e5..78ca9e4cea 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs @@ -335,5 +335,82 @@ public async Task ParkedCaller_IsWokenBySweep_WhenConnectionIsEmancipatedAfterPa GC.KeepAlive(waitingOwner); } + + /// + /// The synchronous counterpart of the test above. The sync path does not await the idle + /// channel; it blocks the calling thread inside ReadChannelSyncOverAsync, behind a + /// process-wide sync-over-async semaphore. That makes it a genuinely different code path + /// from ReadAsync, so the sweep's ability to wake a parked caller is proven for both. + /// + /// + /// The caller runs on a dedicated thread rather than a pooled one: it blocks for the whole + /// parked wait, and parking it on a thread pool thread would consume a worker for the + /// duration. It also holds a slot in the shared sync-over-async semaphore while parked, + /// which is why the wait is kept as short as possible. + /// + [Fact] + public void ParkedSyncCaller_IsWokenBySweep_WhenConnectionIsEmancipatedAfterParking() + { + var fakeTime = new FakeTimeProvider(); + var pool = ConstructPool(maxPoolSize: 1, timeProvider: fakeTime); + + // Occupy the pool's only slot, keeping the owner rooted so a collection triggered by a + // test running in parallel cannot emancipate it before the second caller has parked. + DbConnectionInternal leaked = CheckOutAndRootOwner(pool, out GCHandle leakedOwnerRoot); + Assert.Equal(1, pool.Count); + + // A second caller finds the pool exhausted and blocks inside the sync read. + DbConnectionInternal? syncResult = null; + Exception? syncFailure = null; + SqlConnection waitingOwner = new(); + Thread caller = new(() => + { + try + { + pool.TryGetConnection( + waitingOwner, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(30)), + out syncResult); + } + catch (Exception ex) + { + syncFailure = ex; + } + }) + { + IsBackground = true, + Name = nameof(ParkedSyncCaller_IsWokenBySweep_WhenConnectionIsEmancipatedAfterParking) + }; + caller.Start(); + + WaitFor(() => pool.Reclaimer.ParkedWaiters == 1, "the sync caller should register as parked"); + Assert.True(pool.Reclaimer.IsTimerEnabled); + + // ParkedWaiters is incremented before the semaphore is taken, so the caller may not be + // on the channel yet. Wait for the blocking read itself, otherwise the sweep below could + // reclaim and route the connection before anyone is there to be woken by it. + WaitFor( + () => (caller.ThreadState & System.Threading.ThreadState.WaitSleepJoin) != 0, + "the sync caller should block inside the channel read"); + + // Only now does the abandoned owner become collectable, which is precisely the case the + // caller's own inline sweep cannot cover. + leakedOwnerRoot.Free(); + CollectAbandonedOwners(); + Assert.True(leaked.IsEmancipated); + + // FakeTimeProvider invokes timer callbacks synchronously on Advance. + fakeTime.Advance(PoolReclaimer.SweepInterval); + + Assert.True(caller.Join(TimeSpan.FromSeconds(30)), "the sync caller should be woken by the sweep"); + Assert.Null(syncFailure); + Assert.NotNull(syncResult); + Assert.Equal(leaked, syncResult); + Assert.Equal(0, pool.Reclaimer.ParkedWaiters); + Assert.False(pool.Reclaimer.IsTimerEnabled); + + GC.KeepAlive(waitingOwner); + } } } From 831737284992ec6dab712fef76e5e675518872b1 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 19 Aug 2026 12:06:13 -0700 Subject: [PATCH 06/27] update comments --- .../ConnectionPool/ChannelDbConnectionPool.cs | 12 +++++------- .../Data/SqlClient/ConnectionPool/PoolReclaimer.cs | 8 -------- 2 files changed, 5 insertions(+), 15 deletions(-) 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 6786d4d80c..b93210a18b 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 @@ -202,10 +202,8 @@ internal ChannelDbConnectionPool( Pruner = new PoolPruner(this, PoolGroupOptions.IdleTimeout); } - // Reclamation, unlike pruning, applies to every pool configuration: a caller can leak a - // connection whatever the pool's sizing or idle timeout, and a fixed-size pool is the - // case where a leaked slot hurts most. The timer inside is created disarmed and only - // runs while callers are parked, so an unblocked pool pays nothing for it. + // Reclamation applies to every pool configuration. The timer inside is created disarmed and only + // runs while pool consumers are parked waiting for a connection.. Reclaimer = new PoolReclaimer(this, _timeProvider); State = Running; @@ -1386,7 +1384,7 @@ private void RemoveConnection(DbConnectionInternal connection) // Removing a connection from the pool opens a free slot. // Write a null to the idle connection channel to wake up a waiter, who can now open a new - // connection. Statement order is important since we have synchronous completions on the channel. + // connection. _idleChannel.TryWrite(null); connection.Dispose(); @@ -1647,9 +1645,9 @@ internal bool ReclaimEmancipatedConnections() // One summary trace rather than the entry trace plus a line per connection that // WaitHandleDbConnectionPool emits. - SqlClientEventSource.Log.TryPoolerTraceEvent( + SqlClientEventSource.Log.TryPoolerTraceEvent( "ChannelDbConnectionPool.ReclaimEmancipatedConnections | INFO | {0}, Reclaimed {1} emancipated connection(s).", - Id, + Id, reclaimed.Count); foreach (DbConnectionInternal connection in reclaimed) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs index f3a314f485..66241b2519 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs @@ -228,14 +228,6 @@ internal void OnSweepCallback() // Sweep outside the lock: reclamation deactivates connections, which can make server // round trips, and must not block EnterParkedWait/ExitParkedWait on the hot path. - // - // This can run for longer than the sweep itself. Routing a reclaimed connection into - // the idle channel completes a parked caller's ReadAsync, and channel completions are - // synchronous, so that caller's continuation can be inlined onto this timer thread. - // Because the re-arm below happens only after this returns, a slow continuation delays - // the next sweep. Acceptable: the delay is bounded by the work a woken caller does - // before its next await, and delaying a sweep only postpones reclaiming a connection - // that is already reclaimable. It does not drop one. try { if (_pool.IsRunning) From aaddedd01091ef029a03e1c01f9901b20946f6eb Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 19 Aug 2026 15:07:17 -0700 Subject: [PATCH 07/27] Route reclamation through ReturnInternalConnection Reclamation returned a leaked connection by calling the deactivate/route helper directly, skipping the soft disconnect that ReturnInternalConnection emits. A checkout is counted, so a leaked connection was counted as checked out forever and counted again each time it was re-vended, drifting activeSoftConnections up on every leak. Routing the return through ReturnInternalConnection instead of duplicating part of it keeps the accounting in one place. That means the sweep can no longer claim the connection itself, because ReturnInternalConnection re-takes the connection lock to call PrePush and PrePush throws on a second push. Gate the sweep to close that gap. The sweep now selects under the per- connection lock and returns after releasing it, so something has to guarantee nothing claims the connection in between. Of the paths that move the pooled count, only a second sweep can reach an emancipated connection: a handout needs it in the idle channel, a normal return needs a live owner, DelegatedTransactionEnded is gated on a flag IsEmancipated already excludes, and Clear only drains the idle channel. So excluding concurrent sweeps is sufficient. TryEnter rather than Enter, so no thread ever waits on the gate. A saturated pool previously ran one O(MaxPoolSize) walk per waiting caller over the same slots; now a caller that arrives mid-sweep skips its walk and parks, and the in-flight sweep's connection is waiting in the channel when it does. It also matters that nobody blocks here because the gate is held across deactivation, which can make server round trips. WaitHandleDbConnectionPool has the same accounting hole. Left as-is and tracked by #4555, so the parity test now asserts the drift for that pool and the corrected value for this one. Also move the Reclaimer property in with the other properties, and fix the indentation of the reclaim summary trace. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 88 ++++++++++++++----- .../ConnectionPool/IDbConnectionPool.cs | 8 +- .../DbConnectionPoolInstrumentationTest.cs | 13 ++- .../TransactedConnectionPoolTest.cs | 2 +- 4 files changed, 81 insertions(+), 30 deletions(-) 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 b93210a18b..2e348e7665 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 @@ -75,6 +75,13 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool, IDisposable private readonly int _instanceId = Interlocked.Increment(ref _instanceCount); + /// + /// Serializes emancipated-connection sweeps. Held for the duration of a sweep, including the + /// routing of every reclaimed connection, so that a connection this sweep has selected + /// cannot be claimed by another sweep before it is returned. + /// + private readonly object _reclaimSweepGate = new(); + /// /// Tracks all connections currently managed by this pool, whether idle or busy. /// Only updated rarely - when physical connections are opened/closed - but is read in perf-sensitive contexts. @@ -295,6 +302,12 @@ public ConcurrentDictionary< /// private bool HasTransactionAffinity => PoolGroupOptions.HasTransactionAffinity; + /// + /// Drives background sweeps for emancipated connections. Unlike this is + /// always present, since reclamation applies to every pool configuration. + /// + internal PoolReclaimer Reclaimer { get; } + /// /// The most recently launched warmup/replenishment loop task, exposed so tests can await a /// warmup pass to a deterministic completion instead of polling pool counters. May be null @@ -545,7 +558,7 @@ public DbConnectionInternal ReplaceConnection( } /// - public void ReturnInternalConnection(DbConnectionInternal connection, DbConnection owningObject) + public void ReturnInternalConnection(DbConnectionInternal connection, DbConnection? owningObject) { Metrics.SoftDisconnectRequest(); @@ -557,9 +570,7 @@ public void ReturnInternalConnection(DbConnectionInternal connection, DbConnecti /// /// 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. + /// transacted pool, stasis, or destruction as appropriate. /// /// The connection to deactivate and route. private void DeactivateAndRouteConnection(DbConnectionInternal connection) @@ -1594,6 +1605,42 @@ private async Task GetInternalConnection( /// /// True if at least one connection was reclaimed; otherwise, false. internal bool ReclaimEmancipatedConnections() + { + // Only one sweep at a time. Both the timer and any caller that finds the pool saturated + // sweep, so without this a saturated pool runs one O(MaxPoolSize) walk per waiting + // caller over the same slots. Holding it for the whole sweep is also what lets the + // routing below go through ReturnInternalConnection, which re-takes the connection lock + // to call PrePush: the gate keeps anything else from claiming the connection in between. + // + // TryEnter, so no thread ever waits here. A caller that arrives mid-sweep parks instead, + // and whatever the in-flight sweep reclaims lands in the idle channel, so it is still + // served. That matters because the gate is held across deactivation, which can make + // server round trips. + bool sweeping = false; + try + { + Monitor.TryEnter(_reclaimSweepGate, ref sweeping); + if (!sweeping) + { + return false; + } + + return SweepEmancipatedConnections(); + } + finally + { + if (sweeping) + { + Monitor.Exit(_reclaimSweepGate); + } + } + } + + /// + /// Body of . Must be called with + /// held. + /// + private bool SweepEmancipatedConnections() { List? reclaimed = null; @@ -1603,9 +1650,9 @@ internal bool ReclaimEmancipatedConnections() // this sweep is looking for. A connection is only emancipated while it is checked out, // and a checked-out connection is not in the idle channel, so neither the pruner nor // Clear can remove it underneath us. The only code that can claim it is another sweep, - // which is excluded by the per-connection lock below. A concurrently replaced slot can - // therefore cost this sweep a miss, never a connection resurrected after removal, and a - // miss is picked up by the next sweep. + // which is excluded by the sweep gate. A concurrently replaced slot can therefore cost + // this sweep a miss, never a connection resurrected after removal, and a miss is picked + // up by the next sweep. foreach (DbConnectionInternal connection in _connectionSlots) { // TryEnter rather than Enter: IsEmancipated must be read under the connection lock to @@ -1619,10 +1666,6 @@ internal bool ReclaimEmancipatedConnections() 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); } } @@ -1645,9 +1688,9 @@ internal bool ReclaimEmancipatedConnections() // One summary trace rather than the entry trace plus a line per connection that // WaitHandleDbConnectionPool emits. - SqlClientEventSource.Log.TryPoolerTraceEvent( + SqlClientEventSource.Log.TryPoolerTraceEvent( "ChannelDbConnectionPool.ReclaimEmancipatedConnections | INFO | {0}, Reclaimed {1} emancipated connection(s).", - Id, + Id, reclaimed.Count); foreach (DbConnectionInternal connection in reclaimed) @@ -1656,8 +1699,16 @@ internal bool ReclaimEmancipatedConnections() // is meaningful under pool V2 as well; it previously always read zero here. Metrics.ReclaimedConnectionRequest(); + // Detached before the return rather than after the claim, which is the order + // WaitHandleDbConnectionPool uses. The only step that reads the pooled count is + // DelegatedTransactionEnded, unreachable here since IsEmancipated already excludes + // IsTxRootWaitingForTxEnd, so the two orders agree. connection.DetachCurrentTransactionIfEnded(); - DeactivateAndRouteConnection(connection); + + // Routed through the normal return path so reclamation cannot drift away from the + // accounting a return performs. The owner was collected, so there is none to + // validate against: PrePush(null) asserts exactly that. + ReturnInternalConnection(connection, owningObject: null); } return true; @@ -2050,14 +2101,5 @@ internal void PruneConnections(int count) pruned); } #endregion - - #region Reclamation - /// - /// Drives background sweeps for emancipated connections while callers are parked on the idle - /// channel. Unlike this is always present, because a connection can leak - /// in any pool configuration, including a fixed-size one. - /// - internal PoolReclaimer Reclaimer { get; } - #endregion } } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IDbConnectionPool.cs index ec30b4efa6..178eb03a15 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IDbConnectionPool.cs @@ -156,8 +156,12 @@ internal interface IDbConnectionPool /// Returns an internal connection to the pool. /// /// The internal connection to return to the pool. - /// The connection that currently owns this internal connection. Used to verify ownership. - void ReturnInternalConnection(DbConnectionInternal obj, DbConnection owningObject); + /// + /// The connection that currently owns this internal connection. Used to verify ownership. + /// Null when the pool is reclaiming an emancipated connection, whose owner was garbage + /// collected without closing it; the ownership check then asserts that no owner is alive. + /// + void ReturnInternalConnection(DbConnectionInternal obj, DbConnection? owningObject); /// /// Puts an internal connection from a transacted pool back into the general pool. diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs index c2611dce9d..043e469b5c 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs @@ -744,6 +744,9 @@ public void IdleSweepOfTransactionRoot_CountsStasisExitOnReturnToPool() /// and a second caller then requests one. Each implementation reaches its own reclaim path /// from that saturated request, so the test asserts observable behavior rather than a /// specific internal entry point. + /// + /// The two pools agree on every counter except activeSoftConnections; see the assertion + /// below. /// [Theory] [InlineData(PoolImplementation.WaitHandle)] @@ -781,15 +784,17 @@ public void EmancipatedConnection_IsReclaimedAndCounted(PoolImplementation imple out DbConnectionInternal? reclaimed)); // Assert - the same physical connection was handed out again, with no second connect. - // softDisconnects stays at zero: reclamation never balances the leaked checkout, so - // activeSoftConnections drifts up by one per leaked connection. That is long-standing - // WaitHandle behavior, asserted here to pin the two implementations to the same numbers. + // The two pools disagree on softDisconnects: the channel pool routes reclamation + // through ReturnInternalConnection, so the leaked checkout is balanced and + // activeSoftConnections settles back to one. WaitHandle reclaims without that + // accounting, so its gauge drifts up by one per leaked connection. Tracked by #4555; + // asserted here so the drift is pinned rather than assumed. Assert.Same(leaked, reclaimed); AssertCounters( metrics, hardConnects: 1, softConnects: 2, - softDisconnects: 0, + softDisconnects: implementation == PoolImplementation.Channel ? 1 : 0, pooledConnections: 1, reclaimedConnections: 1, activeConnections: 1); diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/TransactedConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/TransactedConnectionPoolTest.cs index 6866fb1fa2..cc9610932a 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/TransactedConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/TransactedConnectionPoolTest.cs @@ -699,7 +699,7 @@ public DbConnectionInternal ReplaceConnection(DbConnection owningObject, DbConne throw new NotImplementedException(); } - public void ReturnInternalConnection(DbConnectionInternal obj, DbConnection owningObject) + public void ReturnInternalConnection(DbConnectionInternal obj, DbConnection? owningObject) { throw new NotImplementedException(); } From 905bb5ad49663f99739aa79d68b29d1a87a8759a Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 19 Aug 2026 15:16:39 -0700 Subject: [PATCH 08/27] Inline DeactivateAndRouteConnection into its only caller Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 23 +++---------------- 1 file changed, 3 insertions(+), 20 deletions(-) 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 2e348e7665..89832204ec 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 @@ -564,17 +564,6 @@ 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. - /// - /// The connection to deactivate and route. - private void DeactivateAndRouteConnection(DbConnectionInternal connection) - { SqlClientEventSource.Log.TryPoolerTraceEvent( "ChannelDbConnectionPool.ReturnInternalConnection | INFO | {0}, Connection {1}, Deactivating.", Id, @@ -794,15 +783,9 @@ public void Shutdown() "ChannelDbConnectionPool.Shutdown | INFO | {0}, Pruner.Dispose threw, continuing shutdown: {1}", Id, ex); } - // Stop the background reclaim sweep before draining. This only narrows the window: - // ITimer.Dispose does not wait for a callback that is already running, so a sweep can - // still be in flight below. What actually guarantees a late-reclaimed connection is - // disposed rather than stranded is the ordering further down: _idleChannel.Complete() - // runs before the drain, after which TryWrite fails and PutConnectionInIdleChannel - // destroys the connection instead of pooling it. Any write that did succeed necessarily - // happened before Complete(), so the final unbounded drain mops it up. The State check - // in DeactivateAndRouteConnection catches most late sweeps earlier, but State is not - // synchronized against this write, so it cannot be relied on alone. + // Best effort: ITimer.Dispose does not wait for a sweep already in flight. Late + // reclaims are handled by _idleChannel.Complete() below, after which connections are + // destroyed rather than pooled. try { Reclaimer.Dispose(); From 07fd6fdb7549b32dc8b10a078a2624f7558a742e Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 19 Aug 2026 15:29:15 -0700 Subject: [PATCH 09/27] Drop the inline reclaim sweep, leaving reclamation to the timer Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 70 ++++++------------- ...ChannelDbConnectionPoolReclaimTimerTest.cs | 20 +++--- .../DbConnectionPoolInstrumentationTest.cs | 58 ++++++++++++--- 3 files changed, 77 insertions(+), 71 deletions(-) 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 89832204ec..dc207f8006 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 @@ -1511,27 +1511,17 @@ 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), 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. // - // That FIFO guarantee is also why the reclaim sweep above cannot simply be retried - // on this thread while we wait: cancelling the read to re-sweep would send us to - // the back of the queue behind callers that arrived later. Instead we register - // with the reclaimer, which sweeps on a timer for as long as anyone is parked and - // routes anything it reclaims back through this channel, waking us here. + // Connections whose owning SqlConnection was garbage collected without being + // closed still occupy pool slots, so at MaxPoolSize every subsequent request + // would otherwise wait forever. Rather than sweeping for them here, we register + // with the reclaimer, which sweeps on a timer for as long as anyone is parked + // and routes anything it reclaims back through this channel, waking us here. + // Sweeping inline would cost an O(MaxPoolSize) walk on every saturated acquire + // in applications that never leak. if (connection is null) { Reclaimer.EnterParkedWait(); @@ -1586,29 +1576,27 @@ private async Task GetInternalConnection( /// 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. - internal bool ReclaimEmancipatedConnections() + internal void ReclaimEmancipatedConnections() { - // Only one sweep at a time. Both the timer and any caller that finds the pool saturated - // sweep, so without this a saturated pool runs one O(MaxPoolSize) walk per waiting - // caller over the same slots. Holding it for the whole sweep is also what lets the - // routing below go through ReturnInternalConnection, which re-takes the connection lock - // to call PrePush: the gate keeps anything else from claiming the connection in between. + // Only one sweep at a time. Sweeps are driven by the reclaim timer, which can overlap + // itself: ExitParkedWait can disarm and EnterParkedWait re-arm while a sweep is still + // running. Holding the gate for the whole sweep is also what lets the routing below go + // through ReturnInternalConnection, which re-takes the connection lock to call PrePush: + // the gate keeps anything else from claiming the connection in between. // - // TryEnter, so no thread ever waits here. A caller that arrives mid-sweep parks instead, - // and whatever the in-flight sweep reclaims lands in the idle channel, so it is still - // served. That matters because the gate is held across deactivation, which can make - // server round trips. + // TryEnter, so no thread ever waits here. Whatever the in-flight sweep reclaims lands in + // the idle channel, so parked callers are still served. That matters because the gate is + // held across deactivation, which can make server round trips. bool sweeping = false; try { Monitor.TryEnter(_reclaimSweepGate, ref sweeping); if (!sweeping) { - return false; + return; } - return SweepEmancipatedConnections(); + SweepEmancipatedConnections(); } finally { @@ -1623,7 +1611,7 @@ internal bool ReclaimEmancipatedConnections() /// Body of . Must be called with /// held. /// - private bool SweepEmancipatedConnections() + private void SweepEmancipatedConnections() { List? reclaimed = null; @@ -1663,14 +1651,9 @@ private bool SweepEmancipatedConnections() if (reclaimed is null) { - // Deliberately silent. This runs once per second for as long as any caller is - // parked, so tracing an empty sweep would flood the trace stream of a pool that is - // merely busy. A sweep that finds nothing is reported by its absence. - return false; + return; } - // One summary trace rather than the entry trace plus a line per connection that - // WaitHandleDbConnectionPool emits. SqlClientEventSource.Log.TryPoolerTraceEvent( "ChannelDbConnectionPool.ReclaimEmancipatedConnections | INFO | {0}, Reclaimed {1} emancipated connection(s).", Id, @@ -1678,23 +1661,10 @@ private bool SweepEmancipatedConnections() foreach (DbConnectionInternal connection in reclaimed) { - // Matches WaitHandleDbConnectionPool so the number-of-reclaimed-connections counter - // is meaningful under pool V2 as well; it previously always read zero here. Metrics.ReclaimedConnectionRequest(); - - // Detached before the return rather than after the claim, which is the order - // WaitHandleDbConnectionPool uses. The only step that reads the pooled count is - // DelegatedTransactionEnded, unreachable here since IsEmancipated already excludes - // IsTxRootWaitingForTxEnd, so the two orders agree. connection.DetachCurrentTransactionIfEnded(); - - // Routed through the normal return path so reclamation cannot drift away from the - // accounting a return performs. The owner was collected, so there is none to - // validate against: PrePush(null) asserts exactly that. ReturnInternalConnection(connection, owningObject: null); } - - return true; } /// diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs index 78ca9e4cea..1268b03664 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs @@ -281,8 +281,7 @@ public void Shutdown_DisposesReclaimer() /// /// End-to-end coverage of the gap this feature closes. A caller parks on an exhausted pool, - /// and only afterwards does the owner of the sole connection become collectable. The inline - /// sweep the caller performed before parking could not have seen that, so without a + /// and only afterwards does the owner of the sole connection become collectable. Without a /// background sweep the caller would wait out its entire timeout even though a slot was /// recoverable. Advancing the injected fires the sweep, which /// reclaims the connection and wakes the caller. @@ -294,14 +293,13 @@ public async Task ParkedCaller_IsWokenBySweep_WhenConnectionIsEmancipatedAfterPa var pool = ConstructPool(maxPoolSize: 1, timeProvider: fakeTime); // Occupy the pool's only slot. The owner is deliberately kept rooted until after the - // second caller has parked: a collection triggered by a test running in parallel would - // otherwise emancipate it early, letting the second caller's inline sweep succeed so it - // never parks and the case under test never arises. + // second caller has parked, so the connection is provably still owned at that point and + // the sweep below is the only thing that can free it. DbConnectionInternal leaked = CheckOutAndRootOwner(pool, out GCHandle leakedOwnerRoot); Assert.Equal(1, pool.Count); - // A second caller finds the pool exhausted and parks. Its inline sweep runs before the - // collection below, so it finds nothing. The async path is used deliberately: the sync + // A second caller finds the pool exhausted and parks. The async path is used + // deliberately: the sync // path first takes a process-wide sync-over-async semaphore, so a sync caller could sit // behind unrelated tests rather than on the idle channel this test is exercising. SqlConnection waitingOwner = new(); @@ -317,8 +315,8 @@ public async Task ParkedCaller_IsWokenBySweep_WhenConnectionIsEmancipatedAfterPa Assert.True(pool.Reclaimer.IsTimerEnabled); Assert.False(parked.Task.IsCompleted); - // Only now does the abandoned owner become collectable, which is precisely the case the - // caller's own inline sweep cannot cover. + // Only now does the abandoned owner become collectable, which is precisely the case a + // one-shot sweep at request time cannot cover. leakedOwnerRoot.Free(); CollectAbandonedOwners(); Assert.True(leaked.IsEmancipated); @@ -394,8 +392,8 @@ public void ParkedSyncCaller_IsWokenBySweep_WhenConnectionIsEmancipatedAfterPark () => (caller.ThreadState & System.Threading.ThreadState.WaitSleepJoin) != 0, "the sync caller should block inside the channel read"); - // Only now does the abandoned owner become collectable, which is precisely the case the - // caller's own inline sweep cannot cover. + // Only now does the abandoned owner become collectable, which is precisely the case a + // one-shot sweep at request time cannot cover. leakedOwnerRoot.Free(); CollectAbandonedOwners(); Assert.True(leaked.IsEmancipated); diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs index 043e469b5c..920f25be2e 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs @@ -78,9 +78,11 @@ private static IDbConnectionPool ConstructPool( string connectionString = "Data Source=localhost;", int maxPoolSize = 50, int minPoolSize = 0, - int idleTimeout = 0) + int idleTimeout = 0, + FakeTimeProvider? timeProvider = null) { DbConnectionPoolGroup poolGroup = ConstructPoolGroup(connectionString, maxPoolSize, minPoolSize, idleTimeout); + timeProvider ??= new FakeTimeProvider(); return implementation switch { @@ -89,7 +91,7 @@ private static IDbConnectionPool ConstructPool( poolGroup, DbConnectionPoolIdentity.NoIdentity, new DbConnectionPoolProviderInfo(), - timeProvider: new FakeTimeProvider(), + timeProvider: timeProvider, metrics: metrics), PoolImplementation.Channel => new ChannelDbConnectionPool( @@ -98,7 +100,7 @@ private static IDbConnectionPool ConstructPool( DbConnectionPoolIdentity.NoIdentity, new DbConnectionPoolProviderInfo(), connectionCreationRateLimiter: null, - timeProvider: new FakeTimeProvider(), + timeProvider: timeProvider, metrics: metrics), _ => throw new ArgumentOutOfRangeException(nameof(implementation)), @@ -743,7 +745,9 @@ public void IdleSweepOfTransactionRoot_CountsStasisExitOnReturnToPool() /// reclaim methods: the pool is capped at a single connection, that connection is leaked, /// and a second caller then requests one. Each implementation reaches its own reclaim path /// from that saturated request, so the test asserts observable behavior rather than a - /// specific internal entry point. + /// specific internal entry point. WaitHandle sweeps inline before waiting; the channel pool + /// sweeps from its reclaim timer, so the request runs on a background thread and the fake + /// clock is advanced once it parks. /// /// The two pools agree on every counter except activeSoftConnections; see the assertion /// below. @@ -755,11 +759,13 @@ public void EmancipatedConnection_IsReclaimedAndCounted(PoolImplementation imple { // Arrange FakeSqlClientMetrics metrics = new(); + FakeTimeProvider fakeTime = new(); IDbConnectionPool pool = ConstructPool( implementation, metrics, new SuccessfulSqlConnectionFactory(metrics), - maxPoolSize: 1); + maxPoolSize: 1, + timeProvider: fakeTime); DbConnectionInternal leaked = CheckOutAndAbandonOwner(pool); AssertCounters( @@ -777,11 +783,41 @@ public void EmancipatedConnection_IsReclaimedAndCounted(PoolImplementation imple // Act - the pool's only slot is occupied by a connection nobody can return, so this // request can only be served by reclaiming it. - Assert.True(pool.TryGetConnection( - new SqlConnection(), - null, - TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), - out DbConnectionInternal? reclaimed)); + DbConnectionInternal? reclaimed = null; + Exception? failure = null; + SqlConnection waitingOwner = new(); + Thread caller = new(() => + { + try + { + Assert.True(pool.TryGetConnection( + waitingOwner, + null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out reclaimed)); + } + catch (Exception ex) + { + failure = ex; + } + }) + { + IsBackground = true, + Name = nameof(EmancipatedConnection_IsReclaimedAndCounted) + }; + caller.Start(); + + if (pool is ChannelDbConnectionPool channelPool) + { + // FakeTimeProvider invokes timer callbacks synchronously on Advance, so the sweep + // runs on this thread. Whether it lands before or after the caller reaches the + // channel read does not matter: the connection is queued either way. + SpinWait.SpinUntil(() => channelPool.Reclaimer.ParkedWaiters == 1, TimeSpan.FromSeconds(30)); + fakeTime.Advance(PoolReclaimer.SweepInterval); + } + + Assert.True(caller.Join(TimeSpan.FromSeconds(30)), "the caller should be served by the reclaimed connection"); + Assert.Null(failure); // Assert - the same physical connection was handed out again, with no second connect. // The two pools disagree on softDisconnects: the channel pool routes reclamation @@ -798,6 +834,8 @@ public void EmancipatedConnection_IsReclaimedAndCounted(PoolImplementation imple pooledConnections: 1, reclaimedConnections: 1, activeConnections: 1); + + GC.KeepAlive(waitingOwner); } /// From 36ba0abc375cd85ea33e823f2814fd16d783527c Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 19 Aug 2026 15:39:00 -0700 Subject: [PATCH 10/27] Count reclaims only on success and tolerate a failed return Replace the hand-rolled slot enumerator with an iterator method. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 34 ++++++++++--- .../ConnectionPool/ConnectionPoolSlots.cs | 49 +++++-------------- 2 files changed, 39 insertions(+), 44 deletions(-) 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 dc207f8006..dc0d23b808 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 @@ -1654,16 +1654,36 @@ private void SweepEmancipatedConnections() return; } - SqlClientEventSource.Log.TryPoolerTraceEvent( - "ChannelDbConnectionPool.ReclaimEmancipatedConnections | INFO | {0}, Reclaimed {1} emancipated connection(s).", - Id, - reclaimed.Count); - + int returned = 0; foreach (DbConnectionInternal connection in reclaimed) { + try + { + connection.DetachCurrentTransactionIfEnded(); + ReturnInternalConnection(connection, owningObject: null); + } + catch (Exception ex) + { + // One connection failing to return must not strand the rest of the sweep. + SqlClientEventSource.Log.TryPoolerTraceEvent( + "ChannelDbConnectionPool.ReclaimEmancipatedConnections | ERR | {0}, Connection {1}, Return threw: {2}.", + Id, + connection.ObjectID, + ex); + + continue; + } + Metrics.ReclaimedConnectionRequest(); - connection.DetachCurrentTransactionIfEnded(); - ReturnInternalConnection(connection, owningObject: null); + returned++; + } + + if (returned > 0) + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + "ChannelDbConnectionPool.ReclaimEmancipatedConnections | INFO | {0}, Reclaimed {1} emancipated connection(s).", + Id, + returned); } } 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 66dc1aae1b..a16a6f7b69 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; @@ -205,48 +206,22 @@ internal bool TryReplace(DbConnectionInternal oldConnection, DbConnectionInterna } /// - /// Enumerates the connections currently tracked by this collection without allocating. - /// The enumeration is best-effort: connections may be added or removed while it is in - /// progress, so callers must tolerate entries that have since left the pool, and an entry - /// added during the walk may or may not be seen. This is safe because the backing array has - /// a fixed capacity and is never reallocated; each slot is read individually. Intended for - /// infrequent bookkeeping passes (e.g. reclaiming emancipated connections), not for hot - /// paths. + /// Enumerates the connections currently tracked by this collection. The enumeration is + /// best-effort: connections may be added or removed while it is in progress, so callers must + /// tolerate entries that have since left the pool, and an entry added during the walk may or + /// may not be seen. This is safe because the backing array has a fixed capacity and is never + /// reallocated; each slot is read individually. Intended for infrequent bookkeeping passes + /// (e.g. reclaiming emancipated connections), not for hot paths. /// - public Enumerator GetEnumerator() => new(_connections); - - /// - /// A non-allocating enumerator over the occupied slots of a . - /// Declared as a mutable struct and returned by value from so a - /// foreach over the collection allocates nothing; do not copy it into a local. - /// - public struct Enumerator + public IEnumerator GetEnumerator() { - private readonly DbConnectionInternal?[] _connections; - private int _index; - - internal Enumerator(DbConnectionInternal?[] connections) - { - _connections = connections; - _index = -1; - Current = null!; - } - - public DbConnectionInternal Current { get; private set; } - - public bool MoveNext() + for (int i = 0; i < _connections.Length; i++) { - while (++_index < _connections.Length) + DbConnectionInternal? connection = Volatile.Read(ref _connections[i]); + if (connection is not null) { - DbConnectionInternal? connection = Volatile.Read(ref _connections[_index]); - if (connection is not null) - { - Current = connection; - return true; - } + yield return connection; } - - return false; } } From 4b0cd2e5f3a8eafda64947dfc3df5491d6ab1ee2 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 19 Aug 2026 15:40:56 -0700 Subject: [PATCH 11/27] Drop the vacuous reclaimer construction test Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- ...ChannelDbConnectionPoolReclaimTimerTest.cs | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs index 1268b03664..42fe7eef13 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs @@ -132,25 +132,6 @@ private static void WaitFor(Func condition, string because) #endregion - /// - /// The reclaimer must exist for every pool configuration. A connection can be leaked - /// regardless of sizing or idle timeout, and a fixed-size pool (where is deliberately null) is the configuration where - /// a permanently occupied slot hurts most. - /// - [Theory] - [InlineData(0, 50, 300)] // Growable pool with idle reclamation: pruner also present. - [InlineData(5, 5, 300)] // Fixed-size pool: no pruner. - [InlineData(0, 50, 0)] // Idle reclamation disabled: no pruner. - public void Reclaimer_IsConstructedForEveryPoolConfiguration(int minPoolSize, int maxPoolSize, int idleTimeout) - { - var pool = ConstructPool(minPoolSize: minPoolSize, maxPoolSize: maxPoolSize, idleTimeout: idleTimeout); - - Assert.NotNull(pool.Reclaimer); - Assert.False(pool.Reclaimer.IsTimerEnabled); - Assert.Equal(0, pool.Reclaimer.ParkedWaiters); - } - /// /// The timer is demand-driven: armed by the first parked caller and disarmed by the last one /// to leave, so a pool with no blocked callers never wakes the process. From 97deec547c205bdd3b69df2aa9ebfead3cf0aca0 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 19 Aug 2026 15:54:26 -0700 Subject: [PATCH 12/27] Share the reclamation tests across both pool implementations Reclaiming a leaked connection is behavior both pools owe, so the tests for it should not be written against one of them. Extract the pool construction and leak helpers into PoolTestHarness and add DbConnectionPoolReclamationTest, a theory over (implementation, sync/async) that asserts only what a caller can observe, driving whichever mechanism a pool uses by advancing a fake clock. ChannelDbConnectionPoolReclaimTimerTest becomes PoolReclaimerTest and keeps only the timer arm/disarm bookkeeping, which has no shared counterpart. Serving a connection leaked after the request began stays channel-only: WaitHandle stops reclaiming once it gives up on the creation mutex, so it times out with a recoverable slot sitting there. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- ...ChannelDbConnectionPoolReclaimTimerTest.cs | 395 ------------------ .../DbConnectionPoolInstrumentationTest.cs | 117 +----- .../DbConnectionPoolReclamationTest.cs | 220 ++++++++++ .../ConnectionPool/PoolReclaimerTest.cs | 153 +++++++ .../ConnectionPool/PoolTestHarness.cs | 211 ++++++++++ 5 files changed, 600 insertions(+), 496 deletions(-) delete mode 100644 src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs create mode 100644 src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolReclamationTest.cs create mode 100644 src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/PoolReclaimerTest.cs create mode 100644 src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/PoolTestHarness.cs diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs deleted file mode 100644 index 42fe7eef13..0000000000 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReclaimTimerTest.cs +++ /dev/null @@ -1,395 +0,0 @@ -// 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. - -using System; -using System.Diagnostics; -using System.Runtime.CompilerServices; -using System.Runtime.InteropServices; -using System.Threading; -using System.Threading.Tasks; -using Microsoft.Data.Common.ConnectionString; -using Microsoft.Data.ProviderBase; -using Microsoft.Data.SqlClient.ConnectionPool; -using Microsoft.Extensions.Time.Testing; -using Xunit; - -namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool -{ - /// - /// Unit tests for , the background sweep that reclaims emancipated - /// connections in while callers are parked on the idle - /// channel. - /// - public class ChannelDbConnectionPoolReclaimTimerTest - { - private static readonly SqlConnectionFactory ConnectionFactory = - new ChannelDbConnectionPoolTest.SuccessfulSqlConnectionFactory(); - - #region Helpers - - private static ChannelDbConnectionPool ConstructPool( - int minPoolSize = 0, - int maxPoolSize = 50, - int idleTimeout = 0, - TimeProvider? timeProvider = null) - { - var poolGroupOptions = new DbConnectionPoolGroupOptions( - poolByIdentity: false, - minPoolSize: minPoolSize, - maxPoolSize: maxPoolSize, - creationTimeout: 15, - loadBalanceTimeout: 0, - hasTransactionAffinity: true, - idleTimeout: idleTimeout - ); - var dbConnectionPoolGroup = new DbConnectionPoolGroup( - new SqlConnectionOptions("Data Source=localhost;"), - new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), - poolGroupOptions - ); - return new ChannelDbConnectionPool( - ConnectionFactory, - dbConnectionPoolGroup, - DbConnectionPoolIdentity.NoIdentity, - new DbConnectionPoolProviderInfo(), - connectionCreationRateLimiter: null, - timeProvider: timeProvider - ); - } - - /// - /// Checks out a connection and abandons its owning , returning - /// only the internal connection. Marked so the - /// owner's stack slot is guaranteed to be gone when the caller collects: in Debug builds - /// locals stay alive to the end of their enclosing method, so the owner has to be confined - /// to a frame that has already been popped. - /// - [MethodImpl(MethodImplOptions.NoInlining)] - private static DbConnectionInternal CheckOutAndAbandonOwner(ChannelDbConnectionPool pool) - { - SqlConnection owner = new(); - bool completed = pool.TryGetConnection( - owner, - taskCompletionSource: null, - TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), - out DbConnectionInternal? connection); - - Assert.True(completed); - Assert.NotNull(connection); - return connection!; - } - - /// - /// Checks out a connection and roots its owner in a rather than a - /// local. The owner stays alive until the caller frees the returned handle, which gives the - /// test exact control over when the connection becomes emancipated. A local cannot do this: - /// in a Debug build its stack slot roots the object for the rest of the enclosing method - /// even after it is assigned null, so the collection would never happen. The checkout runs - /// in its own non-inlined frame so no slot in the caller's frame ever holds the owner. - /// - [MethodImpl(MethodImplOptions.NoInlining)] - private static DbConnectionInternal CheckOutAndRootOwner(ChannelDbConnectionPool pool, out GCHandle ownerRoot) - { - SqlConnection owner = new(); - ownerRoot = GCHandle.Alloc(owner); - - bool completed = pool.TryGetConnection( - owner, - taskCompletionSource: null, - TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), - out DbConnectionInternal? connection); - - Assert.True(completed); - Assert.NotNull(connection); - return connection!; - } - - /// - /// Forces collection of an abandoned owner so its connection becomes emancipated. - /// - private static void CollectAbandonedOwners() - { - GC.Collect(); - GC.WaitForPendingFinalizers(); - GC.Collect(); - } - - /// - /// Spins until holds, failing the test if it does not happen - /// within a generous timeout. Used to observe a background caller reaching its parked wait, - /// which is inherently a cross-thread transition and cannot be awaited directly. - /// - private static void WaitFor(Func condition, string because) - { - Stopwatch stopwatch = Stopwatch.StartNew(); - while (!condition()) - { - Assert.True(stopwatch.Elapsed < TimeSpan.FromSeconds(30), because); - Thread.Sleep(10); - } - } - - #endregion - - /// - /// The timer is demand-driven: armed by the first parked caller and disarmed by the last one - /// to leave, so a pool with no blocked callers never wakes the process. - /// - [Fact] - public void EnterAndExitParkedWait_ArmsAndDisarmsTimer() - { - var pool = ConstructPool(); - PoolReclaimer reclaimer = pool.Reclaimer; - - reclaimer.EnterParkedWait(); - Assert.True(reclaimer.IsTimerEnabled); - Assert.Equal(1, reclaimer.ParkedWaiters); - - reclaimer.ExitParkedWait(); - Assert.False(reclaimer.IsTimerEnabled); - Assert.Equal(0, reclaimer.ParkedWaiters); - } - - /// - /// With several callers parked, the timer stays armed until the last one leaves. Disarming - /// on the first departure would strand the callers still waiting. - /// - [Fact] - public void ExitParkedWait_WithOtherWaitersRemaining_KeepsTimerArmed() - { - var pool = ConstructPool(); - PoolReclaimer reclaimer = pool.Reclaimer; - - reclaimer.EnterParkedWait(); - reclaimer.EnterParkedWait(); - reclaimer.EnterParkedWait(); - Assert.Equal(3, reclaimer.ParkedWaiters); - - reclaimer.ExitParkedWait(); - Assert.True(reclaimer.IsTimerEnabled); - reclaimer.ExitParkedWait(); - Assert.True(reclaimer.IsTimerEnabled); - - reclaimer.ExitParkedWait(); - Assert.False(reclaimer.IsTimerEnabled); - } - - /// - /// After the timer has been disarmed and re-armed, it must still fire. This guards the - /// one-shot re-arm bookkeeping, where losing the armed flag would silently stop all - /// subsequent sweeps. - /// - [Fact] - public void EnterParkedWait_AfterFullDrain_ReArmsTimer() - { - var pool = ConstructPool(); - PoolReclaimer reclaimer = pool.Reclaimer; - - reclaimer.EnterParkedWait(); - reclaimer.ExitParkedWait(); - Assert.False(reclaimer.IsTimerEnabled); - - reclaimer.EnterParkedWait(); - Assert.True(reclaimer.IsTimerEnabled); - } - - /// - /// A sweep callback that was already scheduled when the last caller left must not run. The - /// pool is no longer blocked, so there is nobody to wake and no reason to pay for the sweep. - /// - [Fact] - public void OnSweepCallback_WhenDisarmed_DoesNotSweep() - { - var pool = ConstructPool(maxPoolSize: 1); - DbConnectionInternal connection = CheckOutAndAbandonOwner(pool); - CollectAbandonedOwners(); - Assert.True(connection.IsEmancipated); - - // Never armed, so this stands in for a callback that raced with the last exit. - pool.Reclaimer.OnSweepCallback(); - - Assert.Equal(0, pool.IdleCount); - Assert.True(connection.IsEmancipated); - } - - /// - /// A sweep reclaims an emancipated connection and routes it back to the idle channel, which - /// is what makes it visible to a parked caller. - /// - [Fact] - public void OnSweepCallback_WhenArmed_ReclaimsEmancipatedConnection() - { - var pool = ConstructPool(maxPoolSize: 1); - DbConnectionInternal connection = CheckOutAndAbandonOwner(pool); - CollectAbandonedOwners(); - Assert.True(connection.IsEmancipated); - Assert.Equal(0, pool.IdleCount); - - pool.Reclaimer.EnterParkedWait(); - try - { - pool.Reclaimer.OnSweepCallback(); - } - finally - { - pool.Reclaimer.ExitParkedWait(); - } - - Assert.Equal(1, pool.IdleCount); - } - - /// - /// Shutting the pool down releases the timer. Otherwise a scheduled sweep would keep the - /// pool reachable and could route a connection back into the channel after the shutdown - /// drain had already passed it. - /// - [Fact] - public void Shutdown_DisposesReclaimer() - { - var pool = ConstructPool(); - pool.Reclaimer.EnterParkedWait(); - Assert.True(pool.Reclaimer.IsTimerEnabled); - - pool.Shutdown(); - - Assert.False(pool.Reclaimer.IsTimerEnabled); - - // Arming after disposal must be a no-op rather than resurrecting the timer. - pool.Reclaimer.EnterParkedWait(); - Assert.False(pool.Reclaimer.IsTimerEnabled); - } - - /// - /// End-to-end coverage of the gap this feature closes. A caller parks on an exhausted pool, - /// and only afterwards does the owner of the sole connection become collectable. Without a - /// background sweep the caller would wait out its entire timeout even though a slot was - /// recoverable. Advancing the injected fires the sweep, which - /// reclaims the connection and wakes the caller. - /// - [Fact] - public async Task ParkedCaller_IsWokenBySweep_WhenConnectionIsEmancipatedAfterParking() - { - var fakeTime = new FakeTimeProvider(); - var pool = ConstructPool(maxPoolSize: 1, timeProvider: fakeTime); - - // Occupy the pool's only slot. The owner is deliberately kept rooted until after the - // second caller has parked, so the connection is provably still owned at that point and - // the sweep below is the only thing that can free it. - DbConnectionInternal leaked = CheckOutAndRootOwner(pool, out GCHandle leakedOwnerRoot); - Assert.Equal(1, pool.Count); - - // A second caller finds the pool exhausted and parks. The async path is used - // deliberately: the sync - // path first takes a process-wide sync-over-async semaphore, so a sync caller could sit - // behind unrelated tests rather than on the idle channel this test is exercising. - SqlConnection waitingOwner = new(); - TaskCompletionSource parked = new(); - pool.TryGetConnection( - waitingOwner, - parked, - TimeoutTimer.StartNew(TimeSpan.FromSeconds(30)), - out DbConnectionInternal? immediate); - - Assert.Null(immediate); - WaitFor(() => pool.Reclaimer.ParkedWaiters == 1, "the second caller should park on the idle channel"); - Assert.True(pool.Reclaimer.IsTimerEnabled); - Assert.False(parked.Task.IsCompleted); - - // Only now does the abandoned owner become collectable, which is precisely the case a - // one-shot sweep at request time cannot cover. - leakedOwnerRoot.Free(); - CollectAbandonedOwners(); - Assert.True(leaked.IsEmancipated); - - // FakeTimeProvider invokes timer callbacks synchronously on Advance. - fakeTime.Advance(PoolReclaimer.SweepInterval); - - DbConnectionInternal? result = await parked.Task; - - Assert.NotNull(result); - Assert.Equal(leaked, result); - Assert.Equal(0, pool.Reclaimer.ParkedWaiters); - Assert.False(pool.Reclaimer.IsTimerEnabled); - - GC.KeepAlive(waitingOwner); - } - - /// - /// The synchronous counterpart of the test above. The sync path does not await the idle - /// channel; it blocks the calling thread inside ReadChannelSyncOverAsync, behind a - /// process-wide sync-over-async semaphore. That makes it a genuinely different code path - /// from ReadAsync, so the sweep's ability to wake a parked caller is proven for both. - /// - /// - /// The caller runs on a dedicated thread rather than a pooled one: it blocks for the whole - /// parked wait, and parking it on a thread pool thread would consume a worker for the - /// duration. It also holds a slot in the shared sync-over-async semaphore while parked, - /// which is why the wait is kept as short as possible. - /// - [Fact] - public void ParkedSyncCaller_IsWokenBySweep_WhenConnectionIsEmancipatedAfterParking() - { - var fakeTime = new FakeTimeProvider(); - var pool = ConstructPool(maxPoolSize: 1, timeProvider: fakeTime); - - // Occupy the pool's only slot, keeping the owner rooted so a collection triggered by a - // test running in parallel cannot emancipate it before the second caller has parked. - DbConnectionInternal leaked = CheckOutAndRootOwner(pool, out GCHandle leakedOwnerRoot); - Assert.Equal(1, pool.Count); - - // A second caller finds the pool exhausted and blocks inside the sync read. - DbConnectionInternal? syncResult = null; - Exception? syncFailure = null; - SqlConnection waitingOwner = new(); - Thread caller = new(() => - { - try - { - pool.TryGetConnection( - waitingOwner, - taskCompletionSource: null, - TimeoutTimer.StartNew(TimeSpan.FromSeconds(30)), - out syncResult); - } - catch (Exception ex) - { - syncFailure = ex; - } - }) - { - IsBackground = true, - Name = nameof(ParkedSyncCaller_IsWokenBySweep_WhenConnectionIsEmancipatedAfterParking) - }; - caller.Start(); - - WaitFor(() => pool.Reclaimer.ParkedWaiters == 1, "the sync caller should register as parked"); - Assert.True(pool.Reclaimer.IsTimerEnabled); - - // ParkedWaiters is incremented before the semaphore is taken, so the caller may not be - // on the channel yet. Wait for the blocking read itself, otherwise the sweep below could - // reclaim and route the connection before anyone is there to be woken by it. - WaitFor( - () => (caller.ThreadState & System.Threading.ThreadState.WaitSleepJoin) != 0, - "the sync caller should block inside the channel read"); - - // Only now does the abandoned owner become collectable, which is precisely the case a - // one-shot sweep at request time cannot cover. - leakedOwnerRoot.Free(); - CollectAbandonedOwners(); - Assert.True(leaked.IsEmancipated); - - // FakeTimeProvider invokes timer callbacks synchronously on Advance. - fakeTime.Advance(PoolReclaimer.SweepInterval); - - Assert.True(caller.Join(TimeSpan.FromSeconds(30)), "the sync caller should be woken by the sweep"); - Assert.Null(syncFailure); - Assert.NotNull(syncResult); - Assert.Equal(leaked, syncResult); - Assert.Equal(0, pool.Reclaimer.ParkedWaiters); - Assert.False(pool.Reclaimer.IsTimerEnabled); - - GC.KeepAlive(waitingOwner); - } - } -} diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs index 920f25be2e..772501361e 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs @@ -20,6 +20,7 @@ using Xunit; using static Microsoft.Data.SqlClient.UnitTests.ConnectionPool.ChannelDbConnectionPoolTest; +using static Microsoft.Data.SqlClient.UnitTests.ConnectionPool.PoolTestHarness; namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool { @@ -31,45 +32,10 @@ namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool /// public class DbConnectionPoolInstrumentationTest { - /// - /// Identifies which pool implementation a parameterized metric test exercises. - /// - public enum PoolImplementation - { - /// The legacy . - WaitHandle, - - /// The . - Channel, - } - - /// - /// Builds the pool group shared by both pool implementations. - /// - private static DbConnectionPoolGroup ConstructPoolGroup( - string connectionString, - int maxPoolSize, - int minPoolSize, - int idleTimeout) - { - DbConnectionPoolGroupOptions poolGroupOptions = new( - poolByIdentity: false, - minPoolSize: minPoolSize, - maxPoolSize: maxPoolSize, - creationTimeout: 15, - loadBalanceTimeout: 0, - hasTransactionAffinity: true, - idleTimeout: idleTimeout); - - return new DbConnectionPoolGroup( - new SqlConnectionOptions(connectionString), - new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), - poolGroupOptions); - } - /// /// Builds the requested pool implementation behind the shared pool interface, reporting to - /// the supplied metrics instance. + /// the supplied metrics instance. Keeps this class's argument order over + /// , which every test here shares. /// private static IDbConnectionPool ConstructPool( PoolImplementation implementation, @@ -80,32 +46,15 @@ private static IDbConnectionPool ConstructPool( int minPoolSize = 0, int idleTimeout = 0, FakeTimeProvider? timeProvider = null) - { - DbConnectionPoolGroup poolGroup = ConstructPoolGroup(connectionString, maxPoolSize, minPoolSize, idleTimeout); - timeProvider ??= new FakeTimeProvider(); - - return implementation switch - { - PoolImplementation.WaitHandle => new WaitHandleDbConnectionPool( - connectionFactory, - poolGroup, - DbConnectionPoolIdentity.NoIdentity, - new DbConnectionPoolProviderInfo(), - timeProvider: timeProvider, - metrics: metrics), - - PoolImplementation.Channel => new ChannelDbConnectionPool( - connectionFactory, - poolGroup, - DbConnectionPoolIdentity.NoIdentity, - new DbConnectionPoolProviderInfo(), - connectionCreationRateLimiter: null, - timeProvider: timeProvider, - metrics: metrics), - - _ => throw new ArgumentOutOfRangeException(nameof(implementation)), - }; - } + => PoolTestHarness.ConstructPool( + implementation, + connectionFactory, + metrics, + timeProvider, + connectionString, + maxPoolSize, + minPoolSize, + idleTimeout); /// /// Asserts the exact value of every counter a pool is responsible for. Any counter not named @@ -741,13 +690,8 @@ public void IdleSweepOfTransactionRoot_CountsStasisExitOnReturnToPool() /// and counted, in both pool implementations. /// /// - /// The sweep is triggered the same way for both pools rather than by calling their internal - /// reclaim methods: the pool is capped at a single connection, that connection is leaked, - /// and a second caller then requests one. Each implementation reaches its own reclaim path - /// from that saturated request, so the test asserts observable behavior rather than a - /// specific internal entry point. WaitHandle sweeps inline before waiting; the channel pool - /// sweeps from its reclaim timer, so the request runs on a background thread and the fake - /// clock is advanced once it parks. + /// This covers the counters only. The reclamation behavior itself is asserted by + /// . /// /// The two pools agree on every counter except activeSoftConnections; see the assertion /// below. @@ -776,9 +720,7 @@ public void EmancipatedConnection_IsReclaimedAndCounted(PoolImplementation imple activeConnections: 1); // The owner is only collectable now that the helper's frame has been popped. - GC.Collect(); - GC.WaitForPendingFinalizers(); - GC.Collect(); + CollectAbandonedOwners(); Assert.True(leaked.IsEmancipated); // Act - the pool's only slot is occupied by a connection nobody can return, so this @@ -807,16 +749,7 @@ public void EmancipatedConnection_IsReclaimedAndCounted(PoolImplementation imple }; caller.Start(); - if (pool is ChannelDbConnectionPool channelPool) - { - // FakeTimeProvider invokes timer callbacks synchronously on Advance, so the sweep - // runs on this thread. Whether it lands before or after the caller reaches the - // channel read does not matter: the connection is queued either way. - SpinWait.SpinUntil(() => channelPool.Reclaimer.ParkedWaiters == 1, TimeSpan.FromSeconds(30)); - fakeTime.Advance(PoolReclaimer.SweepInterval); - } - - Assert.True(caller.Join(TimeSpan.FromSeconds(30)), "the caller should be served by the reclaimed connection"); + AdvanceUntil(fakeTime, () => caller.Join(TimeSpan.Zero), "the caller should be served by the reclaimed connection"); Assert.Null(failure); // Assert - the same physical connection was handed out again, with no second connect. @@ -838,24 +771,6 @@ public void EmancipatedConnection_IsReclaimedAndCounted(PoolImplementation imple GC.KeepAlive(waitingOwner); } - /// - /// Checks out a connection and drops the only reference to its owner. Kept in its own - /// non-inlined method so the owner becomes collectable as soon as this frame is popped; a - /// local in the calling test would stay rooted until the end of that method in a debug build. - /// - [MethodImpl(MethodImplOptions.NoInlining)] - private static DbConnectionInternal CheckOutAndAbandonOwner(IDbConnectionPool pool) - { - SqlConnection owner = new(); - Assert.True(pool.TryGetConnection( - owner, - null, - TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), - out DbConnectionInternal? connection)); - Assert.NotNull(connection); - return connection!; - } - #region Test classes /// diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolReclamationTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolReclamationTest.cs new file mode 100644 index 0000000000..ec74b8397e --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolReclamationTest.cs @@ -0,0 +1,220 @@ +// 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. + +using System; +using System.Runtime.InteropServices; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Data.ProviderBase; +using Microsoft.Data.SqlClient.ConnectionPool; +using Microsoft.Extensions.Time.Testing; +using Xunit; + +using static Microsoft.Data.SqlClient.UnitTests.ConnectionPool.PoolTestHarness; + +namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool +{ + /// + /// Behavior both pool implementations owe around emancipated connections: a connection whose + /// owning was collected without being closed must not permanently + /// occupy a pool slot. + /// + /// + /// These tests are deliberately black-box. The pools reclaim by different means, and which one + /// is used is not part of the contract: sweeps inline + /// when a request finds it saturated, while sweeps from a + /// background timer. Tests drive both through and assert only what a + /// caller can observe. The channel pool's timer mechanics are covered separately by + /// . + /// + public class DbConnectionPoolReclamationTest + { + /// + /// How long a saturated caller is willing to wait for a connection. The pool group default + /// is 15ms, which is too short for a caller to observably block. + /// + private const int PoolWaitMs = 30_000; + + /// + /// The pool's only slot is held by a leaked connection, so the request can only be served by + /// reclaiming it. Without reclamation the caller waits out its entire timeout even though a + /// slot is recoverable. + /// + [Theory] + [InlineData(PoolImplementation.WaitHandle, false)] + [InlineData(PoolImplementation.WaitHandle, true)] + [InlineData(PoolImplementation.Channel, false)] + [InlineData(PoolImplementation.Channel, true)] + public void SaturatedRequest_IsServedByReclaimingLeakedConnection(PoolImplementation implementation, bool async) + { + FakeTimeProvider fakeTime = new(); + IDbConnectionPool pool = ConstructPool(implementation, timeProvider: fakeTime, maxPoolSize: 1, creationTimeout: PoolWaitMs); + + DbConnectionInternal leaked = CheckOutAndAbandonOwner(pool); + CollectAbandonedOwners(); + Assert.True(leaked.IsEmancipated); + + DbConnectionInternal? served = RequestConnection(pool, async, fakeTime, out SqlConnection waitingOwner); + + Assert.Same(leaked, served); + GC.KeepAlive(waitingOwner); + } + + /// + /// The connection becomes collectable only after the caller is already waiting, so a pool + /// that reclaims once at request time cannot see it. Reclamation has to keep looking for as + /// long as someone is blocked. + /// + /// + /// Channel-only, because this is the gap the sweep timer closes. + /// reclaims only while it is still trying to create + /// a connection; once it gives up on the creation mutex it waits on the semaphore alone and + /// never looks again, so the caller times out with a recoverable slot sitting there. + /// + [Theory] + [InlineData(false)] + [InlineData(true)] + public void WaitingCaller_IsServed_WhenConnectionIsLeakedAfterTheRequestBegins(bool async) + { + FakeTimeProvider fakeTime = new(); + IDbConnectionPool pool = ConstructPool( + PoolImplementation.Channel, + timeProvider: fakeTime, + maxPoolSize: 1, + creationTimeout: PoolWaitMs); + + // Rooted in a handle rather than a local so the test controls exactly when the + // connection becomes emancipated, and a collection triggered by a test running in + // parallel cannot do it early. + DbConnectionInternal leaked = CheckOutAndRootOwner(pool, out GCHandle leakedOwnerRoot); + + DbConnectionInternal? served = null; + Exception? failure = null; + SqlConnection waitingOwner = new(); + Thread caller = new(() => + { + try + { + served = RequestAndWait(pool, waitingOwner, async); + } + catch (Exception ex) + { + failure = ex; + } + }) + { + IsBackground = true, + Name = nameof(WaitingCaller_IsServed_WhenConnectionIsLeakedAfterTheRequestBegins) + }; + caller.Start(); + + WaitFor( + () => (caller.ThreadState & System.Threading.ThreadState.WaitSleepJoin) != 0, + "the caller should block waiting for a connection"); + + // Only now does the abandoned owner become collectable. + leakedOwnerRoot.Free(); + CollectAbandonedOwners(); + Assert.True(leaked.IsEmancipated); + + AdvanceUntil(fakeTime, () => caller.Join(TimeSpan.Zero), "the waiting caller should be served by reclamation"); + + Assert.Null(failure); + Assert.Same(leaked, served); + GC.KeepAlive(waitingOwner); + } + + /// + /// A reclaimed connection is reused rather than reopened, and is counted as reclaimed. + /// + [Theory] + [InlineData(PoolImplementation.WaitHandle)] + [InlineData(PoolImplementation.Channel)] + public void ReclaimedConnection_IsReusedRatherThanReopened(PoolImplementation implementation) + { + FakeSqlClientMetrics metrics = new(); + FakeTimeProvider fakeTime = new(); + IDbConnectionPool pool = ConstructPool( + implementation, + new ChannelDbConnectionPoolTest.SuccessfulSqlConnectionFactory(metrics), + metrics, + fakeTime, + maxPoolSize: 1, + creationTimeout: PoolWaitMs); + + DbConnectionInternal leaked = CheckOutAndAbandonOwner(pool); + CollectAbandonedOwners(); + Assert.True(leaked.IsEmancipated); + + DbConnectionInternal? served = RequestConnection(pool, async: false, fakeTime, out SqlConnection waitingOwner); + + Assert.Same(leaked, served); + Assert.Equal(1, metrics.ReclaimedConnections); + Assert.Equal(1, metrics.HardConnects); + GC.KeepAlive(waitingOwner); + } + + /// + /// Issues a request on a background thread and drives the clock until it completes, so the + /// caller can block on a pool that only reclaims from a timer. + /// + private static DbConnectionInternal? RequestConnection( + IDbConnectionPool pool, + bool async, + FakeTimeProvider fakeTime, + out SqlConnection owner) + { + DbConnectionInternal? connection = null; + Exception? failure = null; + SqlConnection waitingOwner = new(); + Thread caller = new(() => + { + try + { + connection = RequestAndWait(pool, waitingOwner, async); + } + catch (Exception ex) + { + failure = ex; + } + }) + { + IsBackground = true, + Name = nameof(RequestConnection) + }; + caller.Start(); + + AdvanceUntil(fakeTime, () => caller.Join(TimeSpan.Zero), "the caller should be served by reclamation"); + + Assert.Null(failure); + owner = waitingOwner; + return connection; + } + + /// + /// Requests a connection over the sync or async path and blocks until it is served, so both + /// paths present the same shape to a test. + /// + private static DbConnectionInternal RequestAndWait(IDbConnectionPool pool, SqlConnection owner, bool async) + { + TaskCompletionSource? completion = async ? new() : null; + + bool completed = pool.TryGetConnection( + owner, + completion, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(30)), + out DbConnectionInternal? connection); + + if (completed) + { + Assert.NotNull(connection); + return connection!; + } + + // The async path hands the request off to the completion source rather than blocking. + Assert.True(async); + return completion!.Task.GetAwaiter().GetResult(); + } + } +} diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/PoolReclaimerTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/PoolReclaimerTest.cs new file mode 100644 index 0000000000..a9201ef40a --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/PoolReclaimerTest.cs @@ -0,0 +1,153 @@ +// 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. + +using System; +using Microsoft.Data.ProviderBase; +using Microsoft.Data.SqlClient.ConnectionPool; +using Xunit; + +using static Microsoft.Data.SqlClient.UnitTests.ConnectionPool.PoolTestHarness; + +namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool +{ + /// + /// Unit tests for , the demand-driven timer that sweeps emancipated + /// connections for . + /// + /// + /// These are deliberately white-box: they drive the reclaimer's arm/disarm bookkeeping directly + /// because it has no observable effect other than the reclamation it produces, and that is + /// covered for both pool implementations by . + /// + public class PoolReclaimerTest + { + private static ChannelDbConnectionPool ConstructChannelPool(int maxPoolSize = 50) + => (ChannelDbConnectionPool)ConstructPool(PoolImplementation.Channel, maxPoolSize: maxPoolSize); + + /// + /// The timer is demand-driven: armed by the first parked caller and disarmed by the last one + /// to leave, so a pool with no blocked callers never wakes the process. + /// + [Fact] + public void EnterAndExitParkedWait_ArmsAndDisarmsTimer() + { + PoolReclaimer reclaimer = ConstructChannelPool().Reclaimer; + + reclaimer.EnterParkedWait(); + Assert.True(reclaimer.IsTimerEnabled); + Assert.Equal(1, reclaimer.ParkedWaiters); + + reclaimer.ExitParkedWait(); + Assert.False(reclaimer.IsTimerEnabled); + Assert.Equal(0, reclaimer.ParkedWaiters); + } + + /// + /// With several callers parked, the timer stays armed until the last one leaves. Disarming + /// on the first departure would strand the callers still waiting. + /// + [Fact] + public void ExitParkedWait_WithOtherWaitersRemaining_KeepsTimerArmed() + { + PoolReclaimer reclaimer = ConstructChannelPool().Reclaimer; + + reclaimer.EnterParkedWait(); + reclaimer.EnterParkedWait(); + reclaimer.EnterParkedWait(); + Assert.Equal(3, reclaimer.ParkedWaiters); + + reclaimer.ExitParkedWait(); + Assert.True(reclaimer.IsTimerEnabled); + reclaimer.ExitParkedWait(); + Assert.True(reclaimer.IsTimerEnabled); + + reclaimer.ExitParkedWait(); + Assert.False(reclaimer.IsTimerEnabled); + } + + /// + /// After the timer has been disarmed and re-armed, it must still fire. This guards the + /// one-shot re-arm bookkeeping, where losing the armed flag would silently stop all + /// subsequent sweeps. + /// + [Fact] + public void EnterParkedWait_AfterFullDrain_ReArmsTimer() + { + PoolReclaimer reclaimer = ConstructChannelPool().Reclaimer; + + reclaimer.EnterParkedWait(); + reclaimer.ExitParkedWait(); + Assert.False(reclaimer.IsTimerEnabled); + + reclaimer.EnterParkedWait(); + Assert.True(reclaimer.IsTimerEnabled); + } + + /// + /// A sweep callback that was already scheduled when the last caller left must not run. The + /// pool is no longer blocked, so there is nobody to wake and no reason to pay for the sweep. + /// + [Fact] + public void OnSweepCallback_WhenDisarmed_DoesNotSweep() + { + ChannelDbConnectionPool pool = ConstructChannelPool(maxPoolSize: 1); + DbConnectionInternal connection = CheckOutAndAbandonOwner(pool); + CollectAbandonedOwners(); + Assert.True(connection.IsEmancipated); + + // Never armed, so this stands in for a callback that raced with the last exit. + pool.Reclaimer.OnSweepCallback(); + + Assert.Equal(0, pool.IdleCount); + Assert.True(connection.IsEmancipated); + } + + /// + /// A sweep reclaims an emancipated connection and routes it back to the idle channel, which + /// is what makes it visible to a parked caller. + /// + [Fact] + public void OnSweepCallback_WhenArmed_ReclaimsEmancipatedConnection() + { + ChannelDbConnectionPool pool = ConstructChannelPool(maxPoolSize: 1); + DbConnectionInternal connection = CheckOutAndAbandonOwner(pool); + CollectAbandonedOwners(); + Assert.True(connection.IsEmancipated); + Assert.Equal(0, pool.IdleCount); + + pool.Reclaimer.EnterParkedWait(); + try + { + pool.Reclaimer.OnSweepCallback(); + } + finally + { + pool.Reclaimer.ExitParkedWait(); + } + + Assert.Equal(1, pool.IdleCount); + } + + /// + /// Shutting the pool down releases the timer. Otherwise a scheduled sweep would keep the + /// pool reachable and could route a connection back into the channel after the shutdown + /// drain had already passed it. + /// + [Fact] + public void Shutdown_DisposesReclaimer() + { + ChannelDbConnectionPool pool = ConstructChannelPool(); + pool.Reclaimer.EnterParkedWait(); + Assert.True(pool.Reclaimer.IsTimerEnabled); + + pool.Shutdown(); + + Assert.False(pool.Reclaimer.IsTimerEnabled); + + // Arming after disposal must be a no-op rather than resurrecting the timer. + pool.Reclaimer.EnterParkedWait(); + Assert.False(pool.Reclaimer.IsTimerEnabled); + } + } +} diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/PoolTestHarness.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/PoolTestHarness.cs new file mode 100644 index 0000000000..846a635bf1 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/PoolTestHarness.cs @@ -0,0 +1,211 @@ +// 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. + +using System; +using System.Diagnostics; +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; +using System.Threading; +using Microsoft.Data.Common.ConnectionString; +using Microsoft.Data.ProviderBase; +using Microsoft.Data.SqlClient.ConnectionPool; +using Microsoft.Data.SqlClient.Diagnostics; +using Microsoft.Extensions.Time.Testing; +using Xunit; + +namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool +{ + /// + /// Which pool implementation a test is running against. Tests that assert behavior both pools + /// owe should be a over this, so the two cannot silently diverge. + /// + public enum PoolImplementation + { + /// The legacy . + WaitHandle, + + /// The . + Channel, + } + + /// + /// Construction and lifecycle helpers shared by the connection pool test classes, written + /// against so a test can run against either implementation. + /// + internal static class PoolTestHarness + { + /// + /// Builds the requested pool implementation behind the shared pool interface. + /// + /// Which pool to construct. + /// + /// Factory the pool creates connections with. Defaults to one that always succeeds. + /// + /// Metrics sink, or null for the pool's default. + /// + /// How long, in milliseconds, a caller waits for a pooled connection to free up. The default + /// matches the pool group default and is short enough that a saturated wait times out almost + /// immediately; raise it in tests that need a caller to genuinely block. + /// + /// + /// Time source, or null for a fresh . Pass one in to drive + /// timer-based pool behavior deterministically; see . + /// + internal static IDbConnectionPool ConstructPool( + PoolImplementation implementation, + SqlConnectionFactory? connectionFactory = null, + ISqlClientMetrics? metrics = null, + TimeProvider? timeProvider = null, + string connectionString = "Data Source=localhost;", + int maxPoolSize = 50, + int minPoolSize = 0, + int idleTimeout = 0, + int creationTimeout = 15) + { + connectionFactory ??= new ChannelDbConnectionPoolTest.SuccessfulSqlConnectionFactory(); + timeProvider ??= new FakeTimeProvider(); + + DbConnectionPoolGroup poolGroup = ConstructPoolGroup(connectionString, maxPoolSize, minPoolSize, idleTimeout, creationTimeout); + + return implementation switch + { + PoolImplementation.WaitHandle => new WaitHandleDbConnectionPool( + connectionFactory, + poolGroup, + DbConnectionPoolIdentity.NoIdentity, + new DbConnectionPoolProviderInfo(), + timeProvider: timeProvider, + metrics: metrics), + + PoolImplementation.Channel => new ChannelDbConnectionPool( + connectionFactory, + poolGroup, + DbConnectionPoolIdentity.NoIdentity, + new DbConnectionPoolProviderInfo(), + connectionCreationRateLimiter: null, + timeProvider: timeProvider, + metrics: metrics), + + _ => throw new ArgumentOutOfRangeException(nameof(implementation)), + }; + } + + /// + /// Builds a pool group with the given sizing, for tests that construct a pool directly. + /// + internal static DbConnectionPoolGroup ConstructPoolGroup( + string connectionString = "Data Source=localhost;", + int maxPoolSize = 50, + int minPoolSize = 0, + int idleTimeout = 0, + int creationTimeout = 15) + { + var poolGroupOptions = new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: minPoolSize, + maxPoolSize: maxPoolSize, + creationTimeout: creationTimeout, + loadBalanceTimeout: 0, + hasTransactionAffinity: true, + idleTimeout: idleTimeout); + + return new DbConnectionPoolGroup( + new SqlConnectionOptions(connectionString), + new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), + poolGroupOptions); + } + + /// + /// Checks out a connection and abandons its owning , returning + /// only the internal connection. Marked so the + /// owner's stack slot is guaranteed to be gone when the caller collects: in Debug builds + /// locals stay alive to the end of their enclosing method, so the owner has to be confined + /// to a frame that has already been popped. + /// + [MethodImpl(MethodImplOptions.NoInlining)] + internal static DbConnectionInternal CheckOutAndAbandonOwner(IDbConnectionPool pool) + { + SqlConnection owner = new(); + Assert.True(pool.TryGetConnection( + owner, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? connection)); + + Assert.NotNull(connection); + return connection!; + } + + /// + /// Checks out a connection and roots its owner in a rather than a + /// local. The owner stays alive until the caller frees the returned handle, which gives the + /// test exact control over when the connection becomes emancipated. A local cannot do this: + /// in a Debug build its stack slot roots the object for the rest of the enclosing method + /// even after it is assigned null, so the collection would never happen. The checkout runs + /// in its own non-inlined frame so no slot in the caller's frame ever holds the owner. + /// + [MethodImpl(MethodImplOptions.NoInlining)] + internal static DbConnectionInternal CheckOutAndRootOwner(IDbConnectionPool pool, out GCHandle ownerRoot) + { + SqlConnection owner = new(); + ownerRoot = GCHandle.Alloc(owner); + + Assert.True(pool.TryGetConnection( + owner, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? connection)); + + Assert.NotNull(connection); + return connection!; + } + + /// + /// Forces collection of an abandoned owner so its connection becomes emancipated. + /// + internal static void CollectAbandonedOwners() + { + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + } + + /// + /// Spins until holds, failing the test if it does not happen + /// within a generous timeout. Used to observe a background caller reaching a blocking wait, + /// which is inherently a cross-thread transition and cannot be awaited directly. + /// + internal static void WaitFor(Func condition, string because) + { + Stopwatch stopwatch = Stopwatch.StartNew(); + while (!condition()) + { + Assert.True(stopwatch.Elapsed < TimeSpan.FromSeconds(30), because); + Thread.Sleep(10); + } + } + + /// + /// Advances until holds, failing the + /// test if it does not happen within a generous timeout. + /// + /// + /// Lets a test assert that a pool eventually does something without knowing what drives it. + /// The pools differ here: reclaims inline when a + /// request finds it saturated, so the condition is typically already true on the first + /// check, while reclaims from a sweep timer and needs + /// the clock to move. Advancing a pool that did not need it is harmless. + /// + internal static void AdvanceUntil(FakeTimeProvider time, Func condition, string because) + { + Stopwatch stopwatch = Stopwatch.StartNew(); + while (!condition()) + { + Assert.True(stopwatch.Elapsed < TimeSpan.FromSeconds(30), because); + time.Advance(TimeSpan.FromSeconds(1)); + Thread.Sleep(10); + } + } + } +} From 6e0ac76d8f4d39ab830d9332aa5421bda166ee6a Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 19 Aug 2026 15:56:01 -0700 Subject: [PATCH 13/27] Note why the reclaimer is internal rather than private Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 dc0d23b808..1683621c53 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 @@ -304,7 +304,8 @@ public ConcurrentDictionary< /// /// Drives background sweeps for emancipated connections. Unlike this is - /// always present, since reclamation applies to every pool configuration. + /// always present, since reclamation applies to every pool configuration. Internal rather + /// than private so tests can drive the timer bookkeeping directly. /// internal PoolReclaimer Reclaimer { get; } From 90173d42b0ab7bcad4c691820e7a1e935eb16d5d Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 19 Aug 2026 15:56:56 -0700 Subject: [PATCH 14/27] Trim the sweep gate comment Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) 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 1683621c53..5d91331d7b 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 @@ -1579,15 +1579,9 @@ private async Task GetInternalConnection( /// internal void ReclaimEmancipatedConnections() { - // Only one sweep at a time. Sweeps are driven by the reclaim timer, which can overlap - // itself: ExitParkedWait can disarm and EnterParkedWait re-arm while a sweep is still - // running. Holding the gate for the whole sweep is also what lets the routing below go - // through ReturnInternalConnection, which re-takes the connection lock to call PrePush: - // the gate keeps anything else from claiming the connection in between. - // - // TryEnter, so no thread ever waits here. Whatever the in-flight sweep reclaims lands in - // the idle channel, so parked callers are still served. That matters because the gate is - // held across deactivation, which can make server round trips. + // One sweep at a time, so nothing else can claim a connection between the point it is + // found emancipated and the PrePush that claims it. TryEnter rather than Enter: whatever + // the in-flight sweep reclaims lands in the idle channel either way. bool sweeping = false; try { From b57b3f4bb986fdcd2d00d31ebd6d9dfc140a1137 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 19 Aug 2026 15:58:06 -0700 Subject: [PATCH 15/27] Lead the sweep walk comment with the IsEmancipated lock requirement Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../SqlClient/ConnectionPool/ChannelDbConnectionPool.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 5d91331d7b..1d049ba31e 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 @@ -1621,10 +1621,10 @@ private void SweepEmancipatedConnections() // up by the next sweep. foreach (DbConnectionInternal connection in _connectionSlots) { - // 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. + // IsEmancipated is only stable under the connection lock, which guards the + // PrePush/PostPop that move it in and out of the pool. TryEnter rather than Enter: a + // connection someone else holds is mid-handout or mid-return, so it is not + // emancipated anyway and blocking on it would only stall that caller. bool locked = false; try { From 26a37d906c4ec0bf9ed355671d3dc573d4436f89 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 19 Aug 2026 16:07:13 -0700 Subject: [PATCH 16/27] Use SpinUntil in the test harness and prune the new comments WaitFor and AdvanceUntil rolled their own sleep loops; both go through SpinWait.SpinUntil now. AdvanceUntil spins in short slices rather than passing the whole timeout, so the fake clock only moves while the condition is unmet. The PoolReclaimer class doc still said the pool sweeps inline before parking, which stopped being true when that sweep was removed. Dropped comments that restate the code or repeat a doc a line or two away, and condensed the slot-walk and enumerator docs to the arguments that aren't evident from the code. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 28 +++++--------- .../ConnectionPool/ConnectionPoolSlots.cs | 10 ++--- .../SqlClient/ConnectionPool/PoolReclaimer.cs | 14 +++---- .../DbConnectionPoolInstrumentationTest.cs | 1 - .../DbConnectionPoolReclamationTest.cs | 3 -- .../ConnectionPool/PoolTestHarness.cs | 38 +++++++++---------- 6 files changed, 37 insertions(+), 57 deletions(-) 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 1d049ba31e..a547ce0fd4 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 @@ -209,8 +209,6 @@ internal ChannelDbConnectionPool( Pruner = new PoolPruner(this, PoolGroupOptions.IdleTimeout); } - // Reclamation applies to every pool configuration. The timer inside is created disarmed and only - // runs while pool consumers are parked waiting for a connection.. Reclaimer = new PoolReclaimer(this, _timeProvider); State = Running; @@ -1516,13 +1514,10 @@ private async Task GetInternalConnection( // timeout. Note that Channels guarantee fair FIFO behavior to callers of ReadAsync // (first-come, first-served), which is crucial to us. // - // Connections whose owning SqlConnection was garbage collected without being - // closed still occupy pool slots, so at MaxPoolSize every subsequent request - // would otherwise wait forever. Rather than sweeping for them here, we register - // with the reclaimer, which sweeps on a timer for as long as anyone is parked - // and routes anything it reclaims back through this channel, waking us here. - // Sweeping inline would cost an O(MaxPoolSize) walk on every saturated acquire - // in applications that never leak. + // Registering with the reclaimer is what keeps a leaked connection from stranding + // us here forever; it sweeps on a timer while anyone is parked and routes what it + // reclaims back through this channel. Sweeping inline instead would cost an + // O(MaxPoolSize) walk on every saturated acquire in applications that never leak. if (connection is null) { Reclaimer.EnterParkedWait(); @@ -1610,15 +1605,12 @@ private void SweepEmancipatedConnections() { List? reclaimed = null; - // Unlike WaitHandleDbConnectionPool, which scans under lock (_objectList), this walk - // takes no collection-level lock: the enumerator reads each slot individually, so it can - // observe a slot that was concurrently emptied or refilled. That is safe because of what - // this sweep is looking for. A connection is only emancipated while it is checked out, - // and a checked-out connection is not in the idle channel, so neither the pruner nor - // Clear can remove it underneath us. The only code that can claim it is another sweep, - // which is excluded by the sweep gate. A concurrently replaced slot can therefore cost - // this sweep a miss, never a connection resurrected after removal, and a miss is picked - // up by the next sweep. + // No collection-level lock, unlike WaitHandleDbConnectionPool's scan under lock + // (_objectList): each slot is read individually, so the walk can see a slot that was + // concurrently emptied or refilled. Safe here because a connection is only emancipated + // while checked out, and a checked-out connection is not in the idle channel, so neither + // the pruner nor Clear can remove it underneath us. A concurrently replaced slot costs + // this sweep a miss, never a connection resurrected after removal. foreach (DbConnectionInternal connection in _connectionSlots) { // IsEmancipated is only stable under the connection lock, which guards the 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 a16a6f7b69..fb315add2a 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 @@ -206,12 +206,10 @@ internal bool TryReplace(DbConnectionInternal oldConnection, DbConnectionInterna } /// - /// Enumerates the connections currently tracked by this collection. The enumeration is - /// best-effort: connections may be added or removed while it is in progress, so callers must - /// tolerate entries that have since left the pool, and an entry added during the walk may or - /// may not be seen. This is safe because the backing array has a fixed capacity and is never - /// reallocated; each slot is read individually. Intended for infrequent bookkeeping passes - /// (e.g. reclaiming emancipated connections), not for hot paths. + /// Enumerates the connections currently tracked by this collection. Best-effort: slots are + /// read individually, so a caller must tolerate entries that have since left the pool, and an + /// entry added during the walk may or may not be seen. Intended for infrequent bookkeeping + /// passes, not for hot paths. /// public IEnumerator GetEnumerator() { diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs index 66241b2519..72099807a5 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs @@ -16,12 +16,11 @@ namespace Microsoft.Data.SqlClient.ConnectionPool /// while callers are parked waiting for a connection. /// /// A connection becomes emancipated when its owning - /// is garbage collected without ever being closed or disposed. The pool sweeps for these inline - /// just before a caller parks on the idle channel, but that single sweep is not enough: - /// emancipation only becomes observable once the garbage collector has collected the owner, which - /// routinely happens after the caller has already parked. Without a background sweep nothing would - /// then reclaim the connection, and every parked caller would wait out its full timeout even - /// though a pool slot was recoverable the whole time. + /// is garbage collected without ever being closed or disposed. Emancipation only becomes + /// observable once the collector has run, which routinely happens after a caller has already + /// parked, so a single sweep at request time cannot cover it. Without a background sweep every + /// parked caller would wait out its full timeout even though a pool slot was recoverable the + /// whole time. /// /// /// The sweep cannot run on the parked caller's own thread. @@ -56,9 +55,6 @@ internal sealed class PoolReclaimer : IDisposable /// internal static readonly TimeSpan SweepInterval = TimeSpan.FromSeconds(1); - /// - /// The owning connection pool that is swept for emancipated connections. - /// private readonly ChannelDbConnectionPool _pool; /// diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs index 772501361e..87fe71b6b6 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs @@ -719,7 +719,6 @@ public void EmancipatedConnection_IsReclaimedAndCounted(PoolImplementation imple pooledConnections: 1, activeConnections: 1); - // The owner is only collectable now that the helper's frame has been popped. CollectAbandonedOwners(); Assert.True(leaked.IsEmancipated); diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolReclamationTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolReclamationTest.cs index ec74b8397e..37ac9bfcfe 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolReclamationTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolReclamationTest.cs @@ -84,9 +84,6 @@ public void WaitingCaller_IsServed_WhenConnectionIsLeakedAfterTheRequestBegins(b maxPoolSize: 1, creationTimeout: PoolWaitMs); - // Rooted in a handle rather than a local so the test controls exactly when the - // connection becomes emancipated, and a collection triggered by a test running in - // parallel cannot do it early. DbConnectionInternal leaked = CheckOutAndRootOwner(pool, out GCHandle leakedOwnerRoot); DbConnectionInternal? served = null; diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/PoolTestHarness.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/PoolTestHarness.cs index 846a635bf1..898a06d204 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/PoolTestHarness.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/PoolTestHarness.cs @@ -35,6 +35,12 @@ public enum PoolImplementation /// internal static class PoolTestHarness { + /// + /// How long a test waits for a cross-thread transition before failing. Generous, because it + /// is only ever reached when something is actually broken. + /// + private static readonly TimeSpan WaitTimeout = TimeSpan.FromSeconds(30); + /// /// Builds the requested pool implementation behind the shared pool interface. /// @@ -138,12 +144,9 @@ internal static DbConnectionInternal CheckOutAndAbandonOwner(IDbConnectionPool p } /// - /// Checks out a connection and roots its owner in a rather than a - /// local. The owner stays alive until the caller frees the returned handle, which gives the - /// test exact control over when the connection becomes emancipated. A local cannot do this: - /// in a Debug build its stack slot roots the object for the rest of the enclosing method - /// even after it is assigned null, so the collection would never happen. The checkout runs - /// in its own non-inlined frame so no slot in the caller's frame ever holds the owner. + /// Checks out a connection and roots its owner in a , so the caller + /// decides exactly when the connection becomes emancipated by freeing the handle. Same + /// non-inlining requirement as . /// [MethodImpl(MethodImplOptions.NoInlining)] internal static DbConnectionInternal CheckOutAndRootOwner(IDbConnectionPool pool, out GCHandle ownerRoot) @@ -173,22 +176,15 @@ internal static void CollectAbandonedOwners() /// /// Spins until holds, failing the test if it does not happen - /// within a generous timeout. Used to observe a background caller reaching a blocking wait, - /// which is inherently a cross-thread transition and cannot be awaited directly. + /// within . Used to observe a background caller reaching a blocking + /// wait, which is inherently a cross-thread transition and cannot be awaited directly. /// internal static void WaitFor(Func condition, string because) - { - Stopwatch stopwatch = Stopwatch.StartNew(); - while (!condition()) - { - Assert.True(stopwatch.Elapsed < TimeSpan.FromSeconds(30), because); - Thread.Sleep(10); - } - } + => Assert.True(SpinWait.SpinUntil(condition, WaitTimeout), because); /// /// Advances until holds, failing the - /// test if it does not happen within a generous timeout. + /// test if it does not happen within . /// /// /// Lets a test assert that a pool eventually does something without knowing what drives it. @@ -200,11 +196,13 @@ internal static void WaitFor(Func condition, string because) internal static void AdvanceUntil(FakeTimeProvider time, Func condition, string because) { Stopwatch stopwatch = Stopwatch.StartNew(); - while (!condition()) + + // Spun in short slices rather than passing the whole timeout, so the clock only moves + // when the condition has not been met yet. + while (!SpinWait.SpinUntil(condition, TimeSpan.FromMilliseconds(50))) { - Assert.True(stopwatch.Elapsed < TimeSpan.FromSeconds(30), because); + Assert.True(stopwatch.Elapsed < WaitTimeout, because); time.Advance(TimeSpan.FromSeconds(1)); - Thread.Sleep(10); } } } From ed65fa11ddf173dd04ba46d5a873d7f2a339981b Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 19 Aug 2026 16:11:38 -0700 Subject: [PATCH 17/27] Keep the shared harness to pool construction The leak-specific helpers only a couple of reclamation tests use were crowding out the part every pool test actually wants. CheckOutAndRootOwner, WaitFor and AdvanceUntil move to their callers; AdvanceUntil is duplicated rather than shared, which is cheaper than a harness nobody can skim. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../DbConnectionPoolInstrumentationTest.cs | 16 +++++ .../DbConnectionPoolReclamationTest.cs | 58 ++++++++++++++++ .../ConnectionPool/PoolTestHarness.cs | 66 +------------------ 3 files changed, 77 insertions(+), 63 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs index 87fe71b6b6..845a7d95bc 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs @@ -5,6 +5,7 @@ using System; using System.Collections.Generic; using System.Data.Common; +using System.Diagnostics; using System.Linq; using System.Runtime.CompilerServices; using System.Threading; @@ -770,6 +771,21 @@ public void EmancipatedConnection_IsReclaimedAndCounted(PoolImplementation imple GC.KeepAlive(waitingOwner); } + /// + /// Advances until holds, failing the + /// test if it does not. Lets a counter assertion wait on a pool that only reclaims from a + /// timer without naming which pool that is. + /// + private static void AdvanceUntil(FakeTimeProvider time, Func condition, string because) + { + Stopwatch stopwatch = Stopwatch.StartNew(); + while (!SpinWait.SpinUntil(condition, TimeSpan.FromMilliseconds(50))) + { + Assert.True(stopwatch.Elapsed < TimeSpan.FromSeconds(30), because); + time.Advance(TimeSpan.FromSeconds(1)); + } + } + #region Test classes /// diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolReclamationTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolReclamationTest.cs index 37ac9bfcfe..95a5d86b4f 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolReclamationTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolReclamationTest.cs @@ -3,6 +3,8 @@ // See the LICENSE file in the project root for more information. using System; +using System.Diagnostics; +using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Threading; using System.Threading.Tasks; @@ -36,6 +38,12 @@ public class DbConnectionPoolReclamationTest /// private const int PoolWaitMs = 30_000; + /// + /// How long a test waits for a cross-thread transition before failing. Generous, because it + /// is only ever reached when something is actually broken. + /// + private static readonly TimeSpan WaitTimeout = TimeSpan.FromSeconds(30); + /// /// The pool's only slot is held by a leaked connection, so the request can only be served by /// reclaiming it. Without reclamation the caller waits out its entire timeout even though a @@ -189,6 +197,35 @@ public void ReclaimedConnection_IsReusedRatherThanReopened(PoolImplementation im return connection; } + /// + /// Checks out a connection and roots its owner in a , so the test + /// decides exactly when the connection becomes emancipated by freeing the handle. Same + /// non-inlining requirement as . + /// + [MethodImpl(MethodImplOptions.NoInlining)] + private static DbConnectionInternal CheckOutAndRootOwner(IDbConnectionPool pool, out GCHandle ownerRoot) + { + SqlConnection owner = new(); + ownerRoot = GCHandle.Alloc(owner); + + Assert.True(pool.TryGetConnection( + owner, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? connection)); + + Assert.NotNull(connection); + return connection!; + } + + /// + /// Spins until holds, failing the test if it does not. Used to + /// observe a background caller reaching a blocking wait, which is a cross-thread transition + /// and cannot be awaited directly. + /// + private static void WaitFor(Func condition, string because) + => Assert.True(SpinWait.SpinUntil(condition, WaitTimeout), because); + /// /// Requests a connection over the sync or async path and blocks until it is served, so both /// paths present the same shape to a test. @@ -213,5 +250,26 @@ private static DbConnectionInternal RequestAndWait(IDbConnectionPool pool, SqlCo Assert.True(async); return completion!.Task.GetAwaiter().GetResult(); } + + /// + /// Advances until holds, failing the + /// test if it does not. + /// + /// + /// Lets a test assert that a pool eventually reclaims without knowing what drives it. + /// WaitHandle reclaims inline when a request finds the pool saturated, so the condition is + /// typically already true on the first check; the channel pool needs its sweep timer to + /// fire. Spun in short slices rather than passing the whole timeout, so the clock only moves + /// while the condition is unmet. + /// + private static void AdvanceUntil(FakeTimeProvider time, Func condition, string because) + { + Stopwatch stopwatch = Stopwatch.StartNew(); + while (!SpinWait.SpinUntil(condition, TimeSpan.FromMilliseconds(50))) + { + Assert.True(stopwatch.Elapsed < WaitTimeout, because); + time.Advance(TimeSpan.FromSeconds(1)); + } + } } } diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/PoolTestHarness.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/PoolTestHarness.cs index 898a06d204..388b0f2842 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/PoolTestHarness.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/PoolTestHarness.cs @@ -3,9 +3,7 @@ // See the LICENSE file in the project root for more information. using System; -using System.Diagnostics; using System.Runtime.CompilerServices; -using System.Runtime.InteropServices; using System.Threading; using Microsoft.Data.Common.ConnectionString; using Microsoft.Data.ProviderBase; @@ -30,17 +28,12 @@ public enum PoolImplementation } /// - /// Construction and lifecycle helpers shared by the connection pool test classes, written - /// against so a test can run against either implementation. + /// Pool construction shared by the connection pool test classes, written against + /// so a test can run against either implementation. Helpers that + /// only a few tests need live with those tests instead. /// internal static class PoolTestHarness { - /// - /// How long a test waits for a cross-thread transition before failing. Generous, because it - /// is only ever reached when something is actually broken. - /// - private static readonly TimeSpan WaitTimeout = TimeSpan.FromSeconds(30); - /// /// Builds the requested pool implementation behind the shared pool interface. /// @@ -143,27 +136,6 @@ internal static DbConnectionInternal CheckOutAndAbandonOwner(IDbConnectionPool p return connection!; } - /// - /// Checks out a connection and roots its owner in a , so the caller - /// decides exactly when the connection becomes emancipated by freeing the handle. Same - /// non-inlining requirement as . - /// - [MethodImpl(MethodImplOptions.NoInlining)] - internal static DbConnectionInternal CheckOutAndRootOwner(IDbConnectionPool pool, out GCHandle ownerRoot) - { - SqlConnection owner = new(); - ownerRoot = GCHandle.Alloc(owner); - - Assert.True(pool.TryGetConnection( - owner, - taskCompletionSource: null, - TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), - out DbConnectionInternal? connection)); - - Assert.NotNull(connection); - return connection!; - } - /// /// Forces collection of an abandoned owner so its connection becomes emancipated. /// @@ -173,37 +145,5 @@ internal static void CollectAbandonedOwners() GC.WaitForPendingFinalizers(); GC.Collect(); } - - /// - /// Spins until holds, failing the test if it does not happen - /// within . Used to observe a background caller reaching a blocking - /// wait, which is inherently a cross-thread transition and cannot be awaited directly. - /// - internal static void WaitFor(Func condition, string because) - => Assert.True(SpinWait.SpinUntil(condition, WaitTimeout), because); - - /// - /// Advances until holds, failing the - /// test if it does not happen within . - /// - /// - /// Lets a test assert that a pool eventually does something without knowing what drives it. - /// The pools differ here: reclaims inline when a - /// request finds it saturated, so the condition is typically already true on the first - /// check, while reclaims from a sweep timer and needs - /// the clock to move. Advancing a pool that did not need it is harmless. - /// - internal static void AdvanceUntil(FakeTimeProvider time, Func condition, string because) - { - Stopwatch stopwatch = Stopwatch.StartNew(); - - // Spun in short slices rather than passing the whole timeout, so the clock only moves - // when the condition has not been met yet. - while (!SpinWait.SpinUntil(condition, TimeSpan.FromMilliseconds(50))) - { - Assert.True(stopwatch.Elapsed < WaitTimeout, because); - time.Advance(TimeSpan.FromSeconds(1)); - } - } } } From 2b74aaef92e3f3ec97a3ea1d29f82616a859a044 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 19 Aug 2026 16:18:57 -0700 Subject: [PATCH 18/27] Remove misleading reclaimer comparison Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Data/SqlClient/ConnectionPool/PoolReclaimer.cs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs index 72099807a5..1f789d49a8 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs @@ -45,13 +45,6 @@ internal sealed class PoolReclaimer : IDisposable /// It is not worth going lower, because emancipation only becomes observable after a garbage /// collection and sweeping faster than the collector produces new information just burns CPU. /// - /// - /// For reference, reclaims on its cleanup timer, - /// which runs on a 2-4 minute randomized schedule by default. That is far too slow to rescue - /// a caller inside its connect timeout, so the legacy pool depends on its own inline sweeps - /// instead. Sweeping only while callers are parked lets this pool afford a much tighter - /// interval than the legacy background cadence while doing strictly less work when idle. - /// /// internal static readonly TimeSpan SweepInterval = TimeSpan.FromSeconds(1); From 5cb6dd78673af35846058fdb0807c1e972a2d2c2 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 19 Aug 2026 16:22:35 -0700 Subject: [PATCH 19/27] Remove redundant reclaimer comment Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs index 1f789d49a8..493abe8274 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs @@ -175,10 +175,6 @@ internal void ExitParkedWait() { lock (_lock) { - // An unbalanced call means some park site is missing its EnterParkedWait, which - // would leave the timer armed forever (or, if it under-counted the other way, stop - // sweeping while a caller is still parked). Clamped rather than thrown in release - // builds: a mis-paired registration is not worth failing a connection attempt over. Debug.Assert(_parkedWaiters > 0, "ExitParkedWait called without a matching EnterParkedWait."); if (_parkedWaiters > 0) From e85d9e743dc9ce044329554e9b4f0fe12850cfe5 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 19 Aug 2026 16:24:16 -0700 Subject: [PATCH 20/27] Remove unnecessary callback lock Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Data/SqlClient/ConnectionPool/PoolReclaimer.cs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs index 493abe8274..0b0ae13749 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs @@ -202,15 +202,6 @@ internal void ExitParkedWait() /// internal void OnSweepCallback() { - lock (_lock) - { - // Disarmed (or disposed) after this callback was already scheduled. - if (!_armed || _disposed) - { - return; - } - } - // Sweep outside the lock: reclamation deactivates connections, which can make server // round trips, and must not block EnterParkedWait/ExitParkedWait on the hot path. try From 556f873d023879cb1e379d1c26bbd49edf844041 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 19 Aug 2026 16:44:30 -0700 Subject: [PATCH 21/27] Remove unnecessary connection keep-alive Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index 65d67c7c8e..17b7d859d2 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -419,8 +419,6 @@ out DbConnectionInternal? failedConnection // Assert Assert.Equal(firstConnection, recycledConnection); await Assert.ThrowsAsync(async () => await failedTask); - - GC.KeepAlive(owningConnections); } /// @@ -2426,4 +2424,3 @@ public void GetConnection_TimeoutTimerReflectsPoolWaitTime() #endregion } } - From 369ae7752bf751bfdca2abda6679b0af9dbd7eaa Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 19 Aug 2026 16:45:44 -0700 Subject: [PATCH 22/27] Remove remaining connection keep-alives Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index 17b7d859d2..3e1a14d42c 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -290,7 +290,6 @@ out DbConnectionInternal? extraConnection // Assert Assert.Equal(firstConnection, extraConnection); - GC.KeepAlive(owningConnections); } /// @@ -488,7 +487,6 @@ out DbConnectionInternal? failedConnection Assert.Equal(firstConnection, recycledConnection); await Assert.ThrowsAsync(async () => failedConnection = await failedCompletionSource.Task); - GC.KeepAlive(owningConnections); } /// From 030750882caeb414094adb573354c5641c02124b Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 19 Aug 2026 16:47:20 -0700 Subject: [PATCH 23/27] Keep pool test owners alive Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index 3e1a14d42c..d647ab0914 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -290,6 +290,7 @@ out DbConnectionInternal? extraConnection // Assert Assert.Equal(firstConnection, extraConnection); + GC.KeepAlive(owningConnections); } /// @@ -418,6 +419,8 @@ out DbConnectionInternal? failedConnection // Assert Assert.Equal(firstConnection, recycledConnection); await Assert.ThrowsAsync(async () => await failedTask); + + GC.KeepAlive(owningConnections); } /// @@ -487,6 +490,7 @@ out DbConnectionInternal? failedConnection Assert.Equal(firstConnection, recycledConnection); await Assert.ThrowsAsync(async () => failedConnection = await failedCompletionSource.Task); + GC.KeepAlive(owningConnections); } /// From cd0b11a5019a9ecc37d9f123837c26a98ff532ef Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 19 Aug 2026 16:55:44 -0700 Subject: [PATCH 24/27] Combine reclamation reuse coverage Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../DbConnectionPoolReclamationTest.cs | 41 +++++-------------- 1 file changed, 10 insertions(+), 31 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolReclamationTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolReclamationTest.cs index 95a5d86b4f..508f0afbd6 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolReclamationTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolReclamationTest.cs @@ -56,8 +56,15 @@ public class DbConnectionPoolReclamationTest [InlineData(PoolImplementation.Channel, true)] public void SaturatedRequest_IsServedByReclaimingLeakedConnection(PoolImplementation implementation, bool async) { + FakeSqlClientMetrics metrics = new(); FakeTimeProvider fakeTime = new(); - IDbConnectionPool pool = ConstructPool(implementation, timeProvider: fakeTime, maxPoolSize: 1, creationTimeout: PoolWaitMs); + IDbConnectionPool pool = ConstructPool( + implementation, + new ChannelDbConnectionPoolTest.SuccessfulSqlConnectionFactory(metrics), + metrics, + fakeTime, + maxPoolSize: 1, + creationTimeout: PoolWaitMs); DbConnectionInternal leaked = CheckOutAndAbandonOwner(pool); CollectAbandonedOwners(); @@ -66,6 +73,8 @@ public void SaturatedRequest_IsServedByReclaimingLeakedConnection(PoolImplementa DbConnectionInternal? served = RequestConnection(pool, async, fakeTime, out SqlConnection waitingOwner); Assert.Same(leaked, served); + Assert.Equal(1, metrics.ReclaimedConnections); + Assert.Equal(1, metrics.HardConnects); GC.KeepAlive(waitingOwner); } @@ -130,36 +139,6 @@ public void WaitingCaller_IsServed_WhenConnectionIsLeakedAfterTheRequestBegins(b GC.KeepAlive(waitingOwner); } - /// - /// A reclaimed connection is reused rather than reopened, and is counted as reclaimed. - /// - [Theory] - [InlineData(PoolImplementation.WaitHandle)] - [InlineData(PoolImplementation.Channel)] - public void ReclaimedConnection_IsReusedRatherThanReopened(PoolImplementation implementation) - { - FakeSqlClientMetrics metrics = new(); - FakeTimeProvider fakeTime = new(); - IDbConnectionPool pool = ConstructPool( - implementation, - new ChannelDbConnectionPoolTest.SuccessfulSqlConnectionFactory(metrics), - metrics, - fakeTime, - maxPoolSize: 1, - creationTimeout: PoolWaitMs); - - DbConnectionInternal leaked = CheckOutAndAbandonOwner(pool); - CollectAbandonedOwners(); - Assert.True(leaked.IsEmancipated); - - DbConnectionInternal? served = RequestConnection(pool, async: false, fakeTime, out SqlConnection waitingOwner); - - Assert.Same(leaked, served); - Assert.Equal(1, metrics.ReclaimedConnections); - Assert.Equal(1, metrics.HardConnects); - GC.KeepAlive(waitingOwner); - } - /// /// Issues a request on a background thread and drives the clock until it completes, so the /// caller can block on a pool that only reclaims from a timer. From 8fd89a34f0287a5966c88d29924a7e853c6806a8 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 19 Aug 2026 17:01:56 -0700 Subject: [PATCH 25/27] Align legacy pool return nullability Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs index afaa827fce..efb130b6cf 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs @@ -1434,7 +1434,7 @@ private bool IsIdleExpired(DbConnectionInternal obj) DateTime.UtcNow - obj.ReturnedTime > idleTimeout; } - public void ReturnInternalConnection(DbConnectionInternal obj, DbConnection owningObject) + public void ReturnInternalConnection(DbConnectionInternal obj, DbConnection? owningObject) { Debug.Assert(obj != null, "null obj?"); From 05d15f9ab191879347534f5a723f3114bb118e92 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 19 Aug 2026 17:02:56 -0700 Subject: [PATCH 26/27] Restore reclaimer callback guard Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Data/SqlClient/ConnectionPool/PoolReclaimer.cs | 8 ++++++++ .../ConnectionPool/WaitHandleDbConnectionPool.cs | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs index 0b0ae13749..ffd4884346 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs @@ -202,6 +202,14 @@ internal void ExitParkedWait() /// internal void OnSweepCallback() { + lock (_lock) + { + if (!_armed || _disposed) + { + return; + } + } + // Sweep outside the lock: reclamation deactivates connections, which can make server // round trips, and must not block EnterParkedWait/ExitParkedWait on the hot path. try diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs index efb130b6cf..afaa827fce 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs @@ -1434,7 +1434,7 @@ private bool IsIdleExpired(DbConnectionInternal obj) DateTime.UtcNow - obj.ReturnedTime > idleTimeout; } - public void ReturnInternalConnection(DbConnectionInternal obj, DbConnection? owningObject) + public void ReturnInternalConnection(DbConnectionInternal obj, DbConnection owningObject) { Debug.Assert(obj != null, "null obj?"); From 24a3c7a7cc56204f13d83c875393c8c2bee98a04 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 20 Aug 2026 08:59:19 -0700 Subject: [PATCH 27/27] Avoid allocations while walking pool slots Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ConnectionPoolSlots.cs | 35 ++++++++++++++++--- .../ConnectionPool/ConnectionPoolSlotsTest.cs | 29 +++++++++++++++ 2 files changed, 59 insertions(+), 5 deletions(-) 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 fb315add2a..7a8eb631f1 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 @@ -211,15 +211,40 @@ internal bool TryReplace(DbConnectionInternal oldConnection, DbConnectionInterna /// entry added during the walk may or may not be seen. Intended for infrequent bookkeeping /// passes, not for hot paths. /// - public IEnumerator GetEnumerator() + public Enumerator GetEnumerator() => new(_connections); + + /// + /// Enumerates the non-null slots without allocating an iterator state machine. + /// + internal struct Enumerator { - for (int i = 0; i < _connections.Length; i++) + private readonly DbConnectionInternal?[] _connections; + private int _index; + private DbConnectionInternal? _current; + + internal Enumerator(DbConnectionInternal?[] connections) + { + _connections = connections; + _index = -1; + _current = null; + } + + public DbConnectionInternal Current => _current!; + + public bool MoveNext() { - DbConnectionInternal? connection = Volatile.Read(ref _connections[i]); - if (connection is not null) + while (++_index < _connections.Length) { - yield return connection; + DbConnectionInternal? connection = Volatile.Read(ref _connections[_index]); + if (connection is not null) + { + _current = connection; + return true; + } } + + _current = null; + return false; } } diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ConnectionPoolSlotsTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ConnectionPoolSlotsTest.cs index 53390068fb..f6b26bcb14 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ConnectionPoolSlotsTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ConnectionPoolSlotsTest.cs @@ -225,6 +225,35 @@ public void Add_MultipleConnections_IncrementsReservationCountCorrectly() Assert.Equal(1, createCallbackCount2); } + [Fact] + public void GetEnumerator_ReturnsTrackedConnections() + { + // Arrange + var poolSlots = new ConnectionPoolSlots(3); + DbConnectionInternal first = poolSlots.Add( + createCallback: () => new MockDbConnectionInternal(), + cleanupCallback: (conn) => { })!; + DbConnectionInternal second = poolSlots.Add( + createCallback: () => new MockDbConnectionInternal(), + cleanupCallback: (conn) => { })!; + + // Act + int count = 0; + bool sawFirst = false; + bool sawSecond = false; + foreach (DbConnectionInternal connection in poolSlots) + { + count++; + sawFirst |= connection == first; + sawSecond |= connection == second; + } + + // Assert + Assert.Equal(2, count); + Assert.True(sawFirst); + Assert.True(sawSecond); + } + [Fact] public void TryRemove_ExistingConnection_ReturnsTrueAndDecrementsReservationCount() {