SemaphoreSlim disposed before released?

Question

In the update to the accepted answer to this stack-overflow question it is mentioned that

Semaphore may be disposed before completion of tasks and will raise exception when Release() method is called so before exiting the using block must wait for the completion of all created Tasks

How can this happen?

I'll paste the code here:

int maxConcurrency=10;
var messages = new List<string>();
using(SemaphoreSlim concurrencySemaphore = new SemaphoreSlim(maxConcurrency))
{
    List<Task> tasks = new List<Task>();
    foreach(var msg in messages)
    {
        concurrencySemaphore.Wait();

        var t = Task.Factory.StartNew(() =>
        {

            try
            {
                 Process(msg);
            }
            finally
            {
                concurrencySemaphore.Release();
            }
        });

        tasks.Add(t);
    }

    Task.WaitAll(tasks.ToArray());
}

So 1. Why is the task list needed?

And 2. In my case there is no foreach but rather a socket is randomly accepting requests & calls the Task.Factory.StartNew. I cannot wait for all threads to complete. Can this semaphore method still be used?

`Semaphore may be disposed before completion of tasks` - it is not possible in current version of the code. Comment was for the previous version of the answer https://stackoverflow.com/revisions/36571420/2 — Dmitry Pavliv, Aug 13 '18 at 13:23
@DmitryPavliv - yes, the question is why was the update needed, specifically how will the semaphore be disposed before release — grunt, Aug 13 '18 at 13:25
This doesn't help the question and it may not make a difference for your application but technically speaking; shouldn't ```concurrencySemaphore.Wait(); ``` be placed inside of ```StartNew(() => ``` just before the ```try```? — Michael Puckett II, Aug 13 '18 at 13:25
@MichaelPuckettII no, that way you will create ton of tasks which are waiting semaphore to allow them start code execution. It will be waste of resources — Dmitry Pavliv, Aug 13 '18 at 13:28
@DmitryPavliv Yes, this is true but is the method you're showing us now in it's own ```Task```? If it's on the primary then you're primary is blocked awaiting the ```Semaphore```. If the method you're showing us here is on it's on thread then I'm sold. — Michael Puckett II, Aug 13 '18 at 13:36
@MichaelPuckettII `Task.WaitAll` will also block. This is a separate question which can be solved by `await Task.WhenAll(..)` and `await concurrencySemaphore.WaitAsync()` — Dmitry Pavliv, Aug 13 '18 at 13:38
The call to `Task.WaitAll` prevents the `SemaphoreSlim` from being disposed before all tasks have been finished. If you don't store references to the tasks and wait for them to be completed before you dispose the `SemaphoreSlim`, it is possible that it will be disposed before all calls to `Process` have returned. — mm8, Aug 13 '18 at 13:39
@DmitryPavliv I guess the point I'm making is; you'll never get to ```Task.WaitAll``` if you're held by the ```Semaphore``` in the ```foreach``` loop. Yes, this is something separate but it's always nice to have someone 'comment' helpful suggestions to the code. It's also helpful to talk to the OP in the comments instead of having the editor of the question become the OP. — Michael Puckett II, Aug 13 '18 at 13:40
@mm8 - yes, how will the semaphore be disposed before release — grunt, Aug 13 '18 at 13:42
@mm8 It's ok if it's disposed before they return because it will never reach the ```Task.WaitAll``` while the Semaphore is holding the thread. By the time they are allowed to pass it will be the last item in the loop passing and the Semaphore is clear to be forgotten. I believe the design of this method is logically ok but not designed in a fashion that works best. — Michael Puckett II, Aug 13 '18 at 13:42
@MichaelPuckettII: You are creating `messages.Count` number of tasks and the `SemaphoreSlim` accepts `maxConcurrency` initial number of requests. — mm8, Aug 13 '18 at 13:48
@mm8 Yeah? So if the max is 10 and I send 20 then 10 are on hold until one after another they are released. At which point the foreach loop is on hold also. It'll never reach Task.WaitAll until they are all at least started. Once they are all started, yes he can wait for them to finish to prevent disposing or he could forget about it and just let it go. Put the dispose checks in the finally statement and be done with it. I don't know if there's a use case to wait around or not at this point. If waiting should be done then yes, he needs to use Task.WaitAll. — Michael Puckett II, Aug 13 '18 at 13:53
If you omit Task.WaitAll() then you have *two* problems. Yes, the Release() call is likely to bomb since the main thread motored on and disposed the semaphore. But worse, much worse. you won't notice. *Any* exception thrown by the code in the task goes unobserved. Code that fails randomly without a diagnostic is impossible to make reliable. — Hans Passant, Aug 13 '18 at 14:01

score 1 · Answer 1 · answered Aug 13 '18 at 14:00

Why is the task list needed?

To be able to call Task.WaitAll to wait for all tasks to finish before the SemaphoreSlim is disposed. If you don't do this, it's possible that the SemaphoreSlim is disposed before all tasks have finisihed and then you will get an ObjectDisposedException when any of the remaining tasks call concurrencySemaphore.Release().

You are creating messages.Count number of tasks and the SemaphoreSlim accepts maxConcurrency (10) initial number of requests. So if for example messages.Count equals to 5, all five tasks will be started and added to tasks more or less immediately before the SemaphoreSlim is disposed - unless you call Task.WaitAll.

Consider the following sample code where I have commented out the call to Task.WaitAll and replaced your call to Process with a call to Thread.Sleep:

int maxConcurrency = 10;
var messages = new List<string>() { "1", "2", "3" };
using (SemaphoreSlim concurrencySemaphore = new SemaphoreSlim(maxConcurrency))
{
    //List<Task> tasks = new List<Task>();
    foreach (var msg in messages)
    {
        concurrencySemaphore.Wait();

        var t = Task.Factory.StartNew(() =>
        {
            try
            {
                Thread.Sleep(5000);
            }
            finally
            {
                concurrencySemaphore.Release();
            }
        });
        //tasks.Add(t);
    }
    //Task.WaitAll(tasks.ToArray());
}

This code will throw an ObjectDisposedException for each task.

I cannot wait for all threads to complete. Can this semaphore method still be used?

If your tasks access any member of the SemaphoreSlim, you should not dispose it before you are sure that all tasks have finished. If you can't or don't want to wait for the tasks to complete, you should probably define the SemaphoreSlim as a private field of your class and implement the IDisposable interface.

SemaphoreSlim disposed before released?

1 Answers1