Skip to content
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

Feature Request: Make AsyncGate.LockAsync cancellable #2148

Open
gmkado opened this issue Jul 16, 2024 · 5 comments · May be fixed by #2153
Open

Feature Request: Make AsyncGate.LockAsync cancellable #2148

gmkado opened this issue Jul 16, 2024 · 5 comments · May be fixed by #2153

Comments

@gmkado
Copy link

gmkado commented Jul 16, 2024

The underlying SemaphoreSlim.WaitAsync already supports cancellation. Can a parameter be exposed to pass a token down to this line?

return new ValueTask<Releaser>(_semaphore.WaitAsync().ContinueWith(_ => new Releaser(this)));

@idg10
Copy link
Collaborator

idg10 commented Jul 17, 2024

It's not AsyncRx.NET's goal to provide general-purpose asynchronous utilities, so I would only want to add features to AsyncGate if they directly serve Rx-related purposes.

In fact, my first thought here is: should AsyncGate be made internal?

This is not an area of AsyncRx.Net that I've explored yet. (We inherited this code when we took over maintenance of Rx, and most of our focus so far has been elsewhere.) So I need to attempt to understand the thinking behind the fact that AsyncGate exists.

As far as I can tell, it currently appears in just two public APIs: AsyncObservable.Synchronize and AsyncObserver.Synchronize. By providing overloads that accept an AsyncGate, it becomes possible to create multiple synchronized observers and observables that are all mutually synchronized. System.Reactive offers conceptually equivalent overloads:

public static class Observable
{
    public static IObservable<TSource> Synchronize<TSource>(this IObservable<TSource> source, object gate)
}
public static class Observer
{
    public static IObserver<T> Synchronize<T>(IObserver<T> observer, object gate)
}

Note that these take a plain object as the 'gate'. They can do that because in .NET , any object can be used as a synchronization primitive. Rx.NET is simply using the C# lock keyword on this object to implement the behaviour that Synchronize promises.

So these Rx.NET methods offer two capabilities:

  1. it is possible to create multiple observers and observables all mutually synchronized by passing the same gate object to each call to Synchronize
  2. it is possible for other program activity also to be mutually synchronized with the Rx activity protected by Synchronize by writing application code that also calls lock with the same gate object

AsyncRx.NET faces a challenge in implementing the same service: the synchronization capability available for any object (i.e. what we use through lock) was designed long before async and await, so it just blocks the thread when you use lock. That would conflict with the entire point of AsyncRx.NET (and of async/await in general) which is to enable non-blocking asynchronous work.

So to enable the two capabilities described above we can't use object as the gate type.

What we would want to do is use the async equivalent of lock. Unfortunately, no such thing really exists. And even the new System.Threading.Lock that has appeared in recent .NET 9.0 previews doesn't add any async support. This was discussed:

dotnet/runtime#34812 (comment)

(The basic issue is that thread affinity is part of the design, which rules out async.)

There is a very long-standing request to add an AsyncLock to the .NET runtime libraries (open for 5 and a half years as I write this). This suggests that we should not hold our breath waiting for .NET to grow such a feature...

So the original AsyncRx.NET implementors' decision to define their own AsyncGate seems like the only option in practice.

That shuts down my initial thought: no, this type should not be internal; it needs to be accessible to application code to support the use cases listed above. Since .NET offers no suitable type and is unlikely to any time soon, AsyncRx.NET would either need to take a dependency on some library that already does (but no well-supported library with a clearly defined future exists), or define its own. And it can't just be an opaque handle—that would work for 1 above, but not 2.

So, given that we need to provide AsyncGate, and that it needs to be possible for application code to acquire it, cancellation is a reasonable feature request: even though it is very much not our goal to be providing synchronization primitives, we have no choice here, and so there's a case that the thing we provide should be usable in the ways people are likely to want.

However...

There's one notable feature of the discussion linked to above in the proposal for adding AsyncLock to .NET: many people have attempted to add cancellation to Stephen Toub's original AsyncLock, and they all seem to end up with buggy implementations. That makes me somewhat reluctant to tread this same treacherous path that has caught out many others in the past.

I'm wondering if we should in fact take a completely different approach. I'm wondering if we should define an IAsyncGate interface that provides the minimal API that AsyncRx.NET requires (i.e., without async), and to make Synchronize accept any implementation of that. Our AsyncGate would continue to provide a minimal implementation, but if applications want to do 2 above with cancellation support, they are free to define their own implementations (and to run into the same bugs everyone else who attempts this seem to run into).

(AsyncRx.NET is still in preview, so we can make breaking changes like this.)

In fact I'd be inclined to make all of AsyncGate's current public members either private or explicit interface implementations of this new IAsyncGate, and to define a new AsyncGate.Create. That way if .NET ever does implement its own async lock, we could move over to using that in place of our AsyncGate.

So AsyncRx.NET would make no attempt to support cancellation, but it would become possible for application code to supply its own IAsyncGate implementation that supports this or any other semantics the application wants.

This approach would remove the block that prevents you from doing what you want. It's not quite what you asked for, because we wouldn't be providing the solution we want, but we would a) make it possible for you to write such a solution, and b) leave the door open for using a future .NET-supplied async lock primitive if they ever add such a thing. Also, if you're already using a library that provide such a primitive (and I gather project Orleans does) you could use that. I prefer that to an API design that forces you to use our type.

So this would provide a way for you to do what you want. How does that sound?

@gmkado
Copy link
Author

gmkado commented Jul 17, 2024

@idg10 thanks for the thorough response, using an interface sounds like an appropriate fix for Rx. It's easy enough for me to copy over AsyncGate and extend it to support cancellation.

I'll admit I'm using some (useful) exposed public classes outside of the scope of Rx. For example, I'm guessing I shouldn't rely on the implementation of RefCountAsyncDisposable in this other discussion I posted

@idg10
Copy link
Collaborator

idg10 commented Jul 18, 2024

I'm guessing I shouldn't rely on the implementation of RefCountAsyncDisposable

I apologise for not noticing that Q&A early. I thought GitHub would notify me when people posted in there, but I guess not!

You can totally rely on all our public IDisposable implementations continuing to work exactly as they do today. But they are there to be what Rx needs them to be, and not to be the ultimate all-things-to-all-people IDisposable implementation. We will not be removing them from the public API or making breaking changes. But feature requests will always be evaluated based on whether the feature is aligned to the goals of Rx.

@idg10
Copy link
Collaborator

idg10 commented Jul 18, 2024

I've prototyped an implementation of this IAsyncGate concept. However, it turns out that to make that available as a NuGet package, I need to get some help from the .NET Foundation, because our code signing mechanisms are no longer working, and we can't publish NuGet packages (even preview ones) until that is fixed. Sorry!

I'll post again here once a usable preview is available.

@idg10
Copy link
Collaborator

idg10 commented Aug 29, 2024

(I am still trying to solve the code signing! I was off sick for a bit, and then on vacation for a while, and I now think it's possible that the person at the .NET Foundation whose help I need to get code signing reinstated may be on vacation... But I haven't forgotten this.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants