Executor types remove circular refs#400
Merged
Merged
Conversation
4cd3361 to
e5298bc
Compare
The executors are circular ref'ing with the std::shared_ptr being copied into worker threads for coro::thread_pool and coro::io_scheduler (spawn mode). I'm leaving all other construct types that take an executor by shared_ptr to continue to do so, its important that the executors they run on do not destruct until all the tasks are completed, so the user needs to take care to destroy objects in a meaningful way so the executor is the last object to destruct.
e5298bc to
7e0820f
Compare
Have not switched coro::condition_variable or other constructs (but likely need to?)
I think shared_ptr usage is just flawed since it means that any lingering coroutine could destruct the executors. This will force the user to make sure it outlives any coroutines at the cost of possibly runtime nullptr references if they are going to use it past the lifetime of the executor.
wence-
reviewed
Oct 9, 2025
wence-
left a comment
Contributor
There was a problem hiding this comment.
This looks good to me, I tried it out in our larger application and everything seems good (in combination with #399). (We're not using any of the io_scheduler/networking features so not affected by any changes there).
Trivial nitpicks around some comments.
jbaldwin
commented
Oct 9, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The executors are circular ref'ing with the std::shared_ptr being copied into worker threads for coro::thread_pool and coro::io_scheduler (spawn mode).
I've determined that using std::shared_ptr is not the way to go since it gives an indeterminate destruction time, which can cause the coro::thread_pool or coro::io_scheduler to be destroyed on one of the threads running coroutines, possibly joining to itself and causing a SIGABRT. This should have been expected and I missed it and didn't consider the consequences and thought that using std::shared_ptr would make sure all the task constructs and the executors would keep each other alive. It did, since the executors circular referenced themselves, but without that circular reference the destruction is indeterminate as pointed out and cannot be reliably controlled. I think this means the best course of action is to revert to using
std::unique_ptrfor all executor types and force the user to make sure it lives for the correct livetime, and gets destroyed at the appropriate time on the correct thread.