-
Pardon the long-winded writeup. This isn't regarding anything related to Rx but just something I came across when trying to use I have a scenario where I want to create a "scope" that can be entered in a multi-threaded environment. The first time the scope is entered, it performs some asynchronous entry action, then when all the scopes are left it performs some asynchronous exit action. After that it resets so the next entry calls the entry action, etc.
private RefCountAsyncDisposable? refCounted;
private readonly AsyncGate gate = new();
async Task<IAsyncDisposable> GetScopeBad()
{
using var _ = await gate.LockAsync();
if(refCounted == null)
{
/* do entry action */
refCounted = new RefCountAsyncDisposable(AsyncDisposable.Create(async () => {
/* do exit action */
refCounted = null;
}));
var scope = await refCounted.GetDisposableAsync();
await refCounted.DisposeAsync();
return scope;
}
// Race condition here if previous scopes are now exited
return await refCounted.GetDisposableAsync();
} This works for the most part, the first time around I can think of one race condition where we've entered the scope previously so Here is my second approach: async Task<IAsyncDisposable> GetScopeGood()
{
using var _ = await gate.LockAsync();
var scope = refCounted != null ? await refCounted.GetDisposableAsync() : AsyncDisposable.Nop;
if(scope == AsyncDisposable.Nop)
{
/* do entry action */
refCounted = new RefCountAsyncDisposable(AsyncDisposable.Create(async () => {
/* do exit action */
}));
scope = await refCounted.GetDisposableAsync();
await refCounted.DisposeAsync();
return scope;
}
// Now if previous scopes are exited, the scope obtained above will keep refCounted from disposing
return scope;
} I believe this covers the aforementioned race condition but relies on Am I overthinking this? Is there something better suited for this kind of thing? Here's my full interactive c# playground demoing the race condition: #r "nuget: System.Reactive.Async, 6.0.0-alpha.18"
#nullable enable
using System.Reactive.Disposables;
using System.Threading.Tasks;
using System.Threading;
using System;
RefCountAsyncDisposable? refCounted;
AsyncGate gate = new();
delegate Task<IAsyncDisposable> GetScopeDelegate(IAsyncDisposable? previous = null);
async Task SimulateRaceCondition(IAsyncDisposable previous)
{
Console.WriteLine("Simulating race condition");
using(ExecutionContext.SuppressFlow())
{
// need to suppress flow for async gate to simulate properly, but Console.WriteLine will no longer work (???)
Task.Run(previous.DisposeAsync);
}
// Some time to make sure its been called if its going to be called
await Task.Delay(100);
Console.WriteLine("Done simulating race condition");
}
async Task<IAsyncDisposable> GetScopeBad(IAsyncDisposable? previous = null)
{
using var _ = await gate.LockAsync();
if(refCounted == null)
{
Console.WriteLine("refCount reset");
refCounted = new RefCountAsyncDisposable(AsyncDisposable.Create(async () => {
using var _ = await gate.LockAsync();
refCounted = null;
Console.WriteLine("dispose complete");
}));
var scope = await refCounted.GetDisposableAsync();
await refCounted.DisposeAsync();
return scope;
}
if(previous != null) await SimulateRaceCondition(previous);
return await refCounted.GetDisposableAsync();
}
async Task<IAsyncDisposable> GetScopeGood(IAsyncDisposable? previous = null)
{
using var _ = await gate.LockAsync();
var scope = refCounted != null ? await refCounted.GetDisposableAsync() : AsyncDisposable.Nop;
if(scope == AsyncDisposable.Nop)
{
Console.WriteLine("refCount reset");
refCounted = new RefCountAsyncDisposable(AsyncDisposable.Create(async () => {
using var _ = await gate.LockAsync();
Console.WriteLine("dispose complete");
}));
scope = await refCounted.GetDisposableAsync();
await refCounted.DisposeAsync();
return scope;
}
if(previous != null) await SimulateRaceCondition(previous);
return scope;
}
GetScopeDelegate GetScope = GetScopeBad;
// GetScopeDelegate GetScope = GetScopeGood;
async Task PrintDisposeState()
{
var scope = refCounted != null ? await refCounted.GetDisposableAsync() : AsyncDisposable.Nop;
Console.WriteLine($"refCounted is {(scope == AsyncDisposable.Nop ? "disposed": "not disposed")}");
await scope.DisposeAsync();
}
async Task Run()
{
Console.WriteLine("Acquiring 1...");
var r1 = await GetScope();
Console.WriteLine("Acquiring 2 and disposing 1 (race condition!)...");
var r2 = await GetScope(r1);
await PrintDisposeState();
Console.WriteLine("Disposing 2");
await r2.DisposeAsync();
}
await Run();
Console.WriteLine("At this point we should be disposed...");
await PrintDisposeState();
Console.WriteLine("Running again...");
await Run(); BAD (without lock):
BAD (with lock):
GOOD:
|
Beta Was this translation helpful? Give feedback.
Replies: 2 comments 1 reply
-
Although it's possible to create an
The reason I bring up the general pattern, and The danger is that if you misuse a To generalize that: if we take things that are normally one-shot and make them resettable, we introduce the potential for a bug that can't be detected at runtime. If an object is used after being reset it has no way of knowing whether that's a correct use from its new owner, or it's the original owner incorrectly trying to use it after it is no longer the owner. So for this reason I don't want to make any of our Also, the primary purpose of the Rx Disposables is to do what Rx needs. Since they are public, we will fully support their existing API into the future. And that commitment cuts both ways: since we intend to support all public features of these types, that means adding new capabilities increases ongoing maintenance overheads. So for that reason, we don't really want to be expanding the feature set of these types outside of what Rx itself need from them. That's not an absolutely inflexible position. I would always consider a request for new functionality, and there may well be cases where we would actually add something new. However in this particular case, the change from "one shot" to "resettable" is a big change, and as I've described above, one I consider to enable somewhat risky patterns. For the problem you're trying to solve, I my first thought would have been an |
Beta Was this translation helpful? Give feedback.
-
Once again thanks @idg10 for the detailed response! I posted this as a discussion and not an issue because I don't want to make any changes to existing Rx Disposables API, but I wanted to discuss how I'm using the public API for this specific use case. Using a thread-safe counter is much simpler than what I had using Here's what I ended up with: public async Task<IAsyncDisposable> GetScopeAsync(CancellationToken cancellationToken)
{
using (await _gate.LockAsync())
{
_counter++;
if(_counter == 1)
{
await _onEnter(cancellationToken);
}
}
return AsyncDisposable.Create(async ()=>
{
using var _ = await _gate.LockAsync();
_counter--;
if(_counter == 0)
{
await _onExit();
}
});
} |
Beta Was this translation helpful? Give feedback.
Although it's possible to create an
IDiposable
that can be undisposed, so to speak, it's not the norm, and it presents some dangers.IDisposable
is an instance of a more general pattern of a "one-shot" kind of usage model.Task
is another. From the consumer's perspective, these types will do their job and then you can't do anything more with them. If you've got anIDisposable
object then the normal rule are that you can't use it again after you callDispose
. If you've got aTask
, it completes once, then remains in a completed state and you can't ask it to run again.The reason I bring up the general pattern, and
Task
in particular is thatValueTask
sheds some light on what happens if you …