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:notifications@github.com> Sent: Sunday, January 8, 2017 10:45 PM To: dotnet/corefx<mailto:corefx@noreply.github.com> Cc: Oren Novotny<mailto:oren@novotny.org>; Comment<mailto:comment@noreply.github.com> 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?