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