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..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 @@ -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; @@ -74,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. @@ -201,6 +209,8 @@ internal ChannelDbConnectionPool( Pruner = new PoolPruner(this, PoolGroupOptions.IdleTimeout); } + Reclaimer = new PoolReclaimer(this, _timeProvider); + State = Running; SqlClientEventSource.Log.TryPoolerTraceEvent( @@ -290,6 +300,13 @@ 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 rather + /// than private so tests can drive the timer bookkeeping directly. + /// + 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 @@ -540,7 +557,7 @@ public DbConnectionInternal ReplaceConnection( } /// - public void ReturnInternalConnection(DbConnectionInternal connection, DbConnection owningObject) + public void ReturnInternalConnection(DbConnectionInternal connection, DbConnection? owningObject) { Metrics.SoftDisconnectRequest(); @@ -765,6 +782,19 @@ public void Shutdown() "ChannelDbConnectionPool.Shutdown | INFO | {0}, Pruner.Dispose threw, continuing shutdown: {1}", Id, ex); } + // 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(); + } + catch (Exception ex) + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + "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 // during the blocking period would keep this pool reachable and continue firing // callbacks/logging after shutdown. @@ -1347,7 +1377,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(); @@ -1483,13 +1513,24 @@ 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 + // + // 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) { - connection ??= ReadChannelSyncOverAsync(cancellationToken); + Reclaimer.EnterParkedWait(); + try + { + connection = async + ? await _idleChannel.ReadAsync(cancellationToken).ConfigureAwait(false) + : ReadChannelSyncOverAsync(cancellationToken); + } + finally + { + Reclaimer.ExitParkedWait(); + } } } catch (OperationCanceledException) @@ -1526,6 +1567,113 @@ 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. + /// + internal void ReclaimEmancipatedConnections() + { + // 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 + { + Monitor.TryEnter(_reclaimSweepGate, ref sweeping); + if (!sweeping) + { + return; + } + + SweepEmancipatedConnections(); + } + finally + { + if (sweeping) + { + Monitor.Exit(_reclaimSweepGate); + } + } + } + + /// + /// Body of . Must be called with + /// held. + /// + private void SweepEmancipatedConnections() + { + List? reclaimed = null; + + // 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 + // 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 + { + Monitor.TryEnter(connection, ref locked); + + if (locked && connection.IsEmancipated) + { + (reclaimed ??= new List()).Add(connection); + } + } + finally + { + if (locked) + { + Monitor.Exit(connection); + } + } + } + + if (reclaimed is null) + { + return; + } + + 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(); + returned++; + } + + if (returned > 0) + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + "ChannelDbConnectionPool.ReclaimEmancipatedConnections | INFO | {0}, Reclaimed {1} emancipated connection(s).", + Id, + returned); + } + } + /// /// 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..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 @@ -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,49 @@ internal bool TryReplace(DbConnectionInternal oldConnection, DbConnectionInterna return false; } + /// + /// 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 Enumerator GetEnumerator() => new(_connections); + + /// + /// Enumerates the non-null slots without allocating an iterator state machine. + /// + internal struct Enumerator + { + 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() + { + while (++_index < _connections.Length) + { + DbConnectionInternal? connection = Volatile.Read(ref _connections[_index]); + if (connection is not null) + { + _current = connection; + return true; + } + } + + _current = null; + return false; + } + } + /// /// Attempts to reserve a spot in the collection. /// 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/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..ffd4884346 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolReclaimer.cs @@ -0,0 +1,256 @@ +// 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.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. 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. + /// 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. + /// + /// + internal static readonly TimeSpan SweepInterval = TimeSpan.FromSeconds(1); + + 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 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 + { + 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( + "PoolReclaimer.EnterParkedWait | INFO | {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) + { + Debug.Assert(_parkedWaiters > 0, "ExitParkedWait called without a matching EnterParkedWait."); + + if (_parkedWaiters > 0) + { + _parkedWaiters--; + } + + if (_parkedWaiters != 0 || !_armed) + { + return; + } + + _armed = false; + _timer.Change(Timeout.InfiniteTimeSpan, Timeout.InfiniteTimeSpan); + + SqlClientEventSource.Log.TryPoolerTraceEvent( + "PoolReclaimer.ExitParkedWait | INFO | {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) + { + 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( + "PoolReclaimer.OnSweepCallback | ERR | {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/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..d647ab0914 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); } /// @@ -2401,4 +2426,3 @@ public void GetConnection_TimeoutTimerReflectsPoolWaitTime() #endregion } } - 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() { diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs index 25d98b351e..845a7d95bc 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolInstrumentationTest.cs @@ -5,7 +5,9 @@ using System; using System.Collections.Generic; using System.Data.Common; +using System.Diagnostics; using System.Linq; +using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; using System.Transactions; @@ -19,6 +21,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 { @@ -30,45 +33,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, @@ -77,32 +45,17 @@ private static IDbConnectionPool ConstructPool( string connectionString = "Data Source=localhost;", int maxPoolSize = 50, int minPoolSize = 0, - int idleTimeout = 0) - { - DbConnectionPoolGroup poolGroup = ConstructPoolGroup(connectionString, maxPoolSize, minPoolSize, idleTimeout); - - return implementation switch - { - PoolImplementation.WaitHandle => new WaitHandleDbConnectionPool( - connectionFactory, - poolGroup, - DbConnectionPoolIdentity.NoIdentity, - new DbConnectionPoolProviderInfo(), - timeProvider: new FakeTimeProvider(), - metrics: metrics), - - PoolImplementation.Channel => new ChannelDbConnectionPool( - connectionFactory, - poolGroup, - DbConnectionPoolIdentity.NoIdentity, - new DbConnectionPoolProviderInfo(), - connectionCreationRateLimiter: null, - timeProvider: new FakeTimeProvider(), - metrics: metrics), - - _ => throw new ArgumentOutOfRangeException(nameof(implementation)), - }; - } + int idleTimeout = 0, + FakeTimeProvider? timeProvider = null) + => 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 @@ -733,6 +686,106 @@ 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. + /// + /// + /// 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. + /// + [Theory] + [InlineData(PoolImplementation.WaitHandle)] + [InlineData(PoolImplementation.Channel)] + public void EmancipatedConnection_IsReclaimedAndCounted(PoolImplementation implementation) + { + // Arrange + FakeSqlClientMetrics metrics = new(); + FakeTimeProvider fakeTime = new(); + IDbConnectionPool pool = ConstructPool( + implementation, + metrics, + new SuccessfulSqlConnectionFactory(metrics), + maxPoolSize: 1, + timeProvider: fakeTime); + + DbConnectionInternal leaked = CheckOutAndAbandonOwner(pool); + AssertCounters( + metrics, + hardConnects: 1, + softConnects: 1, + pooledConnections: 1, + activeConnections: 1); + + CollectAbandonedOwners(); + 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. + 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(); + + 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. + // 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: implementation == PoolImplementation.Channel ? 1 : 0, + pooledConnections: 1, + reclaimedConnections: 1, + activeConnections: 1); + + 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 new file mode 100644 index 0000000000..508f0afbd6 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolReclamationTest.cs @@ -0,0 +1,254 @@ +// 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.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; + + /// + /// 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 + /// 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) + { + 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, fakeTime, out SqlConnection waitingOwner); + + Assert.Same(leaked, served); + Assert.Equal(1, metrics.ReclaimedConnections); + Assert.Equal(1, metrics.HardConnects); + 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); + + 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); + } + + /// + /// 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; + } + + /// + /// 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. + /// + 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(); + } + + /// + /// 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/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..388b0f2842 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/PoolTestHarness.cs @@ -0,0 +1,149 @@ +// 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.CompilerServices; +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, + } + + /// + /// 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 + { + /// + /// 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!; + } + + /// + /// Forces collection of an abandoned owner so its connection becomes emancipated. + /// + internal static void CollectAbandonedOwners() + { + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + } + } +} 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(); }