EventLoopScheduler does not dispose SemaphoreSlim if thread not running when Disposed
- Dominant language
- C#
- Stars
- 7.2k
- Forks
- 798
- PR merge metrics
- No merged PRs in 30d
Description
#### Bug
Library version: 5.0
All platforms
The `EventLoopScheduler` creates a `SemaphoreSlim` (referred to by its `_evt` field). By design, its `Dispose` method does not call `_evt.Dispose` directly. This is because the scheduler's designated thread might be using it. So that designated thread is responsible for calling `Dispose` on the `_evt` when it detects disposal.
The problem with this is that an `EventLoopScheduler` can be in a state where there is no thread currently running. There are two ways this can occur:
1. before the first work is scheduled
2. when being used via the derived `NewThreadScheduler`, the thread shuts down any time the scheduler goes idle (more generally this happens if `ExitIfEmpty` is true, but that internal property is currently set only by `NewThreadScheduler`
If `Dispose` is called when there is no current thread, the `SemaphoreSlim` is not disposed.
If we run this code:
```cs
var s = new EventLoopScheduler();
IDisposable d = s;
s.Schedule(() => Console.WriteLine("Scheduled work!"));
d.Dispose();
```
the `SemaphoreSlim` will be disposed. (It might not happen immediately, because it won't occur until the event loop thread detects disposal, but it generally happens eitehr during or very soon after `Dispose`.)
But if we remove the call to `Schedule`:
```cs
var s = new EventLoopScheduler();
IDisposable d = s;
d.Dispose();
```
the `SemaphoreSlim` will never be disposed.
For the `EventLoopScheduler`, this is unlikely to cause problems in practice because it would be unusual to create one without then using it, and even then it would be fairly unusual to create a lot of them. So the impact is likely to be only that one `SemaphoreSlim` will be relying on finalization to perform any necessary cleanup.
It is more of an issue for the derived `NewThreadScheduler`, in which we expect to enter a "no thread" state every time the work queue is drained. It's the default state. Even then the impact is probably small because we don't expect large numbers of scheduler instances to be created. Nonetheless, this is a bug. The semaphore should be disposed of even when the scheduler has no current thread.
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.