dotnet / dotnet/reactive

EventLoopScheduler does not dispose SemaphoreSlim if thread not running when Disposed

Open
#1,926 1 comment 0 reactions 0 assignees View on GitHub
[area] Rx
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.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.