Skip to content
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

Fix cancellation propagation by moving responsability to awaiter #1246

Merged
merged 5 commits into from
Dec 19, 2022

Conversation

sylveon
Copy link
Contributor

@sylveon sylveon commented Dec 13, 2022

& make propagation tests more reliable at detecting future failures. Relying on a race proved to be not reliable, as shown by #1228 only causing intermittent failures instead of consistent ones.

Some notes:

  • I made cancellable_awaiter use CRTP to avoid a virtual call to enable_cancellation, but this comes at the size of potential code size increase. Can easily revert to a virtual call to make cancellable_awaiter a non-template.
  • I had to move the awaiters for resume_on_signal and resume_after to be outside of their respective functions, otherwise I could not make await_suspend a template.
  • Are there any docs changes needed? I know this blog post will probably need one: https://devblogs.microsoft.com/oldnewthing/20200923-00/?p=104261
  • I guess now everybody can make cancellable promises by deriving from impl::cancellable_promise?
  • If cppwinrt v3 happens, should we make cancellation propagation always on?

Fixes #1243

Make propagation tests more reliable at detecting future failures
@sylveon
Copy link
Contributor Author

sylveon commented Dec 13, 2022

Looks like await_completed tests fail due to a slight increase in stack size caused by moving most of await_suspend to a non-template member function as to avoid duplicate codegen. Seems to work fine in release mode thanks to compiler optimizations probably eliding/inlining things. Is this an acceptable increase that we need to adjust the test for? Should we disable it in debug builds?

@alvinhochun
Copy link
Contributor

When you rebase this, please also revert the following change:

diff --git a/test/test/async_propagate_cancel.cpp b/test/test/async_propagate_cancel.cpp
index a3e5af749..b2e331929 100644
--- a/test/test/async_propagate_cancel.cpp
+++ b/test/test/async_propagate_cancel.cpp
@@ -132,7 +132,8 @@ namespace
 // FIXME: Test is known to segfault when built with Clang.
 TEST_CASE("async_propagate_cancel", "[.clang-crash]")
 #else
-TEST_CASE("async_propagate_cancel")
+// FIXME: mayfail because of https://github.com/microsoft/cppwinrt/issues/1243
+TEST_CASE("async_propagate_cancel", "[!mayfail]")
 #endif
 {
     Check(Action);

@oldnewthing
Copy link
Member

Looks like await_completed tests fail due to a slight increase in stack size... Is this an acceptable increase that we need to adjust the test for?

Yes, it's just a heuristic.

Should we disable it in debug builds?

No, because it is common to run unit tests only on debug builds before creating the PR, on the theory that debug builds will catch more errors than retail builds.

I had to move the awaiters for resume_on_signal and resume_after to be outside of their respective functions, otherwise I could not make await_suspend a template.

Specifically, because template type parameters must have linkage, so they cannot be local classes.

I guess now everybody can make cancellable promises by deriving from impl::cancellable_promise?

The impl namespace is off-limits, so people shouldn't be trying to derive from anything in there.

@sylveon
Copy link
Contributor Author

sylveon commented Dec 16, 2022

Yes, it's just a heuristic.

Yeah, I fixed it now.

No, because it is common to run unit tests only on debug builds before creating the PR, on the theory that debug builds will catch more errors than retail builds.

Looks like build_test_all.cmd uses release mode, which is why I didn't catch it myself when running the tests

The impl namespace is off-limits, so people shouldn't be trying to derive from anything in there.

Yeah, that was just a off-hand comment that this enables potentially more flexibility (e.g. if we move this out of the impl namespace third party coroutine classes could also opt into cancellation propagation).

@kennykerr kennykerr merged commit 8ac2b79 into microsoft:master Dec 19, 2022
fredemmott added a commit to OpenKneeboard/OpenKneeboard that referenced this pull request Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cancel propagation broken for nested IAsyncAction after #1228
4 participants