Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Async.StartWithContinuations executing different continuation on .NET Core #1416

Closed
ncave opened this issue Aug 10, 2016 · 11 comments
Closed
Labels

Comments

@ncave
Copy link
Contributor

ncave commented Aug 10, 2016

When running Async.StartWithContinuations on a cancelled task, we can observe different behavior between net46 and netcoreapp1.0 targets:

  • net46 goes to the cancellation handler (correct)
  • netcoreapp1.0 goes to the exception handler (incorrect)

Sample code:

    open System

    let testAsyncContinuations =
        let res = ref "start"
        let tcs = new System.Threading.CancellationTokenSource()
        let work =
            let work = async {
                do! Async.Sleep 100
                res := "end"
            }
            tcs.Cancel false
            Task.Async.StartAsTask(work, cancellationToken=tcs.Token) |> Async.AwaitTask

        Async.StartWithContinuations(work,    // if work is a cancelled task
            (fun _ -> res := "continuation"),
            (fun _ -> res := "exception"),    // <-- netcoreapp1.0 goes here (incorrect)
            (fun _ -> res := "cancellation")) // <-- net46 goes here (correct)

        Async.Sleep 200 |> Async.RunSynchronously
        res

    [<EntryPoint>]
    let main argv =
        printf "==> %A" testAsyncContinuations
        0

@eiriktsarpalis
Copy link
Member

@ncave you're probably seeing this because of the changes introduced by #1166, #1191. In short, binding on cancelled tasks should not trigger the cancellation continuation, but rather force a catchable TaskCancelledException. The cancellation continuation is reserved for cooperative cancellation by the async workflow only.

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Aug 11, 2016

@ncave which means that the behavior will also be changing as of F# 4.1 in net46

@ncave
Copy link
Contributor Author

ncave commented Aug 11, 2016

@eiriktsarpalis Thank you for your explanation, it was very helpful.

I probably don't have a good grasp on the design behind the Async API, but isn't your PR #1191 kind of invalidating the whole purpose of having a cancellation continuation, if we'll have to check for System.OperationCanceledException in the exception continuation anyway, even when the task was explicitly cancelled with a cancellation token without throwing an exception?

I get that some tasks may be cancelled on timeout when they actually should be throwing exceptions, but isn't that a problem with the task's implementation that needs to be addressed there? If not, if the expected behavior is to always throw on cancellation, then maybe we don't need the cancellation continuation? It will just confuse the user, who will expect the cancellation continuation to run on a cancelled task, which it will not.

(just trying to understand the design better, thank you for your help)

@eiriktsarpalis
Copy link
Member

No, the intended purpose of the cancellation continuation is to provide a means for workflow cancellation:

let cts = new CancellationTokenSource()
Async.StartAsTask(asyncWorkflow, cancellationToken = cts.Token)
cts.Cancel()

It should only be fired in the event of the externally supplied cancellation token being cancelled. In F# <= 4.0 the Async.AwaitTask implementation was erroneously triggering the cancellation continuation whenever the task got cancelled, meaning that it would be impossible to exception handle in the async context.

@ncave
Copy link
Contributor Author

ncave commented Aug 11, 2016

Do you mean the intended purpose of cancellation token source, or cancellation continuation? My question was about the purpose of the cancellation continuation, if (now) cancelling the token would always trigger the exception continuation.

To be more specific, I'm referring to the perceived inconsistency in Async.StartWithContinuations:

  • if you start with a task that gets cancelled, it calls the exception continuation, not the cancellation continuation.
  • if you start with a task and supply the (optional) token source parameter that you cancel afterwards, it calls the cancellation continuation. But it's an optional parameter, so if the user omits it she may have the wrong expectations about which continuation will be executed.

If this is how it should be, maybe it just needs to be documented better, but it does seem a bit counter-intuitive.

@eiriktsarpalis
Copy link
Member

I'm referring to the cancellation continuation. Also, this is unrelated to Async.StartWithContinuations. The observed behavior really reflects the implementation of Async.AwaitTask. Please see #1166 for a minimal example that illustrates the introduced change.

@ncave
Copy link
Contributor Author

ncave commented Aug 13, 2016

I did not realize ccont also raises exception, then it's kind of the same. But then continueWithUnit needs to be fixed the same way, right?

The change does alter the expected behavior downstream (e.g. Async.StartWithContinuations, see above), so it would be nice if that can be fixed somehow (so the correct continuation is called by Async.StartWithContinuations), or at least the new behavior needs to be well documented.

@eiriktsarpalis
Copy link
Member

I might raise an exception eventually, but the two are fundamentally different by the fact that the cancellation cont cannot be caught by exception handlers.

That's a good observation actually, continueWithUnit needs to be changed as well. We currently have inconsistent behavior. @dsyme @KevinRansom This probably needs to be fixed for F# 4.1. I'll create a PR

@dsyme
Copy link
Contributor

dsyme commented Nov 24, 2016

@eiriktsarpalis @ncave Can we close this?

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Nov 24, 2016 via email

@ncave
Copy link
Contributor Author

ncave commented Nov 24, 2016

Yes, although it's still a bit unexpected behavior from user's point of view. Closing.

@ncave ncave closed this as completed Nov 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants