-
Notifications
You must be signed in to change notification settings - Fork 751
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
base: main
Are you sure you want to change the base?
Conversation
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.
/// 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"/> |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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 useobject
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 theAsyncGate
type. This makes it possible to use application-supplied gate implementations.Resolves #2148