-
Notifications
You must be signed in to change notification settings - Fork 329
Implement pool shutdown for ChannelDbConnectionPool and harden WaitHandleDbConnectionPool shutdown #4302
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Implement pool shutdown for ChannelDbConnectionPool and harden WaitHandleDbConnectionPool shutdown #4302
Changes from 2 commits
6431209
b24d4fb
7ba73a8
b8d56d9
9a45a66
954679d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -329,6 +329,14 @@ public bool IsRunning | |
|
|
||
| private 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 | ||
| // 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) | ||
| { | ||
| // 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("<prov.DbConnectionPool.ErrorCallback|RES|CPOOL> {0}, Resetting Error handling.", Id); | ||
| _errorOccurred = false; | ||
| _waitHandles.ErrorEvent.Reset(); | ||
|
|
@@ -956,6 +971,31 @@ 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. | ||
| if (State != Running) | ||
| { | ||
| SqlClientEventSource.Log.TryPoolerTraceEvent("<prov.DbConnectionPool.GetConnection|RES|CPOOL> {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; | ||
| } | ||
|
|
||
| // 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 +1521,50 @@ public void Startup() | |
| public void Shutdown() | ||
| { | ||
| SqlClientEventSource.Log.TryPoolerTraceEvent("<prov.DbConnectionPool.Shutdown|RES|INFO|CPOOL> {0}", Id); | ||
|
|
||
| // Idempotent: subsequent calls observe ShuttingDown and bail. | ||
| if (State == ShuttingDown) | ||
| { | ||
| return; | ||
| } | ||
|
Comment on lines
1521
to
+1529
|
||
| State = ShuttingDown; | ||
|
|
||
| // deactivate timer callbacks | ||
| Timer t = _cleanupTimer; | ||
| _cleanupTimer = null; | ||
| if (t != null) | ||
| // 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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's make sure we've addressed this bug: #1881
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This PR adds the per-pool Shutdown() machinery that #1881 ultimately needs, but it doesn't actually touch SqlConnectionFactory._pruningTimer — so the symptom (the PruneConnectionPoolGroups trace firing every 30 s after ClearAllPools()) will still reproduce. I'd prefer to land #4302 as the foundation and address #1881 in a small follow-up that adds a "park the timer when there's no work" guard to PruneConnectionPoolGroups and a re-arm in GetConnectionPoolGroup. Happy to file that follow-up issue and link it. |
||
| cleanup?.Dispose(); | ||
| Timer error = Interlocked.Exchange(ref _errorTimer, null); | ||
| error?.Dispose(); | ||
|
|
||
| // Wake any threads parked in WaitHandle.WaitAny by releasing as many semaphore | ||
| // slots as there are recorded waiters. Using _waitCount (rather than MaxPoolSize) | ||
| // avoids ArgumentOutOfRangeException when MaxPoolSize == 0 (unlimited) and ensures | ||
| // we wake every parked waiter even when _waitCount exceeds MaxPoolSize. Waiters | ||
| // observe State != Running after wake-up and bail. | ||
| int waitersToWake = Volatile.Read(ref _waitCount); | ||
| if (waitersToWake > 0) | ||
| { | ||
| try | ||
| { | ||
| _waitHandles.PoolSemaphore.Release(waitersToWake); | ||
| } | ||
| catch (SemaphoreFullException) | ||
| { | ||
| // Semaphore already saturated; nothing to do. | ||
| } | ||
| } | ||
|
|
||
| // 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)) | ||
|
priyankatiwari08 marked this conversation as resolved.
Outdated
|
||
| { | ||
| DestroyObject(newObj); | ||
| } | ||
| while (_stackOld.TryPop(out DbConnectionInternal oldObj)) | ||
| { | ||
| t.Dispose(); | ||
| DestroyObject(oldObj); | ||
| } | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.