AsyncLock

[MgSam]

Please add @stephentoub's excellent AsyncLock and async primitive friends into Core FX.

I searched and was surprised not to find this suggested before. Feel free to close this if it was and I just couldn't find it.

While it is true that Stephen Cleary has added this to his great AsyncEx library, I think this is a common enough need that it should be part of the framework itself.

[ichensky]

SemaphoreSlim already have implemented method: WaitAsync, that return Task.

[Clockwork-Muse]

@ichensky - The primary benefit of AsyncLock would be having an IDisposable implementation, so that unlocks are automatic at the end of a scope. Probably, you'd build it around a SemaphoreSlim, yes (although something making use of ValueTask might perform better).

Note, however, that although an async lock is useful/necessary in some situations, blind use makes it really easy to lead to deadlocks. For instance, if you have another async call inside the scope of an AsyncLock... the lock is retained for the duration of the suspension. Which is normally necessary, but is lengthening the time the lock is held, which is troublesome.

[MgSam]

@Clockwork-Muse I'd argue any locking can easily lead to deadlocks. Or race conditions.

Obviously, its far better to avoid writing to shared data structures with multi-threaded code, and if you do, use a framework type. But sometimes that's just not possible and you need to lock on something.

[Clockwork-Muse]

Oh, no, I agree, sometimes you really need to do something like that. For instance, I was messing with something that was doing multi-threaded writes to a shared TCP connection (in response to received messages, no less).

It's just that the way continuations work means that the potential for deadlock is much higher, because of the assumption of safety, and the types of resources being accessed.

[sakno]

It would be nice to add asynchronous version of ReaderWriterLockSlim and, probably, AutoResetEvent/ManualResetEvent. IMHO, these classes should have the same public methods as their blocking friends with one exception - lock acquisition methods are asynchronous and return Task. AsyncLock can provide abstraction over all asynchronous locks (not only exclusive lock) which allows to acquire and release the lock with using statement.

I would like to propose ready-to-use implementations of these locks as a proof of concept. AsyncExclusiveLock, AsyncReaderWriteLock, AsyncManualResetEvent and AsyncAutolResetEvent are asynchronous versions of Monitor, ReaderWriterLockSlim, ManualResetEvent and AutoResetEvent respectively. AsyncLock represents acquired asynchronous lock regardless of its type using extension methods.

var readerWriterLock = new AsyncReaderWriteLock();

using(await readerWriterLock.AcquireReadLock(CancellationToken.None))
{
  //reader
}

using(await readerWriterLock.AcquireWriteLock(TimeSpan.FromSeconds(1))
{
  //writer
}

using(await readerWriterLock.AcquireUpgradeableLock(TimeSpan.FromSeconds(1), CancellationToken.None)
{
  //code protected by upgradeable lock
}

var exclusiveLock = new AsyncExclusiveLock();

using(await exclusiveLock.AcquireLock(CancellationToken.None))
{
  //code protected by mutually exclusive lock
}

var sem = new SemaphoreSlim(0, 42);

using(await sem.AcquireLockAsync(CancellationToken.None))
{

}

During the implementation I found that usage of ValueTask was unnecessary. It seems to me that .NET team has come to the same conclusion during implementation of SemaphoreSlim. @Clockwork-Muse, let me explain why.

According with official doc:

A method may return an instance of this value type when it's likely that the result of its operation will be available synchronously

It means that a) ValueTask valuable in the context of async method declaration which is b) likely to be completed synchronously. Async method is implemented as state machine. AsyncValueTaskMethodBuilder is used if async method returns ValueTask. In this case, the builder will not allocate task instance on the heap if method has been completed synchronously, in contrast to Task-based method. For such kind of methods, AsyncTaskMethodBuilder is used instead. If the method completes synchronously then it calls SetResult but the Task instance was already allocated on the heap. As a result, the completed task instance is not cached and created for each call of async method.

Asynchronous lock acquisition methods in SemaphoreSlim and proposed implementations from my side do not use async method builders and handle lock contention more accurately. If there is no lock contention then cached completed task will be returned. Look at here, here and here and you'll see optimization that I'm talking about. If there is a lock contention then new instance of task will be allocated. Moreover, you need to have some kind of dynamic data structure to store queue of waiters. In case of SemaphoreSlim, it is linked list. Each node is allocated on the heap so there is no benefits to use ValueTask in case of lock contention.

[quinmars]

The advantage of ValueTask is that, unlike Task, it is not disposable, so you cannot accidently forget to await it.

using (locker.LockAsync())  // oops, Task is disposable
{
// ...
}

[RainingNight]

Any news?

[momvart]

After this long time passed since adding tasks, I want to add a vote to this issue to have synchronization primitives in task-based codes.
Also, it may be worth mentioning that the Visual Studio team has implemented this in Microsoft.VisualStudio.Threading.

[crozone]

No updates?

In the meantime I've just been using a small modification of Stephen Toub's version based around SemaphoreSlim:

public class AsyncLock
{
    private readonly SemaphoreSlim semaphore = new SemaphoreSlim(1, 1);
    private readonly Task<IDisposable> releaser;

    public AsyncLock()
    {
        releaser = Task.FromResult((IDisposable)new AsyncLockReleaser(this));
    }

    public Task<IDisposable> LockAsync(CancellationToken cancellationToken)
    {
        Task wait = semaphore.WaitAsync(cancellationToken);
        return wait.IsCompletedSuccessfully
            ? releaser
            : wait.ContinueWith(
                (_, state) => (IDisposable)state!,
                releaser.Result,
                CancellationToken.None,
                TaskContinuationOptions.ExecuteSynchronously,
                TaskScheduler.Default);
    }

    private sealed class AsyncLockReleaser : IDisposable
    {
        private readonly AsyncLock toRelease;

        internal AsyncLockReleaser(AsyncLock toRelease) {
            this.toRelease = toRelease;
        }

        public void Dispose()
        {
            toRelease.semaphore.Release();
        }
    }
}

It would be really nice if an equivalent was included in .NET given how commonly this pattern is used.

**EDIT: ** Changed IsCompleted to IsCompletedSuccessfully to fix case when cancellation token is already cancelled.

[molinch]

@crozone While the semaphore does not need to be disposed when we don't call AvailableWaitHandle, I believe that the finalizer will still kick-in which has minor performance impact.

[antmjones]

In the meantime I've just been using a small modification of Stephen Toub's version based around SemaphoreSlim:

This is broken for the case that the cancellation token is cancelled.

In the uncontented case, .IsCompleted will return true and the cached releaser task will be returned (rather than returning a cancelled task). In the contended case .ContinueWith(...) will execute the continuation function even if the task returned by semaphore.WaitAsync is in the cancelled state.

In both cases, this will lead to code being executed without the lock being held, and then when .Dispose() is called on the AsyncLockReleaser a SemaphoreFullException will be thrown.

[antmjones]

Further to my previous comment, I'd also add that it appears that many of the implementations of AsyncLock that are based on Stephen Toub's original (but with added support for cancellation) also have similar bugs. For example just by searching under the Microsoft organistation on Github:

https://github.com/microsoft/azure-sdk-for-net-ace/blob/3d338076b1e5802b88a808c500b9b3c97d880dc9/sdk/eventhub/Microsoft.Azure.EventHubs/src/Primitives/AsyncLock.cs#L55

https://github.com/microsoft/sqltoolsservice/blob/6864eb5d95fe0cff9bcf7ef82b8c9d2957039d64/src/Microsoft.SqlTools.Hosting/Utility/AsyncLock.cs#L64

https://github.com/microsoft/electionguard-core2/blob/ae1362bb8846f566ba817daae8fbf854d3b9569c/bindings/netstandard/ElectionGuard/ElectionGuard.ElectionSetup/Concurrency/AsyncLock.cs#L19

