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

Add a way to await a CancellationToken #14991

Closed
GSPP opened this issue Aug 10, 2015 · 39 comments
Closed

Add a way to await a CancellationToken #14991

GSPP opened this issue Aug 10, 2015 · 39 comments
Assignees
Milestone

Comments

@GSPP
Copy link

GSPP commented Aug 10, 2015

As of .NET 4.6 there does not seem to be a way to create a Task from a CancellationToken. This functionality is of course easy to build with the Register method but it should be included in the framework. The signature would be

Task WhenCancelled(this CancellationToken ct)

The task should transition to a successful completion state when the token is cancelled. It should not become cancelled (or faulted in case of a timed out token) because that makes this task hard to use. This is not an error condition.

@antiufo
Copy link
Contributor

antiufo commented Aug 14, 2015

A CancellationToken is not guaranteed to transition to canceled.
Awaiting a CancellationToken would make it very easy to create tasks that never complete, which is a bit counterintuitive.
This also means that some resources might never be disposed:

using(something)
{
    /* Potentially some intermediate asynchronous functions in the call tree */
        await token.WhenCancelled();
}

@svick
Copy link
Contributor

svick commented Aug 14, 2015

I think this makes most sense when you're combining it with other Tasks. For example, if you have a problem like:

I have a list of Tasks. I want to wait until all of them complete, or until some other part of the code says to stop waiting.

With WhenCancelled(), you could implement that easily as:

await Task.WhenAny(Task.WhenAll(tasks), cancellationToken.WhenCancelled());

Though it's possible that for this problem, another approach would make more sense. For example, making Task.WhenAny() take a CancellationToken parameter directly.

@GSPP Is this similar to what you imagined it would be used for? Or do you have other use cases?

@stephentoub
Copy link
Member

Just FYI since it's relevant to the conversation:
http://blogs.msdn.com/b/pfxteam/archive/2012/10/05/how-do-i-cancel-non-cancelable-async-operations.aspx

And if you really want such a WhenCanceled() method, it's possible write one, e.g.

public static Task WhenCanceled(this CancellationToken cancellationToken)
{
    var tcs = new TaskCompletionSource<bool>();
    cancellationToken.Register(s => ((TaskCompletionSource<bool>)s).SetResult(true), tcs);
    return tcs.Task;
}

@GSPP
Copy link
Author

GSPP commented Aug 14, 2015

@svick yes, that is one potential use. Another one is cleanup on cancellation.

@antiufo it is true that such a task might never complete. This is not a GC problem because once the CTS becomes unreachable the task any anything that hangs off it also becomes eligible for collection. The using example is applicable but how common is that? I think it is quite intuitive that a cancellation based task might never complete. You wouldn't acquire resources before awaiting it.

@stephentoub I have written exactly this method. It should be built-in I think. This request of mine is prompted by personal needs and also by Stack Overflow questions that I saw.

Right now CancellationTokens do not compose well. Everything is task based. This is a useful adapter method.

@stephentoub
Copy link
Member

The proposal is essentially to add either an instance method to CancellationToken:

public Task WaitAsync();

or an extension method somewhere:

public static Task WaitAsync(this CancellationToken cancellationToken);

(names would obviously be flexible... WaitAsync, WhenCanceled, etc.)

Personally, I don't think we should add this, for the reasons called out on this thread. In particular, making it easy for code to do:

await cancellationToken.WaitAsync();

with built-in support for this will make it very easy to accidentally leak resources due to awaiting a long-lived CancellationToken that never has cancellation requested. State associated with the continuation chain will be kept alive indefinitely, and resources cleaned up by finally's and the like will never be cleaned up until the resources are eventually collected and any finalizers are able to do the associated cleanup non-deterministically.

I do understand there are valid use cases for this and I do understand the desire to have it built-in, but it's one of those pieces of functionality that's easy to implement out-of-band from the core libraries and that could potentially lead people down a bad path if it were included in-band. I've seen multiple cases in the past (in Visual Studio, for example) where we've had to track down difficult-to-diagnose bugs due to awaiting things that never complete, and I'm loathe to make it easier to introduce such cases.

If the Task.WhenAny scenario was the driving one, then I'd prefer to see overloads of WhenAny and WhenAll added that take a CancellationToken (note that WhenAll couldn't be achieved just with a WaitAsync method on CancellationToken). IMHO, that'd be a reasonable addition.

@GSPP
Copy link
Author

GSPP commented Oct 5, 2015

Fair enough. From my own experience I would disagree with the assessment that "abandoned" resources around such a cancellationToken.WaitAsync task would be rare enough. But of course that depends on personal history and willingness to take API risk.

