add test and fix #3254 by matthid · Pull Request #3255 · dotnet/fsharp

matthid · 2017-06-24T15:43:02Z

msftclas · 2017-06-24T15:43:07Z

@matthid,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla.microsoft.com.

It will cover your contributions to all Microsoft-managed open source projects.
Thanks,
Microsoft Pull Request Bot

msftclas · 2017-06-24T15:53:57Z

@matthid, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, Microsoft Pull Request Bot

eiriktsarpalis · 2017-06-24T15:56:49Z

@matthid good catch, this should never have been added. Could also remove the continueWithExtra definition?

eiriktsarpalis · 2017-06-24T15:58:42Z

-
+
+ [<Test>]
+ member this.RunSynchronouslyCancellationWithDelayedResult () =


Could you add some kind of timeout to this test? Assuming the fix is not in place that test would run forever. Perhaps configurable via nunit?

I actually thought about that, but to be honest there are a lot of tests with similar expectations. Not sure a timeout there is worth it, but I can of course add one if you don't agree

as long as it doesn't obscure the intention of the test I guess

https://www.nunit.org/index.php?p=timeout&r=2.5

Thanks, done

matthid · 2017-06-24T15:59:42Z

@matthid good catch, this should never have been added. Could also remove the continueWithExtra definition?

Yep added.

eiriktsarpalis · 2017-06-24T16:13:08Z

@matthid this issue seems related #2770 (comment) could you also include this here? @saul

matthid · 2017-06-24T16:19:48Z

@eiriktsarpalis Any idea how a test for this would look like or what wrong behavior this would expose?
I can include it here but then there will be no test :)
To be honest I don't even think this code-path is used in any real life code. Only Async.Sleep is using it and there the result is correct, isn't it?

eiriktsarpalis · 2017-06-24T16:21:33Z

To be honest I don't even think this code-path is used in any real life code. Only Async.Sleep is using it and there the result is correct, isn't it?

That's correct. It's splitting hairs really, but I happened to spot this in the same PR you're reverting.

matthid · 2017-06-24T16:23:55Z

@eiriktsarpalis ok perfect, added it :)

eiriktsarpalis · 2017-06-24T16:25:59Z

Cheers, lgtm

matthid · 2017-06-24T17:38:34Z

@eiriktsarpalis It seems like something doesn't like the timeout attribute, ideas?

This reverts commit aeeaa50.

dsyme · 2017-06-26T14:43:38Z

What the cancellationToken does at this place is cancel the Task which is RETURNED by ContinueWith which means the continuation is never called. I'd remove this parameter.

Ugh... good catch, and I'm glad we found this now rather than later.

dsyme

I need to look at this again after reading further comments

dsyme · 2017-06-26T15:33:35Z

 args.aux.trampolineHolder.Protect((fun () ->
 if completedTask.IsCanceled then
 if useCcontForTaskCancellation
- then args.aux.ccont (new OperationCanceledException(args.aux.token))


Is this an intrinsic part of this fix? Or is only https://github.com/Microsoft/visualfsharp/pull/3255/files#diff-34ee82347e9e0b47265fcecaa4850174R1567 needed? If the latter, please remove this change to keep the changes entirely separate,

dsyme · 2017-06-26T15:33:42Z

 else
 args.cont completedTask.Result)) |> unfake

- task.ContinueWith(Action<Task<'T>>(continuation), continueWithExtra args.aux.token) |> ignore |> fake


Is this an intrinsic part of this fix? Or is only https://github.com/Microsoft/visualfsharp/pull/3255/files#diff-34ee82347e9e0b47265fcecaa4850174R1567 needed? If the latter, please remove this change to keep the changes entirely separate,

dsyme · 2017-06-26T15:33:46Z

 if completedTask.IsCanceled then
 if useCcontForTaskCancellation
- then args.aux.ccont (new OperationCanceledException(args.aux.token))
+ then args.aux.ccont (new OperationCanceledException())


Is this an intrinsic part of this fix? Or is only https://github.com/Microsoft/visualfsharp/pull/3255/files#diff-34ee82347e9e0b47265fcecaa4850174R1567 needed? If the latter, please remove this change to keep the changes entirely separate,

dsyme · 2017-06-26T15:34:52Z

My understanding is that only the one line change https://github.com/Microsoft/visualfsharp/pull/3255/files#diff-34ee82347e9e0b47265fcecaa4850174L1575 is required for the fix, the rest is a different issue?

matthid · 2017-06-26T16:26:39Z

The change was requested because it's part of the commit which broke this. I honestly don't care if it is part of this or not as I noted above it's basically changing nothing...

So I don't understand why I had to add it (it's basically cleanup) nor why now I should remove it again :/

