From 6431209ad91ae4d18efced5c390bbbbc05682d20 Mon Sep 17 00:00:00 2001 From: Priyanka Tiwari Date: Thu, 21 May 2026 17:48:23 +0530 Subject: [PATCH 1/2] 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 2/2] =?UTF-8?q?=EF=BB=BF=20=20Address=20PR=20#4302=20revie?= =?UTF-8?q?w=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();