Skip to content
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

Proposal: Asynchronously cancel a CancellationTokenSource #23405

Closed
kevingosse opened this issue Aug 31, 2017 · 5 comments · Fixed by #81444
Closed

Proposal: Asynchronously cancel a CancellationTokenSource #23405

kevingosse opened this issue Aug 31, 2017 · 5 comments · Fixed by #81444
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Threading.Tasks
Milestone

Comments

@kevingosse
Copy link
Contributor

Proposal: Add a way to asynchronously cancel a CancellationTokenSource

I would like to suggest adding a method to CancellationTokenSource:

public Task CancelAsync()

The method would work like Cancel, with a difference:

  • If no callback is registered, then the method processes synchronously and returns a completed task
  • If callbacks are registered, then a task is created and returned. That task will execute the callbacks and propagate the exceptions if any (just like Cancel)

Alternatively, it could just be fire-and-forget:

public void CancelAsync()

Use case:

Whenever a CancellationTokenSource is cancelled, all registration callbacks are executed synchronously. While this is OK in most cases, I ran into the following scenario:

  • I have a long-running thread that can't afford to be held for a long time. This thread is in charge of cancelling the token (among other things). The thread is part of a framework.
  • The CancellationToken is handed to client code, over which I have no control. So I can't make sure that all callbacks can execute in a short time

My problem is: in this scenario, how to guarantee that the client code won't ever block my thread for a significant amount of time?

The most obvious solution is to call Cancel in another thread:

Task.Run(cts.Cancel);

This would work. However, I would end up queuing work to the threadpool everytime I cancel a token, even though most of the tokens won't have a callback registered. It feels wrong having to add a cost to the 99% case to handle the 1%.

The "CancelAsync" method can't be implemented from outside:

  • To the best of my knowledge, there is no way to know whether a callback is registered on a CancellationToken
  • Even if there was, there would still be a race condition between the registration and the check

I don't see any major obstacle to implementation, and I think it can be done without adding an additional field to the class. That said, I'm aware of how specific is the scenario I described, and I don't know if many applications would benefit from this.


Past discussion

This was moved from https://github.com/dotnet/coreclr/issues/13494
Copy of the discussion:


davidfowl

This is similar to dotnet/corefx#14903 (but for the actual cancellation call, not dispose). I think you need to be clear if you want to

The "CancelAsync" method can't be implemented from outside

Sure it can be. See http://referencesource.microsoft.com/#System.Web/Util/CancellationTokenHelper.cs,cbfb354e962dfb2a for an example of how it could be done.

There are some questions I have based on this request

  • Do you want the ability to wait on the callbacks?
  • Do you want wait on async callbacks? That is, if the user code you were calling was itself async, would you want to await that?

kevingosse

Sure it can be. See http://referencesource.microsoft.com/#System.Web/Util/CancellationTokenHelper.cs,cbfb354e962dfb2a for an example of how it could be done.