So I'm fine either way but don't make me change that back and forth. @dsyme Did you see the above discussion and still think I should remove it?

/cc @eiriktsarpalis

dsyme · 2017-06-26T21:14:35Z

So I don't understand why I had to add it (it's basically cleanup) nor why now I should remove it again :/

Remove it please to minimise the required change, thanks

dsyme · 2017-06-27T09:50:30Z

@matthid I updated the PR directly, thanks

dsyme · 2017-06-27T10:13:55Z

@KevinRansom this is ready

matthid · 2017-06-27T13:21:18Z

@dsyme thanks. i'd probably rebase it and remove all those commits. but if you are fine with them I can leave it as is

matthid · 2017-06-27T13:21:56Z

Problem is I'm quite busy the mext days so it could take a while

dsyme · 2017-06-27T15:53:30Z

@matthid No problem - we will squash & merge in any case :)

add test andfixdotnet#3254
3d0d28a

msftclas added the cla-required label Jun 24, 2017

msftclas added cla-signed and removed cla-required labels Jun 24, 2017

remove continueWithExtra
d3f5357

eiriktsarpalis approved these changes Jun 24, 2017
View reviewed changes

add timeout attribute
aeeaa50

don't delegate the cancellationtoken to the operationcancelledexception.
cf55e6b

matthid mentioned this pull request Jun 24, 2017
Pass cancellation tokens and tasks to OperationCanceledException and TaskCanceledException #2770
Merged

Revert "add timeout attribute"
41718f1
This reverts commit aeeaa50.

This was referenced Jun 24, 2017
Don't hide exceptions on the async cancellation-path #3257
Closed
Fix 3219 - make StartAsTask wait for cancellation to take effect on task #3256
Merged

KevinRansom approved these changes Jun 24, 2017
View reviewed changes

dsyme approved these changes Jun 26, 2017
View reviewed changes

dsyme requested changes Jun 26, 2017
View reviewed changes

Update control.fs
75657e1

dsyme approved these changes Jun 27, 2017
View reviewed changes

dsyme merged commit c6a070b into dotnet:masterJun 27, 2017



		[<Test>]
		member this.RunSynchronouslyCancellationWithDelayedResult () =

Uh oh!

Conversation

matthid commented Jun 24, 2017

Uh oh!

msftclas commented Jun 24, 2017

Uh oh!

msftclas commented Jun 24, 2017

Uh oh!

eiriktsarpalis commented Jun 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthid commented Jun 24, 2017

Uh oh!

eiriktsarpalis commented Jun 24, 2017

Uh oh!

matthid commented Jun 24, 2017

Uh oh!

eiriktsarpalis commented Jun 24, 2017

Uh oh!

matthid commented Jun 24, 2017

Uh oh!

eiriktsarpalis commented Jun 24, 2017

Uh oh!

matthid commented Jun 24, 2017

Uh oh!

dsyme commented Jun 26, 2017

Uh oh!

dsyme left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsyme commented Jun 26, 2017

Uh oh!

matthid commented Jun 26, 2017

Uh oh!

dsyme commented Jun 26, 2017

Uh oh!

dsyme commented Jun 27, 2017

Uh oh!

dsyme commented Jun 27, 2017

Uh oh!

matthid commented Jun 27, 2017

Uh oh!

matthid commented Jun 27, 2017

Uh oh!

dsyme commented Jun 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants