Merged
Conversation
Pins the required semantics of capy::run at a cross-executor boundary: the forward trip must enqueue the target task, and the return trip must enqueue the caller's continuation. Covers five scenarios: - run(pe)(inner) from a handler on pe must not let inner cut the queue ahead of other pending work. - When the target runs synchronously, the return trip must still tick the caller's executor, so higher-priority work there runs before the caller resumes. - run(inner)(work) from inside a strand must release the strand while work runs. - A handler that does co_await run(strand)(task) must be outside the strand after the await returns. - An io loop that does co_await run(compute_pool)(task) must resume on the io thread, not on a compute worker. All five fail against the current dispatch-based run() and will pass once run() posts on both trips. Adds a test-only priority_executor support header used by the first three.
📝 WalkthroughWalkthroughRenamed internal coroutine trampoline from Changes
Sequence Diagram(s)sequenceDiagram
participant Inner as Inner coroutine
participant Trampoline as boundary_trampoline
participant Executor as caller_ex_ (executor)
participant Parent as Parent continuation
Inner->>Trampoline: finishes -> final_suspend awaiter
Trampoline->>Executor: caller_ex_.post(parent_)
Executor-->>Parent: invokes parent continuation (enqueued)
Trampoline-->>Inner: returns std::noop_coroutine()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 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://268.capy.prtest3.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-04-23 16:01:40 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/boost/capy/ex/run.hpp (1)
63-69:⚠️ Potential issue | 🟡 MinorRefresh the trampoline docs to match the new
post()boundary behavior.These comments still describe a “dispatch trampoline” that “dispatches” the parent, but the implementation now posts the parent and returns
std::noop_coroutine().Suggested wording update
-/** Minimal coroutine that dispatches through the caller's executor. +/** Minimal coroutine that hops through the caller's executor boundary. Sits between the inner task and the parent when executors diverge. The inner task's `final_suspend` resumes this trampoline via symmetric transfer. The trampoline's own - `final_suspend` dispatches the parent through the caller's - executor to restore the correct execution context. + `final_suspend` posts the parent through the caller's + executor, forcing resumption across the executor boundary. @@ - A dispatch trampoline handles the executor switch on completion: + A boundary trampoline handles the executor switch on completion: the inner task's `final_suspend` resumes the trampoline, which - dispatches back through the caller's executor. + posts back through the caller's executor.Also applies to: 154-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/capy/ex/run.hpp` around lines 63 - 69, Update the doc comment for the trampoline in include/boost/capy/ex/run.hpp (the description around the trampoline used between the inner task and parent, referencing final_suspend) to reflect the new behavior where the trampoline uses executor.post() to schedule the parent rather than synchronously dispatching it and returns std::noop_coroutine() from its final_suspend; replace wording like “dispatches the parent” and “restores the correct execution context” with language that explicitly states the parent is posted to the caller's executor (a post boundary) and that control returns immediately via std::noop_coroutine(), and make the equivalent wording change in the second occurrence (previously lines 154-156).
🤖 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/detail/await_suspend_helper.hpp`:
- Around line 37-40: The comment text in await_suspend_helper.hpp references
executor::dispatch() specifically but boundary_trampoline now uses post(), so
update the workaround note in the await_suspend documentation to avoid tying it
to dispatch; replace the specific phrase "via executor::dispatch()" (and any
direct mention of dispatch) with a generalized description like "via an executor
handoff (e.g., dispatch(), post())" or "via executor handoff operations" and
keep boundary_trampoline and await_suspend mentioned so readers can locate the
context.
---
Outside diff comments:
In `@include/boost/capy/ex/run.hpp`:
- Around line 63-69: Update the doc comment for the trampoline in
include/boost/capy/ex/run.hpp (the description around the trampoline used
between the inner task and parent, referencing final_suspend) to reflect the new
behavior where the trampoline uses executor.post() to schedule the parent rather
than synchronously dispatching it and returns std::noop_coroutine() from its
final_suspend; replace wording like “dispatches the parent” and “restores the
correct execution context” with language that explicitly states the parent is
posted to the caller's executor (a post boundary) and that control returns
immediately via std::noop_coroutine(), and make the equivalent wording change in
the second occurrence (previously lines 154-156).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 76634e50-f3f6-4e0b-93cb-8f2d2454518b
⛔ Files ignored due to path filters (5)
doc/continuation-rationale.mdis excluded by!**/doc/**test/unit/ex/priority_executor.hppis excluded by!**/test/**test/unit/ex/run.cppis excluded by!**/test/**test/unit/ex/run_priority.cppis excluded by!**/test/**test/unit/ex/strand.cppis excluded by!**/test/**
📒 Files selected for processing (2)
include/boost/capy/detail/await_suspend_helper.hppinclude/boost/capy/ex/run.hpp
run_awaitable_ex::await_suspend and the return trampoline's final_suspend both called dispatch(). On executors with a thread-check fast path (strand, blocking_executor) dispatch can fire an inline symmetric transfer, which does not enqueue the target and does not give the caller's executor a fresh tick on the return. run(ex)(task) then fails to actually run task on ex and leaves the caller resuming on the wrong frame. Switch both trips to post + std::noop_coroutine(). Also rename dispatch_trampoline to boundary_trampoline; the type's purpose is bridging the executor boundary, and the old name named a mechanism that no longer applies. The five previously-failing tests in boost.capy.run.priority, boost.capy.ex.run, and boost.capy.strand now pass; full suite green at 76743 assertions.
sgerbino
force-pushed
the
pr/executor-hops
branch
from
b0ca689 to
c0ea43e
Compare
|
GCOVR code coverage report https://268.capy.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-04-23 14:56:04 UTC |
|
GCOVR code coverage report https://268.capy.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-04-23 15:42:01 UTC |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #268 +/- ##
===========================================
- Coverage 92.47% 92.35% -0.12%
===========================================
Files 168 168
Lines 9339 9354 +15
===========================================
+ Hits 8636 8639 +3
- Misses 703 715 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
enqueue was calling impl_->cv.notify_one() outside the lock scope. A foreign thread could still be inside notify_one() when the main thread drained the queue, completed the task, saw signal_done, and destroyed the context. TSan flagged the read against cv during cond_signal after the waiter had released it. Move notify_one inside the lock_guard scope, matching signal_done. The race was latent before run() was switched to post on the return trip: no foreign thread ever called blocking_executor::post, so enqueue only ran on the pumping thread. testHopsBackToIoThread exercises a compute-pool worker posting back to blocking_executor via the return trampoline, which is what revealed the race under TSan.
Codecov's default behavior is to fail CI on any decrease from the base commit's coverage. Small refactors can move the denominator enough to trip this (recently a 0.12% drop) without the change meaningfully reducing test quality. Mark project and patch status as informational so coverage is reported on PRs but never gates merge. Matches corosio's configuration.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/boost/capy/ex/run.hpp (1)
63-69:⚠️ Potential issue | 🟡 MinorUpdate the stale
dispatchwording.These comments still describe a dispatch trampoline, but the implementation now creates a boundary trampoline and uses
post(). Sincedispatch()andpost()have distinct executor semantics, the old wording can mislead maintainers.Proposed comment update
-/** Minimal coroutine that dispatches through the caller's executor. +/* Minimal coroutine that posts through the caller's executor. Sits between the inner task and the parent when executors diverge. The inner task's `final_suspend` resumes this trampoline via symmetric transfer. The trampoline's own - `final_suspend` dispatches the parent through the caller's + `final_suspend` posts the parent through the caller's executor to restore the correct execution context. @@ - A dispatch trampoline handles the executor switch on completion: + A boundary trampoline handles the executor switch on completion: the inner task's `final_suspend` resumes the trampoline, which - dispatches back through the caller's executor. + posts back through the caller's executor.As per coding guidelines, Don't document detail:: or implementation symbols in javadoc.
Also applies to: 154-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/capy/ex/run.hpp` around lines 63 - 69, Update the top-level comment for the trampoline coroutine to reflect that the implementation now creates a boundary trampoline and uses post() semantics (not dispatch()), so replace wording like "dispatches through the caller's executor" with language describing a boundary trampoline that posts the parent back onto the caller's executor via post(); also remove any references to detail:: or other implementation-only symbols from the javadoc. Make the same wording change at the other comment block covering lines around 154-156 so both descriptions accurately state "post()" and boundary-trampoline behavior instead of "dispatch()".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@include/boost/capy/ex/run.hpp`:
- Around line 63-69: Update the top-level comment for the trampoline coroutine
to reflect that the implementation now creates a boundary trampoline and uses
post() semantics (not dispatch()), so replace wording like "dispatches through
the caller's executor" with language describing a boundary trampoline that posts
the parent back onto the caller's executor via post(); also remove any
references to detail:: or other implementation-only symbols from the javadoc.
Make the same wording change at the other comment block covering lines around
154-156 so both descriptions accurately state "post()" and boundary-trampoline
behavior instead of "dispatch()".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 377f34d0-39e7-4283-8449-5186512afa42
⛔ Files ignored due to path filters (2)
doc/continuation-rationale.mdis excluded by!**/doc/**src/test/run_blocking.cppis excluded by!**/test/**
📒 Files selected for processing (3)
.codecov.ymlinclude/boost/capy/detail/await_suspend_helper.hppinclude/boost/capy/ex/run.hpp
✅ Files skipped from review due to trivial changes (2)
- .codecov.yml
- include/boost/capy/detail/await_suspend_helper.hpp
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.
Summary by CodeRabbit
Documentation
Refactor
Chores