fix: correct disposal ordering to prevent ObjectDisposedException during shutdown#519
Merged
Conversation
2 tasks
…ing shutdown The CancellationTokenSource in MaintenanceBase was being disposed before background tasks (maintenance loops, workers) had a chance to observe the cancellation and terminate. This caused ObjectDisposedException when the still-running tasks accessed DisposedCancellationToken after the CTS was already disposed. Changes: - Use Interlocked.CompareExchange in SignalDispose() for thread-safe disposal (BCL best practice matching MessageBusBase pattern) - Reorder InMemoryQueue.Dispose() to clear queues before waiting for workers (prevents workers from picking up additional work during shutdown) - Remove redundant IsDisposed guard from QueueBase.Dispose() since derived classes own the idempotency check via SignalDispose() - Fix pre-existing NRT warning in CacheClientTestsBase Fixes FoundatioFx/Foundatio.Redis#165
niemyjski
force-pushed
the
fix/dispose-cts-ordering
branch
from
8e6d306 to
b6187bd
Compare
- MessageBusBase.IsDisposed: use Volatile.Read for thread-safe reads (matches MaintenanceBase pattern) - MessageBusBase.DisposeAsync: add diagnostic trace log on re-entry (matches Dispose behavior) - InMemoryQueue.Dispose: add diagnostic trace log on re-entry (matches RedisQueue pattern) - CacheClientTestsBase: use Assert.NotNull before .Value to properly validate expiration exists (fixes NRT warning without weakening assertion semantics)
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| Assert.True((await cache.GetExpirationAsync("test")).Value < TimeSpan.FromSeconds(1)); | ||
| var expiration = await cache.GetExpirationAsync("test"); | ||
| Assert.NotNull(expiration); | ||
| Assert.True(expiration.Value < TimeSpan.FromSeconds(1)); |
SignalDispose() now returns true on first call, false if already signaled. This eliminates the TOCTOU gap between checking IsDisposed and calling SignalDispose() in leaf classes, collapsing two operations into one atomic check. Also extracts SignalDispose() in MessageBusBase for the same pattern consistency.
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
1 participant
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
SignalDispose()inMaintenanceBaseto separate cancellation signaling from resource disposalQueueBase,InMemoryQueueto: signal cancellation, wait for background tasks, then dispose resources (CTS last)Dispose_WithMaintenanceRunning_DoesNotThrowObjectDisposedExceptiontest toQueueTestBaseRoot Cause
MaintenanceBase.Dispose()cancelled AND disposed theCancellationTokenSourcebefore derived classes had a chance to wait on their background tasks. The still-running maintenance loop accessedDisposedCancellationToken(which calls.Tokenon the disposed CTS), throwingObjectDisposedException.Test plan
Dispose_WithMaintenanceRunning_DoesNotThrowObjectDisposedExceptionvalidates the fixFixes FoundatioFx/Foundatio.Redis#165