Fsharp: 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.