Multithreading issue with semaphore

Question

I need to have the piece of code which allowed to execute only by 1 thread at the same time based on parameter key:

    private static readonly ConcurrentDictionary<string, SemaphoreSlim> Semaphores = new();

    private async Task<TModel> GetValueWithBlockAsync<TModel>(string valueKey, Func<Task<TModel>> valueAction)
    {
        var semaphore = Semaphores.GetOrAdd(valueKey, s => new SemaphoreSlim(1, 1));

        try
        {
            await semaphore.WaitAsync();

            return await valueAction();
        }
        finally
        {
            semaphore.Release(); // Exception here - System.ObjectDisposedException
            if (semaphore.CurrentCount > 0 && Semaphores.TryRemove(valueKey, out semaphore))
            {
                semaphore?.Dispose();
            }
        }
    }

Time to time I got the error:

The semaphore has been disposed. : System.ObjectDisposedException: The semaphore has been disposed.
   at System.Threading.SemaphoreSlim.CheckDispose()
   at System.Threading.SemaphoreSlim.Release(Int32 releaseCount)
   at Project.GetValueWithBlockAsync[TModel](String valueKey, Func`1 valueAction)

All cases that I can imagine here are thread safety. Please help, what case I missed?

You've got a race: one thread can get past the `semaphore.CurrentCount > 0` condition and then another thread calls `Semaphores.GetOrAdd` before the first thread is then able to call `Semaphores.TryRemove` — canton7, Apr 04 '22 at 13:00
Disposing locking primitives is normally more effort than it's worth. The general advice is not to, and that this is one of the times when you can lean on the finalizer. If you are creating lots of semaphores and you want to avoid this, you'll probably need a separate lock to protect the creation/disposal of your semaphores — canton7, Apr 04 '22 at 13:02
Note that even if you remove the `Dispose` call here, that still leaves you with a race where two threads with the same `valueKey` can end up running `valueAction` at the same time, after acquiring different locks. Thread 1 runs `semaphore.CurrentCount > 0` and it passes. Thread 2 calls `Semaphores.GetOrAdd` and gets the pre-existing semaphore. Thread 1 runs `Semaphores.TryRemove`. Thread 3 runs `Semaphores.GetOrAdd` and creates a new semaphore. Thread 2 and 3 are then calling `valueAction` on the same `valueKey`, after acquiring different locks — canton7, Apr 04 '22 at 13:08
There is of course a slightly different race here, where Thread 1 holds the lock and Thread 2 is waiting on the `WaitAsync()`. Thread 1 releases the lock, and then checks `semaphore.CurrentCount` before Thread 2 is able to acquire it. — canton7, Apr 04 '22 at 13:13
That's sad (case with - Thread 1 releases the lock, and then checks semaphore.CurrentCount before Thread 2 is able to acquire it). Probably better to choose another approach for such purpose. Thanks — DevForRest, Apr 04 '22 at 13:18
One approach is to have a `Dictionary` where `State` has a `SemaphoreSlim` and `int` members, and the whole dictionary is protected by a lock. At the beginning of `GetValueWithBlockAsync` you acquire the dictionary-wide lock, and get or add a new `State`, incrementing the `int` member to show that you have an interest in the SemaphoreSlim (then release the lock). When you come to release the `SemaphoreSlim`, you then acquire the dictionary-wide lock, and decrement the `int`. If it's zero, then noone else is interested in the SemaphoreSlim, and you can remove it. — canton7, Apr 04 '22 at 13:22
You can probably replace the dictionary-wide lock with a `ConcurrentDictionary` and clever use of `Interlocked.CompareExchange` and a loop, but thinking about it is making my head hurt — canton7, Apr 04 '22 at 13:23
How many different keys will you have? Perhaps you dont need to remove/dispose it? — Magnus, Apr 04 '22 at 14:03
Related: [Asynchronous locking based on a key](https://stackoverflow.com/questions/31138179/asynchronous-locking-based-on-a-key) / [ConcurrentDictionary GetOrAdd async](https://stackoverflow.com/questions/54117652/concurrentdictionary-getoradd-async) — Theodor Zoulias, Apr 04 '22 at 14:25

score 5 · Answer 1 · answered Apr 04 '22 at 13:01

You have a thread race here, where another task is trying to acquire the same semaphore, and acquires it when you Release - i.e. another thread is awaiting the semaphore.WaitAsync(). The check against CurrentCount is a race condition, and it could go either way depending on timing. The check for TryRemove is irrelevant, as the competing thread already got the semaphore out - it was, after all, awaiting the WaitAsync().

canton7 · Accepted Answer · 2022-04-04T14:54:42.837

As discussed in the comments, you have a couple of race conditions here.

Thread 1 holds the lock and Thread 2 is waiting on WaitAsync(). Thread 1 releases the lock, and then checks semaphore.CurrentCount, before Thread 2 is able to acquire it.
Thread 1 holds the lock, releases it, and checks semaphore.CurrentCount which passes. Thread 2 enters GetValueWithBlockAsync, calls Semaphores.GetOrAdd and fetches the semaphore. Thread 1 then calls Semaphores.TryRemove and diposes the semaphore.

You really need locking around the decision to remove an entry from Semaphores -- there's no way around this. You also don't have a way of tracking whether any threads have fetched a semaphore from Semaphores (and are either currently waiting on it, or haven't yet got to that point).

One way is to do something like this: have a lock which is shared between everyone, but which is only needed when fetching/creating a semaphore, and deciding whether to dispose it. We manually keep track of how many threads currently have an interest in a particular semaphore. When a thread has released the semaphore, it then acquires the shared lock to check whether anyone else currently has an interest in that semaphore, and disposes it only if noone else does.

private static readonly object semaphoresLock = new();
private static readonly Dictionary<string, State> semaphores = new();

private async Task<TModel> GetValueWithBlockAsync<TModel>(string valueKey, Func<Task<TModel>> valueAction)
{
    State state;
    lock (semaphoresLock)
    {
        if (!semaphores.TryGetValue(valueKey, out state))
        {
            state = new();
            semaphores[valueKey] = state;
        }
        
        state.Count++;
    }

    try
    {
        await state.Semaphore.WaitAsync();

        return await valueAction();
    }
    finally
    {
        state.Semaphore.Release();
        lock (semaphoresLock)
        {
            state.Count--;
            if (state.Count == 0)
            {
                semaphores.Remove(valueKey);
                state.Semaphore.Dispose();
            }
        }
    }
}

private class State
{
    public int Count { get; set; }
    public SemaphoreSlim Semaphore { get; } = new(1, 1);
}

The other option, of course, is to let Semaphores grow. Maybe you have a periodic operation to go through and clear out anything which isn't being used, but this will of course need to be protected to ensure that a thread doesn't suddenly become interested in a semaphore which is being cleared up.

Multithreading issue with semaphore

2 Answers2