-
-
Notifications
You must be signed in to change notification settings - Fork 359
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Docs: reentrancibility of async lock #257
Comments
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). |
@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. That aside, and even without any 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. |
@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. |
I just noticed that |
Hello! What about another implementation? http://dotnetfiddle.net/lLLMoV |
Your documentation for
AsyncLock
currently says: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.
The text was updated successfully, but these errors were encountered: