Cancel task continuations when async contexts are cancelled

[Frassle]

Currently if you transform a task into an async via Async.AwaitTask and
then run that async the continuation added to the underlying task is
only ever resolved if the task itself resolved (either via completion,
exception, or cancellation). Notably if the async created by
Async.AwaitTask is cancelled the task continuation isn't.

We're seeing this manifest as a large memory leak when we repeatedly run
Async.Choice with a task that only resolves if an error elsewhere in the
system is hit. Each time we run Async.Choice it appends another task
continuation, the async is then cancelled (so if the task did resolve
the async continuation would immediately see it was cancelled anyway)
but critically the task continuations are not cancelled and so stay
allocated.

This commit changes the task continuation to now cancel if the asyncs
cancellation token requests cancellation. Note that we need to have a
second continuation to watch for the case that the first continuation is
cancelled and thus never run, the second continuation then triggers the
async cancellation or exception continuation as appropriate.

This also means that some asyncs will now report cancelled sooner (see
the changes to the StartAsTaskCancellation test) while before they would
of been blocked waiting for a task to complete before any of their
continuations could run.

[Frassle]

Thinking about this over the last few days, I'm thinking that if the Async is cancelled (rather than the task) we should just call the cancellation continuation. This isn't the same as #1166 as this would only be when the asyncs cancellation token was triggered, not when just the task was cancelled.

[matthid]

As I understand it this basically reverts #3256, correct?

Can you add some example code on where this happens? So your scenario is basically having some long running (never ending) Task and repeatedly calling AwaitTask on it?

[matthid]

Can't you just use something similar to this in your application to resolve the issue (basically calling ContinueWith before AwaitTask):

let endlessTask = TaskCompletionSource<unit>().Task
async {
     let! tok = Async.CancellationToken
     do! endlessTask.ContinueWith(Action<Task<'T>>(fun t -> ()), tok) |> AwaitTask
}
|> Async.RunSynchronously

[Frassle]

As I understand it this basically reverts #3256, correct?

No this shouldn't be changing the semantics of StartAsTask at all. This is only to change the semantics of the Async created by AwaitTask.

So your scenario is basically having some long running (never ending) Task and repeatedly calling AwaitTask on it?

Correct, we have a task that is triggered on error and we repeatedly call that with AwaitTask and Async.Choice (which cancels the asyncs you pass to it as soon as one finishes). Because cancelling the asnyc returned by AwaitTask doesn't cancel until the task is finished this results in huge numbers of task continuations being built up.

Can't you just use something similar to this in your application to resolve the issue (basically calling ContinueWith before AwaitTask):

Kinda, that won't propagate the result or exceptions from the underlying task though. But yes the fix for this is to have two ContinueWiths one to handle the task completing and one to handle the async cancelling.

[Frassle]

Can you add some example code on where this happens?

I'm on holiday for 5 days, but will see about adding some tests for this case when I'm back.

[matthid]

As I understand it this basically reverts #3256, correct?

No this shouldn't be changing the semantics of StartAsTask at all. This is only to change the semantics of the Async created by AwaitTask.

Yes, the function is different but as far as I understand it, the async is now "finished" sooner than the task we await. This is exactly the behavior the linked PR removed (there are several places in the implementation where this is relevant).
In fact afaics the test you changed was added in the linked PR.

Because cancelling the asnyc returned by AwaitTask doesn't cancel until the task is finished this results in huge numbers of task continuations being built up.

Question is if this is kind of "expected" and we just need to document it (in addition to the workaround).

Kinda, that won't propagate the result or exceptions from the underlying task though.

Yes the real fix needs to be slightly different (just calling t.Result in the continuation for example), but it is quite straightforward.

I'm just afraid that we should not change the AwaitTask behaviour in the way you suggest it here. The reasoning is that this potentially introduces unexpected parallelism especially when you created the task on the spot (which is the "usual" API design). This means if you want to be sure everything has been cancelled you as a user need to collect all tasks and await them after the async cancellation yourself.

One could argue that we need a ChooseSafe : computations:seq<Async<'T option>> -> Async<'T option * Async<unit>>) which will cancel other childs as soon as one has finished and the returned Async<unit> represents the time when cancellation has finished. In your scenario you would have noticed that the (new return value) Async<unit> never actually finishes. (Or instead of returning an Async<unit> just 'wait' for all cancellations directly, just as it does when exceptions happen)