I have these scenarios in my codebase:

  1. Synchronously waiting for a token to be signaled.

  2. Using WhenAny and not wanting the call to throw. Exception handling for control flow is not a good thing.

  3. Elegantly waiting for many cancellation conditions (timeout+user-initiated+another one):

            var sleepTask = Task.Delay(sleepDuration);
            var breakSleepTask = breakSleepTaskCompletionSource.Task;
            var cancelTask = cancellationToken.WhenCancelled();
            Task.WaitAny(new[] { sleepTask, breakSleepTask, cancelTask });
    

Too often do I see people catching TaskCancelledException and the like on Stack Overflow. Makes for convoluted code, is slow and prone to catching too much.

@stephentoub
Copy link
Member

Thanks, @GSPP.

For (1), you can use CancellationToken.WaitHandle, though in part for some of the same reasons I've called out, I actually wish we hadn't added that in the first place (it also has a memory impact just needing to carry around a field for it). You can of course also use an approach like the one cited and simply complete a ManualResetEventSlim that you block on, or just wait on the Task returned from the example implementations.

For (2), WhenAny won't throw if one of the supplied tasks is canceled; it'll simply return the Task that completed, even if it completed due to cancellation.

For (3), yes, this is a valid scenario, one you can achieve using either the example three-line method I showed, or what is essentially the WhenCanceled method you want already built-in, just with a different name: Task.Delay(-1, cancellationToken).

For concerns around resource cleanup, I continue to believe we would be doing more harm than good in exposing such a method that encouraged these patterns. And given that there are multiple simple solutions for achieving the same thing, I'm going to close this out. Thanks for the discussion.

@GSPP
Copy link
Author

GSPP commented Dec 6, 2015

OK!

That Task.Delay(-1, cancellationToken) is nasty :)

@stephentoub
Copy link
Member

:)

@davidfowl
Copy link
Member

I wrote something like this once https://gist.github.com/davidfowl/6433516#file-streaming-cs-L52

@springy76
Copy link

Are CancellationTokens the new events?

@GSPP
Copy link
Author

GSPP commented Dec 2, 2016

@springy76 seems like a bad idea. These should be Task.

@springy76
Copy link

This is not only a bad idea but ASP.net core 1.0 and 1.1 reality.

Btw: Task.Delay(-1, cancellationToken) throws an TaskCancelled exception, too -- the initial post clearly asked not for such behavior:

"The task should transition to a successful completion state when the token is cancelled. It should not become cancelled (or faulted in case of a timed out token) because that makes this task hard to use. This is not an error condition."

That statement also perfectly fits for the usage pattern of these ugly IApplicationLifetime properties.

@dmitriyse
Copy link

dmitriyse commented Feb 10, 2017

Please reopen this issue.

  1. There are no any convenient way to simply turn CancellationToken in a non exception raising task.
  2. We needs some good Event replacement in TPL. In a light of ValueTask c# 7.0 - TaskCompletionSource is too heavy and requires some dummy type.

@JonHanna
Copy link
Contributor

There are no any convenient way to simply turn CancellationToken in a non exception raising task.

The above works fine without exceptions. E.g.:

CancellationTokenSource source = new CancellationTokenSource();
CancellationToken token = source.Token;
Task.Delay(-1, token).ContinueWith( _ => Console.WriteLine("Cancel happened"), TaskContinuationOptions.OnlyOnCanceled);
Console.WriteLine("Cancel has not yet happened");
source.Cancel();

Or if you really want to await it:

await Task.Delay(-1, token).ContinueWith( _ => {}, TaskContinuationOptions.OnlyOnCanceled);

It's a one-liner, so not dreadfully inconvenient.

We needs some good Event replacement in TPL.

This sounds like asking for a more convenient square peg for a round hole.

@jnm2
Copy link
Contributor

jnm2 commented Feb 10, 2017

@dmitriyse This is about as lean as you can get. Usage:

// ...
await cancellationToken;
// ...

Implementation:

using System;
using System.Runtime.CompilerServices;
using System.Threading;

public static class CancellationTokenExtensions
{
    public static CancellationTokenAwaiter GetAwaiter(this CancellationToken cancellationToken)
    {
        return new CancellationTokenAwaiter(cancellationToken);
    }

    public struct CancellationTokenAwaiter : INotifyCompletion
    {
        private readonly CancellationToken cancellationToken;

        public CancellationTokenAwaiter(CancellationToken cancellationToken)
        {
            this.cancellationToken = cancellationToken;
        }

        public bool IsCompleted => cancellationToken.IsCancellationRequested;

