Reclaim emancipated connections in ChannelDbConnectionPool

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);
+        }
+
+        /// <summary>
+        /// Deactivates a connection that is already marked as owned by the pool (via
+        /// <see cref="DbConnectionInternal.PrePush"/>) 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
+        /// <c>PrePush</c> itself and must not re-validate ownership.
+        /// </summary>
+        /// <param name="connection">The connection to deactivate and route.</param>
+        private void DeactivateAndRouteConnection(DbConnectionInternal connection)
+        {
             SqlClientEventSource.Log.TryPoolerTraceEvent(
                 "<prov.DbConnectionPool.DeactivateObject|RES|CPOOL> {0}, Connection {1}, Deactivating.",
                 Id,
@@ -1340,6 +1354,18 @@ private async Task<DbConnectionInternal> 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<DbConnectionInternal> GetInternalConnection(
             return connection;
         }
 
+        /// <summary>
+        /// Reclaims connections whose owning <see cref="DbConnection"/> 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.
+        /// </summary>
+        /// <returns>True if at least one connection was reclaimed; otherwise, false.</returns>
+        private bool ReclaimEmancipatedConnections()
+        {
+            SqlClientEventSource.Log.TryPoolerTraceEvent(
+                "<prov.DbConnectionPool.ReclaimEmancipatedObjects|RES|CPOOL> {0}", Id);
+
+            List<DbConnectionInternal>? 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<DbConnectionInternal>()).Add(connection);
+                    }
+                }
+                finally
+                {
+                    if (locked)
+                    {
+                        Monitor.Exit(connection);
+                    }
+                }
+            }
+
+            if (reclaimed is null)
+            {
+                return false;
+            }
+
+            foreach (DbConnectionInternal connection in reclaimed)
+            {
+                SqlClientEventSource.Log.TryPoolerTraceEvent(
+                    "<prov.DbConnectionPool.ReclaimEmancipatedObjects|RES|CPOOL> {0}, Connection {1}, Reclaiming.",
+                    Id,
+                    connection.ObjectID);
+
+                connection.DetachCurrentTransactionIfEnded();
+                DeactivateAndRouteConnection(connection);
+            }
+
+            return true;
+        }
+
         /// <summary>
         /// Performs a blocking synchronous read from the idle connection channel.
         /// </summary>

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;
         }
 
+        /// <summary>
+        /// 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.
+        /// </summary>
+        internal List<DbConnectionInternal> Snapshot()
+        {
+            List<DbConnectionInternal> 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;
+        }
+
         /// <summary>
         /// Attempts to reserve a spot in the collection.
         /// </summary>

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;
+
+/// <summary>
+/// Selects the connection pool implementation (<c>WaitHandleDbConnectionPool</c> or
+/// <c>ChannelDbConnectionPool</c>) for the duration of a test.
+///
+/// A pool is bound to an implementation when it is created, so simply flipping the
+/// <c>UseConnectionPoolV2</c> 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 <see cref="LocalAppContextSwitchesHelper"/>, it manipulates global state and enforces a
+/// single-instance policy, so it must not be held for longer than necessary.
+/// </summary>
+public sealed class ConnectionPoolVersionScope : IDisposable
+{
+    private readonly LocalAppContextSwitchesHelper _switches;
+
+    /// <summary>
+    /// Clears all existing pools and selects the requested pool implementation.
+    /// </summary>
+    /// <param name="usePoolV2">
+    /// True to use <c>ChannelDbConnectionPool</c>; false to use <c>WaitHandleDbConnectionPool</c>.
+    /// </param>
+    public ConnectionPoolVersionScope(bool usePoolV2)
+    {
+        _switches = new LocalAppContextSwitchesHelper();
+
+        try
+        {
+            SqlConnection.ClearAllPools();
+            _switches.UseConnectionPoolV2 = usePoolV2;
+        }
+        catch
+        {
+            _switches.Dispose();
+            throw;
+        }
+    }
+
+    /// <summary>
+    /// Clears all pools created under the selected implementation and restores the original
+    /// switch values.
+    /// </summary>
+    public void Dispose()
+    {
+        try
+        {
+            SqlConnection.ClearAllPools();
+        }
+        finally
+        {
+            _switches.Dispose();
+        }
+    }
+}

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
         /// </summary>
         [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
         /// </summary>
         [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();
 

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<SqlConnection> 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);
         }
 
         /// <summary>
@@ -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<SqlConnection> 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<InvalidOperationException>(async () => await failedTask);
+
+            GC.KeepAlive(owningConnections);
         }
 
         /// <summary>
@@ -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<SqlConnection> 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<InvalidOperationException>(async () => failedConnection = await failedCompletionSource.Task);
+
+            GC.KeepAlive(owningConnections);
         }
 
         /// <summary>