Replace coroutine_handle with continuation in executor interface#239
Merged
mvandeberg merged 1 commit intocppalliance:developfrom Mar 21, 2026
Merged
Replace coroutine_handle with continuation in executor interface#239mvandeberg merged 1 commit intocppalliance:developfrom
mvandeberg merged 1 commit intocppalliance:developfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (19)
📒 Files selected for processing (21)
📝 WalkthroughWalkthroughMigrates executor scheduling and awaitable storage from raw Changes
Sequence Diagram(s)sequenceDiagram
participant Coroutine as Coroutine (suspended)
participant Continuation as continuation (object)
participant Executor as Executor/strand/thread_pool
participant Worker as Worker Thread
Coroutine->>Continuation: construct continuation{h}
Continuation->>Executor: post(continuation&)
Executor->>Executor: enqueue continuation (intrusive next)
Executor->>Worker: worker dequeues continuation*
Worker->>Worker: c->h.resume()
Worker-->>Executor: done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
An automated preview of the documentation is available at https://239.capy.prtest3.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-03-21 03:16:43 UTC |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
include/boost/capy/concept/io_awaitable.hpp (1)
82-99:⚠️ Potential issue | 🟠 MajorDon't teach copying
continuationinto async callbacks.The updated example still says "pass members by value" and forwards
cont_tostart_async(). That is unsafe with the intrusive executor API: if the backend captures or forwards a copiedcontinuation,executor.post()can enqueue a node whose storage dies as soon as the callback returns. Keep the continuation in awaiter-owned storage and pass a pointer/reference to that member into the async backend instead.✏️ Suggested documentation fix
- cont_ = continuation{h}; - // Pass members by value; capturing this - // risks use-after-free in async callbacks. + cont_.h = h; + // Copy the stop token / executor, but keep the + // continuation in awaiter-owned storage. // When the async operation completes, resume // via executor.post(cont_) or executor.dispatch(cont_) // rather than calling h.resume() directly. start_async( env_->stop_token, env_->executor, - cont_ ); + &cont_ );The completion callback can then resume via
ex.post(*cont)/ex.dispatch(*cont).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/capy/concept/io_awaitable.hpp` around lines 82 - 99, The await_suspend currently copies continuation into cont_ and forwards that copy into start_async; instead keep the continuation in awaiter-owned storage (cont_ member) and pass a pointer or reference to that member into the async backend (i.e. call start_async with &cont_ or std::addressof(cont_) / cont_& reference) so the backend enqueues/uses the awaiter's storage rather than making its own lifetime-bound copy; update start_async/io backend signatures and any callbacks to accept and use a continuation* / continuation& and ensure callbacks do not capture 'this' or the copied continuation by value.example/asio/api/capy_streams.cpp (2)
112-119:⚠️ Potential issue | 🔴 CriticalType mismatch:
ex.post(h)passesstd::coroutine_handle<>butpostnow expectscontinuation&.The completion handler captures a raw
std::coroutine_handle<>(h) and passes it toex.post(h), but the executor API now requirescontinuation&. This will fail to compile.Store a
continuationmember in the awaitable and post that instead:🐛 Proposed fix
template<capy::MutableBufferSequence MB> class read_awaitable { asio_socket* self_; MB buffers_; capy::io_result<std::size_t> result_; std::shared_ptr<cancel_state> cancel_; + capy::continuation cont_; public: // ... constructor ... std::coroutine_handle<> await_suspend( std::coroutine_handle<> h, capy::io_env const* env) { cancel_ = std::make_shared<cancel_state>(env->stop_token); + cont_.h = h; self_->socket_.async_read_some( capy::to_asio(capy::mutable_buffer_array<8>(buffers_)), net::bind_cancellation_slot( cancel_->signal.slot(), - [this, h, ex = env->executor]( + [this, ex = env->executor]( boost::system::error_code ec, std::size_t n) mutable { result_.ec = ec; std::get<0>(result_.values) = n; - ex.post(h); + ex.post(cont_); })); return std::noop_coroutine(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/asio/api/capy_streams.cpp` around lines 112 - 119, The completion handler currently captures a raw std::coroutine_handle<> named h and calls ex.post(h), but the executor API requires a continuation&, so modify the awaitable to store a continuation member (e.g., continuation cont_) and capture/post that instead of h; update the lambda capture list from h to cont_ (and adjust any places where h is set to instead construct or assign cont_ from the coroutine handle using continuation::from_handle or equivalent), then call ex.post(cont_) inside the handler so result_.ec and result_.values assignment remain unchanged while matching the new executor signature.
174-181:⚠️ Potential issue | 🔴 CriticalSame type mismatch in
write_awaitable.Apply the same fix as
read_awaitable— store acontinuationmember and post it from the completion handler instead of the raw handle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/asio/api/capy_streams.cpp` around lines 174 - 181, The completion handler for write_awaitable currently captures and posts the raw handle (h) causing a type mismatch; mirror the read_awaitable fix by storing the awaitable's continuation in a member (e.g., continuation_) when the awaitable is created and capture that continuation in the lambda instead of h, assign result_.ec and std::get<0>(result_.values) = n as before, and call env->executor.post(continuation_) (or continuation_.post via the stored continuation) from the completion handler so the correct continuation type is posted.
🧹 Nitpick comments (2)
include/boost/capy/ex/async_mutex.hpp (1)
28-60: Refresh the implementation note after theh_→cont_rename.The overview still says the stop callback posts
h_and that the member-ordering constraint depends onh_, but the code now postscont_. Keeping that rationale stale makes the cancellation/lifetime guarantees harder to audit.As per coding guidelines, "Files containing non-trivial implementation logic should include a `/* */` block comment after the includes that provides a high-level overview of how the implementation works" and you should warn if the overview appears stale relative to the code.📝 Minimal doc update
- The stop callback calls ex_.post(h_). The stop_callback is + The stop callback calls ex_.post(cont_). The stop_callback is @@ - the callback accesses (h_, ex_, claimed_, canceled_). If the + the callback accesses (cont_, ex_, claimed_, canceled_). If theAlso applies to: 167-188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/capy/ex/async_mutex.hpp` around lines 28 - 60, Update the top-of-file implementation note in async_mutex to reflect the rename h_ → cont_: change any wording that says the stop callback posts h_ to say it posts cont_, and update the member-ordering constraint to reference cont_ (and the union stop_cb_) instead of h_; ensure examples and explanations that mention h_, stop_callback, stop_cb_, lock_awaiter, claimed_, canceled_, await_suspend, await_resume, unlock(), and async_mutex::waiters_ are consistent with the current code paths (stop callback posts cont_, await_suspend constructs stop_cb_, await_resume/destructor destroy it, unlock() skips claimed_ waiters and pops from waiters_), and add a brief note reminding maintainers to update this overview when renaming members to avoid stale lifetime/cancellation rationale.include/boost/capy/concept/executor.hpp (1)
143-154: Concept usescontinuationby value, but implementations takecontinuation&.The requires expression declares
continuation c(by value), but the documented signatures and all implementations in this PR usecontinuation&(by reference). When checkingce.dispatch(c)andce.post(c), passing a valuecto a function expectingcontinuation&will still bind correctly (lvalue binds to lvalue reference), so the constraint works. However, for documentation consistency and clarity, consider usingcontinuation& cin the requires clause to match the actual API.✏️ Optional: align requires clause with implementation signatures
template<class E> concept Executor = std::is_nothrow_copy_constructible_v<E> && std::is_nothrow_move_constructible_v<E> && - requires(E& e, E const& ce, E const& ce2, continuation c) { + requires(E& e, E const& ce, E const& ce2, continuation& c) { { ce == ce2 } noexcept -> std::convertible_to<bool>; { ce.context() } noexcept; requires std::is_lvalue_reference_v<decltype(ce.context())> && std::derived_from< std::remove_reference_t<decltype(ce.context())>, execution_context>; { ce.on_work_started() } noexcept; { ce.on_work_finished() } noexcept; { ce.dispatch(c) } -> std::same_as<std::coroutine_handle<>>; { ce.post(c) }; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/capy/concept/executor.hpp` around lines 143 - 154, The requires-clause in the executor concept declares a local parameter as "continuation c" but actual APIs and implementations use "continuation&"; change the parameter declaration to "continuation& c" in the requires expression so the concept matches the documented/implemented signatures (update the occurrences that check ce.dispatch(c) and ce.post(c) accordingly), ensuring the checks still reference ce.dispatch and ce.post and the continuation type symbol remains "continuation" by reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@example/asio/api/uni_stream.hpp`:
- Around line 58-68: The methods dispatch and post in the asio_executor_wrapper
use an unqualified type name continuation that won't resolve; update their
parameter types to use the fully-qualified capy::continuation& (and any other
occurrences in asio_executor_wrapper) so the compiler can find the type (e.g.,
change dispatch(continuation& c) and post(continuation& c) signatures to
dispatch(capy::continuation& c) and post(capy::continuation& c) and adjust any
matching declarations/uses accordingly).
In `@include/boost/capy/continuation.hpp`:
- Around line 20-35: Replace the plain block comment above struct continuation
with a proper public-header docstring describing the type: explain that
continuation is an executor-facing schedulable unit that wraps a
std::coroutine_handle<> (h), that the intrusive pointer continuation* next_ is
owned/managed by executors as linkage (not by users), and state the
ownership/aliasing rules — a continuation must have a stable address while
enqueued and must not be enqueued more than once concurrently; also document
copy/move semantics (non-copyable or delegete default as appropriate) and any
thread-safety guarantees or expectations (e.g., callers/executors must
synchronize access). Include these points succinctly in the docstring so tooling
and users see purpose, ownership, copyability/movability, and thread-safety for
continuation, h, and next_.
---
Outside diff comments:
In `@example/asio/api/capy_streams.cpp`:
- Around line 112-119: The completion handler currently captures a raw
std::coroutine_handle<> named h and calls ex.post(h), but the executor API
requires a continuation&, so modify the awaitable to store a continuation member
(e.g., continuation cont_) and capture/post that instead of h; update the lambda
capture list from h to cont_ (and adjust any places where h is set to instead
construct or assign cont_ from the coroutine handle using
continuation::from_handle or equivalent), then call ex.post(cont_) inside the
handler so result_.ec and result_.values assignment remain unchanged while
matching the new executor signature.
- Around line 174-181: The completion handler for write_awaitable currently
captures and posts the raw handle (h) causing a type mismatch; mirror the
read_awaitable fix by storing the awaitable's continuation in a member (e.g.,
continuation_) when the awaitable is created and capture that continuation in
the lambda instead of h, assign result_.ec and std::get<0>(result_.values) = n
as before, and call env->executor.post(continuation_) (or continuation_.post via
the stored continuation) from the completion handler so the correct continuation
type is posted.
In `@include/boost/capy/concept/io_awaitable.hpp`:
- Around line 82-99: The await_suspend currently copies continuation into cont_
and forwards that copy into start_async; instead keep the continuation in
awaiter-owned storage (cont_ member) and pass a pointer or reference to that
member into the async backend (i.e. call start_async with &cont_ or
std::addressof(cont_) / cont_& reference) so the backend enqueues/uses the
awaiter's storage rather than making its own lifetime-bound copy; update
start_async/io backend signatures and any callbacks to accept and use a
continuation* / continuation& and ensure callbacks do not capture 'this' or the
copied continuation by value.
---
Nitpick comments:
In `@include/boost/capy/concept/executor.hpp`:
- Around line 143-154: The requires-clause in the executor concept declares a
local parameter as "continuation c" but actual APIs and implementations use
"continuation&"; change the parameter declaration to "continuation& c" in the
requires expression so the concept matches the documented/implemented signatures
(update the occurrences that check ce.dispatch(c) and ce.post(c) accordingly),
ensuring the checks still reference ce.dispatch and ce.post and the continuation
type symbol remains "continuation" by reference.
In `@include/boost/capy/ex/async_mutex.hpp`:
- Around line 28-60: Update the top-of-file implementation note in async_mutex
to reflect the rename h_ → cont_: change any wording that says the stop callback
posts h_ to say it posts cont_, and update the member-ordering constraint to
reference cont_ (and the union stop_cb_) instead of h_; ensure examples and
explanations that mention h_, stop_callback, stop_cb_, lock_awaiter, claimed_,
canceled_, await_suspend, await_resume, unlock(), and async_mutex::waiters_ are
consistent with the current code paths (stop callback posts cont_, await_suspend
constructs stop_cb_, await_resume/destructor destroy it, unlock() skips claimed_
waiters and pops from waiters_), and add a brief note reminding maintainers to
update this overview when renaming members to avoid stale lifetime/cancellation
rationale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bac6df14-e21b-4f00-9dd6-2bae70f1980b
⛔ Files ignored due to path filters (18)
bench/bench.cppis excluded by!**/bench/**doc/continuation-rationale.mdis excluded by!**/doc/**doc/modules/ROOT/pages/4.coroutines/4c.executors.adocis excluded by!**/doc/**doc/modules/ROOT/pages/7.examples/7n.custom-executor.adocis excluded by!**/doc/**doc/modules/ROOT/pages/8.design/8k.Executor.adocis excluded by!**/doc/**doc/unlisted/execution-executors.adocis excluded by!**/doc/**doc/unlisted/io-awaitables-executor.adocis excluded by!**/doc/**include/boost/capy/test/run_blocking.hppis excluded by!**/test/**include/boost/capy/test/stream.hppis excluded by!**/test/**src/test/run_blocking.cppis excluded by!**/test/**test/unit/ex/any_executor.cppis excluded by!**/test/**test/unit/ex/executor_ref.cppis excluded by!**/test/**test/unit/ex/run_async.cppis excluded by!**/test/**test/unit/ex/strand.cppis excluded by!**/test/**test/unit/ex/thread_pool.cppis excluded by!**/test/**test/unit/ex/work_guard.cppis excluded by!**/test/**test/unit/task.cppis excluded by!**/test/**test/unit/test_helpers.hppis excluded by!**/test/**
📒 Files selected for processing (21)
example/asio/api/capy_streams.cppexample/asio/api/uni_stream.hppexample/custom-executor/custom_executor.cppinclude/boost/capy/concept/executor.hppinclude/boost/capy/concept/io_awaitable.hppinclude/boost/capy/continuation.hppinclude/boost/capy/delay.hppinclude/boost/capy/ex/any_executor.hppinclude/boost/capy/ex/async_event.hppinclude/boost/capy/ex/async_mutex.hppinclude/boost/capy/ex/executor_ref.hppinclude/boost/capy/ex/io_env.hppinclude/boost/capy/ex/run.hppinclude/boost/capy/ex/run_async.hppinclude/boost/capy/ex/strand.hppinclude/boost/capy/ex/thread_pool.hppinclude/boost/capy/timeout.hppinclude/boost/capy/when_all.hppinclude/boost/capy/when_any.hppsrc/ex/detail/strand_service.cppsrc/ex/thread_pool.cpp
|
GCOVR code coverage report https://239.capy.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-03-21 03:09:18 UTC |
mvandeberg
force-pushed
the
pr/continuation
branch
2 times, most recently
from
9dd6429 to
834a2eb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
include/boost/capy/timeout.hpp (1)
38-38: Maketimeout.hppself-contained by adding direct includes.
timeout_statedirectly usesstd::array<continuation, 2>on line 38, but this header gets both symbols transitively fromwhen_all.hpp. Adding direct includes for<array>and<boost/capy/continuation.hpp>will make the public header less brittle to future include reorganizations.♻️ Suggested include additions
+#include <array> `#include` <boost/capy/detail/config.hpp> `#include` <boost/capy/concept/io_awaitable.hpp> `#include` <boost/capy/delay.hpp> `#include` <boost/capy/detail/io_result_combinators.hpp> +#include <boost/capy/continuation.hpp> `#include` <boost/capy/error.hpp> `#include` <boost/capy/io_result.hpp> `#include` <boost/capy/task.hpp> `#include` <boost/capy/when_all.hpp>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/capy/timeout.hpp` at line 38, timeout_state uses std::array<continuation, 2> (member runner_handles_) but relies on transitive includes; make the header self-contained by adding direct includes for <array> and <boost/capy/continuation.hpp> at the top of timeout.hpp so continuation and std::array are declared where timeout_state and runner_handles_ are defined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/boost/capy/concept/executor.hpp`:
- Around line 48-49: Add a resource-lifetime paragraph to the executor concept
docs clarifying the continuation lifetime: state that ce.dispatch(continuation)
and ce.post(continuation) operate on the supplied continuation object itself and
that the caller must ensure the continuation remains alive until the dispatcher
resumes it; explicitly document that dispatch returns std::coroutine_handle<>
referring to the continuation and that destroying or moving the continuation
before resume is undefined behavior. Include this text near the existing
descriptions of ce.dispatch and ce.post (and in the `@par` Buffer Lifetime /
Resource Lifetime section referenced for the concept), and mirror the same
wording for the other occurrences around the comments for lines referencing
dispatch/post/continuation so implementers see a consistent public contract.
---
Nitpick comments:
In `@include/boost/capy/timeout.hpp`:
- Line 38: timeout_state uses std::array<continuation, 2> (member
runner_handles_) but relies on transitive includes; make the header
self-contained by adding direct includes for <array> and
<boost/capy/continuation.hpp> at the top of timeout.hpp so continuation and
std::array are declared where timeout_state and runner_handles_ are defined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cc7a63cd-31c2-43f5-9d14-721661b68b94
⛔ Files ignored due to path filters (19)
bench/bench.cppis excluded by!**/bench/**doc/continuation-rationale.mdis excluded by!**/doc/**doc/modules/ROOT/pages/4.coroutines/4c.executors.adocis excluded by!**/doc/**doc/modules/ROOT/pages/7.examples/7n.custom-executor.adocis excluded by!**/doc/**doc/modules/ROOT/pages/8.design/8k.Executor.adocis excluded by!**/doc/**doc/unlisted/execution-executors.adocis excluded by!**/doc/**doc/unlisted/io-awaitables-executor.adocis excluded by!**/doc/**include/boost/capy/test/run_blocking.hppis excluded by!**/test/**include/boost/capy/test/stream.hppis excluded by!**/test/**src/test/run_blocking.cppis excluded by!**/test/**test/unit/ex/any_executor.cppis excluded by!**/test/**test/unit/ex/executor_ref.cppis excluded by!**/test/**test/unit/ex/frame_cb.cppis excluded by!**/test/**test/unit/ex/run_async.cppis excluded by!**/test/**test/unit/ex/strand.cppis excluded by!**/test/**test/unit/ex/thread_pool.cppis excluded by!**/test/**test/unit/ex/work_guard.cppis excluded by!**/test/**test/unit/task.cppis excluded by!**/test/**test/unit/test_helpers.hppis excluded by!**/test/**
📒 Files selected for processing (21)
example/asio/api/capy_streams.cppexample/asio/api/uni_stream.hppexample/custom-executor/custom_executor.cppinclude/boost/capy/concept/executor.hppinclude/boost/capy/concept/io_awaitable.hppinclude/boost/capy/continuation.hppinclude/boost/capy/delay.hppinclude/boost/capy/ex/any_executor.hppinclude/boost/capy/ex/async_event.hppinclude/boost/capy/ex/async_mutex.hppinclude/boost/capy/ex/executor_ref.hppinclude/boost/capy/ex/io_env.hppinclude/boost/capy/ex/run.hppinclude/boost/capy/ex/run_async.hppinclude/boost/capy/ex/strand.hppinclude/boost/capy/ex/thread_pool.hppinclude/boost/capy/timeout.hppinclude/boost/capy/when_all.hppinclude/boost/capy/when_any.hppsrc/ex/detail/strand_service.cppsrc/ex/thread_pool.cpp
✅ Files skipped from review due to trivial changes (2)
- include/boost/capy/ex/async_mutex.hpp
- include/boost/capy/continuation.hpp
🚧 Files skipped from review as they are similar to previous changes (12)
- example/asio/api/uni_stream.hpp
- include/boost/capy/delay.hpp
- example/custom-executor/custom_executor.cpp
- include/boost/capy/ex/strand.hpp
- include/boost/capy/concept/io_awaitable.hpp
- include/boost/capy/ex/thread_pool.hpp
- include/boost/capy/ex/any_executor.hpp
- src/ex/thread_pool.cpp
- example/asio/api/capy_streams.cpp
- include/boost/capy/ex/executor_ref.hpp
- src/ex/detail/strand_service.cpp
- include/boost/capy/ex/io_env.hpp
mvandeberg
force-pushed
the
pr/continuation
branch
3 times, most recently
from
89da966 to
500e87b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
include/boost/capy/when_all.hpp (1)
514-517: Phase 2 iteration count is correct, but consider caching for clarity.The logic correctly reads
remaining_count_before any runners have completed, so it equals the original count. However, reading from an atomic here when the value is guaranteed stable (no concurrent decrements yet) may be confusing to future readers.Consider caching the count locally in Phase 1 or using a named constant to make the invariant more explicit.
♻️ Optional: Cache count locally
// Phase 1: Create all runners without dispatching. std::size_t index = 0; for(auto&& a : *range_) { // ... existing code ... ++index; } // Phase 2: Post all runners. Any may complete synchronously. // After last post, state_ and this may be destroyed. auto* handles = state_->runner_handles_.get(); - std::size_t count = state_->core_.remaining_count_.load(std::memory_order_relaxed); + std::size_t count = index; // equals original count from Phase 1 for(std::size_t i = 0; i < count; ++i) caller_env->executor.post(handles[i]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/capy/when_all.hpp` around lines 514 - 517, Readability: cache the phase-2 iteration count instead of re-reading the atomic remaining_count_ when dispatching runners. Capture the stable count into a local (e.g., auto phase2_count = state_->core_.remaining_count_.load(...); or better, store the original count into a named field like state_->core_.initial_count_ during Phase 1) and then loop using that local (for (std::size_t i = 0; i < phase2_count; ++i) caller_env->executor.post(handles[i]);) so the code no longer reads the atomic at dispatch and the invariant is explicit; update uses of remaining_count_ accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@example/asio/api/capy_streams.cpp`:
- Around line 245-249: The example no longer compiles because executor_ref::post
now requires a continuation&, but the read/write callbacks capture and call the
old executor posting pattern; fix both awaitables (read_awaitable and
write_awaitable) by giving each awaitable a stable member of type continuation
(e.g., continuation cont_;), assign cont_ = c inside await_suspend, and update
the completion lambdas to call ex_.post(cont_) (or call net::post(ex_, [this]{
cont_.resume(); }) if using net::post) instead of posting the raw coroutine
handle; ensure the member is used for both read_awaitable and write_awaitable so
the callbacks use the continuation API.
- Around line 238-242: dispatch() is currently always queuing the continuation
via net::post and returning std::noop_coroutine(), which breaks symmetric
transfer because run_awaitable_ex::await_suspend() and the trampoline
final_suspend() expect dispatch() to return the child's handle for inline
transfer when not deferred; change dispatch(continuation& c) to only call
net::post(ex_, ...) and return std::noop_coroutine() when the continuation must
be deferred, otherwise return c.h (preserving the inline path so run() on
asio_executor can transfer directly to the child/parent coroutine); update
dispatch() logic to detect whether execution can be performed inline and choose
between returning c.h or scheduling with net::post accordingly.
---
Nitpick comments:
In `@include/boost/capy/when_all.hpp`:
- Around line 514-517: Readability: cache the phase-2 iteration count instead of
re-reading the atomic remaining_count_ when dispatching runners. Capture the
stable count into a local (e.g., auto phase2_count =
state_->core_.remaining_count_.load(...); or better, store the original count
into a named field like state_->core_.initial_count_ during Phase 1) and then
loop using that local (for (std::size_t i = 0; i < phase2_count; ++i)
caller_env->executor.post(handles[i]);) so the code no longer reads the atomic
at dispatch and the invariant is explicit; update uses of remaining_count_
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ba4bd244-cb1c-462c-a281-dbe0dfbabf3d
⛔ Files ignored due to path filters (19)
bench/bench.cppis excluded by!**/bench/**doc/continuation-rationale.mdis excluded by!**/doc/**doc/modules/ROOT/pages/4.coroutines/4c.executors.adocis excluded by!**/doc/**doc/modules/ROOT/pages/7.examples/7n.custom-executor.adocis excluded by!**/doc/**doc/modules/ROOT/pages/8.design/8k.Executor.adocis excluded by!**/doc/**doc/unlisted/execution-executors.adocis excluded by!**/doc/**doc/unlisted/io-awaitables-executor.adocis excluded by!**/doc/**include/boost/capy/test/run_blocking.hppis excluded by!**/test/**include/boost/capy/test/stream.hppis excluded by!**/test/**src/test/run_blocking.cppis excluded by!**/test/**test/unit/ex/any_executor.cppis excluded by!**/test/**test/unit/ex/executor_ref.cppis excluded by!**/test/**test/unit/ex/frame_cb.cppis excluded by!**/test/**test/unit/ex/run_async.cppis excluded by!**/test/**test/unit/ex/strand.cppis excluded by!**/test/**test/unit/ex/thread_pool.cppis excluded by!**/test/**test/unit/ex/work_guard.cppis excluded by!**/test/**test/unit/task.cppis excluded by!**/test/**test/unit/test_helpers.hppis excluded by!**/test/**
📒 Files selected for processing (21)
example/asio/api/capy_streams.cppexample/asio/api/uni_stream.hppexample/custom-executor/custom_executor.cppinclude/boost/capy/concept/executor.hppinclude/boost/capy/concept/io_awaitable.hppinclude/boost/capy/continuation.hppinclude/boost/capy/delay.hppinclude/boost/capy/ex/any_executor.hppinclude/boost/capy/ex/async_event.hppinclude/boost/capy/ex/async_mutex.hppinclude/boost/capy/ex/executor_ref.hppinclude/boost/capy/ex/io_env.hppinclude/boost/capy/ex/run.hppinclude/boost/capy/ex/run_async.hppinclude/boost/capy/ex/strand.hppinclude/boost/capy/ex/thread_pool.hppinclude/boost/capy/timeout.hppinclude/boost/capy/when_all.hppinclude/boost/capy/when_any.hppsrc/ex/detail/strand_service.cppsrc/ex/thread_pool.cpp
✅ Files skipped from review due to trivial changes (3)
- include/boost/capy/continuation.hpp
- example/asio/api/uni_stream.hpp
- include/boost/capy/timeout.hpp
🚧 Files skipped from review as they are similar to previous changes (8)
- include/boost/capy/ex/async_event.hpp
- include/boost/capy/ex/run_async.hpp
- include/boost/capy/ex/thread_pool.hpp
- include/boost/capy/ex/strand.hpp
- include/boost/capy/ex/any_executor.hpp
- example/custom-executor/custom_executor.cpp
- include/boost/capy/ex/async_mutex.hpp
- src/ex/detail/strand_service.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (3)
include/boost/capy/ex/run_async.hpp (1)
97-98: Minor:task_h_andtask_cont_.hstore the same handle.Both
task_h_(line 97/205) andtask_cont_.h(set at line 409) hold the same coroutine handle.task_h_remains necessary becauseframe_guardinmake_trampoline(line 293) references it. This dual storage is intentional but creates a subtle coupling—if one were updated without the other, behavior would be inconsistent.Consider adding a brief comment near
task_h_clarifying its continued purpose (forframe_guard) vs.task_cont_(for executor dispatch).Also applies to: 205-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/capy/ex/run_async.hpp` around lines 97 - 98, Add a short clarifying comment near the members task_h_ and task_cont_ explaining why both store the same coroutine handle: state that task_h_ is retained specifically for frame_guard usage inside make_trampoline while task_cont_.h is used for executor/continuation dispatch, so both must be kept and updated together; reference the member names task_h_, task_cont_, the frame_guard and the make_trampoline function in the comment so future maintainers understand the coupling and do not remove or update one without the other.include/boost/capy/ex/async_event.hpp (1)
175-178: Consider resetting moved-from continuation state defensively.
cont_(o.cont_)copieshandnext_as-is. Clearingo.cont_during move makes future refactors safer if move timing ever changes.Suggested defensive tweak
wait_awaiter(wait_awaiter&& o) noexcept : e_(o.e_) - , cont_(o.cont_) + , cont_(std::exchange(o.cont_, {})) , ex_(o.ex_) , claimed_(o.claimed_.load( std::memory_order_relaxed))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/capy/ex/async_event.hpp` around lines 175 - 178, The move constructor wait_awaiter(wait_awaiter&& o) noexcept currently copies cont_ from o via cont_(o.cont_) but leaves the moved-from object's continuation intact; update the move ctor to defensively clear the source continuation (e.g., reset o.cont_ or set its handle/next_ to null) after transferring so the moved-from wait_awaiter is left in a safe empty state; locate the wait_awaiter move constructor and modify the handling of cont_ and o.cont_ accordingly to transfer ownership and clear the source.src/ex/detail/strand_service.cpp (1)
243-248: Consider extracting the invoker-posting logic to reduce duplication.The
ifblocks indispatch(lines 243-248) andpost(lines 257-262) are identical. A small helper could consolidate this:♻️ Optional: Extract helper to reduce duplication
+ static void + post_invoker(strand_impl& impl, executor_ref ex) + { + auto invoker = strand_service_impl::make_invoker(impl); + auto& self = invoker.h_.promise().self_; + self.h = invoker.h_; + ex.post(self); + } + friend class strand_service; };Then in
dispatchandpost:if(strand_service_impl::enqueue(impl, h)) - { - auto invoker = strand_service_impl::make_invoker(impl); - auto& self = invoker.h_.promise().self_; - self.h = invoker.h_; - ex.post(self); - } + strand_service_impl::post_invoker(impl, ex);Also applies to: 257-262
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ex/detail/strand_service.cpp` around lines 243 - 248, Extract the duplicated invoker-posting logic into a small helper function (e.g., make_and_post_invoker or post_invoker) so both dispatch and post call it; the helper should call strand_service_impl::make_invoker(impl), retrieve invoker.h_.promise().self_, set self.h = invoker.h_ and then call ex.post(self). Replace the identical blocks in dispatch and post with a call to this new helper to remove duplication while preserving the invoker, self, and ex.post semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@include/boost/capy/ex/async_event.hpp`:
- Around line 175-178: The move constructor wait_awaiter(wait_awaiter&& o)
noexcept currently copies cont_ from o via cont_(o.cont_) but leaves the
moved-from object's continuation intact; update the move ctor to defensively
clear the source continuation (e.g., reset o.cont_ or set its handle/next_ to
null) after transferring so the moved-from wait_awaiter is left in a safe empty
state; locate the wait_awaiter move constructor and modify the handling of cont_
and o.cont_ accordingly to transfer ownership and clear the source.
In `@include/boost/capy/ex/run_async.hpp`:
- Around line 97-98: Add a short clarifying comment near the members task_h_ and
task_cont_ explaining why both store the same coroutine handle: state that
task_h_ is retained specifically for frame_guard usage inside make_trampoline
while task_cont_.h is used for executor/continuation dispatch, so both must be
kept and updated together; reference the member names task_h_, task_cont_, the
frame_guard and the make_trampoline function in the comment so future
maintainers understand the coupling and do not remove or update one without the
other.
In `@src/ex/detail/strand_service.cpp`:
- Around line 243-248: Extract the duplicated invoker-posting logic into a small
helper function (e.g., make_and_post_invoker or post_invoker) so both dispatch
and post call it; the helper should call
strand_service_impl::make_invoker(impl), retrieve invoker.h_.promise().self_,
set self.h = invoker.h_ and then call ex.post(self). Replace the identical
blocks in dispatch and post with a call to this new helper to remove duplication
while preserving the invoker, self, and ex.post semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 13a97ddb-3170-43cb-ae7b-00fb60d9c13d
⛔ Files ignored due to path filters (19)
bench/bench.cppis excluded by!**/bench/**doc/continuation-rationale.mdis excluded by!**/doc/**doc/modules/ROOT/pages/4.coroutines/4c.executors.adocis excluded by!**/doc/**doc/modules/ROOT/pages/7.examples/7n.custom-executor.adocis excluded by!**/doc/**doc/modules/ROOT/pages/8.design/8k.Executor.adocis excluded by!**/doc/**doc/unlisted/execution-executors.adocis excluded by!**/doc/**doc/unlisted/io-awaitables-executor.adocis excluded by!**/doc/**include/boost/capy/test/run_blocking.hppis excluded by!**/test/**include/boost/capy/test/stream.hppis excluded by!**/test/**src/test/run_blocking.cppis excluded by!**/test/**test/unit/ex/any_executor.cppis excluded by!**/test/**test/unit/ex/executor_ref.cppis excluded by!**/test/**test/unit/ex/frame_cb.cppis excluded by!**/test/**test/unit/ex/run_async.cppis excluded by!**/test/**test/unit/ex/strand.cppis excluded by!**/test/**test/unit/ex/thread_pool.cppis excluded by!**/test/**test/unit/ex/work_guard.cppis excluded by!**/test/**test/unit/task.cppis excluded by!**/test/**test/unit/test_helpers.hppis excluded by!**/test/**
📒 Files selected for processing (21)
example/asio/api/capy_streams.cppexample/asio/api/uni_stream.hppexample/custom-executor/custom_executor.cppinclude/boost/capy/concept/executor.hppinclude/boost/capy/concept/io_awaitable.hppinclude/boost/capy/continuation.hppinclude/boost/capy/delay.hppinclude/boost/capy/ex/any_executor.hppinclude/boost/capy/ex/async_event.hppinclude/boost/capy/ex/async_mutex.hppinclude/boost/capy/ex/executor_ref.hppinclude/boost/capy/ex/io_env.hppinclude/boost/capy/ex/run.hppinclude/boost/capy/ex/run_async.hppinclude/boost/capy/ex/strand.hppinclude/boost/capy/ex/thread_pool.hppinclude/boost/capy/timeout.hppinclude/boost/capy/when_all.hppinclude/boost/capy/when_any.hppsrc/ex/detail/strand_service.cppsrc/ex/thread_pool.cpp
✅ Files skipped from review due to trivial changes (2)
- example/asio/api/uni_stream.hpp
- include/boost/capy/continuation.hpp
🚧 Files skipped from review as they are similar to previous changes (12)
- include/boost/capy/concept/io_awaitable.hpp
- include/boost/capy/ex/run.hpp
- include/boost/capy/ex/async_mutex.hpp
- include/boost/capy/timeout.hpp
- src/ex/thread_pool.cpp
- include/boost/capy/ex/any_executor.hpp
- example/custom-executor/custom_executor.cpp
- include/boost/capy/ex/io_env.hpp
- include/boost/capy/ex/thread_pool.hpp
- include/boost/capy/ex/executor_ref.hpp
- include/boost/capy/ex/strand.hpp
- include/boost/capy/concept/executor.hpp
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #239 +/- ##
===========================================
+ Coverage 92.39% 92.42% +0.03%
===========================================
Files 168 168
Lines 9327 9339 +12
===========================================
+ Hits 8618 8632 +14
+ Misses 709 707 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 18 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
mvandeberg
force-pushed
the
pr/continuation
branch
from
500e87b to
4a8bf7e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
include/boost/capy/ex/run_async.hpp (1)
97-102: Prefer one source of truth for the task handle.
task_h_andtask_cont_.halways refer to the same coroutine, so this duplicates state across both promise specializations and the launch path. If a later edit updates only one of them, cleanup and scheduling diverge. Usingtask_cont_.hfor the trampoline cleanup path would let you drop the extra handle entirely.Refactor sketch
- std::coroutine_handle<> task_h_; continuation task_cont_; ... - } guard{p.task_h_}; + } guard{p.task_cont_.h}; ... - p.task_h_ = task_h; p.task_cont_.h = task_h;Apply the same field removal in both
promise_typespecializations.Also applies to: 208-213, 415-416
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/capy/ex/run_async.hpp` around lines 97 - 102, The code duplicates the coroutine handle via task_h_ and task_cont_.h; remove the extra raw handle and make task_cont_.h the single source of truth by deleting task_h_ from both promise_type specializations and updating make_trampoline, frame_guard cleanup, and the launch path to use task_cont_.h everywhere for cancellation/cleanup and scheduling; ensure any places that read or assign task_h_ now reference task_cont_.h (and update comments) so both promise specializations are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ex/thread_pool.cpp`:
- Around line 95-103: impl::~impl() currently drains abandoned continuations too
late (after thread_pool::shutdown()/destroy()), which allows suspended-frame
destructors (e.g., delay_awaitable::~delay_awaitable() calling
timer_service::cancel() via its cached ts_) to run after services are torn down;
move the queue-drain logic that calls pop() and destroys coroutine handles out
of impl::~impl() and invoke it explicitly from thread_pool::~thread_pool()
before calling shutdown() and destroy(), ensuring you still use the same pop()
loop and h.destroy() behavior but executed prior to shutdown()/destroy() so
suspended frames are torn down while services (timer_service, etc.) are still
valid.
---
Nitpick comments:
In `@include/boost/capy/ex/run_async.hpp`:
- Around line 97-102: The code duplicates the coroutine handle via task_h_ and
task_cont_.h; remove the extra raw handle and make task_cont_.h the single
source of truth by deleting task_h_ from both promise_type specializations and
updating make_trampoline, frame_guard cleanup, and the launch path to use
task_cont_.h everywhere for cancellation/cleanup and scheduling; ensure any
places that read or assign task_h_ now reference task_cont_.h (and update
comments) so both promise specializations are consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 58b8a08d-da94-4f48-8198-0038866d5b23
⛔ Files ignored due to path filters (19)
bench/bench.cppis excluded by!**/bench/**doc/continuation-rationale.mdis excluded by!**/doc/**doc/modules/ROOT/pages/4.coroutines/4c.executors.adocis excluded by!**/doc/**doc/modules/ROOT/pages/7.examples/7n.custom-executor.adocis excluded by!**/doc/**doc/modules/ROOT/pages/8.design/8k.Executor.adocis excluded by!**/doc/**doc/unlisted/execution-executors.adocis excluded by!**/doc/**doc/unlisted/io-awaitables-executor.adocis excluded by!**/doc/**include/boost/capy/test/run_blocking.hppis excluded by!**/test/**include/boost/capy/test/stream.hppis excluded by!**/test/**src/test/run_blocking.cppis excluded by!**/test/**test/unit/ex/any_executor.cppis excluded by!**/test/**test/unit/ex/executor_ref.cppis excluded by!**/test/**test/unit/ex/frame_cb.cppis excluded by!**/test/**test/unit/ex/run_async.cppis excluded by!**/test/**test/unit/ex/strand.cppis excluded by!**/test/**test/unit/ex/thread_pool.cppis excluded by!**/test/**test/unit/ex/work_guard.cppis excluded by!**/test/**test/unit/task.cppis excluded by!**/test/**test/unit/test_helpers.hppis excluded by!**/test/**
📒 Files selected for processing (21)
example/asio/api/capy_streams.cppexample/asio/api/uni_stream.hppexample/custom-executor/custom_executor.cppinclude/boost/capy/concept/executor.hppinclude/boost/capy/concept/io_awaitable.hppinclude/boost/capy/continuation.hppinclude/boost/capy/delay.hppinclude/boost/capy/ex/any_executor.hppinclude/boost/capy/ex/async_event.hppinclude/boost/capy/ex/async_mutex.hppinclude/boost/capy/ex/executor_ref.hppinclude/boost/capy/ex/io_env.hppinclude/boost/capy/ex/run.hppinclude/boost/capy/ex/run_async.hppinclude/boost/capy/ex/strand.hppinclude/boost/capy/ex/thread_pool.hppinclude/boost/capy/timeout.hppinclude/boost/capy/when_all.hppinclude/boost/capy/when_any.hppsrc/ex/detail/strand_service.cppsrc/ex/thread_pool.cpp
✅ Files skipped from review due to trivial changes (3)
- include/boost/capy/continuation.hpp
- include/boost/capy/ex/strand.hpp
- include/boost/capy/concept/io_awaitable.hpp
🚧 Files skipped from review as they are similar to previous changes (6)
- include/boost/capy/ex/async_event.hpp
- example/asio/api/capy_streams.cpp
- include/boost/capy/ex/any_executor.hpp
- include/boost/capy/ex/io_env.hpp
- include/boost/capy/ex/thread_pool.hpp
- include/boost/capy/ex/async_mutex.hpp
The executor concept's dispatch() and post() now accept continuation& instead of std::coroutine_handle<>. A continuation wraps a coroutine handle with an intrusive next_ pointer, enabling executors to queue work without per-post heap allocation. The thread pool's post() previously allocated a work node on every call (new work(h)). It now links the caller's continuation directly into an intrusive queue — zero allocation on the hot path. The continuation lives in the I/O awaitable for caller-handle posting (delay, async_mutex, async_event, stream), and in combinator/trampoline state for parent-dispatch and child-launch patterns (when_all, when_any, timeout, run, run_async). The IoAwaitable concept and io_awaitable_promise_base are unchanged. Breaking change: all Executor implementations must update dispatch() and post() signatures from coroutine_handle<> to continuation&.
mvandeberg
force-pushed
the
pr/continuation
branch
from
4a8bf7e to
81fb33d
Compare
Contributor
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
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 executor concept's dispatch() and post() now accept continuation& instead of std::coroutine_handle<>. A continuation wraps a coroutine handle with an intrusive next_ pointer, enabling executors to queue work without per-post heap allocation.
The thread pool's post() previously allocated a work node on every call (new work(h)). It now links the caller's continuation directly into an intrusive queue — zero allocation on the hot path.
The continuation lives in the I/O awaitable for caller-handle posting (delay, async_mutex, async_event, stream), and in combinator/trampoline state for parent-dispatch and child-launch patterns (when_all, when_any, timeout, run, run_async). The IoAwaitable concept and io_awaitable_promise_base are unchanged.
Breaking change: all Executor implementations must update dispatch() and post() signatures from coroutine_handle<> to continuation&.
Summary by CodeRabbit
New Features
Refactor
Performance
Documentation