Fix 3219 - make StartAsTask wait for cancellation to take effect on task #3256

matthid · 2017-06-24T18:54:59Z

This fixes that StartAsTask will not wait properly for cancellation to finish, see #3219 (comment)

msftclas · 2017-06-24T18:55:04Z

@matthid,
Thanks for having already signed the Contribution License Agreement. Your agreement has not been validated yet. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

eiriktsarpalis · 2017-06-24T19:00:08Z

src/fsharp/FSharp.Core/control.fs

-            Start(token, a)
+            queueAsync
+                token
+                (fun r -> tcs.SetResult r |> fake)


@matthid shouldn't this be using the Try* methods instead?

Why? It should be guaranteed that only a single continuation is taken?

I would not rely on that assumption TBH

I guess you have some scenario in mind? Because currently I'm thinking: "Just let it crash if someone breaks that assumption"

I would not rely on that assumption TBH

My understanding is that in this situation you can rely on only one continuation being called.

matthid · 2017-06-24T19:09:24Z

src/fsharp/FSharp.Core/control.fs

+                token
+                (fun r -> tcs.SetResult r |> fake)
+                (fun edi -> tcs.SetException edi.SourceException |> fake)
+                (fun _ -> tcs.SetCanceled() |> fake)


We probably should even use (fun exn -> tcs.SetException(exn)).
I think consumers don't really see a difference and it unifies StartAsTask and RunSynchronously

Yes it looks like SetException is actually the right thing to do:
https://github.com/dotnet/coreclr/blob/68f72dd2587c3365a9fe74d1991f93612c3bc62a/src/mscorlib/src/System/Runtime/CompilerServices/AsyncMethodBuilder.cs#L670
It will set the task to "canceled" internally.

See also how this is potentially changed in #3257

matthid · 2017-06-24T19:54:23Z

src/fsharp/FSharp.Core.Unittests/FSharp.Core/Microsoft.FSharp.Control/AsyncType.fs

+        let tcs = TaskCompletionSource<unit>()
+        let a = async {
+            cts.CancelAfter (100)
+            do! tcs.Task |> Async.AwaitTask }


It makes me a bit nervous that I don't see a way to remove this race condition (ie calling cts.Cancel after the bind)

matthid · 2017-06-24T20:51:54Z

I had to rebase on top of #3255 because otherwise the new tests actually hit the fixed bug...

dsyme · 2017-06-26T14:45:46Z

@matthid @KevinRansom I'm labeling this as WIP - I definitely want much more review on this one. Reading through #3219 it is not yet at all clear to me the various options for resolution (and there tradeoffs against leaving things as they are, for example)

matthid · 2017-06-26T15:32:35Z

@dsyme I'm not sure what you mean. IMHO with current behavior it's very hard to write correct code. Especially if you don't understand exactly what it did before this change, it is very likely that most users of this API have race conditions in place...

matthid · 2017-06-26T15:34:13Z

At least I'd like to point out that I currently don't see any action I could do to make this a non WIP. Can we at least clarify who needs to do what?

dsyme · 2017-06-26T15:49:30Z

At least I'd like to point out that I currently don't see any action I could do to make this a non WIP. Can we at least clarify who needs to do what?

We need multiple review approvers and plenty of time to think about it. WIP is as good a way as any of saying "really do not merge yet"

dsyme · 2017-06-26T15:50:28Z

@matthid For example, let's finalize #3255 first

KevinRansom · 2017-08-29T17:15:52Z

@matthid, @dsyme this looks good to me, I resolved the conflict.

Is it done?

Thanks

Kevin

matthid · 2017-08-29T17:29:02Z

I don't know what to do, lets wait for @dsyme.

Apparently we need more review on this?

dsyme · 2017-08-29T18:59:25Z

src/fsharp/FSharp.Core/control.fs

