Feature Request: Make `AsyncGate.LockAsync` cancellable

[gmkado]

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

reactive/AsyncRx.NET/System.Reactive.Async/Threading/AsyncGate.cs

Line 35 in 5f831de

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

[idg10]

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 cancellation), 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]

@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]

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]

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]

(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.)

[idg10]

OK, that took a lot longer than expected! (The .NET Foundation wanted to move people over to use of Managed Identities for code signing auth. This is a great idea, but since Rx.NET was the first project to attempt this, it took a while to work out how to get the config right.) Anyway, we are now able to sign packages again! 🎉

There is, finally, a preview build of System.Reactive.Async with a prototype of this proposed feature up at https://dev.azure.com/dotnet/Rx.NET/_artifacts/feed/RxNet/NuGet/System.Reactive.Async/overview/6.0.0-alpha.44.g5ca1c61383

To use the preview package feed, you'll need to configure NuGet to use this endpoint:

https://pkgs.dev.azure.com/dotnet/Rx.NET/_packaging/RxNet/nuget/v3/index.json

This is how a reference to the specific preview that has this feature looks in a .csproj:

<PackageReference Include="System.Reactive.Async" Version="6.0.0-alpha.44.g5ca1c61383" />

To test this out, I thought I'd see if I could write an adapter enabling us to use someone else's cancellable asynchronous lock with AsyncRx.NET's Synchronize operator. I chose Stephen Cleary's AsyncEx library, which defines a Nito.AsyncEx.AsyncLock type.

This is not an endorsement of that lock type. I have great admiration for Stephen Cleary's work, but I have done nothing to test this, or even to find out what state of maturity he considers this lock to be in. It was simply a readily usable async lock with cancellation support.

It has revealed a possible shortcoming with my design, which I'd be interested in your (or anyone else's) view on.

Here's my adapter:

using System.Reactive.Threading;

namespace TestRxAsyncPluggableGate;

/// <summary>
/// An adapter implementating of AsyncRx.NET's <see cref="IAsyncGate"/> around Nito.AsyncEx's
/// <see cref="Nito.AsyncEx.AsyncLock"/>.
/// </summary>
internal class NitoAsyncLockAsRxAsyncGate : IAsyncGate
{
    private IDisposable? _releaser;

    public Nito.AsyncEx.AsyncLock Lock { get; } = new();

    public async ValueTask<AsyncGateReleaser> LockAsync()
    {
        // The Nito AsyncLock returns a disposable that we must use to release the lock.
        // Since this gate is by design one-at-a-time (it does not support re-entrancy,
        // unlike AsyncRx.NET's own AsyncGate) we can be confident that once this
        // returns, we will have the lock, and the _releaser will be null.
        IDisposable releaser = await this.Lock.LockAsync().ConfigureAwait(false);

        // Locking purely to avoid any out-of-order memory issues. A more clever implementation
        // could use memory barriers, but it's really easy to introduce bugs that way.
        lock (this)
        {
            if (this._releaser is not null)
            {
                throw new InvalidOperationException("NitoAsyncLockAsRxAsyncGate in invalid state - _releaser not null even though nothing else should own this lock");
            }

            this._releaser = releaser;
        }

        return new AsyncGateReleaser(this);
    }

    public void Release()
    {
        // As above, we are using lock just to avoid out-of-order memory access issues.
        // We shouldn't ever actually see concurrent access to _releaser here, because LockAsync
        // can't proceed until the underlying lock becomes available, and that won't happen
        // until this method reaches the point where it calls releaser.Dispose(), which happens
        // after we've finished checking and then updating the field.
        // But without some sort of memory barrier, it's possible in theory that the memory
        // write that sets _releaser to null could become visible to another thread only after
        // the call to releaser.Dispose(). Similarly, without memory barriers, the write that
        // LockAsync makes to set _releaser might not be visible to this thread even though
        // the lock has been acquired.
        // (This is all vanishingly unlikely stuff, but that just means problems caused by
        // bugs in this area are nearly impossible to reproduce and debug, but can still happen
        // in production, especially under heavily load. The simplest way to be safe is just to
        // use lock, and since we expect no contention in most cases, this should be fine.)
        IDisposable releaser;
        lock (this)
        {
            if (this._releaser is null)
            {
                throw new InvalidOperationException("Release called when NitoAsyncLockAsRxAsyncGate not held");
            }

            releaser = this._releaser;
            this._releaser = null;
        }

        releaser.Dispose();
    }
}

This was more complex than I had hoped. When I did the original design for IAsyncGate, I hadn't thought about what it would take to write an adapter for a lock that also wanted to return you some sort of IDisposable that you had to call Dispose on. My design requires that IAsyncGate provide a Release method, and that this be able to release the lock without being given any additional context. But since Stephen's AsyncLock requires me to call Dispose on a specific object that it returns, it has been a bit more complex.

Now there is a YOLO version:

internal class NitoAsyncLockAsRxAsyncGate : IAsyncGate
{
    private IDisposable? _releaser;

    public Nito.AsyncEx.AsyncLock Lock { get; } = new();

    public async ValueTask<AsyncGateReleaser> LockAsync()
    {
        IDisposable releaser = await this.Lock.LockAsync().ConfigureAwait(false);
        this._releaser = releaser;
        return new AsyncGateReleaser(this);
    }

    public void Release()
    {
        IDisposable releaser = this._releaser;
        this._releaser = null;
        releaser.Dispose();
    }
}

That's actually the essence of it. And perhaps we might think there's no need for any sort of locking around access to the _releaser field because first line in LockAsync is inherently in a kind of rendez-vous with the Dispose call in Release. However, out of order memory accesses can sometimes really mess with this sort of reasoning. It would take quite a lot of work to be absolutely confident that this simpler version is actually OK.

Either way, this is more complex than I'd hoped. The design of IAsyncGate was specifically to avoid forcing lock implementations to provide an IDisposable if they didn't want to. But what I failed to anticipate is that it would make life harder for implementations that wrap something that does provide an IDisposable for unlocking.

So I'm now wondering whether I should make a small addition to AsyncGateReleaser. If we gave it two constructors, one taking an IAsyncGate and the other an IDisposable, and it would call either Release or Dispose when disposed depending on how it was initialized, this lock adapter would just become this:

internal class NitoAsyncLockAsRxAsyncGate : IAsyncGate
{
    public Nito.AsyncEx.AsyncLock Lock { get; } = new();

    public async ValueTask<AsyncGateReleaser> LockAsync()
    {
        IDisposable releaser = await this.Lock.LockAsync().ConfigureAwait(false);
        return new AsyncGateReleaser(release);
    }

    public void Release()
    {
        // Nothing to do, because the IDisposable we return via LockAsync does all the work
    }
}

