Test code for NeoSmart.AsyncLock() may be showcasing _correct_ behavior.

[Nerketur]

cs-reentrant-async-lock/README.md

Line 56 in 50d2632

* [NeoSmart.AsyncLock](https://github.com/neosmart/AsyncLock) does not provide reentrance with mutual exclusion: https://dotnetfiddle.net/CkK674

I'll admit I'm not a master of async code, but isn't the code, as written guaranteed to deadlock? Sending code off to a Task.Run() will create a new thread, so "reentry" isn't valid anymore. In that way, I think AsyncLock performs correctly by returning Deadlock? here, because the "reentrant" code is on a different thread (and thus will be blocked from entering).

Of course, this does also mean all the other ones claimed incorrect are indeed incorrect, it's only this one I'm not convinced is actually wrong. It might be the only one to be actually right.

If Task.Run() does not run things in a new thread, then I'm wrong and will fully own up to my mistake.

[matthew-a-thomas]

Hello!

isn't the code, as written guaranteed to deadlock?

No. I mean obviously AsyncLock is guaranteed to deadlock, but in my view that's a problem with how AsyncLock is implemented and not with the code as written. Compare that code with this automated test, which is identical other than using ReentrantAsyncLock from this repo, and which passes.

Sending code off to a Task.Run() will create a new thread, so "reentry" isn't valid anymore. In that way, I think AsyncLock performs correctly by returning Deadlock? here, because the "reentrant" code is on a different thread (and thus will be blocked from entering).

I'm open to calling it something other than "reentry", but the idea is similar to normal single-threaded lock reentrance in that once the lock is acquired it can be acquired again "inside" the guarded section of the lock. Just so we're on the same page, here's what I'm thinking of when I say "normal single-threaded lock reentrance":

var gate = new object();
lock(gate)
{
  // This is the guarded section of the lock
  lock(gate)
  {
    // I'm inside the guarded section, and can acquire the lock again without deadlocking
  }
}

I have no doubt you get that, I'm just being as clear and precise as I can be. And as you know this is what I mean by "reentry" in the case of an async lock:

var asyncLock = new PickYourFavoriteAsyncLock();
using (await asyncLock.LockAsync(CancellationToken.None))
{
  // This is the guarded section of the lock
  await Task.Run(async () =>
  {
    // I'm "inside" the guarded section of the lock, so to speak
    using (await asyncLock.LockAsync(CancellationToken.None))
    {
      // ...and if execution gets to here then I've "reentered" the lock
    }
  });
}

I think the difference between our views might lie in the presence of the await Task.Run(async () => {... does the body of that lambda count as being "inside" the guarded section or not? I tend to think so. And I suspect you might tend to think not.

But either way, as you know I've pointed out in the readme that ReentrantAsyncLock from this repo as well as the one from Cogs.Threading are the only two implementations of asynchronous locks I know of that allow execution to get to that innermost part, while also guaranteeing mutual exclusion.

Now, is it actually a problem that AsyncLock deadlocks in that code you're referring to? From one point of view, no it's no problem since they haven't advertised it as being capable of doing that. But from another point of view, the authors do advertise its ability to reenter, so what's keeping it from being reentrant in this way?

I suspect which opinion you take will depend a lot on how useful the feature is to your problem at hand. And to be perfectly honest, I totally get it if uber-reentrance (or whatever we'll call it) isn't really what people are after. I mean, I'm making a career from writing C# and I'm even the author of this library, and I have to admit that I myself have hardly any need for a reentrant async lock. I've even written publicly advising people to prefer options other than a reentrant async lock. Stephen Cleary has some really good points in this article. In my personal experience every time I have found myself wishing for a reentrant async lock, I have also immediately realized my software is too complex in that area and would benefit from some refactoring, and then the need just goes away. So I get it :)

---

Pedantic note: Task.Run() (well really async/await in general) is orthogonal to threading and parallelism. It's possible to have Task/async/await with only a single OS thread. The Rust programming language comes to mind as a good example of this—there are implementations of single-threaded task executors for that language. Indeed this library here guarantees that async code inside a guarded section is never executed on more than one thread at once, even inside an await Task.Run(() => lambda). I can elaborate on any of this if you'd like.

[Nerketur]

I think the only issue I have with saying the code works as designed is you can write the same code with different lambdas (with functions even), and the code is now no longer in the guarded section of code but (will still)? function the same as before.

For example:

        var asyncLock = new PickYourFavoriteAsyncLock();
        innerAwait = async () => {
            // I will still be run "inside" the guarded section of the lock, so to speak
            // However, this is not guaranteed.  I could also be run in a completely different lock.
            using (await asyncLock.LockAsync(CancellationToken.None)) {
                // ...Have I "reentered" the lock, here?  I was defined outside of the using block.
            }
        }
        using (await asyncLock.LockAsync(CancellationToken.None)) {
            // This is the guarded section of the lock
            await Task.Run(innerAwait);
        }

Practically speaking, yes, it's still "re-entrant" using the new definition, but I argue this is problematic, because async/await does not always return to the calling thread. (Say, in the case of ConfigureAwait(false), which I think is rather common for cases where you would ever want Async locks in the first place.) It also breaks the "visual" of being in the same block, but that is more of a style issue than a code one.

As far as what prevents the code from being reentrant in cases as you describe, I suspect for the same reason you'd want two different click events to be considered separate blocks even though they technically run in the same task ID (the UI thread). (as seen here: https://neosmart.net/blog/asynclock-an-asyncawait-friendly-locking-library-for-c-and-net/ )

All this having been said, you could argue avoiding deadlock in all possible cases is preferable to implementing Async Locks "correctly", so perhaps my disagreement is somewhat moot. I do see cases where you could want either way of thinking of things. :)

I suppose my real concern was not saying this way was "wrong", but to be careful when saying your way is "correct". But hey, I wholeheartedly agree that the cases where this is actually desired are few and far between. I've been developing for 25+ years and only today have I ever even considered using anything like it. Asynchronous programming is hard to do correctly.

I also want to reiterate, I'm no master. I could be way off base. For as long as I've been creating code, I still consider myself a beginner with async, await, parallelism, and Tasks.

[matthew-a-thomas]

Hello again, and thank you for your reply.

I think the only issue I have with saying the code works as designed is you can write the same code with different lambdas (with functions even), and the code is now no longer in the guarded section of code but (will still)? function the same as before.

I think it will help us come to a mutual understanding more quickly if I focus here. Allow me to rewrite your code to use a synchronous lock:

var syncLock = new object();

Action innerFn = () =>
{
  lock (syncLock)
  {
    // Have I "reentered" the lock?
  }
};

lock (syncLock)
{
  innerFn();
}

I raise the same question as you did, "have I reentered the lock?" Of course the answer is yes. And so the "insideness" of this type of lock doesn't depend on where the inner function is defined. Instead it depends on how (and whether) a specific kind of lock context flows into the inner function. In this case the context has to do with the identity of the thread which is executing that code. Again, I have no doubt you understand this. I am only spelling this out in order to set the stage:

Similarly, above when I wrote await Task.Run(async () => lambda) and then claimed that the lambda was "inside" the async lock, I didn't mean it was inside because it was defined inside of it. Instead I claimed it was "inside" it because a specific kind of context flows into that lambda. The context in this case has to do with a specific implementation of SynchronizationContext and how that SynchronizationContext flows down through await points (and is normally restored after them).

So, yes, my points still stand if we refactor my example code to be this:

var asyncLock = new PickYourFavoriteAsyncLock();
Func<Task> innerFn = async () =>
{
  // I'm "inside" the guarded section of the lock, so to speak
  using (await asyncLock.LockAsync(CancellationToken.None))
  {
    // ...and if execution gets to here then I've "reentered" the lock
  }
};
using (await asyncLock.LockAsync(CancellationToken.None))
{
  // This is the guarded section of the lock
  await Task.Run(async () => await innerFn());
}

I wrote "I suspect you might tend to think [the lambda is not 'inside' the guarded section]" because I thought you were hesitant to think of anything inside an await as also being "in" the guarded section of an async lock. Feel free to clarify your point there.

Does this help clarify what I wrote earlier?

[matthew-a-thomas]

I'm not sure if this will just be a distraction, but I can try to give an answer to other things you wrote:

Practically speaking, yes, it's still "re-entrant" using the new definition, but I argue this is problematic, because async/await does not always return to the calling thread. (Say, in the case of ConfigureAwait(false), which I think is rather common for cases where you would ever want Async locks in the first place.) It also breaks the "visual" of being in the same block, but that is more of a style issue than a code one.

ConfigureAwait(false) is indeed very common. In fact we can go farther than that and say it is recommended practice depending on the situation. And you are also correct that you aren't necessarily guaranteed to be on the same thread on the other side of an await (even without ConfigureAwait(false)).

But you are guaranteed to be on the same SynchronizationContext when an await comes back (as long as you don't await task.ConfigureAwait(false)).

And because this library uses a special SynchronizationContext, once execution enters the guarded section of a ReentrantAsyncLock you actually are guaranteed to be the only thread currently executing guarded code. You may or may not find yourself on a different thread after an await point—run this code a few times and you'll see what I mean—but that actually doesn't matter with respect to mutual exclusion. Mutual exclusion does not require that it's always the exact same thread that gets to run guarded code; instead it only requires that only up to a single thread is running a given guarded section at any given time.

Now, ConfigureAwait(false) does have an impact because it wipes out the SynchronizationContext which is making everything work. That's why I say don't use it directly inside a guarded section, and explain what to do instead. That's not to say that ConfigureAwait(false) in some nested async function call breaks everything... let me know if the readme isn't clear enough on this point.

---

As far as what prevents the code from being reentrant in cases as you describe, I suspect for the same reason you'd want two different click events to be considered separate blocks even though they technically run in the same task ID (the UI thread). (as seen here: https://neosmart.net/blog/asynclock-an-asyncawait-friendly-locking-library-for-c-and-net/ )

I think it's significant that this library's ReentrantAsyncLock can provide mutual exclusion to two different click event handlers just as well as AsyncLock can.

In the case of ReentrantAsyncLock the mutual exclusion comes about because both click handlers start out running in some SynchronizationContext other than the one associated with the instance of ReentrantAsyncLock. In the case of WPF they might be executing in some DispatcherSynchronizationContext. In the case of WinForms they might be executing in some WindowsFormsSynchronizationContext. Or perhaps they are executing with SynchronizationContext.Current set to null. In any case, both handlers must await the async lock. That await causes the guarded section of code itself to be executed on the special SynchronizationContext associated with the instance of ReentrantAsyncLock. I've been talking a lot about SynchronizationContext but ExecutionContext is also just as important for accomplishing things. I can explain more as desired.

My point here is that AsyncLock's deadlock arises only because of an implementation detail. The deadlock doesn't necessarily arise from mutual exclusion itself. And so I don't think that bestowing mutual exclusion to click handlers is a compelling reason to prevent reentry.

[matthew-a-thomas]

I'm terribly sorry, but I just realized that I've been wrong this whole time about await Task.Run(async () => lambda) causing the lambda to also be part of the guarded section. In fact it is not; the following code prints "False":

var asyncLock = new ReentrantAsyncLock();
await using (await asyncLock.LockAsync(CancellationToken.None))
{
    await Task.Run(() =>
    {
        Console.WriteLine(asyncLock.IsOnQueue);
    });
}

I've recorded this error as #7. I need to reevaluate what I've written so far.

[Nerketur]

I don't know if Task.Run() guarantees a separate thread, but in my experience it seems to. If it does, then there are now two threads running inside the critical section. (A normal await on a task does not guarantee a new thread, but still can create one.)

Also, you pose a good point about the critical section only really requiring a maximum of one thread, regardless of whether that thread changes while inside the section. I didn't think of that. :)

That said, there is one issue I foresee with this method.

1.) Task.Run() (or, even simply await) can be used to run multiple threads at one time (using Task.WhenAll() as a joiner.) A single task that signifies when the other tasks are done. So a single Task (by default) cannot (and does not) guarantee a single thread when awaited. (You may have fixed this issue already with the special context.) I haven't looked into the code of WhenAll(), however. It may, in fact, just keep awaiting them synchronously.

My point here is that AsyncLock's deadlock arises only because of an implementation detail. The deadlock doesn't necessarily arise from mutual exclusion itself. And so I don't think that bestowing mutual exclusion to click handlers is a compelling reason to prevent reentry.

I still believe it isn't an incorrect implementation detail, as Task.Run() does, in fact, usually create and run more threads. Without Task.Run(), AsyncLock() does, in fact, seem to be mutually exclusive and reentrant.

The reason it deadlocks, it seems to me (and the reason I personally find that to be correct), is precisely because Task.Run() creates a different thread, now running at "the same time" as the original. The lock will only let one through, but as the original (the one that created this thread) has not left the lock's critical section, this new thread is blocked by the "new" critical section. And the original thread waits for the new to complete.

I don't know if there's a way to say "Allow all child threads to also be reentrant with the same lock as the original", but that seems to be what you are going for. I'm rooting for you to be able to figure it out. I'm not well-versed enough to do it yet. Async is hard :)

[matthew-a-thomas]

Thank you for your reply. I appreciate this extended dialog with you.

I hear you saying two things: first, that I'm being too hard on AsyncLock; second, that the way AsyncLock behaves feels more correct.

In favor of AsyncLock is the fact that it does allow one type of reentrance. The following two code segments both run to completion:

var asyncLock = new AsyncLock();
using (await asyncLock.LockAsync())
{
    using (await asyncLock.LockAsync())
    {
        // It supports reentrance
    }
}

var asyncLock = new AsyncLock();
async Task InnerFunctionAsync()
{
    using (await asyncLock.LockAsync())
    {
        // It supports reentrance here, too
    }
}
using (await asyncLock.LockAsync())
{
    await InnerFunctionAsync();
}

Now, you might be surprised to learn that AsyncLock supports reentrance with Task.Run. This code also runs to completion:

var asyncLock = new AsyncLock();
async Task InnerFunctionAsync()
{
    using (await asyncLock.LockAsync())
    {
        //
    }
}
using (await asyncLock.LockAsync())
{
    await Task.Run(InnerFunctionAsync); // Reentrance with Task.Run
}

But the wheels seem to fall off the AsyncLock cart after that. On my machine this deadlocks unless I introduce a delay between the two calls to Task.Run (or unless I debug it, which I'm guessing is merely introducing a delay):

var asyncLock = new AsyncLock();
async Task InnerFunctionAsync()
{
    using (await asyncLock.LockAsync())
    {
        //
    }
}
using (await asyncLock.LockAsync())
{
    await Task.Run(InnerFunctionAsync);
    await Task.Run(InnerFunctionAsync);
}

Even this deadlocks for me (again, unless I introduce a delay between the two calls):

var asyncLock = new AsyncLock();
async Task InnerFunctionAsync()
{
    using (await asyncLock.LockAsync())
    {
        //
    }
}
using (await asyncLock.LockAsync())
{
    await InnerFunctionAsync();
    await InnerFunctionAsync();
}

You can see for yourself that this code never prints "done". By the way the authors of that library have been aware of this issue for a little while. I think the above represents a more minimal reproduction than they had before, so I posted it to their issue.

I don't think I'm being too hard on AsyncLock.

As for AsyncLock's way feeling more correct, I'm torn on how to respond.

On the one hand I appreciate the opinion. And if I want to make ReentrantAsyncLock look good I can point out that ReentrantAsyncLock is able to do the same thing as AsyncLock. And so users are free to limit themselves to only "synchronous" reentrance like my last code block above—and <jab-at-competition>it'll even work correctly!</jab-at-competition>

But on the other hand I wonder if ReentrantAsyncLock only feels odd to you because it's surprising. And just because something is surprising doesn't mean it's less correct. And so I wonder if I should defend the correctness of ReentrantAsyncLock. I'll chase this lead down a little further.

You are correct that Task.Run has a lot to do with multiple threads. To be a little more pedantic, Task.Run will grab an available thread from the default thread pool, or under certain conditions will ask the thread pool to spin up a new thread for it, depending on configuration and a bunch of other details. But I don't think these pedantic details alter what you're saying. You're saying it's odd to allow reentrance inside a Task.Run because that very likely will be running on a different thread.

But again, which thread it is doesn't matter at all for mutual exclusion. The important thing for mutual exclusion is for there never to be multiple simultaneous threads. And ReentrantAsyncLock guarantees mutual exclusion.

For example:

var asyncLock = new ReentrantAsyncLock();
var guardedSectionCount = 0;
async Task InnerFunctionAsync()
{
    await using (await asyncLock.LockAsync(CancellationToken.None))
    {
        if (Interlocked.Increment(ref guardedSectionCount) != 1)
        {
            Environment.FailFast("Mutual exclusion violation");
        }
        Interlocked.Decrement(ref guardedSectionCount);
    }
}
while (true)
{
    Task.Run(InnerFunctionAsync);
}

The above code will kill the process if it detects a mutual exclusion violation. Based on the small assumption that Interlocked.Increment and Decrement work properly, if there is a mutual exclusion violation then guardedSectionCount will be incremented to 2 instead of 1. In which case the process kills itself through Environment.FailFast.

I ran the above code on my machine until it had used about 20 GB of ram from all the Task objects that Task.Run creates. I then killed the process; it didn't kill itself.

I repeated a similar test with the following code:

var asyncLock = new ReentrantAsyncLock();
var guardedSectionCount = 0;
async Task InnerFunctionAsync()
{
    await using (await asyncLock.LockAsync(CancellationToken.None))
    {
        if (Interlocked.Increment(ref guardedSectionCount) != 1)
        {
            Environment.FailFast("Mutual exclusion violation");
        }
        Interlocked.Decrement(ref guardedSectionCount);
    }
}
await using (await asyncLock.LockAsync(CancellationToken.None))
{
    while (true)
    {
        _ = Task.Run(InnerFunctionAsync);
    }
}

The difference here is the InnerFunctionAsync function is reentering the lock. But again, there were no mutual exclusion violations; it only used up a bunch of RAM.

I hope that settles any doubts about ReentrantAsyncLock. If not, I would encourage you to write a code example for me of something about ReentrantAsyncLock that seems less than correct.