mentioning @eiriktsarpalis as he warned about this

[Frassle]

Haven't had time to reply properly to this. Still need to sit down and write up some more test, but wanted to get some response out.

Yes, the function is different but as far as I understand it, the async is now "finished" sooner than the task we await. This is exactly the behavior the linked PR removed (there are several places in the implementation where this is relevant).
In fact afaics the test you changed was added in the linked PR.

As far as I can tell the linked PR was just changing the behavior of StartAsTask, the fact that AwaitTask didn't cancel wasn't changed by that PR. Yes the test has changed, but the test was added to check that the Task from StartAsTask didn't cancel until the async was canceled (rather than triggering the tasks cancel as soon as the token was triggered). I see now that the new test isn't testing the same thing, it needs something to block the cancellation but I don't think that means AwaitTask needs to be blocking.

Question is if this is kind of "expected" and we just need to document it (in addition to the workaround).

We certainly didn't expect it. It seems strange to have an Async that rather silently and implicitly doesn't respect cancellation. AwaitEvent has a similar behavior but at least there it's a clear opt in where you have to pass a function to be triggered when cancel happens as a compensation for async cancellation not happening.

I'm just afraid that we should not change the AwaitTask behaviour in the way you suggest it here. The reasoning is that this potentially introduces unexpected parallelism ...

Choice also introduces unexpected parallelism. In fact if choice blocked waiting for all cancellations we would of picked up this leak much sooner.

This means if you want to be sure everything has been cancelled you as a user need to collect all tasks and await them after the async cancellation yourself.

Sure I see how this could be looked at either way. I first came at this thinking the Async is not the Task (and thus can be cancelled independently), but I understand how a lot of people could write it assuming the Async is the Task. I guess it's semantics of have we mapped the Task in Async land, or do we just have an async that happens to be waiting for a task.
It was mentioned on #2127 that it should be configurable, and maybe that's the answer although from a composition point of view I think maybe we're missing something like Async.BlockCancellation.

I say that because you've got things like AwaitEvent which also has to solve this cancellation problem (which it does by the passed in cancel function). I don't think it's Event or Task specific though. If all these Await Asyncs just respected cancellation you could compose them with a BlockCancellation async to get either behavior.

BlockCancellation is a pretty lame name but signature wise I was thinking something like (unit -> bool) -> 'a Async -> 'a Async such that you'd just run the inner async but with a fresh cancellation token and when the outer token was triggered you'd call the unit -> bool function to give notice that cancellation is requested and a chance to either stop or continue it. If cancellation continues it would be just like normal, the inner async would cancel and once done our cancellation continuation would fire, else if cancellation was blocked the outer asnyc wouldn't resolve till the inner async finished.

Or a variation on simple bools, (CancellationTokenSource -> unit) -> 'a Async -> 'a Async where we pass the token source, allowing the cancellation to either be triggered now, or maybe later (a simple thing could be just giving this bit of the system some extra time to cancel by having your function just call CancelAfter(timeout).

This compose with things like AwaitTask, where if you want the current blocking till the task is done behaviour you can do task |> Async.AwaitTask |> Async.BlockCancellation ignore. It also compose with all other Asyncs, could even deprecate Async.AwaitEvent that takes a cancel function.

mentioning @eiriktsarpalis as he warned about this

An accurate warning, I'd support Choice awaiting all cancellations just like Parallel.

edit

A better name is probably CatchCancellation

[KevinRansom]

@Frassle , there has been no movement on this for almost a year, I am going to close it. Please re-open this PR when you are ready to proceed with it.

Thank you for this contribution

Kevin

[Frassle]

Sure sounds like this isn't mergable until a decision on #2127 is made.

[KevinRansom]

@Frassle , okay, when you are ready to reactivate this, we will consider it at that time.

Once more thanks for the work you put in on this

Kevin