Skip to content

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

Merged
KevinRansom merged 2 commits into
dotnet:masterfrom
Frassle:asynccancellationtest
Sep 27, 2019
Merged

Change cancellation test to not depend on how AwaitTask works#7467
KevinRansom merged 2 commits into
dotnet:masterfrom
Frassle:asynccancellationtest

Conversation

@Frassle

@FrassleFrassle commented Aug 29, 2019

Copy link
Copy Markdown
Contributor

This test is to check that when an Async is started as a Task (via
StartAsTask) that when cancelled the Task isn't immediately marked as
cancelled but instead waits for the underlying Async to finish (normally
via cancellation)

Prompted from comments on #7357.

@Frassle

Frassle commented Aug 29, 2019

Copy link
Copy Markdown
ContributorAuthor

@matthid I think this test is still testing what you intended it to.

@matthid

matthid commented Aug 29, 2019

Copy link
Copy Markdown
Contributor

@matthid I think this test is still testing what you intended it to.

No, the test was exactly how I intended it ;)

@Frassle
Change cancellation test to not depend on how AwaitTask works
c322b87
This test is to check that when an Async is started as a Task (via
StartAsTask) that when cancelled the Task isn't immediately marked as
cancelled but instead waits for the underlying Async to finish (normally
via cancellation)
@Frassle
Add test that AwaitTask ignore async cancellation
75500f1
@Frassle
Frassleforce-pushed the asynccancellationtest branch from 5b96759 to 75500f1CompareAugust 29, 2019 18:29
@Frassle

Frassle commented Aug 29, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Bit sad that it tested both the behaviour of StartAsTask and AwaitTask in the same test. How about now, split into two tests, does that still test what you intended?

@matthid

matthid commented Aug 29, 2019

Copy link
Copy Markdown
Contributor

Yes I think they do, though testing those two primitives together is probably fine as they are often used together as well.

In any case increasing test coverage is a good thing.

matthid
matthid reviewed Aug 29, 2019
View reviewed changes
Comment threadtests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/AsyncType.fs
let innerTcs = new TaskCompletionSource<unit>()
let a = innerTcs.Task |> Async.AwaitTask

Async.StartWithContinuations(a, tcs.SetResult, tcs.SetException, ignore >> tcs.SetCanceled, cts.Token)

@matthidmatthidAug 29, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks a bit bogus to me you await the task here and use setresult as continuation. But if the task finished setresult is already called and the continuation will throw

@matthidmatthidAug 29, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah you have two tcs so this is basically just StartAsTask or very similar

@FrassleFrassleAug 29, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

tcs vs innerTcs, this is setting the tcs result, while the task that we AwaitTask on is from innerTcs.

@KevinRansom

KevinRansom commented Sep 1, 2019

Copy link
Copy Markdown
Contributor

@Frassle, I'm not sure I understand what benefits, this test change achieves. Are you sure you wish to perservere with this PR?

@Frassle

Frassle commented Sep 1, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Prompted by comments on #7357 (which is a PR to fix #2127).

It was pointed out that StartAsTaskCancellation was added as part of #3256 which changed how cancellation worked for asyncs started via StartAsTask. Pre-3256 the task would cancel as soon as the cancellation token was triggered, while post-3256 the task only cancels once the underlying async has finished. StartAsTaskCancellation tested this by using the fact that Async.AwaitTask ignores async cancellation to create an async where it was easy to control when it cancelled.

That use of AwaitTask is in conflict with #2127/#7357 and it isn't actually necessary for the test of StartAsTask. There's also currently no test to show that AwaitTask can't be cancelled. This PR hopefully makes the current state clearer. There's one test to show that AwaitTask can't be cancelled, and another test to show that StartAsTask waits for the underlying async (with less dependence on how other async functions work because we're now blocking via a spin loop).

KevinRansom
KevinRansom approved these changes Sep 24, 2019
View reviewed changes

@KevinRansomKevinRansom left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for this contribution.

Kevin

@KevinRansom
KevinRansom merged commit b2c8d66 into dotnet:masterSep 27, 2019
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
@Frassle@nosami
Change cancellation test to not depend on how AwaitTask works (dotnet…
857ea9d
…#7467)
* Change cancellation test to not depend on how AwaitTask works
This test is to check that when an Async is started as a Task (via
StartAsTask) that when cancelled the Task isn't immediately marked as
cancelled but instead waits for the underlying Async to finish (normally
via cancellation)
* Add test that AwaitTask ignore async cancellation
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@matthidmatthidmatthid left review comments
@KevinRansomKevinRansomKevinRansom approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Frassle@matthid@KevinRansom