-
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
Draft
idg10
wants to merge
2
commits into
main
Choose a base branch
from
feature/2148-pluggable-async-gate
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
15 changes: 15 additions & 0 deletions
15
AsyncRx.NET/System.Reactive.Async/Threading/AsyncGateReleaser.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT License. | ||
// See the LICENSE file in the project root for more information. | ||
|
||
namespace System.Reactive.Threading | ||
{ | ||
public readonly struct AsyncGateReleaser : IDisposable | ||
{ | ||
private readonly IAsyncGate _parent; | ||
|
||
public AsyncGateReleaser(IAsyncGate parent) => _parent = parent; | ||
|
||
public void Dispose() => _parent.Release(); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT License. | ||
// See the LICENSE file in the project root for more information. | ||
|
||
using System.Reactive.Linq; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
|
||
namespace System.Reactive.Threading | ||
{ | ||
/// <summary> | ||
/// Synchronization primitive that provides <see cref="System.Threading.Monitor"/>-style | ||
/// exclusive access semantics, but with an asynchronous API. | ||
/// </summary> | ||
/// <remarks> | ||
/// <para> | ||
/// This enables <see cref="AsyncObservable.Synchronize{TSource}(IAsyncObservable{TSource}, IAsyncGate)"/> | ||
/// and <see cref="AsyncObserver.Synchronize{TSource}(IAsyncObserver{TSource}, IAsyncGate)"/> | ||
/// to be used to synchronize access to an observer with a custom synchronization primitive. | ||
/// </para> | ||
/// <para> | ||
/// These methods model the equivalents for <see cref="IObservable{T}"/> and <see cref="IObserver{T}"/> | ||
/// in <c>System.Reactive</c>. Those offer overloads accepting a 'gate' parameter, and if you pass | ||
/// the same object to multiple calls to these methods, they will all synchronize their operation | ||
/// through that same gate object. The <c>gate</c> parameter in those methods is of type | ||
/// <see cref="System.Object"/>, which works because all .NET objects have an associated monitor. | ||
/// (It's created on demand when you first use <c>lock</c> or something equivalent.) | ||
/// </para> | ||
/// <para> | ||
/// That approach is problematic in an async world, because this built-in monitor blocks the | ||
/// calling thread when contention occurs. The basic idea of AsyncRx.NET is to avoid such | ||
/// blocking. It can't always be avoided, and in cases where we can be certain that lock | ||
/// acquisition times will be short, the conventional .NET monitor is still a good choice. | ||
/// But since these <c>Synchronize</c> operators allow the caller to pass a gate which the | ||
/// application code itself might lock, we have no control over how long the lock might be | ||
/// held. So it would be inappropriate to use a monitor here. | ||
/// </para> | ||
/// <para> | ||
/// Since the .NET runtime does not currently offer any asynchronous direct equivalent to | ||
/// monitor, this interface defines the required API. The <see cref="AsyncGate"/> class | ||
/// provide a basic implementation. If applications require additional features, (e.g. | ||
/// if they want cancellation support when the application tries to acquire the lock) | ||
/// they can provide their own implementation. | ||
/// </para> | ||
/// </remarks> | ||
public interface IAsyncGate | ||
{ | ||
/// <summary> | ||
/// Acquires the lock. | ||
/// </summary> | ||
/// <returns> | ||
/// A task that completes when the lock has been acquired, returning an <see cref="AsyncGateReleaser"/> | ||
/// which can be disposed to release the lock. | ||
/// </returns> | ||
/// <remarks> | ||
/// <para> | ||
/// Applications release the lock by disposing the <see cref="AsyncGateReleaser"/> returned by this | ||
/// method. Typically this is done with a <c>using</c> statement or declaration. | ||
/// </para> | ||
/// </remarks> | ||
public ValueTask<AsyncGateReleaser> LockAsync(); | ||
|
||
/// <summary> | ||
/// Releases the lock. Applications typically won't call this directly, and will use | ||
/// 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"/> | ||
/// can be shared by all implementations of this interface. | ||
/// </remarks> | ||
public void Release(); | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 makeLockAsync
returnValueTask<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 returningDisposable.Create(Release)
inAsyncGate.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 calledLockAsync
.In the current implementation (i.e., before this PR)
LockAsync
returns astruct
, and although that does implementIDisposable
, C# is able to use itsDispose
method fromusing
statements or declarations directly without needing to cast it toIDisposable
. (Casting astruct
to an interface type causes it to be boxed.)So when you write this sort of thing:
it becomes something like this:
But if you make
LockAsync
returnValueTask<IDisposable>
C# has to do something more like this: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-blockingawait
is allocation-free, and by having it return astruct
, no allocation is required for theusing
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 enableusing
, 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 theIAsyncGate
, which defines theLeave
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 beIDisposable
. But the whole point ofAsyncGateReleaser
is that gives you anIDisposable
(so you can useusing
) 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 useusing
.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 ofEnter
andLeave
methods is a common kind of API for locks, so it makes sense. (And it doesn't make sense forAsyncGate
to implementIDisposable
becauseLeave
very much isn't the same thing asDispose
.) We could imagine anILeaveable
which captured this "I'm done with this for now, but might use it again" (as opposed to the inherently terminalIDispose
) 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.