Allow fire and forget CancellationTokenRegisteration.Dispose

[davidfowl]

Right now disposing a CancellationTokenRegistration will wait for pending callbacks to run if it's disposed on a separate thread from the callback itself. This can cause deadlocks for e.g.:

aspnet/KestrelHttpServer#1278

It would be nice to have a way to disable the blocking on dispose. It might require an API change.

/cc @stephentoub

---

EDIT 6/29/2018 from stephentoub:
This is the proposed method to be added to CancellationTokenRegistration:

public struct CancellationTokenRegistration
{
    public bool Unregister();
    ...
}

We can separately have a discussion about adding a DisposeAsync when the corresponding interface comes along, but we know we can use this API and use it now (it's already there, just not public), and it has a different purpose than a DisposeAsync would. The semantics here are simply that whereas Dispose unregisters and then if the callback is currently running waits for it, this API just unregisters.

[davidfowl]

Here's the workaround we use to get around this:

https://github.com/aspnet/KestrelHttpServer/blob/ce80760c2f14c3a9622b6411aa41235c9cff8ec9/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/CancellationTokenExtensions.cs

[stephentoub]

Right now disposing a CancellationTokenRegistration will wait for pending callbacks to run if it's disposed on a separate thread from the callback itself

Yeah, the idea being that once Dispose returns, the caller can be sure that the callback won't be invoked, and they can proceed to use resources that may be used by the callback without concurrency concerns.

It would be nice to have a way to disable the blocking on dispose. It might require an API change.

It should be trivial, but would also require an API addition:

public void Dispose() => Dispose(true);
public void Dispose(bool waitForCallbacksToComplete)
{
    ...
}

Essentially if waitForCallbacksToComplete was false, it would just exit after the TryDeregister call:
https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Threading/CancellationTokenRegistration.cs#L65

[clairernovotny]

Just a note that public void Dispose(bool waitForCallbacksToComplete) could conflict with the common, and recommended, pattern of protected void Dispose(bool disposing) to indicate a Dispose call from the Finalizer.

[stephentoub]

Just a note that public void Dispose(bool waitForCallbacksToComplete) could conflict with the common, and recommended, pattern of protected void Dispose(bool disposing) to indicate a Dispose call from the Finalizer.

Not on a struct, which CancellationTokenRegistration is. Such Dispose(bool) methods according to the pattern are also protected. But I understand the general point around a conflict in understandability. It could be exposed through some other overload or name if it was really concerning.

[clairernovotny]

Yup, it's more about a conflict of understandability where that particular method signature already has a common/popular meaning.

[jnm2]

I dislike this because it forces me to write my own try...finally blocks. I'd really prefer using (token.Register(action).WaitForCallbacksToComplete(false)) or similar.

[davidfowl]

@stephentoub what about adding a Register overload on CancellationToken?

public CancellationTokenRegistration Register(Action<object> callback, bool useSynchronizationContext, bool waitForCallbacksToComplete, object state);

Not sure how many overloads we'd want to add here since there's already 4 overloads.

[stephentoub]

what about adding a Register overload on CancellationToken?

Potentially, but that Boolean value would need to propagate to the disposal, which means it would need to be stored somewhere. Given the current shape of things, that could be either in the CancellationCallbackInfo, which would increase the size of that allocation by a word, or it would be on the CancellationTokenRegistration struct, which would increase its size by the same, and while it's a struct, it's often stored as a field in some other object (like the boxed state machine for an async method), so increasing its size has a similar effect on the heap.

[jnm2]

In using (token.Register(action).WaitForCallbacksToComplete(false)), WaitForCallbacksToComplete could return a struct containing a CancellationTokenRegistration and which on disposal calls CancellationTokenRegistration.Dispose(waitForCallbacksToComplete: false).

[clairernovotny]

@stephentoub it's cheating a bit, but what if you use a similar trick to dotnet/coreclr#2883, and use a derived type of CancellationCallbackInfo to indicate the flag for waitForCallbacksToComplete. Then it's a typeof check in the Dispose method itself?

Given what's currently there, I'd see the following:

CancellationCallbackInfo
NoWaitCancellationCallbackInfo : CancellationCallbackInfo
WithSyncContextNoWait : NoWaitCancellationCallbackInfo
WithSyncContext : CancellationCallbackInfo

[stephentoub]

what if you use a similar trick

It's possible and worth exploring. But it's also more expensive in this case, doing multiple type checks on the common path, whereas that other case only did a type check when cancellation was requested, which is at least an order of magnitude less common than registering/unregistering.

[clairernovotny]

Just thought of something to avoid the typeof - what about the same hierarchy but a polymorphic property in the types virtual bool ShouldWait { get;}. Then that could return true or false depending, thereby avoiding the type checking.

[stephentoub]

As with the other, it's a good suggestion that can be explored to understand what perf impact it has (due to adding a virtual call on a common path). I expect we'd need to pick either to introduce another struct as a return type or to take such a perf hit... and which we'd choose would in part depend on whether it's actually measurable and to what extent.

[clairernovotny]

Yup, makes sense. One further refinement. Factor the current callbackinfo class into an abstract base with four impls that return the correct val for the property but can then all be sealed.

…

Sent from my Windows 10 phone From: Stephen Toub<mailto:[email protected]> Sent: Sunday, January 8, 2017 10:45 PM To: dotnet/corefx<mailto:[email protected]> Cc: Oren Novotny<mailto:[email protected]>; Comment<mailto:[email protected]> Subject: Re: [dotnet/corefx] Allow fire and forget CancellationTokenRegisteration.Dispose (#14903) As with the other, it's a good suggestion that can be explored to understand what perf impact it has (due to adding a virtual call on a common path). I expect we'd need to pick either to introduce another struct as a return type or to take such a perf hit... and which we'd choose would in part depend on whether it's actually measurable and to what extent. - You are receiving this because you commented. Reply to this email directly, view it on GitHub<https://github.com/dotnet/corefx/issues/14903#issuecomment-271206821>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABXHVO75dWlNxH3EtxzDdmJFNSssIMUPks5rQa1tgaJpZM4LcBDs>.

[davidfowl]

@stephentoub do you have a preference here?

[stephentoub]

@stephentoub do you have a preference here?

My preference would be either:

public void Dispose(bool waitForRegistration);

or

public void DisposeNoWait();

though I'm not particularly attached to this naming if folks don't like it and have better ideas. And I understand the objections to the first one, so maybe the second.

The current implementation is the way it is because the vast majority case is needing to ensure that the registration won't fire after Dispose; logic in many situations gets broken if it could. I see the value in being able to disable it, but I don't think it's going to be so common that we should expend a lot of energy/surface area in making it super easy to use (and in the process complicating the majority case), e.g. I don't think we should add new types to enable using it with using (if someone really finds themselves doing it a lot, nothing stops them from adding such support on top of this).

My $.02.

[stephentoub]

@davidfowl, how do you feel about:

public void DisposeWithoutWaiting();

?

[sharwell]

@stephentoub Dispose pattern is already so widely misunderstood, I can't see anything but problems coming from a Dispose(bool) signature. I agree with @jnm2 that any solution which accommodates using statements without the overhead of extra allocation is preferable to a solution which does not allow such usage. Within those bounds I don't believe I have a particular preference.

[stephentoub]

I agree with @jnm2 that any solution which accommodates using statements without the overhead of extra allocation is preferable to a solution which does not allow such usage

And I don't believe this is a common enough need to add any overhead to the existing code paths. Do you see a solution that meets that? That's why I'm suggesting a separate one-off method; for the 5% case where this is needed, people can use a try/finally, if it's even in a finally... often usage is such that disposal is in an entirely separate method.

[jnm2]

@stephentoub If you add DisposeWithoutWaiting(), I will certainly write a WaitForCallbacksToComplete(false) extension method to wrap the registration with a struct that calls DisposeWithoutWaiting when it is disposed, if you don't include such an API on CancellationTokenRegistration.

[stephentoub]

I will certainly write a WaitForCallbacksToComplete(false)extension method to wrap the registration with a struct that calls DisposeWithoutWaiting when it is disposed

And you're of course welcome to do that.

[jnm2]

Seems like that would suit everyone then. What exactly is it you don't like about having just a WaitForCallbacksToComplete(false) API? Is it the extra bool in the wrapper struct? If so, what about WithoutWaiting() with a wrapper struct that unconditionally calls .DisposeWithoutWaiting()? That should get inlined completely away, right?

[stephentoub]

Is it the extra bool in the wrapper struct?

Yes, or more generally, the need for either a different public registration struct type or bloating the size of CancellationTokenRegistration (or one of the heap-allocated objects it references). Keep in mind, it's not just "a bool"... the types involved here are already "full" with regards to alignment, so an additional bool will actually increase the size of the CancellationTokenRegistration by 8 bytes (on 64-bit), and CTRs are often stored in heap-allocated objects, so this little bool for a rarely used feature will end up bloating many objects by another 8 bytes. I'm trying to avoid that.

If so, what about WithoutWaiting() with a wrapper struct that unconditionally calls .DisposeWithoutWaiting()?

That's adding yet another public type. In my experience, this need is in the far minority. In my opinion, it's useful enough to enable the functionality but with minimal surface area, i.e. a single method that can be used when needed, rather than yet another IDisposable-implementing struct type and a method that returns it on top of the minimal method that's needed. But if in the future it became clear that I'm wrong and such a thing would be useful, and it's problematic for some reason for folks to write:

var ctr = ct.Register(...);
try
{
    ...
}
finally { ctr.DisposeWithoutWaiting(); }

instead of:

using (ct.Register(...).WithoutDisposalWaiting())
{
    ...
}

then such an additional struct-type and helper method could be added in the future, and in the meantime folks could write it on their own if they really wanted to.

[davidfowl]

IMO DisposeWithoutWaiting is fine because in situations like this, I'm almost never in a using. It's always some other method doing the disposal.

[stephentoub]

IMO DisposeWithoutWaiting is fine because in situations like this, I'm almost never in a using. It's always some other method doing the disposal.

Exactly

[sharwell]

@stephentoub I think you (slightly) misread what I wrote. One solution which would meet the conditions I described is a solution which allows a user to write their own extension method which returns a struct wrapping the cancellation registration for the purpose of calling DisposeWithoutWaiting at the end of a using statement.

[stephentoub]

a solution which allows a user to write their own extension method

Ah, I misunderstood what you were saying was your min bar. In that case, sounds like we're in agreement.

[terrajobst]

We just discussed this. It seems to be a general issue with synchronous Dispose. If we had an asynchronous Dispose, we'd probably like something more along the lines of:

public struct CancellationTokenRegistration
{
    public Task DisposeAsync();
    ...
}

@stephentoub offered to write up a proposal for discussion with LDM on async dispose.

[sharwell]

@terrajobst It wouldn't happen to look anything like dotnet/roslyn#114, would it? 😉

[karelz]

FYI: The API review discussion was recorded - see https://youtu.be/VppFZYG-9cA?t=152 (10 min duration)

[davidfowl]

@stephentoub offered to write up a proposal for discussion with LDM on async dispose.

YESSSS it'll also fix our stream problems

[stephentoub]

I've updated the top post with a revised proposal. I think we should add Unregister now; then we can separately look at adding DisposeAsync as part of implementing a new IAsyncDisposable interface. They serve different purposes, though, and we actually already have Unregister, it's just not exposed publicly.

[terrajobst]

That makes sense. Looks good!

[davidfowl]

Wooo!

[stephentoub]

😄