Tests #89
Tests #89
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 WalkthroughWalkthroughThe PR changes the trampoline promise_type constructor to take executor and handlers by lvalue reference, modifies io_awaitable_support lifecycle and complete() semantics (cont_ initialization and destructor), and adds tests for teardown and deletion semantics in async tasks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
An automated preview of the documentation is available at https://89.capy.prtest3.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-01-23 03:23:34 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/unit/task.cpp (1)
16-24: Add<utility>forstd::exchange.
tear_down_tusesstd::exchange(Line 1127); relying on transitive includes is brittle.🧩 Proposed include
`#include` <stdexcept> `#include` <string> `#include` <type_traits> +#include <utility> `#include` <vector>
…onfig Configure code rabbit
klemens-morgenstern
force-pushed
the
develop
branch
from
04182b5 to
8ef188d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@include/boost/capy/io_awaitable.hpp`:
- Around line 549-552: The destructor io_awaitable_support() currently calls
cont_.destroy() which can destroy a caller-owned continuation if complete() was
never called; change the ownership semantics so the promise only destroys cont_
when it actually owns it: add an ownership flag (e.g., bool cont_owned_) set to
true when ownership is transferred (or set when complete() takes the handle),
guard cont_.destroy() in the destructor with that flag (or check cont_.address()
!= nullptr/noop before destroying), and add an assertion or log when the
destructor finds a non-noop cont_ but cont_owned_ is false to catch unintended
abandonment; update any code paths that transfer the continuation (methods that
assign cont_) to set/clear cont_owned_ appropriately.
🧹 Nitpick comments (2)
include/boost/capy/io_awaitable.hpp (2)
416-423: The null checkif(!cont_)is ineffective as a guard against the noop sentinel.
std::noop_coroutine()returns a non-null handle, sooperator bool()returnstrue. This means!cont_is alwaysfalsewhencont_holds the noop sentinel.The current logic still works correctly because returning/exchanging a noop_coroutine is harmless, but if the intent is to detect "no continuation was set," consider tracking this with an explicit boolean flag or use a different sentinel approach.
💡 Potential clarification using explicit flag
+ bool has_continuation_{false}; coro cont_{std::noop_coroutine()}; executor_ref caller_ex_; void set_continuation(coro cont, executor_ref caller_ex) noexcept { cont_ = cont; caller_ex_ = caller_ex; + has_continuation_ = true; } coro complete() noexcept { - if(!cont_) + if(!has_continuation_) return std::noop_coroutine(); // ...
20-24: Consider adding a high-level overview section after the includes.Per coding guidelines, a
/* */comment block after the includes explaining how the implementation works would help developers quickly understand the file's purpose and design. The existing docstrings are thorough, but a brief overview of the concepts relationship (IoAwaitable→IoAwaitableTask→IoLaunchableTask) and the role ofio_awaitable_supportCRTP mixin would be valuable.📝 Example overview section
`#include` <type_traits> /* io_awaitable.hpp - I/O Awaitable Protocol * * This header defines the awaitable protocol for coroutines that * participate in scheduler-affinity and cancellation: * * - IoAwaitable: awaitable accepts executor + stop_token in await_suspend * - IoAwaitableTask: task whose promise supports context injection * - IoLaunchableTask: task that can be launched from non-coroutine contexts * * io_awaitable_support<Derived> is a CRTP mixin providing storage for * executor, stop_token, and continuation, plus await_transform hooks * for co_await get_executor() and co_await get_stop_token(). */ namespace boost {Based on coding guidelines: "The top of the file after the includes, put a nice /* */ section which gives a high level overview of how the implementation works."
| ~io_awaitable_support() | ||
| { | ||
| cont_.destroy(); | ||
| } |
There was a problem hiding this comment.
Destructor destroys a continuation handle that this promise may not own.
When complete() was never called (e.g., coroutine abandoned mid-execution), cont_ still holds the caller's continuation handle. Destroying it here destroys a coroutine frame that belongs to the caller, not this promise.
Typical coroutine ownership semantics: the caller owns their coroutine frame; the callee should only resume (or not resume) the continuation, never destroy it.
This aligns with the PR description noting "the second test currently fails" and "uncertainty about whether the observed behavior constitutes undefined behavior."
Recommendations:
- If the intent is cleanup on abandonment, document this ownership transfer clearly and ensure callers understand they're transferring ownership of their continuation.
- If the intent is defensive cleanup, consider only destroying if the continuation handle is valid AND some explicit "owned" flag is set.
- Consider logging or asserting if
cont_is not noop at destruction to catch unintended abandonment.
🛠️ Safer alternative: guard destruction with ownership flag
+ bool owns_continuation_{false};
coro cont_{std::noop_coroutine()};
// ... in set_continuation or a separate method for ownership transfer ...
+ void take_continuation_ownership(coro cont, executor_ref caller_ex) noexcept
+ {
+ cont_ = cont;
+ caller_ex_ = caller_ex;
+ owns_continuation_ = true;
+ }
~io_awaitable_support()
{
- cont_.destroy();
+ if(owns_continuation_ && cont_)
+ cont_.destroy();
}🤖 Prompt for AI Agents
In `@include/boost/capy/io_awaitable.hpp` around lines 549 - 552, The destructor
io_awaitable_support() currently calls cont_.destroy() which can destroy a
caller-owned continuation if complete() was never called; change the ownership
semantics so the promise only destroys cont_ when it actually owns it: add an
ownership flag (e.g., bool cont_owned_) set to true when ownership is
transferred (or set when complete() takes the handle), guard cont_.destroy() in
the destructor with that flag (or check cont_.address() != nullptr/noop before
destroying), and add an assertion or log when the destructor finds a non-noop
cont_ but cont_owned_ is false to catch unintended abandonment; update any code
paths that transfer the continuation (methods that assign cont_) to set/clear
cont_owned_ appropriately.
|
GCOVR code coverage report https://89.capy.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-01-23 03:32:25 UTC |
|
Can you please squash this down and rebase |
4 participants
promise_type constructor should capture by ref, otherwise it'll cause a copy.
The tests check some lifetime issues. The second one is failing, as I would expect the handler to be destroyed.
I don't know if what I'm doing here is supposed to be UB though.
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.