Asynchronous locking based on a key

As the other answerer noted, the original code is removing the SemaphoreSlim from the ConcurrentDictionary before it releases the semaphore. So, you've got too much semaphore churn going on - they're being removed from the dictionary when they could still be in use (not acquired, but already retrieved from the dictionary).

The problem with this kind of "mapping lock" is that it's difficult to know when the semaphore is no longer necessary. One option is to never dispose the semaphores at all; that's the easy solution, but may not be acceptable in your scenario. Another option - if the semaphores are actually related to object instances and not values (like strings) - is to attach them using ephemerons; however, I believe this option would also not be acceptable in your scenario.

So, we do it the hard way. :)

There are a few different approaches that would work. I think it makes sense to approach it from a reference-counting perspective (reference-counting each semaphore in the dictionary). Also, we want to make the decrement-count-and-remove operation atomic, so I just use a single lock (making the concurrent dictionary superfluous):

public sealed class AsyncDuplicateLock{  private sealed class RefCounted<T>  {    public RefCounted(T value)    {      RefCount = 1;      Value = value;    }    public int RefCount { get; set; }    public T Value { get; private set; }  }  private static readonly Dictionary<object, RefCounted<SemaphoreSlim>> SemaphoreSlims                        = new Dictionary<object, RefCounted<SemaphoreSlim>>();  private SemaphoreSlim GetOrCreate(object key)  {    RefCounted<SemaphoreSlim> item;    lock (SemaphoreSlims)    {      if (SemaphoreSlims.TryGetValue(key, out item))      {        ++item.RefCount;      }      else      {        item = new RefCounted<SemaphoreSlim>(new SemaphoreSlim(1, 1));        SemaphoreSlims[key] = item;      }    }    return item.Value;  }  public IDisposable Lock(object key)  {    GetOrCreate(key).Wait();    return new Releaser { Key = key };  }  public async Task<IDisposable> LockAsync(object key)  {    await GetOrCreate(key).WaitAsync().ConfigureAwait(false);    return new Releaser { Key = key };  }  private sealed class Releaser : IDisposable  {    public object Key { get; set; }    public void Dispose()    {      RefCounted<SemaphoreSlim> item;      lock (SemaphoreSlims)      {        item = SemaphoreSlims[Key];        --item.RefCount;        if (item.RefCount == 0)          SemaphoreSlims.Remove(Key);      }      item.Value.Release();    }  }}

For a given key,

1. Thread 1 calls GetOrAdd and adds a new semaphore and acquires it via Wait
2. Thread 2 calls GetOrAdd and gets the existing semaphore and blocks on Wait
3. Thread 1 releases the semaphore, only after having called TryRemove, which removed the semaphore from the dictionary
4. Thread 2 now acquires the semaphore.
5. Thread 3 calls GetOrAdd for the same key as thread 1 and 2. Thread 2 is still holding the semaphore, but the semaphore is not in the dictionary, so thread 3 creates a new semaphore and both threads 2 and 3 access the same protected resource.

You need to adjust your logic. The semaphore should only be removed from the dictionary when it has no waiters.

Here is one potential solution, minus the async part:

public sealed class AsyncDuplicateLock{    private class LockInfo    {        private SemaphoreSlim sem;        private int waiterCount;        public LockInfo()        {            sem = null;            waiterCount = 1;        }        // Lazily create the semaphore        private SemaphoreSlim Semaphore        {            get            {                var s = sem;                if (s == null)                {                    s = new SemaphoreSlim(0, 1);                    var original = Interlocked.CompareExchange(ref sem, null, s);                    // If someone else already created a semaphore, return that one                    if (original != null)                        return original;                }                return s;            }        }        // Returns true if successful        public bool Enter()        {            if (Interlocked.Increment(ref waiterCount) > 1)            {                Semaphore.Wait();                return true;            }            return false;        }        // Returns true if this lock info is now ready for removal        public bool Exit()        {            if (Interlocked.Decrement(ref waiterCount) <= 0)                return true;            // There was another waiter            Semaphore.Release();            return false;        }    }    private static readonly ConcurrentDictionary<object, LockInfo> activeLocks = new ConcurrentDictionary<object, LockInfo>();    public static IDisposable Lock(object key)    {        // Get the current info or create a new one        var info = activeLocks.AddOrUpdate(key,          (k) => new LockInfo(),          (k, v) => v.Enter() ? v : new LockInfo());        DisposableScope releaser = new DisposableScope(() =>        {            if (info.Exit())            {                // Only remove this exact info, in case another thread has                // already put its own info into the dictionary                ((ICollection<KeyValuePair<object, LockInfo>>)activeLocks)                  .Remove(new KeyValuePair<object, LockInfo>(key, info));            }        });        return releaser;    }    private sealed class DisposableScope : IDisposable    {        private readonly Action closeScopeAction;        public DisposableScope(Action closeScopeAction)        {            this.closeScopeAction = closeScopeAction;        }        public void Dispose()        {            this.closeScopeAction();        }    }}

I rewrote the @StephenCleary answer with this:

public sealed class AsyncLockList {    readonly Dictionary<object, SemaphoreReferenceCount> Semaphores = new Dictionary<object, SemaphoreReferenceCount>();    SemaphoreSlim GetOrCreateSemaphore(object key) {        lock (Semaphores) {            if (Semaphores.TryGetValue(key, out var item)) {                item.IncrementCount();            } else {                item = new SemaphoreReferenceCount();                Semaphores[key] = item;            }            return item.Semaphore;        }    }    public IDisposable Lock(object key) {        GetOrCreateSemaphore(key).Wait();        return new Releaser(Semaphores, key);    }    public async Task<IDisposable> LockAsync(object key) {        await GetOrCreateSemaphore(key).WaitAsync().ConfigureAwait(false);        return new Releaser(Semaphores, key);    }    sealed class SemaphoreReferenceCount {        public readonly SemaphoreSlim Semaphore = new SemaphoreSlim(1, 1);        public int Count { get; private set; } = 1;        [MethodImpl(MethodImplOptions.AggressiveInlining)]        public void IncrementCount() => Count++;        [MethodImpl(MethodImplOptions.AggressiveInlining)]        public void DecrementCount() => Count--;    }    sealed class Releaser : IDisposable {        readonly Dictionary<object, SemaphoreReferenceCount> Semaphores;        readonly object Key;        [MethodImpl(MethodImplOptions.AggressiveInlining)]        public Releaser(Dictionary<object, SemaphoreReferenceCount> semaphores, object key) {            Semaphores = semaphores;            Key = key;        }        public void Dispose() {            lock (Semaphores) {                var item = Semaphores[Key];                item.DecrementCount();                if (item.Count == 0)                    Semaphores.Remove(Key);                item.Semaphore.Release();            }        }    }}