        public void OnCompleted(Action continuation) => cancellationToken.Register(continuation);

        public void GetResult() => cancellationToken.WaitHandle.WaitOne();
    }
}

@jnm2
Copy link
Contributor

jnm2 commented Feb 10, 2017

Perhaps GetResult could use a ManualResetEventSlim instead of allocating a wait handle. This could possibly be lighter, at the risk of allocating yet another wait handle:

public void GetResult()
{
    using (var evt = new ManualResetEventSlim())
        evt.Wait(cancellationToken);
}

This is the approach Tasks take, though they share the MRES per task once created.

@dmitriyse
Copy link

dmitriyse commented Feb 13, 2017

Probably this implementation will also work but will not allocate MRE internally.

using System;
using System.Runtime.CompilerServices;
using System.Threading;

public static class CancellationTokenExtensions
{
    public static CancellationTokenAwaiter GetAwaiter(this CancellationToken cancellationToken)
    {
        return new CancellationTokenAwaiter(cancellationToken);
    }

    public struct CancellationTokenAwaiter : INotifyCompletion
    {
        private readonly CancellationToken cancellationToken;

        public CancellationTokenAwaiter(CancellationToken cancellationToken)
        {
            this.cancellationToken = cancellationToken;
        }

        public bool IsCompleted => cancellationToken.IsCancellationRequested;

        public void OnCompleted(Action continuation) => cancellationToken.Register(continuation);

        public void GetResult() => {}; // No any wait required.
    }
}

@dmitriyse
Copy link

dmitriyse commented Feb 13, 2017

Also, although this code works

await Task.Delay(-1, token).ContinueWith( _ => {}, TaskContinuationOptions.OnlyOnCanceled);

it's not so usable as some direct method, for example:

await token.AsTask();
...
public static  Task AsTask(this CancellationToken token)
{
     return Task.Delay(-1, token).ContinueWith( _ => {}, TaskContinuationOptions.OnlyOnCanceled);
}

