Async.StartAsTask behavior in cancellation scenarios.

[matthid]

Today I stumbled on some unexpected behavior when cancelling a asynchronous workflow which interacts with Tasks.
When StartAsTask is used it is not giving any chance to the underlying workflow to cancel in a regular way and just continues with a OperationCanceledException, basically throwing away all information from the workflow.

DISCLAIMER: Maybe this works as designed, but we should put a warning somewhere in that case :/

Repro steps

OK, to explain this see how different information can flow through the async-workflow:

    let tcs = new TaskCompletionSource<_>()
    let cts = new CancellationTokenSource()
    use reg = cts.Token.Register(fun () -> tcs.SetException(Exception "Something bad happened"))
    let a =
        async {
            cts.CancelAfter 500
            do! tcs.Task |> Async.AwaitTask
        } |> fun a -> Async.RunSynchronously(a, cancellationToken = cts.Token)
    ()

In this case you get (as imho expected) an AggregateException with the "Something bad happened" message.
Now use StartAsTask which imho is just another way to start the same workflow:

    let tcs = new TaskCompletionSource<_>()
    let cts = new CancellationTokenSource()
    use reg = cts.Token.Register(fun () -> tcs.SetException(Exception "Something bad happened"))
    let a =
        async {
            cts.CancelAfter 500
            do! tcs.Task |> Async.AwaitTask
        } |> fun a -> Async.StartAsTask(a, cancellationToken = cts.Token)
    a.Result
    ()

Expected behavior

Accessing .Result throwing an AggregateException (possibly wrapping another AggregateException) wrapping the "Something bad happened" exception.

Actual behavior

Message: System.AggregateException : One or more errors occurred.
  ----> System.Threading.Tasks.TaskCanceledException : A task was canceled.

Known workarounds

Do not use StartAsTask. I copied the library implementation and added a timeout parameter (to give the underlying task some time to use the token and finish regulary).
Maybe the correct thing to do is to assume the workflow is finishing correctly when the token is forwarded?

Related information

Related discussions:

• Async.AwaitTask behaviour on task canceled
• Async.AwaitTask does not cancel on workflow cancellation
  This one in particular is interesting because I'm suggesting the complete reverse (on a different API).
  I'm suggesting we should "ignore" cancellation when interacting with tasks and probably add some other APIs to add timeouts or early cancellation on top. IMHO it's a better default than loosing information and having to write own implementations.

/cc @eiriktsarpalis

Note that you might argue that the "different information flow" only works because it was the last thing the workflow was waiting for, but this works as expected as well:

    let tcs = new TaskCompletionSource<_>()
    let cts = new CancellationTokenSource()
    use reg = cts.Token.Register(fun () -> tcs.SetException(Exception "Something bad happened"))
    let a =
        async {
            cts.CancelAfter 500
            do! tcs.Task |> Async.AwaitTask
            printfn "test"
            return! async {
                do! Async.Sleep 100
                return 4 }
        } |> fun a -> Async.RunSynchronously(a, cancellationToken = cts.Token)
    ()

(IE. the "Something bad happened" is returned)

• Operating system: Windows
• Branch: FSharp.Core Nuget package (4.1.17)
• .NET Runtime 4.5
• Visual Studio 2017
• Indications of severity: Not sure if this is a bug

[matthid]

Would a PR be accepted or at least help with the discussion?

[dsyme]

@matthid Yes, that would be great

[eiriktsarpalis]

Note that you might argue that the "different information flow" only works because it was the last thing the workflow was waiting for, but this works as expected as well

This does the trick:

let test() =
    let tcs = new TaskCompletionSource<unit>()
    let cts = new CancellationTokenSource()
    let _ = cts.Token.Register(fun () -> tcs.SetException(Exception "Something bad happened"))
    let a = 
        async {
            cts.CancelAfter 500
            try do! tcs.Task |> Async.AwaitTask
            with :? System.IO.FileNotFoundException -> ()
        } |> fun a -> Async.RunSynchronously(a, cancellationToken = cts.Token)
    a

Honestly, I think that the problem here lies with Async.RunSynchronously, which is inconsistently surfacing exceptions on workflow cancellation. As soon as the exception continuation leaves the tail position, Async.RunSynchronously also surfaces an OperationCanceledException.

[matthid]

@eiriktsarpalis That answer makes me a bit sad to be honest.

In principle I agree with you: Asyncs should be cancelled immediately.
But when interacting with the real work (ie. with tasks) it doesn't make any sense to just "pretend" that stuff has been cancelled when in fact it hasn't. It's not only wrong, it can set wrong expectations and introduce subtle bugs.

When you get the cancellation-information you should be sure nothing is running anymore. This is not true at the moment as far as I can see when using StartAsTask (when combined with AwaitTask, which obviously happens all the time).

About your last counter example I'd actually argue the reverse. There is no reason for the cancellation to throw the information about the exception "away". Why should it do that?

But I think I get your point: What if we start handling the exception and doing other async work in the "catch"? I think this is quite a special case. My solution would be something like: Wrapping the original exception within a new TaskCancelledException (as inner exception) and ignore the exception handlers. This is probably the most natural way of dealing with that situation.

Also when doing cancellation I personally find the current way very frustrating and limited. There is no way for custom information/logic to flow back to the place which is waiting for the result. To be honest if we even remove the existing way I'm using with exceptions, then I'd probably fall-back to just providing cancellation-tokens as parameters just like in C#. I'd probably never use the async build-in one because it's useless/too limited.

[matthid]

@eiriktsarpalis Maybe I should ask the other way around: What would you suggest for my scenario? I use asyncs though some level of a call-stack and have cancellation in place. Now I want to mark some "await" places in such a way that I can print where it was waiting at some topmost waiting place.

Example:

let Inner1Task1() = Task.Delay(500)
let Inner1Task2() = Task.Delay(500)
let Inner2Task1() = Task.Delay(500)
let Inner2Task2() = Task.Delay(500)

let inner1() =
    async {
        // stuff before
        do! Inner1Task1() |> Async.AwaitTask
        // stuff
        do! Inner1Task2() |> Async.AwaitTask
        // stuff after
    }

let inner2() =
    async {
        // stuff before
        do! Inner2Task1() |> Async.AwaitTask
        // stuff
        do! Inner2Task2() |> Async.AwaitTask
        // stuff after
    }

let outer() =
    async {
        // stuff before
        do! inner1()
        // stuff
        do! inner2()
        // stuff after
    }

let blocking() =
    let tcs = new TaskCompletionSource<unit>()
    let cts = new CancellationTokenSource()
    let _ = cts.Token.Register(fun () -> tcs.SetException(Exception "Something bad happened"))
    cts.CancelAfter 500
    try
        // this is expected to finish after 500ms
        outer() |> fun a -> Async.RunSynchronously(a, cancellationToken = cts.Token)
    with e ->
        // information which task was the "faulty" task was should be here
        printfn "Error: %O" e

Note: The only thing I want is useful/custom information in the exception object.

What I did was to add some short-circuit logic:

let awaitTaskWithToken (fallBack:unit -> 'T) (item: Task<'T>) : Async<'T> =
    async {
        let! ct = Async.CancellationToken
        return! Async.FromContinuations(fun (success, error, cancel) ->
            async {
                let l = obj()
                let mutable finished = false
                let whenFinished f =
                    let isJustFinished =
                        if finished then false
                        else
                            lock l (fun () ->
                                if finished then
                                    false
                                else
                                    finished <- true
                                    true
                            )
                    if isJustFinished then
                        f()
                use! reg = Async.OnCancel(fun () ->
                    whenFinished (fun () ->
                        try let result = fallBack()
                            success result
                        with e -> error e))
                item.ContinueWith (fun (t:Task<'T>) ->
                    whenFinished (fun () ->
                        if t.IsCanceled then
                            cancel (OperationCanceledException("The underlying task has been cancelled"))
                        elif t.IsFaulted then
                            if t.Exception.InnerExceptions.Count = 1 then
                                error t.Exception.InnerExceptions.[0]
                            else
                                error t.Exception
                        else success t.Result))
                    |> ignore
            } |> fun a -> Async.Start(a, ct)
        )
    }

With this I can mark those places via:

let inner1() =
    async {
        // stuff before
        do! Inner1Task1() |> awaitTaskWithToken (fun () -> failwith "inner1task1")
        // stuff
        do! Inner1Task2() |> awaitTaskWithToken (fun () -> failwith "inner1task2")
        // stuff after
    }

I know that this basically means the same thing: I accept that those tasks are still running at the background. But the difference is that I explicitly decided to do it this way, and I can decide to apply that only for some part of it.

(You can definitely argue about the API - regarding the callback only being useful when throwing an exception - , but it's about the general idea)

Maybe I'm doing it all wrong :/

[eiriktsarpalis]

But when interacting with the real work (ie. with tasks) it doesn't make any sense to just "pretend" that stuff has been cancelled when in fact it hasn't. It's not only wrong, it can set wrong expectations and introduce subtle bugs.

This really begs the question: why are you passing a cancellation token to the parent workflow in the first place?

[eiriktsarpalis]

About your last counter example I'd actually argue the reverse. There is no reason for the cancellation to throw the information about the exception "away". Why should it do that?

It's pretty much the point of the cancellation continuation: signalling cooperative cancellation without caring about the result of the computation. It should be used with care.

[matthid]

@eiriktsarpalis Sorry for being so resistant and thanks for taking the time to answer :)

This really begs the question: why are you passing a cancellation token to the parent workflow in the first place?

So this basically means to never use it? Tbh. I already argued above that it's probably better to pass my own tokens. I thought I could use the build-in feature for this and get around passing my own tokens everywhere....

It's pretty much the point of the cancellation continuation: signalling cooperative cancellation without caring about the result of the computation. It should be used with care.

I do not care about the result. I do care about errors in the cancellation process. And I do care about the stuff being actually canceled when the cancellation signal arrives.

let outer() =
    async {
        do! Task.Run (fun () ->
                // Some real-life task
                for i in 1..10 do
                    printfn "Task %i" i
                    Thread.Sleep 500
            )
            |> Async.AwaitTask
    }

let blocking() =
    let cts = new CancellationTokenSource()
    cts.CancelAfter 500
    try
        // this is expected to finish after 500ms
        outer() |> fun a -> Async.RunSynchronously(a, cancellationToken = cts.Token)
    with e ->
        // information which task was the "faulty" task was should be here
        printfn "%O" e

this prints

> blocking();;
Task 1
Task 2
Task 3
Task 4
Task 5
Task 6
Task 7
Task 8
Task 9
Task 10
System.OperationCanceledException: The operation was canceled.
   at Microsoft.FSharp.Control.AsyncBuilderImpl.commit[a](AsyncImplResult`1 res)
   at Microsoft.FSharp.Control.CancellationTokenOps.RunSynchronouslyInAnotherThread[a](CancellationToken token, FSharpAsync`1 computation, FSharpOption`1 timeout)
   at Microsoft.FSharp.Control.CancellationTokenOps.RunSynchronously[a](CancellationToken token, FSharpAsync`1 computation, FSharpOption`1 timeout)
   at Microsoft.FSharp.Control.FSharpAsync.RunSynchronously[T](FSharpAsync`1 computation, FSharpOption`1 timeout, FSharpOption`1 cancellationToken)
   at FSI_0010.blocking()
val it : unit = ()

While

let outer() =
    async {
        do! Task.Run (fun () ->
                // Some real-life task
                for i in 1..10 do
                    printfn "Task %i" i
                    Thread.Sleep 500
            )
            |> Async.AwaitTask
    }

let blocking() =
    let cts = new CancellationTokenSource()
    cts.CancelAfter 500
    try
        // this is expected to finish after 500ms
        let t = outer() |> fun a -> Async.StartAsTask(a, cancellationToken = cts.Token)
        t.Result
    with e ->
        // information which task was the "faulty" task was should be here
        printfn "%O" e

prints

> blocking();;
Task 1
Task 2
System.AggregateException: One or more errors occurred. ---> System.Threading.Tasks.TaskCanceledException: A task was canceled.
   --- End of inner exception stack trace ---
   at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
   at System.Threading.Tasks.Task`1.GetResultCore(Boolean waitCompletionNotification)
   at System.Threading.Tasks.Task`1.get_Result()
   at FSI_0008.blocking()
---> (Inner Exception #0) System.Threading.Tasks.TaskCanceledException: A task was canceled.<---

val it : unit = ()

> Task 3
Task 4
Task 5
Task 6
Task 7
Task 8
Task 9
Task 10
- ;;

Don't you agree that the StartAsTask version is plain wrong?
The second one pretends the thing is cancelled but you can clearly see that it isn't.

[eiriktsarpalis]

I do not care about the result. I do care about errors in the cancellation process. And I do care about the stuff being actually canceled when the cancellation signal arrives.

For all intents and purposes I consider exceptions to also be results of a computation. Exceptions can be caught and used for control flow. Cancellation cannot be caught and does not yield a result; it only allows for finally blocks to be executed.

Don't you agree that the StartAsTask version is plain wrong?

I don't. I content that the behaviour isn't even related to StartAsTask, but it's part of the nature of tasks, that is different to Async in many significant ways. Async inherits its cancellation context from its parent computation, whereas Tasks are standalone futures whose cancellation needs to be configured explicitly. Most C# async APIs address this by exposing an appropriate method, for example this or this. The proper way to consume it in async is as follows:

async {
    let! ct = Async.CancellationToken
    let! response = httpClient.SendAsync(request, ct) |> Async.AwaitTask
    ...
}

Your example does not cancel properly because you just called Task.Run, which has no provision for cancellation. To achieve proper cancellation in your case you would have to resort to something like TaskCompletionSource.

[matthid]

@eiriktsarpalis Yes I agree that tasks work differently and work by passing the token explicitly.
But that's not the point I'm making. Even if you use httpClient.SendAsync(request, ct) or a similar function we currently don't "wait" for the cancellation to properly finish. We just immediately abort the Task returned by StartAsTask. It's not a cooperative cancellation whatsover. It is really just hardcoded as "all cleanup doesn't take any time"

It isn't even related to using AwaitTask. Now with your explanation on finally, here is an example without AwaitTask:

let outer() =
    async {
        try
            printfn "starting task"
        finally
            Thread.Sleep 2000
            printfn "cleanup"
    }

let blocking() =
    let cts = new CancellationTokenSource()
    cts.CancelAfter 500
    try
        // this is expected to finish after 500ms
        let t = outer() |> fun a -> Async.StartAsTask(a, cancellationToken = cts.Token)
        t.Result
    with e ->
        // information which task was the "faulty" task was should be here
        printfn "%O" e

it prints

- blocking();;
starting task
System.AggregateException: One or more errors occurred. ---> System.Threading.Tasks.TaskCanceledException: A task was canceled.
   --- End of inner exception stack trace ---
   at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
   at System.Threading.Tasks.Task`1.GetResultCore(Boolean waitCompletionNotification)
   at System.Threading.Tasks.Task`1.get_Result()
   at FSI_0014.blocking()
---> (Inner Exception #0) System.Threading.Tasks.TaskCanceledException: A task was canceled.<---

val outer : unit -> Async<unit>
val blocking : unit -> unit
val it : unit = ()

> cleanup

Look how it prints "cleanup" AFTER everything...

Sorry I think it is just broken...

[matthid]

And fundamentally I still think we should forward the exception information as inner exceptions there is just no reason not to do that

[matthid]

This might even bring our two opinions together because then I'd agree that we can change the RunSynchronously behavior (but again we need to preserve the original information as "inner" Exception, otherwise it is lost)

[eiriktsarpalis]

And fundamentally I still think we should forward the exception information as inner exceptions there is just no reason not to do that

Cancellation continuations don't pass any information, for good reason imo. I don't see how you could achieve this without rewriting async internals and risk breaking backward compat in the process. The payoff seems extremely tiny to justify this, and goes against -at least what I consider- good practice when using async cancellation.

You are right to point out that StartAsTask cancels immediately on token cancellation. This is the offending code. Arguably the promise should have been cancelled by the child's root cancellation continuation, not the the cancellation token itself. I am not particularly bothered by this. In fact, I have argued in this issue in favor of extending this behaviour to Async.AwaitTask.

Also, I should point out that this issue is completely independent of whether cancellation should surface exceptions.

[matthid]

@eiriktsarpalis Ok one last try to clarify..

The payoff seems extremely tiny to justify this, and goes against -at least what I consider- good practice when using async cancellation.

Are those written anywhere to compare with?

Also, I should point out that this issue is completely independent of whether cancellation should surface exceptions.

IMHO this issue is all about which information the cancellation should give to the waiter. You say it should be none, but than there is absolutely no point in the cancellation-process (1). For me this information should be given:

• It should return (by default) only when everything was successfully cancelled (Because this is the actually hard part). So nothing should be running in the background at this point. Cancellation should not be a way to introduce parallelism.
• It should return if there where errors in the process (Because otherwise this information is lost, bugs are never found)
• You should consider/expect that cancellation is not implemented properly. Especially considering 3-rd party API. They should still be usable in a reasonable way. This means that the cleanup-code might throw - with important data in the exception - or take some time to finish the cancellation.

Here are the reasons why I don't like your point of view:

• It is harmful by throwing information away (2)
• It is harmful by setting expectations about stuff has finished when it has not

You are right, it feels like the problems have actually increased within the discussion, because I wasn't aware that the current situation is actually that bad (I only noticed the part with throwing away the exception and the parallelism in StartAsTask. I didn't notice that regular APIs like TryWith are also buggy - IMHO of course)

(1)

To clarify: There is no point, because this behavior is so easy to implement yourself. It's so easy to throw those 3 points I think Cancellation should do away, but quite hard to ensure them.

(2)

let outer() =
    async {
        try 
            try
                printfn "starting task"
            finally
                Thread.Sleep 2000
                printfn "cleanup"
                // Bug only happending when cancelling
                raise <| Exception "Cancellation Bug"
        with :? System.IO.FileNotFoundException -> ()
    }

let blocking() =
    let cts = new CancellationTokenSource()
    cts.CancelAfter 500
    try
        // this is expected to finish after 500ms
        outer() |> fun a -> Async.RunSynchronously(a, cancellationToken = cts.Token)
    with e ->
        // information which task was the "faulty" task was should be here
        printfn "%O" e

prints

> blocking();;
starting task
cleanup
System.OperationCanceledException: The operation was canceled.
   at Microsoft.FSharp.Control.AsyncBuilderImpl.commit[a](AsyncImplResult`1 res)
   at Microsoft.FSharp.Control.CancellationTokenOps.RunSynchronouslyInAnotherThread[a](CancellationToken token, FSharpAsync`1 computation, FSharpOption`1 timeout)
   at Microsoft.FSharp.Control.CancellationTokenOps.RunSynchronously[a](CancellationToken token, FSharpAsync`1 computation, FSharpOption`1 timeout)
   at Microsoft.FSharp.Control.FSharpAsync.RunSynchronously[T](FSharpAsync`1 computation, FSharpOption`1 timeout, FSharpOption`1 cancellationToken)
   at FSI_0021.blocking()
val it : unit = ()

where is my bug? It's just hidden somewhere. I consider this wrong on so many levels I don't even know why It's so hard to convince you.

To be completely honest: I think I'll just took at how hopac has done it. This discussion feels pointless, I may have completely wrong expectations about the cancellation process.