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

Enable application-defined Synchronize gate in AsyncRx.NET #2153

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

idg10
Copy link
Collaborator

@idg10 idg10 commented Jul 18, 2024

The AsyncRx.NET Synchronize overloads that accept an application-supplied 'gate' (enabling multiple synchronized observables and observers to share a single lock, and also enabling application to acquire that same lock itself) can't use object like Rx.NET does, because the monitor built into every .NET object supports only blocking lock acquisition. Avoiding thread blocking is a big reason for using AsyncRx.NET, so the normal .NET monitor doesn't really work here.

So AsyncRx.NET defines an AsyncGate type to enable this kind of shared lock but with an asynchronous lock acquisition method.

Our AsyncGate does not support cancellation, and for now at least we don't want to add it. (For one thing, it opens the can of worms of whether we want to attempt to support cancellation across the board in AsyncRx.NET. But also, more or less everyone who tries to add cancellation support to this sort of primitive ends up creating subtle bugs.)

However, we want it to be possible for applications that want cancellable awaits to have them in exchange for a bit of work. So this defines an IAsyncGate interface modifies the Synchronize operators to use that instead of requiring the AsyncGate type. This makes it possible to use application-supplied gate implementations.

Resolves #2148

Our AsyncGate does not support cancellation, and for now at least we don't want to add it. (For one thing, it opens the can of worms of whether we want to attempt to support cancellation across the board in AsyncRx.NET. But also, more or less everyone who tries to add cancellation support to this sort of primitive ends up creating subtle bugs.)

So this defines an IAsyncGate interface and Synchronize overloads that accept it, enabling them to work with application-supplied implementations.
@idg10 idg10 self-assigned this Jul 18, 2024
/// the <see cref="AsyncGateReleaser"/> returned by <see cref="LockAsync"/> instead.
/// </summary>
/// <remarks>
/// This method needs to be publicly accessible so that a single <see cref="AsyncGateReleaser"/>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make more sense to get rid of this method, make AsyncGateReleaser internal, and make LockAsync return ValueTask<IDisposable>, and let other implementers decide how to release the lock in the dispose?

And actually, I don't really understand the advantage of having a separate AsyncGateReleaser class vs returning Disposable.Create(Release) in AsyncGate.LockAsync

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or is it to keep the semantics of a lock separate from the semantics of IDisposable?

Copy link
Collaborator Author

@idg10 idg10 Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for not returning IDisposable is that that would require an allocation on the GC heap every time you called LockAsync.

In the current implementation (i.e., before this PR) LockAsync returns a struct, and although that does implement IDisposable, C# is able to use its Dispose method from using statements or declarations directly without needing to cast it to IDisposable. (Casting a struct to an interface type causes it to be boxed.)

So when you write this sort of thing:

using (await gate.LockAsync())
{
    // Do work
}

it becomes something like this:

// Note: variable is of `struct` type, so no heap allocation required
AsyncGate.Releaser d = await gate.LockAsync();
try
{
     // Do work...
}
finally
{
    d.Dispose();
}

But if you make LockAsync return ValueTask<IDisposable> C# has to do something more like this:

// Note: variable is of an interface type, so it has to refer to an object on the heap
IDisposable d = await gate.LockAsync();
try
{
     // Do work...
}
finally
{
    d.Dispose();
}

With the existing design, in cases where there was no contention for the lock, the whole 'acquire, release' sequence could be completely allocation-free. We're using ValueTask<T> so a non-blocking await is allocation-free, and by having it return a struct, no allocation is required for the using logic either.

I wanted to preserve that characteristic. Most people aren't going to bring their own gate implementation, and I didn't want to increase the cost for the most common case in which people just use AsyncGate.

I did consider making the interface generic: IAsyncGate<TReleaser> where TReleaser : struct, IDisposable. This would enable each implementation to bring its own releaser implementation. But really the only point of the releaser is to enable using, so I didn't think there was really any use in making every implementation write its own version.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@idg10 would it make sense to consider adding a ValueDisposable helper class to assist creation of allocation-free dispose semantics?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glopesdev what would this ValueDisposable take as its input?

This AsyncGateReleaser takes the IAsyncGate, which defines the Leave method that it calls. So it is specialized for this scenario.

A more general-purpose ValueDisposable would need some more general purpose resource release mechanism, which would presumably be IDisposable. But the whole point of AsyncGateReleaser is that gives you an IDisposable (so you can use using) in a scenario where you don't already have one.

If you've already got an IDisposable, you don't need to wrap it. You can already use using.

So I think this pattern is only useful in cases where you have something other than IDisposable to start with. In this particular case, a pair of Enter and Leave methods is a common kind of API for locks, so it makes sense. (And it doesn't make sense for AsyncGate to implement IDisposable because Leave very much isn't the same thing as Dispose.) We could imagine an ILeaveable which captured this "I'm done with this for now, but might use it again" (as opposed to the inherently terminal IDispose) then I could see how a generic type of this kind could work. But there isn't such an interface, and it feels scope-creepish for Rx to define it.

But perhaps I'm missing how this would work in the absence of such an interface.

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

Successfully merging this pull request may close these issues.

Feature Request: Make AsyncGate.LockAsync cancellable
3 participants