So this like method should be in BCL.
Otherwise thousands of developers will posts the same question. And discuss multiple approaches (http://stackoverflow.com/questions/18670111/task-from-cancellation-token). We need a one standard approach in BCL.

@springy76
Copy link

springy76 commented Feb 16, 2017

Whatever you invent, please consider what happens when the ThreadPool is starved due to heavy load.

Task.Delay(-1, token).Continue[..] has the worst behavior in my test:

enum Method
{
	Delay,
	CT_Delay,
	CT_Register,
}

private int pending;

async Task Main()
{
	LINQPad.Util.CreateSynchronizationContext(true, true);
	ThreadPool.SetMinThreads(10, 4); // TPL will add 1 Thread per Second when there are pending Tasks
	ThreadPool.SetMaxThreads(80, 8);

	var method = await LINQPad.Util.ReadLineAsync<Method>(
		"Specify test method",
		Method.Delay,
		Enum.GetValues(typeof(Method)).OfType<Method>());

	var active = true;
	ThreadStart ts = () =>
	{
		var sw = Stopwatch.StartNew();
		while (active)
		{
			int cpu, io;
			ThreadPool.GetAvailableThreads(out cpu, out io);
			Util.Metatext($"[{sw.Elapsed.TotalMilliseconds,7:n1}] CPU:{80 - cpu} IO:{8 - io} pending:{pending} @ T#{Environment.CurrentManagedThreadId}").Dump();
			Thread.Sleep(200);
		}
	};

	try
	{
		var t = new Thread(ts) { IsBackground = true };
		t.Start();
		await this.Work(method, false);
		await this.Work(method, true);
	}
	finally
	{
		active = false;
	}
}

async Task Work(Method method, bool continueOnCapturedContext)
{
	new { method, continueOnCapturedContext }.Dump("ROUND");
	// create Threadpool starvation (simulate very busy ASP.NET)
	var starvation = Enumerable.Range(1, 25)
		.Select(i => { Interlocked.Increment(ref pending); return Task.Run(() => { Thread.Sleep(2000); Interlocked.Decrement(ref pending); }); })
		.ToArray()
		//.Dump("simulate CPU work [NOT await Task.Delay()!")
		;

	Thread.Sleep(200); // give TPL some time to spin up

	CancellationTokenSource cts;
	var sw = Stopwatch.StartNew();
	switch (method)
	{
		case Method.Delay:
			await Task.Delay(100)
				.ConfigureAwait(continueOnCapturedContext);
			break;

		case Method.CT_Delay:
			cts = new CancellationTokenSource(100);
			await Task.Delay(-1, cts.Token)
				.ContinueWith(_ => { }, TaskContinuationOptions.OnlyOnCanceled)
				.ConfigureAwait(continueOnCapturedContext);
			break;

		case Method.CT_Register:
			cts = new CancellationTokenSource(100);
			try
			{
				await cts.Token.AsTask()
					.ConfigureAwait(continueOnCapturedContext);
			}
			catch (Exception ex)
			{
				Util.Highlight(ex.Message, "tomato").Dump();
			}
			break;
	}
	Console.WriteLine("awaited 100ms in {0:n1} ms", sw.Elapsed.TotalMilliseconds);

	Console.WriteLine("awaiting unlock");
	await Task.WhenAll(starvation);
	Console.WriteLine("done");
}

static class ExtensionMethods
{
	// http://stackoverflow.com/a/18672893
	public static Task AsTask(this CancellationToken cancellationToken)
	{
		var tcs = new TaskCompletionSource<object>();
		cancellationToken.Register(() => tcs.TrySetCanceled(),
			useSynchronizationContext: false);
		return tcs.Task;
	}
}

BTW: System.Threading.Timer seems to depend on the ThreadPool and therefore also starts acting messy, that's why I used a custom Thread using while...Sleep(200).

@tibitoth
Copy link

tibitoth commented Mar 2, 2018

Are CancellationTokens the new events?

@springy76 I'm just shocked, why are those tokens and not events? @davidfowl

@davidfowl
Copy link
Member

@totht91 you can read why events suck here https://github.com/dotnet/corefx/issues/16221

@crozone
Copy link

crozone commented May 14, 2018

I've also hit the need to do this, it would be nice if the framework had something built in, but at least when you implement it yourself you understand the ramifications of doing it.

Since this this issue is still the top hit on Google , I'll leave my extension implementation here:

public static async Task WhenCancelled(this CancellationToken cancellationToken)
{
    TaskCompletionSource<bool> taskCompletionSource = new TaskCompletionSource<bool>();

    using (cancellationToken.Register(() =>
    {
        taskCompletionSource.TrySetResult(true);
    }))
    {
        await taskCompletionSource.Task;
    }
}

It's similar to the one above, but it cleanly disposes of the registration. I think the other extension method above doesn't dispose the hook on the cancellation token, so the completion source and lambda won't be garbage collected until the token goes away.

I'm mainly using this pattern in an async main context:

CancellationTokenSource cts = new CancellationTokenSource();

Console.CancelKeyPress += (s, e) =>
{
    e.Cancel = true;
    cts.Cancel();
};

try
{
    // Initialize some module that maintains tasks inside of it with its own internal cancellation tokens
    //
    await someModule.InitializeAsync();

    // Wait until the user hits Ctrl-C to close the application
    //
    await cts.WhenCancelled();

    // Deinitialize the module and wait for it to shut down
    //
    await someModule.DeinitializeAsync();
}
catch (OperationCanceledException)
{

}

@mgwalm
Copy link

mgwalm commented May 14, 2018

I thought you could use a continuation to achieve this?

@hankbeasley
Copy link

Seems like all of these approaches have memory leaks if the same token is used in a low level method that needs to be awaited until canceled. cancellationToken.Register will be called continually and slowly leak memory.

@svick
Copy link
Contributor

svick commented Jun 28, 2018

@hankbeasley There is no leak if you can Dispose() the object returned by Register().

@mgwalm
Copy link

mgwalm commented Jul 7, 2018

public static async Task<Response[]> CallService(Request[] requests, ClientDescription description, CancellationTokenSource cts = null)
{
  try
  {
    var mainTask = RequestClient.CallHttpWebService(requests, description, cts);
    var cancellask = mainTask.ContinueWith(
       antecedent =>
       {
         Debug.WriteLine($"CallService cancellask main task cancelled");
       },
       TaskContinuationOptions.OnlyOnCanceled);

    var compTask = await Task.WhenAny(mainTask, cancellask);
    if (mainTask.Exception != null)
    {
      Debug.WriteLine($"CallService main task exception");
      throw mainTask.Exception.Flatten().InnerExceptions[0];
    }
    if (mainTask.Result != null)
    {
      return mainTask.Result;
    }
    if (compTask.Id == cancellask.Id)
    {
      throw new TaskCanceledException();
    }
  }
  catch (AggregateException ex)
  {
    Debug.WriteLine($"CallService AggregateException={ex.Message}");
    throw ex.Flatten().InnerExceptions[0];
  }
  return null;
}

@maximecaron
Copy link

maximecaron commented Sep 5, 2018

hankbeasley is correct, if the CancellationToken is a static global variable and we call WhenCancelled() 2000 time per seconds you can clearly see how it would leak.

Each call to WhenCancelled() create a registration in the CancellationToken this registration contain a reference to the task and keep it in memory.

If the CancellationToken get cancelled the await will return and the using block will call dipose() on the registration which will remove the registration from the list of registration enabling the task to be garbage collected.

But if the CancellationToken instance is never cancelled all those task and registration would need to stay in memory and can't be collected.

I think the real solution is for Task.WhenAny() to take a CancellationToken parameter directly!

@crozone
Copy link

crozone commented Sep 8, 2018

I think the real solution is for Task.WhenAny() to take a CancellationToken parameter directly!

Absolutely. It should then throw a TaskCanceledException.

@shadowbane1000
Copy link

Old thread... but a related question. What are the drawbacks of this line of implementation:

public static class TaskExtensions
{
  public static async Task CancellableAwait(this Task task, CancellationToken cancellationToken)
  {
    var tcs = new TaskCompletionSource<bool>();
    cancellationToken.Register(s => ((TaskCompletionSource<bool>) s).SetResult(true), tcs);
    await Task.WhenAny(task, tcs.Task);
    cancellationToken.ThrowIfCancellationRequested();
  }
}

Thereby allowing you to cancel an await. Then my AsyncManualResetEvent is just like in Stephen Toub's Blog post. And I don't have to worry about being able to cancel the await in there.

var myEvent = new AsyncManualResetEvent();`
myEvent.WaitAsync().CancellableAwait(cancellationToken);`

Thoughts?

@jnm2
Copy link
Contributor

jnm2 commented Mar 29, 2019

@shadowbane1000 Who handles the exception if the task fails later on?

@shadowbane1000
Copy link

shadowbane1000 commented Mar 29, 2019

That's a good question. In this case, in my application, if there is an exception later on, it's not too much of a concern, as the most I could do with it would be to log it and ignore it as the work behind the scenes will also be canceled. But in general, it's something I need to find a solution for. I frequently find myself doing things in a fire-and-forget manner because they are status updates that I don't want to wait until they propogate. So I am frequently doing this:
var dontwait = tellTheRemoteServerAsync();
and then never awaiting the task. So much so that I have an override

public static class TaskExtensions
{
  public static void DontWait(this Task _){}
}

So that I avoid the unused variable warning.
tellTheRemoteServerAsync().DontWait();

Perhaps that is a bad pattern and there is a better way to handle it.

@shadowbane1000
Copy link

Perhaps it would be better implemented within the AsymcManualResetEvent after all, so that people aren't tempted to make tasks that are more than a Wait also cancelable that way. Waits seem safer to be cancellable w.r.t. throws within them.

@jnm2
Copy link
Contributor

jnm2 commented Mar 29, 2019

@shadowbane1000 It also makes it really hard to know when your test is complete.

I've started using this instead of fire-and-forget:

AmbientTasks.Add(DoSomethingWithoutWaitingAsync(...));

In Program.Main:

// Handle immediately rather than waiting for GC to detect that a task was unobserved
AmbientTasks.BeginContext(ex => HandleTopLevelException(ex));

In tests:

AmbientTasks.BeginContext();

// Call something that ends up calling AmbientTasks.Add

await AmbientTasks.WaitAllAsync();

// Clean up

Impl: https://gist.github.com/jnm2/ab5624b10efd1ae6fbd6aa8f081a0ec9#file-ambienttasks-cs

If you use NUnit: https://gist.github.com/jnm2/0dfcf7dabfb8db76865c124d3a59b20f#file-waitforambienttasksattribute-cs

@shadowbane1000
Copy link

@jnm2
I like the capture of the tasks in to a pool that allows a separate thread to report exceptions on them. I'll look through the implementation you sent. Thanks for that.

@jnm2
Copy link
Contributor

jnm2 commented Mar 29, 2019

@shadowbane1000 I have a good set of tests for it too. Perhaps I should set up a GitHub project.

@shadowbane1000
Copy link

@jnm2 Moved comments over to the gist so as to avoid hijacking this thread any longer.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 1.0.0-rtm milestone Jan 31, 2020
@jnm2
Copy link
Contributor

jnm2 commented Feb 1, 2020

@shadowbane1000 ℹ I just published https://github.com/Techsola/AmbientTasks.

@jnm2
Copy link
Contributor

jnm2 commented Sep 29, 2020

@dmitriyse I'm not sure what I was thinking, but you're definitely right that GetResult should be empty.

@dotnet dotnet locked as resolved and limited conversation to collaborators Jan 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests