-
Notifications
You must be signed in to change notification settings - Fork 868
Fix 3219 - make StartAsTask wait for cancellation to take effect on task #3256
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -974,33 +974,16 @@ namespace Microsoft.FSharp.Control | |
| let tcs = new TaskCompletionSource<_>(taskCreationOptions) | ||
|
|
||
| // The contract: | ||
| // a) cancellation signal should always propagate to task | ||
| // b) CancellationTokenSource that produced a token must not be disposed until the task.IsComplete | ||
| // We are: | ||
| // 1) registering for cancellation signal here so that not to miss the signal | ||
| // 2) disposing the registration just before setting result/exception on TaskCompletionSource - | ||
| // otherwise we run a chance of disposing registration on already disposed CancellationTokenSource | ||
| // (See (b) above) | ||
| // 3) ensuring if reg is disposed, we do SetResult | ||
| let barrier = VolatileBarrier() | ||
| let reg = token.Register(fun _ -> if barrier.Proceed then tcs.SetCanceled()) | ||
| // a) cancellation signal should always propagate to the computation | ||
| // b) when the task IsCompleted -> nothing is running anymore | ||
| let task = tcs.Task | ||
| let disposeReg() = | ||
| barrier.Stop() | ||
| if not (task.IsCanceled) then reg.Dispose() | ||
|
|
||
| let a = | ||
| async { | ||
| try | ||
| let! result = computation | ||
| do | ||
| disposeReg() | ||
| tcs.TrySetResult(result) |> ignore | ||
| with exn -> | ||
| disposeReg() | ||
| tcs.TrySetException(exn) |> ignore | ||
| } | ||
| Start(token, a) | ||
| queueAsync | ||
| token | ||
| (fun r -> tcs.SetResult r |> fake) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @matthid shouldn't this be using the Try* methods instead?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? It should be guaranteed that only a single continuation is taken?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would not rely on that assumption TBH
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess you have some scenario in mind? Because currently I'm thinking: "Just let it crash if someone breaks that assumption"
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
My understanding is that in this situation you can rely on only one continuation being called. |
||
| (fun edi -> tcs.SetException edi.SourceException |> fake) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any way to maintain the stack trace here, e.g. some kind of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it worse than before?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically I think Task will take care of not loosing info (wrap the exception in an aggregateexception or take care while rethrowing) I don't think we have enough access to the primitives to do something more useful (there are in fact some framework internal interesting apis) |
||
| (fun _ -> tcs.SetCanceled() |> fake) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We probably should even use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes it looks like
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See also how this is potentially changed in #3257 |
||
| computation | ||
| |> unfake | ||
| task | ||
|
|
||
| [<Sealed>] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes me a bit nervous that I don't see a way to remove this race condition (ie calling
cts.Cancelafter the bind)