Async.AwaitTask does not cancel on workflow cancellation

[eiriktsarpalis]

Not sure whether a bug or by design, but I have discovered a hanging behaviour that seems to be related to the cancellation of sibling computations in the event of an exception

Repro steps

open System
open System.Threading.Tasks

let test() = 
    async {
        let worker i = async {
            if i = 9 then
                do failwith "error"
            else
                do! Task.Delay Int32.MaxValue |> Async.AwaitTask
        }

        return! Seq.init 10 worker |> Async.Parallel
    } |> Async.RunSynchronously

test()

Expected behavior

Should fail immediately with an exception

Actual behavior

Hangs indefinitely

Known workarounds

Have the awaited task cancel cooperatively by passing the cancellation token.

Related information

I've been able to reproduce this in builds of F# 4.1 in Desktop CLR and mono.

[majocha]

Looks like by design.

Async.Parallel<'T>

  If any child computation raises an exception, then the overall computation will trigger an exception, and cancel the others. The overall computation will respond to cancellation while executing the child computations. If cancelled, the computation will cancel any remaining child computations but will still wait for the other child computations to complete.

[eiriktsarpalis]

@majocha OK. In that case it seems to me that Async.AwaitTask should be taking the ambient cancellation token into account. It's interesting to note that the behaviour does not reproduce if we replace Task.Delay >> Async.AwaitTask with Async.Sleep

[majocha]

It's not hanging. If you wait for 24 days it will throw the exception :)

[dsyme]

So this seems by design. Can we close this?

[eiriktsarpalis]

@dsyme Perhaps, the fact that replacing Task.Delay with Async.Sleep produces a different outcome does seem to indicate that Async.AwaitTask should perhaps take cancellation into account.

[eiriktsarpalis]

The point being Async.Sleep takes special care to ensure early cancellation, wondering whether we should update Async.AwaitTask to do the same thing.

[majocha]

@eiriktsarpalis I guess it's not possible. Tasks have no built-in cancellation, unlike async workflows. Task cancellation is cooperative only.

[eiriktsarpalis]

@majocha as you said, an async workflow awaiting on a task can be cancelled using the built-in mehanism.

[eiriktsarpalis]

I've updated the issue topic to better reflect the actual issue

[dsyme]

@eiriktsarpalis Isn't this by design? You have to grab the ambient cancellation token from the async context and pass it into the task creation - there's just no other way.

Giving some warning when the ambient cancellation token could have been passed to some overload could in theory be feasible, though quirky to implement

[eiriktsarpalis]

@dsyme To clarify, I'm not saying we should try to cancel the awaited task. You are right to say this is not achievable in general and I may add wrong. I am however suggesting we should make the task awaiter honor the ambient cancellation token and fire the cancellation continuation regardless of task outcome.

Here is a proof of concept: http://fssnip.net/7Rw

[vladima]

I'm not sure that this is right thing to do since AwaitTask is a general purpose thing and awaiting Task.Delay is just one scenario. For many other we'll end up in a situation when we've already proceeded with the cancellation branch but initial task is still running with unpredictable consequences.

[vladima]

As an alternative we can do the following:

• in TaskHelpers.continueWIth* pass ambient cancellation token to ContinueWith
• by default instead of TaskContinuationOptions.None use TaskContinuationOptions.LazyCancellation to preserve existing behavior
• add optional parameter to AwaitTask that will allow user to override lazy cancellation behavior and use eager cancellation.

Downside: TaskContinuationOptions.LazyCancellation appear in .NET 4.5 and is not available in earlier versions.

[eiriktsarpalis]

@vladima parameterizing the behaviour is an interesting proposition. Pretty sure it can be achieved without introducing dependency on net45. I would argue that eager cancellation makes for a better default but you make a good point and preserving backward compatibility is always a winner.

[Frassle]

We hit this issue at work this week. The fact that Async.AwaitTask doesn't cancel the task it creates when the Async is cancelled was causing huge memory leaks in our system. I opened #7357 to fix this, which we've had to also rewrite in terms of Async.FromContinuations to use in our system for now.

I don't think this behavior should be parameterized, it just shouldn't be possible to leak continuations.
If you want an async to not complete until the task is complete just don't cancel the async. That's the only code paths that could depend on this. Calling AwaitTask, canceling it, then waiting for it it finish and assuming that means the task is finished, in which case just don't cancel it to get the current behavior.

[Frassle]

Discussion about this came up again at work this week. We discussed how there's actually separate cases where you need to transform a Task into an Async.

Case 1) Your treating a Task as an external event to the workflow. This is the situation we had at work in 2019 where we had a Async.Choice that was listening to two Tasks to see which completed first, but we didn't want to cancel the other task when the first finished. We wrote our own version of AwaitTask that respected async cancellation for this purpose.

Case 2) Your creating a Task as part of the async workflow (like calling into ReadAsync or something). In this case your trying to make the Task fit into the async workflow and the right way to do this is to pass the workflows cancellation token to the thing making the task, and then in AwaitTask wait for the task itself to get cancelled (in the same way how if your awaiting another async you expect it to respect the cancellation and return that up).

A work college suggested that case 2 might be better done with the method signature (CancellationToken -> Task<'T>) -> Async<'T> to make it clear that you should make use of the workflows cancellation token in the task.

Given so much probably already depends on the current semantics of AwaitTask it might be better to add an overload for Case 1 instead. AwaitTaskCancellable(task : Task<'T>) or maybe AwaitTask(task : Task<'T>, respectCancellation : bool)

[bartelink]

This issue seems unfocused in that it's covering
a) honoring of external cancellation via the ambient CT
b) honoring of cancellation in the context of Async.Sleep vs not doing so in Task.Delay

Ultimately, there's no obvious behavior change in the impl of FSharp.Core that could resolve the forces outlined.

I suggest closing this one on the basis that the concerns it covers are better addressed via the combination of:

• Add Async.AwaitTask overloads which helps with CancellationToken passing, and a new warning fsharp/fslang-suggestions#1284, which deals with propagation of CTs when commencing a Task
• Async.Await overload (esp. AwaitTask without throwing AggregateException) fsharp/fslang-suggestions#840, which deals with the honoring of cancellation while awaiting a Task (as required for Async.Parallel to function correctly)