@@ -1201,7 +1184,7 @@ namespace Microsoft.FSharp.Control
                args.aux.trampolineHolder.Protect((fun () ->
                    if completedTask.IsCanceled then
                        if useCcontForTaskCancellation
-                        then args.aux.ccont (new OperationCanceledException(args.aux.token))


Hmm... I thought this change was done in a separate PR (or we decided not to do it in a separate PR....)

dsyme · 2017-08-29T18:59:34Z

src/fsharp/FSharp.Core/control.fs

@@ -1216,7 +1199,7 @@ namespace Microsoft.FSharp.Control
                args.aux.trampolineHolder.Protect((fun () ->
                    if completedTask.IsCanceled then
                        if useCcontForTaskCancellation
-                        then args.aux.ccont (new OperationCanceledException(args.aux.token))
+                        then args.aux.ccont (new OperationCanceledException())


Likewise this one?

dsyme · 2017-08-29T19:01:44Z

This looks fine to me, apart form the fact that these two changes look unrelated? https://github.com/Microsoft/visualfsharp/pull/3256/files#diff-34ee82347e9e0b47265fcecaa4850174L1204

I'd also like additional approvers

matthid · 2017-08-29T19:56:40Z

@dsyme the changes are an artifact because you squash merge here and git can no longer recognize the change is already in the repository :/

matthid · 2017-08-29T19:57:08Z

I can try to get rid of it if you want.

dsyme · 2017-08-29T20:01:22Z

@dsyme the changes are an artifact because you squash merge here and git can no longer recognize the change is already in the repository :/

Oh, yes, I see. But yes, please get rid of them, makes me more comfortable

matthid · 2017-08-29T20:05:23Z

Ok I force pushed the rebase. I can see why you are kind of hesitant to merge this.

It really is a bit scary when a complicated primitive simplifies like this...

dsyme · 2017-08-29T20:08:36Z

src/fsharp/FSharp.Core/control.fs

+            queueAsync
+                token
+                (fun r -> tcs.SetResult r |> fake)
+                (fun edi -> tcs.SetException edi.SourceException |> fake)


Is there any way to maintain the stack trace here, e.g. some kind of tcs.SetExceptionDispatchInfo? I presume we lose the stack trace right now

is it worse than before?

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)

dsyme · 2017-08-29T20:13:35Z

It really is a bit scary when a complicated primitive simplifies like this...

Well, the code looks much better. But yes, more eyes are needed

matthid · 2017-08-29T20:19:06Z

But yes, more eyes are needed

It is a bit sad that we can't trust our test-suite on this. But I definitely agree - with all the stuff we fixed in that area lately - we can't trust it...

matthid · 2017-08-29T20:35:53Z

I think @tpetricek once said "if it's not simple it's impossible" ;)

dsyme · 2017-08-29T22:01:29Z

Technically I think Task will take care of not loosing info (wrap the exception in an aggregateexception or take care while rethrowing)

Could you add a test for the stack trace we get? e.g. check the names of the methods on the stack?

And check against existing behaviour too please, thanks

dsyme · 2017-08-29T22:03:56Z

Also compare with #2534 please

KevinRansom · 2017-08-30T00:49:06Z

@dsyme the callstack in an exception is not really part of the contract. Why is that interesting?

dsyme · 2017-08-30T11:12:26Z

@dsyme the callstack in an exception is not really part of the contract. Why is that interesting?

@KevinRansom That's very philosophical :) Though good stack traces are interesting, regardless of whether they are part of the contract :)

Anyways.... all the "ExceptionDispatchInfo" work in control.fs exists to give better stack traces. We somehow need to validate that we are not degrading that here. I personally prefer to put this under CI test and adjust should something change in .NET (which it is unlikely too in debug mode).

matthid · 2017-08-30T11:28:18Z

@dsyme
While I agree that such a test might be good, please don't try to push the requirements too high on me.
I already provided tests and SetException was called before my change the same way as it is now.

From my understanding the ExceptionDispatchInfo is a helper to keep stacktraces when re-throwing them. But we don't do that here.

Also I will provide the test but keep in mind that Stacktraces in async code are basically quite useless and they always where (even with the latest DispatchInfo additions). Everyone using async knows this, so please lets fix this but not as part of this bugfix.

dsyme · 2017-08-30T11:44:01Z

While I agree that such a test might be good, please don't try to push the requirements too high on me. I already provided tests and SetException was called before my change the same way as it is now.

The bar for changes is always high and I'd like us all to work together to make it much higher. I am doing what I want you to do to me :) I want more and more push back from all reviewers, especially given the fact we've introduced occasional regressions recently.

That said, I took a look at the code again and what you say is reasonable - the thing that had made me request this was the elimination of the EDI here:

(fun edi -> tcs.SetException edi.SourceException |> fake)

But as you say this EDI is newly introduced

...Stacktraces in async code are basically quite useless and they always where (even with the latest DispatchInfo additions)....

The important thing is that we don't regress, and that we improve where possible. The EDI changes improved some situations dramatically, though not enough as any of us would like.

dsyme · 2017-08-30T11:44:42Z

@matthid Given what you say above (SetException was called before) a CI test isn't required

matthid · 2017-08-30T11:52:17Z

The really important question to answer is about the change of behavior: As you can see from the linked issue my argument is that the previous behavior doesn't make any sense and cannot be used to write correct code. While there are arguments for the other behavior I don't really share any of them.

Also another hint that this is the "correct" change from a design perspective is that you cannot really achieve this primitive (as it is with this path) in user code, but you can very easily achieve the "old" behavior with this (see linked discussion).

Once we agree on the answer on this we should discuss potential breaking code (which IMHO always means the code is basically already broken right now if it depends on this behavior)

dsyme · 2017-08-30T12:07:26Z

The really important question to answer is about the change of behavior

I'm ok with the change of behaviour "make StartAsTask wait for cancellation to take effect on task", it is the intended behaviour for StartAsTask

We should discuss potential breaking code

It sounds like an RFC discussion, to discuss the breaking change. That would be the appropriate route to permanently document a change in behaviour. Alternatively, open a specific new issue on this and document the exact case there, or do it in the description of this PR?

matthid · 2017-08-30T12:12:27Z

We already have the discussion issue. For my part I really see it as bugfix to restore the intended behaviour (it seems you do as well), it's really up to you guys on how far you want to take that (I just wanted to be fair regarding to the critical voice of @eiriktsarpalis in the discussion)

dsyme · 2017-08-30T12:33:29Z

@matthid Yes, thanks.

Stepping back, I must say I basically very much prefer the implementation you propose. It makes total sense to me - it very much aligns with StartWithContinuations, and StartAsTask should basically be implementable in terms of a TCS and StartWithContinuations, which is effectively what you are doing. And that is also aligning with the proposed implementation of StartImmediateAsTask. When I saw it, I had no idea why the existing implementation was so contorted - and, as you point out, dubious under cancellation (nb. I didn't write the original code for StartAsTask)

msftclas added the cla-signed label Jun 24, 2017

eiriktsarpalis reviewed Jun 24, 2017

View reviewed changes

matthid commented Jun 24, 2017

View reviewed changes

matthid force-pushed the fix_3219 branch 2 times, most recently from bf40ea3 to eedeb09 Compare June 24, 2017 20:04

matthid mentioned this pull request Jun 24, 2017

Don't hide exceptions on the async cancellation-path #3257

Closed

KevinRansom approved these changes Jun 24, 2017

View reviewed changes

dsyme changed the title ~~Fix 3219~~ Fix 3219 - make StartAsTask wait properly for cancellation to take effect on task Jun 26, 2017

dsyme changed the title ~~Fix 3219 - make StartAsTask wait properly for cancellation to take effect on task~~ Fix 3219 - make StartAsTask wait for cancellation to take effect on task Jun 26, 2017

KevinRansom changed the title ~~Fix 3219 - make StartAsTask wait for cancellation to take effect on task~~ [WIP] Fix 3219 - make StartAsTask wait for cancellation to take effect on task Jun 26, 2017

KevinRansom changed the title ~~[WIP] Fix 3219 - make StartAsTask wait for cancellation to take effect on task~~ Fix 3219 - make StartAsTask wait for cancellation to take effect on task Aug 29, 2017

dsyme reviewed Aug 29, 2017

View reviewed changes

matthid added 2 commits August 29, 2017 22:02

add assertion message

c8e4003

fix Async.StartAsTask cancelling behavior (wait for stuff to cleanup)

2b902e5

matthid force-pushed the fix_3219 branch from b89c015 to 2b902e5 Compare August 29, 2017 20:02

dsyme reviewed Aug 29, 2017

View reviewed changes

dsyme approved these changes Aug 30, 2017

View reviewed changes

KevinRansom merged commit 6794923 into dotnet:master Sep 6, 2017

matthid mentioned this pull request Mar 9, 2018

Async.StartAsTask behavior in cancellation scenarios. #3219

Closed

matthid mentioned this pull request Aug 10, 2019

Cancel task continuations when async contexts are cancelled #7357

Closed

Frassle mentioned this pull request Sep 1, 2019

Change cancellation test to not depend on how AwaitTask works #7467

Merged

Fix 3219 - make StartAsTask wait for cancellation to take effect on task #3256

Fix 3219 - make StartAsTask wait for cancellation to take effect on task #3256

Conversation

matthid commented Jun 24, 2017

msftclas commented Jun 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthid commented Jun 24, 2017

dsyme commented Jun 26, 2017

matthid commented Jun 26, 2017

matthid commented Jun 26, 2017

dsyme commented Jun 26, 2017

dsyme commented Jun 26, 2017

KevinRansom commented Aug 29, 2017

matthid commented Aug 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsyme commented Aug 29, 2017

matthid commented Aug 29, 2017

matthid commented Aug 29, 2017

dsyme commented Aug 29, 2017

matthid commented Aug 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsyme commented Aug 29, 2017

matthid commented Aug 29, 2017

matthid commented Aug 29, 2017

dsyme commented Aug 29, 2017 • edited Loading

dsyme commented Aug 29, 2017

KevinRansom commented Aug 30, 2017

dsyme commented Aug 30, 2017

matthid commented Aug 30, 2017

dsyme commented Aug 30, 2017

dsyme commented Aug 30, 2017

matthid commented Aug 30, 2017

dsyme commented Aug 30, 2017

matthid commented Aug 30, 2017

dsyme commented Aug 30, 2017 • edited Loading

dsyme commented Aug 29, 2017 •

edited

Loading

dsyme commented Aug 30, 2017 •

edited

Loading