4
\$\begingroup\$

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?

\$\endgroup\$
0

1 Answer 1

Reset to default
3
\$\begingroup\$

The ConcurrenctDictionary

The key

In your example you have used object as key, which can work, but you can make it better. The CD does not force you to use a key that 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 can we 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

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

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 not 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

\$\endgroup\$
2
  • \$\begingroup\$ "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." \$\endgroup\$ Commented Jun 6 at 20:56
  • \$\begingroup\$ @I'lladdcommentstomorrow Fair point, updated the post. \$\endgroup\$ Commented Jun 7 at 8:10

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.