Conversation
There was a problem hiding this comment.
Pull request overview
This PR tightens exception-safety contracts across the Condy async runtime by broadly adding noexcept to core primitives (invokers, finish handles, awaiters, helpers) and by introducing internal traits to propagate nothrow guarantees. It also improves error reporting for mmap failures and updates tests accordingly.
Changes:
- Add
noexceptacross many callback/awaiter/finish-handle APIs and introducecondy/type_traits.hppto drive conditionalnoexcept. - Update channel helper behavior (
force_push) to avoid throwing from completion contexts. - Improve provided-buffer allocation failure errors (use
make_system_error("mmap")) and update tests to usenoexceptlambdas where required.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_op_finish_handle.cpp | Marks test callbacks noexcept to satisfy new nothrow constraints. |
| tests/test_op_awaiter.cpp | Marks multishot callback noexcept to match updated awaiter/handle requirements. |
| tests/test_async_operations.1.cpp | Updates multishot completion callbacks to noexcept. |
| tests/test_async_operations.2.cpp | Updates multishot completion callbacks to noexcept and adjusts formatting. |
| tests/test_async_operations.3.cpp | Updates zero-copy and multishot callbacks to noexcept. |
| tests/test_async_operations.4.cpp | Updates multishot callback to noexcept. |
| include/condy/work_type.hpp | Makes encode/decode helpers noexcept. |
| include/condy/utils.hpp | Adds nothrow annotations, adds make_system_error overload, and removes MaybeMutex. |
| include/condy/type_traits.hpp | New traits for conditional noexcept on suspend/extract operations. |
| include/condy/task.hpp | Adds noexcept to invoker paths and co_spawn/co_switch. |
| include/condy/sync_wait.hpp | Marks default runtime options accessor noexcept. |
| include/condy/singleton.hpp | Marks thread-local singleton accessor noexcept. |
| include/condy/runtime.hpp | Marks current_runtime() noexcept. |
| include/condy/provided_buffers.hpp | Uses make_system_error for mmap failure and adds noexcept to several accessors/handlers. |
| include/condy/invoker.hpp | Switches invoker function pointer type to noexcept and propagates noexcept through invocation. |
| include/condy/helpers.hpp | Makes helper functors noexcept and adjusts stored types for helpers. |
| include/condy/finish_handles.hpp | Propagates noexcept, adds static assertions for nothrow callbacks, and uses new type traits. |
| include/condy/cqe_handler.hpp | Marks CQE handlers’ handle_cqe/extract_result as noexcept. |
| include/condy/coro.inl | Marks coroutine promise invoker noexcept. |
| include/condy/context.hpp | Marks context initialization/reset/accessors noexcept. |
| include/condy/channel.hpp | Makes force_push noexcept with internal panic-on-failure behavior; adds noexcept to various helpers. |
| include/condy/buffers.hpp | Marks buffer wrapper accessors/constructors noexcept. |
| include/condy/awaiters.hpp | Adds conditional noexcept driven by new type traits across awaiter operations. |
Comments suppressed due to low confidence (5)
include/condy/finish_handles.hpp:233
- RangedParallelFinishHandle::init is marked noexcept but performs vector resize operations (child_invokers_.resize / order_.resize) which can throw std::bad_alloc. Either remove noexcept or make the no-throw guarantee real (e.g., use preallocated storage / fallible allocation strategy).
void init(std::vector<Handle *> handles) noexcept {
handles_ = std::move(handles);
child_invokers_.resize(handles_.size());
for (size_t i = 0; i < handles_.size(); i++) {
auto *handle = handles_[i];
auto &invoker = child_invokers_[i];
include/condy/finish_handles.hpp:254
- RangedParallelFinishHandle::extract_result is marked noexcept based only on child extract_result(), but it allocates and fills std::vector, which can throw (reserve/push_back). The noexcept condition should account for these allocations, or just drop noexcept here.
ReturnType extract_result() noexcept(is_nothrow_extract_result_v<Handle>) {
std::vector<ChildReturnType> result;
result.reserve(handles_.size());
for (size_t i = 0; i < handles_.size(); i++) {
result.push_back(handles_[i]->extract_result());
include/condy/awaiters.hpp:180
- RangedParallelAwaiterBase::init_finish_handle is marked noexcept but allocates (std::vector handles + reserve/push_back). This can throw, so the noexcept spec is incorrect; consider removing it or making it conditional on allocation-free behavior.
void init_finish_handle() noexcept {
using ChildHandle = typename Awaiter::HandleType;
std::vector<ChildHandle *> handles;
handles.reserve(awaiters_.size());
for (auto &awaiter : awaiters_) {
include/condy/provided_buffers.hpp:344
- BundledProvidedBufferPool::handle_finish is marked noexcept but it constructs and grows a std::vector (emplace_back in a loop), which can throw on allocation. The noexcept spec is incorrect unless you switch to an allocation-free result representation.
ReturnType handle_finish(int32_t res, uint32_t flags) noexcept {
std::vector<ProvidedBuffer> buffers;
if (!(flags & IORING_CQE_F_BUFFER)) {
return buffers;
}
include/condy/provided_buffers.hpp:455
- ProvidedBufferPool::handle_finish is marked noexcept but it depends on BundledProvidedBufferPool::handle_finish (which allocates a std::vector) and can therefore throw. Consider dropping noexcept here (or making the underlying implementation allocation-free).
ReturnType handle_finish(int32_t res, uint32_t flags) noexcept {
auto buffers = BundledProvidedBufferPool::handle_finish(res, flags);
if (buffers.empty()) {
return ReturnType();
}
assert(buffers.size() == 1);
return std::move(buffers[0]);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 9 comments.
Comments suppressed due to low confidence (3)
tests/test_async_operations.2.cpp:333
- This multishot accept completion callback is now
noexceptbut still uses doctestREQUIRE(...). A failingREQUIREmay throw / abort in a way that will terminate the process due tonoexcept. PreferCHECKinside the callback and assert results outside, or avoid assertions directly inside thenoexceptcallback.
[&](int conn_fd) noexcept {
REQUIRE(conn_fd >= 0);
count++;
if (count == 4) {
REQUIRE(done_channel.try_push(std::monostate{}));
}
tests/test_async_operations.3.cpp:736
- This multishot recv callback is now
noexceptbut uses several doctestREQUIRE(...)assertions. If any assertion fails, doctest may throw/abort and will triggerstd::terminatebecause ofnoexcept, reducing test diagnostics. Consider usingCHECKinside the callback and verifying accumulated results outside the callback.
auto [n, buf] = co_await condy::async_recv_multishot(
sv[0], pool, 0, [&](auto res) noexcept {
auto &[n, buf] = res;
REQUIRE(n == 256);
actual.append(static_cast<char *>(buf.data()), n);
count++;
tests/test_async_operations.3.cpp:752
- This second multishot recv callback is also
noexceptwhile using doctestREQUIRE(...). A failingREQUIREcan lead tostd::terminatefrom thenoexceptboundary. PreferCHECKinside the callback and assert outside, or capture failures for later verification.
auto [n2, buf2] = co_await condy::async_recv_multishot(
sv[0], pool, 0, [&](auto res) noexcept {
auto &[n, buf] = res;
REQUIRE(n == 256);
actual.append(static_cast<char *>(buf.data()), n);
count++;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
No description provided.