Reclaim emancipated connections, including while callers are parked

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;
@@ -201,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(
@@ -539,6 +546,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(
                 "ChannelDbConnectionPool.ReturnInternalConnection | INFO | {0}, Connection {1}, Deactivating.",
                 Id,
@@ -758,6 +778,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(
+                    "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.
@@ -1465,16 +1498,40 @@ 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.
-                    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)
@@ -1508,6 +1565,71 @@ 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>
+        internal bool ReclaimEmancipatedConnections()
+        {
+            SqlClientEventSource.Log.TryPoolerTraceEvent(
+                "ChannelDbConnectionPool.ReclaimEmancipatedConnections | INFO | {0}, Sweeping for emancipated connections.", Id);
+
+            List<DbConnectionInternal>? reclaimed = null;
+
+            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.
+                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(
+                    "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);
+            }
+
+            return true;
+        }
+
         /// <summary>
         /// Performs a blocking synchronous read from the idle connection channel.
         /// </summary>
@@ -1891,5 +2013,14 @@ internal void PruneConnections(int count)
                 pruned);
         }
         #endregion
+
+        #region Reclamation
+        /// <summary>
+        /// Drives background sweeps for emancipated connections while callers are parked on the idle
+        /// channel. Unlike <see cref="Pruner"/> this is always present, because a connection can leak
+        /// in any pool configuration, including a fixed-size one.
+        /// </summary>
+        internal PoolReclaimer Reclaimer { get; }
+        #endregion
     }
 }

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs

@@ -202,6 +202,52 @@ internal bool TryReplace(DbConnectionInternal oldConnection, DbConnectionInterna
             return false;
         }
 
+        /// <summary>
+        /// 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.
+        /// </summary>
+        public Enumerator GetEnumerator() => new(_connections);
+
+        /// <summary>
+        /// A non-allocating enumerator over the occupied slots of a <see cref="ConnectionPoolSlots"/>.
+        /// Declared as a mutable struct and returned by value from <see cref="GetEnumerator"/> so a
+        /// foreach over the collection allocates nothing; do not copy it into a local.
+        /// </summary>
+        public struct Enumerator
+        {
+            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()
+            {
+                while (++_index < _connections.Length)
+                {
+                    DbConnectionInternal? connection = Volatile.Read(ref _connections[_index]);
+                    if (connection is not null)
+                    {
+                        Current = connection;
+                        return true;
+                    }
+                }
+
+                return false;
+            }
+        }
+
         /// <summary>
         /// Attempts to reserve a spot in the collection.
         /// </summary>