Do not destroy co-routines too early. #26

rgriebl · 2021-12-06T00:21:47Z

Co-routines returning immediately with co_return would have an invalid
result, because the co-routine handle was destroyed before the caller
had a chance to get the result.

We do have to make sure to still destroy co-routines in final-suspend,
if nobody is awaiting the result (e.g. when called from Qt's event
loop).

Fixes: #24

danvratil · 2021-12-06T13:58:42Z

Generally this look OK, thanks a lot for working on the patch!

I think we need to extend the test suite to cover all of the possible cases, including your original case in the bug report, as well as other cases when the Task is ignored (but has a value).

I have some tests locally, I'll push them when I get to my home computer and try to write some more :-)

danvratil · 2021-12-07T15:26:49Z

Hi. I've merged some additional test cases for Task (see #29) - unfortunately the tests don't pass with your patch. I'll get back to you when I figure out what's wrong :)

danvratil · 2021-12-07T19:23:37Z

With some small corrections to the tests, the sync coroutine test passes, but all the other tests fail with use-after-free error.

Looks like when we destroy the coroutine in ~Task, it causes a use-after-free error in TaskFinalSuspend afterwards, since it checks whether the coroutine is done or not, using a handle to the now-destroyed coroutine.

Co-routines returning immediately with co_return would have an invalid result, because the co-routine handle was destroyed before the caller had a chance to get the result. We do have to make sure to still destroy co-routines in final-suspend, if nobody is awaiting the result (e.g. when called from Qt's event loop). Fixes: qcoro#24

rgriebl · 2021-12-08T11:22:29Z

I have a fix for the use-after-free. The problem comes from the resuming of the mAwaitingCoroutine in final_suspend: when the resume() call finally returns, ~Task() may have been run already and it would have destroyed the finishedCoroutine. Sadly there's no easy way (e.g. using a smart pointer) to detect that the finishedCoroutine handle is dead and dangling. The only way I found around this problem is another bool flag to notify ~Task that the coroutine is already stuck in final_suspend.

With this version, all your tests on main pass, but I had to fix the life-time of the TestContext object like this:


diff --git a/tests/testobject.h b/tests/testobject.h
index 84559a1..422a930 100644
--- a/tests/testobject.h
+++ b/tests/testobject.h
@@ -66,8 +66,9 @@ protected:
         QEventLoop el;
         QTimer::singleShot(5s, &el, [&el]() mutable { el.exit(1); });
 
+{
         [[maybe_unused]] const auto unused = (static_cast<TestClass *>(this)->*testFunction)(el);
-
+}
         bool testFinished = el.property("testFinished").toBool();
         const bool shouldNotSuspend = el.property("shouldNotSuspend").toBool();
         if (testFinished) {

github-actions · 2021-12-08T11:27:00Z

Unit Test Results

  12 files +  5 12 suites +5 53m 14s ⏱️ + 49m 59s
  13 tests ±  0   2 ✔️ - 10 0 💤 ±0   11 ❌ +  10
146 runs +62 24 ✔️ - 58 0 💤 ±0 122 ❌ +120

For more details on these failures, see this check.

Results for commit 8b7b312. ± Comparison against base commit 194a6ef.

danvratil

Hi @rgriebl , sorry for the huge delay in review. I tried to come up with some solution myself but got stuck and eventually distracted by other work. Sadly, the holidays weren't as peaceful as I hoped :-)

Anyway, I please add the scope fix for the test coroutine that you posted in your comment, I think we could land the patch this way.

I'll try to get back to reworking the Task in some way that will solve this problem in some generic way.

danvratil · 2022-01-04T12:53:52Z

qcoro/task.h

+            if (mCoroutine.done() && !mCoroutine.promise().isInFinalSuspend())
+                mCoroutine.destroy();
+            else
+                mCoroutine.promise().setDestroyOnFinalSuspend();


Please add braces, even for one-line if blocks.

danvratil · 2022-01-05T20:17:14Z

Hi,

there was another patch to solve this issue (#35) that goes basically the same way as yours, but the code is simpler (one atomics fewer) and has passing tests. I decided to merge that one. Apologies for not picking up your patch, hope you don't mind.

rgriebl force-pushed the fix-24 branch from b920f2e to 8b7b312 Compare December 8, 2021 11:15

danvratil reviewed Jan 4, 2022

View reviewed changes

danvratil mentioned this pull request Jan 4, 2022

Fix issue #24: Early co_returns do not resume the caller #35

Merged

danvratil closed this Jan 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not destroy co-routines too early. #26

Do not destroy co-routines too early. #26

Do not destroy co-routines too early. #26

Do not destroy co-routines too early. #26

Conversation

Unit Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment