487

The await keyword in C# (.NET Async CTP) is not allowed from within a lock statement.

From MSDN:

An await expression cannot be used in a synchronous function, in a query expression, in the catch or finally block of an exception handling statement, in the block of a lock statement, or in an unsafe context.

I assume this is either difficult or impossible for the compiler team to implement for some reason.

I attempted a work around with the using statement:

class Async
{
    public static async Task<IDisposable> Lock(object obj)
    {
        while (!Monitor.TryEnter(obj))
            await TaskEx.Yield();

        return new ExitDisposable(obj);
    }

    private class ExitDisposable : IDisposable
    {
        private readonly object obj;
        public ExitDisposable(object obj) { this.obj = obj; }
        public void Dispose() { Monitor.Exit(this.obj); }
    }
}

// example usage
using (await Async.Lock(padlock))
{
    await SomethingAsync();
}

However this does not work as expected. The call to Monitor.Exit within ExitDisposable.Dispose seems to block indefinitely (most of the time) causing deadlocks as other threads attempt to acquire the lock. I suspect the unreliability of my work around and the reason await statements are not allowed in lock statement are somehow related.

Does anyone know why await isn't allowed within the body of a lock statement?

Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104
Kevin
  • 8,312
  • 4
  • 27
  • 31
  • 3
    May I suggest this link: http://www.hanselman.com/blog/ComparingTwoTechniquesInNETAsynchronousCoordinationPrimitives.aspx and this one : http://blogs.msdn.com/b/pfxteam/archive/2012/02/12/10266988.aspx – hans Dec 25 '12 at 20:27
  • I'm just starting to catch up and learn a little more about async programming. After numerous deadlocks in my wpf applications, I found this article to be a great safe guard in async programming practices. https://msdn.microsoft.com/en-us/magazine/jj991977.aspx?f=255&MSPPError=-2147217396 – C. Tewalt Mar 28 '16 at 17:01
  • 3
    Lock is designed to prevent async access when async access would break your code, ergo if you are using async inside a lock your have invalidated your lock.. so if you need to await something inside your lock you are not using the lock correctly – MikeT Jan 08 '18 at 14:01
  • 1
    blogs.msdn.com/b/pfxteam/archive/2012/02/12/10266988.aspx is dead, I believe it is https://devblogs.microsoft.com/pfxteam/building-async-coordination-primitives-part-6-asynclock/ and https://devblogs.microsoft.com/pfxteam/building-async-coordination-primitives-part-7-asyncreaderwriterlock/ now – prime23 Dec 21 '21 at 06:39
  • Here you can find a possible solution for you: https://blog.cdemi.io/async-waiting-inside-c-sharp-locks/ – peter70 May 11 '22 at 10:04

9 Answers9

493

I assume this is either difficult or impossible for the compiler team to implement for some reason.

No, it is not at all difficult or impossible to implement -- the fact that you implemented it yourself is a testament to that fact. Rather, it is an incredibly bad idea and so we don't allow it, so as to protect you from making this mistake.

call to Monitor.Exit within ExitDisposable.Dispose seems to block indefinitely (most of the time) causing deadlocks as other threads attempt to acquire the lock. I suspect the unreliability of my work around and the reason await statements are not allowed in lock statement are somehow related.

Correct, you have discovered why we made it illegal. Awaiting inside a lock is a recipe for producing deadlocks.

I'm sure you can see why: arbitrary code runs between the time the await returns control to the caller and the method resumes. That arbitrary code could be taking out locks that produce lock ordering inversions, and therefore deadlocks.

Worse, the code could resume on another thread (in advanced scenarios; normally you pick up again on the thread that did the await, but not necessarily) in which case the unlock would be unlocking a lock on a different thread than the thread that took out the lock. Is that a good idea? No.

I note that it is also a "worst practice" to do a yield return inside a lock, for the same reason. It is legal to do so, but I wish we had made it illegal. We're not going to make the same mistake for "await".

Eric Lippert
  • 647,829
  • 179
  • 1,238
  • 2,067
  • 274
    How do you handle a scenario where you need to return a cache entry, and if the entry does not exist you need to compute asynchronously the content then add+return the entry, making sure nobody else calls you in the meantime ? – Softlion Aug 25 '12 at 05:32
  • 16
    I realise I'm late to the party here, however I was surprised to see that you put deadlocks as the primary reason why this is a bad idea. I had come to the conclusion in my own thinking that the re-entrant nature of lock/Monitor would be a bigger part of the problem. That is, you queue two tasks to the thread pool which lock(), that in a synchronous world would execute on separate threads. But now with await (if allowed I mean) you could have two tasks executing within the lock block because the thread was reused. Hilarity ensues. Or did I misunderstand something? – Gareth Wilson Dec 12 '12 at 22:56
  • 5
    @GarethWilson: I talked about deadlocks *because the question asked was about deadlocks*. You are correct that bizarre re-entrancy issues are possible and seem likely. – Eric Lippert Dec 12 '12 at 23:10
  • 1
    @EricLippert why a full set of coordination pimitives weren't added as well to .NET framework? There seems to exist AsyncSemaphore, AsyncManualResetEvent, but they are part of Microsoft.VisualStudio.Threading namespace. Also AsyncLock and and AsynAutoResetEvent are covered by Stephen Toub blog. Not having them in the core framework gives me the feeling I'm still missing something important in understanding async/await pattern. – ceztko Mar 12 '14 at 21:30
  • 16
    @Eric Lippert. Given that the `SemaphoreSlim.WaitAsync` class was added to the .NET framework well after you posted this answer, I think we can safely assume that it is possible now. Regardless of this, your comments about the difficulty of implementing such a construct are still entirely valid. – Contango Sep 12 '14 at 08:34
  • 16
    "arbitrary code runs between the time the await returns control to the caller and the method resumes" - surely this is true of any code, even in the absence of async/await, in a multithreaded context: other threads may execute arbitrary code at any time, and said arbitrary code as you say "could be taking out locks that produce lock ordering inversions, and therefore deadlocks." So why is this of particular significance with async/await? I understand the second point re "the code could resume on another thread" of being of particular significance to async/await. – bacar Sep 28 '16 at 22:12
  • 2
    @Contango `SemaphoreSlim` still allows you to do a lock inversion (ouch), but at least it doesn't have thread-affinity and reëntrancy. That at least means you don't care on what thread you do the "blocking". – Luaan Oct 10 '16 at 10:59
  • 1
    As @Contango mentioned, here is a good implementation i found: https://blog.cdemi.io/async-waiting-inside-c-sharp-locks – Rzassar Jan 29 '19 at 17:59
  • How do you solve write/delete/read data race with the same file with uwp api? In uwp I only have async file api, So I have to lock those file operation with aysnc api. So it is not a bad idea, it is the only way to solve this problem. – bronze man Jun 27 '19 at 13:04
  • 1
    Related: [What is a “lock ordering inversion”?](https://softwareengineering.stackexchange.com/questions/425016/what-is-a-lock-ordering-inversion) – OfirD Jun 17 '21 at 22:05
  • 1
    I've voted this answer down because I find it incredibly confusing. Eric talks about "arbitrary code" running in a way that makes it seem unique to async/await, but gives no worked example. I don't understand why this applies specifically to async/await at all, and it seems to me just a general point about needing to avoid code that produces deadlocks, not an argument for "don't use locks", which appears to be his argument. @EricLippert I urge you to edit this answer and make it a LOT clearer. Don't assume we know what you're talking about. – Jez Jan 29 '23 at 12:47
  • @Jez: A lock statement has a body. If there is no await in a lock body then the *only* code that can run *after* the lock is acquired and *before* it is released is the code *in the body*. If there were an await in the body, then *any async code in the program whatsoever* can run during the asynchronous wait. I'm not sure what a "worked example" would look like. – Eric Lippert Jan 30 '23 at 16:19
  • @Jez: If there's some concept about multithreaded code, locks, or asynchrony that you don't understand, maybe ask a question about that to clarify your understanding; this is a question-and-answer site after all. The answer was intended to inform the person asking the question, and it did. If it's not pitched at a level that you find valuable, well, that wasn't my intention in the first place so its not surprising that I didn't achieve it. – Eric Lippert Jan 30 '23 at 16:20
  • 1
    Yes, but what's the precise problem with that "any async code in the program whatsoever can run during the asynchronous wait"? Other code running that wishes to get the lock for the current lock body will have to block until it's released, The worked example I'd like is a small code example, followed by an order of events of 2 threads that would produce deadlock under an async context but not under a non-async context, and the "why" clearly explained (for the sake of the example, assume that when code execution resumes, it is always picked up on the same thread). – Jez Jan 31 '23 at 00:43
  • @Jez: That sounds like a great question to ask on Stack Overflow. My answers don't come with a terms of service level agreement I'm afraid. – Eric Lippert Jan 31 '23 at 16:11
433

Use the SemaphoreSlim.WaitAsync method.

 await mySemaphoreSlim.WaitAsync();
 try {
     await Stuff();
 } finally {
     mySemaphoreSlim.Release();
 }
Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104
user1639030
  • 4,377
  • 1
  • 13
  • 5
  • 21
    As this method was introduced into the .NET framework recently, I think we can assume that the concept of locking in an async/await world is now well proven. – Contango Sep 12 '14 at 08:41
  • Upvoted, but is awaiting within the `try` this such a good idea? From my understanding, if `Stuff` is something long running like making an HTTP request, it could hold the semaphore for seconds. In the meantime, other tasks calling `WaitAsync` would be unable to continue. While this might not cause responsiveness issues because they yield immediately to the caller, there would be a bunch of tasks just doing nothing for a long time. – James Ko Nov 04 '16 at 22:40
  • 8
    For some more info, search for the text "SemaphoreSlim" in this article: [Async/Await - Best Practices in Asynchronous Programming](https://msdn.microsoft.com/en-us/magazine/jj991977.aspx) – BobbyA Jun 23 '17 at 20:28
  • 2
    @JamesKo if all those tasks are waiting for the result of `Stuff` I don't see any way around it... – Ohad Schneider Jun 29 '17 at 15:50
  • @Contango no a semaphore and a lock are not the same thing, A lock uses a semaphore internally to operated the lock which has lead to people misusing them as a semaphore, but a lock should NEVER contain async code – MikeT Jan 08 '18 at 14:11
  • 32
    Shouldn't it be initialized as `mySemaphoreSlim = new SemaphoreSlim(1, 1)` in order to work like `lock(...)`? – Serg May 02 '18 at 16:14
  • 5
    Added the extended version of this answer: https://stackoverflow.com/a/50139704/1844247 – Serg May 02 '18 at 16:53
  • 2
    @JamesKo that's the whole point of using a semaphore as a mutex. Only one task should execute Stuff, therefore all other tasks have to wait for the previous task to release lock and take their turn into acquiring it one by one. It's like a bunch of people waiting outside the toilet to use it (having one toilet only). When they say to their friends "i'm going to the toilet" it doesn't mean they go and come back instantly. It means they go, wait for all others to finish, then use the toilet and come back. – Thanasis Ioannidis Jun 22 '18 at 11:35
  • 1
    Awaiting semaphore outside `try` block can be dangerous, see the following thread: https://stackoverflow.com/a/61806749/7889645 – AndreyCh May 14 '20 at 20:30
  • @Serg and others using `mySemaphoreSlim = new SemaphoreSlim(1, 1)` assuming it works like `lock(...)`...it is close but not a perfect replacement because the same thread can call `lock(...)` multiple times without blocking (because monitors/mutexes are re-entrant) but you CAN'T do that with a semaphore that has a count of 1 (first call would succeed and reduce the count to 0, then subsequent calls would block). – Eric Mutta May 19 '23 at 12:08
86

This is just an extension to this answer by user1639030.


Basic Version


using System;
using System.Threading;
using System.Threading.Tasks;

public class SemaphoreLocker
{
    private readonly SemaphoreSlim _semaphore = new SemaphoreSlim(1, 1);

    public async Task LockAsync(Func<Task> worker)
    {
        await _semaphore.WaitAsync();
        try
        {
            await worker();
        }
        finally
        {
            _semaphore.Release();
        }
    }

    // overloading variant for non-void methods with return type (generic T)
    public async Task<T> LockAsync<T>(Func<Task<T>> worker)
    {
        await _semaphore.WaitAsync();
        try
        {
            return await worker();
        }
        finally
        {
            _semaphore.Release();
        }
    }
}

Usage:

public class Test
{
    private static readonly SemaphoreLocker _locker = new SemaphoreLocker();

    public async Task DoTest()
    {
        await _locker.LockAsync(async () =>
        {
            // [async] calls can be used within this block 
            // to handle a resource by one thread. 
        });
        // OR
        var result = await _locker.LockAsync(async () =>
        {
            // [async] calls can be used within this block 
            // to handle a resource by one thread. 
        });
    }
}

Extended Version


A version of the LockAsync method that claims to be completely deadlock-safe (from the 4th revision suggested by Jez).

using System;
using System.Threading;
using System.Threading.Tasks;

public class SemaphoreLocker
{
    private readonly SemaphoreSlim _semaphore = new SemaphoreSlim(1, 1);

    public async Task LockAsync(Func<Task> worker)
    {
        var isTaken = false;
        try
        {
            do
            {
                try
                {
                }
                finally
                {
                    isTaken = await _semaphore.WaitAsync(TimeSpan.FromSeconds(1));
                }
            }
            while (!isTaken);
            await worker();
        }
        finally
        {
            if (isTaken)
            {
                _semaphore.Release();
            }
        }
    }

    // overloading variant for non-void methods with return type (generic T)
    public async Task<T> LockAsync<T>(Func<Task<T>> worker)
    {
        var isTaken = false;
        try
        {
            do
            {
                try
                {
                }
                finally
                {
                    isTaken = await _semaphore.WaitAsync(TimeSpan.FromSeconds(1));
                }
            }
            while (!isTaken);
            return await worker();
        }
        finally
        {
            if (isTaken)
            {
                _semaphore.Release();
            }
        }
    }
}

Usage:

public class Test
{
    private static readonly SemaphoreLocker _locker = new SemaphoreLocker();

    public async Task DoTest()
    {
        await _locker.LockAsync(async () =>
        {
            // [async] calls can be used within this block 
            // to handle a resource by one thread. 
        });
        // OR
        var result = await _locker.LockAsync(async () =>
        {
            // [async] calls can be used within this block 
            // to handle a resource by one thread. 
        });
    }
}
Serg
  • 6,742
  • 4
  • 36
  • 54
  • 5
    It can be dangerous to get the semaphore lock outside the `try` block - if an exception happens between `WaitAsync` and `try` the semaphore will be never released (deadlock). On the other hand, moving `WaitAsync` call into the `try` block will introduce another issue, when the semaphore can be released without a lock being acquired. See the related thread where this problem was explained: https://stackoverflow.com/a/61806749/7889645 – AndreyCh May 14 '20 at 20:24
  • I can't believe this actually helped me. Huge thanks to this answer. The only thing I should add is that you should add a generic type so if someone needs to "get a value from an async method" he will be able to use this. `Task LockAsync(Func> worker)` ... and then you assign the return value as T result = default; then in the try you write result = await worker(); and after the finally block you return result; It's simple, but not everyone knows how to deal with generics, Func,Task types, etc.Still great answer though. If you have time, add the return functionality. Thanks again – Nikolai Jun 29 '20 at 13:16
  • @Nikolai Do you mean to add a second generic method in addition to the current one? – Serg Jun 29 '20 at 13:59
  • @Sergey Yes. It's hard to explain in a comment. I will show you what I needed: `Skill = await locker.LockAsync(async () => { return await skillRepository.GetByIdAsync(skill.Id); });` And I basically needed to add a generic type so the LockAsync to return the result from the async method. As I said I knew how to "tweak" your method and it worked like a charm. Many people will need something similar and it would be nice to have both of the solutions - for Task void calls and Task with returned value of type T. – Nikolai Jun 29 '20 at 14:46
  • @Nikolai Sounds reasonable, but I haven't used Semaphore for a while and therefore I'm a bit out of this context. If you're sure, you're welcome to edit my answer and add your function, if you'd like to. – Serg Jun 29 '20 at 16:10
  • @Sergey Done. Check the edit. It's not that related to semaphore. It's related to retrieving the type from the function we're locking, because you're solution is great, but only works if the function we're locking is void and I just added an overloaded version where we can get also the returned value. It's pretty simple, but nice addition I think. – Nikolai Jun 30 '20 at 09:30
  • 3
    @Nikolai thank you for participating! You're right, but I haven't been using `async`/`await` as well for more than a year since I've shifted my technology stack a bit. By the way, what do you think of AndreyCh's comment? I really didn't have time to go into his remark and say anything about it. – Serg Jun 30 '20 at 16:48
  • @Sergey It's probably really reasonable, but I'm not an expert with Semaphore and I can't make the proper tests to enforce deadlock scenarios. I will try his solution tomorrow, but for now I used your solution and it worked like a charm. I'm using your solution for the last 2 days and I haven't had any deadlock issues yet. – Nikolai Jun 30 '20 at 23:55
  • add `void Lock(Action worker)` and `T Lock(Func worker)` to make it work for regular functions. – Tonghua Aug 11 '22 at 03:01
  • @Serg best I can tell from the comments at the answer @AndrewCh references, the possible `ThreadAbortException` isn't a concern with recent non-framework .Net versions – John Cummings Feb 15 '23 at 22:18
  • @JohnCummings It may not be much of a concern with recent framework versions but it's relatively easy to put in there for good measure, anyway. I edited the answer to put it in. – Jez Feb 25 '23 at 16:58
  • 2
    @Jez I think that your modification ([4th revision](https://stackoverflow.com/revisions/50139704/4)) deviates too much from the OP's original answer. I would suggest to post it as a separate answer, so that it can be evaluated (voted) on its own merit. – Theodor Zoulias Feb 25 '23 at 18:32
  • @TheodorZoulias Who made you judge? I don't think it deviates too much. – Jez Feb 25 '23 at 18:52
  • 1
    @Jez ([your version](https://stackoverflow.com/revisions/50139704/4)) has a lot more code, that exists for not obvious reasons, and introduces not obvious performance implications. And is accompanied by zero explanation/documentation. This is a five year old answer, that has been evaluated positively for its usefulness and its simplicity. Had it been authored from the start as complex as you suggest, it wouldn't have a tenth of its current upvotes. You are welcome to prove me wrong by posting your suggestion as a separate answer. – Theodor Zoulias Feb 25 '23 at 19:05
  • @Jez I added a link to your (presumably) deadlock-safe `LockAsync` version inside the question, so that people who need it can find it more easily. – Theodor Zoulias Feb 26 '23 at 08:36
  • 1
    @TheodorZoulias I've posted my own answer as I extended it further to be usable either synchronously or asynchronously. Nice to be able to just always use this instead of `lock` I think. – Jez Feb 26 '23 at 13:22
  • @Jez to be honest I am not a fan of the `Task LockAsync(Func worker)` pattern, because it causes captured variables (closures) and allocations. I prefer slightly the `Task LockAsync()` pattern, in combination with the `using` statement, or simply a bare-bone `SemaphoreSlim(1,1)` with the `WaitAsync`/`try`/`finally { semaphore.Release(); }` pattern. It's not that much code anyway. – Theodor Zoulias Feb 26 '23 at 14:30
  • What's the problem with that, just reduced performance? – Jez Feb 26 '23 at 15:58
  • 1
    Guys, to stop the comment war, I suggest publishing both points of view, leaving the choice to the reader. – Serg Feb 27 '23 at 12:27
80

Basically it would be the wrong thing to do.

There are two ways this could be implemented:

  • Keep hold of the lock, only releasing it at the end of the block.
    This is a really bad idea as you don't know how long the asynchronous operation is going to take. You should only hold locks for minimal amounts of time. It's also potentially impossible, as a thread owns a lock, not a method - and you may not even execute the rest of the asynchronous method on the same thread (depending on the task scheduler).

  • Release the lock in the await, and reacquire it when the await returns
    This violates the principle of least astonishment IMO, where the asynchronous method should behave as closely as possible like the equivalent synchronous code - unless you use Monitor.Wait in a lock block, you expect to own the lock for the duration of the block.

So basically there are two competing requirements here - you shouldn't be trying to do the first here, and if you want to take the second approach you can make the code much clearer by having two separated lock blocks separated by the await expression:

// Now it's clear where the locks will be acquired and released
lock (foo)
{
}
var result = await something;
lock (foo)
{
}

So by prohibiting you from awaiting in the lock block itself, the language is forcing you to think about what you really want to do, and making that choice clearer in the code that you write.

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • 5
    Given that the `SemaphoreSlim.WaitAsync` class was added to the .NET framework well after you posted this answer, I think we can safely assume that it is possible now. Regardless of this, your comments about the difficulty of implementing such a construct are still entirely valid. – Contango Sep 12 '14 at 08:37
  • 9
    @Contango: Well that's not *quite* the same thing. In particular, the semaphore isn't tied to a specific thread. It achieves similar goals to lock, but there are significant differences. – Jon Skeet Sep 12 '14 at 12:43
  • @JonSkeet i know this is a veryyy old thread and all, but i am not sure how the something() call is protected using those locks in the second way ? when a thread is executing something() any other thread can get involve in it as well ! Am i missing something here ? –  Jul 11 '19 at 09:10
  • @Joseph: It's not protected at that point. It's the second approach, which makes it clear that you're acquiring/releasing, then acquiring/releasing again, possibly on a different thread. Because the first approach is a bad idea, as per Eric's answer. – Jon Skeet Jul 11 '19 at 09:17
19

This referes to Building Async Coordination Primitives, Part 6: AsyncLock , http://winrtstoragehelper.codeplex.com/ , Windows 8 app store and .net 4.5

Here is my angle on this:

The async/await language feature makes many things fairly easy but it also introduces a scenario that was rarely encounter before it was so easy to use async calls: reentrance.

This is especially true for event handlers, because for many events you don't have any clue about whats happening after you return from the event handler. One thing that might actually happen is, that the async method you are awaiting in the first event handler, gets called from another event handler still on the same thread.

Here is a real scenario I came across in a windows 8 App store app: My app has two frames: coming into and leaving from a frame I want to load/safe some data to file/storage. OnNavigatedTo/From events are used for the saving and loading. The saving and loading is done by some async utility function (like http://winrtstoragehelper.codeplex.com/). When navigating from frame 1 to frame 2 or in the other direction, the async load and safe operations are called and awaited. The event handlers become async returning void => they cant be awaited.

However, the first file open operation (lets says: inside a save function) of the utility is async too and so the first await returns control to the framework, which sometime later calls the other utility (load) via the second event handler. The load now tries to open the same file and if the file is open by now for the save operation, fails with an ACCESSDENIED exception.

A minimum solution for me is to secure the file access via a using and an AsyncLock.

private static readonly AsyncLock m_lock = new AsyncLock();
...

using (await m_lock.LockAsync())
{
    file = await folder.GetFileAsync(fileName);
    IRandomAccessStream readStream = await file.OpenAsync(FileAccessMode.Read);
    using (Stream inStream = Task.Run(() => readStream.AsStreamForRead()).Result)
    {
        return (T)serializer.Deserialize(inStream);
    }
}

Please note that his lock basically locks down all file operation for the utility with just one lock, which is unnecessarily strong but works fine for my scenario.

Here is my test project: a windows 8 app store app with some test calls for the original version from http://winrtstoragehelper.codeplex.com/ and my modified version that uses the AsyncLock from Stephen Toub.

May I also suggest this link: http://www.hanselman.com/blog/ComparingTwoTechniquesInNETAsynchronousCoordinationPrimitives.aspx

prime23
  • 3,362
  • 2
  • 36
  • 52
hans
  • 1,001
  • 1
  • 11
  • 14
11

Stephen Taub has implemented a solution to this question, see Building Async Coordination Primitives, Part 7: AsyncReaderWriterLock.

Stephen Taub is highly regarded in the industry, so anything he writes is likely to be solid.

I won't reproduce the code that he posted on his blog, but I will show you how to use it:

/// <summary>
///     Demo class for reader/writer lock that supports async/await.
///     For source, see Stephen Taub's brilliant article, "Building Async Coordination
///     Primitives, Part 7: AsyncReaderWriterLock".
/// </summary>
public class AsyncReaderWriterLockDemo
{
    private readonly IAsyncReaderWriterLock _lock = new AsyncReaderWriterLock(); 

    public async void DemoCode()
    {           
        using(var releaser = await _lock.ReaderLockAsync()) 
        { 
            // Insert reads here.
            // Multiple readers can access the lock simultaneously.
        }

        using (var releaser = await _lock.WriterLockAsync())
        {
            // Insert writes here.
            // If a writer is in progress, then readers are blocked.
        }
    }
}

If you want a method that's baked into the .NET framework, use SemaphoreSlim.WaitAsync instead. You won't get a reader/writer lock, but you will get tried and tested implementation.

Pang
  • 9,564
  • 146
  • 81
  • 122
Contango
  • 76,540
  • 58
  • 260
  • 305
  • I'm curious to know if there are any caveats to using this code. If anyone can demonstrate any issues with this code, I'd like to know. However, what is true is that the concept of async/await locking is definitely well proven, as `SemaphoreSlim.WaitAsync` is in the .NET framework. All this code does is add a reader/writer lock concept. – Contango Sep 12 '14 at 08:40
  • 1
    A reader/writer concept is pretty useful, though. This sort of stuff should be baked into .NET. Isn't the whole reason for having a framework so that you can rely on it to provide you with useful stuff like these kinds of primitives? – Jez Feb 28 '23 at 10:12
1

Hmm, looks ugly, seems to work.

static class Async
{
    public static Task<IDisposable> Lock(object obj)
    {
        return TaskEx.Run(() =>
            {
                var resetEvent = ResetEventFor(obj);

                resetEvent.WaitOne();
                resetEvent.Reset();

                return new ExitDisposable(obj) as IDisposable;
            });
    }

    private static readonly IDictionary<object, WeakReference> ResetEventMap =
        new Dictionary<object, WeakReference>();

    private static ManualResetEvent ResetEventFor(object @lock)
    {
        if (!ResetEventMap.ContainsKey(@lock) ||
            !ResetEventMap[@lock].IsAlive)
        {
            ResetEventMap[@lock] =
                new WeakReference(new ManualResetEvent(true));
        }

        return ResetEventMap[@lock].Target as ManualResetEvent;
    }

    private static void CleanUp()
    {
        ResetEventMap.Where(kv => !kv.Value.IsAlive)
                     .ToList()
                     .ForEach(kv => ResetEventMap.Remove(kv));
    }

    private class ExitDisposable : IDisposable
    {
        private readonly object _lock;

        public ExitDisposable(object @lock)
        {
            _lock = @lock;
        }

        public void Dispose()
        {
            ResetEventFor(_lock).Set();
        }

        ~ExitDisposable()
        {
            CleanUp();
        }
    }
}
Anton Pogonets
  • 1,162
  • 7
  • 16
0

I created a MutexAsyncable class, inspired by Stephen Toub's AsyncLock implementation (discussion at this blog post), which can be used as a drop-in replacement for a lock statement in either sync or async code:

using System;
using System.Threading;
using System.Threading.Tasks;

namespace UtilsCommon.Lib;

/// <summary>
/// Class that provides (optionally async-safe) locking using an internal semaphore.
/// Use this in place of a lock() {...} construction.
/// Bear in mind that all code executed inside the worker must finish before the next
/// thread is able to start executing it, so long-running code should be avoided inside
/// the worker if at all possible.
///
/// Example usage for sync:
/// using (mutex.LockSync()) {
///     // ... code here which is synchronous and handles a shared resource ...
///     return[ result];
/// }
///
/// ... or for async:
/// using (await mutex.LockAsync()) {
///     // ... code here which can use await calls and handle a shared resource ...
///     return[ result];
/// }
/// </summary>
public sealed class MutexAsyncable {
    #region Internal classes

    private sealed class Releaser : IDisposable {
        private readonly MutexAsyncable _toRelease;
        internal Releaser(MutexAsyncable toRelease) { _toRelease = toRelease; }
        public void Dispose() { _toRelease._semaphore.Release(); }
    }

    #endregion

    private readonly SemaphoreSlim _semaphore = new(1, 1);
    private readonly Task<IDisposable> _releaser;

    public MutexAsyncable() {
        _releaser = Task.FromResult((IDisposable)new Releaser(this));
    }

    public IDisposable LockSync() {
        _semaphore.Wait();
        return _releaser.Result;
    }

    public Task<IDisposable> LockAsync() {
        var wait = _semaphore.WaitAsync();
        if (wait.IsCompleted) { return _releaser; }
        else {
            // Return Task<IDisposable> which completes once WaitAsync does
            return wait.ContinueWith(
                (_, state) => (IDisposable)state!,
                _releaser.Result,
                CancellationToken.None,
                TaskContinuationOptions.ExecuteSynchronously,
                TaskScheduler.Default
            );
        }
    }
}

It's safe to use the above if you're using .NET 5+ because that won't ever throw ThreadAbortException.

I also created an extended SemaphoreLocker class, inspired by this answer, which can be a general-purpose replacement for lock, usable either synchronously or asynchronously. It is less efficient than the above MutexAsyncable and allocates more resources, although it has the benefit of forcing the worker code to release the lock once it's finished (technically, the IDisposable returned by the MutexAsyncable could not get disposed by calling code and cause deadlock). It also has extra try/finally code to deal with the possibility of ThreadAbortException, so should be usable in earlier .NET versions:

using System;
using System.Threading;
using System.Threading.Tasks;

namespace UtilsCommon.Lib;

/// <summary>
/// Class that provides (optionally async-safe) locking using an internal semaphore.
/// Use this in place of a lock() {...} construction.
/// Bear in mind that all code executed inside the worker must finish before the next thread is able to
/// start executing it, so long-running code should be avoided inside the worker if at all possible.
///
/// Example usage:
/// [var result = ]await _locker.LockAsync(async () => {
///     // ... code here which can use await calls and handle a shared resource one-thread-at-a-time ...
///     return[ result];
/// });
///
/// ... or for sync:
/// [var result = ]_locker.LockSync(() => {
///     // ... code here which is synchronous and handles a shared resource one-thread-at-a-time ...
///     return[ result];
/// });
/// </summary>
public sealed class SemaphoreLocker : IDisposable {
    private readonly SemaphoreSlim _semaphore = new(1, 1);

    /// <summary>
    /// Runs the worker lambda in a locked context.
    /// </summary>
    /// <typeparam name="T">The type of the worker lambda's return value.</typeparam>
    /// <param name="worker">The worker lambda to be executed.</param>
    public T LockSync<T>(Func<T> worker) {
        var isTaken = false;
        try {
            do {
                try {
                }
                finally {
                    isTaken = _semaphore.Wait(TimeSpan.FromSeconds(1));
                }
            }
            while (!isTaken);
            return worker();
        }
        finally {
            if (isTaken) {
                _semaphore.Release();
            }
        }
    }

    /// <inheritdoc cref="LockSync{T}(Func{T})" />
    public void LockSync(Action worker) {
        var isTaken = false;
        try {
            do {
                try {
                }
                finally {
                    isTaken = _semaphore.Wait(TimeSpan.FromSeconds(1));
                }
            }
            while (!isTaken);
            worker();
        }
        finally {
            if (isTaken) {
                _semaphore.Release();
            }
        }
    }

    /// <summary>
    /// Runs the worker lambda in an async-safe locked context.
    /// </summary>
    /// <typeparam name="T">The type of the worker lambda's return value.</typeparam>
    /// <param name="worker">The worker lambda to be executed.</param>
    public async Task<T> LockAsync<T>(Func<Task<T>> worker) {
        var isTaken = false;
        try {
            do {
                try {
                }
                finally {
                    isTaken = await _semaphore.WaitAsync(TimeSpan.FromSeconds(1));
                }
            }
            while (!isTaken);
            return await worker();
        }
        finally {
            if (isTaken) {
                _semaphore.Release();
            }
        }
    }

    /// <inheritdoc cref="LockAsync{T}(Func{Task{T}})" />
    public async Task LockAsync(Func<Task> worker) {
        var isTaken = false;
        try {
            do {
                try {
                }
                finally {
                    isTaken = await _semaphore.WaitAsync(TimeSpan.FromSeconds(1));
                }
            }
            while (!isTaken);
            await worker();
        }
        finally {
            if (isTaken) {
                _semaphore.Release();
            }
        }
    }

    /// <summary>
    /// Releases all resources used by the current instance of the SemaphoreLocker class.
    /// </summary>
    public void Dispose() {
        _semaphore.Dispose();
    }
}
Jez
  • 27,951
  • 32
  • 136
  • 233
  • The code contains some long comments that cause the appearance of the horizontal scrollbar, making it hard to read the code. I would suggest shortening the comments by adding a few line-breaks. – Theodor Zoulias Feb 26 '23 at 14:16
  • I would suggest adding `Configure.Await(false)` in the `await worker()`. Also explaining the reason for the `TimeSpan.FromSeconds(1)` and the mysterious empty `try/finally` blocks would be nice. Some cautionary note about the performance implications of the `TimeSpan.FromSeconds(1)`, for example in case there are thousands of execution flows suspended on an `await LockAsync` command, would be nice too. – Theodor Zoulias Feb 26 '23 at 14:20
  • Why would there be thousands of suspended execution flows? Sounds to me like your program would have bigger problems in that case. – Jez Feb 26 '23 at 16:01
  • Thousands might be an exaggeration. Several dozens is more plausible, for example for synchronizing the enumeration of the `source` in a `Parallel.ForEachAsync` [implementation](https://github.com/dotnet/runtime/blob/eff44b870be6dd091d04d0a65a326431c9e9948b/src/libraries/System.Threading.Tasks.Parallel/src/System/Threading/Tasks/Parallel.ForEachAsync.cs#L98). The point is that you are offering the `SemaphoreLocker` as a general-purpose tool, without mentioning its unusual performance characteristics. People who think that they can use it as freely as a `SemaphoreSlim`, might get into trouble. – Theodor Zoulias Feb 26 '23 at 19:37
  • I guess you'd recommend AsyncEx's `AsyncLock` as a better substitute? – Jez Feb 26 '23 at 21:42
  • No. IMHO the AsyncEx library is a bit dated. It's also not optimized for performance, which makes it a bit meh. – Theodor Zoulias Feb 26 '23 at 21:52
  • So what the heck would you recommend then? – Jez Feb 27 '23 at 01:43
  • A [plain vanilla](https://stackoverflow.com/questions/75570920/what-are-the-differences-between-asyncex-asynclock-and-scott-hanselmans-asynclo/75575557#75575557 "What are the differences between AsyncEx.AsyncLock and Scott Hanselman's AsyncLock?") `SemaphoreSlim(1, 1)`. Whether you agree with my preference has nothing to do with the fact that your answer currently contains obscure code without any explanation. Please consider improving it. – Theodor Zoulias Feb 27 '23 at 05:55
  • I notice, by the way, that your example code doesn't dispose of the `SemaphoreSlim` although it implements `IDisposable`. Does it really need to be disposed? It's rather annoying as it might be long-lived and unsuitable for a `using` statement. – Jez Feb 27 '23 at 12:05
  • [Do I need to Dispose a SemaphoreSlim?](https://stackoverflow.com/questions/32033416/do-i-need-to-dispose-a-semaphoreslim) In general I don't have trouble disposing the `SemaphoreSlim`s I create, but it depends on the scenario I guess. – Theodor Zoulias Feb 27 '23 at 12:23
  • How come your example above doesn't then? And nor does Stephen Toub's `AsyncLock` implementation? – Jez Feb 27 '23 at 12:40
  • My [example](https://stackoverflow.com/questions/75570920/what-are-the-differences-between-asyncex-asynclock-and-scott-hanselmans-asynclo/75575557#75575557) is incomplete. It doesn't even define the `SemaphoreSlim`. If there was a definition, it would be `using SemaphoreSlim mutex = new(1, 1);`. As for Stephen Toub's [`AsyncLock`](https://devblogs.microsoft.com/pfxteam/building-async-coordination-primitives-part-6-asynclock/), it seems like an incomplete implementation too. – Theodor Zoulias Feb 27 '23 at 12:55
  • 1
    I've updated my answer, adding a hopefully more efficient `MutexAsyncable` class for .NET 5+. – Jez Feb 27 '23 at 13:13
  • I am not entirely sure that the `try`/`finally` trick prevents completely the possibility of a deadlock on .NET Framework. I am not saying that you are wrong. I am just not sure. It's not very important anyway since the .NET Framework is a non-evolving platform. Also I don't get how the `TimeSpan.FromSeconds(1)` forces the worker code to release the lock. If I pass `() => Thread.Sleep(-1)` as `action`, or `() => Task.Delay(-1)` in the async version, my understanding is that one execution flow will keep the lock forever, and all other flows will spin every second unproductively forever. – Theodor Zoulias Feb 27 '23 at 13:44
  • Good job I never use .NET Framework anymore, really. – Jez Feb 27 '23 at 13:52
-1

I did try using a Monitor (code below) which appears to work but has a GOTCHA... when you have multiple threads it will give...

System.Threading.SynchronizationLockException Object synchronization method was called from an unsynchronized block of code.

using System;
using System.Threading;
using System.Threading.Tasks;

namespace MyNamespace
{
    public class ThreadsafeFooModifier : 
    {
        private readonly object _lockObject;

        public async Task<FooResponse> ModifyFooAsync()
        {
            FooResponse result;
            Monitor.Enter(_lockObject);
            try
            {
                result = await SomeFunctionToModifyFooAsync();
            }
            finally
            {
                Monitor.Exit(_lockObject);
            }
            return result;
        }
    }
}

Prior to this I was simply doing this, but it was in an ASP.NET controller so it resulted in a deadlock.

public async Task<FooResponse> ModifyFooAsync()
{
    lock(lockObject)
    {
        return SomeFunctionToModifyFooAsync.Result;
    }
}
Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104
andrew pate
  • 3,833
  • 36
  • 28
  • NOTE: I did add this answer to give an insight into what might look to work, but will bite you eventually.... rather than code you should copy and use! ... The SemaphoreSlim is what should be used, do not forget to dispose it properly though. – andrew pate Dec 15 '22 at 17:09