Implement pool shutdown for ChannelDbConnectionPool and harden WaitHandleDbConnectionPool shutdown#4302
Draft
priyankatiwari08 wants to merge 1 commit into
Draft
Conversation
…ndleDbConnectionPool 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.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements the “pool shutdown” contract (spec 004-pool-shutdown) across both connection pool implementations so that retiring a pool (via SqlConnectionFactory.QueuePoolForRelease) reliably stops background work, unblocks waiters, and destroys idle/returning connections.
Changes:
- Implement
ChannelDbConnectionPool.Shutdown()by transitioning toShuttingDown, completing/draining the idle channel, and making shutdown idempotent. - Harden
WaitHandleDbConnectionPool.Shutdown()by making it idempotent, disposing timers, draining idle stacks, and attempting to wake blocked synchronous waiters; guard timer callbacks during shutdown. - Add unit tests covering shutdown behavior for both pool types.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs | Implements real shutdown behavior (state transition, channel completion/drain, safer return-on-shutdown) and traces Startup(). |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IdleConnectionChannel.cs | Adds Complete() to allow the pool to terminate the channel writer on shutdown. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs | Adds shutdown idempotency, disposes both timers, drains idle stacks, attempts to unblock sync waiters, and guards timer callbacks during shutdown. |
| src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolShutdownTest.cs | Adds unit coverage for channel-pool shutdown semantics (drain, idempotency, unblock waiters, destroy-on-return). |
| src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs | Adds unit coverage for wait-handle-pool shutdown semantics (timer disposal, drain, idempotency, unblock waiter, callback no-op). |
Comment on lines
971
to
+983
| { | ||
| 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("<prov.DbConnectionPool.GetConnection|RES|CPOOL> {0}, Pool is shutting down; abandoning wait.", Id); | ||
| Interlocked.Decrement(ref _waitCount); | ||
| connection = null; | ||
| return false; | ||
| } | ||
|
|
Comment on lines
+1526
to
+1536
| // 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. | ||
| } |
Comment on lines
+216
to
+217
| // Give the waiter time to enter WaitAny. | ||
| Thread.Sleep(200); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ChannelDbConnectionPool.Shutdown()/Startup()were stubs (// No-op for now). This PR provides a real implementation and tightens the existingWaitHandleDbConnectionPoolshutdown path so both pools behave consistently whenSqlConnectionFactory.QueuePoolForReleaseretires a pool.Changes
ChannelDbConnectionPoolShutdown()transitionsStatetoShuttingDown, completes the idle channel writer, drains buffered idle connections, and is idempotent (Interlocked.CompareExchangeguard).Startup()traces and is a no-op today (no background timers in this pool yet); kept as the symmetric counterpart.ReturnInternalConnectionno longer asserts on a failed channel write — it falls back toRemoveConnection, closing the shutdown-return race window.IdleConnectionChannelComplete()to terminate the underlyingChannel<T>writer so in-flight / future readers wake up and no further idle connections can be enqueued.WaitHandleDbConnectionPoolShutdown()is now idempotent, disposes both_cleanupTimerand_errorTimer(previously only_cleanupTimer), drains_stackNew/_stackOld, and releases_waitHandles.PoolSemaphoreto wake any blocked waiters.State == ShuttingDownso in-flight callbacks don't re-arm or do work after shutdown.Tests
Two new xUnit test files (12 tests total):
ChannelDbConnectionPoolShutdownTest.cs— state transition, drain, return-after-shutdown destruction, idempotency, async-waiter unblock, post-shutdownTryGetConnectionfails, terminal-state guarantee.WaitHandleDbConnectionPoolShutdownTest.cs— both timer disposes, stack drain, idempotency, waiter wake-up, callback no-op after shutdown, transaction-root stasis path regression check.Full UnitTests run: 340/340 passing locally.
Spec coverage
P1 stories (FR-001 … FR-007) are fully covered. FR-008 / FR-009 / FR-010 (channel-pool transaction-root stasis on shutdown) are intentionally deferred —
ChannelDbConnectionPool.PutObjectFromTransactedPoolandTransactionEndedstill throwNotImplementedExceptionpending broader transaction support in the new pool; a follow-up will land alongside that work. Until then, returns during shutdown fall back toRemoveConnectionper the spec's "destroy on shutdown" fallback.