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.AwaitTask behaviour on task canceled #1166

Closed
eiriktsarpalis opened this issue May 9, 2016 · 5 comments
Closed

Async.AwaitTask behaviour on task canceled #1166

eiriktsarpalis opened this issue May 9, 2016 · 5 comments

Comments

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented May 9, 2016

Reading this stackoverflow issue I was reminded of a peculiar behaviour that the Async.AwaitTask method has when the input task has been cancelled. Namely, it will trigger the cancellation continuation of the awaiting workflow, without the possibility of recovery.

Here's a simplistic reproduction that demonstrates the behaviour:

Async.RunSynchronously(async {
    let tcs = new System.Threading.Tasks.TaskCompletionSource<unit>()
    tcs.SetCanceled()
    try return! Async.AwaitTask tcs.Task
    with :? System.OperationCanceledException -> ()
})

I believe that this behaviour is wrong, for a couple of reasons:

  • It is contrary to the intended use of the cancellation continuation, which is to provide a means for cooperative cancellation by the creator of the asynchronous job.
  • Input tasks often originate from third party libraries, whose cancellation behaviour might be difficult to predict. A good example is the aforementioned SO thread.

My recommendation here would be to amend the implementation of Async.AwaitTask so that cancelled tasks should trigger the exception continuation pipeline. This would allow developers to reason about the origin of the problem and potentially prevent catastrophic wipe-outs of execution state.

@eiriktsarpalis
Copy link
Member Author

(bump)

Any thoughts on this? I could provide a PR if you like.

@dsyme
Copy link
Contributor

dsyme commented May 11, 2016

What you say is reasonable. It's plain wrong to cancel the whole workflow when Async.CancellationToken hasn't even been set.

  • Is this the only known problem with Async.AwaitTask? IIRC I've noticed other discussions about it's behaviour - do they all come down to this in the end?
  • Do you think it's definitely best to adjust the behavior of the current method, or should we add a new one (e.g. Async.AwaitTaskSafe) be added and the current one deprecated? I can see a logic to considering this a bug fix rather than a design change. Could you show some examples of code would change behaviour? Is it only those that explicitly catch-all exceptions (or catch OperationCanceledException?) within the workflow itself?
  • IIRC some task-returning APIs allow you to specify a cancellation token when creating a task (though I couldn't find particular examples). What would be the guidance for awaiting tasks in those cases? Is the guidance to pass Async.CancellationToken as the cancellation token, or to use CancellationTokenSource.CreateLinkedTokenSource to create a new cancellation token to pass into the task? If the latter .should we give assistance with that?

@eiriktsarpalis
Copy link
Member Author

  • Another possible issue with Async.AwaitTask is that it wraps all exceptions in an AggregateException. This merely mirrors the default behaviour of Task, however it is inconvenient when used in the context of async. It's not really a bug though, and changing it could break existing code.
  • I think we could safely modify the existing method. The current behaviour is so destructive that I doubt there is any production code out there that relies on it. AFAIK only the exception handling cases that you mention are going to demonstrate a change of behaviour. Finally clauses would run unaffected.
  • In most cases, simply passing Async.CancellationToken should suffice. Using a linked token is more appropriate in advanced scenaria where the user wants to combine different tokens or add manual control over task cancellation. We could possibly introduce
type Async with
    static member CreateLinkedTokenSource([<ParamArray>]otherTokens:CancellationToken[]) = async {
        let! ct = Async.CancellationToken
        return CancellationTokenSource.CreateLinkedTokenSource(Array.append [|ct|] otherTokens)
}

which generates a new CTS bound to the ambient CT.

@dsyme
Copy link
Contributor

dsyme commented May 11, 2016

@eiriktsarpalis OK, please submit a PR for this, as far as I'm concerned (from what I've seen so far) we can treat it as a bug fix.

@dsyme
Copy link
Contributor

dsyme commented May 26, 2016

Closing as it is fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants