From 39f4d39d7f0991d3e168d92b94b4cdb31f8df285 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 21 Sep 2026 15:19:07 -0700 Subject: [PATCH 01/13] Fix OpenAsync retry after pool clear Route pending async opens on a cleared wait-handle pool back through the connection factory so they can complete from the replacement pool instead of surfacing a misleading pool timeout. Add a regression test that parks an async pending open, clears the pool group, and verifies completion from a replacement pool. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../PoolShutdownOpenRetryException.cs | 23 ++++++ .../WaitHandleDbConnectionPool.cs | 14 +++- .../Microsoft/Data/SqlClient/SqlConnection.cs | 60 ++++++++++++++- .../WaitHandleDbConnectionPoolShutdownTest.cs | 77 +++++++++++++++++++ 4 files changed, 168 insertions(+), 6 deletions(-) create mode 100644 src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolShutdownOpenRetryException.cs diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolShutdownOpenRetryException.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolShutdownOpenRetryException.cs new file mode 100644 index 0000000000..35bc29a104 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolShutdownOpenRetryException.cs @@ -0,0 +1,23 @@ +// 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; + +namespace Microsoft.Data.SqlClient.ConnectionPool +{ + /// + /// Internal signal used when an asynchronous open is parked on a pool that is shut down before + /// the request is satisfied. The connection layer consumes this and retries against the current + /// pool group. + /// + internal sealed class PoolShutdownOpenRetryException : Exception + { + internal static PoolShutdownOpenRetryException Create() => new(); + + private PoolShutdownOpenRetryException() + : base("The connection pool shut down while an asynchronous open was pending.") + { + } + } +} 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 afaa827fce..77dfe71e03 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 @@ -827,11 +827,19 @@ private void WaitForPendingOpen() } else { - Debug.Assert(connection != null, "connection should never be null in success case"); + if (connection is null) + { + next.Completion.TrySetException(PoolShutdownOpenRetryException.Create()); + continue; + } + if (!next.Completion.TrySetResult(connection)) { // if the completion was cancelled, lets try and get this connection back for the next try - ReturnInternalConnection(connection, next.Owner); + if (connection is not null) + { + ReturnInternalConnection(connection, next.Owner); + } } } } @@ -1005,7 +1013,7 @@ private bool TryGetConnection(DbConnection owningObject, uint waitForMultipleObj } Interlocked.Decrement(ref _waitCount); connection = null; - return false; + return true; } // From the WaitAny docs: "If more than one object became signaled during diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs index 66b8df4564..25010b07df 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs @@ -2077,7 +2077,7 @@ private Task InternalOpenAsync(SqlConnectionOverrides overrides, bool forceNewCo { registration = cancellationToken.Register(s_openAsyncCancel, completion); } - OpenAsyncRetry retry = new OpenAsyncRetry(this, completion, result, overrides, registration, forceNewConnection); + OpenAsyncRetry retry = new OpenAsyncRetry(this, completion, result, overrides, registration, forceNewConnection, cancellationToken); _currentCompletion = new Tuple, Task>(completion, result.Task); completion.Task.ContinueWith(retry.Retry, TaskScheduler.Default); return result.Task; @@ -2191,8 +2191,9 @@ private class OpenAsyncRetry private SqlConnectionOverrides _overrides; private CancellationTokenRegistration _registration; private bool _forceNewConnection; + private CancellationToken _cancellationToken; - public OpenAsyncRetry(SqlConnection parent, TaskCompletionSource retry, TaskCompletionSource result, SqlConnectionOverrides overrides, CancellationTokenRegistration registration, bool forceNewConnection) + public OpenAsyncRetry(SqlConnection parent, TaskCompletionSource retry, TaskCompletionSource result, SqlConnectionOverrides overrides, CancellationTokenRegistration registration, bool forceNewConnection, CancellationToken cancellationToken) { _parent = parent; _retry = retry; @@ -2200,6 +2201,7 @@ public OpenAsyncRetry(SqlConnection parent, TaskCompletionSource retryTask) if (retryTask.IsFaulted) { - Exception e = retryTask.Exception.InnerException; + if (retryTask.Exception.InnerException is PoolShutdownOpenRetryException) + { + RetryAfterPoolShutdown(retryTask.AsyncState); + return; + } + _parent.CloseInnerConnection(); _parent._currentCompletion = null; _result.SetException(retryTask.Exception.InnerException); @@ -2261,6 +2268,53 @@ internal void Retry(Task retryTask) _result.SetException(e); } } + + private void RetryAfterPoolShutdown(object asyncState) + { + _parent.CloseInnerConnection(); + + if (_cancellationToken.IsCancellationRequested) + { + _parent._currentCompletion = null; + _result.SetCanceled(); + return; + } + + var retry = new TaskCompletionSource(asyncState); + CancellationTokenRegistration registration = new CancellationTokenRegistration(); + if (_cancellationToken.CanBeCanceled) + { + registration = _cancellationToken.Register(s_openAsyncCancel, retry); + } + + try + { + bool result; + lock (_parent.InnerConnection) + { + result = _parent.TryOpen(retry, _forceNewConnection, _overrides); + } + + if (result) + { + registration.Dispose(); + _parent._currentCompletion = null; + _result.SetResult(null); + } + else + { + _retry = retry; + _registration = registration; + _parent._currentCompletion = new Tuple, Task>(retry, _result.Task); + retry.Task.ContinueWith(Retry, TaskScheduler.Default); + } + } + catch + { + registration.Dispose(); + throw; + } + } } private void PrepareStatisticsForNewConnection() diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs index 17ed3e63f8..729fdf5872 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs @@ -42,6 +42,32 @@ private static WaitHandleDbConnectionPool CreatePool(int maxPoolSize = 5) return pool; } + /// + /// Builds a running pool through so clearing the group + /// swaps the old pool out exactly like ClearPool/ClearAllPools. + /// + /// Maximum number of connections the pool can reserve. + /// The registered wait-handle pool. + private static WaitHandleDbConnectionPool CreateRegisteredPool(int maxPoolSize = 5) + { + var poolGroupOptions = new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: 0, + maxPoolSize: maxPoolSize, + creationTimeout: 15000, + loadBalanceTimeout: 0, + hasTransactionAffinity: true, + idleTimeout: 0); + + var dbConnectionPoolGroup = new DbConnectionPoolGroup( + new SqlConnectionOptions("Data Source=localhost;"), + new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), + poolGroupOptions); + + IDbConnectionPool pool = dbConnectionPoolGroup.GetConnectionPool(new WaitHandleDbConnectionPoolTransactionTest.MockSqlConnectionFactory()); + return Assert.IsType(pool); + } + // State transitions to ShuttingDown on Shutdown. [Fact] public void Shutdown_TransitionsState_ToShuttingDown() @@ -160,6 +186,57 @@ public void TryGetConnection_Async_AfterShutdown_ShortCircuits_NoPendingOpenSche Assert.Equal(0, Volatile.Read(ref pool._waitCount)); } + /// + /// Verifies that an async pending open parked on a pool cleared by ClearPool/ClearAllPools + /// completes with the internal retry signal instead of faulting with a misleading pool timeout. + /// + [Fact] + public async Task TryGetConnection_AsyncPendingOpenDuringClear_CompletesWithRetrySignal() + { + var pool = CreateRegisteredPool(maxPoolSize: 1); + + var blockingOwner = new SqlConnection(); + Assert.True(pool.TryGetConnection( + blockingOwner, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? blocking)); + Assert.NotNull(blocking); + + var pendingCompletion = new TaskCompletionSource(); + var pendingOwner = new SqlConnection + { + PoolGroup = pool.PoolGroup + }; + + bool completed = pool.TryGetConnection( + pendingOwner, + pendingCompletion, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? pending); + + Assert.False(completed); + Assert.Null(pending); + + var deadline = DateTime.UtcNow.AddSeconds(5); + while (DateTime.UtcNow < deadline && Volatile.Read(ref pool._waitCount) < 1) + { + await Task.Yield(); + } + + Assert.True(Volatile.Read(ref pool._waitCount) >= 1, "Pending open did not park within 5s."); + + pool.PoolGroup.Clear(); + + Task completedTask = await Task.WhenAny(pendingCompletion.Task, Task.Delay(TimeSpan.FromSeconds(5))); + Assert.Same(pendingCompletion.Task, completedTask); + await Assert.ThrowsAsync(() => pendingCompletion.Task); + Assert.Equal(0, Volatile.Read(ref pool._waitCount)); + + pool.ReturnInternalConnection(blocking!, blockingOwner); + Assert.Equal(0, pool.Count); + } + // Shutdown wakes up a thread parked in WaitHandle.WaitAny. [Trait("category", "flaky")] // Failed Microsoft.Data.SqlClient.UnitTests.ConnectionPool.WaitHandleDbConnectionPoolShutdownTest.Shutdown_UnblocksSyncWaiter [5 s] From 153f8793e6672816ce6a0652fdae4ea21708b5ad Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 21 Sep 2026 16:46:31 -0700 Subject: [PATCH 02/13] Preserve in-flight opens when clearing wait-handle pools Remove shutdown interruption and retry routing. Let admitted requests finish on the retired pool and dispose their connections on return. Preserve error expiry for remaining waiters. Cover sync and async clearing, cancellation, timeouts, and error expiry. Add a SQL Server regression workload for #4714. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../SqlConnection.xml | 4 + .../PoolShutdownOpenRetryException.cs | 23 -- .../WaitHandleDbConnectionPool.cs | 74 +---- .../Microsoft/Data/SqlClient/SqlConnection.cs | 60 +--- .../PoolClearDuringOpenTest.cs | 93 ++++++ ...andleDbConnectionPoolBlockingPeriodTest.cs | 20 ++ .../WaitHandleDbConnectionPoolShutdownTest.cs | 267 ++++++++++-------- .../PoolClearDuringOpenTests.cs | 129 +++++++++ 8 files changed, 402 insertions(+), 268 deletions(-) delete mode 100644 src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolShutdownOpenRetryException.cs create mode 100644 src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/PoolClearDuringOpenTest.cs create mode 100644 src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs diff --git a/doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml b/doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml index 7a9af8b6b4..8f6aadc1b8 100644 --- a/doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml +++ b/doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml @@ -847,6 +847,8 @@ The following example creates a an resets (or empties) all connection pools. If there are connections in use at the time of the call, they are marked appropriately and will be discarded (instead of being returned to a pool) when is called on them. +With the default connection pool, clearing does not cancel requests already waiting for a connection or logging in. These requests can complete on the retired pool, and their connections are discarded when closed. Normal timeouts and cancellation still apply. + > [!CAUTION] > Clearing the pool is an expensive operation and should only be used if required. This operation may negatively interfere with pool warmup and generate high connection churn as the warmup operation continually opens new connections to attempt to reach min pool size. This situation is especially likely if clear is called in a tight loop. @@ -865,6 +867,8 @@ The following example creates a an clears the connection pool that is associated with the `connection`. If additional connections associated with `connection` are in use at the time of the call, they are marked appropriately and are discarded (instead of being returned to the pool) when is called on them. +With the default connection pool, clearing does not cancel requests already waiting for a connection or logging in. These requests can complete on the retired pool, and their connections are discarded when closed. Normal timeouts and cancellation still apply. + > [!CAUTION] > Clearing the pool is an expensive operation and should only be used if required. This operation may negatively interfere with pool warmup and generate high connection churn as the warmup operation continually opens new connections to attempt to reach min pool size. This situation is especially likely if clear is called in a tight loop. diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolShutdownOpenRetryException.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolShutdownOpenRetryException.cs deleted file mode 100644 index 35bc29a104..0000000000 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/PoolShutdownOpenRetryException.cs +++ /dev/null @@ -1,23 +0,0 @@ -// 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; - -namespace Microsoft.Data.SqlClient.ConnectionPool -{ - /// - /// Internal signal used when an asynchronous open is parked on a pool that is shut down before - /// the request is satisfied. The connection layer consumes this and retries against the current - /// pool group. - /// - internal sealed class PoolShutdownOpenRetryException : Exception - { - internal static PoolShutdownOpenRetryException Create() => new(); - - private PoolShutdownOpenRetryException() - : base("The connection pool shut down while an asynchronous open was pending.") - { - } - } -} 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 77dfe71e03..55097bf979 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 @@ -827,19 +827,11 @@ private void WaitForPendingOpen() } else { - if (connection is null) - { - next.Completion.TrySetException(PoolShutdownOpenRetryException.Create()); - continue; - } - + Debug.Assert(connection != null, "connection should never be null in success case"); if (!next.Completion.TrySetResult(connection)) { // if the completion was cancelled, lets try and get this connection back for the next try - if (connection is not null) - { - ReturnInternalConnection(connection, next.Owner); - } + ReturnInternalConnection(connection, next.Owner); } } } @@ -908,12 +900,8 @@ public bool TryGetConnection(DbConnection owningObject, TaskCompletionSource {0}, Pool is shutting down; abandoning wait.", Id); - if (waitResult == SEMAPHORE_HANDLE || waitResult == WAIT_ABANDONED + SEMAPHORE_HANDLE) - { - try - { - _waitHandles.PoolSemaphore.Release(1); - } - catch (SemaphoreFullException) - { - // Pool semaphore was already saturated by Shutdown's bulk release; safe to ignore. - } - } - Interlocked.Decrement(ref _waitCount); - connection = null; - return true; - } - // From the WaitAny docs: "If more than one object became signaled during // the call, this is the array index of the signaled object with the // smallest index value of all the signaled objects." This is important @@ -1605,31 +1565,13 @@ public void Shutdown() } State = ShuttingDown; - // Dispose all background timers so they no longer schedule new work. - // Note that any timer callback already in flight may still observe State == ShuttingDown - // and short-circuit (see CleanupCallback / ErrorCallback). + // Stop maintenance, but let admitted requests finish. Their connections are + // destroyed by DeactivateObject when returned to this retired pool. Timer cleanup = Interlocked.Exchange(ref _cleanupTimer, null); cleanup?.Dispose(); - _errorState.Dispose(); - - // Wake any threads parked in WaitHandle.WaitAny by releasing as many semaphore - // slots as there are recorded waiters. Using _waitCount (rather than MaxPoolSize) - // avoids ArgumentOutOfRangeException when MaxPoolSize == 0 (unlimited) and ensures - // we wake every parked waiter even when _waitCount exceeds MaxPoolSize. Waiters - // observe State is not Running after wake-up and bail. - int waitersToWake = Volatile.Read(ref _waitCount); - if (waitersToWake > 0) - { - try - { - _waitHandles.PoolSemaphore.Release(waitersToWake); - } - catch (SemaphoreFullException) - { - // Semaphore already saturated; nothing to do. - } - } + // Keep the cached error and its expiry timer available to admitted waiters. + // Disposing the error state here leaves ErrorEvent signaled without an error. // Reuse Clear() to doom every connection (including active checked-out ones), drain // both idle stacks, and reclaim emancipated objects. Active connections destroy diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs index 25010b07df..66b8df4564 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs @@ -2077,7 +2077,7 @@ private Task InternalOpenAsync(SqlConnectionOverrides overrides, bool forceNewCo { registration = cancellationToken.Register(s_openAsyncCancel, completion); } - OpenAsyncRetry retry = new OpenAsyncRetry(this, completion, result, overrides, registration, forceNewConnection, cancellationToken); + OpenAsyncRetry retry = new OpenAsyncRetry(this, completion, result, overrides, registration, forceNewConnection); _currentCompletion = new Tuple, Task>(completion, result.Task); completion.Task.ContinueWith(retry.Retry, TaskScheduler.Default); return result.Task; @@ -2191,9 +2191,8 @@ private class OpenAsyncRetry private SqlConnectionOverrides _overrides; private CancellationTokenRegistration _registration; private bool _forceNewConnection; - private CancellationToken _cancellationToken; - public OpenAsyncRetry(SqlConnection parent, TaskCompletionSource retry, TaskCompletionSource result, SqlConnectionOverrides overrides, CancellationTokenRegistration registration, bool forceNewConnection, CancellationToken cancellationToken) + public OpenAsyncRetry(SqlConnection parent, TaskCompletionSource retry, TaskCompletionSource result, SqlConnectionOverrides overrides, CancellationTokenRegistration registration, bool forceNewConnection) { _parent = parent; _retry = retry; @@ -2201,7 +2200,6 @@ public OpenAsyncRetry(SqlConnection parent, TaskCompletionSource retryTask) if (retryTask.IsFaulted) { - if (retryTask.Exception.InnerException is PoolShutdownOpenRetryException) - { - RetryAfterPoolShutdown(retryTask.AsyncState); - return; - } - + Exception e = retryTask.Exception.InnerException; _parent.CloseInnerConnection(); _parent._currentCompletion = null; _result.SetException(retryTask.Exception.InnerException); @@ -2268,53 +2261,6 @@ internal void Retry(Task retryTask) _result.SetException(e); } } - - private void RetryAfterPoolShutdown(object asyncState) - { - _parent.CloseInnerConnection(); - - if (_cancellationToken.IsCancellationRequested) - { - _parent._currentCompletion = null; - _result.SetCanceled(); - return; - } - - var retry = new TaskCompletionSource(asyncState); - CancellationTokenRegistration registration = new CancellationTokenRegistration(); - if (_cancellationToken.CanBeCanceled) - { - registration = _cancellationToken.Register(s_openAsyncCancel, retry); - } - - try - { - bool result; - lock (_parent.InnerConnection) - { - result = _parent.TryOpen(retry, _forceNewConnection, _overrides); - } - - if (result) - { - registration.Dispose(); - _parent._currentCompletion = null; - _result.SetResult(null); - } - else - { - _retry = retry; - _registration = registration; - _parent._currentCompletion = new Tuple, Task>(retry, _result.Task); - retry.Task.ContinueWith(Retry, TaskScheduler.Default); - } - } - catch - { - registration.Dispose(); - throw; - } - } } private void PrepareStatisticsForNewConnection() diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/PoolClearDuringOpenTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/PoolClearDuringOpenTest.cs new file mode 100644 index 0000000000..adfc028189 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/PoolClearDuringOpenTest.cs @@ -0,0 +1,93 @@ +// 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.Linq; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Data.SqlClient.Tests.Common; +using Xunit; + +namespace Microsoft.Data.SqlClient.ManualTesting.Tests +{ + /// Isolates process-wide pool clearing and the pool-selection switch. + [CollectionDefinition(Name, DisableParallelization = true)] + public sealed class PoolClearDuringOpenCollection + { + public const string Name = "PoolClearDuringOpen"; + } + + /// Exercises the independent-pool workload from issue #4714 against SQL Server. + [Collection(PoolClearDuringOpenCollection.Name)] + [Trait("Set", "3")] + public class PoolClearDuringOpenTest + { + /// + /// Concurrent pool clearing must not turn healthy opens into premature pool timeouts. + /// + [ConditionalTheory(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] + [InlineData(false)] + [InlineData(true)] + public async Task ClearAllPools_ConcurrentOpens_Complete(bool async) + { + using var switches = new LocalAppContextSwitchesHelper { UseConnectionPoolV2 = false }; + using var stopClearing = new CancellationTokenSource(); + var start = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + string applicationName = $"PoolClearDuringOpen-{Guid.NewGuid():N}"; + int opens = 0; + int clears = 0; + + Task[] workers = Enumerable.Range(0, 16).Select(worker => Task.Run(async () => + { + string connectionString = new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString) + { + ApplicationName = $"{applicationName}-{worker}", + Pooling = true, + MinPoolSize = 0, + MaxPoolSize = 100, + }.ConnectionString; + await start.Task; + for (int i = 0; i < 32; i++) + { + using var connection = new SqlConnection(connectionString); + if (async) + { + await connection.OpenAsync(); + } + else + { + connection.Open(); + } + Interlocked.Increment(ref opens); + } + })).ToArray(); + + Task clearer = Task.Run(async () => + { + start.SetResult(true); + SqlConnection.ClearAllPools(); + Interlocked.Increment(ref clears); + while (!stopClearing.IsCancellationRequested) + { + await Task.Delay(5); + SqlConnection.ClearAllPools(); + Interlocked.Increment(ref clears); + } + }); + + try + { + await Task.WhenAll(workers); + } + finally + { + stopClearing.Cancel(); + await clearer; + SqlConnection.ClearAllPools(); + } + Assert.Equal(16 * 32, opens); + Assert.True(clears > 0); + } + } +} diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolBlockingPeriodTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolBlockingPeriodTest.cs index c5d628c618..80027ac319 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolBlockingPeriodTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolBlockingPeriodTest.cs @@ -130,6 +130,26 @@ public void TryGetConnection_WhenFactoryThrows_EntersBlockingPeriod() Assert.Equal(1, factory.CreateConnectionCallCount); } + /// + /// Shutdown preserves the cached error for admitted waiters and lets its timer expire. + /// + [Fact] + public void Shutdown_WhileBlocked_PreservesErrorUntilExpiry() + { + var clock = new FakeTimeProvider(); + SqlException failure = SqlExceptionHelper.CreateSqlException("server unreachable"); + var factory = new ConfigurableSqlConnectionFactory(_ => throw failure); + var pool = CreatePool(factory, timeProvider: clock); + using var owner = new SqlConnection(); + + Assert.Throws(() => TryGetConnectionSync(pool, owner, out _)); + pool.Shutdown(); + Assert.True(pool.ErrorOccurred); + + clock.Advance(TimeSpan.FromSeconds(5)); + Assert.False(pool.ErrorOccurred); + } + /// /// Verifies that once the pool is in the blocking period, a subsequent request fast-fails /// with the cached exception without invoking the connection factory again. The first throw diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs index 729fdf5872..f7334328fc 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs @@ -3,8 +3,11 @@ // See the LICENSE file in the project root for more information. using System; +using System.Data.Common; +using System.Diagnostics; using System.Threading; using System.Threading.Tasks; +using Microsoft.Data.Common; using Microsoft.Data.Common.ConnectionString; using Microsoft.Data.ProviderBase; using Microsoft.Data.SqlClient.ConnectionPool; @@ -17,13 +20,13 @@ namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool /// public class WaitHandleDbConnectionPoolShutdownTest { - private static WaitHandleDbConnectionPool CreatePool(int maxPoolSize = 5) + private static WaitHandleDbConnectionPool CreatePool(int maxPoolSize = 5, int creationTimeout = 15000, SqlConnectionFactory? factory = null) { var poolGroupOptions = new DbConnectionPoolGroupOptions( poolByIdentity: false, minPoolSize: 0, maxPoolSize: maxPoolSize, - creationTimeout: 15000, + creationTimeout: creationTimeout, loadBalanceTimeout: 0, hasTransactionAffinity: true, idleTimeout: 0); @@ -34,7 +37,7 @@ private static WaitHandleDbConnectionPool CreatePool(int maxPoolSize = 5) poolGroupOptions); var pool = new WaitHandleDbConnectionPool( - new WaitHandleDbConnectionPoolTransactionTest.MockSqlConnectionFactory(), + factory ?? new WaitHandleDbConnectionPoolTransactionTest.MockSqlConnectionFactory(), dbConnectionPoolGroup, DbConnectionPoolIdentity.NoIdentity, new DbConnectionPoolProviderInfo()); @@ -42,32 +45,6 @@ private static WaitHandleDbConnectionPool CreatePool(int maxPoolSize = 5) return pool; } - /// - /// Builds a running pool through so clearing the group - /// swaps the old pool out exactly like ClearPool/ClearAllPools. - /// - /// Maximum number of connections the pool can reserve. - /// The registered wait-handle pool. - private static WaitHandleDbConnectionPool CreateRegisteredPool(int maxPoolSize = 5) - { - var poolGroupOptions = new DbConnectionPoolGroupOptions( - poolByIdentity: false, - minPoolSize: 0, - maxPoolSize: maxPoolSize, - creationTimeout: 15000, - loadBalanceTimeout: 0, - hasTransactionAffinity: true, - idleTimeout: 0); - - var dbConnectionPoolGroup = new DbConnectionPoolGroup( - new SqlConnectionOptions("Data Source=localhost;"), - new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), - poolGroupOptions); - - IDbConnectionPool pool = dbConnectionPoolGroup.GetConnectionPool(new WaitHandleDbConnectionPoolTransactionTest.MockSqlConnectionFactory()); - return Assert.IsType(pool); - } - // State transitions to ShuttingDown on Shutdown. [Fact] public void Shutdown_TransitionsState_ToShuttingDown() @@ -187,120 +164,166 @@ public void TryGetConnection_Async_AfterShutdown_ShortCircuits_NoPendingOpenSche } /// - /// Verifies that an async pending open parked on a pool cleared by ClearPool/ClearAllPools - /// completes with the internal retry signal instead of faulting with a misleading pool timeout. + /// Keeps a pending request on its original pool across shutdown and disposes its + /// connection on return, including when cancellation wins the completion race. /// - [Fact] - public async Task TryGetConnection_AsyncPendingOpenDuringClear_CompletesWithRetrySignal() + [Theory] + [InlineData(false, false)] + [InlineData(true, false)] + [InlineData(true, true)] + public async Task Shutdown_InFlightRequest_CompletesOnRetiredPool(bool async, bool cancel) { - var pool = CreateRegisteredPool(maxPoolSize: 1); - - var blockingOwner = new SqlConnection(); - Assert.True(pool.TryGetConnection( - blockingOwner, - taskCompletionSource: null, - TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), - out DbConnectionInternal? blocking)); - Assert.NotNull(blocking); - - var pendingCompletion = new TaskCompletionSource(); - var pendingOwner = new SqlConnection + using var factory = new GatedConnectionFactory(); + var pool = CreatePool(maxPoolSize: 2, factory: factory); + using var firstOwner = new SqlConnection(); + using var pendingOwner = new SqlConnection(); + var completion = new TaskCompletionSource(); + Task first = Acquire(pool, firstOwner, completion: null); + Task? pending = null; + try { - PoolGroup = pool.PoolGroup - }; + Assert.True(factory.Entered.Wait(TimeSpan.FromSeconds(10))); + pending = Acquire(pool, pendingOwner, async ? completion : null); + Assert.True(SpinWait.SpinUntil(() => Volatile.Read(ref pool._waitCount) == 2, TimeSpan.FromSeconds(10))); - bool completed = pool.TryGetConnection( - pendingOwner, - pendingCompletion, - TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), - out DbConnectionInternal? pending); - - Assert.False(completed); - Assert.Null(pending); + pool.Shutdown(); + Assert.False(pool.IsRunning); + if (cancel) + { + completion.SetCanceled(); + } + factory.Release.Set(); - var deadline = DateTime.UtcNow.AddSeconds(5); - while (DateTime.UtcNow < deadline && Volatile.Read(ref pool._waitCount) < 1) + Assert.Same(first, await Task.WhenAny(first, Task.Delay(TimeSpan.FromSeconds(10)))); + Assert.Same(pool, (await first)!.Pool); + Assert.Same(pending, await Task.WhenAny(pending, Task.Delay(TimeSpan.FromSeconds(10)))); + if (cancel) + { + await Assert.ThrowsAnyAsync(() => pending); + Assert.True(SpinWait.SpinUntil(() => pool.Count == 1 && Volatile.Read(ref pool._waitCount) == 0, TimeSpan.FromSeconds(10))); + } + else + { + Assert.Same(pool, (await pending)!.Pool); + Assert.Equal(0, Volatile.Read(ref pool._waitCount)); + } + } + finally { - await Task.Yield(); + factory.Release.Set(); + pool.Shutdown(); + await ReturnWhenCompleted(pool, firstOwner, first); + if (pending is not null) + { + await ReturnWhenCompleted(pool, pendingOwner, pending); + } } - - Assert.True(Volatile.Read(ref pool._waitCount) >= 1, "Pending open did not park within 5s."); - - pool.PoolGroup.Clear(); - - Task completedTask = await Task.WhenAny(pendingCompletion.Task, Task.Delay(TimeSpan.FromSeconds(5))); - Assert.Same(pendingCompletion.Task, completedTask); - await Assert.ThrowsAsync(() => pendingCompletion.Task); - Assert.Equal(0, Volatile.Read(ref pool._waitCount)); - - pool.ReturnInternalConnection(blocking!, blockingOwner); + Assert.Equal(0, pool.IdleCount); Assert.Equal(0, pool.Count); } - // Shutdown wakes up a thread parked in WaitHandle.WaitAny. - [Trait("category", "flaky")] - // Failed Microsoft.Data.SqlClient.UnitTests.ConnectionPool.WaitHandleDbConnectionPoolShutdownTest.Shutdown_UnblocksSyncWaiter [5 s] - // ##[error]EXEC(0,0): Error Message: - // EXEC : error Message: [D:\a\_work\1\s\build.proj] - // Waiter did not park within 5s. - // Stack Trace: - // at Microsoft.Data.SqlClient.UnitTests.ConnectionPool.WaitHandleDbConnectionPoolShutdownTest.Shutdown_UnblocksSyncWaiter() in D:\a\_work\1\s\src\Microsoft.Data.SqlClient\tests\UnitTests\ConnectionPool\WaitHandleDbConnectionPoolShutdownTest.cs:line 207 - [Fact] - public void Shutdown_UnblocksSyncWaiter() + /// + /// A saturated request retains its normal wait timeout instead of failing immediately + /// when its pool is retired. + /// + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task Shutdown_SaturatedRequest_RespectsTimeout(bool async) { - var pool = CreatePool(maxPoolSize: 1); - - // Saturate the pool. - var owner = new SqlConnection(); - Assert.True(pool.TryGetConnection(owner, taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? blocking)); + const int waitMilliseconds = 1000; + var pool = CreatePool(maxPoolSize: 1, creationTimeout: waitMilliseconds); + using var owner = new SqlConnection(); + using var pendingOwner = new SqlConnection(); + Assert.True(pool.TryGetConnection(owner, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? blocking)); Assert.NotNull(blocking); - - // Park a sync waiter on a worker thread with a long creation timeout. - DbConnectionInternal? waiterResult = null; - bool waiterCompleted = false; - Exception? waiterEx = null; - - var t = new Thread(() => + Task? pending = null; + var elapsed = Stopwatch.StartNew(); + try { - try + pending = Acquire(pool, pendingOwner, async ? new TaskCompletionSource() : null, + TimeSpan.FromMilliseconds(waitMilliseconds)); + Assert.True(SpinWait.SpinUntil(() => Volatile.Read(ref pool._waitCount) == 1, TimeSpan.FromSeconds(10))); + pool.Shutdown(); + if (async) { - waiterCompleted = pool.TryGetConnection( - new SqlConnection(), - taskCompletionSource: null, - TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), - out waiterResult); + InvalidOperationException error = await Assert.ThrowsAsync(() => pending); + Assert.Equal(ADP.PooledOpenTimeout().Message, error.Message); } - catch (Exception ex) + else { - waiterEx = ex; + Assert.Null(await pending); } - }) - { IsBackground = true }; - t.Start(); - - // Wait deterministically until the worker has incremented _waitCount, which - // happens immediately before it enters WaitHandle.WaitAny. Polling avoids the - // CI-flakiness of a fixed Thread.Sleep on slow agents. Volatile.Read ensures - // we see the worker's Interlocked.Increment without depending on CPU memory - // ordering of plain int reads. - var deadline = DateTime.UtcNow.AddSeconds(5); - while (DateTime.UtcNow < deadline && Volatile.Read(ref pool._waitCount) < 1) + Assert.True(elapsed.ElapsedMilliseconds >= waitMilliseconds - 50, $"Request completed after {elapsed.ElapsedMilliseconds}ms."); + Assert.Equal(0, Volatile.Read(ref pool._waitCount)); + } + finally { - Thread.Yield(); + pool.Shutdown(); + if (pending is not null) + { + await ReturnWhenCompleted(pool, pendingOwner, pending); + } + pool.ReturnInternalConnection(blocking!, owner); } - Assert.True(Volatile.Read(ref pool._waitCount) >= 1, "Waiter did not park within 5s."); - Assert.True(t.IsAlive, "Waiter should be parked, but thread already exited."); + } - pool.Shutdown(); + /// Starts a sync acquisition on a dedicated thread or queues an async acquisition. + private static Task Acquire(WaitHandleDbConnectionPool pool, SqlConnection owner, + TaskCompletionSource? completion, TimeSpan? timeout = null) + { + TimeoutTimer timer = TimeoutTimer.StartNew(timeout ?? TimeSpan.FromSeconds(15)); + if (completion is null) + { + return Task.Factory.StartNew(() => + { + Assert.True(pool.TryGetConnection(owner, null, timer, out DbConnectionInternal connection)); + return (DbConnectionInternal?)connection; + }, CancellationToken.None, TaskCreationOptions.LongRunning, TaskScheduler.Default); + } + Assert.False(pool.TryGetConnection(owner, completion, timer, out DbConnectionInternal pending)); + Assert.Null(pending); + return completion.Task!; + } + + /// Drains test work and returns successful acquisitions even when an assertion failed. + private static async Task ReturnWhenCompleted(WaitHandleDbConnectionPool pool, SqlConnection owner, Task task) + { + Assert.Same(task, await Task.WhenAny(task, Task.Delay(TimeSpan.FromSeconds(20)))); + if (task.Status == TaskStatus.RanToCompletion && task.Result is { } connection) + { + pool.ReturnInternalConnection(connection, owner); + } + else if (task.IsFaulted) + { + // Observe failures during assertion cleanup without replacing the original failure. + _ = task.Exception; + } + } - Assert.True(t.Join(TimeSpan.FromSeconds(5)), "Waiter did not unblock within 5s of Shutdown."); - // Acceptable outcomes: either returned false/null (timed out / abandoned) or - // returned true/null (state-check short-circuit). Either way, it must NOT block - // forever, and it must NOT vend a real connection from a shut-down pool. - Assert.Null(waiterResult); - Assert.Null(waiterEx); - // Suppress unused warning - presence of waiterCompleted just documents the contract. - _ = waiterCompleted; + /// Holds the first physical creation so a second acquisition waits on its semaphore. + private sealed class GatedConnectionFactory : WaitHandleDbConnectionPoolTransactionTest.MockSqlConnectionFactory, IDisposable + { + internal readonly ManualResetEventSlim Entered = new(); + internal readonly ManualResetEventSlim Release = new(); + private int _calls; + + protected override DbConnectionInternal CreateConnection(SqlConnectionOptions options, ConnectionPoolKey poolKey, + DbConnectionPoolGroupProviderInfo poolGroupProviderInfo, IDbConnectionPool pool, DbConnection owningConnection, TimeoutTimer timeout) + { + if (Interlocked.Increment(ref _calls) == 1) + { + Entered.Set(); + Assert.True(Release.Wait(TimeSpan.FromSeconds(15)), "Physical creation was not released."); + } + return base.CreateConnection(options, poolKey, poolGroupProviderInfo, pool, owningConnection, timeout); + } + + public void Dispose() + { + Entered.Dispose(); + Release.Dispose(); + } } // Startup() must be a no-op when the pool has already been shut down. Without the diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs new file mode 100644 index 0000000000..5433c2489b --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs @@ -0,0 +1,129 @@ +// 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.Threading; +using System.Threading.Tasks; +using Microsoft.Data.SqlClient.ConnectionPool; +using Microsoft.Data.SqlClient.Tests.Common; +using Microsoft.SqlServer.TDS.Servers; +using Xunit; + +namespace Microsoft.Data.SqlClient.UnitTests.SimulatedServerTests; + +/// +/// Exercises public pool clearing while one login is in flight and another open is waiting. +/// +[Collection(SimulatedServerTestCollection.Name)] +public class PoolClearDuringOpenTests +{ + /// + /// Admitted opens finish on the retired wait-handle pool and new opens use its replacement. + /// + [Theory] + [InlineData(false, false)] + [InlineData(false, true)] + [InlineData(true, false)] + [InlineData(true, true)] + public async Task ClearDuringOpen_InFlightConnectionsAreDestroyedOnClose(bool async, bool clearAll) + { + using var switches = new LocalAppContextSwitchesHelper { UseConnectionPoolV2 = false }; + using var loginEntered = new ManualResetEventSlim(); + using var releaseLogin = new ManualResetEventSlim(); + using TdsServer server = new(); + int logins = 0; + server.OnLogin7Validated = _ => + { + if (Interlocked.Increment(ref logins) == 1) + { + loginEntered.Set(); + Assert.True(releaseLogin.Wait(TimeSpan.FromSeconds(15)), "Login was not released."); + } + }; + server.Start(); + + string connectionString = new SqlConnectionStringBuilder + { + DataSource = $"localhost,{server.EndPoint.Port}", + Encrypt = SqlConnectionEncryptOption.Optional, + Pooling = true, + MaxPoolSize = 2, + ConnectTimeout = 15, + }.ConnectionString; + using var first = new SqlConnection(connectionString); + using var pending = new SqlConnection(connectionString); + using var replacement = new SqlConnection(connectionString); + Task firstOpen = Open(first, async); + Task? pendingOpen = null; + + try + { + Assert.True(loginEntered.Wait(TimeSpan.FromSeconds(10)), "First login did not start."); + var retiredPool = Assert.IsType(first.PoolGroup.GetConnectionPool(SqlConnectionFactory.Instance)); + pendingOpen = Open(pending, async); + if (async) + { + // The single pending-open worker is still processing the first login. + Assert.Equal(ConnectionState.Connecting, pending.State); + Assert.False(pendingOpen.IsCompleted); + } + else + { + Assert.True(SpinWait.SpinUntil(() => Volatile.Read(ref retiredPool._waitCount) == 2, TimeSpan.FromSeconds(10)), + "Second open did not wait for the creation semaphore."); + } + + if (clearAll) + { + SqlConnection.ClearAllPools(); + } + else + { + SqlConnection.ClearPool(first); + } + Assert.False(retiredPool.IsRunning); + releaseLogin.Set(); + Task opens = Task.WhenAll(firstOpen, pendingOpen); + Assert.Same(opens, await Task.WhenAny(opens, Task.Delay(TimeSpan.FromSeconds(20)))); + await opens; + + Assert.Equal(ConnectionState.Open, first.State); + Assert.Equal(ConnectionState.Open, pending.State); + Assert.Same(retiredPool, first.InnerConnection.Pool); + Assert.Same(retiredPool, pending.InnerConnection.Pool); + Assert.Equal(2, retiredPool.Count); + + await Open(replacement, async); + Assert.NotSame(retiredPool, replacement.InnerConnection.Pool); + Assert.True(replacement.InnerConnection.Pool.IsRunning); + first.Close(); + pending.Close(); + Assert.Equal(0, retiredPool.Count); + Assert.Equal(0, retiredPool.IdleCount); + Assert.Equal(0, Volatile.Read(ref retiredPool._waitCount)); + } + finally + { + releaseLogin.Set(); + Task opens = pendingOpen is null ? firstOpen : Task.WhenAll(firstOpen, pendingOpen); + Assert.Same(opens, await Task.WhenAny(opens, Task.Delay(TimeSpan.FromSeconds(20)))); + if (opens.IsFaulted) + { + // Observe background failures without replacing an earlier assertion failure. + _ = opens.Exception; + } + first.Close(); + pending.Close(); + replacement.Close(); + SqlConnection.ClearPool(first); + } + } + + /// Runs synchronous opens on a dedicated thread so the test can release the login. + private static Task Open(SqlConnection connection, bool async) => + async + ? connection.OpenAsync() + : Task.Factory.StartNew(connection.Open, CancellationToken.None, TaskCreationOptions.LongRunning, TaskScheduler.Default); +} From 73f03bfa4b05cfbcf21b7934f84439d4db6f2187 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 21 Sep 2026 17:10:57 -0700 Subject: [PATCH 03/13] Make pool-clear regression coverage deterministic Remove the concurrent stress workload and elapsed-time assertions. Gate physical creation and assert exact request ordering and creation counts around pool shutdown. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../PoolClearDuringOpenTest.cs | 93 ------------------- .../WaitHandleDbConnectionPoolShutdownTest.cs | 70 ++++---------- .../PoolClearDuringOpenTests.cs | 7 ++ 3 files changed, 23 insertions(+), 147 deletions(-) delete mode 100644 src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/PoolClearDuringOpenTest.cs diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/PoolClearDuringOpenTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/PoolClearDuringOpenTest.cs deleted file mode 100644 index adfc028189..0000000000 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/PoolClearDuringOpenTest.cs +++ /dev/null @@ -1,93 +0,0 @@ -// 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.Linq; -using System.Threading; -using System.Threading.Tasks; -using Microsoft.Data.SqlClient.Tests.Common; -using Xunit; - -namespace Microsoft.Data.SqlClient.ManualTesting.Tests -{ - /// Isolates process-wide pool clearing and the pool-selection switch. - [CollectionDefinition(Name, DisableParallelization = true)] - public sealed class PoolClearDuringOpenCollection - { - public const string Name = "PoolClearDuringOpen"; - } - - /// Exercises the independent-pool workload from issue #4714 against SQL Server. - [Collection(PoolClearDuringOpenCollection.Name)] - [Trait("Set", "3")] - public class PoolClearDuringOpenTest - { - /// - /// Concurrent pool clearing must not turn healthy opens into premature pool timeouts. - /// - [ConditionalTheory(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] - [InlineData(false)] - [InlineData(true)] - public async Task ClearAllPools_ConcurrentOpens_Complete(bool async) - { - using var switches = new LocalAppContextSwitchesHelper { UseConnectionPoolV2 = false }; - using var stopClearing = new CancellationTokenSource(); - var start = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); - string applicationName = $"PoolClearDuringOpen-{Guid.NewGuid():N}"; - int opens = 0; - int clears = 0; - - Task[] workers = Enumerable.Range(0, 16).Select(worker => Task.Run(async () => - { - string connectionString = new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString) - { - ApplicationName = $"{applicationName}-{worker}", - Pooling = true, - MinPoolSize = 0, - MaxPoolSize = 100, - }.ConnectionString; - await start.Task; - for (int i = 0; i < 32; i++) - { - using var connection = new SqlConnection(connectionString); - if (async) - { - await connection.OpenAsync(); - } - else - { - connection.Open(); - } - Interlocked.Increment(ref opens); - } - })).ToArray(); - - Task clearer = Task.Run(async () => - { - start.SetResult(true); - SqlConnection.ClearAllPools(); - Interlocked.Increment(ref clears); - while (!stopClearing.IsCancellationRequested) - { - await Task.Delay(5); - SqlConnection.ClearAllPools(); - Interlocked.Increment(ref clears); - } - }); - - try - { - await Task.WhenAll(workers); - } - finally - { - stopClearing.Cancel(); - await clearer; - SqlConnection.ClearAllPools(); - } - Assert.Equal(16 * 32, opens); - Assert.True(clears > 0); - } - } -} diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs index f7334328fc..e7634e5c81 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs @@ -4,10 +4,8 @@ using System; using System.Data.Common; -using System.Diagnostics; using System.Threading; using System.Threading.Tasks; -using Microsoft.Data.Common; using Microsoft.Data.Common.ConnectionString; using Microsoft.Data.ProviderBase; using Microsoft.Data.SqlClient.ConnectionPool; @@ -20,13 +18,13 @@ namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool /// public class WaitHandleDbConnectionPoolShutdownTest { - private static WaitHandleDbConnectionPool CreatePool(int maxPoolSize = 5, int creationTimeout = 15000, SqlConnectionFactory? factory = null) + private static WaitHandleDbConnectionPool CreatePool(int maxPoolSize = 5, SqlConnectionFactory? factory = null) { var poolGroupOptions = new DbConnectionPoolGroupOptions( poolByIdentity: false, minPoolSize: 0, maxPoolSize: maxPoolSize, - creationTimeout: creationTimeout, + creationTimeout: 15000, loadBalanceTimeout: 0, hasTransactionAffinity: true, idleTimeout: 0); @@ -185,6 +183,9 @@ public async Task Shutdown_InFlightRequest_CompletesOnRetiredPool(bool async, bo Assert.True(factory.Entered.Wait(TimeSpan.FromSeconds(10))); pending = Acquire(pool, pendingOwner, async ? completion : null); Assert.True(SpinWait.SpinUntil(() => Volatile.Read(ref pool._waitCount) == 2, TimeSpan.FromSeconds(10))); + Assert.Equal(1, factory.CreateCount); + Assert.False(first.IsCompleted); + Assert.False(pending.IsCompleted); pool.Shutdown(); Assert.False(pool.IsRunning); @@ -195,7 +196,9 @@ public async Task Shutdown_InFlightRequest_CompletesOnRetiredPool(bool async, bo factory.Release.Set(); Assert.Same(first, await Task.WhenAny(first, Task.Delay(TimeSpan.FromSeconds(10)))); - Assert.Same(pool, (await first)!.Pool); + DbConnectionInternal? firstConnection = await first; + Assert.NotNull(firstConnection); + Assert.Same(pool, firstConnection.Pool); Assert.Same(pending, await Task.WhenAny(pending, Task.Delay(TimeSpan.FromSeconds(10)))); if (cancel) { @@ -204,9 +207,12 @@ public async Task Shutdown_InFlightRequest_CompletesOnRetiredPool(bool async, bo } else { - Assert.Same(pool, (await pending)!.Pool); + DbConnectionInternal? pendingConnection = await pending; + Assert.NotNull(pendingConnection); + Assert.Same(pool, pendingConnection.Pool); Assert.Equal(0, Volatile.Read(ref pool._waitCount)); } + Assert.Equal(2, factory.CreateCount); } finally { @@ -222,57 +228,11 @@ public async Task Shutdown_InFlightRequest_CompletesOnRetiredPool(bool async, bo Assert.Equal(0, pool.Count); } - /// - /// A saturated request retains its normal wait timeout instead of failing immediately - /// when its pool is retired. - /// - [Theory] - [InlineData(false)] - [InlineData(true)] - public async Task Shutdown_SaturatedRequest_RespectsTimeout(bool async) - { - const int waitMilliseconds = 1000; - var pool = CreatePool(maxPoolSize: 1, creationTimeout: waitMilliseconds); - using var owner = new SqlConnection(); - using var pendingOwner = new SqlConnection(); - Assert.True(pool.TryGetConnection(owner, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? blocking)); - Assert.NotNull(blocking); - Task? pending = null; - var elapsed = Stopwatch.StartNew(); - try - { - pending = Acquire(pool, pendingOwner, async ? new TaskCompletionSource() : null, - TimeSpan.FromMilliseconds(waitMilliseconds)); - Assert.True(SpinWait.SpinUntil(() => Volatile.Read(ref pool._waitCount) == 1, TimeSpan.FromSeconds(10))); - pool.Shutdown(); - if (async) - { - InvalidOperationException error = await Assert.ThrowsAsync(() => pending); - Assert.Equal(ADP.PooledOpenTimeout().Message, error.Message); - } - else - { - Assert.Null(await pending); - } - Assert.True(elapsed.ElapsedMilliseconds >= waitMilliseconds - 50, $"Request completed after {elapsed.ElapsedMilliseconds}ms."); - Assert.Equal(0, Volatile.Read(ref pool._waitCount)); - } - finally - { - pool.Shutdown(); - if (pending is not null) - { - await ReturnWhenCompleted(pool, pendingOwner, pending); - } - pool.ReturnInternalConnection(blocking!, owner); - } - } - /// Starts a sync acquisition on a dedicated thread or queues an async acquisition. private static Task Acquire(WaitHandleDbConnectionPool pool, SqlConnection owner, - TaskCompletionSource? completion, TimeSpan? timeout = null) + TaskCompletionSource? completion) { - TimeoutTimer timer = TimeoutTimer.StartNew(timeout ?? TimeSpan.FromSeconds(15)); + TimeoutTimer timer = TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)); if (completion is null) { return Task.Factory.StartNew(() => @@ -308,6 +268,8 @@ private sealed class GatedConnectionFactory : WaitHandleDbConnectionPoolTransact internal readonly ManualResetEventSlim Release = new(); private int _calls; + internal int CreateCount => Volatile.Read(ref _calls); + protected override DbConnectionInternal CreateConnection(SqlConnectionOptions options, ConnectionPoolKey poolKey, DbConnectionPoolGroupProviderInfo poolGroupProviderInfo, IDbConnectionPool pool, DbConnection owningConnection, TimeoutTimer timeout) { diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs index 5433c2489b..7e9169f4df 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs @@ -75,6 +75,11 @@ public async Task ClearDuringOpen_InFlightConnectionsAreDestroyedOnClose(bool as "Second open did not wait for the creation semaphore."); } + Assert.Same(first.PoolGroup, pending.PoolGroup); + Assert.Equal(1, Volatile.Read(ref logins)); + Assert.False(firstOpen.IsCompleted); + Assert.False(pendingOpen.IsCompleted); + if (clearAll) { SqlConnection.ClearAllPools(); @@ -94,10 +99,12 @@ public async Task ClearDuringOpen_InFlightConnectionsAreDestroyedOnClose(bool as Assert.Same(retiredPool, first.InnerConnection.Pool); Assert.Same(retiredPool, pending.InnerConnection.Pool); Assert.Equal(2, retiredPool.Count); + Assert.Equal(2, Volatile.Read(ref logins)); await Open(replacement, async); Assert.NotSame(retiredPool, replacement.InnerConnection.Pool); Assert.True(replacement.InnerConnection.Pool.IsRunning); + Assert.Equal(3, Volatile.Read(ref logins)); first.Close(); pending.Close(); Assert.Equal(0, retiredPool.Count); From 699da9db7feefdb56b10327194555db774ed5268 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 22 Sep 2026 11:58:53 -0700 Subject: [PATCH 04/13] Remove pool-clearing documentation changes Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml b/doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml index 8f6aadc1b8..7a9af8b6b4 100644 --- a/doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml +++ b/doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml @@ -847,8 +847,6 @@ The following example creates a an resets (or empties) all connection pools. If there are connections in use at the time of the call, they are marked appropriately and will be discarded (instead of being returned to a pool) when is called on them. -With the default connection pool, clearing does not cancel requests already waiting for a connection or logging in. These requests can complete on the retired pool, and their connections are discarded when closed. Normal timeouts and cancellation still apply. - > [!CAUTION] > Clearing the pool is an expensive operation and should only be used if required. This operation may negatively interfere with pool warmup and generate high connection churn as the warmup operation continually opens new connections to attempt to reach min pool size. This situation is especially likely if clear is called in a tight loop. @@ -867,8 +865,6 @@ With the default connection pool, clearing does not cancel requests already wait clears the connection pool that is associated with the `connection`. If additional connections associated with `connection` are in use at the time of the call, they are marked appropriately and are discarded (instead of being returned to the pool) when is called on them. -With the default connection pool, clearing does not cancel requests already waiting for a connection or logging in. These requests can complete on the retired pool, and their connections are discarded when closed. Normal timeouts and cancellation still apply. - > [!CAUTION] > Clearing the pool is an expensive operation and should only be used if required. This operation may negatively interfere with pool warmup and generate high connection churn as the warmup operation continually opens new connections to attempt to reach min pool size. This situation is especially likely if clear is called in a tight loop. From 5fc86cb670761c051b15f407d39573b1304c44c8 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 22 Sep 2026 12:17:05 -0700 Subject: [PATCH 05/13] Leave retired pool clearing to the factory Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../WaitHandleDbConnectionPool.cs | 8 +++---- .../WaitHandleDbConnectionPoolShutdownTest.cs | 22 ++++++++++++++++--- 2 files changed, 22 insertions(+), 8 deletions(-) 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 55097bf979..887bcd5c35 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 @@ -1573,11 +1573,9 @@ public void Shutdown() // Keep the cached error and its expiry timer available to admitted waiters. // Disposing the error state here leaves ErrorEvent signaled without an error. - // Reuse Clear() to doom every connection (including active checked-out ones), drain - // both idle stacks, and reclaim emancipated objects. Active connections destroy - // themselves on return either via the doom flag or via DeactivateObject's - // State == ShuttingDown branch. - Clear(); + // Leave Clear() to the factory's explicit-clear or deferred-pruning path. + // Shutdown can run under the pool-group lock, where reclamation and connection + // disposal must not be added before the pool is queued for release. } // TransactionEnded merely provides the plumbing for DbConnectionInternal to access the transacted pool diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs index e7634e5c81..158c20b21d 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs @@ -68,9 +68,11 @@ public void Shutdown_DisposesCleanupTimer() Assert.Null(pool._cleanupTimer); } - // Drains idle stacks. + /// + /// Leaves idle connections for the factory's explicit or deferred Clear call. + /// [Fact] - public void Shutdown_DrainsIdleStacks() + public void Shutdown_LeavesIdleConnectionsUntilClear() { var pool = CreatePool(); @@ -87,7 +89,21 @@ public void Shutdown_DrainsIdleStacks() Assert.Equal(2, pool.IdleCount); Assert.Equal(2, pool.Count); - pool.Shutdown(); + try + { + pool.Shutdown(); + + Assert.False(pool.IsRunning); + Assert.Equal(2, pool.IdleCount); + Assert.Equal(2, pool.Count); + Assert.True(c1!.CanBePooled); + Assert.True(c2!.CanBePooled); + } + finally + { + pool.Shutdown(); + pool.Clear(); + } Assert.Equal(0, pool.IdleCount); Assert.Equal(0, pool.Count); From 44b516361c54a5cdc7e361bbfffe08638de6dd05 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 22 Sep 2026 12:24:49 -0700 Subject: [PATCH 06/13] Clarify gated pool shutdown test scenario Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../WaitHandleDbConnectionPoolShutdownTest.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs index 158c20b21d..0b6285cee9 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs @@ -178,8 +178,12 @@ public void TryGetConnection_Async_AfterShutdown_ShortCircuits_NoPendingOpenSche } /// - /// Keeps a pending request on its original pool across shutdown and disposes its - /// connection on return, including when cancellation wins the completion race. + /// Starts with no idle connections. Pauses the first physical connection creation + /// while it holds the creation semaphore, then admits a second request that waits + /// for that semaphore. Shuts down the pool before releasing the first creation. + /// Both requests create their own connections on the retired pool, and both + /// connections are destroyed when returned. In the cancellation case, the worker + /// returns and destroys the second connection instead of delivering it to the caller. /// [Theory] [InlineData(false, false)] From a3a4ff5075cb97724b3a353f7c5d20dc0680c7ba Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 22 Sep 2026 12:31:29 -0700 Subject: [PATCH 07/13] Separate pool-clear scenario from test plumbing Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../PoolClearDuringOpenTests.cs | 143 +++++++++++------- 1 file changed, 90 insertions(+), 53 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs index 7e9169f4df..c6667bce88 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs @@ -20,7 +20,9 @@ namespace Microsoft.Data.SqlClient.UnitTests.SimulatedServerTests; public class PoolClearDuringOpenTests { /// - /// Admitted opens finish on the retired wait-handle pool and new opens use its replacement. + /// Blocks the first login, admits a second open, then clears the pool before releasing + /// the login. Both opens finish on the retired pool and their connections are destroyed + /// on close. A subsequent open uses a replacement pool. /// [Theory] [InlineData(false, false)] @@ -30,55 +32,22 @@ public class PoolClearDuringOpenTests public async Task ClearDuringOpen_InFlightConnectionsAreDestroyedOnClose(bool async, bool clearAll) { using var switches = new LocalAppContextSwitchesHelper { UseConnectionPoolV2 = false }; - using var loginEntered = new ManualResetEventSlim(); - using var releaseLogin = new ManualResetEventSlim(); - using TdsServer server = new(); - int logins = 0; - server.OnLogin7Validated = _ => - { - if (Interlocked.Increment(ref logins) == 1) - { - loginEntered.Set(); - Assert.True(releaseLogin.Wait(TimeSpan.FromSeconds(15)), "Login was not released."); - } - }; - server.Start(); - - string connectionString = new SqlConnectionStringBuilder - { - DataSource = $"localhost,{server.EndPoint.Port}", - Encrypt = SqlConnectionEncryptOption.Optional, - Pooling = true, - MaxPoolSize = 2, - ConnectTimeout = 15, - }.ConnectionString; - using var first = new SqlConnection(connectionString); - using var pending = new SqlConnection(connectionString); - using var replacement = new SqlConnection(connectionString); - Task firstOpen = Open(first, async); - Task? pendingOpen = null; + using var server = new GatedLoginServer(); + using var first = new SqlConnection(server.ConnectionString); + using var pending = new SqlConnection(server.ConnectionString); + using var replacement = new SqlConnection(server.ConnectionString); + Task opens = Open(first, async); try { - Assert.True(loginEntered.Wait(TimeSpan.FromSeconds(10)), "First login did not start."); + server.WaitForFirstLogin(); var retiredPool = Assert.IsType(first.PoolGroup.GetConnectionPool(SqlConnectionFactory.Instance)); - pendingOpen = Open(pending, async); - if (async) - { - // The single pending-open worker is still processing the first login. - Assert.Equal(ConnectionState.Connecting, pending.State); - Assert.False(pendingOpen.IsCompleted); - } - else - { - Assert.True(SpinWait.SpinUntil(() => Volatile.Read(ref retiredPool._waitCount) == 2, TimeSpan.FromSeconds(10)), - "Second open did not wait for the creation semaphore."); - } - + Assert.False(opens.IsCompleted); + Task pendingOpen = Open(pending, async); + opens = Task.WhenAll(opens, pendingOpen); + AssertPendingOpen(pending, pendingOpen, retiredPool, async); Assert.Same(first.PoolGroup, pending.PoolGroup); - Assert.Equal(1, Volatile.Read(ref logins)); - Assert.False(firstOpen.IsCompleted); - Assert.False(pendingOpen.IsCompleted); + Assert.Equal(1, server.LoginCount); if (clearAll) { @@ -89,9 +58,8 @@ public async Task ClearDuringOpen_InFlightConnectionsAreDestroyedOnClose(bool as SqlConnection.ClearPool(first); } Assert.False(retiredPool.IsRunning); - releaseLogin.Set(); - Task opens = Task.WhenAll(firstOpen, pendingOpen); - Assert.Same(opens, await Task.WhenAny(opens, Task.Delay(TimeSpan.FromSeconds(20)))); + server.ReleaseFirstLogin(); + await AssertCompletes(opens); await opens; Assert.Equal(ConnectionState.Open, first.State); @@ -99,12 +67,12 @@ public async Task ClearDuringOpen_InFlightConnectionsAreDestroyedOnClose(bool as Assert.Same(retiredPool, first.InnerConnection.Pool); Assert.Same(retiredPool, pending.InnerConnection.Pool); Assert.Equal(2, retiredPool.Count); - Assert.Equal(2, Volatile.Read(ref logins)); + Assert.Equal(2, server.LoginCount); await Open(replacement, async); Assert.NotSame(retiredPool, replacement.InnerConnection.Pool); Assert.True(replacement.InnerConnection.Pool.IsRunning); - Assert.Equal(3, Volatile.Read(ref logins)); + Assert.Equal(3, server.LoginCount); first.Close(); pending.Close(); Assert.Equal(0, retiredPool.Count); @@ -113,9 +81,8 @@ public async Task ClearDuringOpen_InFlightConnectionsAreDestroyedOnClose(bool as } finally { - releaseLogin.Set(); - Task opens = pendingOpen is null ? firstOpen : Task.WhenAll(firstOpen, pendingOpen); - Assert.Same(opens, await Task.WhenAny(opens, Task.Delay(TimeSpan.FromSeconds(20)))); + server.ReleaseFirstLogin(); + await AssertCompletes(opens); if (opens.IsFaulted) { // Observe background failures without replacing an earlier assertion failure. @@ -128,9 +95,79 @@ public async Task ClearDuringOpen_InFlightConnectionsAreDestroyedOnClose(bool as } } + /// Confirms the second open is admitted before the test clears its pool. + private static void AssertPendingOpen(SqlConnection pending, Task open, WaitHandleDbConnectionPool pool, bool async) + { + if (async) + { + // The single pending-open worker is still processing the first login. + Assert.Equal(ConnectionState.Connecting, pending.State); + } + else + { + Assert.True(SpinWait.SpinUntil(() => Volatile.Read(ref pool._waitCount) == 2, TimeSpan.FromSeconds(10)), + "Second open did not wait for the creation semaphore."); + } + Assert.False(open.IsCompleted); + } + + /// Bounds a wait without propagating task failures that cleanup must only observe. + private static async Task AssertCompletes(Task task) => + Assert.Same(task, await Task.WhenAny(task, Task.Delay(TimeSpan.FromSeconds(20)))); + /// Runs synchronous opens on a dedicated thread so the test can release the login. private static Task Open(SqlConnection connection, bool async) => async ? connection.OpenAsync() : Task.Factory.StartNew(connection.Open, CancellationToken.None, TaskCreationOptions.LongRunning, TaskScheduler.Default); + + /// Owns the simulated server and a gate that blocks only its first login. + private sealed class GatedLoginServer : IDisposable + { + private readonly TdsServer _server = new(); + private readonly ManualResetEventSlim _loginEntered = new(); + private readonly ManualResetEventSlim _releaseLogin = new(); + private int _logins; + + internal int LoginCount => Volatile.Read(ref _logins); + internal string ConnectionString { get; } + + /// Starts an isolated server with room for both admitted connections. + internal GatedLoginServer() + { + _server.OnLogin7Validated = _ => + { + if (Interlocked.Increment(ref _logins) == 1) + { + _loginEntered.Set(); + Assert.True(_releaseLogin.Wait(TimeSpan.FromSeconds(15)), "Login was not released."); + } + }; + _server.Start(); + ConnectionString = new SqlConnectionStringBuilder + { + DataSource = $"localhost,{_server.EndPoint.Port}", + Encrypt = SqlConnectionEncryptOption.Optional, + Pooling = true, + MaxPoolSize = 2, + ConnectTimeout = 15, + }.ConnectionString; + } + + /// Waits until the first login reaches the gate. + internal void WaitForFirstLogin() => + Assert.True(_loginEntered.Wait(TimeSpan.FromSeconds(10)), "First login did not start."); + + /// Allows the first login to finish, including during failure cleanup. + internal void ReleaseFirstLogin() => _releaseLogin.Set(); + + /// Releases the gate and disposes the server before its synchronization objects. + public void Dispose() + { + ReleaseFirstLogin(); + _server.Dispose(); + _loginEntered.Dispose(); + _releaseLogin.Dispose(); + } + } } From 416abfdd9ee46f0929f8bc44b5e90c257818175c Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 22 Sep 2026 12:34:09 -0700 Subject: [PATCH 08/13] Organize pool regression tests as arrange act assert Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- ...andleDbConnectionPoolBlockingPeriodTest.cs | 8 +++++++ .../WaitHandleDbConnectionPoolShutdownTest.cs | 22 +++++++++++++++---- .../PoolClearDuringOpenTests.cs | 13 ++++++++++- 3 files changed, 38 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolBlockingPeriodTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolBlockingPeriodTest.cs index 80027ac319..163023a0f4 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolBlockingPeriodTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolBlockingPeriodTest.cs @@ -136,6 +136,7 @@ public void TryGetConnection_WhenFactoryThrows_EntersBlockingPeriod() [Fact] public void Shutdown_WhileBlocked_PreservesErrorUntilExpiry() { + // Arrange var clock = new FakeTimeProvider(); SqlException failure = SqlExceptionHelper.CreateSqlException("server unreachable"); var factory = new ConfigurableSqlConnectionFactory(_ => throw failure); @@ -143,10 +144,17 @@ public void Shutdown_WhileBlocked_PreservesErrorUntilExpiry() using var owner = new SqlConnection(); Assert.Throws(() => TryGetConnectionSync(pool, owner, out _)); + + // Act pool.Shutdown(); + + // Assert Assert.True(pool.ErrorOccurred); + // Act: expire the preserved blocking period. clock.Advance(TimeSpan.FromSeconds(5)); + + // Assert Assert.False(pool.ErrorOccurred); } diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs index 0b6285cee9..58e5d6248a 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs @@ -74,6 +74,7 @@ public void Shutdown_DisposesCleanupTimer() [Fact] public void Shutdown_LeavesIdleConnectionsUntilClear() { + // Arrange var pool = CreatePool(); // Vend a few connections then return them so they sit in _stackNew. @@ -91,22 +92,29 @@ public void Shutdown_LeavesIdleConnectionsUntilClear() try { + // Act pool.Shutdown(); + // Assert Assert.False(pool.IsRunning); Assert.Equal(2, pool.IdleCount); Assert.Equal(2, pool.Count); Assert.True(c1!.CanBePooled); Assert.True(c2!.CanBePooled); + + // Act: drain the retired pool explicitly. + pool.Clear(); + + // Assert + Assert.Equal(0, pool.IdleCount); + Assert.Equal(0, pool.Count); } finally { + // Cleanup pool.Shutdown(); pool.Clear(); } - - Assert.Equal(0, pool.IdleCount); - Assert.Equal(0, pool.Count); } // Shutdown is idempotent. @@ -191,6 +199,7 @@ public void TryGetConnection_Async_AfterShutdown_ShortCircuits_NoPendingOpenSche [InlineData(true, true)] public async Task Shutdown_InFlightRequest_CompletesOnRetiredPool(bool async, bool cancel) { + // Arrange using var factory = new GatedConnectionFactory(); var pool = CreatePool(maxPoolSize: 2, factory: factory); using var firstOwner = new SqlConnection(); @@ -207,14 +216,16 @@ public async Task Shutdown_InFlightRequest_CompletesOnRetiredPool(bool async, bo Assert.False(first.IsCompleted); Assert.False(pending.IsCompleted); + // Act pool.Shutdown(); - Assert.False(pool.IsRunning); if (cancel) { completion.SetCanceled(); } factory.Release.Set(); + // Assert + Assert.False(pool.IsRunning); Assert.Same(first, await Task.WhenAny(first, Task.Delay(TimeSpan.FromSeconds(10)))); DbConnectionInternal? firstConnection = await first; Assert.NotNull(firstConnection); @@ -236,6 +247,7 @@ public async Task Shutdown_InFlightRequest_CompletesOnRetiredPool(bool async, bo } finally { + // Cleanup factory.Release.Set(); pool.Shutdown(); await ReturnWhenCompleted(pool, firstOwner, first); @@ -244,6 +256,8 @@ public async Task Shutdown_InFlightRequest_CompletesOnRetiredPool(bool async, bo await ReturnWhenCompleted(pool, pendingOwner, pending); } } + + // Assert: returned connections were destroyed rather than pooled. Assert.Equal(0, pool.IdleCount); Assert.Equal(0, pool.Count); } diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs index c6667bce88..72c9168339 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs @@ -31,6 +31,7 @@ public class PoolClearDuringOpenTests [InlineData(true, true)] public async Task ClearDuringOpen_InFlightConnectionsAreDestroyedOnClose(bool async, bool clearAll) { + // Arrange using var switches = new LocalAppContextSwitchesHelper { UseConnectionPoolV2 = false }; using var server = new GatedLoginServer(); using var first = new SqlConnection(server.ConnectionString); @@ -49,6 +50,7 @@ public async Task ClearDuringOpen_InFlightConnectionsAreDestroyedOnClose(bool as Assert.Same(first.PoolGroup, pending.PoolGroup); Assert.Equal(1, server.LoginCount); + // Act if (clearAll) { SqlConnection.ClearAllPools(); @@ -57,11 +59,12 @@ public async Task ClearDuringOpen_InFlightConnectionsAreDestroyedOnClose(bool as { SqlConnection.ClearPool(first); } - Assert.False(retiredPool.IsRunning); server.ReleaseFirstLogin(); await AssertCompletes(opens); await opens; + // Assert + Assert.False(retiredPool.IsRunning); Assert.Equal(ConnectionState.Open, first.State); Assert.Equal(ConnectionState.Open, pending.State); Assert.Same(retiredPool, first.InnerConnection.Pool); @@ -69,18 +72,26 @@ public async Task ClearDuringOpen_InFlightConnectionsAreDestroyedOnClose(bool as Assert.Equal(2, retiredPool.Count); Assert.Equal(2, server.LoginCount); + // Act: open a new connection after clearing. await Open(replacement, async); + + // Assert Assert.NotSame(retiredPool, replacement.InnerConnection.Pool); Assert.True(replacement.InnerConnection.Pool.IsRunning); Assert.Equal(3, server.LoginCount); + + // Act: return the connections that finished on the retired pool. first.Close(); pending.Close(); + + // Assert Assert.Equal(0, retiredPool.Count); Assert.Equal(0, retiredPool.IdleCount); Assert.Equal(0, Volatile.Read(ref retiredPool._waitCount)); } finally { + // Cleanup server.ReleaseFirstLogin(); await AssertCompletes(opens); if (opens.IsFaulted) From 588b046e3654eb1f3f888f52611d8607ec6d9daf Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 22 Sep 2026 12:50:02 -0700 Subject: [PATCH 09/13] Explain pool regression test interleavings inline Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- ...andleDbConnectionPoolBlockingPeriodTest.cs | 7 +++- .../WaitHandleDbConnectionPoolShutdownTest.cs | 28 +++++++++++--- .../PoolClearDuringOpenTests.cs | 37 +++++++++++++++---- 3 files changed, 56 insertions(+), 16 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolBlockingPeriodTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolBlockingPeriodTest.cs index 163023a0f4..48682ee7d5 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolBlockingPeriodTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolBlockingPeriodTest.cs @@ -143,18 +143,21 @@ public void Shutdown_WhileBlocked_PreservesErrorUntilExpiry() var pool = CreatePool(factory, timeProvider: clock); using var owner = new SqlConnection(); + // A failed creation installs the cached exception and starts the blocking-period timer. Assert.Throws(() => TryGetConnectionSync(pool, owner, out _)); // Act pool.Shutdown(); - // Assert + // Assert: shutdown must not discard the error that admitted waiters can still observe. Assert.True(pool.ErrorOccurred); // Act: expire the preserved blocking period. + // Advance the injected clock by the initial five-second blocking period. This fires + // its expiry callback without sleeping or waiting five seconds of wall-clock time. clock.Advance(TimeSpan.FromSeconds(5)); - // Assert + // Assert: shutdown left the expiry mechanism working, rather than freezing the error. Assert.False(pool.ErrorOccurred); } diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs index 58e5d6248a..279f689cf7 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs @@ -77,7 +77,8 @@ public void Shutdown_LeavesIdleConnectionsUntilClear() // Arrange var pool = CreatePool(); - // Vend a few connections then return them so they sit in _stackNew. + // Hold two distinct connections before returning either, then make both idle. + // This gives Clear real inventory to drain rather than testing an empty pool. var owner1 = new SqlConnection(); var owner2 = new SqlConnection(); pool.TryGetConnection(owner1, taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? c1); @@ -95,14 +96,15 @@ public void Shutdown_LeavesIdleConnectionsUntilClear() // Act pool.Shutdown(); - // Assert + // Assert: shutdown stops admissions without draining or marking idle objects. + // CanBePooled describes the objects, not whether this retired pool accepts opens. Assert.False(pool.IsRunning); Assert.Equal(2, pool.IdleCount); Assert.Equal(2, pool.Count); Assert.True(c1!.CanBePooled); Assert.True(c2!.CanBePooled); - // Act: drain the retired pool explicitly. + // Act: perform the Clear that the factory owns, separately from shutdown. pool.Clear(); // Assert @@ -199,7 +201,9 @@ public void TryGetConnection_Async_AfterShutdown_ShortCircuits_NoPendingOpenSche [InlineData(true, true)] public async Task Shutdown_InFlightRequest_CompletesOnRetiredPool(bool async, bool cancel) { - // Arrange + // Arrange: use an empty pool with capacity for both connections. The first + // acquisition always runs synchronously on its own thread, holding the creation + // semaphore inside the mocked factory. Only the second acquisition varies. using var factory = new GatedConnectionFactory(); var pool = CreatePool(maxPoolSize: 2, factory: factory); using var firstOwner = new SqlConnection(); @@ -211,12 +215,15 @@ public async Task Shutdown_InFlightRequest_CompletesOnRetiredPool(bool async, bo { Assert.True(factory.Entered.Wait(TimeSpan.FromSeconds(10))); pending = Acquire(pool, pendingOwner, async ? completion : null); + // Both acquisitions have entered the pool, but only the first reached the + // factory. The gate keeps this interleaving stable until after shutdown. Assert.True(SpinWait.SpinUntil(() => Volatile.Read(ref pool._waitCount) == 2, TimeSpan.FromSeconds(10))); Assert.Equal(1, factory.CreateCount); Assert.False(first.IsCompleted); Assert.False(pending.IsCompleted); - // Act + // Act: shut down before either creation can finish. Cancellation, when + // requested, must win before the worker can deliver its connection. pool.Shutdown(); if (cancel) { @@ -226,6 +233,8 @@ public async Task Shutdown_InFlightRequest_CompletesOnRetiredPool(bool async, bo // Assert Assert.False(pool.IsRunning); + // These delays are hang guards, not race triggers. WhenAny identifies the + // completed task; the following awaits read its result or cancellation. Assert.Same(first, await Task.WhenAny(first, Task.Delay(TimeSpan.FromSeconds(10)))); DbConnectionInternal? firstConnection = await first; Assert.NotNull(firstConnection); @@ -234,6 +243,8 @@ public async Task Shutdown_InFlightRequest_CompletesOnRetiredPool(bool async, bo if (cancel) { await Assert.ThrowsAnyAsync(() => pending); + // Cancellation completes the caller's task before worker cleanup. + // Wait until the worker destroys its connection, leaving only the first. Assert.True(SpinWait.SpinUntil(() => pool.Count == 1 && Volatile.Read(ref pool._waitCount) == 0, TimeSpan.FromSeconds(10))); } else @@ -243,11 +254,14 @@ public async Task Shutdown_InFlightRequest_CompletesOnRetiredPool(bool async, bo Assert.Same(pool, pendingConnection.Pool); Assert.Equal(0, Volatile.Read(ref pool._waitCount)); } + // Even the canceled case must reach the second creation. Otherwise an + // early shutdown failure could be hidden by the already-canceled task. Assert.Equal(2, factory.CreateCount); } finally { - // Cleanup + // Cleanup: release a blocked factory even if a precondition failed. + // Shutdown is safe to repeat and makes returned connections get destroyed. factory.Release.Set(); pool.Shutdown(); await ReturnWhenCompleted(pool, firstOwner, first); @@ -275,6 +289,8 @@ public async Task Shutdown_InFlightRequest_CompletesOnRetiredPool(bool async, bo return (DbConnectionInternal?)connection; }, CancellationToken.None, TaskCreationOptions.LongRunning, TaskScheduler.Default); } + // A false return here means async acquisition was queued, not that it failed. + // Its eventual result is delivered through completion.Task. Assert.False(pool.TryGetConnection(owner, completion, timer, out DbConnectionInternal pending)); Assert.Null(pending); return completion.Task!; diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs index 72c9168339..55c9bbc574 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs @@ -31,7 +31,8 @@ public class PoolClearDuringOpenTests [InlineData(true, true)] public async Task ClearDuringOpen_InFlightConnectionsAreDestroyedOnClose(bool async, bool clearAll) { - // Arrange + // Arrange: exercise the default pool through the public APIs, not a mocked factory. + // Each server has its own port, so these connections start with an empty pool. using var switches = new LocalAppContextSwitchesHelper { UseConnectionPoolV2 = false }; using var server = new GatedLoginServer(); using var first = new SqlConnection(server.ConnectionString); @@ -41,16 +42,20 @@ public async Task ClearDuringOpen_InFlightConnectionsAreDestroyedOnClose(bool as try { + // Stop the first open inside LOGIN7. It cannot finish until this test releases + // the gate, leaving time to admit a second open on the same pool before clearing. server.WaitForFirstLogin(); var retiredPool = Assert.IsType(first.PoolGroup.GetConnectionPool(SqlConnectionFactory.Instance)); Assert.False(opens.IsCompleted); Task pendingOpen = Open(pending, async); + // Keep both opens together for the success path and for cleanup on failure. opens = Task.WhenAll(opens, pendingOpen); AssertPendingOpen(pending, pendingOpen, retiredPool, async); Assert.Same(first.PoolGroup, pending.PoolGroup); Assert.Equal(1, server.LoginCount); - // Act + // Act: retire the pool while both opens are outstanding. The gate, rather than + // a sleep or repeated attempts, forces clearing to happen before login finishes. if (clearAll) { SqlConnection.ClearAllPools(); @@ -60,10 +65,13 @@ public async Task ClearDuringOpen_InFlightConnectionsAreDestroyedOnClose(bool as SqlConnection.ClearPool(first); } server.ReleaseFirstLogin(); + // WhenAll includes the second open, not just the login we released. The helper + // bounds the wait; awaiting opens then propagates either open's failure. await AssertCompletes(opens); await opens; - // Assert + // Assert: both admitted opens succeeded on the original, now-retired pool. + // Exactly two logins rules out reusing an idle connection or an extra login retry. Assert.False(retiredPool.IsRunning); Assert.Equal(ConnectionState.Open, first.State); Assert.Equal(ConnectionState.Open, pending.State); @@ -75,7 +83,8 @@ public async Task ClearDuringOpen_InFlightConnectionsAreDestroyedOnClose(bool as // Act: open a new connection after clearing. await Open(replacement, async); - // Assert + // Assert: only opens admitted before clearing may finish on the retired pool. + // This later open must use a running replacement and perform the third login. Assert.NotSame(retiredPool, replacement.InnerConnection.Pool); Assert.True(replacement.InnerConnection.Pool.IsRunning); Assert.Equal(3, server.LoginCount); @@ -84,14 +93,16 @@ public async Task ClearDuringOpen_InFlightConnectionsAreDestroyedOnClose(bool as first.Close(); pending.Close(); - // Assert + // Assert: closing removes both connections instead of caching them for reuse. + // No acquisition should remain counted as waiting on the retired pool. Assert.Equal(0, retiredPool.Count); Assert.Equal(0, retiredPool.IdleCount); Assert.Equal(0, Volatile.Read(ref retiredPool._waitCount)); } finally { - // Cleanup + // Cleanup also runs if setup or an assertion fails. Release the login first, + // then wait for both opens before closing connections and disposing the server. server.ReleaseFirstLogin(); await AssertCompletes(opens); if (opens.IsFaulted) @@ -102,6 +113,7 @@ public async Task ClearDuringOpen_InFlightConnectionsAreDestroyedOnClose(bool as first.Close(); pending.Close(); replacement.Close(); + // Closing replacement returns it to its live pool; clear that pool as well. SqlConnection.ClearPool(first); } } @@ -111,18 +123,25 @@ private static void AssertPendingOpen(SqlConnection pending, Task open, WaitHand { if (async) { - // The single pending-open worker is still processing the first login. + // OpenAsync has queued the second request behind the worker handling the first + // login. It has not entered the inner acquisition loop, so _waitCount is still 1. Assert.Equal(ConnectionState.Connecting, pending.State); } else { + // Sync opens run on separate threads. Both have entered acquisition, but the + // first holds the creation semaphore and the second cannot create yet. Assert.True(SpinWait.SpinUntil(() => Volatile.Read(ref pool._waitCount) == 2, TimeSpan.FromSeconds(10)), "Second open did not wait for the creation semaphore."); } Assert.False(open.IsCompleted); } - /// Bounds a wait without propagating task failures that cleanup must only observe. + /// + /// Fails if the delay wins, preventing a stuck open from hanging the test. + /// WhenAny returns the completed task without propagating its outcome; the caller + /// awaits that task to check success, or only observes its exception during cleanup. + /// private static async Task AssertCompletes(Task task) => Assert.Same(task, await Task.WhenAny(task, Task.Delay(TimeSpan.FromSeconds(20)))); @@ -148,6 +167,8 @@ internal GatedLoginServer() { _server.OnLogin7Validated = _ => { + // Only the first login blocks. The second and replacement opens must be + // able to finish without another release from the test. if (Interlocked.Increment(ref _logins) == 1) { _loginEntered.Set(); From 5e94513d32cad0c5884bb04cca521f7dc08a1a6a Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 22 Sep 2026 12:51:00 -0700 Subject: [PATCH 10/13] Trim blocking-period test commentary Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../WaitHandleDbConnectionPoolBlockingPeriodTest.cs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolBlockingPeriodTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolBlockingPeriodTest.cs index 48682ee7d5..163023a0f4 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolBlockingPeriodTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolBlockingPeriodTest.cs @@ -143,21 +143,18 @@ public void Shutdown_WhileBlocked_PreservesErrorUntilExpiry() var pool = CreatePool(factory, timeProvider: clock); using var owner = new SqlConnection(); - // A failed creation installs the cached exception and starts the blocking-period timer. Assert.Throws(() => TryGetConnectionSync(pool, owner, out _)); // Act pool.Shutdown(); - // Assert: shutdown must not discard the error that admitted waiters can still observe. + // Assert Assert.True(pool.ErrorOccurred); // Act: expire the preserved blocking period. - // Advance the injected clock by the initial five-second blocking period. This fires - // its expiry callback without sleeping or waiting five seconds of wall-clock time. clock.Advance(TimeSpan.FromSeconds(5)); - // Assert: shutdown left the expiry mechanism working, rather than freezing the error. + // Assert Assert.False(pool.ErrorOccurred); } From 7627746e64152f3e775f0684f9c66401832b1578 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 22 Sep 2026 12:54:00 -0700 Subject: [PATCH 11/13] Explain regression conditions and assertion evidence Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../WaitHandleDbConnectionPoolShutdownTest.cs | 39 +++++++------ .../PoolClearDuringOpenTests.cs | 57 +++++++++---------- 2 files changed, 50 insertions(+), 46 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs index 279f689cf7..b198c5a4ba 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs @@ -77,8 +77,8 @@ public void Shutdown_LeavesIdleConnectionsUntilClear() // Arrange var pool = CreatePool(); - // Hold two distinct connections before returning either, then make both idle. - // This gives Clear real inventory to drain rather than testing an empty pool. + // An empty pool would pass even if Shutdown still called Clear internally. + // Keep idle inventory so the two lifecycle operations have distinguishable effects. var owner1 = new SqlConnection(); var owner2 = new SqlConnection(); pool.TryGetConnection(owner1, taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? c1); @@ -96,8 +96,8 @@ public void Shutdown_LeavesIdleConnectionsUntilClear() // Act pool.Shutdown(); - // Assert: shutdown stops admissions without draining or marking idle objects. - // CanBePooled describes the objects, not whether this retired pool accepts opens. + // Assert: unchanged inventory and poolability detect both effects of an + // unintended Clear: draining idle objects and marking them non-poolable. Assert.False(pool.IsRunning); Assert.Equal(2, pool.IdleCount); Assert.Equal(2, pool.Count); @@ -201,9 +201,9 @@ public void TryGetConnection_Async_AfterShutdown_ShortCircuits_NoPendingOpenSche [InlineData(true, true)] public async Task Shutdown_InFlightRequest_CompletesOnRetiredPool(bool async, bool cancel) { - // Arrange: use an empty pool with capacity for both connections. The first - // acquisition always runs synchronously on its own thread, holding the creation - // semaphore inside the mocked factory. Only the second acquisition varies. + // Arrange: contention must come from an in-flight creation, not MaxPoolSize. + // A synchronous first request holds the creation semaphore inside the factory, + // letting the second request reach the wait loop even when it is asynchronous. using var factory = new GatedConnectionFactory(); var pool = CreatePool(maxPoolSize: 2, factory: factory); using var firstOwner = new SqlConnection(); @@ -215,26 +215,30 @@ public async Task Shutdown_InFlightRequest_CompletesOnRetiredPool(bool async, bo { Assert.True(factory.Entered.Wait(TimeSpan.FromSeconds(10))); pending = Acquire(pool, pendingOwner, async ? completion : null); - // Both acquisitions have entered the pool, but only the first reached the - // factory. The gate keeps this interleaving stable until after shutdown. + // Starting a request is not enough: it must enter acquisition before shutdown, + // or this would test rejection of a new arrival rather than an admitted waiter. + // Two acquisitions but one factory call pins the second at the creation wait. Assert.True(SpinWait.SpinUntil(() => Volatile.Read(ref pool._waitCount) == 2, TimeSpan.FromSeconds(10))); Assert.Equal(1, factory.CreateCount); Assert.False(first.IsCompleted); Assert.False(pending.IsCompleted); - // Act: shut down before either creation can finish. Cancellation, when - // requested, must win before the worker can deliver its connection. + // Act: release the creation semaphore only after retirement, forcing the second + // request to encounter the removed post-wait shutdown guard. In the async case, + // that guard previously turned retirement into a misleading timeout. pool.Shutdown(); if (cancel) { + // Make cancellation win before creation resumes, so cleanup is exercised + // deterministically rather than racing task completion by chance. completion.SetCanceled(); } factory.Release.Set(); // Assert Assert.False(pool.IsRunning); - // These delays are hang guards, not race triggers. WhenAny identifies the - // completed task; the following awaits read its result or cancellation. + // Pool identity rules out silently moving the requests to a new pool. + // The bounded waits only prevent hangs; they do not establish the interleaving. Assert.Same(first, await Task.WhenAny(first, Task.Delay(TimeSpan.FromSeconds(10)))); DbConnectionInternal? firstConnection = await first; Assert.NotNull(firstConnection); @@ -243,8 +247,9 @@ public async Task Shutdown_InFlightRequest_CompletesOnRetiredPool(bool async, bo if (cancel) { await Assert.ThrowsAnyAsync(() => pending); - // Cancellation completes the caller's task before worker cleanup. - // Wait until the worker destroys its connection, leaving only the first. + // A canceled task alone says nothing about the connection created for it. + // With the first connection still checked out, Count == 1 and no waiters + // establish that the worker has finished returning the canceled result. Assert.True(SpinWait.SpinUntil(() => pool.Count == 1 && Volatile.Read(ref pool._waitCount) == 0, TimeSpan.FromSeconds(10))); } else @@ -260,8 +265,8 @@ public async Task Shutdown_InFlightRequest_CompletesOnRetiredPool(bool async, bo } finally { - // Cleanup: release a blocked factory even if a precondition failed. - // Shutdown is safe to repeat and makes returned connections get destroyed. + // Cleanup must also handle failure before the Act section. Otherwise the + // first creation could stay blocked or its connection return to a live pool. factory.Release.Set(); pool.Shutdown(); await ReturnWhenCompleted(pool, firstOwner, first); diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs index 55c9bbc574..76fe2f95cb 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs @@ -31,8 +31,8 @@ public class PoolClearDuringOpenTests [InlineData(true, true)] public async Task ClearDuringOpen_InFlightConnectionsAreDestroyedOnClose(bool async, bool clearAll) { - // Arrange: exercise the default pool through the public APIs, not a mocked factory. - // Each server has its own port, so these connections start with an empty pool. + // Arrange: use the affected wait-handle pool. A unique server port prevents an + // existing idle connection from satisfying an open without exercising login. using var switches = new LocalAppContextSwitchesHelper { UseConnectionPoolV2 = false }; using var server = new GatedLoginServer(); using var first = new SqlConnection(server.ConnectionString); @@ -42,20 +42,21 @@ public async Task ClearDuringOpen_InFlightConnectionsAreDestroyedOnClose(bool as try { - // Stop the first open inside LOGIN7. It cannot finish until this test releases - // the gate, leaving time to admit a second open on the same pool before clearing. + // The regression requires clearing after pool selection but before login finishes. + // Holding the server's LOGIN7 response fixes that window in place. The second open + // cannot get past the first creation, even though the pool has capacity for both. server.WaitForFirstLogin(); var retiredPool = Assert.IsType(first.PoolGroup.GetConnectionPool(SqlConnectionFactory.Instance)); Assert.False(opens.IsCompleted); Task pendingOpen = Open(pending, async); - // Keep both opens together for the success path and for cleanup on failure. opens = Task.WhenAll(opens, pendingOpen); AssertPendingOpen(pending, pendingOpen, retiredPool, async); Assert.Same(first.PoolGroup, pending.PoolGroup); Assert.Equal(1, server.LoginCount); - // Act: retire the pool while both opens are outstanding. The gate, rather than - // a sleep or repeated attempts, forces clearing to happen before login finishes. + // Act: the pending request now belongs to the pool being retired. With the old + // post-wait shutdown guard, the queued async request reported a pool timeout here, + // despite available capacity and a server that can complete both logins. if (clearAll) { SqlConnection.ClearAllPools(); @@ -65,13 +66,13 @@ public async Task ClearDuringOpen_InFlightConnectionsAreDestroyedOnClose(bool as SqlConnection.ClearPool(first); } server.ReleaseFirstLogin(); - // WhenAll includes the second open, not just the login we released. The helper - // bounds the wait; awaiting opens then propagates either open's failure. + // Releasing the gate is not evidence of success: both opens must actually finish. await AssertCompletes(opens); await opens; - // Assert: both admitted opens succeeded on the original, now-retired pool. - // Exactly two logins rules out reusing an idle connection or an extra login retry. + // Assert: success alone could hide a retry against a replacement pool. Checking + // ownership proves the admitted requests finished on their original pool. + // Two logins and two retained connections account for both physical creations. Assert.False(retiredPool.IsRunning); Assert.Equal(ConnectionState.Open, first.State); Assert.Equal(ConnectionState.Open, pending.State); @@ -80,29 +81,29 @@ public async Task ClearDuringOpen_InFlightConnectionsAreDestroyedOnClose(bool as Assert.Equal(2, retiredPool.Count); Assert.Equal(2, server.LoginCount); - // Act: open a new connection after clearing. + // Act await Open(replacement, async); - // Assert: only opens admitted before clearing may finish on the retired pool. - // This later open must use a running replacement and perform the third login. + // Assert: allowing admitted opens to finish must not make the retired pool + // available to new callers. A different, running pool proves that distinction. Assert.NotSame(retiredPool, replacement.InnerConnection.Pool); Assert.True(replacement.InnerConnection.Pool.IsRunning); Assert.Equal(3, server.LoginCount); - // Act: return the connections that finished on the retired pool. + // Act first.Close(); pending.Close(); - // Assert: closing removes both connections instead of caching them for reuse. - // No acquisition should remain counted as waiting on the retired pool. + // Assert: ordinary Close would leave these connections idle in a running pool. + // Zero total and idle counts prove the retired pool kept neither connection. Assert.Equal(0, retiredPool.Count); Assert.Equal(0, retiredPool.IdleCount); Assert.Equal(0, Volatile.Read(ref retiredPool._waitCount)); } finally { - // Cleanup also runs if setup or an assertion fails. Release the login first, - // then wait for both opens before closing connections and disposing the server. + // Cleanup: a failed precondition can leave login blocked. Do not dispose the + // server while an open still uses it, or cleanup could obscure the original failure. server.ReleaseFirstLogin(); await AssertCompletes(opens); if (opens.IsFaulted) @@ -113,7 +114,6 @@ public async Task ClearDuringOpen_InFlightConnectionsAreDestroyedOnClose(bool as first.Close(); pending.Close(); replacement.Close(); - // Closing replacement returns it to its live pool; clear that pool as well. SqlConnection.ClearPool(first); } } @@ -123,14 +123,15 @@ private static void AssertPendingOpen(SqlConnection pending, Task open, WaitHand { if (async) { - // OpenAsync has queued the second request behind the worker handling the first - // login. It has not entered the inner acquisition loop, so _waitCount is still 1. + // The first login occupies the sole pending-open worker. OpenAsync must have + // returned an incomplete task on this pool before we clear it; waiting for + // _waitCount == 2 would deadlock setup because the worker cannot start request two. Assert.Equal(ConnectionState.Connecting, pending.State); } else { - // Sync opens run on separate threads. Both have entered acquisition, but the - // first holds the creation semaphore and the second cannot create yet. + // Merely starting a thread does not prove it selected the old pool. Waiting for + // both acquisitions to enter prevents clearing before the second request arrives. Assert.True(SpinWait.SpinUntil(() => Volatile.Read(ref pool._waitCount) == 2, TimeSpan.FromSeconds(10)), "Second open did not wait for the creation semaphore."); } @@ -138,9 +139,9 @@ private static void AssertPendingOpen(SqlConnection pending, Task open, WaitHand } /// - /// Fails if the delay wins, preventing a stuck open from hanging the test. - /// WhenAny returns the completed task without propagating its outcome; the caller - /// awaits that task to check success, or only observes its exception during cleanup. + /// Bounds failures so a stranded request cannot hang the suite. This does not establish + /// the race ordering or assert success; the gate establishes ordering and the caller + /// awaits the completed task to check its outcome. /// private static async Task AssertCompletes(Task task) => Assert.Same(task, await Task.WhenAny(task, Task.Delay(TimeSpan.FromSeconds(20)))); @@ -167,8 +168,6 @@ internal GatedLoginServer() { _server.OnLogin7Validated = _ => { - // Only the first login blocks. The second and replacement opens must be - // able to finish without another release from the test. if (Interlocked.Increment(ref _logins) == 1) { _loginEntered.Set(); From 5208da2d434857e68729f093e077fa612c4edb60 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 22 Sep 2026 13:06:03 -0700 Subject: [PATCH 12/13] Clarify shutdown test walkthrough Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../WaitHandleDbConnectionPoolShutdownTest.cs | 115 ++++++++---------- 1 file changed, 53 insertions(+), 62 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs index b198c5a4ba..c0be93f156 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs @@ -201,9 +201,8 @@ public void TryGetConnection_Async_AfterShutdown_ShortCircuits_NoPendingOpenSche [InlineData(true, true)] public async Task Shutdown_InFlightRequest_CompletesOnRetiredPool(bool async, bool cancel) { - // Arrange: contention must come from an in-flight creation, not MaxPoolSize. - // A synchronous first request holds the creation semaphore inside the factory, - // letting the second request reach the wait loop even when it is asynchronous. + // Arrange + // Initiate a request to the pool. It will be blocked by the gated connection factory. using var factory = new GatedConnectionFactory(); var pool = CreatePool(maxPoolSize: 2, factory: factory); using var firstOwner = new SqlConnection(); @@ -211,69 +210,61 @@ public async Task Shutdown_InFlightRequest_CompletesOnRetiredPool(bool async, bo var completion = new TaskCompletionSource(); Task first = Acquire(pool, firstOwner, completion: null); Task? pending = null; - try + + // Make sure the first request is blocked in the connection factory. Then, initiate a second request. + // The second request will be blocked on the create semaphore in the pool. + Assert.True(factory.Entered.Wait(TimeSpan.FromSeconds(10))); + pending = Acquire(pool, pendingOwner, async ? completion : null); + // Make sure the second request is also blocked. + Assert.True(SpinWait.SpinUntil(() => Volatile.Read(ref pool._waitCount) == 2, TimeSpan.FromSeconds(10))); + Assert.Equal(1, factory.CreateCount); + Assert.False(first.IsCompleted); + Assert.False(pending.IsCompleted); + + // Act + // Now, shut down the pool. New requests will no longer be accepted, but in-flight requests should proceed. + pool.Shutdown(); + if (cancel) { - Assert.True(factory.Entered.Wait(TimeSpan.FromSeconds(10))); - pending = Acquire(pool, pendingOwner, async ? completion : null); - // Starting a request is not enough: it must enter acquisition before shutdown, - // or this would test rejection of a new arrival rather than an admitted waiter. - // Two acquisitions but one factory call pins the second at the creation wait. - Assert.True(SpinWait.SpinUntil(() => Volatile.Read(ref pool._waitCount) == 2, TimeSpan.FromSeconds(10))); - Assert.Equal(1, factory.CreateCount); - Assert.False(first.IsCompleted); - Assert.False(pending.IsCompleted); - - // Act: release the creation semaphore only after retirement, forcing the second - // request to encounter the removed post-wait shutdown guard. In the async case, - // that guard previously turned retirement into a misleading timeout. - pool.Shutdown(); - if (cancel) - { - // Make cancellation win before creation resumes, so cleanup is exercised - // deterministically rather than racing task completion by chance. - completion.SetCanceled(); - } - factory.Release.Set(); + completion.SetCanceled(); + } - // Assert - Assert.False(pool.IsRunning); - // Pool identity rules out silently moving the requests to a new pool. - // The bounded waits only prevent hangs; they do not establish the interleaving. - Assert.Same(first, await Task.WhenAny(first, Task.Delay(TimeSpan.FromSeconds(10)))); - DbConnectionInternal? firstConnection = await first; - Assert.NotNull(firstConnection); - Assert.Same(pool, firstConnection.Pool); - Assert.Same(pending, await Task.WhenAny(pending, Task.Delay(TimeSpan.FromSeconds(10)))); - if (cancel) - { - await Assert.ThrowsAnyAsync(() => pending); - // A canceled task alone says nothing about the connection created for it. - // With the first connection still checked out, Count == 1 and no waiters - // establish that the worker has finished returning the canceled result. - Assert.True(SpinWait.SpinUntil(() => pool.Count == 1 && Volatile.Read(ref pool._waitCount) == 0, TimeSpan.FromSeconds(10))); - } - else - { - DbConnectionInternal? pendingConnection = await pending; - Assert.NotNull(pendingConnection); - Assert.Same(pool, pendingConnection.Pool); - Assert.Equal(0, Volatile.Read(ref pool._waitCount)); - } - // Even the canceled case must reach the second creation. Otherwise an - // early shutdown failure could be hidden by the already-canceled task. - Assert.Equal(2, factory.CreateCount); + // Unblock the first request, allowing both to proceed, in turn. + factory.Release.Set(); + + // Assert + // Wait for the first request to complete + Assert.False(pool.IsRunning); + Assert.Same(first, await Task.WhenAny(first, Task.Delay(TimeSpan.FromSeconds(10)))); + DbConnectionInternal? firstConnection = await first; + Assert.NotNull(firstConnection); + Assert.Same(pool, firstConnection.Pool); + + // Wait for the second request to complete + Assert.Same(pending, await Task.WhenAny(pending, Task.Delay(TimeSpan.FromSeconds(10)))); + if (cancel) + { + await Assert.ThrowsAnyAsync(() => pending); + Assert.True(SpinWait.SpinUntil(() => pool.Count == 1 && Volatile.Read(ref pool._waitCount) == 0, TimeSpan.FromSeconds(10))); } - finally + else { - // Cleanup must also handle failure before the Act section. Otherwise the - // first creation could stay blocked or its connection return to a live pool. - factory.Release.Set(); - pool.Shutdown(); - await ReturnWhenCompleted(pool, firstOwner, first); - if (pending is not null) - { - await ReturnWhenCompleted(pool, pendingOwner, pending); - } + DbConnectionInternal? pendingConnection = await pending; + Assert.NotNull(pendingConnection); + Assert.Same(pool, pendingConnection.Pool); + Assert.Equal(0, Volatile.Read(ref pool._waitCount)); + } + + // Assert that both requests created new connections + Assert.Equal(2, factory.CreateCount); + + // Cleanup + factory.Release.Set(); + pool.Shutdown(); + await ReturnWhenCompleted(pool, firstOwner, first); + if (pending is not null) + { + await ReturnWhenCompleted(pool, pendingOwner, pending); } // Assert: returned connections were destroyed rather than pooled. From f24ed279648d83596d571c7918e16419a8df1748 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 22 Sep 2026 13:06:44 -0700 Subject: [PATCH 13/13] Match pool-clear comments to shutdown walkthrough Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../PoolClearDuringOpenTests.cs | 53 ++++++++++--------- 1 file changed, 27 insertions(+), 26 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs index 76fe2f95cb..9b6bf1f2b4 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/PoolClearDuringOpenTests.cs @@ -31,8 +31,8 @@ public class PoolClearDuringOpenTests [InlineData(true, true)] public async Task ClearDuringOpen_InFlightConnectionsAreDestroyedOnClose(bool async, bool clearAll) { - // Arrange: use the affected wait-handle pool. A unique server port prevents an - // existing idle connection from satisfying an open without exercising login. + // Arrange + // Initiate an open using the wait-handle pool. Its login will be blocked by the gated server. using var switches = new LocalAppContextSwitchesHelper { UseConnectionPoolV2 = false }; using var server = new GatedLoginServer(); using var first = new SqlConnection(server.ConnectionString); @@ -42,21 +42,21 @@ public async Task ClearDuringOpen_InFlightConnectionsAreDestroyedOnClose(bool as try { - // The regression requires clearing after pool selection but before login finishes. - // Holding the server's LOGIN7 response fixes that window in place. The second open - // cannot get past the first creation, even though the pool has capacity for both. + // Make sure the first open is blocked during login. Then, initiate a second open. + // The second open must wait for the first creation to finish. server.WaitForFirstLogin(); var retiredPool = Assert.IsType(first.PoolGroup.GetConnectionPool(SqlConnectionFactory.Instance)); Assert.False(opens.IsCompleted); Task pendingOpen = Open(pending, async); opens = Task.WhenAll(opens, pendingOpen); + + // Make sure the second open is pending on the same pool and has not started its login. AssertPendingOpen(pending, pendingOpen, retiredPool, async); Assert.Same(first.PoolGroup, pending.PoolGroup); Assert.Equal(1, server.LoginCount); - // Act: the pending request now belongs to the pool being retired. With the old - // post-wait shutdown guard, the queued async request reported a pool timeout here, - // despite available capacity and a server that can complete both logins. + // Act + // Now, clear the pool. New opens should use a new pool, but these in-flight opens should proceed. if (clearAll) { SqlConnection.ClearAllPools(); @@ -65,45 +65,48 @@ public async Task ClearDuringOpen_InFlightConnectionsAreDestroyedOnClose(bool as { SqlConnection.ClearPool(first); } + + // Unblock the first login, allowing both opens to proceed, in turn. server.ReleaseFirstLogin(); - // Releasing the gate is not evidence of success: both opens must actually finish. + + // Assert + // Wait for both opens to complete, then confirm they succeeded on the original pool. await AssertCompletes(opens); await opens; - // Assert: success alone could hide a retry against a replacement pool. Checking - // ownership proves the admitted requests finished on their original pool. - // Two logins and two retained connections account for both physical creations. Assert.False(retiredPool.IsRunning); Assert.Equal(ConnectionState.Open, first.State); Assert.Equal(ConnectionState.Open, pending.State); Assert.Same(retiredPool, first.InnerConnection.Pool); Assert.Same(retiredPool, pending.InnerConnection.Pool); + + // Assert that both opens created new connections. Assert.Equal(2, retiredPool.Count); Assert.Equal(2, server.LoginCount); // Act + // Initiate another open after clearing. This one should use a new pool. await Open(replacement, async); - // Assert: allowing admitted opens to finish must not make the retired pool - // available to new callers. A different, running pool proves that distinction. + // Assert + // Make sure the new open used a different, running pool and performed another login. Assert.NotSame(retiredPool, replacement.InnerConnection.Pool); Assert.True(replacement.InnerConnection.Pool.IsRunning); Assert.Equal(3, server.LoginCount); // Act + // Close the connections that completed on the retired pool. They should be destroyed, not reused. first.Close(); pending.Close(); - // Assert: ordinary Close would leave these connections idle in a running pool. - // Zero total and idle counts prove the retired pool kept neither connection. + // Assert: the retired pool has no connections or pending requests left. Assert.Equal(0, retiredPool.Count); Assert.Equal(0, retiredPool.IdleCount); Assert.Equal(0, Volatile.Read(ref retiredPool._waitCount)); } finally { - // Cleanup: a failed precondition can leave login blocked. Do not dispose the - // server while an open still uses it, or cleanup could obscure the original failure. + // Cleanup server.ReleaseFirstLogin(); await AssertCompletes(opens); if (opens.IsFaulted) @@ -123,15 +126,14 @@ private static void AssertPendingOpen(SqlConnection pending, Task open, WaitHand { if (async) { - // The first login occupies the sole pending-open worker. OpenAsync must have - // returned an incomplete task on this pool before we clear it; waiting for - // _waitCount == 2 would deadlock setup because the worker cannot start request two. + // The second async open is queued behind the worker handling the first login. + // It has not entered the pool's wait loop yet, so _waitCount will still be 1. Assert.Equal(ConnectionState.Connecting, pending.State); } else { - // Merely starting a thread does not prove it selected the old pool. Waiting for - // both acquisitions to enter prevents clearing before the second request arrives. + // The sync opens run on separate threads. Wait until the second has entered + // the pool and is blocked on the creation semaphore before allowing the test to clear it. Assert.True(SpinWait.SpinUntil(() => Volatile.Read(ref pool._waitCount) == 2, TimeSpan.FromSeconds(10)), "Second open did not wait for the creation semaphore."); } @@ -139,9 +141,8 @@ private static void AssertPendingOpen(SqlConnection pending, Task open, WaitHand } /// - /// Bounds failures so a stranded request cannot hang the suite. This does not establish - /// the race ordering or assert success; the gate establishes ordering and the caller - /// awaits the completed task to check its outcome. + /// Prevents a stuck open from hanging the test. The caller awaits the completed task + /// separately to check whether the opens succeeded. /// private static async Task AssertCompletes(Task task) => Assert.Same(task, await Task.WhenAny(task, Task.Delay(TimeSpan.FromSeconds(20))));