Skip to content
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

EventLoopScheduler does not dispose SemaphoreSlim if thread not running when Disposed #1926

Open
idg10 opened this issue May 4, 2023 · 1 comment

Comments

@idg10
Copy link
Collaborator

idg10 commented May 4, 2023

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:

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:

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.

@idg10 idg10 added the [area] Rx label May 4, 2023
@idg10
Copy link
Collaborator Author

idg10 commented May 4, 2023

Note that this is an area of the code in we've had subtle bugs before, so this may not be entirely straightforward to fix: #286

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant