Atomicity in the Release method

[ErikPilsits-RJW]

I think you have an atomicity issue in the release method.

        public void Release(T idToUnlock)
        {
            InnerSemaphore semaphore;
            if (lockDictionary.TryGetValue(idToUnlock, out semaphore))
            {
                semaphore.Release();
                if (!semaphore.HasWaiters && lockDictionary.TryRemove(idToUnlock, out semaphore))
                    semaphore.Dispose();
            }
        }

There is a chance that between checking !semaphore.HasWaiters and removing the dictionary entry, that another thread could call Wait(). You would know this after the removal if the semaphore suddenly has waiters. At that point you could try to add the semaphore back to the dictionary, but there's a chance that yet another thread has called Wait() and created a new dictionary entry.

[coderdnewbie]

Erik,

This maybe an even better solution

#5

[ErikPilsits-RJW]

The issue I'm pointing out doesn't have to do with the GetOrAdd factory. In fact, this library doesn't use that overload of GetOrAdd, so the Lazy implementation isn't even needed.

The issue I'm describing is that the functionality of the Release method must be atomic. There's no way to do that with ConcurrentDictionary. I ended up taking this library as a base and reimplemented with a normal dictionary and an AsyncLock around the dictionary code in the Wait and Release methods. I also added a small IDisposable wrapper class as a return from the Wait methods, so you can use the locks in a using ( .. ) { } block.

[coderdnewbie]

Yes, I understand, and the information you stated above is correct, but I think the Microsoft faster team have a better solution as they are not doing any of that extra work that Darkseal kindly implemented.

I just think the Microsoft solution I linked in issue 5 is more elegant and Darkseal can simplify the existing lock provider code using SafeConcurrentDictionary and have a simpler solution.

I am just sharing some knowledge I found out recently.

[Darkseal]

Hi @ErikPilsits-RJW , thanks for sharing your thoughts: I think that I can switch this library with your implementation as well. Do you mind to share it?

Many thanks,

[ErikPilsits-RJW]

This is what I ended up with. It requires Nito.AsyncEx.Coordination for the async lock.

using Nito.AsyncEx;
using System;
using System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;

namespace Common.Synchronization
{
    public interface ILockProvider<T>
    {
        /// <summary>
        /// Blocks the current thread (according to the given ID) until it can enter the LockProvider.
        /// </summary>
        /// <param name="idToLock">The unique ID to perform the lock.</param>
        LockProvider<T>.DisposableLock Wait(T idToLock);

        /// <summary>
        /// Asynchronously puts thread to wait (according to the given ID) until it can enter the LockProvider.
        /// </summary>
        /// <param name="idToLock">The unique ID to perform the lock.</param>
        /// <param name="cancellationToken"></param>
        Task<LockProvider<T>.DisposableLock> WaitAsync(T idToLock, CancellationToken cancellationToken = default);

        /// <summary>
        /// Releases a lock (according to the given ID).
        /// </summary>
        /// <param name="idToUnlock">The unique ID to release the lock.</param>
        void Release(T idToUnlock);
    }

    /// <summary>
    /// A LockProvider based on the below project.
    /// ----------
    /// A LockProvider based upon the SemaphoreSlim class to selectively lock objects, resources or statement blocks 
    /// according to given unique IDs in a sync or async way.
    /// 
    /// SAMPLE USAGE & ADDITIONAL INFO:
    /// - https://www.ryadel.com/en/asp-net-core-lock-threads-async-custom-ids-lockprovider/
    /// - https://github.com/Darkseal/LockProvider/
    /// </summary>
    public class LockProvider<T> : ILockProvider<T>
    {
        private static readonly Dictionary<T, InnerSemaphore> LockDictionary = new Dictionary<T, InnerSemaphore>();
        // ReSharper disable once StaticMemberInGenericType
        private static readonly AsyncLock Lock = new AsyncLock();

        public DisposableLock Wait(T idToLock)
        {
            InnerSemaphore semaphore;

            using (Lock.Lock())
            {
                semaphore = GetOrCreate(idToLock);
                semaphore.Increment();
            }

            semaphore.Wait();

            return new DisposableLock(this, idToLock);
        }

        public async Task<DisposableLock> WaitAsync(T idToLock, CancellationToken cancellationToken = default)
        {
            InnerSemaphore semaphore;

            using (await Lock.LockAsync(cancellationToken).ConfigureAwait(false))
            {
                semaphore = GetOrCreate(idToLock);
                semaphore.Increment();
            }

            await semaphore.WaitAsync(cancellationToken).ConfigureAwait(false);

            return new DisposableLock(this, idToLock);
        }

        private InnerSemaphore GetOrCreate(T idToLock)
        {
            if (LockDictionary.TryGetValue(idToLock, out var sem)) return sem;

            var semaphore = new InnerSemaphore(1, 1);
            LockDictionary.Add(idToLock, semaphore);
            return semaphore;
        }

        public void Release(T idToUnlock)
        {
            using (Lock.Lock())
            {
                if (LockDictionary.TryGetValue(idToUnlock, out var semaphore))
                {
                    semaphore.Release();
                    if (!semaphore.HasWaiters && LockDictionary.Remove(idToUnlock))
                        semaphore.Dispose();
                }
            }
        }

        private class InnerSemaphore : IDisposable
        {
            private readonly SemaphoreSlim _semaphore;
            private int _waiters;
            private bool _disposedValue;

            public bool HasWaiters => _waiters > 0;

            public InnerSemaphore(int initialCount, int maxCount)
            {
                _semaphore = new SemaphoreSlim(initialCount, maxCount);
                _waiters = 0;
            }

            public void Wait()
            {
                _semaphore.Wait();
            }

            public Task WaitAsync(CancellationToken cancellationToken = default)
            {
                return _semaphore.WaitAsync(cancellationToken);
            }

            public void Increment()
            {
                _waiters++;
            }

            public void Release()
            {
                _waiters--;
                _semaphore.Release();
            }

            public void Dispose()
            {
                if (!_disposedValue)
                {
                    _semaphore?.Dispose();
                    _disposedValue = true;
                }
            }
        }

        /// <summary>
        /// An <see cref="IDisposable"/> wrapper around a lock taken from an <see cref="ILockProvider{T}"/>.
        /// </summary>
        public class DisposableLock : IDisposable
        {
            private readonly ILockProvider<T> _lockProvider;
            private readonly T _idToLock;

            private bool _disposedValue;

            public DisposableLock(ILockProvider<T> lockProvider, T idToLock)
            {
                _lockProvider = lockProvider;
                _idToLock = idToLock;
            }

            public void Dispose()
            {
                if (!_disposedValue)
                {
                    _lockProvider.Release(_idToLock);
                    _disposedValue = true;
                }
            }
        }
    }
}

[ErikPilsits-RJW]

I don't think that will work. The idea of the lock is to be multi-threaded, so there will be different threads calling the same idToLock. That's why there is a waiter counter.

Also, the thread id doesn't hold up when you start dealing with async tasks. See here.
https://neosmart.net/blog/2017/asynclock-an-asyncawait-friendly-locking-library-for-c-and-net/