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

[matthid]

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

/cc @eiriktsarpalis

[msftclas]

@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]

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

[matthid]

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

[eiriktsarpalis]

I would not rely on that assumption TBH

[matthid]

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

[dsyme]

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]

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

[matthid]

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.

[matthid]

See also how this is potentially changed in #3257

[matthid]

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]

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

[dsyme]

@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]

@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]

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]

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]

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

[KevinRansom]

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

Is it done?

Thanks

Kevin

[matthid]

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

Apparently we need more review on this?

[dsyme]

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

[dsyme]

Likewise this one?

[dsyme]

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]

@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]

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

[dsyme]

@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]

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]

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

[matthid]

is it worse than before?

[matthid]

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]

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]

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]

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

[dsyme]

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]

Also compare with #2534 please

[KevinRansom]

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

[dsyme]

@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]

@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]

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]

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

[matthid]

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]

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]

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]

@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)