If I understand this code correctly, it always uses the threadpool to call Cancel. I was thinking of a way to use the threadpool only if there is a callback registered, and otherwise execute the call inline. The rationale being that in most cases there will be no callback (this is true for the codebase I work on, I'm not sure how representative it is).

Do you want the ability to wait on the callbacks?

I don't need it in my scenario, that's why I also suggested the fire-and-forget version. In fact, I was first thinking of a Cancel(bool executeCallbacksAsynchronously) overload, but that's not possible because there's already a Cancel(bool throwOnFirstException). Then I thought of CancelAsync. I'm not sure the task would actually be useful to anybody.

Do you want wait on async callbacks? That is, if the user code you were calling was itself async, would you want to await that?

Same answer as above.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@stephentoub stephentoub removed the untriaged New issue has not been triaged by the area owner label Feb 25, 2020
@stephentoub stephentoub modified the milestones: 5.0, Future Feb 25, 2020
@mlidbom
Copy link

mlidbom commented Jul 18, 2020

I had a look at the CancellationTokenSource code and put together this hack. It uses a private field in CancellationTokenSource to figure out if any callbacks have ever been registered and decides based on that whether to invoke Cancel synchronously or via Task.Run.

I'm offering it here as a possible workaround for those for whom whom this behaviour is badly needed.

A warning though. I'm anything but an expert at async code. I'm inviting those who are to please check if it has any major disadvantages other than being vulnerable to internal changes in the CancellationTokenSource class.

EDIT: The original version below gives no option to await the async invocation of the callbacks . This extension method version provides that ability.
EDIT3: Fixed that in the version below. Some might prefer an extension to replacing the class though so I'm leaving this version around.

//Hack to implement the suggested framework fix from here: https://github.com/dotnet/runtime/issues/23405 so that calling cancel on a CancellationTokenSource does not call registrations synchronously.
public static class CancellationTokenSourceAsyncExtension
{
    public static Task CancelAsync(this CancellationTokenSource source)
    {
        if(source.HasOrHasHadCallbacks())
        {
            return Task.Run(source.Cancel);
        } else
        {
            source.Cancel();
            return Task.CompletedTask;
        }
    }

    static bool HasOrHasHadCallbacks(this CancellationTokenSource source) => GetCallbackPartitionsAsObject(source) != null;

    static readonly Func<CancellationTokenSource, IEnumerable> GetCallbackPartitionsAsObject = CreateCallbackPartitionsAccessor();

    static Func<CancellationTokenSource, IEnumerable> CreateCallbackPartitionsAccessor()
    {
        FieldInfo callbackPartitionsField = typeof(CancellationTokenSource).GetField("_callbackPartitions", BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance)
                                         ?? throw new Exception("Failed to find the internal field:_callbackPartitions. You may be running a different version of .Net than this hack supports.");

        var cancellationTokenSource = Expression.Parameter(typeof(CancellationTokenSource), "cancellationTokenSource");

        return Expression.Lambda<Func<CancellationTokenSource, IEnumerable>>(
            Expression.Convert(
                Expression.Field(cancellationTokenSource, callbackPartitionsField),
                typeof(IEnumerable)),
            cancellationTokenSource).Compile();
    }
}

EDIT2: Name the method to make it clear that it is async and return Task that can be awaited

using System;
using System.Collections;
using System.Linq.Expressions;
using System.Reflection;
using System.Threading;
using System.Threading.Tasks;
using NUnit.Framework;

namespace CancellationTokenSourceExtensions
{
    //Hack to implement the suggested framework fix from here: https://github.com/dotnet/runtime/issues/23405 so that calling cancel on a CancellationTokenSource does not call registrations synchronously.
    public sealed class AsyncCancellationTokenSource : IDisposable
    {
        static readonly Func<CancellationTokenSource, IEnumerable> GetCallbackPartitionsAsObject = CreateCallbackPartitionsAccessor();
        readonly CancellationTokenSource _source;

        public AsyncCancellationTokenSource() => _source = new CancellationTokenSource();
        public AsyncCancellationTokenSource(TimeSpan delay) => _source = new CancellationTokenSource(delay);
        public AsyncCancellationTokenSource(int millisecondsDelay) => _source = new CancellationTokenSource(millisecondsDelay);

        public bool HasOrHasHadCallbacks => GetCallbackPartitionsAsObject(_source) != null;

        public CancellationToken Token => _source.Token;

        public bool IsCancellationRequested => _source.IsCancellationRequested;

        public Task CancelAsync()
        {
            if(HasOrHasHadCallbacks)
            {
                return Task.Run(() => _source.Cancel());
            } else
            {
                _source.Cancel();
                return Task.CompletedTask;
            }
        }

        public void CancelAfter(TimeSpan delay) => _source.CancelAfter(delay);

        public void CancelAfter(int millisecondsDelay) => _source.CancelAfter(millisecondsDelay);

        public void Dispose() => _source.Dispose();

        static Func<CancellationTokenSource, IEnumerable> CreateCallbackPartitionsAccessor()
        {
            FieldInfo callbackPartitionsField = typeof(CancellationTokenSource).GetField("_callbackPartitions", BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance)
                                             ?? throw new Exception("Failed to find the internal field:_callbackPartitions. You may be running a different version of .Net than this hack supports.");

            var cancellationTokenSource = Expression.Parameter(typeof(CancellationTokenSource), "cancellationTokenSource");

            return Expression.Lambda<Func<CancellationTokenSource, IEnumerable>>(
                Expression.Convert(
                    Expression.Field(cancellationTokenSource, callbackPartitionsField),
                    typeof(IEnumerable)),
                cancellationTokenSource).Compile();
        }
    }

    [TestFixture] public class AsyncCancellationTokenSourceTests
    {
        AsyncCancellationTokenSource _tokenSource;

        [SetUp] public void SetupTask() => _tokenSource = new AsyncCancellationTokenSource();

        [TearDown] public void TearDownTask() => _tokenSource.Dispose();

        [Test] public void WithNoCallbacksRegisteredCancelIsInvokedSynchronously()
        {
            _tokenSource.CancelAsync();
            //It seems unlikely that a thread has been spawned and changed the value between this thread executing the previous line and this line
            Assert.True(_tokenSource.Token.IsCancellationRequested);
        }

        [Test] public void WithCallbacksCancelIsInvokedAsynchronously()
        {
            _tokenSource.Token.Register(() => Thread.Sleep(1000));
            var now = DateTime.UtcNow;
            _tokenSource.CancelAsync();
            Assert.LessOrEqual((DateTime.UtcNow - now),
                               TimeSpan.FromMilliseconds(100),
                               "If we syncronously wait for the sleep in the registered callback we should not get here for at least 1000 milliseconds");
        }

        //This test verifies that the class does not perform as optimally as might be wished for. If it starts failing we should be happy :)
        [Test] public void IfCallbacksHaveBeenRegisteredAndRemovedCancelIsStillInvokedAsynchronously()
        {
            var registration = _tokenSource.Token.Register(() => {});
            registration.Unregister();
            registration.Dispose();
            _tokenSource.CancelAsync();
            //It seems unlikely that a thread has been spawned and changed the value between this thread executing the previous line and this line
            Assert.False(_tokenSource.Token.IsCancellationRequested);
        }
    }
}

@kirsan31
Copy link

kirsan31 commented Aug 25, 2021

It's needed feature. Especially when you use CancellationTokenSource.Cancel with Task.Delay(Int32, CancellationToken) (not using SyncContext). This can lead to surprising deadlocks...
Many folks are confusing with this, for example:
https://stackoverflow.com/questions/31495411/a-call-to-cancellationtokensource-cancel-never-returns
https://stackoverflow.com/questions/52450528/cancellationtokensource-cancel-hangs
For now, as some strange workaround we can call CancelAfter(0).
We defiantly need an fire-and-forget (non blocking) call to CancellationTokenSource.Cancel...

@joperezr joperezr moved this from Needs triage to Future in Triage POD for Meta, Reflection, etc Nov 24, 2021
@stephentoub stephentoub added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-needs-work API needs work before it is approved, it is NOT ready for implementation labels Jan 20, 2023
@stephentoub
Copy link
Member

This issue was opened first, but I'm going to close it as a dup in favor of the broader #31315. Thanks!

@stephentoub
Copy link
Member

Actually, I'm going to reopen this one for CancelAsync and remove CancelAsync from the other one.

@stephentoub stephentoub reopened this Jan 23, 2023
@stephentoub stephentoub modified the milestones: Future, 8.0.0 Jan 23, 2023
@stephentoub stephentoub self-assigned this Jan 23, 2023
@bartonjs
Copy link
Member

bartonjs commented Jan 31, 2023

Video

Looks good as proposed

namespace System.Threading
{
    public partial class CancellationTokenSource
    {
        public Task CancelAsync();
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jan 31, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 31, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 13, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Threading.Tasks
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

7 participants