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.

[matthid]

I think we should talk about compatibility later (if we can agree on something), but to be honest I don't see a lot of problems there: I don't think there is any existing code depending on "InnerException" being null on the OperationCancelledException. I feel the change you suggest on RunSynchronously could be more harmful if people depend on a different exception at the moment (but I'd accept that as bugfix, if my other points are honored).

[eiriktsarpalis]

I think I need ask again, why do you want to cancel the parent workflow?

…

On Sat, Jun 24, 2017 at 08:37 Matthias Dittrich ***@***.***> wrote: I think we should task about compatibility later (if we can agree on something), but to be honest I don't see a lot of problems there: I don't think there is any existing code depending on "InnerException" being null on the OperationCancelledException. I feel the change you suggest on RunSynchronously could be more harmful if people depend on a different exception at the moment (but I'd accept that as bugfix, if my other points are honored). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3219 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACrtszeo18LeFVANw6epqnIiyiEK41N7ks5sHLymgaJpZM4N9gqN> .

[matthid]

@eiriktsarpalis I don't understand. What else should I cancel? Can you give a sample?

[eiriktsarpalis]

In the app you are writing, what prompts you to wire the parent to an external cancellation token?

…

On Sat, Jun 24, 2017 at 08:51 Matthias Dittrich ***@***.***> wrote: @eiriktsarpalis <https://github.com/eiriktsarpalis> I don't understand. What else should I cancel? Can you give a sample? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3219 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACrts8xo5GRevVadQmAHAMdiXsoLHh04ks5sHMAOgaJpZM4N9gqN> .

[matthid]

@eiriktsarpalis Not sure how that matters here. I'm discussing the problems on a fundamental level.

But basically you can see it in the paket pull request I linked in my initial post. What I was trying to do is to start some request logic which is doing requests to servers, some logic and eventually end up with a value. This is started in some custom "pre-loading" scheduler. Now at some point such tasks can be blocking for the paket resolver (if they don't already finish beforehand) in that case I wanted to implement cancellation, but at least print out which request wasn't working.

I wanted to use the build-in cancellation mechanism for that (with a custom token), but as you can see I couldn't make it work. Passing the tokens throughout the application might have worked but CancellationToken -> Async feels completely wrong, doesn't it? And I didn't want to do that, because it would be so invasive throughout the code-base.

[eiriktsarpalis]

My opinion is that binding on an asynchronous computation that could be cancelled is bad practice. I typically reserve the functionality to workflows that don't return anything (such as disposing mailbox processors or scheduling `Async<unit>` jobs that have proper fault tolerance set up). Attempting to route the ccont into the econt is futile, just look at how [`bind` is implemented]( https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/FSharp.Core/control.fs#L818). Unless you take care to isolate cancellation properly, your code is only one Bind (or TryWith as illustrated in my previous example) from stepping back into ccont. In your particular application I would just wrap the children in StartAsTask.

…

On Sat, Jun 24, 2017 at 09:00 Matthias Dittrich ***@***.***> wrote: @eiriktsarpalis <https://github.com/eiriktsarpalis> Not sure how that matters here. I'm discussing the problems on a fundamental level. But basically you can see it in the paket pull request I linked in my initial post. What I was trying to do is to start some request logic which is doing requests to servers, some logic and eventually end up with a value. This is started in some custom "pre-loading" scheduler. Now at some point such tasks can be blocking for the paket resolver (if they don't already finish beforehand) in that case I wanted to implement cancellation, but at least print out which request wasn't working. I wanted to use the build-in cancellation mechanism for that (with a custom token), but as you can see I couldn't make it work. Passing the tokens throughout the application might have worked but CancellationToken -> Async feels completely wrong, doesn't it? And I didn't want to do that, because it would be so invasive throughout the code-base. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3219 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACrts8KibRbkxF13u9XK6_K0X-tH3M4Vks5sHMIwgaJpZM4N9gqN> .

[matthid]

@eiriktsarpalis Sorry I don't get it. Escaping the token is not my problem. I already scoped it in StartAsTask, that's why I opened this issue (which is about StartAsTask behavior being wrong).

So maybe it helps to answer my question about which properties you expect from cancellation? Because obviously you don't agree with mine from above...

[eiriktsarpalis]

So to summarize what (I think) is being discussed here:

1. Async.StartAsTask cancellation wired directly to the input token, rather than a cancellation continuation.
2. Cancellation continuations should preserve exception information, or should even just use the exception continuation.

I maintain that the two issues are independent. Are you aiming at both here or do you really only care about one of the two? At the risk of repeating myself, here's my response to each of those:

1. I don't necessarily believe that the current behaviour is a bug. In any case, special applications can take advantage of the Async.StartWithContinuations primitive, like yours did.
2. I think this is where all the confusion rises from. Cancellation in async is cooperative and does not guarantee termination at all (for example, your async workflow could end into a non-monadic infinite loop and thus never again check the cancellation token). Exception continuations are another case in which cancellation checks may not be performed at all (which is why you were seeing the exception surface in your Async.RunSynchronously example). This happens because e.g. bind reuses the outer exception continuation verbatim but not all async combinators do this (e.g. TryWith as illustrated before). With this I'm trying to convince you that this behaviour is by design. More evidence of this fact can be found in this comment. The point of cancellation is that it provides a mechanism for the parent to signal cancellation for the child. The async mechanism will make best efforts to terminate the child, but this is not guaranteed to happen. It also implies that the parent forfeits any interest in the actual outcome of the computation. Signaling cancellation makes the computation effectively nondeterministic: it may complete, it may diverge, it may return an exception and in most cases it throws a placeholder OperationCanceledException. It's pretty much undefined behaviour. You may disagree with the design, but that's how it works and I doubt this will ever be changed for this library in particular in the interest of backward compatibility.

[matthid]

Wow sad. So you actually saying cancellation is undefined (there are no guarantees to anything). Which imho means it should just never be used ever. And we leave it like this for the sake of backwards compat...

[eiriktsarpalis]

Am I being sad for pointing this out? Is it an issue of opinion? I think I've provided more than enough data to back this up.

[eiriktsarpalis]

Please also explain to me how you expect the following to have reliable cancellation:

async { do while true do () }

[matthid]

Am I being sad for pointing this out? Is it an issue of opinion? I think I've provided more than enough data to back this up.

No I'm sad :). Sorry it shouldn't be offensive. It's just that I don't agree with your position. I think we should have a set of guarantees in the default implementations and provide primitives to lift some of those guarantees explicitly (if the user don't need them).

Which data? I can only see references to the existing implementation. I'm talking about changing those. I also don't agree with your position on #2127 it's basically the same reason others have already pointed out and I have pointed out here several times.

Please also explain to me how you expect the following to have deterministic cancellation:

I can't. Why should I? There is just no way to cancel this code. So why make people "belief" there would be a way when using StartAsTask and leave a thread blocked forever? If they want to have cancellation they need to implement and test it like everything else. That's the whole point of this issue.

[matthid]

That's what I'm talking about the whole time so your example is a good one. We should take a practical approach here. We shouldn't pretend that we can cancel something running in the real world, this only leads to confusion and broken code/subtle bugs.

[matthid]

This happens because e.g. bind reuses the outer exception continuation verbatim but not all async combinators do this (e.g. TryWith as illustrated before). With this I'm trying to convince you that this behaviour is by design. More evidence of this fact can be found in this comment.

This doesn't make any sense to me. You say it's by design because it is implemented this way?

also implies that the parent forfeits any interest in the actual outcome of the computation. Signaling cancellation makes the computation effectively nondeterministic: it may complete, it may diverge, it may return an exception and in most cases it throws a placeholder OperationCanceledException. It's pretty much undefined behaviour.

I don't get why we cannot change this, make the thing well defined and keep potentially critical information. I think I will step in and try to provide an implementation (to see what changes are actually required to provide this)... Accepted or not: It might make the discussion easier (because atm I don't think too much has to be changed)

[eiriktsarpalis]

I will reiterate that this not my opinion, I'm just describing how cancellation works in F# async. I would still like to know whether you're concerned about issue (1) or issue (2) as I described them earlier. I would also like to know whether your proposal of addressing (2) includes passing cancellation through the exception continuation.

I think a PR addressing (1) would be easier to accept, pending thorough investigation on how this might break backcompat. Issue (2) sounds like a big rabbithole, you're basically intent on changing the semantics of async. You would need to revise each and every async combinator that handles continuations so that it becomes compatible with your idea of cancellation. This of course won't include third-party async combinators found in the ecosystem, which presumably implement cancellation using the current semantics (MBrace being a good example). The task is daunting, the payoff is small, and the possibility of introducing unintended bugs is significant. People (including me) that have async code running in production wouldn't be too happy about such a change.

[eiriktsarpalis]

On a final note, consider Async.Parallel and the recent Async.Choice. These are parallelism combinators whose exception semantics behave as follows: on the first child to raise an exception, its siblings are signaled for cancellation and their results are discarded. This happens regardless of whether the siblings were successful or exceptional, their result is simply never surfaced by async. In the case of Choice, this happens even in the happy path, the first computation to succeed triggers cancellation and abandonment of its siblings.

Contrast this with Task.WhenAny and Task.WhenAll, where all child exceptions are surfaced in an AggregateException. Do you propose we also change the exception semantics of async?

[matthid]

@eiriktsarpalis I'm concerned about both

(1) Yes a fix is probably more straightforward to accept. Not sure if application should depend on the current (imho broken) behavior. We could certainly have some new function (maybe Task * CancellationToken -> Task for immediate cancellation - leaving the original task running or an additional parameter).

(2) I'm still looking at it but it doesn't look as invasive from my point of view. The resulting Exception is exactly the same as today. The only difference is that it would have an non-null InnerException providing the additional info. I don't think existing applications would change their behavior based on this (yeah it is possible but very very unlikely)

About Async.Choice and Async.Parallel, no I don't have any concrete idea how to improve them. But yeah I don't suggest using them because yes they potentially hide introduced bugs (and actually have in the paket code-base)

[eiriktsarpalis]

Implementation difficulties aside, I'm not sure I like the inner exception idea either. In my view an inner exception indicates a root cause for the parent exception. In this case we have an exception saying "I fired because the workflow canceled, but incidentally here's an unrelated exception (or two) that I happened to come across as I was unwinding the stack".

[matthid]

When an exception X is thrown as a direct result of a previous exception Y, the InnerException property of X should contain a reference to Y.

Use the InnerException property to obtain the set of exceptions that led to the current exception.

Now it really is a philosophical question if the cancellation started and continued because the task we were awaiting has honored the cancellation and thrown an exception or if we cancel because the token was marked as cancelled. Again for pragmatical reasons I'd just use that field :)
The super clear solution would probably be to define our own exception type which can collect errors happened while doing cleanup/cancellation. But I don't think that is actually an option at this point (for the sake of compatibility)

[matthid]

Closing in favor of #3257 and #3256 which basically show what I suggested here.