Feature request: AsyncReaderWriterLock support for UpgradeableReadLock
timonkrebs opened this issue · 11 comments
The ReaderWriterLock supports UpgradeableReadLock.
I would need this feature.
Is there a chance to get this?
It was removed in v5 (See #97).
Alternatively, you can use the AsyncReaderWriterLock from my ProtoPromise library that supports upgradeable lock. https://github.com/timcassell/ProtoPromise#asyncreaderwriterlock
I would need this feature.
There's almost always better solutions than an upgradeable rwl. What's your use case?
I am working on a solid.js and structured concurrency inspired signal (reactivity/declerative) implementation for c#.
I already have a thread-safe solid.js implementation in a synchronous/sequential version of this:
This is probably the most useful entry point:
https://github.com/timonkrebs/MemoizR/blob/main/MemoizR/Signal.cs
I have a signal that can be written to. Everyone that is listening gets notified but the evaluation is not triggered. Therefor a read lock is sufficient.
Only when someone reads the value the graph gets evaluated. But the evaluation involves dynamic dependency tracking and memoization that is not safe to do concurrently. Therefor a writelock is needed.
https://github.com/timonkrebs/MemoizR/blob/main/MemoizR.Reactive/Reaction.cs
The UpgradeableReadLock is needed because we have to transition from the readlock to the writelock when we immediately need to react to invalidation of the graph.
Now I would like to work on fully supporting async, because at the moment i think i need it to implement my vision of a fully "declerative structured concurrency" approach.
This would probably also need the LockRecursionPolicy.SupportsRecursion... I think I have a workaround for UpgradeableReadLock but still need LockRecursionPolicy.SupportsRecursion. I think i could implement a custom async locking mechanism just for my case... But I would prefere not to. It is still quite experimental...
I have a version that seems to work with Recursion. It works with AsyncLocal and therefore can only work if there always is a AsyncStateMachine present when used. In my case this can be ensured. Maybe this could also be ensured in general. If the AsyncReaderWriterLock itself enforces this it would be possible.
At the moment I work with the mentioned workaround for UpgradeableReadLock. But I would really like to use this with AsyncEx.
My solution is something like:
private int lockScope;
private Random rand = new();
static AsyncLocal<int> _asyncLocalScope = new AsyncLocal<int>();
public IDisposable WriterLock(CancellationToken cancellationToken)
{
return RequestWriterLockAsync(cancellationToken, Environment.CurrentManagedThreadId).GetAwaiter().GetResult();
}
public AwaitableDisposable<IDisposable> WriterLockAsync(CancellationToken cancellationToken)
{
var lockScope = _asyncLocalScope.Value;
if(lockScope == 0){
lockScope = rand.Next();
_asyncLocalScope.Value = lockScope;
}
return new AwaitableDisposable<IDisposable>(RequestWriterLockAsync(cancellationToken, lockScope));
}private Task<IDisposable> RequestWriterLockAsync(CancellationToken cancellationToken, int lockScope)
{
Task<IDisposable> ret;
lock (this)
{
// If the lock is available, take it immediately.
if (_locksHeld == 0)
{
_locksHeld = -1;
#pragma warning disable CA2000 // Dispose objects before losing scope
ret = Task.FromResult<IDisposable>(new WriterKey(this));
#pragma warning restore CA2000 // Dispose objects before losing scope
this.lockScope = lockScope;
}
else if (_locksHeld < 0 && this.lockScope == lockScope)
{
--_locksHeld;
#pragma warning disable CA2000 // Dispose objects before losing scope
ret = Task.FromResult<IDisposable>(new WriterKey(this));
#pragma warning restore CA2000 // Dispose objects before losing scope
}
else
{
// Wait for the lock to become available or cancellation.
ret = higherlevel.Enqueue(this, cancellationToken, lockScope);
}
}
ReleaseWaitersWhenCanceled(ret);
return ret;
}If RequestWriterLockAsync was async then you could guarantee that there is a AsyncStateMachine. Do you think there is any significant downside in doing that?
It was removed in v5 (See #97).
Alternatively, you can use the
AsyncReaderWriterLockfrom my ProtoPromise library that supports upgradeable lock. https://github.com/timcassell/ProtoPromise#asyncreaderwriterlock
Thank you for this input, but I think for my case it would be preferable to prioritize the write lock. And as mentioned I also need the Recursion.
Thank you for this input, but I think for my case it would be preferable to prioritize the write lock.
What do you mean?
And as mentioned I also need the Recursion.
AsyncEx doesn't support recursion, either. It's fundamentally unsafe for async locks. That's something you have to implement yourself, and use very carefully. It's simple enough to implement with AsyncLocal, as you mentioned, and which you can enable support for in ProtoPromise.
as mentioned in the Readme of AsyncReaderWriterLock
Similar to System.Threading.ReaderWriterLockSlim, except, just like AsyncLock, recursion is not supported. Also, unlike ReaderWriterLockSlim, this is a balanced reader/writer lock. That means readers and writers take turns so that no one will get starved out.
readers and writers take turns I think that this is not what I need. I think I need the opposite behaviour of AsyncEx AsyncReaderWriterLock which prioritizes the WriterLocks. I think it in my case its preferable to have the ReaderLocks prioritized.
It's fundamentally unsafe for async locks.
What is unsafe in the example I gave here? This could be implemented in AsyncEx as implementation detail.
readers and writers take turns I think that this is not what I need. I think I need the opposite behaviour of AsyncEx AsyncReaderWriterLock which prioritizes the WriterLocks. I think it in my case its preferable to have the ReaderLocks prioritized.
I implemented it using a balanced approach to avoid lock starvation. With reader lock prioritization, it's very easy for reader locks to prevent a writer from ever acquiring the lock, especially in an async context. That's how you get stale data. With writer lock prioritization, it's the opposite: it's easy for readers to get starved out. My balanced implementation prevents starvation from either side. And even with that, you can still have many simultaneous readers.
[Edit] I should note that my implementation still favors readers over writers (just not to the same extreme as ReaderWriterLockSlim). If there are multiple writers queued up, readers will be able to take the lock in-between each writer.
What is unsafe in the example I gave here?
Give this a read. https://itnext.io/reentrant-recursive-async-lock-is-impossible-in-c-e9593f4aa38a
My balanced implementation prevents starvation from either side. And even with that, you can still have many simultaneous readers.
I agree in general this is probably the safest way to implement this. But there are use cases where the other approaches are preferred.
Thanks for the information about reentrant recursive async lock. That seems to be a real problem for the general case... But this seems something that can be prevented by following sequential async await patterns.
The problem described in the article feels quite related to: https://vorpus.org/blog/notes-on-structured-concurrency-or-go-statement-considered-harmful/
As I mentioned my motivation is to build something I call Declarative Structured Concurrency. I think this could solve the recursive async lock problem.
The problem described in the article feels quite related to: https://vorpus.org/blog/notes-on-structured-concurrency-or-go-statement-considered-harmful/
That was a very interesting read! It does seem related, in that I think the async lock recursion problem could be solved at the language-level. Unfortunately, we're already past that point in C#, and we'd need a new language. (Not even accounting for the fact that .Net is used by multiple languages.)
Unfortunately, we're already past that point in C#, and we'd need a new language
I don't think that this is true. You can shoot yourself in the foot with almost any language feature in any language if not used correctly. C# has already many concepts that can be used inappropriately (async await itself is actually already quite easy to be used inappropriately. think of: async void, .Wait, not awaiting everything and even .ConfigureAwait). This is one of the primary reasons why I think we need (Declarative) Structured Concurrency in C#. And there is already a compiler warning for not awaiting a task (this call is not awaited). Like Nullable reference Types, sequential async await and (Declarative) Structured Concurrency could be introduced with a flag.
And if you only use concurrency with a (Declarative) Structured Concurrency library then this would probably also not be a problem.
I do not get why "The One Use Case" should not also be valid for asynchronous locks. The same reasoning applies here as well! There are async recursive algorithms that obey sequential async await... Like the one I am envisioning. They need Recursion. But recursive Async Locks should probably be something that is provided by Structured Concurrency libs. That would make it clear when/how they are safe to use.
This has implications on UpgradeableReadLock, because I need the UpgradeableReadLock also because of the recursive nature of the algorithm. But I have a workaround for the UpgradeableReadLock that I think is manageable. And I think it would complicate the implementation of AsyncEx.AsyncReaderWriterLock substantially. A solution that is tailored to my specific problem is probably the most reasonable way forward.