Cleanup | Split out TransactedConnectionPool (#3746)

* Move TransactedConnectionPool to its own class.

* Enable nullable.

* Clean up properties.

* Add doc comments.

* Minor cleanup. Add regions.

* Sync connection pool unit test namespace and folder structure.

* Add unit tests.

* Address copilot comments.

* Address edwardneal's comments.

* Address paul's comments.

* Clean up test variable names. Use ConcurrentBag in tests to avoid concurrency issue.

Author: mdaigle

src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj

@@ -150,6 +150,9 @@
     <Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\ConnectionPool\SqlConnectionPoolProviderInfo.cs">
       <Link>Microsoft\Data\SqlClient\ConnectionPool\SqlConnectionPoolProviderInfo.cs</Link>
     </Compile>
+    <Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\ConnectionPool\TransactedConnectionPool.cs">
+      <Link>Microsoft\Data\SqlClient\ConnectionPool\TransactedConnectionPool.cs</Link>
+    </Compile>
     <Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\ConnectionPool\WaitHandleDbConnectionPool.cs">
       <Link>Microsoft\Data\SqlClient\ConnectionPool\WaitHandleDbConnectionPool.cs</Link>
     </Compile>

src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj

@@ -341,6 +341,9 @@
     <Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\ConnectionPool\SqlConnectionPoolProviderInfo.cs">
       <Link>Microsoft\Data\SqlClient\ConnectionPool\SqlConnectionPoolProviderInfo.cs</Link>
     </Compile>
+    <Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\ConnectionPool\TransactedConnectionPool.cs">
+      <Link>Microsoft\Data\SqlClient\ConnectionPool\TransactedConnectionPool.cs</Link>
+    </Compile>
     <Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\ConnectionPool\WaitHandleDbConnectionPool.cs">
       <Link>Microsoft\Data\SqlClient\ConnectionPool\WaitHandleDbConnectionPool.cs</Link>
     </Compile>

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

@@ -59,22 +59,6 @@ internal sealed class WaitHandleDbConnectionPool : IDbConnectionPool
         // This class is a way to stash our cloned Tx key for later disposal when it's no longer needed.
         // We can't get at the key in the dictionary without enumerating entries, so we stash an extra
         // copy as part of the value.
-        private sealed class TransactedConnectionList : List<DbConnectionInternal>
-        {
-            private Transaction _transaction;
-            internal TransactedConnectionList(int initialAllocation, Transaction tx) : base(initialAllocation)
-            {
-                _transaction = tx;
-            }
-
-            internal void Dispose()
-            {
-                if (_transaction != null)
-                {
-                    _transaction.Dispose();
-                }
-            }
-        }
 
         private sealed class PendingGetConnection
         {
@@ -91,250 +75,6 @@ public PendingGetConnection(long dueTime, DbConnection owner, TaskCompletionSour
             public DbConnectionOptions UserOptions { get; private set; }
         }
 
-        private sealed class TransactedConnectionPool
-        {
-            Dictionary<Transaction, TransactedConnectionList> _transactedCxns;
-
-            IDbConnectionPool _pool;
-
-            private static int _objectTypeCount; // EventSource Counter
-            internal readonly int _objectID = System.Threading.Interlocked.Increment(ref _objectTypeCount);
-
-            internal TransactedConnectionPool(IDbConnectionPool pool)
-            {
-                Debug.Assert(pool != null, "null pool?");
-
-                _pool = pool;
-                _transactedCxns = new Dictionary<Transaction, TransactedConnectionList>();
-                SqlClientEventSource.Log.TryPoolerTraceEvent("<prov.DbConnectionPool.TransactedConnectionPool.TransactedConnectionPool|RES|CPOOL> {0}, Constructed for connection pool {1}", ObjectID, _pool.Id);
-            }
-
-            internal int ObjectID
-            {
-                get
-                {
-                    return _objectID;
-                }
-            }
-
-            internal IDbConnectionPool Pool
-            {
-                get
-                {
-                    return _pool;
-                }
-            }
-
-            internal DbConnectionInternal GetTransactedObject(Transaction transaction)
-            {
-                Debug.Assert(transaction != null, "null transaction?");
-
-                DbConnectionInternal transactedObject = null;
-
-                TransactedConnectionList connections;
-                bool txnFound = false;
-
-                lock (_transactedCxns)
-                {
-                    txnFound = _transactedCxns.TryGetValue(transaction, out connections);
-                }
-
-                // NOTE: GetTransactedObject is only used when AutoEnlist = True and the ambient transaction 
-                //   (Sys.Txns.Txn.Current) is still valid/non-null. This, in turn, means that we don't need 
-                //   to worry about a pending asynchronous TransactionCompletedEvent to trigger processing in
-                //   TransactionEnded below and potentially wipe out the connections list underneath us. It
-                //   is similarly alright if a pending addition to the connections list in PutTransactedObject
-                //   below is not completed prior to the lock on the connections object here...getting a new
-                //   connection is probably better than unnecessarily locking
-                if (txnFound)
-                {
-                    Debug.Assert(connections != null);
-
-                    // synchronize multi-threaded access with PutTransactedObject (TransactionEnded should
-                    //   not be a concern, see comments above)
-                    lock (connections)
-                    {
-                        int i = connections.Count - 1;
-                        if (0 <= i)
-                        {
-                            transactedObject = connections[i];
-                            connections.RemoveAt(i);
-                        }
-                    }
-                }
-
-                if (transactedObject != null)
-                {
-                    SqlClientEventSource.Log.TryPoolerTraceEvent("<prov.DbConnectionPool.TransactedConnectionPool.GetTransactedObject|RES|CPOOL> {0}, Transaction {1}, Connection {2}, Popped.", ObjectID, transaction.GetHashCode(), transactedObject.ObjectID);
-                }
-                return transactedObject;
-            }
-
-            internal void PutTransactedObject(Transaction transaction, DbConnectionInternal transactedObject)
-            {
-                Debug.Assert(transaction != null, "null transaction?");
-                Debug.Assert(transactedObject != null, "null transactedObject?");
-
-                TransactedConnectionList connections;
-                bool txnFound = false;
-
-                // NOTE: because TransactionEnded is an asynchronous notification, there's no guarantee
-                //   around the order in which PutTransactionObject and TransactionEnded are called. 
-
-                lock (_transactedCxns)
-                {
-                    // Check if a transacted pool has been created for this transaction
-                    if (txnFound = _transactedCxns.TryGetValue(transaction, out connections))
-                    {
-                        Debug.Assert(connections != null);
-
-                        // synchronize multi-threaded access with GetTransactedObject
-                        lock (connections)
-                        {
-                            Debug.Assert(0 > connections.IndexOf(transactedObject), "adding to pool a second time?");
-                            SqlClientEventSource.Log.TryPoolerTraceEvent("<prov.DbConnectionPool.TransactedConnectionPool.PutTransactedObject|RES|CPOOL> {0}, Transaction {1}, Connection {2}, Pushing.", ObjectID, transaction.GetHashCode(), transactedObject.ObjectID);
-                            connections.Add(transactedObject);
-                        }
-                    }
-                }
-
-                // CONSIDER: the following code is more complicated than it needs to be to avoid cloning the 
-                //   transaction and allocating memory within a lock. Is that complexity really necessary?
-                if (!txnFound)
-                {
-                    // create the transacted pool, making sure to clone the associated transaction
-                    //   for use as a key in our internal dictionary of transactions and connections
-                    Transaction transactionClone = null;
-                    TransactedConnectionList newConnections = null;
-
-                    try
-                    {
-                        transactionClone = transaction.Clone();
-                        newConnections = new TransactedConnectionList(2, transactionClone); // start with only two connections in the list; most times we won't need that many.
-
-                        lock (_transactedCxns)
-                        {
-                            // NOTE: in the interim between the locks on the transacted pool (this) during 
-                            //   execution of this method, another thread (threadB) may have attempted to 
-                            //   add a different connection to the transacted pool under the same 
-                            //   transaction. As a result, threadB may have completed creating the
-                            //   transacted pool while threadA was processing the above instructions.
-                            if (txnFound = _transactedCxns.TryGetValue(transaction, out connections))
-                            {
-                                Debug.Assert(connections != null);
-
-                                // synchronize multi-threaded access with GetTransactedObject
-                                lock (connections)
-                                {
-                                    Debug.Assert(0 > connections.IndexOf(transactedObject), "adding to pool a second time?");
-                                    SqlClientEventSource.Log.TryPoolerTraceEvent("<prov.DbConnectionPool.TransactedConnectionPool.PutTransactedObject|RES|CPOOL> {0}, Transaction {1}, Connection {2}, Pushing.", ObjectID, transaction.GetHashCode(), transactedObject.ObjectID);
-                                    connections.Add(transactedObject);
-                                }
-                            }
-                            else
-                            {
-                                SqlClientEventSource.Log.TryPoolerTraceEvent("<prov.DbConnectionPool.TransactedConnectionPool.PutTransactedObject|RES|CPOOL> {0}, Transaction {1}, Connection {2}, Adding List to transacted pool.", ObjectID, transaction.GetHashCode(), transactedObject.ObjectID);
-
-                                // add the connection/transacted object to the list
-                                newConnections.Add(transactedObject);
-
-                                _transactedCxns.Add(transactionClone, newConnections);
-                                transactionClone = null; // we've used it -- don't throw it or the TransactedConnectionList that references it away.                                
-                            }
-                        }
-                    }
-                    finally
-                    {
-                        if (transactionClone != null)
-                        {
-                            if (newConnections != null)
-                            {
-                                // another thread created the transaction pool and thus the new 
-                                //   TransactedConnectionList was not used, so dispose of it and
-                                //   the transaction clone that it incorporates.
-                                newConnections.Dispose();
-                            }
-                            else
-                            {
-                                // memory allocation for newConnections failed...clean up unused transactionClone
-                                transactionClone.Dispose();
-                            }
-                        }
-                    }
-                    SqlClientEventSource.Log.TryPoolerTraceEvent("<prov.DbConnectionPool.TransactedConnectionPool.PutTransactedObject|RES|CPOOL> {0}, Transaction {1}, Connection {2}, Added.", ObjectID, transaction.GetHashCode(), transactedObject.ObjectID);
-                }
-
-                SqlClientEventSource.Metrics.EnterFreeConnection();
-            }
-
-            internal void TransactionEnded(Transaction transaction, DbConnectionInternal transactedObject)
-            {
-                SqlClientEventSource.Log.TryPoolerTraceEvent("<prov.DbConnectionPool.TransactedConnectionPool.TransactionEnded|RES|CPOOL> {0}, Transaction {1}, Connection {2}, Transaction Completed", ObjectID, transaction.GetHashCode(), transactedObject.ObjectID);
-                TransactedConnectionList connections;
-                int entry = -1;
-
-                // NOTE: because TransactionEnded is an asynchronous notification, there's no guarantee
-                //   around the order in which PutTransactionObject and TransactionEnded are called. As
-                //   such, it is possible that the transaction does not yet have a pool created.
-
-                // TODO: is this a plausible and/or likely scenario? Do we need to have a mechanism to ensure
-                // TODO:   that the pending creation of a transacted pool for this transaction is aborted when
-                // TODO:   PutTransactedObject finally gets some CPU time?
-
-                lock (_transactedCxns)
-                {
-                    if (_transactedCxns.TryGetValue(transaction, out connections))
-                    {
-                        Debug.Assert(connections != null);
-
-                        bool shouldDisposeConnections = false;
-
-                        // Lock connections to avoid conflict with GetTransactionObject
-                        lock (connections)
-                        {
-                            entry = connections.IndexOf(transactedObject);
-
-                            if (entry >= 0)
-                            {
-                                connections.RemoveAt(entry);
-                            }
-
-                            // Once we've completed all the ended notifications, we can
-                            // safely remove the list from the transacted pool.
-                            if (0 >= connections.Count)
-                            {
-                                SqlClientEventSource.Log.TryPoolerTraceEvent("<prov.DbConnectionPool.TransactedConnectionPool.TransactionEnded|RES|CPOOL> {0}, Transaction {1}, Removing List from transacted pool.", ObjectID, transaction.GetHashCode());
-                                _transactedCxns.Remove(transaction);
-
-                                // we really need to dispose our connection list; it may have 
-                                // native resources via the tx and GC may not happen soon enough.
-                                shouldDisposeConnections = true;
-                            }
-                        }
-
-                        if (shouldDisposeConnections)
-                        {
-                            connections.Dispose();
-                        }
-                    }
-                    else
-                    {
-                        SqlClientEventSource.Log.TryPoolerTraceEvent("<prov.DbConnectionPool.TransactedConnectionPool.TransactionEnded|RES|CPOOL> {0}, Transaction {1}, Connection {2}, Transacted pool not yet created prior to transaction completing. Connection may be leaked.", ObjectID, transaction.GetHashCode(), transactedObject.ObjectID);
-                    }
-                }
-
-                // If (and only if) we found the connection in the list of
-                // connections, we'll put it back...
-                if (0 <= entry)
-                {
-
-                    SqlClientEventSource.Metrics.ExitFreeConnection();
-                    Pool.PutObjectFromTransactedPool(transactedObject);
-                }
-            }
-
-        }
-
         private sealed class PoolWaitHandles
         {
             private readonly Semaphore _poolSemaphore;