From 6431209ad91ae4d18efced5c390bbbbc05682d20 Mon Sep 17 00:00:00 2001 From: Priyanka Tiwari Date: Thu, 21 May 2026 17:48:23 +0530 Subject: [PATCH 01/17] Implement pool shutdown for ChannelDbConnectionPool and harden WaitHandleDbConnectionPool shutdown (AB#44828) ChannelDbConnectionPool: - Shutdown() now transitions State to ShuttingDown (FR-001), completes the idle channel writer to unblock async waiters (FR-002, FR-007), and drains and destroys buffered idle connections (FR-003). Implementation is idempotent via Interlocked.CompareExchange (FR-006). - Startup() emits a trace and is a structural counterpart to Shutdown. - ReturnInternalConnection no longer asserts on TryWrite; if the channel was completed by a concurrent shutdown, the returning connection is destroyed (FR-004 race-window guard). - IdleConnectionChannel.Complete() exposes channel completion to the pool. WaitHandleDbConnectionPool: - Shutdown() is now idempotent (FR-006), disposes both _cleanupTimer and _errorTimer (FR-005), wakes threads parked in WaitHandle.WaitAny by releasing the pool semaphore (FR-007), and drains _stackNew/_stackOld (FR-003). - CleanupCallback and ErrorCallback short-circuit when State == ShuttingDown so an in-flight callback racing with Shutdown does not re-arm work. - The private TryGetConnection wait loop re-checks State after WaitHandle.WaitAny and bails out, so waiters cannot spin against a drained pool. Tests: - ChannelDbConnectionPoolShutdownTest (7 tests) covers state transition, drain, return-after-shutdown, idempotency, async waiter unblock, post-shutdown return, and Startup-after-Shutdown. - WaitHandleDbConnectionPoolShutdownTest (9 tests) covers state transition, cleanup/error timer disposal, drain, idempotency, callback no-op after shutdown, sync TryGetConnection short-circuit, and sync waiter unblock. Verified by running targeted suite: 340 tests passing across net8.0, net9.0, net10.0, and net462. Spec: specs/004-pool-shutdown. --- .../ConnectionPool/ChannelDbConnectionPool.cs | 51 +++- .../ConnectionPool/IdleConnectionChannel.cs | 11 +- .../WaitHandleDbConnectionPool.cs | 66 ++++- .../ChannelDbConnectionPoolShutdownTest.cs | 190 ++++++++++++++ .../WaitHandleDbConnectionPoolShutdownTest.cs | 232 ++++++++++++++++++ 5 files changed, 540 insertions(+), 10 deletions(-) create mode 100644 src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolShutdownTest.cs create mode 100644 src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index b83434cac6..a20829a332 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -92,6 +92,13 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool /// Must be updated using operations to ensure thread safety. /// private volatile int _isClearing; + + /// + /// Tracks whether has already initiated the shutdown sequence so that + /// repeated calls are observed as no-ops (FR-006). Updated atomically via + /// . + /// + private int _shutdownInitiated; #endregion /// @@ -254,21 +261,57 @@ public void ReturnInternalConnection(DbConnectionInternal connection, DbConnecti } else { - var written = _idleChannel.TryWrite(connection); - Debug.Assert(written, "Failed to write returning connection to the idle channel."); + if (!_idleChannel.TryWrite(connection)) + { + // The channel has been completed (pool is shutting down). Race window + // between the State check above and TryWrite: destroy instead of pooling. + RemoveConnection(connection); + } } } /// public void Shutdown() { - // No-op for now, warmup will be implemented later. + // FR-006: idempotent. Compare-and-exchange ensures only one caller performs shutdown work. + if (Interlocked.CompareExchange(ref _shutdownInitiated, 1, 0) != 0) + { + return; + } + + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}", Id); + + // FR-001: transition to ShuttingDown. After this point, ReturnInternalConnection + // routes returning connections to RemoveConnection. + State = ShuttingDown; + + // FR-002 + FR-007: complete the channel writer so: + // - no further idle connections can be enqueued (TryWrite returns false), and + // - in-flight / future async waiters on ReadAsync fault with ChannelClosedException. + _idleChannel.Complete(); + + // FR-003: drain remaining buffered idle connections and destroy them. The channel is + // unbounded so all already-enqueued items can be drained synchronously. + while (_idleChannel.TryRead(out DbConnectionInternal? connection)) + { + if (connection is not null) + { + RemoveConnection(connection); + } + // null sentinels are wake-up signals only; nothing to destroy. + } } /// public void Startup() { - // No-op for now, warmup will be implemented later. + // This pool has no background timers today (idle timeout is enforced lazily in + // IsLiveConnection on retrieval; pruning is not implemented). State is set to Running + // in the constructor, so this is currently the symmetrical counterpart of Shutdown. + // Background work (warmup, pruning timers) will be added here when introduced. + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}", Id); } /// diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IdleConnectionChannel.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IdleConnectionChannel.cs index 00748ab184..348ad33d9e 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IdleConnectionChannel.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IdleConnectionChannel.cs @@ -26,10 +26,19 @@ internal IdleConnectionChannel() { var channel = Channel.CreateUnbounded(); _reader = channel.Reader; - //TODO: the channel should be completed on pool shutdown _writer = channel.Writer; } + /// + /// Marks the channel writer as complete. After completion, + /// returns for any future writes, and any in-flight or future + /// waiters will fault with + /// once the channel is drained. Used by the connection pool to signal shutdown. + /// + /// if this call completed the channel; otherwise + /// (channel was already completed). + internal bool Complete() => _writer.TryComplete(); + /// /// The number of non-null connections currently in the channel. /// 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 4243e777ba..e2ae433ce4 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 @@ -329,6 +329,14 @@ public bool IsRunning private void CleanupCallback(object state) { + // FR-005: if the pool is shutting down, skip work. Shutdown disposes the timer, but + // a callback may already be in-flight when Shutdown runs; this guard ensures it does + // not perform pruning or re-arm pool create requests. + if (State == ShuttingDown) + { + return; + } + // Called when the cleanup-timer ticks over. // This is the automatic pruning method. Every period, we will @@ -767,6 +775,13 @@ private void DestroyObject(DbConnectionInternal obj) private void ErrorCallback(object state) { + // FR-005: skip work if the pool is shutting down. The shutdown path disposes the + // timer; this guard handles the in-flight-callback race. + if (State == ShuttingDown) + { + return; + } + SqlClientEventSource.Log.TryPoolerTraceEvent(" {0}, Resetting Error handling.", Id); _errorOccurred = false; _waitHandles.ErrorEvent.Reset(); @@ -956,6 +971,16 @@ private bool TryGetConnection(DbConnection owningObject, uint waitForMultipleObj { waitResult = WaitHandle.WaitAny(_waitHandles.GetHandles(allowCreate), unchecked((int)waitForMultipleObjectsTimeout)); + // FR-007: after waking, observe shutdown state and bail out so waiters + // do not spin against a drained pool. + if (State != Running) + { + SqlClientEventSource.Log.TryPoolerTraceEvent(" {0}, Pool is shutting down; abandoning wait.", Id); + Interlocked.Decrement(ref _waitCount); + connection = null; + return false; + } + // 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 @@ -1481,14 +1506,45 @@ public void Startup() public void Shutdown() { SqlClientEventSource.Log.TryPoolerTraceEvent(" {0}", Id); + + // FR-006: idempotent. Subsequent calls observe ShuttingDown and bail. + if (State == ShuttingDown) + { + return; + } State = ShuttingDown; - // deactivate timer callbacks - Timer t = _cleanupTimer; - _cleanupTimer = null; - if (t != null) + // FR-005: 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). + Timer cleanup = Interlocked.Exchange(ref _cleanupTimer, null); + cleanup?.Dispose(); + Timer error = Interlocked.Exchange(ref _errorTimer, null); + error?.Dispose(); + + // FR-007: wake any threads parked in WaitHandle.WaitAny by releasing the pool semaphore + // up to its max count. Waiters check State == Running after wake-up and bail. + // We may release more than necessary; SemaphoreFullException is harmless when the + // semaphore is already saturated. + try + { + _waitHandles.PoolSemaphore.Release(MaxPoolSize); + } + catch (SemaphoreFullException) + { + // Already at max count - all slots free. Nothing to do. + } + + // FR-003: drain idle stacks and destroy contained connections. Active checked-out + // connections are destroyed when they are returned (see DeactivateObject's + // State is ShuttingDown branch). + while (_stackNew.TryPop(out DbConnectionInternal newObj)) + { + DestroyObject(newObj); + } + while (_stackOld.TryPop(out DbConnectionInternal oldObj)) { - t.Dispose(); + DestroyObject(oldObj); } } diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolShutdownTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolShutdownTest.cs new file mode 100644 index 0000000000..ba5023214b --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolShutdownTest.cs @@ -0,0 +1,190 @@ +// 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.Collections.Generic; +using System.Data.Common; +using System.Threading.Tasks; +using Microsoft.Data.Common.ConnectionString; +using Microsoft.Data.ProviderBase; +using Microsoft.Data.SqlClient.ConnectionPool; +using Xunit; + +namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool +{ + /// + /// Deterministic tests for shutdown behavior. + /// Verifies the contract defined by spec 004-pool-shutdown FR-001 .. FR-007 (P1 scope). + /// + public class ChannelDbConnectionPoolShutdownTest + { + private static ChannelDbConnectionPool ConstructPool(int maxPoolSize = 5) + { + var poolGroupOptions = new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: 0, + maxPoolSize: maxPoolSize, + creationTimeout: 15, + loadBalanceTimeout: 0, + hasTransactionAffinity: true); + + var dbConnectionPoolGroup = new DbConnectionPoolGroup( + new SqlConnectionOptions("Data Source=localhost;"), + new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), + poolGroupOptions); + + return new ChannelDbConnectionPool( + new ChannelDbConnectionPoolTest.SuccessfulSqlConnectionFactory(), + dbConnectionPoolGroup, + DbConnectionPoolIdentity.NoIdentity, + new DbConnectionPoolProviderInfo()); + } + + // FR-001 + [Fact] + public void Shutdown_TransitionsState_ToShuttingDown() + { + var pool = ConstructPool(); + Assert.True(pool.IsRunning); + Assert.Equal(DbConnectionPoolState.Running, pool.State); + + pool.Shutdown(); + + Assert.False(pool.IsRunning); + Assert.Equal(DbConnectionPoolState.ShuttingDown, pool.State); + } + + // FR-003 + [Fact] + public void Shutdown_DrainsIdleConnections() + { + var pool = ConstructPool(); + + // Vend and return three connections so they sit idle in the channel. + var owners = new List(); + var conns = new List(); + for (int i = 0; i < 3; i++) + { + var owner = new SqlConnection(); + owners.Add(owner); + Assert.True(pool.TryGetConnection(owner, taskCompletionSource: null, out DbConnectionInternal? c)); + Assert.NotNull(c); + conns.Add(c!); + } + for (int i = 0; i < conns.Count; i++) + { + pool.ReturnInternalConnection(conns[i], owners[i]); + } + Assert.Equal(3, pool.IdleCount); + + pool.Shutdown(); + + Assert.Equal(0, pool.IdleCount); + Assert.Equal(0, pool.Count); + } + + // FR-004 - returned connection while shutting down is destroyed, not pooled. + [Fact] + public void Shutdown_ReturnedConnection_IsDestroyedNotPooled() + { + var pool = ConstructPool(); + var owner = new SqlConnection(); + Assert.True(pool.TryGetConnection(owner, taskCompletionSource: null, out DbConnectionInternal? conn)); + Assert.NotNull(conn); + Assert.Equal(1, pool.Count); + Assert.Equal(0, pool.IdleCount); + + pool.Shutdown(); + + // Connection is still checked out; return it now. + pool.ReturnInternalConnection(conn!, owner); + + Assert.Equal(0, pool.IdleCount); + Assert.Equal(0, pool.Count); + } + + // FR-006 + [Fact] + public void Shutdown_IsIdempotent() + { + var pool = ConstructPool(); + pool.Shutdown(); + // Second call must not throw and must leave state intact. + pool.Shutdown(); + pool.Shutdown(); + Assert.Equal(DbConnectionPoolState.ShuttingDown, pool.State); + } + + // FR-007 - async waiter is unblocked when the pool shuts down. + [Fact] + public async Task Shutdown_UnblocksAsyncWaiter() + { + var pool = ConstructPool(maxPoolSize: 1); + + // Saturate the pool. + Assert.True(pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, out DbConnectionInternal? blocking)); + Assert.NotNull(blocking); + + // Park an async waiter. + var tcs = new TaskCompletionSource(); + bool completed = pool.TryGetConnection(new SqlConnection(), tcs, out DbConnectionInternal? waiter); + Assert.False(completed); + Assert.Null(waiter); + Assert.False(tcs.Task.IsCompleted); + + // Shut down the pool. + pool.Shutdown(); + + // Waiter must complete (faulted or with a null connection) within a bounded window. + var winner = await Task.WhenAny(tcs.Task, Task.Delay(TimeSpan.FromSeconds(5))); + Assert.Same(tcs.Task, winner); + // Either an exception was set (channel closed) or the result is null - both are acceptable + // shutdown signals. What matters is the waiter does NOT block forever. + if (tcs.Task.IsFaulted) + { + // Expected path: ChannelClosedException or a wrapped exception. + Assert.NotNull(tcs.Task.Exception); + } + else + { + // Permitted: completed with null result. + Assert.Null(tcs.Task.Result); + } + } + + // Story 3 acceptance #3 - sync get fails fast after shutdown. + // The factory-level retry guard checks IsRunning, but the pool itself must not vend + // new connections after Shutdown. We verify by exhausting the pool first then + // checking that returned connections are destroyed (Count goes back to 0). + [Fact] + public void Shutdown_AfterShutdown_NewReturnsAreDestroyed() + { + var pool = ConstructPool(); + var owner = new SqlConnection(); + Assert.True(pool.TryGetConnection(owner, taskCompletionSource: null, out DbConnectionInternal? c)); + Assert.NotNull(c); + + pool.Shutdown(); + pool.ReturnInternalConnection(c!, owner); + + Assert.False(pool.IsRunning); + Assert.Equal(0, pool.Count); + Assert.Equal(0, pool.IdleCount); + } + + // Sanity: Startup is currently a no-op for this pool but must not throw or change + // shutdown state if invoked after Shutdown. + [Fact] + public void Startup_AfterShutdown_DoesNotResurrectPool() + { + var pool = ConstructPool(); + pool.Shutdown(); + + pool.Startup(); + + Assert.Equal(DbConnectionPoolState.ShuttingDown, pool.State); + Assert.False(pool.IsRunning); + } + } +} diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs new file mode 100644 index 0000000000..a7d6bd877b --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs @@ -0,0 +1,232 @@ +// 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.Reflection; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Data.Common.ConnectionString; +using Microsoft.Data.ProviderBase; +using Microsoft.Data.SqlClient.ConnectionPool; +using Xunit; + +namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool +{ + /// + /// Deterministic tests for shutdown behavior + /// per spec 004-pool-shutdown FR-001 .. FR-007 (P1 scope). + /// + public class WaitHandleDbConnectionPoolShutdownTest + { + private static WaitHandleDbConnectionPool CreatePool(int maxPoolSize = 5) + { + var poolGroupOptions = new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: 0, + maxPoolSize: maxPoolSize, + creationTimeout: 15000, + loadBalanceTimeout: 0, + hasTransactionAffinity: true); + + var dbConnectionPoolGroup = new DbConnectionPoolGroup( + new SqlConnectionOptions("Data Source=localhost;"), + new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), + poolGroupOptions); + + var pool = new WaitHandleDbConnectionPool( + new WaitHandleDbConnectionPoolTransactionTest.MockSqlConnectionFactory(), + dbConnectionPoolGroup, + DbConnectionPoolIdentity.NoIdentity, + new DbConnectionPoolProviderInfo()); + pool.Startup(); + return pool; + } + + private static T GetPrivateField(object instance, string fieldName) + { + var field = instance.GetType().GetField(fieldName, BindingFlags.NonPublic | BindingFlags.Instance); + Assert.NotNull(field); + return (T)field!.GetValue(instance)!; + } + + // FR-001 + [Fact] + public void Shutdown_TransitionsState_ToShuttingDown() + { + var pool = CreatePool(); + Assert.True(pool.IsRunning); + + pool.Shutdown(); + + Assert.False(pool.IsRunning); + Assert.Equal(DbConnectionPoolState.ShuttingDown, pool.State); + } + + // FR-005 - cleanup timer is disposed. + [Fact] + public void Shutdown_DisposesCleanupTimer() + { + var pool = CreatePool(); + var beforeTimer = GetPrivateField(pool, "_cleanupTimer"); + Assert.NotNull(beforeTimer); + + pool.Shutdown(); + + var afterTimer = GetPrivateField(pool, "_cleanupTimer"); + Assert.Null(afterTimer); + } + + // FR-005 - error timer is disposed when present. + [Fact] + public void Shutdown_DisposesErrorTimer_WhenPresent() + { + var pool = CreatePool(); + // Inject a real Timer into _errorTimer to mimic an error-state pool. + var injected = new Timer(_ => { }, null, Timeout.Infinite, Timeout.Infinite); + var field = pool.GetType().GetField("_errorTimer", BindingFlags.NonPublic | BindingFlags.Instance); + Assert.NotNull(field); + field!.SetValue(pool, injected); + + pool.Shutdown(); + + var afterTimer = GetPrivateField(pool, "_errorTimer"); + Assert.Null(afterTimer); + } + + // FR-003 - drains idle stacks. + [Fact] + public void Shutdown_DrainsIdleStacks() + { + var pool = CreatePool(); + + // Vend a few connections then return them so they sit in _stackNew. + var owner1 = new SqlConnection(); + var owner2 = new SqlConnection(); + pool.TryGetConnection(owner1, taskCompletionSource: null, out DbConnectionInternal? c1); + pool.TryGetConnection(owner2, taskCompletionSource: null, out DbConnectionInternal? c2); + Assert.NotNull(c1); + Assert.NotNull(c2); + pool.ReturnInternalConnection(c1!, owner1); + pool.ReturnInternalConnection(c2!, owner2); + + Assert.Equal(2, pool.IdleCount); + Assert.Equal(2, pool.Count); + + pool.Shutdown(); + + Assert.Equal(0, pool.IdleCount); + Assert.Equal(0, pool.Count); + } + + // FR-006 + [Fact] + public void Shutdown_IsIdempotent() + { + var pool = CreatePool(); + pool.Shutdown(); + pool.Shutdown(); + pool.Shutdown(); + Assert.Equal(DbConnectionPoolState.ShuttingDown, pool.State); + } + + // FR-005 acceptance #3 - cleanup-callback after shutdown is a no-op. + [Fact] + public void CleanupCallback_AfterShutdown_IsNoOp() + { + var pool = CreatePool(); + pool.Shutdown(); + + // Invoke the private callback directly via reflection. Must not throw and must + // not re-arm any pool create requests. + var method = pool.GetType().GetMethod("CleanupCallback", BindingFlags.NonPublic | BindingFlags.Instance); + Assert.NotNull(method); + + var ex = Record.Exception(() => method!.Invoke(pool, new object?[] { null })); + Assert.Null(ex); + Assert.Equal(DbConnectionPoolState.ShuttingDown, pool.State); + } + + // FR-005 acceptance #3 - error-callback after shutdown is a no-op. + [Fact] + public void ErrorCallback_AfterShutdown_IsNoOp() + { + var pool = CreatePool(); + pool.Shutdown(); + + var method = pool.GetType().GetMethod("ErrorCallback", BindingFlags.NonPublic | BindingFlags.Instance); + Assert.NotNull(method); + + var ex = Record.Exception(() => method!.Invoke(pool, new object?[] { null })); + Assert.Null(ex); + } + + // FR-007 - sync caller arriving after shutdown gets a null connection (factory will + // see this and return up the retry chain). The pool's TryGetConnection short-circuits + // on State != Running. + [Fact] + public void TryGetConnection_AfterShutdown_ReturnsNullWithoutBlocking() + { + var pool = CreatePool(); + pool.Shutdown(); + + bool completed = pool.TryGetConnection( + new SqlConnection(), + taskCompletionSource: null, + out DbConnectionInternal? conn); + + // TryGetConnection returns true with a null connection when State != Running. + Assert.True(completed); + Assert.Null(conn); + } + + // FR-007 - shutdown wakes up a thread parked in WaitHandle.WaitAny. + [Fact] + public void Shutdown_UnblocksSyncWaiter() + { + var pool = CreatePool(maxPoolSize: 1); + + // Saturate the pool. + var owner = new SqlConnection(); + Assert.True(pool.TryGetConnection(owner, taskCompletionSource: null, 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(() => + { + try + { + waiterCompleted = pool.TryGetConnection( + new SqlConnection(), + taskCompletionSource: null, + out waiterResult); + } + catch (Exception ex) + { + waiterEx = ex; + } + }) + { IsBackground = true }; + t.Start(); + + // Give the waiter time to enter WaitAny. + Thread.Sleep(200); + Assert.True(t.IsAlive, "Waiter should be parked, but thread already exited."); + + pool.Shutdown(); + + 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; + } + } +} From b24d4fbb4635dc8f48991ff417ce609aeeceee96 Mon Sep 17 00:00:00 2001 From: Priyanka Tiwari Date: Fri, 29 May 2026 17:15:38 +0530 Subject: [PATCH 02/17] =?UTF-8?q?=EF=BB=BF=20=20Address=20PR=20#4302=20rev?= =?UTF-8?q?iew=20feedback=20and=20drop=20spec=20FR-00x=20labels?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code review fixes (WaitHandleDbConnectionPool): - TryGetConnection shutdown short-circuit now releases the PoolSemaphore slot when WaitAny was woken by SEMAPHORE_HANDLE (or WAIT_ABANDONED+SEMAPHORE_HANDLE), preventing a semaphore-slot leak that would starve future waiters. - Shutdown() releases Volatile.Read(ref _waitCount) slots instead of MaxPoolSize, fixing ArgumentOutOfRangeException when MaxPoolSize == 0 (unlimited pool) and ensuring every parked waiter is woken under high contention. Kept SemaphoreFullException guard. Test fix: - Shutdown_UnblocksSyncWaiter replaces Thread.Sleep(200) with a bounded poll on the private _waitCount field (via the existing reflection helper) so the test is deterministic on slow CI agents. Documentation cleanup: - Removed inline 'FR-00x' spec-traceability labels from comments and test section banners across both pool implementations and both shutdown test files. Explanatory text is preserved; the spec coverage will live in the PR description instead of the source. --- .../ConnectionPool/ChannelDbConnectionPool.cs | 4 +- .../WaitHandleDbConnectionPool.cs | 54 +++++++++++++------ .../ChannelDbConnectionPoolShutdownTest.cs | 13 +++-- .../WaitHandleDbConnectionPoolShutdownTest.cs | 32 ++++++----- 4 files changed, 64 insertions(+), 39 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index a20829a332..8e74595304 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -95,7 +95,7 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool /// /// Tracks whether has already initiated the shutdown sequence so that - /// repeated calls are observed as no-ops (FR-006). Updated atomically via + /// repeated calls are observed as no-ops. Updated atomically via /// . /// private int _shutdownInitiated; @@ -273,7 +273,7 @@ public void ReturnInternalConnection(DbConnectionInternal connection, DbConnecti /// public void Shutdown() { - // FR-006: idempotent. Compare-and-exchange ensures only one caller performs shutdown work. + // idempotent. Compare-and-exchange ensures only one caller performs shutdown work. if (Interlocked.CompareExchange(ref _shutdownInitiated, 1, 0) != 0) { return; 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 e2ae433ce4..0d6bf40911 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 @@ -329,7 +329,7 @@ public bool IsRunning private void CleanupCallback(object state) { - // FR-005: if the pool is shutting down, skip work. Shutdown disposes the timer, but + // If the pool is shutting down, skip work. Shutdown disposes the timer, but // a callback may already be in-flight when Shutdown runs; this guard ensures it does // not perform pruning or re-arm pool create requests. if (State == ShuttingDown) @@ -775,7 +775,7 @@ private void DestroyObject(DbConnectionInternal obj) private void ErrorCallback(object state) { - // FR-005: skip work if the pool is shutting down. The shutdown path disposes the + // Skip work if the pool is shutting down. The shutdown path disposes the // timer; this guard handles the in-flight-callback race. if (State == ShuttingDown) { @@ -971,11 +971,26 @@ private bool TryGetConnection(DbConnection owningObject, uint waitForMultipleObj { waitResult = WaitHandle.WaitAny(_waitHandles.GetHandles(allowCreate), unchecked((int)waitForMultipleObjectsTimeout)); - // FR-007: after waking, observe shutdown state and bail out so waiters - // do not spin against a drained pool. + // After waking, observe shutdown state and bail out so waiters + // do not spin against a drained pool. If WaitAny consumed a + // PoolSemaphore slot, release it back so the accounting stays + // balanced; otherwise the slot would leak and other waiters + // (or callers that arrive after Shutdown completes its own + // Release loop) would starve. if (State != Running) { SqlClientEventSource.Log.TryPoolerTraceEvent(" {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 false; @@ -1507,14 +1522,14 @@ public void Shutdown() { SqlClientEventSource.Log.TryPoolerTraceEvent(" {0}", Id); - // FR-006: idempotent. Subsequent calls observe ShuttingDown and bail. + // Idempotent: subsequent calls observe ShuttingDown and bail. if (State == ShuttingDown) { return; } State = ShuttingDown; - // FR-005: dispose all background timers so they no longer schedule new work. + // 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). Timer cleanup = Interlocked.Exchange(ref _cleanupTimer, null); @@ -1522,20 +1537,25 @@ public void Shutdown() Timer error = Interlocked.Exchange(ref _errorTimer, null); error?.Dispose(); - // FR-007: wake any threads parked in WaitHandle.WaitAny by releasing the pool semaphore - // up to its max count. Waiters check State == Running after wake-up and bail. - // We may release more than necessary; SemaphoreFullException is harmless when the - // semaphore is already saturated. - try + // 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 != Running after wake-up and bail. + int waitersToWake = Volatile.Read(ref _waitCount); + if (waitersToWake > 0) { - _waitHandles.PoolSemaphore.Release(MaxPoolSize); - } - catch (SemaphoreFullException) - { - // Already at max count - all slots free. Nothing to do. + try + { + _waitHandles.PoolSemaphore.Release(waitersToWake); + } + catch (SemaphoreFullException) + { + // Semaphore already saturated; nothing to do. + } } - // FR-003: drain idle stacks and destroy contained connections. Active checked-out + // Drain idle stacks and destroy contained connections. Active checked-out // connections are destroyed when they are returned (see DeactivateObject's // State is ShuttingDown branch). while (_stackNew.TryPop(out DbConnectionInternal newObj)) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolShutdownTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolShutdownTest.cs index ba5023214b..ae06411a50 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolShutdownTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolShutdownTest.cs @@ -15,7 +15,6 @@ namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool { /// /// Deterministic tests for shutdown behavior. - /// Verifies the contract defined by spec 004-pool-shutdown FR-001 .. FR-007 (P1 scope). /// public class ChannelDbConnectionPoolShutdownTest { @@ -41,7 +40,7 @@ private static ChannelDbConnectionPool ConstructPool(int maxPoolSize = 5) new DbConnectionPoolProviderInfo()); } - // FR-001 + // State transitions to ShuttingDown on Shutdown. [Fact] public void Shutdown_TransitionsState_ToShuttingDown() { @@ -55,7 +54,7 @@ public void Shutdown_TransitionsState_ToShuttingDown() Assert.Equal(DbConnectionPoolState.ShuttingDown, pool.State); } - // FR-003 + // Drains buffered idle connections. [Fact] public void Shutdown_DrainsIdleConnections() { @@ -84,7 +83,7 @@ public void Shutdown_DrainsIdleConnections() Assert.Equal(0, pool.Count); } - // FR-004 - returned connection while shutting down is destroyed, not pooled. + // Returned connection while shutting down is destroyed, not pooled. [Fact] public void Shutdown_ReturnedConnection_IsDestroyedNotPooled() { @@ -104,7 +103,7 @@ public void Shutdown_ReturnedConnection_IsDestroyedNotPooled() Assert.Equal(0, pool.Count); } - // FR-006 + // Shutdown is idempotent. [Fact] public void Shutdown_IsIdempotent() { @@ -116,7 +115,7 @@ public void Shutdown_IsIdempotent() Assert.Equal(DbConnectionPoolState.ShuttingDown, pool.State); } - // FR-007 - async waiter is unblocked when the pool shuts down. + // Async waiter is unblocked when the pool shuts down. [Fact] public async Task Shutdown_UnblocksAsyncWaiter() { @@ -153,7 +152,7 @@ public async Task Shutdown_UnblocksAsyncWaiter() } } - // Story 3 acceptance #3 - sync get fails fast after shutdown. + // Sync get fails fast after shutdown. // The factory-level retry guard checks IsRunning, but the pool itself must not vend // new connections after Shutdown. We verify by exhausting the pool first then // checking that returned connections are destroyed (Count goes back to 0). diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs index a7d6bd877b..6a4564c4e8 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs @@ -14,8 +14,7 @@ namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool { /// - /// Deterministic tests for shutdown behavior - /// per spec 004-pool-shutdown FR-001 .. FR-007 (P1 scope). + /// Deterministic tests for shutdown behavior. /// public class WaitHandleDbConnectionPoolShutdownTest { @@ -50,7 +49,7 @@ private static T GetPrivateField(object instance, string fieldName) return (T)field!.GetValue(instance)!; } - // FR-001 + // State transitions to ShuttingDown on Shutdown. [Fact] public void Shutdown_TransitionsState_ToShuttingDown() { @@ -63,7 +62,7 @@ public void Shutdown_TransitionsState_ToShuttingDown() Assert.Equal(DbConnectionPoolState.ShuttingDown, pool.State); } - // FR-005 - cleanup timer is disposed. + // Cleanup timer is disposed. [Fact] public void Shutdown_DisposesCleanupTimer() { @@ -77,7 +76,7 @@ public void Shutdown_DisposesCleanupTimer() Assert.Null(afterTimer); } - // FR-005 - error timer is disposed when present. + // Error timer is disposed when present. [Fact] public void Shutdown_DisposesErrorTimer_WhenPresent() { @@ -94,7 +93,7 @@ public void Shutdown_DisposesErrorTimer_WhenPresent() Assert.Null(afterTimer); } - // FR-003 - drains idle stacks. + // Drains idle stacks. [Fact] public void Shutdown_DrainsIdleStacks() { @@ -119,7 +118,7 @@ public void Shutdown_DrainsIdleStacks() Assert.Equal(0, pool.Count); } - // FR-006 + // Shutdown is idempotent. [Fact] public void Shutdown_IsIdempotent() { @@ -130,7 +129,7 @@ public void Shutdown_IsIdempotent() Assert.Equal(DbConnectionPoolState.ShuttingDown, pool.State); } - // FR-005 acceptance #3 - cleanup-callback after shutdown is a no-op. + // Cleanup callback after shutdown is a no-op. [Fact] public void CleanupCallback_AfterShutdown_IsNoOp() { @@ -147,7 +146,7 @@ public void CleanupCallback_AfterShutdown_IsNoOp() Assert.Equal(DbConnectionPoolState.ShuttingDown, pool.State); } - // FR-005 acceptance #3 - error-callback after shutdown is a no-op. + // Error callback after shutdown is a no-op. [Fact] public void ErrorCallback_AfterShutdown_IsNoOp() { @@ -161,7 +160,7 @@ public void ErrorCallback_AfterShutdown_IsNoOp() Assert.Null(ex); } - // FR-007 - sync caller arriving after shutdown gets a null connection (factory will + // Sync caller arriving after shutdown gets a null connection (factory will // see this and return up the retry chain). The pool's TryGetConnection short-circuits // on State != Running. [Fact] @@ -180,7 +179,7 @@ public void TryGetConnection_AfterShutdown_ReturnsNullWithoutBlocking() Assert.Null(conn); } - // FR-007 - shutdown wakes up a thread parked in WaitHandle.WaitAny. + // Shutdown wakes up a thread parked in WaitHandle.WaitAny. [Fact] public void Shutdown_UnblocksSyncWaiter() { @@ -213,8 +212,15 @@ public void Shutdown_UnblocksSyncWaiter() { IsBackground = true }; t.Start(); - // Give the waiter time to enter WaitAny. - Thread.Sleep(200); + // 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. + var deadline = DateTime.UtcNow.AddSeconds(5); + while (DateTime.UtcNow < deadline && GetPrivateField(pool, "_waitCount") < 1) + { + Thread.Yield(); + } + Assert.True(GetPrivateField(pool, "_waitCount") >= 1, "Waiter did not park within 5s."); Assert.True(t.IsAlive, "Waiter should be parked, but thread already exited."); pool.Shutdown(); From 7ba73a825516b08e1795b84e6a031227da7fbead Mon Sep 17 00:00:00 2001 From: Priyanka Tiwari Date: Fri, 5 Jun 2026 17:39:30 +0530 Subject: [PATCH 03/17] Address PR #4302 review: avoid reflection in WaitHandleDbConnectionPool shutdown tests Replace reflection-based access of WaitHandleDbConnectionPool internals from the shutdown unit tests with direct field/method access. The pool's _waitCount, _errorTimer, _cleanupTimer, CleanupCallback, and ErrorCallback are now declared internal so the UnitTests assembly (covered by InternalsVisibleTo) can reach them without GetField/Invoke. The GetPrivateField helper is removed. Per @mdaigle review feedback on dotnet/SqlClient#4302. --- .../WaitHandleDbConnectionPool.cs | 10 ++--- .../WaitHandleDbConnectionPoolShutdownTest.cs | 40 +++++-------------- 2 files changed, 15 insertions(+), 35 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 0d6bf40911..54efef5445 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 @@ -189,16 +189,16 @@ public void Dispose() private readonly WaitCallback _poolCreateRequest; - private int _waitCount; + internal int _waitCount; private readonly PoolWaitHandles _waitHandles; private Exception _resError; private volatile bool _errorOccurred; private int _errorWait; - private Timer _errorTimer; + internal Timer _errorTimer; - private Timer _cleanupTimer; + internal Timer _cleanupTimer; private readonly TransactedConnectionPool _transactedConnectionPool; @@ -327,7 +327,7 @@ public bool IsRunning public TransactedConnectionPool TransactedConnectionPool => _transactedConnectionPool; - private void CleanupCallback(object state) + internal void CleanupCallback(object state) { // If the pool is shutting down, skip work. Shutdown disposes the timer, but // a callback may already be in-flight when Shutdown runs; this guard ensures it does @@ -773,7 +773,7 @@ private void DestroyObject(DbConnectionInternal obj) } } - private void ErrorCallback(object state) + internal void ErrorCallback(object state) { // Skip work if the pool is shutting down. The shutdown path disposes the // timer; this guard handles the in-flight-callback race. diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs index 6a4564c4e8..34d200f277 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs @@ -3,7 +3,6 @@ // See the LICENSE file in the project root for more information. using System; -using System.Reflection; using System.Threading; using System.Threading.Tasks; using Microsoft.Data.Common.ConnectionString; @@ -42,13 +41,6 @@ private static WaitHandleDbConnectionPool CreatePool(int maxPoolSize = 5) return pool; } - private static T GetPrivateField(object instance, string fieldName) - { - var field = instance.GetType().GetField(fieldName, BindingFlags.NonPublic | BindingFlags.Instance); - Assert.NotNull(field); - return (T)field!.GetValue(instance)!; - } - // State transitions to ShuttingDown on Shutdown. [Fact] public void Shutdown_TransitionsState_ToShuttingDown() @@ -67,13 +59,11 @@ public void Shutdown_TransitionsState_ToShuttingDown() public void Shutdown_DisposesCleanupTimer() { var pool = CreatePool(); - var beforeTimer = GetPrivateField(pool, "_cleanupTimer"); - Assert.NotNull(beforeTimer); + Assert.NotNull(pool._cleanupTimer); pool.Shutdown(); - var afterTimer = GetPrivateField(pool, "_cleanupTimer"); - Assert.Null(afterTimer); + Assert.Null(pool._cleanupTimer); } // Error timer is disposed when present. @@ -82,15 +72,11 @@ public void Shutdown_DisposesErrorTimer_WhenPresent() { var pool = CreatePool(); // Inject a real Timer into _errorTimer to mimic an error-state pool. - var injected = new Timer(_ => { }, null, Timeout.Infinite, Timeout.Infinite); - var field = pool.GetType().GetField("_errorTimer", BindingFlags.NonPublic | BindingFlags.Instance); - Assert.NotNull(field); - field!.SetValue(pool, injected); + pool._errorTimer = new Timer(_ => { }, null, Timeout.Infinite, Timeout.Infinite); pool.Shutdown(); - var afterTimer = GetPrivateField(pool, "_errorTimer"); - Assert.Null(afterTimer); + Assert.Null(pool._errorTimer); } // Drains idle stacks. @@ -136,12 +122,9 @@ public void CleanupCallback_AfterShutdown_IsNoOp() var pool = CreatePool(); pool.Shutdown(); - // Invoke the private callback directly via reflection. Must not throw and must - // not re-arm any pool create requests. - var method = pool.GetType().GetMethod("CleanupCallback", BindingFlags.NonPublic | BindingFlags.Instance); - Assert.NotNull(method); - - var ex = Record.Exception(() => method!.Invoke(pool, new object?[] { null })); + // Invoke the callback directly. Must not throw and must not re-arm any pool + // create requests. + var ex = Record.Exception(() => pool.CleanupCallback(state: null)); Assert.Null(ex); Assert.Equal(DbConnectionPoolState.ShuttingDown, pool.State); } @@ -153,10 +136,7 @@ public void ErrorCallback_AfterShutdown_IsNoOp() var pool = CreatePool(); pool.Shutdown(); - var method = pool.GetType().GetMethod("ErrorCallback", BindingFlags.NonPublic | BindingFlags.Instance); - Assert.NotNull(method); - - var ex = Record.Exception(() => method!.Invoke(pool, new object?[] { null })); + var ex = Record.Exception(() => pool.ErrorCallback(state: null)); Assert.Null(ex); } @@ -216,11 +196,11 @@ public void Shutdown_UnblocksSyncWaiter() // happens immediately before it enters WaitHandle.WaitAny. Polling avoids the // CI-flakiness of a fixed Thread.Sleep on slow agents. var deadline = DateTime.UtcNow.AddSeconds(5); - while (DateTime.UtcNow < deadline && GetPrivateField(pool, "_waitCount") < 1) + while (DateTime.UtcNow < deadline && pool._waitCount < 1) { Thread.Yield(); } - Assert.True(GetPrivateField(pool, "_waitCount") >= 1, "Waiter did not park within 5s."); + Assert.True(pool._waitCount >= 1, "Waiter did not park within 5s."); Assert.True(t.IsAlive, "Waiter should be parked, but thread already exited."); pool.Shutdown(); From b8d56d9fbb428289e9f2f8d56ddb1f426f9f2da7 Mon Sep 17 00:00:00 2001 From: Priyanka Tiwari Date: Fri, 5 Jun 2026 17:40:54 +0530 Subject: [PATCH 04/17] Address PR #4302 review: reuse Clear() from Shutdown() in both pools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both pool implementations now delegate the connection drain in Shutdown() to the existing Clear() routine instead of duplicating the drain inline. ChannelDbConnectionPool.Shutdown(): keeps the CompareExchange election, State transition and _idleChannel.Complete() call, then calls Clear() (which bumps _clearGeneration and best-effort drains the idle channel). A final unbounded drain loop is retained as a mop-up because Clear() may short-circuit when another Clear() is concurrently in flight; the final loop is safe because the channel is already completed and no new writes can succeed. WaitHandleDbConnectionPool.Shutdown(): keeps the trace, idempotent State check, timer disposal and waiter wake-up, then replaces the inline _stackNew / _stackOld drain loops with a single Clear() call. Clear() additionally dooms every entry in _objectList, decrements the ExitFreeConnection metric per drained connection, and runs ReclaimEmancipatedObjects() — strictly more cleanup than the previous inline drain, with no behaviour change for normal Running-state operations. Per @mdaigle review feedback on dotnet/SqlClient#4302. --- .../ConnectionPool/ChannelDbConnectionPool.cs | 14 ++++++++++---- .../ConnectionPool/WaitHandleDbConnectionPool.cs | 16 +++++----------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 8e74595304..056d18d8b5 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -282,17 +282,23 @@ public void Shutdown() SqlClientEventSource.Log.TryPoolerTraceEvent( " {0}", Id); - // FR-001: transition to ShuttingDown. After this point, ReturnInternalConnection + // Transition to ShuttingDown. After this point, ReturnInternalConnection // routes returning connections to RemoveConnection. State = ShuttingDown; - // FR-002 + FR-007: complete the channel writer so: + // Complete the channel writer so: // - no further idle connections can be enqueued (TryWrite returns false), and // - in-flight / future async waiters on ReadAsync fault with ChannelClosedException. _idleChannel.Complete(); - // FR-003: drain remaining buffered idle connections and destroy them. The channel is - // unbounded so all already-enqueued items can be drained synchronously. + // Reuse Clear() for the drain. Clear bumps _clearGeneration so any active + // checked-out connection fails IsLiveConnection on return and is removed, and it + // drains the idle channel up to its captured IdleCount. + Clear(); + + // Clear() may short-circuit if another caller is already draining. Because the + // channel is now completed, no new items can be enqueued, so it is safe to do a + // final unbounded drain to mop up anything Clear() may have skipped. while (_idleChannel.TryRead(out DbConnectionInternal? connection)) { if (connection is not null) 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 54efef5445..a07f4017e2 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 @@ -1555,17 +1555,11 @@ public void Shutdown() } } - // Drain idle stacks and destroy contained connections. Active checked-out - // connections are destroyed when they are returned (see DeactivateObject's - // State is ShuttingDown branch). - while (_stackNew.TryPop(out DbConnectionInternal newObj)) - { - DestroyObject(newObj); - } - while (_stackOld.TryPop(out DbConnectionInternal oldObj)) - { - DestroyObject(oldObj); - } + // 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(); } // TransactionEnded merely provides the plumbing for DbConnectionInternal to access the transacted pool From 9a45a66f247446e547f24b5be57bbff977cfec72 Mon Sep 17 00:00:00 2001 From: Priyanka Tiwari Date: Fri, 5 Jun 2026 20:25:00 +0530 Subject: [PATCH 05/17] Fix PR #4302 CI: pass TimeoutTimer to ChannelDbConnectionPool.TryGetConnection in shutdown tests PR 4295 (merged to main) added a required TimeoutTimer parameter to ChannelDbConnectionPool.TryGetConnection. The shutdown tests added in this PR still call the pre-4295 3-arg overload, which compiles locally against PoolShutdown's older base but fails in the CI pipeline because the pipeline merges this PR's head into the post-4295 main. Pass TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)) at all 5 call sites to match the new signature. --- .../ChannelDbConnectionPoolShutdownTest.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolShutdownTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolShutdownTest.cs index ae06411a50..79b0321497 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolShutdownTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolShutdownTest.cs @@ -67,7 +67,7 @@ public void Shutdown_DrainsIdleConnections() { var owner = new SqlConnection(); owners.Add(owner); - Assert.True(pool.TryGetConnection(owner, taskCompletionSource: null, out DbConnectionInternal? c)); + Assert.True(pool.TryGetConnection(owner, taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? c)); Assert.NotNull(c); conns.Add(c!); } @@ -89,7 +89,7 @@ public void Shutdown_ReturnedConnection_IsDestroyedNotPooled() { var pool = ConstructPool(); var owner = new SqlConnection(); - Assert.True(pool.TryGetConnection(owner, taskCompletionSource: null, out DbConnectionInternal? conn)); + Assert.True(pool.TryGetConnection(owner, taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? conn)); Assert.NotNull(conn); Assert.Equal(1, pool.Count); Assert.Equal(0, pool.IdleCount); @@ -122,12 +122,12 @@ public async Task Shutdown_UnblocksAsyncWaiter() var pool = ConstructPool(maxPoolSize: 1); // Saturate the pool. - Assert.True(pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, out DbConnectionInternal? blocking)); + Assert.True(pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? blocking)); Assert.NotNull(blocking); // Park an async waiter. var tcs = new TaskCompletionSource(); - bool completed = pool.TryGetConnection(new SqlConnection(), tcs, out DbConnectionInternal? waiter); + bool completed = pool.TryGetConnection(new SqlConnection(), tcs, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? waiter); Assert.False(completed); Assert.Null(waiter); Assert.False(tcs.Task.IsCompleted); @@ -161,7 +161,7 @@ public void Shutdown_AfterShutdown_NewReturnsAreDestroyed() { var pool = ConstructPool(); var owner = new SqlConnection(); - Assert.True(pool.TryGetConnection(owner, taskCompletionSource: null, out DbConnectionInternal? c)); + Assert.True(pool.TryGetConnection(owner, taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? c)); Assert.NotNull(c); pool.Shutdown(); From 954679dfde157f0780d9a08e3029689c864f069e Mon Sep 17 00:00:00 2001 From: Priyanka Tiwari Date: Fri, 5 Jun 2026 20:54:17 +0530 Subject: [PATCH 06/17] Pass TimeoutTimer to WaitHandleDbConnectionPool.TryGetConnection in shutdown tests The TryGetConnection method on main now requires a TimeoutTimer argument. Update all 5 call sites in WaitHandleDbConnectionPoolShutdownTest.cs to pass TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)) so the test project compiles against the current 4-arg signature. --- .../WaitHandleDbConnectionPoolShutdownTest.cs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs index 34d200f277..7b75dc1e97 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs @@ -88,8 +88,8 @@ public void Shutdown_DrainsIdleStacks() // Vend a few connections then return them so they sit in _stackNew. var owner1 = new SqlConnection(); var owner2 = new SqlConnection(); - pool.TryGetConnection(owner1, taskCompletionSource: null, out DbConnectionInternal? c1); - pool.TryGetConnection(owner2, taskCompletionSource: null, out DbConnectionInternal? c2); + pool.TryGetConnection(owner1, taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? c1); + pool.TryGetConnection(owner2, taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? c2); Assert.NotNull(c1); Assert.NotNull(c2); pool.ReturnInternalConnection(c1!, owner1); @@ -152,6 +152,7 @@ public void TryGetConnection_AfterShutdown_ReturnsNullWithoutBlocking() bool completed = pool.TryGetConnection( new SqlConnection(), taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? conn); // TryGetConnection returns true with a null connection when State != Running. @@ -167,7 +168,7 @@ public void Shutdown_UnblocksSyncWaiter() // Saturate the pool. var owner = new SqlConnection(); - Assert.True(pool.TryGetConnection(owner, taskCompletionSource: null, out DbConnectionInternal? blocking)); + Assert.True(pool.TryGetConnection(owner, taskCompletionSource: 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. @@ -182,6 +183,7 @@ public void Shutdown_UnblocksSyncWaiter() waiterCompleted = pool.TryGetConnection( new SqlConnection(), taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out waiterResult); } catch (Exception ex) From 6dc67ce0940fee316fca3b151f783ea708dd56a6 Mon Sep 17 00:00:00 2001 From: Priyanka Tiwari Date: Thu, 11 Jun 2026 14:15:50 +0530 Subject: [PATCH 07/17] Address PR #4302 Copilot review: drop unused usings and use Volatile.Read for _waitCount - WaitHandleDbConnectionPoolShutdownTest.cs: remove unused 'using System.Threading.Tasks;' and read pool._waitCount via Volatile.Read in both the poll-loop condition and the assertion so the test no longer relies on plain int reads observing another thread's Interlocked.Increment.\n- ChannelDbConnectionPoolShutdownTest.cs: remove unused 'using System.Data.Common;'. --- .../ChannelDbConnectionPoolShutdownTest.cs | 1 - .../WaitHandleDbConnectionPoolShutdownTest.cs | 9 +++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolShutdownTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolShutdownTest.cs index 79b0321497..cf3c5da4c4 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolShutdownTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolShutdownTest.cs @@ -4,7 +4,6 @@ using System; using System.Collections.Generic; -using System.Data.Common; using System.Threading.Tasks; using Microsoft.Data.Common.ConnectionString; using Microsoft.Data.ProviderBase; diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs index 7b75dc1e97..032db01d7e 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs @@ -4,7 +4,6 @@ using System; using System.Threading; -using System.Threading.Tasks; using Microsoft.Data.Common.ConnectionString; using Microsoft.Data.ProviderBase; using Microsoft.Data.SqlClient.ConnectionPool; @@ -196,13 +195,15 @@ public void Shutdown_UnblocksSyncWaiter() // 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. + // 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 && pool._waitCount < 1) + while (DateTime.UtcNow < deadline && Volatile.Read(ref pool._waitCount) < 1) { Thread.Yield(); } - Assert.True(pool._waitCount >= 1, "Waiter did not park within 5s."); + 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(); From 21f3aa440a472e6fb1bf9f0af8bcd2a932bbba62 Mon Sep 17 00:00:00 2001 From: Priyanka Tiwari Date: Thu, 11 Jun 2026 14:18:56 +0530 Subject: [PATCH 08/17] Address PR #4302 Copilot review: ChannelDbConnectionPool.TryGetConnection short-circuits when State != Running Mirror WaitHandleDbConnectionPool.TryGetConnection: when the pool has been shut down (or is not yet Running), return (true, null) immediately instead of entering the channel-reader path. The previous behavior could reserve a connection slot via _connectionSlots.Add, open a fresh physical connection, hand it to the caller, and then immediately destroy it on return because ReturnInternalConnection checks State == ShuttingDown. The short-circuit avoids the unnecessary open/close round-trip for both sync and async paths. Adds two regression tests. --- .../ConnectionPool/ChannelDbConnectionPool.cs | 13 ++++++ .../ChannelDbConnectionPoolShutdownTest.cs | 43 +++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 38e5c45d8b..61606e9953 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -352,6 +352,19 @@ public bool TryGetConnection( TimeoutTimer timeout, out DbConnectionInternal? connection) { + // Short-circuit when the pool is not Running (i.e., shut down or never started). + // Returning (true, null) matches WaitHandleDbConnectionPool.TryGetConnection and tells + // the caller "completed; no connection available" without entering the channel path, + // which would otherwise reserve a slot, attempt to open a fresh physical connection, + // and then immediately destroy it on return because State == ShuttingDown. + if (State is not Running) + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, State != Running.", Id); + connection = null; + return true; + } + // If taskCompletionSource is null, we are in a sync context. if (taskCompletionSource is null) { diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolShutdownTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolShutdownTest.cs index cf3c5da4c4..2840ee346d 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolShutdownTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolShutdownTest.cs @@ -184,5 +184,48 @@ public void Startup_AfterShutdown_DoesNotResurrectPool() Assert.Equal(DbConnectionPoolState.ShuttingDown, pool.State); Assert.False(pool.IsRunning); } + + // After Shutdown, a synchronous TryGetConnection must short-circuit and return + // (true, null) rather than entering the channel path and opening a fresh physical + // connection that would be immediately destroyed on return. + [Fact] + public void TryGetConnection_AfterShutdown_Sync_ShortCircuits() + { + var pool = ConstructPool(); + pool.Shutdown(); + + int countBefore = pool.Count; + bool completed = pool.TryGetConnection( + new SqlConnection(), + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(5)), + out DbConnectionInternal? conn); + + Assert.True(completed, "Sync TryGetConnection on a shut-down pool should return true to signal completion."); + Assert.Null(conn); + Assert.Equal(countBefore, pool.Count); + } + + // Same contract for the async (TaskCompletionSource) path: a TryGetConnection call + // issued after Shutdown must short-circuit without opening a new connection. + [Fact] + public void TryGetConnection_AfterShutdown_Async_ShortCircuits() + { + var pool = ConstructPool(); + pool.Shutdown(); + + int countBefore = pool.Count; + var tcs = new TaskCompletionSource(); + bool completed = pool.TryGetConnection( + new SqlConnection(), + tcs, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(5)), + out DbConnectionInternal? conn); + + // Match WaitHandleDbConnectionPool: short-circuit returns (true, null) regardless of TCS. + Assert.True(completed); + Assert.Null(conn); + Assert.Equal(countBefore, pool.Count); + } } } From d3a7200f201722f6ee8d136edcba79a30c54e25d Mon Sep 17 00:00:00 2001 From: Priyanka Tiwari Date: Thu, 11 Jun 2026 14:22:06 +0530 Subject: [PATCH 09/17] Address PR #4302 Copilot review: make WaitHandleDbConnectionPool.Startup() a no-op when ShuttingDown Guard Startup() against being called after Shutdown(). Without this, Startup() would create a fresh _cleanupTimer and queue a PoolCreateRequest against a pool that will never accept connections back, leaking the timer and scheduling background work that immediately short-circuits on State == ShuttingDown. Adds Startup_AfterShutdown_DoesNotResurrectPool regression test. --- .../WaitHandleDbConnectionPool.cs | 10 ++++++++++ .../WaitHandleDbConnectionPoolShutdownTest.cs | 19 +++++++++++++++++++ 2 files changed, 29 insertions(+) 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 23910d2f32..2bdd596f23 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 @@ -1544,6 +1544,16 @@ private bool ReclaimEmancipatedObjects() public void Startup() { SqlClientEventSource.Log.TryPoolerTraceEvent(" {0}, CleanupWait={1}", Id, _cleanupWait); + + // Do not resurrect a shut-down pool. Without this guard a Startup() call after + // Shutdown() would create a fresh _cleanupTimer and queue a PoolCreateRequest + // against a pool that will never accept connections back, leaking the timer + // and scheduling background work that immediately short-circuits. + if (State is ShuttingDown) + { + return; + } + _cleanupTimer = CreateCleanupTimer(); if (NeedToReplenish) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs index 032db01d7e..a46a6509f4 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs @@ -217,5 +217,24 @@ public void Shutdown_UnblocksSyncWaiter() // Suppress unused warning - presence of waiterCompleted just documents the contract. _ = waiterCompleted; } + + // Startup() must be a no-op when the pool has already been shut down. Without the + // guard it would create a fresh _cleanupTimer and queue a PoolCreateRequest against + // a pool that will never accept connections back. + [Fact] + public void Startup_AfterShutdown_DoesNotResurrectPool() + { + var pool = CreatePool(); + pool.Shutdown(); + Assert.Null(pool._cleanupTimer); + Assert.Equal(DbConnectionPoolState.ShuttingDown, pool.State); + + pool.Startup(); + + Assert.Equal(DbConnectionPoolState.ShuttingDown, pool.State); + Assert.False(pool.IsRunning); + // No new cleanup timer must have been scheduled against a shut-down pool. + Assert.Null(pool._cleanupTimer); + } } } From 751ad2a8eb3148e579f4df8321893882152f7c89 Mon Sep 17 00:00:00 2001 From: Priyanka Tiwari Date: Thu, 11 Jun 2026 18:07:00 +0530 Subject: [PATCH 10/17] Address PR #4302 Copilot review: drop unused 'using System.Diagnostics;' from ChannelDbConnectionPool Commit 6431209a replaced the only Debug.Assert in ChannelDbConnectionPool.cs with a race-window-guarded RemoveConnection call but left the 'using System.Diagnostics;' directive in place. Drop the now-unused directive so CS8019 cannot surface where it is promoted to an error. --- .../Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 61606e9953..40698cf3fe 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -5,7 +5,6 @@ using System.Collections.Concurrent; using System.Collections.ObjectModel; using System.Data.Common; -using System.Diagnostics; using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Channels; From 0fc55c240ca46d7ae5b950328a39659b6923ac47 Mon Sep 17 00:00:00 2001 From: Priyanka Tiwari Date: Tue, 30 Jun 2026 12:16:45 +0530 Subject: [PATCH 11/17] Address PR review: harden shutdown drain, async-path short-circuit, style flips, channel Complete tests - ChannelDbConnectionPool.Shutdown: wrap Pruner.Dispose, Clear, and per-connection RemoveConnection in try/catch so a single failure cannot leave the pool half-shut-down. - WaitHandleDbConnectionPool.TryGetConnection: re-check State after inner TryGetConnection on the async path and short-circuit with (true, null) to avoid enqueueing PendingGetConnection work against a non-Running pool. - WaitHandleDbConnectionPool: flip three positive-form shutdown guards (CleanupCallback, ErrorCallback, Startup) to 'State is not Running' for consistency with the rest of the file and to ease the planned bool-state migration. DeactivateObject's stasis-or-destroy branch and Shutdown's idempotency check intentionally left in positive form. - IdleConnectionChannelTest: add 6 tests covering Complete() idempotency, post-complete TryWrite/TryRead behavior, ChannelClosedException after drain, and pending ReadAsync waiter faulting on Complete (FR-007). --- .../ConnectionPool/ChannelDbConnectionPool.cs | 44 ++++++++-- .../WaitHandleDbConnectionPool.cs | 24 ++++-- .../IdleConnectionChannelTest.cs | 86 +++++++++++++++++++ 3 files changed, 144 insertions(+), 10 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 40698cf3fe..d244789584 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -295,30 +295,64 @@ public void Shutdown() // routes returning connections to RemoveConnection. State = ShuttingDown; + // Each cleanup step is independent and best-effort. A failure in one step must not + // prevent later steps from running, otherwise the pool can be left half-shut-down + // (e.g. timer disposed but channel never completed -> waiters stuck forever). + // Stop the idle-pruning timer before draining so a tick cannot race with - // the final drain below. PoolPruner.Dispose is idempotent and non-throwing. - Pruner?.Dispose(); + // the final drain below. PoolPruner.Dispose is idempotent and non-throwing + // in normal use; the catch is defense-in-depth. + try + { + Pruner?.Dispose(); + } + catch (Exception ex) + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Pruner.Dispose threw, continuing shutdown: {1}", Id, ex); + } // Complete the channel writer so: // - no further idle connections can be enqueued (TryWrite returns false), and // - in-flight / future async waiters on ReadAsync fault with ChannelClosedException. + // CAS at the top of Shutdown guarantees we are the only caller, so Complete cannot + // throw InvalidOperationException for double-completion. _idleChannel.Complete(); // Reuse Clear() for the drain. Clear bumps _clearGeneration so any active // checked-out connection fails IsLiveConnection on return and is removed, and it // drains the idle channel up to its captured IdleCount. - Clear(); + try + { + Clear(); + } + catch (Exception ex) + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Clear threw, continuing shutdown: {1}", Id, ex); + } // Clear() may short-circuit if another caller is already draining. Because the // channel is now completed, no new items can be enqueued, so it is safe to do a // final unbounded drain to mop up anything Clear() may have skipped. while (_idleChannel.TryRead(out DbConnectionInternal? connection)) { - if (connection is not null) + if (connection is null) + { + // null sentinels are wake-up signals only; nothing to destroy. + continue; + } + + // Isolate per-connection failure: one bad Dispose must not strand the rest. + try { RemoveConnection(connection); } - // null sentinels are wake-up signals only; nothing to destroy. + catch (Exception ex) + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, RemoveConnection threw during drain, continuing: {1}", Id, ex); + } } } 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 2bdd596f23..eb4940169e 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 @@ -331,10 +331,10 @@ public bool IsRunning internal void CleanupCallback(object state) { - // If the pool is shutting down, skip work. Shutdown disposes the timer, but + // If the pool is not Running, skip work. Shutdown disposes the timer, but // a callback may already be in-flight when Shutdown runs; this guard ensures it does // not perform pruning or re-arm pool create requests. - if (State == ShuttingDown) + if (State is not Running) { return; } @@ -772,9 +772,9 @@ private void DestroyObject(DbConnectionInternal obj) internal void ErrorCallback(object state) { - // Skip work if the pool is shutting down. The shutdown path disposes the + // Skip work if the pool is not Running. The shutdown path disposes the // timer; this guard handles the in-flight-callback race. - if (State == ShuttingDown) + if (State is not Running) { return; } @@ -938,6 +938,20 @@ public bool TryGetConnection(DbConnection owningObject, TaskCompletionSource {0}, Pool not Running after inner TryGetConnection; short-circuit async path.", Id); + connection = null; + return true; + } + long dueTime; if (LocalAppContextSwitches.UseOverallConnectTimeoutForPoolWait) { @@ -1549,7 +1563,7 @@ public void Startup() // Shutdown() would create a fresh _cleanupTimer and queue a PoolCreateRequest // against a pool that will never accept connections back, leaking the timer // and scheduling background work that immediately short-circuits. - if (State is ShuttingDown) + if (State is not Running) { return; } diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/IdleConnectionChannelTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/IdleConnectionChannelTest.cs index 56aa9c234a..faa4073a7d 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/IdleConnectionChannelTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/IdleConnectionChannelTest.cs @@ -5,6 +5,7 @@ using System; using System.Data.Common; using System.Threading; +using System.Threading.Channels; using System.Threading.Tasks; using System.Transactions; using Microsoft.Data.ProviderBase; @@ -160,6 +161,91 @@ await Assert.ThrowsAnyAsync( #endregion + #region Complete + + [Fact] + public void Complete_FirstCall_ReturnsTrue() + { + var channel = new IdleConnectionChannel(); + + Assert.True(channel.Complete()); + } + + [Fact] + public void Complete_SecondCall_ReturnsFalse() + { + var channel = new IdleConnectionChannel(); + Assert.True(channel.Complete()); + + // Idempotent: a second Complete is a safe no-op and must not throw. + Assert.False(channel.Complete()); + } + + [Fact] + public void TryWrite_AfterComplete_ReturnsFalseAndDoesNotIncrementCount() + { + var channel = new IdleConnectionChannel(); + channel.Complete(); + + Assert.False(channel.TryWrite(new StubDbConnectionInternal())); + Assert.False(channel.TryWrite(null)); + Assert.Equal(0, channel.Count); + } + + [Fact] + public void TryRead_AfterComplete_DrainsBufferedItems() + { + var channel = new IdleConnectionChannel(); + channel.TryWrite(new StubDbConnectionInternal()); + channel.TryWrite(new StubDbConnectionInternal()); + Assert.Equal(2, channel.Count); + + channel.Complete(); + + // Completion only stops new writes; already-buffered items remain readable. + Assert.True(channel.TryRead(out var first)); + Assert.NotNull(first); + Assert.True(channel.TryRead(out var second)); + Assert.NotNull(second); + Assert.Equal(0, channel.Count); + + // Once drained, further reads return false. + Assert.False(channel.TryRead(out _)); + } + + [Fact] + public async Task ReadAsync_AfterCompleteAndDrain_ThrowsChannelClosedException() + { + var channel = new IdleConnectionChannel(); + channel.TryWrite(new StubDbConnectionInternal()); + channel.Complete(); + + // Buffered item is still readable. + var buffered = await channel.ReadAsync(CancellationToken.None); + Assert.NotNull(buffered); + + // After the channel is drained, ReadAsync faults with ChannelClosedException. + await Assert.ThrowsAsync( + () => channel.ReadAsync(CancellationToken.None).AsTask()); + } + + [Fact] + public async Task ReadAsync_PendingWaiter_FaultsOnComplete() + { + // FR-007: a caller already parked in ReadAsync when shutdown completes the + // channel must be unblocked (not wait until its connection timeout). + var channel = new IdleConnectionChannel(); + + var readTask = channel.ReadAsync(CancellationToken.None); + Assert.False(readTask.IsCompleted); + + channel.Complete(); + + await Assert.ThrowsAsync(() => readTask.AsTask()); + } + + #endregion + #region Mixed operations [Fact] From 49b3dedb5f6abbabdc5e23d76dff1c5124043cbe Mon Sep 17 00:00:00 2001 From: Priyanka Tiwari Date: Tue, 30 Jun 2026 12:40:45 +0530 Subject: [PATCH 12/17] Pass idleTimeout to DbConnectionPoolGroupOptions in new shutdown tests Merge of main brought in the 7-arg DbConnectionPoolGroupOptions constructor from #4295 (Add configurable idle connection timeout). Update the two new shutdown test fixtures to pass idleTimeout: 0 so the build succeeds against the merged base. --- .../ConnectionPool/ChannelDbConnectionPoolShutdownTest.cs | 3 ++- .../ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolShutdownTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolShutdownTest.cs index 2840ee346d..7246a932ca 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolShutdownTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolShutdownTest.cs @@ -25,7 +25,8 @@ private static ChannelDbConnectionPool ConstructPool(int maxPoolSize = 5) maxPoolSize: maxPoolSize, creationTimeout: 15, loadBalanceTimeout: 0, - hasTransactionAffinity: true); + hasTransactionAffinity: true, + idleTimeout: 0); var dbConnectionPoolGroup = new DbConnectionPoolGroup( new SqlConnectionOptions("Data Source=localhost;"), diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs index a46a6509f4..139640476a 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs @@ -24,7 +24,8 @@ private static WaitHandleDbConnectionPool CreatePool(int maxPoolSize = 5) maxPoolSize: maxPoolSize, creationTimeout: 15000, loadBalanceTimeout: 0, - hasTransactionAffinity: true); + hasTransactionAffinity: true, + idleTimeout: 0); var dbConnectionPoolGroup = new DbConnectionPoolGroup( new SqlConnectionOptions("Data Source=localhost;"), From 615317cb849565f23244d0e7458aea4c95349a5c Mon Sep 17 00:00:00 2001 From: Priyanka Tiwari Date: Tue, 30 Jun 2026 13:40:24 +0530 Subject: [PATCH 13/17] Add async-overload shutdown short-circuit test (outer guard path) Covers Copilot review feedback: the async TryGetConnection overload (taskCompletionSource != null) called after Shutdown must take the (true, null) short-circuit at the outer State guard, leave the TCS untouched, and not enqueue a PendingGetConnection. The post-inner race-window backstop is intentionally not tested here because the async caller does not park inside the inner TryGetConnection (waitForMultipleObjectsTimeout is 0); the window cannot be triggered deterministically without instrumenting production code. --- .../WaitHandleDbConnectionPoolShutdownTest.cs | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs index 139640476a..52bf9ccb29 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs @@ -4,6 +4,7 @@ using System; using System.Threading; +using System.Threading.Tasks; using Microsoft.Data.Common.ConnectionString; using Microsoft.Data.ProviderBase; using Microsoft.Data.SqlClient.ConnectionPool; @@ -160,6 +161,29 @@ public void TryGetConnection_AfterShutdown_ReturnsNullWithoutBlocking() Assert.Null(conn); } + // Async overload (taskCompletionSource != null) called after Shutdown must take the + // (true, null) short-circuit instead of enqueueing a PendingGetConnection and spinning + // up a WaitForPendingOpen background loop. The TCS must be left untouched so the caller + // surfaces a deterministic shutdown signal rather than an eventual PooledOpenTimeout. + [Fact] + public void TryGetConnection_Async_AfterShutdown_ShortCircuits_NoPendingOpenScheduled() + { + var pool = CreatePool(); + pool.Shutdown(); + + var tcs = new TaskCompletionSource(); + bool completed = pool.TryGetConnection( + new SqlConnection(), + tcs, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? conn); + + Assert.True(completed); + Assert.Null(conn); + Assert.False(tcs.Task.IsCompleted); + Assert.Equal(0, Volatile.Read(ref pool._waitCount)); + } + // Shutdown wakes up a thread parked in WaitHandle.WaitAny. [Fact] public void Shutdown_UnblocksSyncWaiter() From f44e5e32fa63cca945b117e797fa45aa1c5a9ae8 Mon Sep 17 00:00:00 2001 From: Priyanka Tiwari Date: Tue, 30 Jun 2026 18:58:06 +0530 Subject: [PATCH 14/17] Fix stale Startup comment in ChannelDbConnectionPool The previous comment claimed pruning was not implemented, but PR #4295 introduced PoolPruner which is constructed eagerly in the ctor (when MinPoolSize < MaxPoolSize) and arms/disarms its timer via UpdateTimer() calls from OpenNewInternalConnection and RemoveConnection. Update the comment so future maintainers see Startup correctly described as a no-op symmetrical counterpart of Shutdown, while accurately documenting that pruning's background timer is owned by PoolPruner and managed by the connection lifecycle paths, not by Startup. --- .../ConnectionPool/ChannelDbConnectionPool.cs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index bd01f54bb4..30e0307033 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -376,10 +376,12 @@ public void Shutdown() /// public void Startup() { - // This pool has no background timers today (idle timeout is enforced lazily in - // IsLiveConnection on retrieval; pruning is not implemented). State is set to Running - // in the constructor, so this is currently the symmetrical counterpart of Shutdown. - // Background work (warmup, pruning timers) will be added here when introduced. + // Startup is currently a no-op for this pool: State is set to Running in the + // constructor, and PoolPruner (when present, i.e. MinPoolSize < MaxPoolSize) is + // also constructed eagerly there; its timer arms/disarms via UpdateTimer() calls + // from OpenNewInternalConnection and RemoveConnection as the pool grows/shrinks. + // This method exists as the symmetrical counterpart of Shutdown and as a hook + // for future warmup behavior. SqlClientEventSource.Log.TryPoolerTraceEvent( " {0}", Id); } From 2a07452fd94606ada7620ca0c3bf40919731fb84 Mon Sep 17 00:00:00 2001 From: Priyanka Tiwari Date: Tue, 30 Jun 2026 19:10:52 +0530 Subject: [PATCH 15/17] Use 'is not Running' consistently in WaitHandleDbConnectionPool Two new shutdown-state checks I added (async-path short-circuit and post-WaitAny re-check) used 'State != Running' while the rest of the file consistently uses 'State is not Running'. Align them, plus the adjacent comments, with the prevailing style. Trace strings are left as '!=' since they are human-readable log output. --- .../ConnectionPool/WaitHandleDbConnectionPool.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 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 55c53b7e01..5a16f482b6 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 @@ -969,12 +969,12 @@ public bool TryGetConnection(DbConnection owningObject, TaskCompletionSource {0}, Pool not Running after inner TryGetConnection; short-circuit async path.", Id); @@ -1054,7 +1054,7 @@ private bool TryGetConnection(DbConnection owningObject, uint waitForMultipleObj // balanced; otherwise the slot would leak and other waiters // (or callers that arrive after Shutdown completes its own // Release loop) would starve. - if (State != Running) + if (State is not Running) { SqlClientEventSource.Log.TryPoolerTraceEvent(" {0}, Pool is shutting down; abandoning wait.", Id); if (waitResult == SEMAPHORE_HANDLE || waitResult == WAIT_ABANDONED + SEMAPHORE_HANDLE) @@ -1660,7 +1660,7 @@ public void Shutdown() // 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 != Running after wake-up and bail. + // observe State is not Running after wake-up and bail. int waitersToWake = Volatile.Read(ref _waitCount); if (waitersToWake > 0) { From c5a55441e2b66a79b0a528419c5f6115d4348174 Mon Sep 17 00:00:00 2001 From: Priyanka Tiwari Date: Tue, 30 Jun 2026 19:39:11 +0530 Subject: [PATCH 16/17] Compensate CreationSemaphore in shutdown fast-path; fix Complete() comment WaitHandleDbConnectionPool: when the shutdown fast-path after WaitAny returns early, the outer finally that normally releases CreationSemaphore on the CREATION_HANDLE path is bypassed. Mirror the existing PoolSemaphore compensation: on waitResult == CREATION_HANDLE (and the WAIT_ABANDONED variant), Release(1) on CreationSemaphore inside a try/catch SemaphoreFullException so accounting stays balanced. ChannelDbConnectionPool: the comment on _idleChannel.Complete() referenced InvalidOperationException, but IdleConnectionChannel.Complete wraps ChannelWriter.TryComplete which is idempotent and never throws. Rewrite the comment to describe TryComplete's actual semantics so future maintainers aren't misled about why this call is safe. --- .../ConnectionPool/ChannelDbConnectionPool.cs | 5 ++-- .../WaitHandleDbConnectionPool.cs | 23 +++++++++++++++---- 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 30e0307033..f5d758ebb7 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -327,8 +327,9 @@ public void Shutdown() // Complete the channel writer so: // - no further idle connections can be enqueued (TryWrite returns false), and // - in-flight / future async waiters on ReadAsync fault with ChannelClosedException. - // CAS at the top of Shutdown guarantees we are the only caller, so Complete cannot - // throw InvalidOperationException for double-completion. + // IdleConnectionChannel.Complete wraps ChannelWriter.TryComplete and is idempotent + // (a second call returns false rather than throwing), so this is safe even if the + // shutdown sequence is ever refactored to invoke this step more than once. _idleChannel.Complete(); // Reuse Clear() for the drain. Clear bumps _clearGeneration so any active 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 5a16f482b6..706281fb0a 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 @@ -1049,11 +1049,13 @@ private bool TryGetConnection(DbConnection owningObject, uint waitForMultipleObj waitResult = WaitHandle.WaitAny(_waitHandles.GetHandles(allowCreate), unchecked((int)waitForMultipleObjectsTimeout)); // After waking, observe shutdown state and bail out so waiters - // do not spin against a drained pool. If WaitAny consumed a - // PoolSemaphore slot, release it back so the accounting stays - // balanced; otherwise the slot would leak and other waiters - // (or callers that arrive after Shutdown completes its own - // Release loop) would starve. + // do not spin against a drained pool. WaitAny consumes one slot from + // whichever wait handle signalled (PoolSemaphore or CreationSemaphore); + // release that slot back so the accounting stays balanced. Otherwise + // the slot would leak and other in-flight pool logic could starve. + // The outer finally that normally releases CreationSemaphore on the + // CREATION_HANDLE path is bypassed by this early return, so we have + // to compensate explicitly here. if (State is not Running) { SqlClientEventSource.Log.TryPoolerTraceEvent(" {0}, Pool is shutting down; abandoning wait.", Id); @@ -1068,6 +1070,17 @@ private bool TryGetConnection(DbConnection owningObject, uint waitForMultipleObj // Pool semaphore was already saturated by Shutdown's bulk release; safe to ignore. } } + else if (waitResult == CREATION_HANDLE || waitResult == WAIT_ABANDONED + CREATION_HANDLE) + { + try + { + _waitHandles.CreationSemaphore.Release(1); + } + catch (SemaphoreFullException) + { + // CreationSemaphore is already at max; safe to ignore. + } + } Interlocked.Decrement(ref _waitCount); connection = null; return false; From 9419ee720301a3eb310c0c0d7bb76c4a6406619c Mon Sep 17 00:00:00 2001 From: Priyanka Tiwari Date: Wed, 1 Jul 2026 17:01:12 +0530 Subject: [PATCH 17/17] Revert CreationSemaphore compensation in shutdown fast-path (fixes CI) Commit c5a55441 added a CreationSemaphore.Release(1) in the shutdown fast-path based on the premise that the early 'return false' bypassed the outer finally that normally releases CreationSemaphore on CREATION_HANDLE. That premise was wrong: C# try/finally executes the finally on return, so the outer finally at line ~1196 already handled the release. My extra Release therefore double-released, tripping SemaphoreFullException (CreationSemaphore max=1) whenever the fast-path was taken with waitResult == CREATION_HANDLE. CI's WaitHandleDbConnectionPoolShutdownTest.Shutdown_UnblocksSyncWaiter hit this reliably in Debug config. Remove the CreationSemaphore else-if branch and rewrite the comment to explain why compensation is intentionally absent (outer finally handles it). Keep the PoolSemaphore compensation - that path has no outer finally, so the manual Release is genuinely required. Verified locally: Shutdown_UnblocksSyncWaiter and 24 other Shutdown-related tests pass on net9.0; 20/20 stress iterations of the previously-failing test pass. --- .../WaitHandleDbConnectionPool.cs | 26 ++++++------------- 1 file changed, 8 insertions(+), 18 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 706281fb0a..2aac0a6ce8 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 @@ -1049,13 +1049,14 @@ private bool TryGetConnection(DbConnection owningObject, uint waitForMultipleObj waitResult = WaitHandle.WaitAny(_waitHandles.GetHandles(allowCreate), unchecked((int)waitForMultipleObjectsTimeout)); // After waking, observe shutdown state and bail out so waiters - // do not spin against a drained pool. WaitAny consumes one slot from - // whichever wait handle signalled (PoolSemaphore or CreationSemaphore); - // release that slot back so the accounting stays balanced. Otherwise - // the slot would leak and other in-flight pool logic could starve. - // The outer finally that normally releases CreationSemaphore on the - // CREATION_HANDLE path is bypassed by this early return, so we have - // to compensate explicitly here. + // do not spin against a drained pool. If WaitAny consumed a + // PoolSemaphore slot, release it back so the accounting stays + // balanced; otherwise the slot would leak and other waiters + // (or callers that arrive after Shutdown completes its own + // Release loop) would starve. CreationSemaphore does NOT need + // compensation here because the outer finally below already + // releases it whenever waitResult == CREATION_HANDLE, and + // that finally runs even on this early return. if (State is not Running) { SqlClientEventSource.Log.TryPoolerTraceEvent(" {0}, Pool is shutting down; abandoning wait.", Id); @@ -1070,17 +1071,6 @@ private bool TryGetConnection(DbConnection owningObject, uint waitForMultipleObj // Pool semaphore was already saturated by Shutdown's bulk release; safe to ignore. } } - else if (waitResult == CREATION_HANDLE || waitResult == WAIT_ABANDONED + CREATION_HANDLE) - { - try - { - _waitHandles.CreationSemaphore.Release(1); - } - catch (SemaphoreFullException) - { - // CreationSemaphore is already at max; safe to ignore. - } - } Interlocked.Decrement(ref _waitCount); connection = null; return false;