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; + } + } +}