Orleans at least appears to get it correct (uses IsCompletedSuccessfully rather than IsCompleted and also avoids .ContinueWith):

https://github.com/dotnet/orleans/blob/deda4baa2892174d0ca2c9fb470f0082ec33a679/src/Orleans.Core/Async/AsyncLock.cs#L67

Although the Orleans implementation always allocates even on the successful case (presumably the motivation for having a separate LockReleaser object for each call is to make multiple calls to LockReleaser.Dispose safe).

All this seems to me to be evidence that it would be worth having a properly tested and supported primitive shipped with .NET.

[cremor]

@antmjones What do you think about https://github.com/BalassaMarton/AsyncExtensions by @BalassaMarton?
I've found it in StephenCleary/AsyncEx#265 (comment)

[antmjones]

@cremor For a start, personally I'm not keen on supporting reentrancy over async/await, for the reasons described here:

https://itnext.io/reentrant-recursive-async-lock-is-impossible-in-c-e9593f4aa38a

(and arguably it's a bad idea even in the non-async case, see https://blog.stephencleary.com/2013/04/recursive-re-entrant-locks.html)

Apart from that it's far more complex than the solutions based on Stephen Toub's original example (so more chance of bugs), and I personally wouldn't want to use any synchronization primitives described as "experimental"!

[BalassaMarton]

@antmjones my repo is not published, nor is production-ready, but might be useful for someone who's trying to build an async locking library. The key idea is the locking token which you can pass down to subsequent methods that need to take the lock.

[crozone]

In the meantime I've just been using a small modification of Stephen Toub's version based around SemaphoreSlim:

This is broken for the case that the cancellation token is cancelled.

In the uncontented case, .IsCompleted will return true and the cached releaser task will be returned (rather than returning a cancelled task). In the contended case .ContinueWith(...) will execute the continuation function even if the task returned by semaphore.WaitAsync is in the cancelled state.

Ahh of course. I think simply changing it to .IsCompletedSuccessfully would fix it?

[antmjones]

Using .IsCompletedSuccessfully or .Status == TaskStatus.RanToCompletion would fix the uncontended case, but you would also need to include TaskContinuationOptions.OnlyOnRanToCompletion in the call to .ContinueWith() or alternatively have a separate method to call in the contended case:

            Task wait = semaphore.WaitAsync(cancellationToken);

            static async Task<IDisposable> Await(Task semaphoreWaitTask, IDisposable result) {
                await semaphoreWaitTask.ConfigureAwait(false);
                return result;
            }

            Task<IDisposable> result = wait.IsCompletedSuccessfully ? releaser : Await(wait, releaser.Result);

Note that if using .ContinueWith I also believe that you don't want to pass the cancellation token as an argument to .ContinueWith, because once the task returned by semaphore.WaitAsync has completed successfully you always want to return a disposable or the semphore won't get released (i.e. at that point it's too late to throw an OperationCancelledException without leaking).

[idg10]

Just to add another vote towards building something of this kind into .NET, the prototype for AsyncRx.NET currently finds itself obliged to provide its own implementation of this type.

Worse, that implementation needs to be public. (This is because classic Rx.NET's Synchronize operators have overloads accepting any object, enabling applications to supply a single object that synchronizes the operation of any number of observables and observes, and also to synchronize application code too. To get equivalent functionality in AsyncRx.NET, there needs to be some type that can be passed in as the optional gate argument which a) AsyncRx.NET can use for locking purposes and b) which the application itself can also lock.)

We would really much rather not be in the business of defining thread synchronization primitives. We've already had requests to enhance the capability of our AsyncGate (and specifically to add cancellation, something that seems to trip up every project that attempts to solve this same problem for itself).

The recurrence of this problem (and the fact that so many projects get it wrong when they bring their own implementation) seems like an argument in favour of building such a thing into the .NET runtime libraries.

[julealgon]

Would be nice if the behavior was unified on top of the recently introduced Lock dedicated type:

• Add first class System.Threading.Lock type #34812