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 issue #24: Early co_returns do not resume the caller #35

Merged
merged 2 commits into from Jan 5, 2022

Conversation

ivknv
Copy link
Contributor

@ivknv ivknv commented Jan 4, 2022

The fix involves adding a flag (TaskPromiseBase::mDestroyHandle) to check whether the coroutine handle can be destroyed. It is set/checked in TaskFinalSuspend and in ~Task(). This ensures that the coroutine handle gets destroyed only once and not too early: either in TaskFinalSuspend (only after ~Task()) or in ~Task() (only after TaskFinalSuspend).

I also had to change one line in tests/testobject.h. Not sure why, but without the change the tests would hang with the new fix applied.

@danvratil
Copy link
Owner

Hmm, looks like you arrived pretty much to the same solution as #26 :-)

I do wonder about the change in testobject.h (@rgriebl had to do the same in his patch), but I suspect it's more related to how checking that the coroutine was been suspended is implemented....

@ivknv
Copy link
Contributor Author

ivknv commented Jan 4, 2022

Hmm, looks like you arrived pretty much to the same solution as #26 :-)

I was actually implementing coroutines in a personal project, I originally wanted them to suspend initially, but then when looking at that issue (#24) I realized that I need them to work pretty much exactly like the ones in your library :). So when I was working on the non-initially-suspending version I came across similar problems. Naturally, I tried to do something similar to #26 but it didn't seem to work (pretty sure there's a race condition). So yeah, it's similar, but unlike #26, it uses fewer atomics and avoids potential race conditions. I also looked at valgrind output, it seems good too.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 4, 2022

Unit Test Results

  6 files  ±0    6 suites  ±0   2m 51s ⏱️ -1s
13 tests ±0  13 ✔️ ±0  0 💤 ±0  0 ±0 
73 runs  ±0  73 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 5aa1e17. ± Comparison against base commit e3ae4e5.

@danvratil danvratil merged commit d599729 into danvratil:main Jan 5, 2022
@danvratil
Copy link
Owner

Thanks a lot for the patch!

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.

None yet

2 participants