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

Pass cancellation tokens and tasks to OperationCanceledException and TaskCanceledException #2770

Merged
merged 3 commits into from
Apr 3, 2017

Conversation

saul
Copy link
Contributor

@saul saul commented Apr 1, 2017

This is exhibiting itself in #2100. Roslyn has a try/catch in ComputeProjectDiagnosticAnalyzerDiagnosticsAsync to see whether the cancellation came from the same cancellation token as the one that was passed into the analyzer.

The problem is Async isn't passing in the Task or CancelationToken to the exceptions, which C# does.

Fixes #2100

@vasily-kirichenko
Copy link
Contributor

Wow. It's deep.

@saul
Copy link
Contributor Author

saul commented Apr 1, 2017

@vasily-kirichenko the only ones that matter for #2100 are the TaskCanceledException. The OperationCanceledException changes are just for completeness ;)

@KevinRansom KevinRansom merged commit dd53a3d into dotnet:master Apr 3, 2017
@dsyme
Copy link
Contributor

dsyme commented Apr 3, 2017

Wow, I never knew. Great fix!

@@ -1541,8 +1541,8 @@ namespace Microsoft.FSharp.Control
let continuation (completedTask : Task<_>) : unit =
args.aux.trampolineHolder.Protect((fun () ->
if completedTask.IsCanceled then
if useCcontForTaskCancellation then args.aux.ccont(new OperationCanceledException())
else args.aux.econt (ExceptionDispatchInfo.Capture(new TaskCanceledException()))
if useCcontForTaskCancellation then args.aux.ccont(new OperationCanceledException(args.aux.token))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saul @dsyme this is incorrect. You are passing the async cancellation token which may not be related to the cancellation of the task we're binding on.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saul @matthid this line should be reverted to the original (unless there is a generic way to obtain a cancellation token from arbitrary tasks which I'm not aware of)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok added in #3255

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eiriktsarpalis @matthid What is this were if args.aux.token.IsCancellationRequested then new OperationCanceledException(args.aux.token) else new OperationCanceledException() ?

I suspect a lot of corresponding C# code would do the same thing: if a cancellation continuation needs to be converted into a OCE, then you would attach the current cancellation token in scope to the OCE, at least on the condition that that the current cancellation token in scope is in the cancelled state,

@@ -1555,8 +1555,8 @@ namespace Microsoft.FSharp.Control
let continuation (completedTask : Task) : unit =
args.aux.trampolineHolder.Protect((fun () ->
if completedTask.IsCanceled then
if useCcontForTaskCancellation then args.aux.ccont (new OperationCanceledException())
else args.aux.econt (ExceptionDispatchInfo.Capture(new TaskCanceledException()))
if useCcontForTaskCancellation then args.aux.ccont (new OperationCanceledException(args.aux.token))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

@saul
Copy link
Contributor Author

saul commented Jun 24, 2017

@eiriktsarpalis if you have a fix please open a PR

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

Successfully merging this pull request may close these issues.

7 participants