Docs: reentrancibility of async lock

[matthew-a-thomas]

Your documentation for AsyncLock currently says:

It's only almost equivalent because the lock keyword permits reentrancy, which is not currently possible to do with an async-ready lock.

Technically reentrancy is possible 😉

P.S. I'm aware this has been attempted a gazillion times before, including by you. But I'm hopeful to have found the proper proportion of ingredients—check out the kinds of tests that pass! I know of no other existing implementation that can pass all of these tests.

[timcassell]

I looked into it, and saw that even your own solution fails this simple test. (Credit)

AsyncLock _asyncLock = new AsyncLock(allowReentry: true);
int CurrentThreadId => Thread.CurrentThread.ManagedThreadId;

async Task MainAsync()  
{  
    using (await _asyncLock.LockAsync())  
    {  
        Console.WriteLine($"1. {nameof(MainAsync)} starting on thread {CurrentThreadId}");  
          
        var childTask = ChildAsync(delay: TimeSpan.FromMilliseconds(100));  
        await Task.Delay(TimeSpan.FromMilliseconds(101)).ConfigureAwait(false);  
          
        Console.WriteLine($"4. {nameof(MainAsync)} continuing on thread {CurrentThreadId}");  
        NonThreadSafe();  
        Console.WriteLine($"6. {nameof(MainAsync)} finished {nameof(NonThreadSafe)}");  
  
        await childTask;  
    }  
}  
  
async Task ChildAsync(TimeSpan delay)  
{  
    Console.WriteLine($"2. {nameof(ChildAsync)} starting on thread {CurrentThreadId}");  
  
    await Task.Delay(delay).ConfigureAwait(false);  
      
    using (await _asyncLock.LockAsync())  
    {  
        Console.WriteLine($"3. {nameof(ChildAsync)} continuing on thread {CurrentThreadId}");  
        NonThreadSafe();  
        Console.WriteLine($"5. {nameof(ChildAsync)} finished {nameof(NonThreadSafe)}");  
    }  
}

I do believe it is possible to solve this case, but only with cooperation from Tasks themselves. Which means the AsyncLock would have to be included in the TPL/BCL itself. And the cost of that cooperation would also slow down tasks in the general case, making it highly unlikely to ever be adopted.

But even then, there would be problems with it: such a solution could not support synchronous locks in a safe way. This simple test would deadlock. Also, it would only be safe to use with Tasks, and it would be dangerous to use with any other custom task-like type (including ValueTask, since they can be backed by any custom type).

[matthew-a-thomas]

@timcassell That article you linked to was actually my main motivation for working on this. Max Fedotov said it's impossible and I wanted to prove him wrong. As I'm sure you noticed, I'm footnote number two in his article. You can read more of our back and forth by following the links on my website. Edit: actually this article (the last in that series) is probably more relevant to this discussion.

The example code you've posted would indeed be unpleasant with my lock. But allow me to explain why I don't think this is significant enough to call my lock "broken".

Notice how that code alters the current synchronization context while in the guarded section of the lock. await Task.Delay(TimeSpan.FromMilliseconds(101)).ConfigureAwait(false); is the line that does that, and it does that with .ConfigureAwait(false). However I explicitly advise people not to do that with my lock. So I don't consider this to be any more threatening to my lock than it's a threat to synchronous locks when you call Monitor.Exit while in a lock body. Synchronous locks are not broken and impossible just because you can abuse them. So also as long as people use my library in the way I intend them to then they will have a working reentrant async lock and the two calls to NonThreadSafe() in your example code will happily be serialized.

That aside, and even without any .ConfigureAwait(false), your code does come close to hitting what I've already identified (with Max's help) here. That's because (if my lock were used in your code) you will have trickled my magic synchronization context down into childTask—you'll have reentered my lock—but then you run both lock-holding asynchronous branches in parallel. As a result any awaits within the guarded sections while that is going on will effectively toggle between the two branches. And that can be... weird. The issue I linked to perhaps gives a more clear example of this happening. But even with that weirdness I'm not convinced my lock is faulty. Although I haven't put much thought into how to improve those ergonomics I suspect an elegant solution is possible. And if not then I'll just have to add another warning to the label.

And at the end of the day I think it's significant that my lock does indeed do things that no other lock can do. Furthermore I think that if you stand far enough back and squint at Max's article, those are exactly the things that he says are impossible.

[timcassell]

@matthew-a-thomas Thanks, I did miss some of that conversation. Also, I started a discussion in my repo about an idea, would love to get your thoughts on it.

[matthew-a-thomas]

I just noticed that Cogs.Threading.ReentrantAsyncLock is another correct implementation: https://dotnetfiddle.net/6WdVy1