Simple async lock using key in .NET

Question

The task is to make a simple LockProvider that will lock a thread by identifier. Basic requirements:

1. Simple call via using block.
2. Support for CancelationToken to get out of waiting without exceptions.
3. Maximum efficient use and release of resources.

Result:

public static class LockProvider
{
 private static readonly ConcurrentDictionary<object, AsyncLock> _locks = new();
 public static async Task<IDisposable> LockAsync(object key, CancellationToken ct = default)
 {
 var asyncLock = _locks.GetOrAdd(key, _ => new AsyncLock());
 var lockHandle = await asyncLock.AcquireAsync(ct).ConfigureAwait(false);
 return new AsyncLockRelease(() => Release(key, asyncLock, lockHandle));
 }
 private static void Release(object key, AsyncLock asyncLock, IDisposable lockHandle)
 {
 try
 {
 lockHandle.Dispose();
 if (asyncLock.IsFree)
 {
 _locks.TryRemove(key, out _);
 }
 }
 catch
 {
 // Ignore release errors.
 }
 }
 private class AsyncLock
 {
 private readonly SemaphoreSlim _semaphore = new(1, 1);
 private int _refCount;
 public bool IsFree => _refCount == 0;
 public async Task<IDisposable> AcquireAsync(CancellationToken ct)
 {
 await _semaphore.WaitAsync(ct).ConfigureAwait(false);
 Interlocked.Increment(ref _refCount);
 return new LockHandle(this);
 }
 private class LockHandle(AsyncLock parent) : IDisposable
 {
 private int _disposed;
 public void Dispose()
 {
 if (Interlocked.Exchange(ref _disposed, 1) == 0)
 {
 Interlocked.Decrement(ref parent._refCount);
 parent._semaphore.Release();
 }
 }
 }
 }
 private class AsyncLockRelease(Action release) : IDisposable
 {
 private int _disposed;
 public void Dispose()
 {
 if (Interlocked.Exchange(ref _disposed, 1) == 0)
 release();
 }
 }
}

And using:

using (await LockProvider.LockAsync("lockId", cancellationToken))
{
 // CONCURRENT WORK
}

Do you see any improvement / issue?

• Peter Csala: Next week I will have some free capacity and I will post a review. I have some improvement ideas.

Answer 1

The ConcurrenctDictionary

The key

In your example you have used object as a key, which can work, but you make it better. The CD does not force you that you should use a key implements the IComperable interface because it uses an IEqualityComparer to determine keys equality. The EqualityComparer<T>.Default uses the IEquatable<T>.Equals. That relies on the Equals and GetHashcode methods.

So, how I can make it better? You should perform null check before you start using it otherwise you will receive runtime errors for example at this line:

var asyncLock = _locks.GetOrAdd(key, _ => new AsyncLock());

If you want to constraint that the key should be always the same type, then you can make your LockProvider generic with a notnull constraint.

public static class LockProvider<TKey>
 where TKey : notnull
{
 private static readonly ConcurrentDictionary<TKey, AsyncLock> _locks = new();
 ...

Thread safety

I think it is worth calling out that the ConcurrentDictionary is thread-safe but there is no guarantee for the extension methods. In your implementation you just relied on the public members of the CD, but it is quite easy to introduce an unsafe method by using one of the IEnumerable's extension methods.

ConfigureAwait

If you are using the LockProvider in .NET 5 or later, or in .NET Core App, where there is no synchronization context, then setting this to false doesn't matter.

Nesting classes

I personally do not like these 3 levels of nested classes LockProvider >> AsyncLock >> LockHandle. Simply creating them separately and marking them as internal would be enough IMHO. I know, I know it can be considered as abstraction leaking and could be misused. BUT separate classes do not mean that you introduce unnecessary coupling. The coherence is still high. The classes are just more manageable IMHO.

Release method

Shallowing the release error does not seem a good idea. I don't see why we need at all here a try-catch block. Either remove it or log it and let it surface the issue.

Please also bear in mind that there is a race condition here. The check of the IsFree and removal of the lock from the dictionary are no one atomic step. So, if another thread acquires the lock after IsFree is checked BUT before the removal of the lock, you could remove a lock that is in use.

One way to solve this is to check against the item not just the key (by using another overload):

lockHandle.Dispose();
if (asyncLock.IsFree)
{
 _locks.TryRemove(new KeyValuePair<object, AsyncLock>(key, asyncLock));
}

It ensures that you remove the lock you released, not the one which is newly created by another thread.

LockHandle's Dispose

In your implementation, you decrement the reference counter and release the semaphore as separate steps. What if an exception occurs between these two operations? It could leave the lock in an inconsistent state. Moving the semaphore release into a finally block would solve this problem.

if (Interlocked.Exchange(ref _disposed, 1) == 0)
{
 try
 {
 Interlocked.Decrement(ref parent._refCount);
 }
 finally
 {
 parent._semaphore.Release();
 }
}

---

It might make sense to check the implementation of the AsnycEx's AsyncLock

• I'll add comments tomorrow: "If you are using .NET 5 or later [...] there is no synchronization context." - this is not true in general. It is for ASP .Net Core, but even then Cleary recommends "You Don’t Need ConfigureAwait(false), But Still Use It in Libraries" A LockProvider is a good candidate for library code. I'd suggest the alternative comment "If you are only using this LockProvider in an application type that has no synchronization context (e.g. ASP .Net Core), setting this false doesn't matter."
• Peter Csala: @I'lladdcommentstomorrow Fair point, updated the post.