Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
f4b0267 to
c9eba52
Compare
15ac3b4 to
da60f82
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/core/receiver.cxx`:
- Around line 167-173: The current stop_source() function returns a reference
into shared receiver state which can dangle if another thread consumes the
receiver via get(); change the API to avoid returning a borrowed cancellation
source: either (preferred) return an owning stop_source by value from
stop_source() (copy or move m_state->stop into the returned object) so the
returned handle keeps the necessary state alive, or expose a new request_stop()
method on the receiver that forwards to m_state->stop.request_stop() without
exposing internal references; update callers to take the owning stop_source or
call request_stop() and remove any code that stores a reference to
m_state->stop.
- Around line 197-200: The code currently checks state->stop.stop_requested()
after wait() and may throw operation_cancelled_error even when a result
completed; to fix, capture the completed snapshot in the producer (root_pkg)
before calling recv->ready.test_and_set(): record the final
state/result/exception and a cancellation flag or set an exception field on
root_pkg, then call ready.test_and_set(); update get() to consume that recorded
snapshot from root_pkg (instead of re-querying state->stop) so a stop requested
after completion does not discard a valid result; ensure this touches the
producer-side path that signals readiness (root_pkg /
recv->ready.test_and_set()) and the consumer get() path that currently throws on
state->stop.stop_requested().
In `@src/core/root.cxx`:
- Around line 51-55: The coroutine allocator rejects frames larger than the
embedded buffer but is missing aligned allocation overloads, so add operator
new(std::size_t, std::align_val_t) and matching operator delete(std::size_t,
std::align_val_t) that check the incoming alignment against k_new_align and
throw or otherwise signal root_alloc_error when alignment > k_new_align (use the
same error type and semantics as the existing scalar operator new used around
recv->buffer and root_alloc_error). Ensure the aligned delete overloads mirror
behavior of the non-aligned delete, and add a unit test that schedules an
over-aligned callable or argument (causing an alignment > k_new_align) to
confirm the allocator rejects it with root_alloc_error.
In `@test/src/cancel.cpp`:
- Around line 413-415: The test uses sc.join() and task<void> but leaves
sentinel updates (count.fetch_add(10000,...)) unchecked; change the stress
helper(s) that perform cancel_source-driven joins to return a bool or an integer
count instead of task<void>, have the helper set/return a value indicating the
sentinel was hit (e.g., true/1 only when count.fetch_add would have been
called), and update callers to await the task result and assert the sentinel was
not hit (assert returned bool is false or returned count is 0). Locate uses
around sc.join(), cancel_source, and count.fetch_add and adjust both the helper
signature and all call sites (including the other instances noted) so the test
fails if the “should not be reached” path executes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b2b56e5-7023-4a8f-ae57-1c8d36568913
📒 Files selected for processing (8)
src/core/concepts/scheduler.cxxsrc/core/promise.cxxsrc/core/receiver.cxxsrc/core/root.cxxsrc/core/schedule.cxxsrc/core/stop.cxxtest/src/cancel.cpptest/src/schedule.cpp
Summary by CodeRabbit
New Features
operation_cancelled_errorexception for cancelled operations.root_alloc_errorexception when coroutine frame exceeds buffer capacity.[[nodiscard]]attribute tostop_source::token()method.Bug Fixes
Tests