That seems simpler, at the expense of a potentially more confusing design. (There are now two ways you can choose to implement gate release in IAsyncGate: either you can return an IDisposable wrapped in an AsyncGateReleaser or, if that's not convenient, you can just have the AsyncGateRelaser call Release (which is the only option in the current design).

Any thoughts?

[idg10]

Tagging @StephenCleary since this is based on his AsyncLock, and he might have a view on the above.

[timcassell]

What about:

public interface IAsyncGate
{
    ValueTask<IAsyncGateReleaser> LockAsync();
}

public interface IAsyncGateReleaser
{
    void Release();
}

Then you can implement like this:

internal class NitoAsyncGate : IAsyncGate
{
    public Nito.AsyncEx.AsyncLock Lock { get; } = new();

    public async ValueTask<IAsyncGateReleaser> LockAsync()
    {
        return new Releaser(await Lock.LockAsync().ConfigureAwait(false));
    }

    private class Releaser(IDisposable releaser) : IAsyncGateReleaser
    {
        public void Release() => releaser.Dispose();
    }
}

Or like this:

internal class MyCustomAsyncGate : IAsyncGate, IAsyncGateReleaser
{
    public MyCustomAsyncLock Lock { get; } = new();

    public async ValueTask<IAsyncGateReleaser> LockAsync()
    {
        await Lock.EnterAsync();
        return this;
    }

    public void Release() => Lock.Exit();
}

---

P.S. I implemented a custom AsyncLock that returns a disposable struct to avoid allocations. It's very efficient, and it would make me want to use public interface IAsyncGate<TAsyncGateReleaser> where TAsyncGateReleaser : IAsyncGateReleaser, but that's a bit of a mouthful, and probably not a very good fit for this library. I could still implement it like your first example.

[idg10]

If LockAsync is producing an IAsyncGateRelease, you can no longer use using to ensure lock release. You'd have to write:

IAsyncGateReleaser r = await gate.LockAsync();
try
{
    // Do protected work...
}
finally
{
    r.Release();
}

At that point, it would be simpler just to define the gate interface as this

public interface IAsyncGate
{
    ValueTask LockAsync();
    void Release();
}

And the usage model then becomes:

await gate.LockAsync();
try
{
    // Do protected work...
}
finally
{
    gate.Release();
}

But the current proposed LockAsync supports using:

using (await gate.LockAsync())
{
}

The obvious (but wrong) way to achieve this with your design would be to modify IAsyncGateReleaser to derive from IDisposable, but at that point, we may as well have LockAsync return ValueTask<IDisposable>. And there's a really good reason to avoid that, as I'll describe later. So that's not the way to go.

In fact, we can layer using support on top of your existing design. We could supply the following:

public struct DisposableGateReleaser(IAsyncGateReleaser gateReleaser) : IDisposable
{
    public void Dispose() => gateReleaser.Release();
}

public static class AsyncGateExtensions
{
    public static ValueTask<DisposableGateReleaser> LockDisposableAsync(this IAsyncGate2 gate)
    {
        // Note, we are avoiding async/await here because we MUST not create a new child execution
        // context. (The AsyncGate.LockAsync method does not use async/await either, and for the same reason.)
        // AsyncGate uses an AsyncLocal<int> to track the recursion count, and when you update an AsyncLocal<T>'s
        // value, that modified value is visible only in the current ExecutionContext and its descendants.
        // An async method effectively introduces a new child context, so any AsyncLocal<T> value changes are
        // lost when the async method returns. But we need the recursion count to live in our caller's context,
        // which is why we must make sure we don't introduce a new child context here. That's why this needs to
        // be old-school manual task management, and not async/await.
        ValueTask<IAsyncGateReleaser> releaserTask = gate.LockAsync();
        if (releaserTask.IsCompleted)
        {
            return ValueTask.FromResult(new DisposableGateReleaser(releaserTask.Result));
        }

        return new ValueTask<DisposableGateReleaser>(releaserTask.AsTask().ContinueWith(t => new DisposableGateReleaser(t.Result)));
    }
}

However, this reveals a possible performance issue. Consider these benchmarks:

[MemoryDiagnoser]
public class AsyncGateBenchmarks
{ 
    private IAsyncGate gate1 = new AsyncGate1();
    private IAsyncGate2 gate2 = new AsyncGate2();
    private IAsyncGate2 gate3 = new AsyncGate3();

    [Benchmark]
    public async ValueTask AcquireAndReleaseExistingDesignReducedAlloc()
    {
        using (await gate1.LockAsync())
        {
        }
    }

    [Benchmark]
    public async ValueTask AcquireAndReleasePerLockReleaser()
    {
        using (await gate2.LockDisposableAsync())
        {
        }
    }

    [Benchmark]
    public async ValueTask AcquireAndReleaseReleaserIsGate()
    {
        using (await gate3.LockDisposableAsync())
        {
        }
    }
}

The IAsyncGate here is the one that currently exists in the 6.0.0-alpha.44.g5ca1c61383 preview build available on the Rx.NET NuGet feed. IAsyncGate2 is your proposed design for IAsyncGate.

AsyncGate1 is almost the same code currently available in that preview, but I've realised as I've been working on this reply that it is unnecessarily efficient in the case where the semaphore can be acquired without waiting. (It always calls ContinueWith on the Task returned by _semaphore.WaitAsync().) My AsyncGate1 has a small modification: it checks to see if the semaphore was acquired without needing to wait, and in that case avoids creating a continuation. (For uncontended lock acquisition, this reduces memory allocation by 66%, and makes it run over 10x faster!)

AsyncGate2 is based on that modified AsyncGate1 but with LockAsync modified to match your new interface definition. It returns an instance of this implementation of IAsyncGateRelease

    private class Releaser(AsyncGate2 g) : IAsyncGateReleaser
    {
        public void Release() => g.Release();
    }

AsyncGate3 is also based on AsyncGate1, but it uses the second approach you suggested in which the gate itself implements the releaser interface:

public sealed class AsyncGate3 : IAsyncGate2, IAsyncGateReleaser
{
...
    void IAsyncGateReleaser.Release() => this.Release();
}

I get these results when running the benchmarks:

Method	Mean	Error	StdDev	Gen0	Allocated
AcquireAndReleaseExistingDesignReducedAlloc	83.50 ns	0.775 ns	0.725 ns	0.0153	192 B
AcquireAndReleasePerLockReleaser	88.18 ns	0.745 ns	0.697 ns	0.0172	216 B
AcquireAndReleaseReleaserIsGate	83.93 ns	0.680 ns	0.637 ns	0.0153	192 B

These benchmarks test the no-contention case, which is an important one because that's the one that has the most to gain from the low-allocation characteristics of ValueTask.

Notice that the AsyncGate2 implementation of the proposed new design allocates more memory.

You'd expect AsyncGate2 (the AcquireAndReleasePerLockReleaser benchmark) to use more memory because it's constructing a new AsyncGate2.Releaser each time we call LockAsync. That's a class so it needs to be allocated on the heap. Making it a struct wouldn't help because this design for IAsyncGate has this signature:

ValueTask<IAsyncGateReleaser> LockAsync();

we need to supply an IAsyncGateReleaser, and as an interface type that's a always a reference type. If our Releaser were a struct, it'd just get boxed in order to produce an IAsyncGateReleaser reference, so there would be an allocation anyway.

However, AsyncGate3 allocates exactly as much memory as in the existing design, and within the limits of experimental error it seems to have essentially the same performance. That's because this doesn't need to allocate an extra object on the heap to supply the IAsyncGateReleaser: it just returns the AsyncGate3 itself, which is already allocated on the heap, so no additional allocation is required.

So although, initially, I was worried that your design would necessarily require more allocation because it has LockAsync producing an interface, this shows that doesn't have to be a problem. As long as we're OK with the possibility that each call to LockAsync returns the same instance, then it's possible to get the same allocation characteristics as the current proposed design.

I can see just one potential problem. A possible advantage of the existing design is that it might be possible to detect misuse. Since each call to LockAsync returns a distinct AsyncGateReleaser, it might in theory be possible for us to detect when the caller is trying to release the lock from a different context than the one on which they acquired it. The current proposed implementation does not in fact do this in its AsyncGateRelease, though (and in fact the use of AsyncLocal means that the underlying AsyncGate can do this check anyway, without the help of the releaser wrapper).

So...we have two viable designs, apparently with equivalent performance characteristics. And the resulting wrapper looks simpler with your design, so I think that is probably preferable.