fix: Correct disposal race condition in LeaderElectionService#5
fix: Correct disposal race condition in LeaderElectionService#5
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements disposal race condition fixes for the LeaderElectionService to prevent ObjectDisposedException errors during shutdown. The changes add proper synchronization mechanisms to wait for active timer callbacks before disposing resources, along with comprehensive test coverage for various disposal scenarios.
- Introduced
IAsyncDisposableimplementation with callback tracking to safely wait for active operations - Added disposal state checks in timer callbacks to prevent operations on disposed objects
- Created comprehensive test suite validating disposal behavior under concurrent scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 13 comments.
| File | Description |
|---|---|
| src/MultiLock/LeaderElectionService.cs | Implemented IAsyncDisposable, added callback tracking with activeCallbacks counter, enhanced disposal logic to wait for active callbacks, and added disposal state checks throughout timer callbacks |
| tests/MultiLock.Tests/DisposalRaceConditionTests.cs | Added comprehensive test suite covering disposal race conditions, concurrent disposal, rapid start/stop cycles, and multi-service disposal scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Implemented IAsyncDisposable to prevent ObjectDisposedException when timer callbacks execute during disposal. Added active callback tracking with atomic operations and wait mechanism to ensure safe shutdown. Includes 8 new tests covering concurrent disposal scenarios.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 18 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (isDisposed) | ||
| return; | ||
|
|
||
| // Set disposal flag first to prevent new operations | ||
| isDisposed = true; |
There was a problem hiding this comment.
The check-then-set pattern for isDisposed (lines 335-339) is not thread-safe. Multiple concurrent calls to DisposeAsync() could both pass the check at line 335 before either sets the flag at line 339, leading to duplicate disposal logic execution. Use Interlocked.CompareExchange to atomically check and set the flag, or convert isDisposed from bool to int and use Interlocked.CompareExchange(ref isDisposed, 1, 0) to ensure only one thread proceeds with disposal.
| ServiceProvider serviceProvider = services.BuildServiceProvider(); | ||
| var host = serviceProvider.GetRequiredService<IHostedService>() as LeaderElectionService; | ||
| host.ShouldNotBeNull(); | ||
|
|
||
| // Act - Start the service and let it acquire leadership | ||
| await host.StartAsync(CancellationToken.None); | ||
| await Task.Delay(200); // Wait for leadership acquisition and heartbeat to start | ||
|
|
||
| // Dispose while heartbeat callbacks are likely running | ||
| var disposeTask = Task.Run(async () => await host.DisposeAsync()); | ||
|
|
||
| // Should complete without throwing | ||
| await disposeTask.WaitAsync(TimeSpan.FromSeconds(15)); | ||
|
|
||
| // Assert - No exception should be thrown | ||
| disposeTask.IsCompletedSuccessfully.ShouldBeTrue(); |
There was a problem hiding this comment.
The serviceProvider created at line 31 is never disposed, causing a resource leak. Add await serviceProvider.DisposeAsync(); after line 46 to properly clean up the service provider and its managed services.
| await disposeTask.WaitAsync(TimeSpan.FromSeconds(15)); | ||
|
|
||
| // Assert - No exception should be thrown | ||
| disposeTask.IsCompletedSuccessfully.ShouldBeTrue(); |
There was a problem hiding this comment.
The serviceProvider created at line 64 is never disposed, causing a resource leak. Add await serviceProvider.DisposeAsync(); after line 79 to properly clean up the service provider and its managed services.
| disposeTask.IsCompletedSuccessfully.ShouldBeTrue(); | |
| disposeTask.IsCompletedSuccessfully.ShouldBeTrue(); | |
| await serviceProvider.DisposeAsync(); |
| await Task.WhenAll(disposeTasks).WaitAsync(TimeSpan.FromSeconds(15)); | ||
|
|
||
| // Assert - All dispose tasks should complete successfully | ||
| disposeTasks.All(t => t.IsCompletedSuccessfully).ShouldBeTrue(); |
There was a problem hiding this comment.
The serviceProvider created at line 96 is never disposed, causing a resource leak. Add await serviceProvider.DisposeAsync(); after line 112 to properly clean up the service provider and its managed services.
| disposeTasks.All(t => t.IsCompletedSuccessfully).ShouldBeTrue(); | |
| disposeTasks.All(t => t.IsCompletedSuccessfully).ShouldBeTrue(); | |
| await serviceProvider.DisposeAsync(); |
| await host.StartAsync(CancellationToken.None); | ||
| await Task.Delay(50); // Very short delay | ||
| await host.StopAsync(CancellationToken.None); | ||
| await host.DisposeAsync(); |
There was a problem hiding this comment.
The serviceProvider created at line 135 is never disposed, causing a resource leak. Add await serviceProvider.DisposeAsync(); after line 142 to properly clean up the service provider and its managed services.
| await host.DisposeAsync(); | |
| await host.DisposeAsync(); | |
| await serviceProvider.DisposeAsync(); |
| var host = serviceProvider.GetRequiredService<IHostedService>() as LeaderElectionService; | ||
| host.ShouldNotBeNull(); |
There was a problem hiding this comment.
Variable host may be null at this access because of this assignment.
| var host = serviceProvider.GetRequiredService<IHostedService>() as LeaderElectionService; | |
| host.ShouldNotBeNull(); | |
| if (serviceProvider.GetRequiredService<IHostedService>() is not LeaderElectionService host) | |
| throw new InvalidOperationException("IHostedService is not a LeaderElectionService"); |
| var host = serviceProvider.GetRequiredService<IHostedService>() as LeaderElectionService; | ||
| host.ShouldNotBeNull(); | ||
|
|
There was a problem hiding this comment.
| host.ShouldNotBeNull(); | ||
|
|
||
| // Act | ||
| await host.StartAsync(CancellationToken.None); |
There was a problem hiding this comment.
| var host = serviceProvider.GetRequiredService<IHostedService>() as LeaderElectionService; | ||
| host.ShouldNotBeNull(); | ||
|
|
||
| await host.StartAsync(CancellationToken.None); |
There was a problem hiding this comment.
| provider.Dispose(); | ||
| } | ||
| // Dispose should never throw, but we catch defensively to ensure cleanup completes | ||
| catch (Exception ex) |
There was a problem hiding this comment.
Generic catch clause.
| catch (Exception ex) | |
| catch (Exception ex) when ( | |
| ex is not OutOfMemoryException && | |
| ex is not StackOverflowException && | |
| ex is not ThreadAbortException) |
Implemented IAsyncDisposable to prevent ObjectDisposedException when timer callbacks execute during disposal. Added active callback tracking with atomic operations and wait mechanism to ensure safe shutdown.
Includes 8 new tests covering concurrent disposal scenarios.