From c8e7f11e84eb98d118a4f7c7cdaeadb87298b887 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 29 Jul 2026 10:26:54 -0700 Subject: [PATCH 1/3] Implement transaction support in ChannelDbConnectionPool Ports the transacted-pool state machine from WaitHandleDbConnectionPool so the channel pool honors ambient System.Transactions enlistment: - Implement PutObjectFromTransactedPool and TransactionEnded (previously NotImplementedException). - Rewrite ReturnInternalConnection to mirror DeactivateObject: deactivate first, then route the connection to the transacted pool, stasis, the idle channel, or destruction under the connection lock. - Vend connections already enlisted in the ambient transaction via a new GetFromTransactedPool helper, and pass the transaction through to PrepareConnection/ActivateConnection. - Set the ambient transaction on the async acquisition path from the TaskCompletionSource's AsyncState. - Guard RemoveConnection against disposing a transaction root that is still waiting for its delegated transaction to end. Adds ChannelDbConnectionPoolTransactionTest mirroring the WaitHandle pool's transaction test suite, and drops the stale NotImplementedException tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 285 ++++- .../ChannelDbConnectionPoolTest.cs | 31 - .../ChannelDbConnectionPoolTransactionTest.cs | 1079 +++++++++++++++++ 3 files changed, 1337 insertions(+), 58 deletions(-) create mode 100644 src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs 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 b2d4427b47..682e1ae15e 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 @@ -5,6 +5,7 @@ using System.Collections.Concurrent; using System.Collections.ObjectModel; using System.Data.Common; +using System.Diagnostics; using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Channels; @@ -250,6 +251,14 @@ public ConcurrentDictionary< private int MinPoolSize => PoolGroupOptions.MinPoolSize; + /// + /// Indicates whether connections may be vended from (and parked in) the + /// . This mirrors automatic transaction enlistment: + /// when enlistment is disabled a connection is never bound to an ambient transaction, so + /// the transacted store must not be consulted. + /// + private bool HasTransactionAffinity => PoolGroupOptions.HasTransactionAffinity; + /// /// The most recently launched warmup/replenishment loop task, exposed so tests can await a /// warmup pass to a deterministic completion instead of polling pool counters. May be null @@ -313,7 +322,31 @@ public void Clear() /// public void PutObjectFromTransactedPool(DbConnectionInternal connection) { - throw new NotImplementedException(); + Debug.Assert(connection is not null, "null connection?"); + Debug.Assert(connection.EnlistedTransaction is null, "connection is still enlisted?"); + + // Called by the transacted connection pool once it has removed the connection from its + // list. We put the connection back into general circulation. + // + // NOTE: no locking is required here because if we're in this method we can safely + // presume that the caller is the only one using the connection, that all pre-push logic + // has been done, and that all transactions have ended. + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, Transaction has ended.", + Id, + connection.ObjectID); + + if (State is Running && connection.CanBePooled) + { + connection.ResetConnection(); + PutConnectionInIdleChannel(connection); + } + else + { + // RemoveConnection triggers replenishment, which is the channel pool's equivalent + // of the wait handle pool's QueuePoolCreateRequest. + RemoveConnection(connection); + } } /// @@ -331,7 +364,8 @@ public DbConnectionInternal ReplaceConnection( if (newConnection is not null) { - // TODO: Full transaction enlistment support (Story 2). + // Carry the old connection's enlistment over to the replacement so that a connection + // replaced mid-transaction stays bound to the same transaction. PrepareConnection(owningObject, newConnection, oldConnection.EnlistedTransaction); oldConnection.DeactivateConnection(); RemoveConnection(oldConnection); @@ -373,7 +407,8 @@ public DbConnectionInternal ReplaceConnection( newConnection.PostPop(owningObject); } - // TODO: Full transaction enlistment support (Story 2). + // Carry the old connection's enlistment over to the replacement so that a + // connection replaced mid-transaction stays bound to the same transaction. newConnection.ActivateConnection(oldConnection.EnlistedTransaction); // Place new into old's slot @@ -414,6 +449,116 @@ public void ReturnInternalConnection(DbConnectionInternal connection, DbConnecti { ValidateOwnershipAndSetPoolingState(connection, owningObject); + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, Deactivating.", + Id, + connection.ObjectID); + + // Deactivate before inspecting the connection's transaction state. Deactivation is what + // detaches a completed transaction, so reading EnlistedTransaction beforehand could park + // a connection in the transacted pool under a transaction that has already ended. + connection.DeactivateConnection(); + + if (connection.IsConnectionDoomed) + { + // The connection is not fit for reuse -- just dispose of it. + RemoveConnection(connection); + return; + } + + bool returnToGeneralPool = false; + bool destroyConnection = false; + bool rootTxn = false; + + // A connection with a delegated transaction cannot be handed to a different customer + // until the transaction actually completes, so we send it into stasis -- the + // System.Transactions transaction object keeps it owned (not lost) and is certain to + // put it back into the pool. The decision is made under the connection lock so that a + // transaction completing asynchronously on another thread cannot race with us. + lock (connection) + { + if (State is ShuttingDown) + { + if (connection.IsTransactionRoot) + { + // Connections affiliated with a root transaction that happen to live in a + // pool being shut down must be put in stasis so the root transaction isn't + // orphaned with no means to promote itself to a full delegated transaction + // or to Commit/Rollback. + connection.SetInStasis(); + rootTxn = true; + } + else + { + destroyConnection = true; + } + } + else if (connection.IsTransactionRoot && connection.Pool is null) + { + connection.SetInStasis(); + rootTxn = true; + } + else if (connection.CanBePooled) + { + Transaction? transaction = connection.EnlistedTransaction; + if (transaction is not null) + { + // NOTE: we're not locking on State, so its value could change between the + // conditional check above and here. Although perhaps not ideal, this is OK + // because the DelegatedTransactionEnded event will clean up the connection + // appropriately regardless of the pool state. + // + // Transacting connections are held in their own store and are never + // proactively closed (doing so would abort the transaction, which can be + // distributed). Idle-timeout enforcement does not apply here, so we do not + // stamp the returned time when parking the connection in the transacted pool. + TransactedConnectionPool.PutTransactedObject(transaction, connection); + rootTxn = true; + } + else + { + returnToGeneralPool = true; + } + } + else if (connection.IsTransactionRoot) + { + // The connection cannot be pooled but is a transaction root, so we must have + // hit a race condition: either the pool was shut down or the load balancing + // timeout expired and marked the connection as non-poolable while we were + // processing within this lock. Put it in stasis so the root transaction isn't + // orphaned with no means to promote itself to a full delegated transaction or + // to Commit/Rollback. + connection.SetInStasis(); + rootTxn = true; + } + else + { + destroyConnection = true; + } + } + + if (returnToGeneralPool) + { + Debug.Assert(!destroyConnection, "Connection cannot both be pooled and destroyed."); + PutConnectionInIdleChannel(connection); + } + else if (destroyConnection) + { + RemoveConnection(connection); + } + + // Ensure the connection was processed by exactly one of the paths above. + Debug.Assert(rootTxn || returnToGeneralPool || destroyConnection, + "Returned connection was neither pooled, destroyed, nor placed in stasis."); + } + + /// + /// Places a connection that is fit for general reuse into the idle channel, stamping its + /// idle-return time and dropping it if it is no longer live. + /// + /// The connection to make available to other callers. + private void PutConnectionInIdleChannel(DbConnectionInternal connection) + { // Stamp the return time before IsLiveConnection runs so the idle-expiry gate inside it // measures time-in-pool, not time-since-last-return. Without this, a connection whose // checkout exceeded IdleTimeout (e.g. a long-running query) would be wrongly evicted on @@ -432,27 +577,12 @@ public void ReturnInternalConnection(DbConnectionInternal connection, DbConnecti return; } - SqlClientEventSource.Log.TryPoolerTraceEvent( - " {0}, Connection {1}, Deactivating.", - Id, - connection.ObjectID); - connection.DeactivateConnection(); - - if (connection.IsConnectionDoomed || - !connection.CanBePooled || - State == ShuttingDown) + if (!_idleChannel.TryWrite(connection)) { + // The channel has been completed (pool is shutting down). Race window + // between the State check by the caller and TryWrite: destroy instead of pooling. RemoveConnection(connection); } - else - { - if (!_idleChannel.TryWrite(connection)) - { - // The channel has been completed (pool is shutting down). Race window - // between the State check above and TryWrite: destroy instead of pooling. - RemoveConnection(connection); - } - } } /// @@ -606,7 +736,21 @@ public void Startup() /// public void TransactionEnded(Transaction transaction, DbConnectionInternal transactedObject) { - throw new NotImplementedException(); + Debug.Assert(transaction is not null, "null transaction?"); + Debug.Assert(transactedObject is not null, "null transactedObject?"); + + // Note: the connection may still be associated with the transaction due to the explicit + // unbinding requirement. + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Transaction {1}, Connection {2}, Transaction Completed", + Id, + transaction.GetHashCode(), + transactedObject.ObjectID); + + // If the connection is in the transacted pool, remove it there and return it to general + // circulation. TransactedConnectionPool.TransactionEnded calls back into + // PutObjectFromTransactedPool for us once it has removed the connection from its list. + TransactedConnectionPool.TransactionEnded(transaction, transactedObject); } /// @@ -674,7 +818,8 @@ public bool TryGetConnection( // We're potentially on a new thread, so we need to properly set the ambient transaction. // We rely on the caller to capture the ambient transaction in the TaskCompletionSource's AsyncState // so that we can access it here. Read: area for improvement. - // TODO: ADP.SetCurrentTransaction(taskCompletionSource.Task.AsyncState as Transaction); + ADP.SetCurrentTransaction(taskCompletionSource.Task.AsyncState as Transaction); + DbConnectionInternal? connection = null; try @@ -941,6 +1086,19 @@ private bool IsLiveConnection(DbConnectionInternal connection) /// The connection to be closed. private void RemoveConnection(DbConnectionInternal connection) { + // A connection with a delegated transaction cannot be disposed of until the delegated + // transaction has actually completed; disposing it would abort the (possibly + // distributed) transaction. Leave it alone: when the transaction completes it comes + // back through PutObjectFromTransactedPool, which calls us again. + if (connection.IsTxRootWaitingForTxEnd) + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, Has Delegated Transaction, waiting to Dispose.", + Id, + connection.ObjectID); + return; + } + _connectionSlots.TryRemove(connection); // Removing a connection from the pool opens a free slot. @@ -1010,14 +1168,26 @@ private async Task GetInternalConnection( TimeoutTimer timeout) { DbConnectionInternal? connection = null; + Transaction? transaction = null; + + // If automatic transaction enlistment is enabled, we first try to get a connection that + // is already enlisted in the ambient transaction. If enlistment is not enabled we cannot + // vend connections from the transacted pool. + if (HasTransactionAffinity) + { + connection = GetFromTransactedPool(out transaction); + } // Derive a CancellationTokenSource from the TimeoutTimer so pool-internal wait operations // (channel reads, semaphore waits) are cancelled when the overall budget expires. using CancellationTokenSource cancellationTokenSource = timeout.CreateCancellationTokenSource(); CancellationToken cancellationToken = cancellationTokenSource.Token; - // Continue looping until we create or retrieve a connection - do + // Continue looping until we create or retrieve a connection. A connection vended from + // the transacted pool skips this loop entirely: it has already been liveness-checked and + // is exempt from the idle/generation gates, because closing it would abort its + // (possibly distributed) transaction. + while (connection is null) { try { @@ -1062,9 +1232,8 @@ private async Task GetInternalConnection( connection = null; } } - while (connection is null); - PrepareConnection(owningConnection, connection); + PrepareConnection(owningConnection, connection, transaction); return connection; } @@ -1133,6 +1302,68 @@ private void PrepareConnection(DbConnection owningObject, DbConnectionInternal c } } + /// + /// Attempts to retrieve a connection that is already enlisted in the ambient transaction. + /// + /// Receives the ambient transaction, or null when there is none. + /// The caller must enlist whatever connection it ends up using in this transaction, even + /// when no transacted connection was available. + /// A live connection already enlisted in the ambient transaction, or null. + private DbConnectionInternal? GetFromTransactedPool(out Transaction? transaction) + { + transaction = ADP.GetCurrentTransaction(); + if (transaction is null) + { + return null; + } + + DbConnectionInternal? connection = TransactedConnectionPool.GetTransactedObject(transaction); + if (connection is null) + { + return null; + } + + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, Popped from transacted pool.", + Id, + connection.ObjectID); + + SqlClientDiagnostics.Metrics.ExitFreeConnection(); + + // Transacting connections are exempt from idle-timeout and clear-generation eviction + // (closing them would abort the transaction, which may be distributed), so only + // liveness is checked here rather than the full IsLiveConnection gate. + if (connection.IsTransactionRoot) + { + try + { + // A dead transaction root must surface the underlying failure to the caller: + // there is no way to recover the delegated transaction on another connection. + connection.IsConnectionAlive(throwOnException: true); + } + catch + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, found dead and removed.", + Id, + connection.ObjectID); + RemoveConnection(connection); + throw; + } + } + else if (!connection.IsConnectionAlive()) + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, found dead and removed.", + Id, + connection.ObjectID); + RemoveConnection(connection); + connection = null; + } + + return connection; + } + /// /// Validates that the connection is owned by the provided DbConnection and that it is in a valid state to be returned to the pool. /// diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index 604964927e..90417c5e96 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -887,37 +887,6 @@ public void TestReplaceConnection() #endregion - #region Not Implemented Method Tests - - /// - /// Verifies that remains - /// unimplemented and throws . - /// - [Fact] - public void TestPutObjectFromTransactedPool() - { - // Arrange - var pool = ConstructPool(SuccessfulConnectionFactory); - - // Act & Assert - Assert.Throws(() => pool.PutObjectFromTransactedPool(null!)); - } - - /// - /// Verifies that - /// remains unimplemented and throws . - /// - [Fact] - public void TestTransactionEnded() - { - // Arrange - var pool = ConstructPool(SuccessfulConnectionFactory); - - // Act & Assert - Assert.Throws(() => pool.TransactionEnded(null!, null!)); - } - #endregion - #region Pool Clear Tests /// diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs new file mode 100644 index 0000000000..7d2bfb95c5 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs @@ -0,0 +1,1079 @@ +// 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. + +using System; +using System.Data; +using System.Data.Common; +using System.Threading; +using System.Threading.Tasks; +using System.Transactions; +using Microsoft.Data.Common.ConnectionString; +using Microsoft.Data.ProviderBase; +using Microsoft.Data.SqlClient.ConnectionPool; +using Xunit; + +namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool; + +/// +/// Deterministic tests for ChannelDbConnectionPool transaction functionality. +/// These tests exercise transacted connection pathways with controlled synchronization +/// to verify correct behavior without relying on probabilistic concurrency. +/// +public class ChannelDbConnectionPoolTransactionTest : IDisposable +{ + private const int DefaultMaxPoolSize = 50; + private const int DefaultMinPoolSize = 0; + private const int DefaultCreationTimeoutInMilliseconds = 15000; + + private IDbConnectionPool _pool = null!; + + public ChannelDbConnectionPoolTransactionTest() + { + _pool = CreatePool(); + } + + public void Dispose() + { + // Verify no leaked transactions before cleanup + Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); + + _pool?.Shutdown(); + _pool?.Clear(); + } + + #region Helper Methods + + private ChannelDbConnectionPool CreatePool( + int maxPoolSize = DefaultMaxPoolSize, + int minPoolSize = DefaultMinPoolSize, + bool hasTransactionAffinity = true) + { + var poolGroupOptions = new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: minPoolSize, + maxPoolSize: maxPoolSize, + creationTimeout: DefaultCreationTimeoutInMilliseconds, + loadBalanceTimeout: 0, + hasTransactionAffinity: hasTransactionAffinity, + idleTimeout: 0 + ); + + var dbConnectionPoolGroup = new DbConnectionPoolGroup( + new SqlConnectionOptions("Data Source=localhost;"), + new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), + poolGroupOptions + ); + + var connectionFactory = new MockSqlConnectionFactory(); + + var pool = new ChannelDbConnectionPool( + connectionFactory, + dbConnectionPoolGroup, + DbConnectionPoolIdentity.NoIdentity, + new DbConnectionPoolProviderInfo() + ); + + pool.Startup(); + return pool; + } + + private DbConnectionInternal GetConnection(SqlConnection owner) + { + _pool.TryGetConnection( + owner, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? connection); + return connection!; + } + + private async Task GetConnectionAsync( + SqlConnection owner, + Transaction? transaction = null) + { + var tcs = new TaskCompletionSource(transaction); + _pool.TryGetConnection( + owner, + taskCompletionSource: tcs, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? connection); + return connection ?? await tcs.Task; + } + + private void ReturnConnection(DbConnectionInternal connection, SqlConnection owner) + { + _pool.ReturnInternalConnection(connection, owner); + } + + private void AssertPoolMetrics() + { + Assert.True(_pool.Count <= _pool.PoolGroupOptions.MaxPoolSize, + $"Pool count ({_pool.Count}) exceeded max pool size ({_pool.PoolGroupOptions.MaxPoolSize})"); + Assert.True(_pool.Count >= 0, + $"Pool count ({_pool.Count}) is negative"); + Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); + } + + #endregion + + #region Transaction Routing Tests + + [Fact] + public void GetConnection_UnderTransaction_RoutesToTransactedPool() + { + // Arrange & Act + using var scope = new TransactionScope(); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + + ReturnConnection(conn, owner); + + // Assert - connection should be in the transacted pool + Assert.True(_pool.TransactedConnectionPool.TransactedConnections.ContainsKey(transaction)); + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]); + + scope.Complete(); + } + + [Fact] + public void GetConnection_WithoutTransaction_RoutesToGeneralPool() + { + // Arrange & Act (no TransactionScope) + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + + ReturnConnection(conn, owner); + + // Assert - transacted pool should be empty + Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); + } + + [Fact] + public void GetConnection_UnderTransaction_ReturnsSameConnectionFromTransactedPool() + { + // Arrange + using var scope = new TransactionScope(); + + // Act - first call creates a new connection + var owner1 = new SqlConnection(); + var conn1 = GetConnection(owner1); + Assert.NotNull(conn1); + ReturnConnection(conn1, owner1); + + // Second call should retrieve the SAME connection from the transacted pool (LIFO) + var owner2 = new SqlConnection(); + var conn2 = GetConnection(owner2); + Assert.NotNull(conn2); + Assert.Same(conn1, conn2); + + ReturnConnection(conn2, owner2); + scope.Complete(); + } + + [Fact] + public async Task GetConnectionAsync_UnderTransaction_ReturnsSameConnectionFromTransactedPool() + { + // Arrange + using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); + var transaction = Transaction.Current; + + // Act - first call creates a new connection + var owner1 = new SqlConnection(); + var conn1 = await GetConnectionAsync(owner1, transaction: transaction); + Assert.NotNull(conn1); + ReturnConnection(conn1, owner1); + + // Second call should retrieve the SAME connection from the transacted pool + var owner2 = new SqlConnection(); + var conn2 = await GetConnectionAsync(owner2, transaction: transaction); + Assert.NotNull(conn2); + Assert.Same(conn1, conn2); + + ReturnConnection(conn2, owner2); + scope.Complete(); + } + + [Fact] + public void GetConnection_WithTransactionAffinityDisabled_SkipsTransactedPool() + { + // Arrange + _pool.Shutdown(); + _pool.Clear(); + _pool = CreatePool(hasTransactionAffinity: false); + + using var scope = new TransactionScope(); + + // Act + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + // Assert - even though a transaction is active, transacted pool is not used + Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); + + scope.Complete(); + } + + #endregion + + #region Transaction Lifecycle Tests + + [Fact] + public void TransactionCommit_ClearsTransactedPool() + { + // Arrange & Act + using (var scope = new TransactionScope()) + { + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + // While transaction is active, connection should be in transacted pool + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + + scope.Complete(); + } + + // Assert - after transaction completes, transacted pool should be empty + AssertPoolMetrics(); + } + + [Fact] + public void TransactionRollback_ClearsTransactedPool() + { + // Arrange & Act + using (var scope = new TransactionScope()) + { + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + + // Don't call scope.Complete() — triggers rollback + } + + // Assert - transacted pool should be empty after rollback too + AssertPoolMetrics(); + } + + [Fact] + public void MultipleGetReturn_SameTransaction_ReusesConnection() + { + // Arrange + using var scope = new TransactionScope(); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + // Act - get and return multiple times within same transaction + for (int i = 0; i < 10; i++) + { + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + } + + // Assert - only one connection should be in the transacted pool + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]); + + scope.Complete(); + } + + [Fact] + public async Task MultipleGetReturn_SameTransaction_Async_ReusesConnection() + { + // Arrange + using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + // Act - get and return multiple times within same transaction + for (int i = 0; i < 10; i++) + { + var owner = new SqlConnection(); + var conn = await GetConnectionAsync(owner, transaction: transaction); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + } + + // Assert - only one connection should be in the transacted pool + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]); + + scope.Complete(); + } + + [Fact] + public void AlternatingCommitAndRollback_MaintainsConsistentState() + { + // Act - alternate between commit and rollback + for (int i = 0; i < 20; i++) + { + using var scope = new TransactionScope(); + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + if (i % 2 == 0) + { + scope.Complete(); + } + // else: rollback (no Complete) + } + + // Assert + AssertPoolMetrics(); + } + + #endregion + + #region Nested Transaction Tests + + [Fact] + public void NestedTransaction_Required_SharesSameTransactedEntry() + { + // Arrange + using var outerScope = new TransactionScope(); + var outerTxn = Transaction.Current; + Assert.NotNull(outerTxn); + + var owner1 = new SqlConnection(); + var conn1 = GetConnection(owner1); + Assert.NotNull(conn1); + ReturnConnection(conn1, owner1); + + // Act - nested scope with Required shares the same transaction + using (var innerScope = new TransactionScope(TransactionScopeOption.Required)) + { + Assert.Same(outerTxn, Transaction.Current); + + var owner2 = new SqlConnection(); + var conn2 = GetConnection(owner2); + Assert.NotNull(conn2); + Assert.Same(conn1, conn2); // Same transaction -> same connection from transacted pool + ReturnConnection(conn2, owner2); + + // Only one transaction tracked + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + + innerScope.Complete(); + } + + outerScope.Complete(); + } + + [Fact] + public void NestedTransaction_RequiresNew_CreatesSeparateTransactedEntry() + { + // Arrange + using var outerScope = new TransactionScope(); + var outerTxn = Transaction.Current; + Assert.NotNull(outerTxn); + + var owner1 = new SqlConnection(); + var conn1 = GetConnection(owner1); + Assert.NotNull(conn1); + ReturnConnection(conn1, owner1); + + // Act - nested scope with RequiresNew creates a new transaction + using (var innerScope = new TransactionScope(TransactionScopeOption.RequiresNew)) + { + var innerTxn = Transaction.Current; + Assert.NotNull(innerTxn); + Assert.NotEqual(outerTxn, innerTxn); + + var owner2 = new SqlConnection(); + var conn2 = GetConnection(owner2); + Assert.NotNull(conn2); + Assert.NotSame(conn1, conn2); // Different transaction -> different connection + ReturnConnection(conn2, owner2); + + // Two separate transactions tracked + Assert.Equal(2, _pool.TransactedConnectionPool.TransactedConnections.Count); + + innerScope.Complete(); + } + + outerScope.Complete(); + } + + [Fact] + public void NestedTransaction_RequiresNew_CompletesIndependently() + { + // Arrange & Act + using (var outerScope = new TransactionScope()) + { + var owner1 = new SqlConnection(); + var conn1 = GetConnection(owner1); + Assert.NotNull(conn1); + ReturnConnection(conn1, owner1); + + using (var innerScope = new TransactionScope(TransactionScopeOption.RequiresNew)) + { + var owner2 = new SqlConnection(); + var conn2 = GetConnection(owner2); + Assert.NotNull(conn2); + ReturnConnection(conn2, owner2); + innerScope.Complete(); + } + + // Inner transaction completed - its entry should be cleared + // Outer transaction entry should still exist + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + + outerScope.Complete(); + } + + // Both completed + AssertPoolMetrics(); + } + + [Fact] + public void DeeplyNestedTransactions_RequiresNew_AllTrackedSeparately() + { + // Arrange & Act + using var scope1 = new TransactionScope(); + var owner1 = new SqlConnection(); + var conn1 = GetConnection(owner1); + ReturnConnection(conn1, owner1); + + using var scope2 = new TransactionScope(TransactionScopeOption.RequiresNew); + var owner2 = new SqlConnection(); + var conn2 = GetConnection(owner2); + ReturnConnection(conn2, owner2); + + using var scope3 = new TransactionScope(TransactionScopeOption.RequiresNew); + var owner3 = new SqlConnection(); + var conn3 = GetConnection(owner3); + ReturnConnection(conn3, owner3); + + // Assert - three separate transactions tracked + Assert.Equal(3, _pool.TransactedConnectionPool.TransactedConnections.Count); + + scope3.Complete(); + scope2.Complete(); + scope1.Complete(); + } + + [Fact] + public void DeeplyNestedTransactions_Required_AllShareOneEntry() + { + // Arrange & Act + using var scope1 = new TransactionScope(); + var txn = Transaction.Current; + var owner1 = new SqlConnection(); + var conn1 = GetConnection(owner1); + ReturnConnection(conn1, owner1); + + using var scope2 = new TransactionScope(TransactionScopeOption.Required); + Assert.Same(txn, Transaction.Current); + var owner2 = new SqlConnection(); + var conn2 = GetConnection(owner2); + Assert.Same(conn1, conn2); + ReturnConnection(conn2, owner2); + + using var scope3 = new TransactionScope(TransactionScopeOption.Required); + Assert.Same(txn, Transaction.Current); + var owner3 = new SqlConnection(); + var conn3 = GetConnection(owner3); + Assert.Same(conn1, conn3); + ReturnConnection(conn3, owner3); + + // Assert - single transaction entry + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + + scope3.Complete(); + scope2.Complete(); + scope1.Complete(); + } + + #endregion + + #region Mixed Transacted and Non-Transacted Tests + + [Fact] + public void MixedWorkload_AlternatingTransactedAndNonTransacted() + { + // Act - alternate between transacted and non-transacted + for (int i = 0; i < 10; i++) + { + if (i % 2 == 0) + { + using var scope = new TransactionScope(); + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + scope.Complete(); + } + else + { + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + } + } + + // Assert + AssertPoolMetrics(); + } + + #endregion + + #region Shared Transaction Tests + + [Fact] + public void SharedTransaction_DependentScopes_UseTransactedPool() + { + // Arrange + using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + // Act - first connection + var owner1 = new SqlConnection(); + var conn1 = GetConnection(owner1); + Assert.NotNull(conn1); + ReturnConnection(conn1, owner1); + + // Use dependent scope on same transaction + using (var innerScope = new TransactionScope(transaction)) + { + Assert.Same(transaction, Transaction.Current); + var owner2 = new SqlConnection(); + var conn2 = GetConnection(owner2); + Assert.NotNull(conn2); + Assert.Same(conn1, conn2); // Same transaction -> same connection + ReturnConnection(conn2, owner2); + innerScope.Complete(); + } + + // Assert - still one transaction entry + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]); + + scope.Complete(); + } + + #endregion + + #region Pool Saturation with Transactions Tests + + [Fact] + public void PoolSaturation_BlocksUntilConnectionAvailable() + { + // Arrange - small pool + _pool.Shutdown(); + _pool.Clear(); + _pool = CreatePool(maxPoolSize: 1); + + using var allAcquired = new ManualResetEventSlim(false); + using var releaseFirst = new ManualResetEventSlim(false); + + var saturatingTask = Task.Run(() => + { + using var scope = new TransactionScope(); + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + + allAcquired.Set(); // Signal that this connection is held + + Assert.True(releaseFirst.Wait(TimeSpan.FromSeconds(15)), + "Timed out waiting for releaseFirst signal."); + + ReturnConnection(conn, owner); + scope.Complete(); + }); + + Assert.True(allAcquired.Wait(TimeSpan.FromSeconds(10)), + "Timed out waiting for connection to be acquired."); + Assert.Equal(1, _pool.Count); + + using var acquired = new ManualResetEventSlim(false); + var waitingTask = Task.Run(() => + { + using var scope = new TransactionScope(); + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + acquired.Set(); + ReturnConnection(conn, owner); + scope.Complete(); + }); + + // Give the waiting task time to block — it should NOT complete yet + Assert.False(acquired.Wait(TimeSpan.FromMilliseconds(500)), + "Waiting task should not have acquired a connection while pool is saturated"); + + // Release one connection to unblock the waiting task + releaseFirst.Set(); + + // Now the waiting task should complete + Assert.True(waitingTask.Wait(TimeSpan.FromSeconds(15)), + "Waiting task should have completed after a connection was released"); + Assert.True(acquired.IsSet); + + // Cleanup remaining held connections + Task.WaitAll(saturatingTask); + } + + #endregion + + #region Controlled Concurrency Tests + + // Flaky under CI load only (never reproduces locally): the two worker tasks are + // scheduled via Task.Run on the thread pool. On a loaded agent the pool can be slow to + // spin up a worker, so task1 starts late and fails to signal task1Returned within + // task2's 10s wait, producing a WaitAll timeout. That is thread-pool starvation, not a + // pool/transaction defect. + [Trait("Category", "flaky")] + [Fact] + public void TwoThreads_SharedTransaction_AccessSameTransactedEntry() + { + // Arrange + // Use 3-phase synchronization so task1 gets AND returns before task2 requests. + // This ensures the connection is back in the transacted pool for task2 to reuse. + using var task1Returned = new ManualResetEventSlim(false); + using var task2Done = new ManualResetEventSlim(false); + DbConnectionInternal? connFromTask1 = null; + DbConnectionInternal? connFromTask2 = null; + + using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + // Act - two threads sharing the same transaction, sequenced so the + // transacted pool can vend the same connection to both. + var task1 = Task.Run(() => + { + using var innerScope = new TransactionScope(transaction); + var owner = new SqlConnection(); + connFromTask1 = GetConnection(owner); + Assert.NotNull(connFromTask1); + + // Return the connection so it's available in the transacted pool + ReturnConnection(connFromTask1, owner); + innerScope.Complete(); + + task1Returned.Set(); // Signal: connection is back in the transacted pool + }); + + var task2 = Task.Run(() => + { + // Wait until task1 has returned the connection to the transacted pool + Assert.True(task1Returned.Wait(TimeSpan.FromSeconds(10)), + "Timed out waiting for task1 to return its connection."); + + using var innerScope = new TransactionScope(transaction); + var owner = new SqlConnection(); + connFromTask2 = GetConnection(owner); + Assert.NotNull(connFromTask2); + ReturnConnection(connFromTask2, owner); + innerScope.Complete(); + }); + + Task.WaitAll(task1, task2); + + // Both tasks should have received the same connection via the transacted pool + Assert.Same(connFromTask1, connFromTask2); + scope.Complete(); + } + + [Fact] + public async Task TwoThreads_SeparateTransactions_Async_IsolatedTransactedEntries() + { + // Arrange + using var barrier = new SemaphoreSlim(0, 2); + + // Act + var task1 = Task.Run(async () => + { + using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); + var transaction = Transaction.Current; + var owner = new SqlConnection(); + var conn = await GetConnectionAsync(owner, transaction: transaction); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + barrier.Release(); // Signal ready + await barrier.WaitAsync(); // Wait for other task + + scope.Complete(); + }); + + var task2 = Task.Run(async () => + { + using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); + var transaction = Transaction.Current; + var owner = new SqlConnection(); + var conn = await GetConnectionAsync(owner, transaction: transaction); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + barrier.Release(); // Signal ready + await barrier.WaitAsync(); // Wait for other task + + scope.Complete(); + }); + + await Task.WhenAll(task1, task2); + + // Assert + AssertPoolMetrics(); + } + + #endregion + + #region Pool Shutdown with Transactions Tests + + [Fact] + public void PoolShutdown_AfterTransactionComplete_NoLeaks() + { + // Arrange + using (var scope = new TransactionScope()) + { + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + scope.Complete(); + } + + // Act + _pool.Shutdown(); + + // Assert + AssertPoolMetrics(); + } + + [Fact] + public void PoolShutdown_WhileConnectionHeld_NoException() + { + // Arrange + using var scope = new TransactionScope(); + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + + // Act - shutdown while connection is held (not yet returned) + _pool.Shutdown(); + + // Return after shutdown — the pool deactivates and disposes the connection + // rather than returning it to the pool. Verify this doesn't throw. + ReturnConnection(conn, owner); + + // Assert + // The connection should have been deactivated and disposed (not returned to the pool). + // After Dispose(), IsConnectionDoomed is set to true and Pool is set to null. + Assert.True(conn.IsConnectionDoomed, + "Connection should be doomed after returning to a shut-down pool."); + Assert.Null(conn.Pool); + } + + #endregion + + #region Transaction Complete Before Return Tests + + [Fact] + public void TransactionComplete_ThenReturn_ConnectionStillReturned() + { + // Arrange + var owner = new SqlConnection(); + DbConnectionInternal conn; + + using (var scope = new TransactionScope()) + { + conn = GetConnection(owner); + Assert.NotNull(conn); + scope.Complete(); + } + // Transaction is fully disposed here + + // Act - return connection after transaction ended + ReturnConnection(conn, owner); + + // Assert - no leak, pool metrics consistent + AssertPoolMetrics(); + Assert.True(_pool.Count > 0, "Pool should still have the connection"); + } + + #endregion + + #region Sequential Transaction Isolation Tests + + [Fact] + public void SequentialTransactions_EachGetsOwnTransactedEntry() + { + // Act - create multiple sequential transactions + for (int i = 0; i < 5; i++) + { + using var scope = new TransactionScope(); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + // Only the current transaction should be tracked + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + Assert.True(_pool.TransactedConnectionPool.TransactedConnections.ContainsKey(transaction)); + + scope.Complete(); + } + + // Assert - after all are done, pool should be clean + AssertPoolMetrics(); + } + + [Fact] + public async Task SequentialTransactions_Async_EachGetsOwnTransactedEntry() + { + // Act - create multiple sequential transactions + for (int i = 0; i < 5; i++) + { + using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + var owner = new SqlConnection(); + var conn = await GetConnectionAsync(owner, transaction: transaction); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + Assert.True(_pool.TransactedConnectionPool.TransactedConnections.ContainsKey(transaction)); + + scope.Complete(); + } + + // Assert + AssertPoolMetrics(); + } + + [Fact] + public void SequentialTransactions_CanReuseConnections() + { + // Act + DbConnectionInternal conn1; + DbConnectionInternal conn2; + Transaction? txn1; + Transaction? txn2; + + using (var scope1 = new TransactionScope()) + { + txn1 = Transaction.Current; + var owner1 = new SqlConnection(); + conn1 = GetConnection(owner1); + Assert.NotNull(conn1); + ReturnConnection(conn1, owner1); + scope1.Complete(); + } + + using (var scope2 = new TransactionScope()) + { + txn2 = Transaction.Current; + var owner2 = new SqlConnection(); + conn2 = GetConnection(owner2); + Assert.NotNull(conn2); + ReturnConnection(conn2, owner2); + scope2.Complete(); + } + + // Assert + // The connection was returned to the general pool and picked up by the second transaction + Assert.NotSame(txn1, txn2); + Assert.Same(conn1, conn2); + AssertPoolMetrics(); + } + + #endregion + + #region Transacted Pool Plumbing Tests + + [Fact] + public void TransactionCompletion_ReturnsConnectionToIdleChannel() + { + // Arrange - park a connection in the transacted pool. + DbConnectionInternal conn; + using (var scope = new TransactionScope()) + { + var owner = new SqlConnection(); + conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + // While the transaction is live the connection is held by the transacted pool and is + // deliberately absent from the idle channel. + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + Assert.Equal(0, _pool.IdleCount); + + scope.Complete(); + } + + // Assert - completion drives TransactionEnded -> PutObjectFromTransactedPool, which puts + // the connection back into general circulation. + Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); + Assert.Equal(1, _pool.IdleCount); + Assert.Equal(1, _pool.Count); + + // The connection is now reusable by a caller with no ambient transaction. + var owner2 = new SqlConnection(); + var conn2 = GetConnection(owner2); + Assert.Same(conn, conn2); + ReturnConnection(conn2, owner2); + } + + [Fact] + public void TransactionCompletion_AfterShutdown_DestroysConnection() + { + // Arrange - park a connection in the transacted pool, then shut the pool down. The + // transacted connection survives the shutdown drain because closing it would abort the + // (possibly distributed) transaction. + DbConnectionInternal conn; + using (var scope = new TransactionScope()) + { + var owner = new SqlConnection(); + conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + // Act + _pool.Shutdown(); + scope.Complete(); + } + + // Assert - a shut-down pool must not re-pool the connection when the transaction ends. + Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); + Assert.Equal(0, _pool.IdleCount); + Assert.Equal(0, _pool.Count); + Assert.True(conn.IsConnectionDoomed); + } + + [Fact] + public void TransactionEnded_UnknownConnection_DoesNotPoolConnection() + { + // Arrange - a connection that was never parked in the transacted pool. + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + + using var scope = new TransactionScope(); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + // Act + _pool.TransactionEnded(transaction!, conn); + + // Assert - nothing to remove, so the connection stays checked out. + Assert.Equal(0, _pool.IdleCount); + Assert.Equal(1, _pool.Count); + + ReturnConnection(conn, owner); + scope.Complete(); + } + + [Fact] + public void ReplaceConnection_CarriesEnlistedTransactionToNewConnection() + { + // Arrange + using var scope = new TransactionScope(); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + var owner = new SqlConnection(); + var oldConnection = GetConnection(owner); + Assert.NotNull(oldConnection); + Assert.Equal(1, _pool.Count); + + // Act + var newConnection = _pool.ReplaceConnection( + owner, + oldConnection, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); + + // Assert - a distinct connection took over the old connection's slot and enlistment. + Assert.NotNull(newConnection); + Assert.NotSame(oldConnection, newConnection); + Assert.Equal(1, _pool.Count); + Assert.True(oldConnection.IsConnectionDoomed); + + ReturnConnection(newConnection, owner); + + // The replacement inherited the transaction, so it parks in the transacted pool. + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction!]); + + scope.Complete(); + } + + #endregion + + #region Mock Classes + + internal class MockSqlConnectionFactory : SqlConnectionFactory + { + protected override DbConnectionInternal CreateConnection( + SqlConnectionOptions options, + ConnectionPoolKey poolKey, + DbConnectionPoolGroupProviderInfo poolGroupProviderInfo, + IDbConnectionPool pool, + DbConnection owningConnection, + TimeoutTimer timeout) + { + return new MockDbConnectionInternal(); + } + } + + internal class MockDbConnectionInternal : DbConnectionInternal + { + private static int s_nextId = 1; + public int MockId { get; } = Interlocked.Increment(ref s_nextId); + + public override string ServerVersion => "Mock"; + + public override ConnectionCapabilities Capabilities => new(); + + public override DbTransaction BeginTransaction(System.Data.IsolationLevel il) + { + throw new NotImplementedException(); + } + + public override void EnlistTransaction(Transaction? transaction) + { + if (transaction != null) + { + EnlistedTransaction = transaction; + } + } + + protected override void Activate(Transaction? transaction) + { + EnlistedTransaction = transaction; + } + + protected override void Deactivate() + { + } + + public override string ToString() => $"MockConnection_{MockId}"; + + internal override void ResetConnection() + { + } + } + + #endregion +} From c5404962c619e3854e688e3987bdb7cfb17d3730 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 29 Jul 2026 11:57:04 -0700 Subject: [PATCH 2/3] Flow the ambient transaction explicitly on the async open path The async open path ran GetInternalConnection inside a Task.Run and restored the ambient transaction by assigning Transaction.Current on that thread pool thread. That assignment writes to thread-static storage which ExecutionContext does not unwind, so the transaction outlived the open and was observable by unrelated work later scheduled onto the same thread -- including the login-time auto-enlistment that non-pooled connections perform against Transaction.Current. A try/finally restore is not sufficient either, because the continuation may resume on a different thread than the one that was polluted. Instead, capture the ambient transaction on the caller's thread (from the TaskCompletionSource's AsyncState, which is where SqlConnection.OpenAsync puts it) and thread it explicitly through GetInternalConnection into GetFromTransactedPool and PrepareConnection. The sync path passes ADP.GetCurrentTransaction() directly since it runs on the caller's thread. Also gate the transaction on HasTransactionAffinity in one place so a pool without automatic enlistment neither reads from nor writes to the transacted store. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 65 ++++++++------ .../ChannelDbConnectionPoolTransactionTest.cs | 90 +++++++++++++++++++ 2 files changed, 129 insertions(+), 26 deletions(-) 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 682e1ae15e..3f8db0d018 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 @@ -776,10 +776,12 @@ public bool TryGetConnection( // If taskCompletionSource is null, we are in a sync context. if (taskCompletionSource is null) { + // We're on the caller's thread, so the ambient transaction is directly observable. var task = GetInternalConnection( owningObject, async: false, - timeout); + timeout, + ADP.GetCurrentTransaction()); // When running synchronously, we are guaranteed that the task is already completed. // We don't need to guard the managed threadpool at this spot because we pass the async flag as false @@ -808,6 +810,22 @@ public bool TryGetConnection( // OpenAsync call. This means that we cannot cancel the connection open operation if the caller's token // is cancelled. We can only cancel based on our own timeout, which is set to the owningObject's // ConnectionTimeout. + // The ambient transaction is captured here, on the caller's thread, because + // Transaction.Current does not flow into the Task.Run below: a TransactionScope keeps + // the ambient transaction in thread-static storage unless it was created with + // TransactionScopeAsyncFlowOption.Enabled. We rely on the caller to capture the ambient + // transaction in the TaskCompletionSource's AsyncState, and then hand it to + // GetInternalConnection explicitly. + // + // Note that we deliberately do not assign Transaction.Current on the thread pool + // thread. That assignment writes to thread-static storage which is *not* unwound when + // the ExecutionContext is restored, so it would outlive this open and be observed by + // unrelated work later scheduled onto the same thread pool thread -- including the + // login-time auto-enlistment that non-pooled connections perform against + // Transaction.Current. The WaitHandle pool can get away with assigning it because it + // processes pending opens on a dedicated non-thread-pool thread. + Transaction? ambientTransaction = taskCompletionSource.Task.AsyncState as Transaction; + Task.Run(async () => { if (taskCompletionSource.Task.IsCompleted) @@ -815,11 +833,6 @@ public bool TryGetConnection( return; } - // We're potentially on a new thread, so we need to properly set the ambient transaction. - // We rely on the caller to capture the ambient transaction in the TaskCompletionSource's AsyncState - // so that we can access it here. Read: area for improvement. - ADP.SetCurrentTransaction(taskCompletionSource.Task.AsyncState as Transaction); - DbConnectionInternal? connection = null; try @@ -827,7 +840,8 @@ public bool TryGetConnection( connection = await GetInternalConnection( owningObject, async: true, - timeout + timeout, + ambientTransaction ).ConfigureAwait(false); if (!taskCompletionSource.TrySetResult(connection)) @@ -1153,6 +1167,10 @@ private void RemoveConnection(DbConnectionInternal connection) /// A boolean indicating whether the operation should be asynchronous. /// The overall timeout budget for this connection request. Time spent waiting /// in the pool is deducted from the budget available for physical connection creation. + /// The ambient transaction captured on the caller's thread, or + /// null when the caller is not inside a transaction. It is passed explicitly rather than read + /// from because this method may run on a thread pool thread + /// that the ambient transaction does not flow to. /// Returns a DbConnectionInternal that is retrieved from the pool. /// /// Thrown when an OperationCanceledException is caught, indicating that the timeout period @@ -1165,17 +1183,20 @@ private void RemoveConnection(DbConnectionInternal connection) private async Task GetInternalConnection( DbConnection owningConnection, bool async, - TimeoutTimer timeout) + TimeoutTimer timeout, + Transaction? ambientTransaction) { DbConnectionInternal? connection = null; - Transaction? transaction = null; - // If automatic transaction enlistment is enabled, we first try to get a connection that - // is already enlisted in the ambient transaction. If enlistment is not enabled we cannot - // vend connections from the transacted pool. - if (HasTransactionAffinity) + // When automatic enlistment is disabled, the connection must never be bound to the + // ambient transaction, so we neither consult the transacted store nor hand the + // transaction to activation. HasTransactionAffinity is derived from the connection + // string's Enlist keyword. + Transaction? transaction = HasTransactionAffinity ? ambientTransaction : null; + + if (transaction is not null) { - connection = GetFromTransactedPool(out transaction); + connection = GetFromTransactedPool(transaction); } // Derive a CancellationTokenSource from the TimeoutTimer so pool-internal wait operations @@ -1303,20 +1324,12 @@ private void PrepareConnection(DbConnection owningObject, DbConnectionInternal c } /// - /// Attempts to retrieve a connection that is already enlisted in the ambient transaction. + /// Attempts to retrieve a connection that is already enlisted in the given transaction. /// - /// Receives the ambient transaction, or null when there is none. - /// The caller must enlist whatever connection it ends up using in this transaction, even - /// when no transacted connection was available. - /// A live connection already enlisted in the ambient transaction, or null. - private DbConnectionInternal? GetFromTransactedPool(out Transaction? transaction) + /// The transaction the connection must already be enlisted in. + /// A live connection already enlisted in the transaction, or null. + private DbConnectionInternal? GetFromTransactedPool(Transaction transaction) { - transaction = ADP.GetCurrentTransaction(); - if (transaction is null) - { - return null; - } - DbConnectionInternal? connection = TransactedConnectionPool.GetTransactedObject(transaction); if (connection is null) { diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs index 7d2bfb95c5..53667f418d 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs @@ -1021,6 +1021,96 @@ public void ReplaceConnection_CarriesEnlistedTransactionToNewConnection() #endregion + #region Async Ambient Transaction Flow Tests + + /// + /// A created without + /// keeps the ambient transaction in thread-static storage, so it is not observable from the thread + /// pool thread the pool opens on. The pool must therefore take the transaction from the + /// 's AsyncState, which is where SqlConnection.OpenAsync + /// captures it. + /// + [Fact] + public async Task GetConnectionAsync_AmbientTransactionNotFlowed_StillEnlistsFromAsyncState() + { + // Arrange - a transaction that is never ambient on any thread, so AsyncState is the only + // way the pool can learn about it. + using var transaction = new CommittableTransaction(); + Assert.Null(Transaction.Current); + Assert.Null(await Task.Run(() => Transaction.Current)); + + // Act + var owner = new SqlConnection(); + var connection = await GetConnectionAsync(owner, transaction); + + // Assert + Assert.NotNull(connection); + Assert.Equal(transaction, connection.EnlistedTransaction); + + ReturnConnection(connection, owner); + + // Being enlisted, the connection parks in the transacted pool rather than the idle channel. + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]); + Assert.Equal(0, _pool.IdleCount); + + transaction.Rollback(); + } + + /// + /// Assigning writes to thread-static storage that the + /// ExecutionContext does not unwind, so doing it on a thread pool thread would leave a stale + /// transaction behind for unrelated work later scheduled onto that same thread -- including the + /// login-time auto-enlistment that non-pooled connections perform against the ambient + /// transaction. The pool must pass the transaction explicitly instead of assigning it. + /// + [Fact] + public async Task GetConnectionAsync_DoesNotLeakAmbientTransactionOntoThreadPool() + { + // Arrange & Act - several async opens under a transaction, each on a thread pool thread. + for (int i = 0; i < 8; i++) + { + using var transaction = new CommittableTransaction(); + var owner = new SqlConnection(); + var connection = await GetConnectionAsync(owner, transaction); + ReturnConnection(connection, owner); + transaction.Rollback(); + } + + // Assert - no thread pool thread was left with an ambient transaction. + for (int i = 0; i < 16; i++) + { + Assert.Null(await Task.Run(() => Transaction.Current)); + } + } + + /// + /// The synchronous path runs on the caller's thread, where the ambient transaction set by a + /// TransactionScope is directly observable and must still be honored. + /// + [Fact] + public void GetConnection_Sync_UsesAmbientTransactionFromCallersThread() + { + // Arrange + using var scope = new TransactionScope(); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + // Act - no transaction is handed to the pool explicitly; it must read Transaction.Current. + var owner = new SqlConnection(); + var connection = GetConnection(owner); + + // Assert + Assert.NotNull(connection); + Assert.Equal(transaction, connection.EnlistedTransaction); + + ReturnConnection(connection, owner); + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction!]); + + scope.Complete(); + } + + #endregion + #region Mock Classes internal class MockSqlConnectionFactory : SqlConnectionFactory From 62d7dc9c2172864237bf89b553aad7511734dead Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 29 Jul 2026 12:16:23 -0700 Subject: [PATCH 3/3] Address review feedback Source: - Flesh out the PutObjectFromTransactedPool asserts to say what invariant is being violated and why it matters. - Replace the five-way nested branch and three bool flags in ReturnInternalConnection with a ReturnDisposition enum and a single DecideReturnDisposition helper. The "shutting down", "transaction root with no pool" and "no longer poolable" cases all collapse into one reusability test followed by "stasis if it's a transaction root, otherwise destroy", which removes the need for the postcondition assert entirely. - Move the transacted-store lookup inside the acquisition loop so a connection returned to the store while we were looping is preferred over opening a fresh one. It breaks out of the loop to keep skipping the idle/generation gate, which must not apply to a transacted connection. - Document why a parked transacted connection is exempt from idle timeout and when its idle clock actually starts. Tests: - Rewrite the suite around specific behaviors: 16 focused tests replacing 33. Dropped the loop-driven stress tests (alternating commit/rollback, mixed workloads, deeply nested scopes, pool saturation, the flaky two-thread test) and the granular cases that were covered by a round trip. - Name the tests after the operation whose behavior they pin down, and document what invariant each one protects. - Assert full pool accounting (Count / IdleCount / transacted connections) at every step via AssertPoolState rather than spot-checking one collection. - Inject a frozen FakeTimeProvider so background maintenance cannot race the assertions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 212 ++-- .../ChannelDbConnectionPoolTransactionTest.cs | 1068 +++++------------ 2 files changed, 431 insertions(+), 849 deletions(-) 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 3f8db0d018..2e52feb9a4 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 @@ -322,8 +322,12 @@ public void Clear() /// public void PutObjectFromTransactedPool(DbConnectionInternal connection) { - Debug.Assert(connection is not null, "null connection?"); - Debug.Assert(connection.EnlistedTransaction is null, "connection is still enlisted?"); + Debug.Assert(connection is not null, + "PutObjectFromTransactedPool was called with a null connection."); + Debug.Assert(connection.EnlistedTransaction is null, + "PutObjectFromTransactedPool was called with a connection that is still enlisted. " + + "The transaction must have ended and been detached before the connection returns to " + + "general circulation, otherwise it could be vended to a caller in a different transaction."); // Called by the transacted connection pool once it has removed the connection from its // list. We put the connection back into general circulation. @@ -466,90 +470,110 @@ public void ReturnInternalConnection(DbConnectionInternal connection, DbConnecti return; } - bool returnToGeneralPool = false; - bool destroyConnection = false; - bool rootTxn = false; - - // A connection with a delegated transaction cannot be handed to a different customer - // until the transaction actually completes, so we send it into stasis -- the - // System.Transactions transaction object keeps it owned (not lost) and is certain to - // put it back into the pool. The decision is made under the connection lock so that a - // transaction completing asynchronously on another thread cannot race with us. + // The disposition is decided while holding the connection's lock so that a transaction + // completing asynchronously on another thread cannot race with us, but the resulting + // I/O is performed outside of it. + ReturnDisposition disposition; lock (connection) { - if (State is ShuttingDown) - { - if (connection.IsTransactionRoot) - { - // Connections affiliated with a root transaction that happen to live in a - // pool being shut down must be put in stasis so the root transaction isn't - // orphaned with no means to promote itself to a full delegated transaction - // or to Commit/Rollback. - connection.SetInStasis(); - rootTxn = true; - } - else - { - destroyConnection = true; - } - } - else if (connection.IsTransactionRoot && connection.Pool is null) - { - connection.SetInStasis(); - rootTxn = true; - } - else if (connection.CanBePooled) - { - Transaction? transaction = connection.EnlistedTransaction; - if (transaction is not null) - { - // NOTE: we're not locking on State, so its value could change between the - // conditional check above and here. Although perhaps not ideal, this is OK - // because the DelegatedTransactionEnded event will clean up the connection - // appropriately regardless of the pool state. - // - // Transacting connections are held in their own store and are never - // proactively closed (doing so would abort the transaction, which can be - // distributed). Idle-timeout enforcement does not apply here, so we do not - // stamp the returned time when parking the connection in the transacted pool. - TransactedConnectionPool.PutTransactedObject(transaction, connection); - rootTxn = true; - } - else - { - returnToGeneralPool = true; - } - } - else if (connection.IsTransactionRoot) + disposition = DecideReturnDisposition(connection); + } + + switch (disposition) + { + case ReturnDisposition.Reuse: + PutConnectionInIdleChannel(connection); + break; + + case ReturnDisposition.Destroy: + RemoveConnection(connection); + break; + + case ReturnDisposition.HeldByTransaction: + // Nothing further to do. The connection is parked in the transacted store or + // in stasis, and comes back through PutObjectFromTransactedPool once its + // transaction ends. + break; + } + } + + /// + /// Decides what should happen to a returning connection that has already been deactivated + /// and is not doomed. Must be called while holding the connection's lock: parking a + /// connection in the transacted store has to be atomic with respect to a transaction + /// completing on another thread. + /// + /// The connection being returned. + /// The action the caller must take for this connection. + private ReturnDisposition DecideReturnDisposition(DbConnectionInternal connection) + { + // A connection is only fit to go back into circulation if the pool is still running, + // the connection itself is still poolable, and it isn't a transaction root that has + // already been detached from its pool. + bool isReusable = + State is Running && + connection.CanBePooled && + !(connection.IsTransactionRoot && connection.Pool is null); + + if (!isReusable) + { + // A transaction root that cannot be pooled must be put in stasis rather than + // closed. Closing it would orphan the root transaction with no means to promote + // itself to a full delegated transaction, or to commit or roll back. + // System.Transactions keeps the connection owned (not lost) and is certain to + // hand it back to us when the transaction ends. + if (connection.IsTransactionRoot) { - // The connection cannot be pooled but is a transaction root, so we must have - // hit a race condition: either the pool was shut down or the load balancing - // timeout expired and marked the connection as non-poolable while we were - // processing within this lock. Put it in stasis so the root transaction isn't - // orphaned with no means to promote itself to a full delegated transaction or - // to Commit/Rollback. connection.SetInStasis(); - rootTxn = true; - } - else - { - destroyConnection = true; + return ReturnDisposition.HeldByTransaction; } - } - if (returnToGeneralPool) - { - Debug.Assert(!destroyConnection, "Connection cannot both be pooled and destroyed."); - PutConnectionInIdleChannel(connection); + return ReturnDisposition.Destroy; } - else if (destroyConnection) + + // A connection that is still enlisted cannot be handed to a different customer until + // its transaction actually completes, so it is parked in the transacted store keyed by + // that transaction and comes back via PutObjectFromTransactedPool when it ends. + // + // NOTE: we do not hold a lock on State, so its value could have changed since the + // check above. That is acceptable because the DelegatedTransactionEnded event cleans + // the connection up appropriately regardless of the pool state. + // + // Transacted connections are deliberately not stamped with a returned time: they are + // never proactively closed (doing so would abort a possibly distributed transaction), + // so idle-timeout enforcement does not apply while they are parked. They are stamped + // when they rejoin the idle channel in PutObjectFromTransactedPool. + Transaction? transaction = connection.EnlistedTransaction; + if (transaction is not null) { - RemoveConnection(connection); + TransactedConnectionPool.PutTransactedObject(transaction, connection); + return ReturnDisposition.HeldByTransaction; } - // Ensure the connection was processed by exactly one of the paths above. - Debug.Assert(rootTxn || returnToGeneralPool || destroyConnection, - "Returned connection was neither pooled, destroyed, nor placed in stasis."); + return ReturnDisposition.Reuse; + } + + /// + /// The outcome of evaluating a connection that is being returned to the pool. + /// + private enum ReturnDisposition + { + /// + /// The connection is fit for general reuse and belongs in the idle channel. + /// + Reuse, + + /// + /// The connection cannot be reused and must be closed. + /// + Destroy, + + /// + /// The connection is owned by a live transaction, either parked in the transacted + /// store or held in stasis, and must not be touched by the pool until that + /// transaction ends. + /// + HeldByTransaction, } /// @@ -565,6 +589,13 @@ private void PutConnectionInIdleChannel(DbConnectionInternal connection) // return even though it was actively in use on the wire. The same gating conditions are // applied here as in IsLiveConnection so we avoid the per-return timestamp read when // idle expiry is disabled or the legacy idle-timeout behavior is in effect. + // + // A connection parked in the transacted store does not pass through here, so it is not + // subject to idle timeout for as long as its transaction is live. That is intentional + // and matches WaitHandleDbConnectionPool: closing it would abort a possibly distributed + // transaction. It is stamped here when the transaction ends and + // PutObjectFromTransactedPool returns it to general circulation, so the idle clock + // starts from the moment it actually becomes available to other callers. if (!LocalAppContextSwitches.UseLegacyIdleTimeoutBehavior && PoolGroupOptions.IdleTimeout != TimeSpan.Zero) { @@ -1194,24 +1225,35 @@ private async Task GetInternalConnection( // string's Enlist keyword. Transaction? transaction = HasTransactionAffinity ? ambientTransaction : null; - if (transaction is not null) - { - connection = GetFromTransactedPool(transaction); - } - // Derive a CancellationTokenSource from the TimeoutTimer so pool-internal wait operations // (channel reads, semaphore waits) are cancelled when the overall budget expires. using CancellationTokenSource cancellationTokenSource = timeout.CreateCancellationTokenSource(); CancellationToken cancellationToken = cancellationTokenSource.Token; - // Continue looping until we create or retrieve a connection. A connection vended from - // the transacted pool skips this loop entirely: it has already been liveness-checked and - // is exempt from the idle/generation gates, because closing it would abort its - // (possibly distributed) transaction. + // Continue looping until we create or retrieve a connection. while (connection is null) { try { + // A connection already enlisted in our transaction is always preferred, since + // reusing it avoids promoting the transaction to a distributed one. This is + // re-checked on every iteration so that a connection returned to the transacted + // store while we were looping is picked up rather than being passed over in + // favor of a fresh connection. + if (transaction is not null) + { + connection = GetFromTransactedPool(transaction); + if (connection is not null) + { + // Skip the liveness/idle/generation gate at the bottom of the loop: + // GetFromTransactedPool has already probed liveness, and a transacted + // connection is exempt from idle-timeout, load-balance and + // clear-generation eviction because closing it would abort its + // (possibly distributed) transaction. + break; + } + } + // Optimistically try to get an idle connection from the channel // Doesn't wait if the channel is empty, just returns null. connection ??= GetIdleConnection(); diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs index 53667f418d..a69941bbbf 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs @@ -3,7 +3,6 @@ // See the LICENSE file in the project root for more information. using System; -using System.Data; using System.Data.Common; using System.Threading; using System.Threading.Tasks; @@ -11,14 +10,16 @@ using Microsoft.Data.Common.ConnectionString; using Microsoft.Data.ProviderBase; using Microsoft.Data.SqlClient.ConnectionPool; +using Microsoft.Extensions.Time.Testing; using Xunit; namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool; /// -/// Deterministic tests for ChannelDbConnectionPool transaction functionality. -/// These tests exercise transacted connection pathways with controlled synchronization -/// to verify correct behavior without relying on probabilistic concurrency. +/// Tests for transaction affinity: routing a returning +/// connection to the transacted store instead of the idle channel, vending an already-enlisted +/// connection back to the same transaction, releasing the connection when the transaction ends, +/// and flowing the ambient transaction on the asynchronous open path. /// public class ChannelDbConnectionPoolTransactionTest : IDisposable { @@ -26,7 +27,7 @@ public class ChannelDbConnectionPoolTransactionTest : IDisposable private const int DefaultMinPoolSize = 0; private const int DefaultCreationTimeoutInMilliseconds = 15000; - private IDbConnectionPool _pool = null!; + private IDbConnectionPool _pool; public ChannelDbConnectionPoolTransactionTest() { @@ -35,15 +36,21 @@ public ChannelDbConnectionPoolTransactionTest() public void Dispose() { - // Verify no leaked transactions before cleanup + // A transaction entry that outlives its transaction is a leak: the connections it holds + // are never returned to general circulation. Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); - _pool?.Shutdown(); - _pool?.Clear(); + _pool.Shutdown(); + _pool.Clear(); } #region Helper Methods + /// + /// Builds a pool for these tests. A frozen is injected so that + /// time-driven background maintenance (idle-timeout pruning, warmup and replenishment, + /// blocking-period expiry) cannot advance and race the assertions about pool contents. + /// private ChannelDbConnectionPool CreatePool( int maxPoolSize = DefaultMaxPoolSize, int minPoolSize = DefaultMinPoolSize, @@ -65,19 +72,33 @@ private ChannelDbConnectionPool CreatePool( poolGroupOptions ); - var connectionFactory = new MockSqlConnectionFactory(); - var pool = new ChannelDbConnectionPool( - connectionFactory, + new MockSqlConnectionFactory(), dbConnectionPoolGroup, DbConnectionPoolIdentity.NoIdentity, - new DbConnectionPoolProviderInfo() + new DbConnectionPoolProviderInfo(), + timeProvider: new FakeTimeProvider() ); pool.Startup(); return pool; } + /// + /// Tears down the pool built by the constructor and replaces it with one configured + /// differently, for the few tests that need non-default pool options. + /// + private void ReplaceFixturePool(bool hasTransactionAffinity) + { + _pool.Shutdown(); + _pool.Clear(); + _pool = CreatePool(hasTransactionAffinity: hasTransactionAffinity); + } + + /// + /// Opens a connection synchronously. The pool reads the ambient transaction off the calling + /// thread on this path. + /// private DbConnectionInternal GetConnection(SqlConnection owner) { _pool.TryGetConnection( @@ -88,6 +109,12 @@ private DbConnectionInternal GetConnection(SqlConnection owner) return connection!; } + /// + /// Opens a connection asynchronously, carrying in the + /// 's AsyncState exactly as + /// SqlConnection.InternalOpenAsync does. The pool must take the transaction from there, + /// because the open runs on a thread pool thread the ambient transaction may not flow to. + /// private async Task GetConnectionAsync( SqlConnection owner, Transaction? transaction = null) @@ -101,903 +128,417 @@ private async Task GetConnectionAsync( return connection ?? await tcs.Task; } - private void ReturnConnection(DbConnectionInternal connection, SqlConnection owner) - { + private void ReturnConnection(DbConnectionInternal connection, SqlConnection owner) => _pool.ReturnInternalConnection(connection, owner); - } - private void AssertPoolMetrics() + /// + /// Asserts the pool's accounting after a step. + /// + /// Total connections owned by the pool, checked out or not. A connection + /// parked in the transacted store still holds its pool slot and so is counted here. + /// Connections sitting in the idle channel, available to any caller. + /// Connections parked in the transacted store across all + /// transactions. These hold a pool slot but are not available to other callers. + private void AssertPoolState(int count, int idleCount, int transactedCount) { - Assert.True(_pool.Count <= _pool.PoolGroupOptions.MaxPoolSize, - $"Pool count ({_pool.Count}) exceeded max pool size ({_pool.PoolGroupOptions.MaxPoolSize})"); - Assert.True(_pool.Count >= 0, - $"Pool count ({_pool.Count}) is negative"); - Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); + Assert.Equal(count, _pool.Count); + Assert.Equal(idleCount, _pool.IdleCount); + Assert.Equal(transactedCount, TotalTransactedConnections()); } - #endregion - - #region Transaction Routing Tests - - [Fact] - public void GetConnection_UnderTransaction_RoutesToTransactedPool() + private int TotalTransactedConnections() { - // Arrange & Act - using var scope = new TransactionScope(); - var transaction = Transaction.Current; - Assert.NotNull(transaction); - - var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); - - ReturnConnection(conn, owner); - - // Assert - connection should be in the transacted pool - Assert.True(_pool.TransactedConnectionPool.TransactedConnections.ContainsKey(transaction)); - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]); - - scope.Complete(); + int total = 0; + foreach (var entry in _pool.TransactedConnectionPool.TransactedConnections) + { + total += entry.Value.Count; + } + return total; } - [Fact] - public void GetConnection_WithoutTransaction_RoutesToGeneralPool() - { - // Arrange & Act (no TransactionScope) - var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); + private int TransactedConnectionsFor(Transaction transaction) => + _pool.TransactedConnectionPool.TransactedConnections.TryGetValue(transaction, out var connections) + ? connections.Count + : 0; - ReturnConnection(conn, owner); + #endregion - // Assert - transacted pool should be empty - Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); - } + #region Connection Return Routing + /// + /// A connection returned while still enlisted must be parked in the transacted store rather + /// than the idle channel, so it cannot be vended to a caller in a different transaction. It + /// keeps its pool slot while parked. + /// [Fact] - public void GetConnection_UnderTransaction_ReturnsSameConnectionFromTransactedPool() + public void ReturnConnection_WhileEnlisted_ParksInTransactedStoreNotIdleChannel() { // Arrange using var scope = new TransactionScope(); + Transaction? transaction = Transaction.Current; + Assert.NotNull(transaction); - // Act - first call creates a new connection - var owner1 = new SqlConnection(); - var conn1 = GetConnection(owner1); - Assert.NotNull(conn1); - ReturnConnection(conn1, owner1); - - // Second call should retrieve the SAME connection from the transacted pool (LIFO) - var owner2 = new SqlConnection(); - var conn2 = GetConnection(owner2); - Assert.NotNull(conn2); - Assert.Same(conn1, conn2); - - ReturnConnection(conn2, owner2); - scope.Complete(); - } - - [Fact] - public async Task GetConnectionAsync_UnderTransaction_ReturnsSameConnectionFromTransactedPool() - { - // Arrange - using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); - var transaction = Transaction.Current; + var owner = new SqlConnection(); + var connection = GetConnection(owner); + Assert.NotNull(connection); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 0); - // Act - first call creates a new connection - var owner1 = new SqlConnection(); - var conn1 = await GetConnectionAsync(owner1, transaction: transaction); - Assert.NotNull(conn1); - ReturnConnection(conn1, owner1); + // Act + ReturnConnection(connection, owner); - // Second call should retrieve the SAME connection from the transacted pool - var owner2 = new SqlConnection(); - var conn2 = await GetConnectionAsync(owner2, transaction: transaction); - Assert.NotNull(conn2); - Assert.Same(conn1, conn2); + // Assert + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); + Assert.Equal(1, TransactedConnectionsFor(transaction!)); - ReturnConnection(conn2, owner2); scope.Complete(); } + /// + /// Without an ambient transaction there is nothing to enlist in, so a returning connection + /// goes straight back into the idle channel. + /// [Fact] - public void GetConnection_WithTransactionAffinityDisabled_SkipsTransactedPool() + public void ReturnConnection_WithoutTransaction_ReturnsToIdleChannel() { // Arrange - _pool.Shutdown(); - _pool.Clear(); - _pool = CreatePool(hasTransactionAffinity: false); - - using var scope = new TransactionScope(); - - // Act var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); - ReturnConnection(conn, owner); + var connection = GetConnection(owner); + Assert.NotNull(connection); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 0); - // Assert - even though a transaction is active, transacted pool is not used - Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); + // Act + ReturnConnection(connection, owner); - scope.Complete(); + // Assert + AssertPoolState(count: 1, idleCount: 1, transactedCount: 0); } - #endregion - - #region Transaction Lifecycle Tests - + /// + /// The pool deactivates a returning connection before reading its enlistment, and deactivation + /// is what detaches a completed transaction. A connection returned after its transaction has + /// already ended must therefore land in the idle channel, not be parked under a dead + /// transaction where nothing would ever release it. + /// [Fact] - public void TransactionCommit_ClearsTransactedPool() + public void ReturnConnection_AfterTransactionCompleted_ReturnsToIdleChannel() { - // Arrange & Act + // Arrange + var owner = new SqlConnection(); + DbConnectionInternal connection; using (var scope = new TransactionScope()) { - var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); - ReturnConnection(conn, owner); - - // While transaction is active, connection should be in transacted pool - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); - + connection = GetConnection(owner); + Assert.NotNull(connection); scope.Complete(); } - // Assert - after transaction completes, transacted pool should be empty - AssertPoolMetrics(); - } - - [Fact] - public void TransactionRollback_ClearsTransactedPool() - { - // Arrange & Act - using (var scope = new TransactionScope()) - { - var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); - ReturnConnection(conn, owner); - - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); - - // Don't call scope.Complete() — triggers rollback - } + // Act - the transaction is fully disposed by this point. + ReturnConnection(connection, owner); - // Assert - transacted pool should be empty after rollback too - AssertPoolMetrics(); + // Assert + AssertPoolState(count: 1, idleCount: 1, transactedCount: 0); } + /// + /// A pool with automatic enlistment disabled must never bind a connection to the ambient + /// transaction, so the transacted store stays out of the picture entirely. + /// [Fact] - public void MultipleGetReturn_SameTransaction_ReusesConnection() + public void ReturnConnection_WithTransactionAffinityDisabled_ReturnsToIdleChannel() { // Arrange + ReplaceFixturePool(hasTransactionAffinity: false); + using var scope = new TransactionScope(); - var transaction = Transaction.Current; - Assert.NotNull(transaction); + var owner = new SqlConnection(); + var connection = GetConnection(owner); + Assert.NotNull(connection); - // Act - get and return multiple times within same transaction - for (int i = 0; i < 10; i++) - { - var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); - ReturnConnection(conn, owner); - } + // Act + ReturnConnection(connection, owner); - // Assert - only one connection should be in the transacted pool - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]); + // Assert + AssertPoolState(count: 1, idleCount: 1, transactedCount: 0); scope.Complete(); } + /// + /// Returning to a pool that has already shut down destroys the connection instead of pooling + /// it, and must not throw. + /// [Fact] - public async Task MultipleGetReturn_SameTransaction_Async_ReusesConnection() + public void ReturnConnection_ToShutDownPool_DestroysConnection() { // Arrange - using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); - var transaction = Transaction.Current; - Assert.NotNull(transaction); - - // Act - get and return multiple times within same transaction - for (int i = 0; i < 10; i++) - { - var owner = new SqlConnection(); - var conn = await GetConnectionAsync(owner, transaction: transaction); - Assert.NotNull(conn); - ReturnConnection(conn, owner); - } - - // Assert - only one connection should be in the transacted pool - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]); - - scope.Complete(); - } + using var scope = new TransactionScope(); + var owner = new SqlConnection(); + var connection = GetConnection(owner); + Assert.NotNull(connection); - [Fact] - public void AlternatingCommitAndRollback_MaintainsConsistentState() - { - // Act - alternate between commit and rollback - for (int i = 0; i < 20; i++) - { - using var scope = new TransactionScope(); - var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); - ReturnConnection(conn, owner); + _pool.Shutdown(); - if (i % 2 == 0) - { - scope.Complete(); - } - // else: rollback (no Complete) - } + // Act + ReturnConnection(connection, owner); - // Assert - AssertPoolMetrics(); + // Assert - Dispose() dooms the connection and clears its pool back-reference. + Assert.True(connection.IsConnectionDoomed, + "A connection returned to a shut-down pool should be destroyed, not pooled."); + Assert.Null(connection.Pool); + AssertPoolState(count: 0, idleCount: 0, transactedCount: 0); } #endregion - #region Nested Transaction Tests - - [Fact] - public void NestedTransaction_Required_SharesSameTransactedEntry() - { - // Arrange - using var outerScope = new TransactionScope(); - var outerTxn = Transaction.Current; - Assert.NotNull(outerTxn); - - var owner1 = new SqlConnection(); - var conn1 = GetConnection(owner1); - Assert.NotNull(conn1); - ReturnConnection(conn1, owner1); - - // Act - nested scope with Required shares the same transaction - using (var innerScope = new TransactionScope(TransactionScopeOption.Required)) - { - Assert.Same(outerTxn, Transaction.Current); - - var owner2 = new SqlConnection(); - var conn2 = GetConnection(owner2); - Assert.NotNull(conn2); - Assert.Same(conn1, conn2); // Same transaction -> same connection from transacted pool - ReturnConnection(conn2, owner2); - - // Only one transaction tracked - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); - - innerScope.Complete(); - } - - outerScope.Complete(); - } + #region Vending From The Transacted Store + /// + /// Round trip: a second request inside the same transaction must be served the connection that + /// is already enlisted in it, rather than a fresh connection. Reusing it is what keeps the + /// transaction from being promoted to a distributed one. + /// [Fact] - public void NestedTransaction_RequiresNew_CreatesSeparateTransactedEntry() + public void GetConnection_UnderSameTransaction_VendsTheAlreadyEnlistedConnection() { // Arrange - using var outerScope = new TransactionScope(); - var outerTxn = Transaction.Current; - Assert.NotNull(outerTxn); - - var owner1 = new SqlConnection(); - var conn1 = GetConnection(owner1); - Assert.NotNull(conn1); - ReturnConnection(conn1, owner1); - - // Act - nested scope with RequiresNew creates a new transaction - using (var innerScope = new TransactionScope(TransactionScopeOption.RequiresNew)) - { - var innerTxn = Transaction.Current; - Assert.NotNull(innerTxn); - Assert.NotEqual(outerTxn, innerTxn); - - var owner2 = new SqlConnection(); - var conn2 = GetConnection(owner2); - Assert.NotNull(conn2); - Assert.NotSame(conn1, conn2); // Different transaction -> different connection - ReturnConnection(conn2, owner2); - - // Two separate transactions tracked - Assert.Equal(2, _pool.TransactedConnectionPool.TransactedConnections.Count); - - innerScope.Complete(); - } - - outerScope.Complete(); - } - - [Fact] - public void NestedTransaction_RequiresNew_CompletesIndependently() - { - // Arrange & Act - using (var outerScope = new TransactionScope()) - { - var owner1 = new SqlConnection(); - var conn1 = GetConnection(owner1); - Assert.NotNull(conn1); - ReturnConnection(conn1, owner1); - - using (var innerScope = new TransactionScope(TransactionScopeOption.RequiresNew)) - { - var owner2 = new SqlConnection(); - var conn2 = GetConnection(owner2); - Assert.NotNull(conn2); - ReturnConnection(conn2, owner2); - innerScope.Complete(); - } - - // Inner transaction completed - its entry should be cleared - // Outer transaction entry should still exist - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); - - outerScope.Complete(); - } - - // Both completed - AssertPoolMetrics(); - } + using var scope = new TransactionScope(); + Transaction? transaction = Transaction.Current; + Assert.NotNull(transaction); - [Fact] - public void DeeplyNestedTransactions_RequiresNew_AllTrackedSeparately() - { - // Arrange & Act - using var scope1 = new TransactionScope(); var owner1 = new SqlConnection(); - var conn1 = GetConnection(owner1); - ReturnConnection(conn1, owner1); - - using var scope2 = new TransactionScope(TransactionScopeOption.RequiresNew); - var owner2 = new SqlConnection(); - var conn2 = GetConnection(owner2); - ReturnConnection(conn2, owner2); - - using var scope3 = new TransactionScope(TransactionScopeOption.RequiresNew); - var owner3 = new SqlConnection(); - var conn3 = GetConnection(owner3); - ReturnConnection(conn3, owner3); + var connection1 = GetConnection(owner1); + Assert.NotNull(connection1); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 0); - // Assert - three separate transactions tracked - Assert.Equal(3, _pool.TransactedConnectionPool.TransactedConnections.Count); - - scope3.Complete(); - scope2.Complete(); - scope1.Complete(); - } - - [Fact] - public void DeeplyNestedTransactions_Required_AllShareOneEntry() - { - // Arrange & Act - using var scope1 = new TransactionScope(); - var txn = Transaction.Current; - var owner1 = new SqlConnection(); - var conn1 = GetConnection(owner1); - ReturnConnection(conn1, owner1); + ReturnConnection(connection1, owner1); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); - using var scope2 = new TransactionScope(TransactionScopeOption.Required); - Assert.Same(txn, Transaction.Current); + // Act var owner2 = new SqlConnection(); - var conn2 = GetConnection(owner2); - Assert.Same(conn1, conn2); - ReturnConnection(conn2, owner2); - - using var scope3 = new TransactionScope(TransactionScopeOption.Required); - Assert.Same(txn, Transaction.Current); - var owner3 = new SqlConnection(); - var conn3 = GetConnection(owner3); - Assert.Same(conn1, conn3); - ReturnConnection(conn3, owner3); - - // Assert - single transaction entry - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); - - scope3.Complete(); - scope2.Complete(); - scope1.Complete(); - } - - #endregion - - #region Mixed Transacted and Non-Transacted Tests - - [Fact] - public void MixedWorkload_AlternatingTransactedAndNonTransacted() - { - // Act - alternate between transacted and non-transacted - for (int i = 0; i < 10; i++) - { - if (i % 2 == 0) - { - using var scope = new TransactionScope(); - var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); - ReturnConnection(conn, owner); - scope.Complete(); - } - else - { - var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); - ReturnConnection(conn, owner); - } - } + var connection2 = GetConnection(owner2); // Assert - AssertPoolMetrics(); - } - - #endregion - - #region Shared Transaction Tests - - [Fact] - public void SharedTransaction_DependentScopes_UseTransactedPool() - { - // Arrange - using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); - var transaction = Transaction.Current; - Assert.NotNull(transaction); - - // Act - first connection - var owner1 = new SqlConnection(); - var conn1 = GetConnection(owner1); - Assert.NotNull(conn1); - ReturnConnection(conn1, owner1); + Assert.Same(connection1, connection2); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 0); - // Use dependent scope on same transaction - using (var innerScope = new TransactionScope(transaction)) - { - Assert.Same(transaction, Transaction.Current); - var owner2 = new SqlConnection(); - var conn2 = GetConnection(owner2); - Assert.NotNull(conn2); - Assert.Same(conn1, conn2); // Same transaction -> same connection - ReturnConnection(conn2, owner2); - innerScope.Complete(); - } - - // Assert - still one transaction entry - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]); + ReturnConnection(connection2, owner2); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); + Assert.Equal(1, TransactedConnectionsFor(transaction!)); scope.Complete(); } - #endregion - - #region Pool Saturation with Transactions Tests - - [Fact] - public void PoolSaturation_BlocksUntilConnectionAvailable() - { - // Arrange - small pool - _pool.Shutdown(); - _pool.Clear(); - _pool = CreatePool(maxPoolSize: 1); - - using var allAcquired = new ManualResetEventSlim(false); - using var releaseFirst = new ManualResetEventSlim(false); - - var saturatingTask = Task.Run(() => - { - using var scope = new TransactionScope(); - var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); - - allAcquired.Set(); // Signal that this connection is held - - Assert.True(releaseFirst.Wait(TimeSpan.FromSeconds(15)), - "Timed out waiting for releaseFirst signal."); - - ReturnConnection(conn, owner); - scope.Complete(); - }); - - Assert.True(allAcquired.Wait(TimeSpan.FromSeconds(10)), - "Timed out waiting for connection to be acquired."); - Assert.Equal(1, _pool.Count); - - using var acquired = new ManualResetEventSlim(false); - var waitingTask = Task.Run(() => - { - using var scope = new TransactionScope(); - var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); - acquired.Set(); - ReturnConnection(conn, owner); - scope.Complete(); - }); - - // Give the waiting task time to block — it should NOT complete yet - Assert.False(acquired.Wait(TimeSpan.FromMilliseconds(500)), - "Waiting task should not have acquired a connection while pool is saturated"); - - // Release one connection to unblock the waiting task - releaseFirst.Set(); - - // Now the waiting task should complete - Assert.True(waitingTask.Wait(TimeSpan.FromSeconds(15)), - "Waiting task should have completed after a connection was released"); - Assert.True(acquired.IsSet); - - // Cleanup remaining held connections - Task.WaitAll(saturatingTask); - } - - #endregion - - #region Controlled Concurrency Tests - - // Flaky under CI load only (never reproduces locally): the two worker tasks are - // scheduled via Task.Run on the thread pool. On a loaded agent the pool can be slow to - // spin up a worker, so task1 starts late and fails to signal task1Returned within - // task2's 10s wait, producing a WaitAll timeout. That is thread-pool starvation, not a - // pool/transaction defect. - [Trait("Category", "flaky")] + /// + /// The asynchronous path must reuse the enlisted connection exactly as the synchronous path + /// does, even though it runs the open on a thread pool thread. + /// [Fact] - public void TwoThreads_SharedTransaction_AccessSameTransactedEntry() + public async Task GetConnectionAsync_UnderSameTransaction_VendsTheAlreadyEnlistedConnection() { // Arrange - // Use 3-phase synchronization so task1 gets AND returns before task2 requests. - // This ensures the connection is back in the transacted pool for task2 to reuse. - using var task1Returned = new ManualResetEventSlim(false); - using var task2Done = new ManualResetEventSlim(false); - DbConnectionInternal? connFromTask1 = null; - DbConnectionInternal? connFromTask2 = null; - using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); - var transaction = Transaction.Current; + Transaction? transaction = Transaction.Current; Assert.NotNull(transaction); - // Act - two threads sharing the same transaction, sequenced so the - // transacted pool can vend the same connection to both. - var task1 = Task.Run(() => - { - using var innerScope = new TransactionScope(transaction); - var owner = new SqlConnection(); - connFromTask1 = GetConnection(owner); - Assert.NotNull(connFromTask1); - - // Return the connection so it's available in the transacted pool - ReturnConnection(connFromTask1, owner); - innerScope.Complete(); - - task1Returned.Set(); // Signal: connection is back in the transacted pool - }); + var owner1 = new SqlConnection(); + var connection1 = await GetConnectionAsync(owner1, transaction); + Assert.NotNull(connection1); + ReturnConnection(connection1, owner1); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); - var task2 = Task.Run(() => - { - // Wait until task1 has returned the connection to the transacted pool - Assert.True(task1Returned.Wait(TimeSpan.FromSeconds(10)), - "Timed out waiting for task1 to return its connection."); + // Act + var owner2 = new SqlConnection(); + var connection2 = await GetConnectionAsync(owner2, transaction); - using var innerScope = new TransactionScope(transaction); - var owner = new SqlConnection(); - connFromTask2 = GetConnection(owner); - Assert.NotNull(connFromTask2); - ReturnConnection(connFromTask2, owner); - innerScope.Complete(); - }); + // Assert + Assert.Same(connection1, connection2); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 0); - Task.WaitAll(task1, task2); + ReturnConnection(connection2, owner2); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); - // Both tasks should have received the same connection via the transacted pool - Assert.Same(connFromTask1, connFromTask2); scope.Complete(); } + /// + /// A connection enlisted in one transaction must never be handed to a caller in a different + /// transaction; each transaction gets its own entry in the transacted store. + /// [Fact] - public async Task TwoThreads_SeparateTransactions_Async_IsolatedTransactedEntries() + public void GetConnection_UnderDifferentTransaction_DoesNotVendTheEnlistedConnection() { // Arrange - using var barrier = new SemaphoreSlim(0, 2); - - // Act - var task1 = Task.Run(async () => - { - using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); - var transaction = Transaction.Current; - var owner = new SqlConnection(); - var conn = await GetConnectionAsync(owner, transaction: transaction); - Assert.NotNull(conn); - ReturnConnection(conn, owner); - - barrier.Release(); // Signal ready - await barrier.WaitAsync(); // Wait for other task + using var outerScope = new TransactionScope(); + Transaction? outerTransaction = Transaction.Current; + Assert.NotNull(outerTransaction); - scope.Complete(); - }); + var owner1 = new SqlConnection(); + var connection1 = GetConnection(owner1); + ReturnConnection(connection1, owner1); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); - var task2 = Task.Run(async () => + // Act - RequiresNew starts an unrelated transaction. + using (var innerScope = new TransactionScope(TransactionScopeOption.RequiresNew)) { - using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); - var transaction = Transaction.Current; - var owner = new SqlConnection(); - var conn = await GetConnectionAsync(owner, transaction: transaction); - Assert.NotNull(conn); - ReturnConnection(conn, owner); - - barrier.Release(); // Signal ready - await barrier.WaitAsync(); // Wait for other task - - scope.Complete(); - }); + Transaction? innerTransaction = Transaction.Current; + Assert.NotEqual(outerTransaction, innerTransaction); - await Task.WhenAll(task1, task2); - - // Assert - AssertPoolMetrics(); - } + var owner2 = new SqlConnection(); + var connection2 = GetConnection(owner2); - #endregion + // Assert - a fresh connection, because connection1 belongs to the outer transaction. + Assert.NotSame(connection1, connection2); + AssertPoolState(count: 2, idleCount: 0, transactedCount: 1); - #region Pool Shutdown with Transactions Tests + ReturnConnection(connection2, owner2); + AssertPoolState(count: 2, idleCount: 0, transactedCount: 2); + Assert.Equal(1, TransactedConnectionsFor(outerTransaction!)); + Assert.Equal(1, TransactedConnectionsFor(innerTransaction!)); - [Fact] - public void PoolShutdown_AfterTransactionComplete_NoLeaks() - { - // Arrange - using (var scope = new TransactionScope()) - { - var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); - ReturnConnection(conn, owner); - scope.Complete(); + innerScope.Complete(); } - // Act - _pool.Shutdown(); + // Completing the inner transaction releases only its connection; the outer transaction's + // connection stays parked. + AssertPoolState(count: 2, idleCount: 1, transactedCount: 1); + Assert.Equal(1, TransactedConnectionsFor(outerTransaction!)); - // Assert - AssertPoolMetrics(); - } - - [Fact] - public void PoolShutdown_WhileConnectionHeld_NoException() - { - // Arrange - using var scope = new TransactionScope(); - var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); - - // Act - shutdown while connection is held (not yet returned) - _pool.Shutdown(); - - // Return after shutdown — the pool deactivates and disposes the connection - // rather than returning it to the pool. Verify this doesn't throw. - ReturnConnection(conn, owner); - - // Assert - // The connection should have been deactivated and disposed (not returned to the pool). - // After Dispose(), IsConnectionDoomed is set to true and Pool is set to null. - Assert.True(conn.IsConnectionDoomed, - "Connection should be doomed after returning to a shut-down pool."); - Assert.Null(conn.Pool); + outerScope.Complete(); } #endregion - #region Transaction Complete Before Return Tests + #region Transaction Completion + /// + /// Committing releases the parked connection back into the idle channel, where it becomes + /// available to any caller. + /// [Fact] - public void TransactionComplete_ThenReturn_ConnectionStillReturned() + public void TransactionCommit_ReturnsParkedConnectionToIdleChannel() { // Arrange - var owner = new SqlConnection(); - DbConnectionInternal conn; - + DbConnectionInternal connection; using (var scope = new TransactionScope()) { - conn = GetConnection(owner); - Assert.NotNull(conn); - scope.Complete(); - } - // Transaction is fully disposed here - - // Act - return connection after transaction ended - ReturnConnection(conn, owner); - - // Assert - no leak, pool metrics consistent - AssertPoolMetrics(); - Assert.True(_pool.Count > 0, "Pool should still have the connection"); - } - - #endregion - - #region Sequential Transaction Isolation Tests - - [Fact] - public void SequentialTransactions_EachGetsOwnTransactedEntry() - { - // Act - create multiple sequential transactions - for (int i = 0; i < 5; i++) - { - using var scope = new TransactionScope(); - var transaction = Transaction.Current; - Assert.NotNull(transaction); - var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); - ReturnConnection(conn, owner); - - // Only the current transaction should be tracked - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); - Assert.True(_pool.TransactedConnectionPool.TransactedConnections.ContainsKey(transaction)); - - scope.Complete(); - } - - // Assert - after all are done, pool should be clean - AssertPoolMetrics(); - } - - [Fact] - public async Task SequentialTransactions_Async_EachGetsOwnTransactedEntry() - { - // Act - create multiple sequential transactions - for (int i = 0; i < 5; i++) - { - using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); - var transaction = Transaction.Current; - Assert.NotNull(transaction); - - var owner = new SqlConnection(); - var conn = await GetConnectionAsync(owner, transaction: transaction); - Assert.NotNull(conn); - ReturnConnection(conn, owner); - - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); - Assert.True(_pool.TransactedConnectionPool.TransactedConnections.ContainsKey(transaction)); + connection = GetConnection(owner); + ReturnConnection(connection, owner); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); + // Act scope.Complete(); } // Assert - AssertPoolMetrics(); - } + AssertPoolState(count: 1, idleCount: 1, transactedCount: 0); - [Fact] - public void SequentialTransactions_CanReuseConnections() - { - // Act - DbConnectionInternal conn1; - DbConnectionInternal conn2; - Transaction? txn1; - Transaction? txn2; - - using (var scope1 = new TransactionScope()) - { - txn1 = Transaction.Current; - var owner1 = new SqlConnection(); - conn1 = GetConnection(owner1); - Assert.NotNull(conn1); - ReturnConnection(conn1, owner1); - scope1.Complete(); - } - - using (var scope2 = new TransactionScope()) - { - txn2 = Transaction.Current; - var owner2 = new SqlConnection(); - conn2 = GetConnection(owner2); - Assert.NotNull(conn2); - ReturnConnection(conn2, owner2); - scope2.Complete(); - } - - // Assert - // The connection was returned to the general pool and picked up by the second transaction - Assert.NotSame(txn1, txn2); - Assert.Same(conn1, conn2); - AssertPoolMetrics(); + // The released connection is reusable by a caller with no ambient transaction. + var owner2 = new SqlConnection(); + var connection2 = GetConnection(owner2); + Assert.Same(connection, connection2); + ReturnConnection(connection2, owner2); } - #endregion - - #region Transacted Pool Plumbing Tests - + /// + /// Rolling back must release the parked connection just as committing does; otherwise an + /// aborted transaction would strand its connection in the transacted store forever. + /// [Fact] - public void TransactionCompletion_ReturnsConnectionToIdleChannel() + public void TransactionRollback_ReturnsParkedConnectionToIdleChannel() { - // Arrange - park a connection in the transacted pool. - DbConnectionInternal conn; - using (var scope = new TransactionScope()) + // Arrange + using (new TransactionScope()) { var owner = new SqlConnection(); - conn = GetConnection(owner); - Assert.NotNull(conn); - ReturnConnection(conn, owner); - - // While the transaction is live the connection is held by the transacted pool and is - // deliberately absent from the idle channel. - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); - Assert.Equal(0, _pool.IdleCount); + var connection = GetConnection(owner); + ReturnConnection(connection, owner); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); - scope.Complete(); + // Act - leaving the scope without calling Complete rolls the transaction back. } - // Assert - completion drives TransactionEnded -> PutObjectFromTransactedPool, which puts - // the connection back into general circulation. - Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); - Assert.Equal(1, _pool.IdleCount); - Assert.Equal(1, _pool.Count); - - // The connection is now reusable by a caller with no ambient transaction. - var owner2 = new SqlConnection(); - var conn2 = GetConnection(owner2); - Assert.Same(conn, conn2); - ReturnConnection(conn2, owner2); + // Assert + AssertPoolState(count: 1, idleCount: 1, transactedCount: 0); } + /// + /// A connection parked in the transacted store survives pool shutdown, because closing it + /// would abort a possibly distributed transaction. Once that transaction ends, the shut-down + /// pool must destroy the connection rather than return it to circulation. + /// [Fact] public void TransactionCompletion_AfterShutdown_DestroysConnection() { - // Arrange - park a connection in the transacted pool, then shut the pool down. The - // transacted connection survives the shutdown drain because closing it would abort the - // (possibly distributed) transaction. - DbConnectionInternal conn; + // Arrange + DbConnectionInternal connection; using (var scope = new TransactionScope()) { var owner = new SqlConnection(); - conn = GetConnection(owner); - Assert.NotNull(conn); - ReturnConnection(conn, owner); + connection = GetConnection(owner); + ReturnConnection(connection, owner); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); - // Act + // Act - the shutdown drain must leave the transacted connection alone. _pool.Shutdown(); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); + scope.Complete(); } - // Assert - a shut-down pool must not re-pool the connection when the transaction ends. - Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); - Assert.Equal(0, _pool.IdleCount); - Assert.Equal(0, _pool.Count); - Assert.True(conn.IsConnectionDoomed); + // Assert + AssertPoolState(count: 0, idleCount: 0, transactedCount: 0); + Assert.True(connection.IsConnectionDoomed, + "A shut-down pool must destroy a connection released by its transaction."); } + /// + /// TransactionEnded for a connection that was never parked must be a no-op. It must not push a + /// connection that is still checked out into the idle channel, where a second caller could + /// pick it up while the first is still using it. + /// [Fact] - public void TransactionEnded_UnknownConnection_DoesNotPoolConnection() + public void TransactionEnded_ForConnectionThatWasNeverParked_LeavesItCheckedOut() { - // Arrange - a connection that was never parked in the transacted pool. + // Arrange var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); + var connection = GetConnection(owner); + Assert.NotNull(connection); using var scope = new TransactionScope(); - var transaction = Transaction.Current; + Transaction? transaction = Transaction.Current; Assert.NotNull(transaction); // Act - _pool.TransactionEnded(transaction!, conn); + _pool.TransactionEnded(transaction!, connection); - // Assert - nothing to remove, so the connection stays checked out. - Assert.Equal(0, _pool.IdleCount); - Assert.Equal(1, _pool.Count); + // Assert + AssertPoolState(count: 1, idleCount: 0, transactedCount: 0); - ReturnConnection(conn, owner); + ReturnConnection(connection, owner); scope.Complete(); } + #endregion + + #region Connection Replacement + + /// + /// Replacing a connection mid-transaction must carry the enlistment across, so the replacement + /// is the one that parks in the transacted store when it is returned. + /// [Fact] public void ReplaceConnection_CarriesEnlistedTransactionToNewConnection() { // Arrange using var scope = new TransactionScope(); - var transaction = Transaction.Current; + Transaction? transaction = Transaction.Current; Assert.NotNull(transaction); var owner = new SqlConnection(); var oldConnection = GetConnection(owner); Assert.NotNull(oldConnection); - Assert.Equal(1, _pool.Count); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 0); // Act var newConnection = _pool.ReplaceConnection( @@ -1005,36 +546,36 @@ public void ReplaceConnection_CarriesEnlistedTransactionToNewConnection() oldConnection, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); - // Assert - a distinct connection took over the old connection's slot and enlistment. + // Assert - a distinct connection took over the old connection's slot. Assert.NotNull(newConnection); Assert.NotSame(oldConnection, newConnection); - Assert.Equal(1, _pool.Count); Assert.True(oldConnection.IsConnectionDoomed); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 0); ReturnConnection(newConnection, owner); - // The replacement inherited the transaction, so it parks in the transacted pool. - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction!]); + // The replacement inherited the transaction, so it parks in the transacted store. + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); + Assert.Equal(1, TransactedConnectionsFor(transaction!)); scope.Complete(); } #endregion - #region Async Ambient Transaction Flow Tests + #region Async Ambient Transaction Flow /// /// A created without - /// keeps the ambient transaction in thread-static storage, so it is not observable from the thread - /// pool thread the pool opens on. The pool must therefore take the transaction from the - /// 's AsyncState, which is where SqlConnection.OpenAsync - /// captures it. + /// keeps the ambient transaction in thread-static storage, so it is not observable from the + /// thread pool thread the pool opens on. The pool must therefore take the transaction from the + /// 's AsyncState. This test uses a transaction that + /// is never ambient anywhere, so AsyncState is the only way the pool can learn about it. /// [Fact] public async Task GetConnectionAsync_AmbientTransactionNotFlowed_StillEnlistsFromAsyncState() { - // Arrange - a transaction that is never ambient on any thread, so AsyncState is the only - // way the pool can learn about it. + // Arrange using var transaction = new CommittableTransaction(); Assert.Null(Transaction.Current); Assert.Null(await Task.Run(() => Transaction.Current)); @@ -1048,10 +589,8 @@ public async Task GetConnectionAsync_AmbientTransactionNotFlowed_StillEnlistsFro Assert.Equal(transaction, connection.EnlistedTransaction); ReturnConnection(connection, owner); - - // Being enlisted, the connection parks in the transacted pool rather than the idle channel. - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]); - Assert.Equal(0, _pool.IdleCount); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); + Assert.Equal(1, TransactedConnectionsFor(transaction)); transaction.Rollback(); } @@ -1059,8 +598,8 @@ public async Task GetConnectionAsync_AmbientTransactionNotFlowed_StillEnlistsFro /// /// Assigning writes to thread-static storage that the /// ExecutionContext does not unwind, so doing it on a thread pool thread would leave a stale - /// transaction behind for unrelated work later scheduled onto that same thread -- including the - /// login-time auto-enlistment that non-pooled connections perform against the ambient + /// transaction behind for unrelated work later scheduled onto that same thread -- including + /// the login-time auto-enlistment that non-pooled connections perform against the ambient /// transaction. The pool must pass the transaction explicitly instead of assigning it. /// [Fact] @@ -1085,17 +624,18 @@ public async Task GetConnectionAsync_DoesNotLeakAmbientTransactionOntoThreadPool /// /// The synchronous path runs on the caller's thread, where the ambient transaction set by a - /// TransactionScope is directly observable and must still be honored. + /// TransactionScope is directly observable and must still be honored even though no + /// transaction is handed to the pool explicitly. /// [Fact] public void GetConnection_Sync_UsesAmbientTransactionFromCallersThread() { // Arrange using var scope = new TransactionScope(); - var transaction = Transaction.Current; + Transaction? transaction = Transaction.Current; Assert.NotNull(transaction); - // Act - no transaction is handed to the pool explicitly; it must read Transaction.Current. + // Act var owner = new SqlConnection(); var connection = GetConnection(owner); @@ -1104,7 +644,7 @@ public void GetConnection_Sync_UsesAmbientTransactionFromCallersThread() Assert.Equal(transaction, connection.EnlistedTransaction); ReturnConnection(connection, owner); - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction!]); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); scope.Complete(); }