Avoid confusing double reservation release. Use params in callbacks to avoid closures.

Author: mdaigle

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

@@ -291,19 +291,24 @@ public bool TryGetConnection(
             return false;
         }
 
+        private struct CreateState
+        {
+            internal ChannelDbConnectionPool pool;
+            internal DbConnection? owningConnection;
+            internal DbConnectionOptions userOptions;
+        }
+
         /// <summary>
         /// Opens a new internal connection to the database.
         /// </summary>
         /// <param name="owningConnection">The owning connection.</param>
         /// <param name="userOptions">The options for the connection.</param>
-        /// <param name="async">Whether to open the connection asynchronously.</param>
         /// <param name="cancellationToken">The cancellation token to cancel the operation.</param>
         /// <returns>A task representing the asynchronous operation, with a result of the new internal connection.</returns>
         /// <throws>InvalidOperationException - when the newly created connection is invalid or already in the pool.</throws>
         private DbConnectionInternal? OpenNewInternalConnection(
             DbConnection? owningConnection, 
             DbConnectionOptions userOptions, 
-            bool async,
             CancellationToken cancellationToken)
         {
             cancellationToken.ThrowIfCancellationRequested();
@@ -313,7 +318,7 @@ public bool TryGetConnection(
             // in case of an exception.
 
             return _connectionSlots.Add(
-                createCallback: () =>
+                createCallback: static (state) =>
                 {
                     // https://github.com/dotnet/SqlClient/issues/3459
                     // TODO: This blocks the thread for several network calls!
@@ -323,20 +328,25 @@ public bool TryGetConnection(
                     // DbConnectionInternal doesn't support an async open. It's better to block this thread and keep
                     // throughput high than to queue all of our opens onto a single worker thread. Add an async path 
                     // when this support is added to DbConnectionInternal.
-                    return ConnectionFactory.CreatePooledConnection(
-                        this,
-                        owningConnection,
-                        PoolGroup.ConnectionOptions,
-                        PoolGroup.PoolKey,
-                        userOptions);
+                    return state.pool.ConnectionFactory.CreatePooledConnection(
+                        state.pool,
+                        state.owningConnection,
+                        state.pool.PoolGroup.ConnectionOptions,
+                        state.pool.PoolGroup.PoolKey,
+                        state.userOptions);
                 },
-                cleanupCallback: (newConnection) =>
+                cleanupCallback: static (newConnection, idleConnectionWriter) =>
                 {
-                    if (newConnection is not null)
-                    {
-                        RemoveConnection(newConnection);
-                    }
-                });
+                    idleConnectionWriter.TryWrite(null);
+                    newConnection?.Dispose();
+                },
+                new CreateState
+                {
+                    pool = this,
+                    owningConnection = owningConnection,
+                    userOptions = userOptions
+                },
+                _idleConnectionWriter);
         }
 
         /// <summary>
@@ -367,7 +377,7 @@ private void RemoveConnection(DbConnectionInternal connection)
         {
             _connectionSlots.TryRemove(connection);
 
-            // Closing a connection opens a free spot in the pool.
+            // 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.
             _idleConnectionWriter.TryWrite(null);
@@ -433,7 +443,6 @@ private async Task<DbConnectionInternal> GetInternalConnection(
                     connection ??= OpenNewInternalConnection(
                         owningConnection,
                         userOptions,
-                        async,
                         cancellationToken);
 
                     // If we're at max capacity and couldn't open a connection. Block on the idle channel with a

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

@@ -5,6 +5,7 @@
 using System;
 using System.Diagnostics;
 using System.Threading;
+using System.Threading.Channels;
 using System.Threading.Tasks;
 using Microsoft.Data.ProviderBase;
 
@@ -45,15 +46,17 @@ namespace Microsoft.Data.SqlClient.ConnectionPool
     internal sealed class ConnectionPoolSlots
     {
 
-        private sealed class Reservation : IDisposable
+        private sealed class Reservation<T> : IDisposable
         {
-            private readonly ConnectionPoolSlots _slots;
+            private Action<T> cleanupCallback;
+            private T state;
             private bool _retain = false;
             private bool _disposed = false;
 
-            internal Reservation(ConnectionPoolSlots slots)
+            internal Reservation(T state, Action<T> cleanupCallback)
             {
-                _slots = slots;
+                this.state = state;
+                this.cleanupCallback = cleanupCallback;
             }
 
             public void Dispose()
@@ -65,19 +68,20 @@ public void Dispose()
 
                 if (!_retain)
                 {
-                    _slots.ReleaseReservation();
-                    _disposed = true;
+                    cleanupCallback(state);
                 }
+
+                _disposed = true;
             }
 
             internal void Keep()
             {
                 _retain = true;
             }
-
         }
 
-        internal delegate void CleanupCallback(DbConnectionInternal? connection);
+        internal delegate T CreateCallback<T, S>(S state);
+        internal delegate void CleanupCallback<T>(DbConnectionInternal? connection, T state);
 
         private readonly DbConnectionInternal?[] _connections;
         private readonly uint _capacity;
@@ -104,11 +108,17 @@ internal ConnectionPoolSlots(uint fixedCapacity)
         /// </summary>
         /// <param name="createCallback">The connection to add to the collection.</param>
         /// <param name="cleanupCallback">Callback to clean up resources if an exception occurs.</param>
+        /// <param name="createState">State made available to the create callback.</param>
+        /// <param name="cleanupState">State made available to the cleanup callback.</param>
         /// <exception cref="InvalidOperationException">
         /// Thrown when unable to find an empty slot. 
         /// This can occur if a reservation is not taken before adding a connection.
         /// </exception>
-        internal DbConnectionInternal? Add(Func<DbConnectionInternal?> createCallback, CleanupCallback cleanupCallback)
+        internal DbConnectionInternal? Add<T, S>(
+            CreateCallback<DbConnectionInternal?, T> createCallback, 
+            CleanupCallback<S> cleanupCallback, 
+            T createState,
+            S cleanupState)
         {
             DbConnectionInternal? connection = null;
             try
@@ -119,7 +129,7 @@ internal ConnectionPoolSlots(uint fixedCapacity)
                     return null;
                 }
 
-                connection = createCallback();
+                connection = createCallback(createState);
 
                 if (connection is null)
                 {
@@ -139,7 +149,7 @@ internal ConnectionPoolSlots(uint fixedCapacity)
             }
             catch (Exception e)
             {
-                cleanupCallback(connection);
+                cleanupCallback(connection, cleanupState);
                 throw new Exception("Failed to create or add connection", e);
             }
         }
@@ -177,7 +187,7 @@ internal bool TryRemove(DbConnectionInternal connection)
         /// Attempts to reserve a spot in the collection.
         /// </summary>
         /// <returns>True if a reservation was successfully obtained.</returns>
-        private Reservation? TryReserve()
+        private Reservation<ConnectionPoolSlots>? TryReserve()
         {
             for (var expected = _reservations; expected < _capacity; expected = _reservations)
             {
@@ -191,7 +201,7 @@ internal bool TryRemove(DbConnectionInternal connection)
                     continue;
                 }
 
-                return new Reservation(this);
+                return new Reservation<ConnectionPoolSlots>(this, (slots) => slots.ReleaseReservation());
             }
             return null;
         }