diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs index e74b0c0ef2..54996d6bc6 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs @@ -357,7 +357,21 @@ public bool IsRunning public TransactedConnectionPool TransactedConnectionPool => _transactedConnectionPool; - internal void CleanupCallback(object state) + /// + /// Periodic pruning callback invoked by . Destroys idle + /// connections that have aged out of the general pool (those above + /// that have been on _stackOld for a full cleanup + /// period) and promotes the remaining _stackNew entries onto _stackOld + /// so they become eligible for pruning on the next tick. Connections held in the + /// are not touched here; the transaction-end + /// event is responsible for releasing them. + /// + /// + /// Exposed as internal (rather than private) solely so unit tests can + /// invoke a deterministic prune cycle without waiting on the cleanup timer. + /// +#nullable enable annotations + internal void CleanupCallback(object? state) { // If the pool is not Running, skip work. Shutdown disposes the timer, but // a callback may already be in-flight when Shutdown runs; this guard ensures it does @@ -483,6 +497,7 @@ internal void CleanupCallback(object state) // Queue up a request to bring us up to MinPoolSize QueuePoolCreateRequest(); } +#nullable restore annotations public void Clear() { diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/TransactionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/TransactionPoolTest.cs index 538359f4d6..925c6a754d 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/TransactionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/TransactionPoolTest.cs @@ -81,54 +81,5 @@ public static async Task BasicTransactionPoolTest(string connectionString) Assert.Equal(2, connectionPool.ConnectionCount); } - - /// - /// Checks that connections in the transaction pool are not cleaned out, and the root transaction is put into "stasis" when it ages - /// Synapse: only supports local transaction request. - /// - /// - // - // Flaky under CI load only: the connection pool intermittently reports one more - // connection than expected because the process-global pool (keyed by connection string) - // is contaminated by a connection opened elsewhere while this test runs, i.e. it is a - // test-isolation / pool-count race, not a product defect. It cannot be made deterministic - // without isolating the shared pool, so it is quarantined until that isolation is added. - // - // Failed Microsoft.Data.SqlClient.ManualTesting.Tests.TransactionPoolTest.TransactionCleanupTest(connectionString: "Data Source=tcp:localhost;Initial Catalog=Northwin"...) [2 s] - // Assert.Equal() Failure: Values differ - // Expected: 2 - // Actual: 3 - // at Microsoft.Data.SqlClient.ManualTesting.Tests.TransactionPoolTest.TransactionCleanupTest(String connectionString) in TransactionPoolTest.cs:line 101 - [Trait("category", "flaky")] - [ConditionalTheory(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureSynapse))] - [ClassData(typeof(ConnectionPoolConnectionStringProvider))] - public static async Task TransactionCleanupTest(string connectionString) - { - SqlConnection.ClearAllPools(); - ConnectionPoolWrapper connectionPool = null; - - using (TransactionScope transScope = new(TransactionScopeAsyncFlowOption.Enabled)) - { - using SqlConnection connection1 = new(connectionString); - using SqlConnection connection2 = new(connectionString); - await OpenWithAccessTokenAsync(connection1); - await OpenWithAccessTokenAsync(connection2); - InternalConnectionWrapper internalConnection1 = new(connection1); - connectionPool = new ConnectionPoolWrapper(connection1); - - connectionPool.Cleanup(); - Assert.Equal(2, connectionPool.ConnectionCount); - - connection1.Close(); - connection2.Close(); - connectionPool.Cleanup(); - Assert.Equal(2, connectionPool.ConnectionCount); - - connectionPool.Cleanup(); - Assert.Equal(2, connectionPool.ConnectionCount); - - transScope.Complete(); - } - } } } diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolTransactionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolTransactionTest.cs index 77905b7eac..f7640ba804 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolTransactionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolTransactionTest.cs @@ -47,7 +47,8 @@ public void Dispose() private WaitHandleDbConnectionPool CreatePool( int maxPoolSize = DefaultMaxPoolSize, int minPoolSize = DefaultMinPoolSize, - bool hasTransactionAffinity = true) + bool hasTransactionAffinity = true, + int idleTimeout = 0) { var poolGroupOptions = new DbConnectionPoolGroupOptions( poolByIdentity: false, @@ -56,7 +57,7 @@ private WaitHandleDbConnectionPool CreatePool( creationTimeout: DefaultCreationTimeoutInMilliseconds, loadBalanceTimeout: 0, hasTransactionAffinity: hasTransactionAffinity, - idleTimeout: 0 + idleTimeout: idleTimeout ); var dbConnectionPoolGroup = new DbConnectionPoolGroup( @@ -915,6 +916,76 @@ public void SequentialTransactions_CanReuseConnections() #endregion + #region Pruning Tests + + /// + /// Verifies that cleanup prunes general idle connections but preserves and reuses + /// connections held by an active transaction. + /// + [Fact] + public void Pruning_IgnoresTransactedConnections() + { + _pool.Shutdown(); + _pool.Clear(); + _pool = CreatePool(idleTimeout: 60); + + // Arrange - place a connection into the transacted pool by returning it + // while a transaction is active. The connection lives in + // TransactedConnectionPool, not in the general pool's _stackOld/_stackNew. + 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); + + // Sanity: connection sits in the transacted pool, not the general pool. + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]); + Assert.Equal(0, _pool.IdleCount); + + // A general idle connection proves cleanup actually prunes rather than being disabled. + using (var suppressedScope = new TransactionScope(TransactionScopeOption.Suppress)) + { + using var idleOwner = new SqlConnection(); + var idleConnection = GetConnection(idleOwner); + Assert.NotNull(idleConnection); + ReturnConnection(idleConnection, idleOwner); + suppressedScope.Complete(); + } + Assert.Equal(1, _pool.IdleCount); + int poolCountBefore = _pool.Count; + + // Act - invoke pruning twice. The cleanup pass moves connections from + // _stackNew to _stackOld on one tick and destroys aged entries on the + // next, so running it twice mirrors a full prune cycle. + var waitHandlePool = (WaitHandleDbConnectionPool)_pool; + waitHandlePool.CleanupCallback(null); + waitHandlePool.CleanupCallback(null); + + // Assert - the transacted connection must still be tracked in the + // transacted pool and must not have been destroyed. + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + Assert.True(_pool.TransactedConnectionPool.TransactedConnections.ContainsKey(transaction)); + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]); + Assert.Equal(0, _pool.IdleCount); + Assert.Equal(poolCountBefore - 1, _pool.Count); + Assert.False(conn.IsConnectionDoomed, + "Transacted connection should not be doomed by the pruning process."); + + // The transacted connection must still be reusable for the same transaction. + var owner2 = new SqlConnection(); + var conn2 = GetConnection(owner2); + Assert.Same(conn, conn2); + ReturnConnection(conn2, owner2); + + scope.Complete(); + } + + #endregion + #region Mock Classes internal class MockSqlConnectionFactory : SqlConnectionFactory diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionFailoverTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionFailoverTests.cs index 7eeb5df3d8..1404f382a3 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionFailoverTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionFailoverTests.cs @@ -747,6 +747,12 @@ public async Task TransientFault_Async_ShouldConnectToPrimary_NotFailover(uint e [InlineData(40613)] [InlineData(42108)] [InlineData(42109)] + // Quarantined due to intermittent failure: + // Assert.Equal() Failure: Strings differ + // v (pos 14) + // Expected: "localhost,56862" + // Actual: "localhost,56861" + [Trait("Category", "flaky")] public async Task TransientFault_WithUserProvidedPartner_Async_ShouldConnectToPrimary_NotFailover(uint errorCode) { // Async parity for TransientFault_WithUserProvidedPartner_ShouldConnectToPrimary. diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionTests.cs index a8216d787f..cc3cd5bb78 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionTests.cs @@ -98,6 +98,8 @@ public async Task RequestEncryption_ServerDoesNotSupportEncryption_ShouldFail() [InlineData(40613)] [InlineData(42108)] [InlineData(42109)] + // Quarantine tracked in https://github.com/dotnet/SqlClient/pull/4344. + // The original intermittent async retry failure did not retain assertion/exception details. public async Task TransientFault_RetryEnabled_ShouldSucceed_Async(uint errorCode) { using TransientTdsErrorTdsServer server = new( @@ -183,6 +185,11 @@ public async Task TransientFault_RetryDisabled_ShouldFail_Async(uint errorCode) [InlineData(40613)] [InlineData(42108)] [InlineData(42109)] + // Quarantined due to intermittent failure: + // Assert.Equal() Failure: Values differ + // Expected: 40613 + // Actual: 42108 + [Trait("Category", "flaky")] public void TransientFault_RetryDisabled_ShouldFail(uint errorCode) { using TransientTdsErrorTdsServer server = new(