Conversation
Extract reactor_basic_socket CRTP base and reactor_socket_service template to deduplicate ~1000 lines across TCP and UDP socket/service implementations. Rename *_socket to *_stream_socket and *_socket_service to *_stream_service for symmetry with *_datagram_* counterparts. New public API: udp_socket, native_udp_socket, udp protocol type, native_udp protocol type, broadcast socket option. Add comprehensive UDP tests covering loopback I/O, cancellation, close-with-pending-ops, stop token cancellation, concurrent send+recv, and empty buffer edges. Add datagram_socket_type/datagram_service_type to backend tags. Run clang-format across the codebase.
Align IOCP file and class naming with epoll/kqueue/select backends using protocol-specific names (win_tcp_socket, win_udp_socket, win_tcp_acceptor, win_tcp_service, win_udp_service). Add iocp_t UDP type aliases in backend.hpp and IOCP include in native_udp_socket. Add native UDP socket test suite for IOCP. Remove comment dividers from IOCP headers.
Add ipv6_address::is_multicast(), native and type-erased multicast option types (join/leave group v4/v6, loop, hops, interface), and multicast round-trip tests for UDP sockets.
UDP sockets can now set a default peer via connect(), then use send()/recv() without endpoint arguments. Separate op types avoid dead address storage and match distinct POSIX/Windows syscalls. All four backends (epoll, kqueue, select, IOCP) implemented.
Skip recv_from when send_to fails with EHOSTUNREACH due to missing multicast routes, preventing an indefinite block.
📝 WalkthroughWalkthroughThis PR adds UDP protocol/socket support and refactors reactor/backends to split TCP/UDP concerns: introduces reactor_basic/stream/datagram sockets and services, renames backend types to tcp_/udp_ variants across epoll/select/kqueue/iocp, and expands socket options (multicast/broadcast) plus native_handle handling. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application Coroutine
participant UDP_Sock as udp_socket
participant UDP_Service as detail::udp_service
participant Scheduler as Backend Scheduler
participant OS as Kernel Socket
App->>UDP_Sock: await send_to(buffers, dest)
UDP_Sock->>UDP_Service: request open/send (implementation)
UDP_Service->>Scheduler: register/send op
Scheduler->>OS: submit send (sendmsg/WSASendTo)
OS-->>Scheduler: completion (bytes/error)
Scheduler->>UDP_Service: on_completion
UDP_Service->>App: resume coroutine (result)
sequenceDiagram
participant App as Application Coroutine
participant UDP_Sock as udp_socket
participant UDP_Service as detail::udp_service
participant Scheduler as Backend Scheduler
participant OS as Kernel Socket
App->>UDP_Sock: await recv_from(buffers)
UDP_Sock->>UDP_Service: request recv registration
UDP_Service->>Scheduler: watch descriptor / register
OS-->>Scheduler: data-ready event
Scheduler->>UDP_Service: invoke recv (recvmsg/WSARecvFrom)
UDP_Service->>App: resume coroutine with bytes + source
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
An automated preview of the documentation is available at https://208.corosio.prtest3.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-03-17 01:06:59 UTC |
|
GCOVR code coverage report https://208.corosio.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-03-17 01:17:08 UTC |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #208 +/- ##
===========================================
- Coverage 77.85% 77.76% -0.09%
===========================================
Files 99 96 -3
Lines 7499 7457 -42
Branches 1846 1836 -10
===========================================
- Hits 5838 5799 -39
Misses 1118 1118
+ Partials 543 540 -3
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
include/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor_service.hpp (1)
159-170:⚠️ Potential issue | 🔴 CriticalGuard
SO_NOSIGPIPEat line 163 to match the pattern at line 356.Line 163 uses
SO_NOSIGPIPEunconditionally in the accepted-socket path, while the same file guards it with#ifdef SO_NOSIGPIPEat line 356 in the listening-socket initialization. The unguarded usage fails to compile on systems where the macro is undefined.Suggested fix
// SO_NOSIGPIPE before budget check so both inline and // queued paths have it applied (macOS lacks MSG_NOSIGNAL) +#ifdef SO_NOSIGPIPE int one = 1; if (::setsockopt( accepted, SOL_SOCKET, SO_NOSIGPIPE, &one, sizeof(one)) == -1) { int errn = errno; ::close(accepted); op.complete(errn, 0); op.impl_ptr = shared_from_this(); svc_.post(&op); return std::noop_coroutine(); } +#endif🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor_service.hpp` around lines 159 - 170, The unguarded use of SO_NOSIGPIPE when setting socket option on the newly accepted socket (the setsockopt call that uses variable accepted) will fail on platforms where SO_NOSIGPIPE is not defined; wrap that setsockopt/errno/close/op.complete/op.impl_ptr/svc_.post return block in an `#ifdef` SO_NOSIGPIPE / `#endif` to match the guarded pattern used for the listening socket initialization elsewhere in kqueue_tcp_acceptor_service.hpp so the code compiles on platforms without SO_NOSIGPIPE.include/boost/corosio/native/detail/iocp/win_tcp_acceptor_service.hpp (1)
386-460:⚠️ Potential issue | 🟠 MajorAdd post-submit cancellation re-checks to
connect()andaccept()to matchread_some()andwrite_some().The stop callback can fire between
on_pending()and the function return. Without a re-check, if the stop token is activated during this window, the coroutine remains suspended until the kernel completes the ConnectEx or AcceptEx operation. Bothread_some()andwrite_some()already perform this check; the same pattern is needed here.Suggested fix for connect() (line 460)
svc_.on_pending(&op); + if (op.cancelled.load(std::memory_order_acquire)) + ::CancelIoEx(reinterpret_cast<HANDLE>(socket_), &op); return std::noop_coroutine();Suggested fix for accept() (line 1177)
svc_.on_pending(&op); + if (op.cancelled.load(std::memory_order_acquire)) + ::CancelIoEx(reinterpret_cast<HANDLE>(socket_), &op); return std::noop_coroutine();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/native/detail/iocp/win_tcp_acceptor_service.hpp` around lines 386 - 460, Add a post-submit cancellation re-check in both connect() and accept(): after calling svc_.on_pending(&op) and before returning std::noop_coroutine(), test the stop token (the parameter named token) and if it has been requested call svc_.on_completion(&op, ERROR_OPERATION_ABORTED, 0) (or the same cancellation error used elsewhere) and return std::noop_coroutine(); this mirrors the pattern used in read_some()/write_some() and ensures the coroutine is completed if cancellation fires between on_pending() and the return.
🧹 Nitpick comments (11)
include/boost/corosio/native/detail/reactor/reactor_socket_service.hpp (1)
23-34: Add the required file-level overview comment.The class doc explains the template parameters, but this new header also owns the non-obvious lifetime rules (
impl_list_,impl_ptrs_, shutdown ordering). A short/* ... */overview after the includes would make that maintenance contract much easier to follow.
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."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/native/detail/reactor/reactor_socket_service.hpp` around lines 23 - 34, Add a file-level /* ... */ overview comment immediately after the includes in reactor_socket_service.hpp that succinctly documents the lifetime and shutdown contract for this header: explain the relationship between impl_list_ and impl_ptrs_, the ownership model, the required shutdown ordering, and any invariants enforced by the CRTP ReactorSocketService (the class template defined in this file) so maintainers can understand how construct/destroy/shutdown/close/post/work interact; place the comment near the top of the file (after includes and before the class/template declarations) and reference the key symbols impl_list_, impl_ptrs_, and the ReactorSocketService CRTP to make the contract explicit.include/boost/corosio/native/detail/reactor/reactor_datagram_socket.hpp (1)
13-23: Add the required file-level implementation overview block.This new header is the core UDP speculative-I/O / reactor-registration layer, but it jumps straight from the includes into declarations. Please add the repo-required
/* */maintainer overview here so the fast-path, registration, and cancellation flow is discoverable.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."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/native/detail/reactor/reactor_datagram_socket.hpp` around lines 13 - 23, Add a file-level C-style block comment immediately after the includes in reactor_datagram_socket.hpp that gives a concise high-level overview of the header's implementation: describe the speculative I/O fast-path, when/why it falls back to reactor registration, how registration with ReactorBasicSocket and the reactor loop is performed, the cancellation flow (how in-flight ops are cancelled and cleaned up), coroutine dispatch interaction (dispatch_coro), and any lifetime/thread-safety constraints for ReactorDatagramSocket and related buffers (capy::buffers). Keep it brief but cover fast-path vs registration, registration lifecycle, cancellation semantics, and where callers can find entry points in the file.src/corosio/src/udp.cpp (1)
15-19: Consider caching or avoiding temporary object creation infamily().The current implementation creates a temporary
native_udpobject on each call tofamily(). While correct, this could be simplified ifnative_udpexposes a staticfamily(bool v6)or if the family value is cached.That said, this is a minor optimization concern and the current approach is functionally correct.
💡 Optional simplification
If
native_udpstores family internally and has a static accessor:int udp::family() const noexcept { - return native_udp(v6_ ? native_udp::v6() : native_udp::v4()).family(); + return v6_ ? AF_INET6 : AF_INET; // If constants are available portably }Otherwise, keep as-is since it's correct and clear.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/corosio/src/udp.cpp` around lines 15 - 19, The udp::family() implementation currently constructs a temporary native_udp each call; to avoid repeated temporaries either (A) add a static/native helper like native_udp::family(bool v6) and change udp::family() to return native_udp::family(v6_), or (B) store and return a cached int member (e.g., cached_family_) computed once when udp is constructed or when v6_ changes; update the udp constructor or the v6_ setter to initialize cached_family_ using native_udp(v6_ ? native_udp::v6() : native_udp::v4()).family(), and have udp::family() simply return cached_family_; references: udp::family(), native_udp, v6_.src/corosio/src/io_context.cpp (1)
11-43: Add a high-level implementation overview block comment after includes.This
.cppnow contains non-trivial backend wiring logic; a short/* ... */overview of scheduler/service construction invariants would help maintainability.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.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/corosio/src/io_context.cpp` around lines 11 - 43, Add a /* ... */ block comment immediately after the includes describing the high-level implementation: explain that io_context wires platform-specific schedulers (epoll_scheduler, select_scheduler, kqueue_scheduler, win_scheduler) and their corresponding TCP/UDP/acceptor services (e.g., epoll_tcp_service, epoll_udp_service, select_tcp_acceptor_service, win_tcp_acceptor_service) based on BOOST_COROSIO_HAS_* macros, outline the invariants for scheduler/service construction (single scheduler per io_context, services must be created/registered with that scheduler before use), and note threading/model expectations (how threads interact with the scheduler and lifetime ordering for create/destroy) so future maintainers can understand the backend wiring without diving into implementation details.include/boost/corosio/native/native_tcp_socket.hpp (1)
30-30: Directly includewin_tcp_service.hppto make the dependency explicit.At line 30 this header includes
win_tcp_acceptor_service.hppand relies on transitive inclusion ofwin_tcp_service.hppto satisfy the type requirements at line 75 (service_type = typename backend_type::tcp_service_type). Add a direct include forwin_tcp_service.hppto follow the "include what you use" principle and reduce include-order fragility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/native/native_tcp_socket.hpp` at line 30, The header relies on types from win_tcp_service.hpp (used for service_type = typename backend_type::tcp_service_type) but only includes win_tcp_acceptor_service.hpp transitively; add a direct `#include` for boost/corosio/native/detail/iocp/win_tcp_service.hpp at the top alongside the existing win_tcp_acceptor_service.hpp include so the dependency is explicit and include-order fragile is avoided.include/boost/corosio/native/native_udp.hpp (2)
79-82: Consider makingfamily()constexpr.The
family()method could beconstexprsince it only uses thev6_member and the ternary operator, both of which are constexpr-compatible. This would align with the other accessors and enable compile-time evaluation.♻️ Optional: make family() constexpr
/// Return the address family (AF_INET or AF_INET6). - int family() const noexcept + constexpr int family() const noexcept { return v6_ ? AF_INET6 : AF_INET; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/native/native_udp.hpp` around lines 79 - 82, Make the family() accessor constexpr so it can be evaluated at compile time: change its declaration to a constexpr method while keeping const and noexcept (i.e. constexpr int family() const noexcept) and leave the implementation using v6_ ? AF_INET6 : AF_INET unchanged; ensure the class and v6_ member remain compatible with constexpr methods.
38-42: Redundant namespace blocks.The forward declaration of
udp_socket(lines 38-42) and thenative_udpclass definition (lines 44-110) are in separate namespace blocks that could be consolidated.♻️ Optional: consolidate namespace blocks
-namespace boost::corosio { - -class udp_socket; - -} // namespace boost::corosio - namespace boost::corosio { +class udp_socket; + /** Inline UDP protocol type with platform constants.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/native/native_udp.hpp` around lines 38 - 42, The file has two separate namespace blocks for boost::corosio which is redundant; consolidate them by moving the forward declaration of udp_socket into the same namespace block that contains the native_udp class so both udp_socket and native_udp are declared inside a single namespace boost::corosio block; update any references in that block accordingly and remove the now-empty extra namespace declaration to keep declarations together and avoid fragmentation.include/boost/corosio/native/detail/reactor/reactor_stream_socket.hpp (1)
13-24: Add the missing file-level overview comment.This file now owns the speculative connect/read/write flow, inline-budget completion, and descriptor-slot/cancel mapping, but there is no
/* */maintainer overview after the includes. A short high-level comment here would make the close/cancel invariants much easier to follow.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."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/native/detail/reactor/reactor_stream_socket.hpp` around lines 13 - 24, Add a file-level /* ... */ block comment immediately after the includes in reactor_stream_socket.hpp that summarizes the high-level design: describe that this file implements the speculative connect/read/write flow, how inline-budget completion is handled, the descriptor-slot <-> cancel mapping, and the close/cancel invariants (who owns descriptor state, when cancel slots are allocated/freed, and how completion vs cancellation races are resolved). Reference the key concepts used in this file (speculative connect/read/write, inline-budget completion, descriptor-slot/cancel mapping, close/cancel invariants, and any top-level classes/types such as reactor_stream_socket) so maintainers can quickly understand control flow and lifetime guarantees.include/boost/corosio/native/detail/epoll/epoll_tcp_acceptor_service.hpp (1)
17-37: Add a file-level maintainer overview block.This header now contains the accept fast-path, shutdown behavior, and peer-socket wiring, but it still lacks the
/* */overview comment that would explain those invariants.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."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/native/detail/epoll/epoll_tcp_acceptor_service.hpp` around lines 17 - 37, Add a file-level C-style block comment immediately after the includes that summarizes the responsibilities and invariants of this header: describe the accept fast-path implemented by epoll_tcp_acceptor/epoll_tcp_acceptor_service, how shutdown sequencing and resource cleanup are handled (including interaction with epoll_scheduler and reactor_service_state), and how peer-socket wiring is performed with epoll_tcp_service and reactor_op_complete; keep it concise (3–6 sentences) and mention any important concurrency/ordering guarantees and error-handling expectations so future maintainers understand the high-level design.include/boost/corosio/native/detail/iocp/win_tcp_acceptor_service.hpp (1)
18-33: Add a file-level maintainer overview block.This header now owns most of the IOCP TCP lifetime, shutdown, and completion wiring, but it has no
/* */overview after the includes explaining those invariants.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."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/native/detail/iocp/win_tcp_acceptor_service.hpp` around lines 18 - 33, Add a file-level /* */ maintainer overview block immediately after the includes that explains the IOCP TCP lifecycle and key invariants for this header: describe how win_tcp_acceptor_service coordinates accept/start/shutdown, how win_tcp_acceptor and win_tcp_service interact for socket lifetime, how completions are dispatched via win_scheduler and win_completion_key, and any ordering/ownership guarantees (who owns sockets, when shutdown is safe, and how errors are propagated using make_err/dispatch_coro). Make the comment concise but cover lifecycle, shutdown sequencing, completion-callback threading model, and references to the key types/functions (win_tcp_acceptor_service, win_tcp_acceptor, win_tcp_service, win_scheduler, win_completion_key, endpoint_convert, make_err, dispatch_coro) so future maintainers can understand invariants without reading all implementation details.include/boost/corosio/native/detail/iocp/win_udp_service.hpp (1)
17-33: Add a file-level maintainer overview block.This new header now carries most of the Windows UDP service, socket, and completion logic, but it has no
/* */overview after the includes describing the ownership and cancellation model.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."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/native/detail/iocp/win_udp_service.hpp` around lines 17 - 33, Add a file-level C-style block comment immediately after the includes in win_udp_service.hpp that provides a concise maintainer overview describing ownership of key components (win_udp_service, win_udp_socket, win_scheduler, win_completion_key, win_mutex, win_wsa_init), the lifetime and ownership rules for sockets and completion keys, how asynchronous operations are queued and completed, and the cancellation/cleanup model (how cancellation requests are propagated and resources released). Keep it short but explicit about who owns buffers, which object is responsible for posting/completing IO, and how errors/cancellations are handled so future maintainers can reason about concurrency and lifetime.
🤖 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/corosio/backend.hpp`:
- Around line 48-54: The new backend header replaced generic aliases
(socket_type, service_type, acceptor_type) with TCP-specific names causing a
source break; restore the legacy compatibility aliases alongside the new
UDP-specific ones. Update each backend typedef block (e.g., scheduler_type,
tcp_socket_type, tcp_service_type, udp_socket_type, udp_service_type,
tcp_acceptor_type, tcp_acceptor_service_type) to also define the legacy aliases
socket_type -> tcp_socket_type, service_type -> tcp_service_type, and
acceptor_type -> tcp_acceptor_type (for select_t, kqueue_t, iocp_t and the other
backend blocks noted) so existing code continues to compile; leave a comment or
mark them for later deprecation but do not remove the legacy names now.
In `@include/boost/corosio/detail/native_handle.hpp`:
- Line 20: Update the doc comment in
include/boost/corosio/detail/native_handle.hpp that currently mentions "SOCKET"
to use platform-neutral wording such as "native OS socket handle" or
"platform-specific socket handle (e.g., int on POSIX)" so the detail header
avoids platform-specific symbols; modify the comment text near the
native_handle.hpp declaration to remove "SOCKET" and replace with the neutral
phrase.
In `@include/boost/corosio/detail/udp_service.hpp`:
- Around line 43-45: The documentation in detail::udp_service.hpp refers to
platform-specific macros; update the parameter descriptions for family, type,
and protocol to platform-neutral wording: change "Address family (e.g.
`AF_INET`, `AF_INET6`)" to something like "Address family (e.g. IPv4 or IPv6)",
change "Socket type (`SOCK_DGRAM`)" to "Socket type (datagram)" and change
"Protocol number (`IPPROTO_UDP`)" to "Protocol number (UDP)"; ensure you only
edit the docstring text around the parameters named family, type, and protocol
and do not introduce any platform macros or headers.
In `@include/boost/corosio/native/detail/epoll/epoll_tcp_acceptor_service.hpp`:
- Around line 149-154: Wrap the raw accepted file descriptor in an RAII guard
before calling socket_svc->construct() so the FD is closed if construct()
throws; then call socket_svc->construct() to create the epoll_tcp_socket wrapper
and only transfer the FD by calling impl.set_socket(accepted) after construction
succeeds (or release the RAII guard when transferring ownership). Update code
around epoll_tcp_acceptor_service where socket_svc->construct(),
epoll_tcp_socket, and set_socket are used to ensure no leak of the accepted FD
on exceptions.
In `@include/boost/corosio/native/detail/iocp/win_udp_service.hpp`:
- Around line 194-200: The code currently only assigns *op->source_out when
op->bytes_transferred > 0, which drops the peer address for zero-byte UDP
datagrams; change the check so that whenever the operation succeeded (dwError ==
0 and not cancelled) and op->source_out is non-null you populate the endpoint
from op->source_storage via from_sockaddr regardless of bytes_transferred (i.e.,
remove the bytes_transferred > 0 requirement in the conditional that assigns
*op->source_out).
- Around line 505-527: After calling op.start(token) (which registers a stop
callback that may synchronously set op.cancelled), check for pre-cancellation
before invoking ::WSAConnect; if the token is already cancelled or op.cancelled
is true, do not call ::WSAConnect and instead complete the operation via
svc_.on_completion(&op, ERROR_OPERATION_ABORTED, 0) (or the appropriate Windows
cancellation error) and return early so the socket is not connected for a
cancelled operation.
- Around line 385-389: The UDP send code is incorrectly short-circuiting
zero-length datagrams by checking both op.wsabuf_count and bufs[0].size(), which
prevents valid empty UDP payloads from being sent; update the send_to and send
handlers (where op.wsabuf_count and svc_.on_completion(&op, ...) are used) to
only treat op.wsabuf_count == 0 as the immediate-complete case and remove the
bufs[0].size() check/empty-buffer logic; apply the same change in the reactor
backend implementations to ensure consistency with TCP write behavior.
In `@include/boost/corosio/native/detail/kqueue/kqueue_udp_service.hpp`:
- Around line 267-274: The unconditional ::setsockopt call using SO_NOSIGPIPE
should be guarded and treated as non-fatal: wrap the call to ::setsockopt(fd,
SOL_SOCKET, SO_NOSIGPIPE, &one, sizeof(one)) in an `#ifdef` SO_NOSIGPIPE block
(matching the select backend), and if setsockopt fails do not close fd or return
make_err; instead ignore the failure (or optionally log it) and continue normal
execution. Update the block around ::setsockopt, SO_NOSIGPIPE, ::close, and
make_err so platforms without SO_NOSIGPIPE (e.g., OpenBSD/NetBSD) won’t hit an
unconditional failure path.
In `@include/boost/corosio/native/detail/reactor/reactor_basic_socket.hpp`:
- Around line 209-230: The code consumes ready_flag and calls op.perform_io()
before checking cancel_flag/op.cancelled, which allows a race where a canceled
operation still performs I/O; to fix, while holding desc_state_.mutex in
register_op(), check cancel_flag and op.cancelled first (clear cancel_flag and
set/acknowledge op.cancelled as currently done) and if canceled skip consuming
ready_flag and skip calling op.perform_io(); only if not canceled consume
ready_flag, call op.perform_io(), compute io_done, and finally post via
svc_.post(&op) / svc_.work_finished() when io_done or op.cancelled is
observed—ensuring cancel handling precedes any perform_io() path.
In `@include/boost/corosio/native/detail/reactor/reactor_datagram_socket.hpp`:
- Around line 432-438: The inline fast-path currently only sets *source when
(source && !err && n > 0), which drops peer endpoints for zero-length datagrams;
update that condition in the block containing
svc_.scheduler().try_consume_inline_budget() so the source is assigned when the
receive succeeded even with 0 bytes — e.g. use (source && !err && (n >= 0 ||
msg.msg_namelen > 0)) or check msg.msg_namelen if available, and call *source =
from_sockaddr(op.source_storage) in that case before returning dispatch_coro(ex,
h).
In `@include/boost/corosio/native/detail/reactor/reactor_descriptor_state.hpp`:
- Line 144: In reactor_descriptor_state::invoke_deferred_io(), preserve
enqueue/close ordering by moving the ready_events_.exchange(0,
std::memory_order_acquire) out of the mutex: under the lock set impl_ref_ and
clear is_enqueued_, then release the lock and only after unlocking call
ready_events_.exchange(...). Ensure no other state mutations that rely on the
previous ordering remain after the lock; leave the exchange as the final step
outside the mutex.
In `@include/boost/corosio/native/detail/reactor/reactor_socket_service.hpp`:
- Around line 51-66: The race occurs because destroy() publishes the impl in
impl_list_/impl_ptrs_ and then calls pre_destroy()/close_socket() without
holding state_->mutex_, allowing shutdown() to iterate impl_list_ and tear down
the same impl; fix by changing destroy() so the transition to "published"
(insertion into impl_list_/impl_ptrs_) is done under state_->mutex_ and obtain a
local shared_ptr (keep a copy from impl_ptrs_ or the insertion result) before
releasing the mutex, then call pre_destroy() and close_socket() while holding
that local shared_ptr so the impl cannot be unlinked under you; apply the same
pattern to the analogous code referenced at lines 82-90.
- Around line 18-20: Add an explicit include for <utility> in this header
because std::move is used (e.g., where std::move appears in
reactor_socket_service.hpp); replace relying on transitive includes by adding
`#include` <utility> near the other standard includes so the use of std::move is
guaranteed to compile across standard library implementations.
In `@include/boost/corosio/native/native_udp_socket.hpp`:
- Around line 36-72: Update the documentation for native_udp_socket's public
async wrappers (the devirtualized send_to and recv_from overloads) to fully
mirror the async contract found on udp_socket: explicitly state buffer
ownership/lifetime rules for CB/MB parameters, describe completion conditions
and return values, document cancellation behavior and how cancellations are
reported, specify concurrency/overlap guarantees (what concurrent calls are
allowed/forbidden), list the CB/MB concept requirements, and include a short
usage example; apply the same full-doc updates to the other native
send_to/recv_from overloads in this header (the adjacent wrappers referenced in
the file) so all public async wrappers have complete contract docs.
- Around line 105-124: Latch the stop_token state at suspension and consult that
latched value at resume to avoid a race that can overwrite a successful I/O with
operation_canceled: in native_send_to_awaitable and native_recv_from_awaitable,
capture env->stop_token.requested() (or equivalent bool) into a member (e.g.,
latched_requested_) inside await_suspend() after assigning token_, and change
await_ready()/await_resume() to consult that latched flag instead of polling the
live token_; keep the existing token_ assignment for use by the underlying
send_to/recv_from call. Also update the public send_to() and recv_from() wrapper
documentation (the overloads that bypass virtual dispatch) to explicitly state
completion conditions (when the coroutine resumes), cancellation semantics via
stop_token (including that cancellation may produce capy::cond::canceled or
std::errc::operation_canceled), precise error-code behavior, buffer
ownership/lifetime requirements, and template parameter constraints so users
know what guarantees send_to() and recv_from() provide.
In `@include/boost/corosio/udp_socket.hpp`:
- Around line 37-85: Expand the public API docs for class udp_socket and its
async operations (recv_from, send_to, send, connect, and any templated buffer
overloads) to explicitly state: buffer ownership and lifetime requirements (who
must keep buffers alive until the awaitable completes), the completion
conditions and what constitutes success vs error (structure and semantics of
io_result including error_code and byte_count), concurrency/overlap rules (which
operations may run concurrently and which must not), cancellation behavior and
how stop_token/cancel() affect in-flight operations, and the concept/type
requirements for templated buffer parameters; also audit and update other
documented ranges noted (lines ~89-235, 495-697) to match this structure.
Finally, replace the hard-coded POSIX sentinel in native_handle() (-1) with a
platform-neutral sentinel (use INVALID_SOCKET on Windows or
std::numeric_limits<socket_type>::min()/an appropriately named constant) or
document the platform-specific semantics in the native_handle() docstring.
- Around line 259-277: The awaitables use a live std::stop_token in
await_ready()/await_resume(), causing a race where token_ is uninitialized in
await_ready() and stop_requested() re-reads shared state in await_resume() and
can override backend results; fix by changing await_ready() to always return
false (remove token_.stop_requested() there) and remove the live-token check
from await_resume() so it returns the backend-populated ec_ (and bytes_ where
applicable) directly; apply this change to send_to_awaitable,
recv_from_awaitable, connect_awaitable, send_awaitable and recv_awaitable
(adjust await_ready(), await_resume(), and keep await_suspend() assigning token_
and calling the backend as-is, relying on ec_/bytes_ set by the backend).
In `@include/boost/corosio/udp.hpp`:
- Around line 63-70: The comments for family(), type(), and protocol() currently
reference platform-specific macro names; update their documentation to be
platform-neutral by describing the semantics only: document family() as
returning the address family indicating whether the endpoint is IPv4 or IPv6,
type() as returning the socket type used for datagram sockets, and protocol() as
returning the IP protocol number corresponding to UDP; do not mention any
OS-specific macros like AF_INET, SOCK_DGRAM, or IPPROTO_UDP and leave the
function signatures unchanged.
---
Outside diff comments:
In `@include/boost/corosio/native/detail/iocp/win_tcp_acceptor_service.hpp`:
- Around line 386-460: Add a post-submit cancellation re-check in both connect()
and accept(): after calling svc_.on_pending(&op) and before returning
std::noop_coroutine(), test the stop token (the parameter named token) and if it
has been requested call svc_.on_completion(&op, ERROR_OPERATION_ABORTED, 0) (or
the same cancellation error used elsewhere) and return std::noop_coroutine();
this mirrors the pattern used in read_some()/write_some() and ensures the
coroutine is completed if cancellation fires between on_pending() and the
return.
In `@include/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor_service.hpp`:
- Around line 159-170: The unguarded use of SO_NOSIGPIPE when setting socket
option on the newly accepted socket (the setsockopt call that uses variable
accepted) will fail on platforms where SO_NOSIGPIPE is not defined; wrap that
setsockopt/errno/close/op.complete/op.impl_ptr/svc_.post return block in an
`#ifdef` SO_NOSIGPIPE / `#endif` to match the guarded pattern used for the listening
socket initialization elsewhere in kqueue_tcp_acceptor_service.hpp so the code
compiles on platforms without SO_NOSIGPIPE.
---
Nitpick comments:
In `@include/boost/corosio/native/detail/epoll/epoll_tcp_acceptor_service.hpp`:
- Around line 17-37: Add a file-level C-style block comment immediately after
the includes that summarizes the responsibilities and invariants of this header:
describe the accept fast-path implemented by
epoll_tcp_acceptor/epoll_tcp_acceptor_service, how shutdown sequencing and
resource cleanup are handled (including interaction with epoll_scheduler and
reactor_service_state), and how peer-socket wiring is performed with
epoll_tcp_service and reactor_op_complete; keep it concise (3–6 sentences) and
mention any important concurrency/ordering guarantees and error-handling
expectations so future maintainers understand the high-level design.
In `@include/boost/corosio/native/detail/iocp/win_tcp_acceptor_service.hpp`:
- Around line 18-33: Add a file-level /* */ maintainer overview block
immediately after the includes that explains the IOCP TCP lifecycle and key
invariants for this header: describe how win_tcp_acceptor_service coordinates
accept/start/shutdown, how win_tcp_acceptor and win_tcp_service interact for
socket lifetime, how completions are dispatched via win_scheduler and
win_completion_key, and any ordering/ownership guarantees (who owns sockets,
when shutdown is safe, and how errors are propagated using
make_err/dispatch_coro). Make the comment concise but cover lifecycle, shutdown
sequencing, completion-callback threading model, and references to the key
types/functions (win_tcp_acceptor_service, win_tcp_acceptor, win_tcp_service,
win_scheduler, win_completion_key, endpoint_convert, make_err, dispatch_coro) so
future maintainers can understand invariants without reading all implementation
details.
In `@include/boost/corosio/native/detail/iocp/win_udp_service.hpp`:
- Around line 17-33: Add a file-level C-style block comment immediately after
the includes in win_udp_service.hpp that provides a concise maintainer overview
describing ownership of key components (win_udp_service, win_udp_socket,
win_scheduler, win_completion_key, win_mutex, win_wsa_init), the lifetime and
ownership rules for sockets and completion keys, how asynchronous operations are
queued and completed, and the cancellation/cleanup model (how cancellation
requests are propagated and resources released). Keep it short but explicit
about who owns buffers, which object is responsible for posting/completing IO,
and how errors/cancellations are handled so future maintainers can reason about
concurrency and lifetime.
In `@include/boost/corosio/native/detail/reactor/reactor_datagram_socket.hpp`:
- Around line 13-23: Add a file-level C-style block comment immediately after
the includes in reactor_datagram_socket.hpp that gives a concise high-level
overview of the header's implementation: describe the speculative I/O fast-path,
when/why it falls back to reactor registration, how registration with
ReactorBasicSocket and the reactor loop is performed, the cancellation flow (how
in-flight ops are cancelled and cleaned up), coroutine dispatch interaction
(dispatch_coro), and any lifetime/thread-safety constraints for
ReactorDatagramSocket and related buffers (capy::buffers). Keep it brief but
cover fast-path vs registration, registration lifecycle, cancellation semantics,
and where callers can find entry points in the file.
In `@include/boost/corosio/native/detail/reactor/reactor_socket_service.hpp`:
- Around line 23-34: Add a file-level /* ... */ overview comment immediately
after the includes in reactor_socket_service.hpp that succinctly documents the
lifetime and shutdown contract for this header: explain the relationship between
impl_list_ and impl_ptrs_, the ownership model, the required shutdown ordering,
and any invariants enforced by the CRTP ReactorSocketService (the class template
defined in this file) so maintainers can understand how
construct/destroy/shutdown/close/post/work interact; place the comment near the
top of the file (after includes and before the class/template declarations) and
reference the key symbols impl_list_, impl_ptrs_, and the ReactorSocketService
CRTP to make the contract explicit.
In `@include/boost/corosio/native/detail/reactor/reactor_stream_socket.hpp`:
- Around line 13-24: Add a file-level /* ... */ block comment immediately after
the includes in reactor_stream_socket.hpp that summarizes the high-level design:
describe that this file implements the speculative connect/read/write flow, how
inline-budget completion is handled, the descriptor-slot <-> cancel mapping, and
the close/cancel invariants (who owns descriptor state, when cancel slots are
allocated/freed, and how completion vs cancellation races are resolved).
Reference the key concepts used in this file (speculative connect/read/write,
inline-budget completion, descriptor-slot/cancel mapping, close/cancel
invariants, and any top-level classes/types such as reactor_stream_socket) so
maintainers can quickly understand control flow and lifetime guarantees.
In `@include/boost/corosio/native/native_tcp_socket.hpp`:
- Line 30: The header relies on types from win_tcp_service.hpp (used for
service_type = typename backend_type::tcp_service_type) but only includes
win_tcp_acceptor_service.hpp transitively; add a direct `#include` for
boost/corosio/native/detail/iocp/win_tcp_service.hpp at the top alongside the
existing win_tcp_acceptor_service.hpp include so the dependency is explicit and
include-order fragile is avoided.
In `@include/boost/corosio/native/native_udp.hpp`:
- Around line 79-82: Make the family() accessor constexpr so it can be evaluated
at compile time: change its declaration to a constexpr method while keeping
const and noexcept (i.e. constexpr int family() const noexcept) and leave the
implementation using v6_ ? AF_INET6 : AF_INET unchanged; ensure the class and
v6_ member remain compatible with constexpr methods.
- Around line 38-42: The file has two separate namespace blocks for
boost::corosio which is redundant; consolidate them by moving the forward
declaration of udp_socket into the same namespace block that contains the
native_udp class so both udp_socket and native_udp are declared inside a single
namespace boost::corosio block; update any references in that block accordingly
and remove the now-empty extra namespace declaration to keep declarations
together and avoid fragmentation.
In `@src/corosio/src/io_context.cpp`:
- Around line 11-43: Add a /* ... */ block comment immediately after the
includes describing the high-level implementation: explain that io_context wires
platform-specific schedulers (epoll_scheduler, select_scheduler,
kqueue_scheduler, win_scheduler) and their corresponding TCP/UDP/acceptor
services (e.g., epoll_tcp_service, epoll_udp_service,
select_tcp_acceptor_service, win_tcp_acceptor_service) based on
BOOST_COROSIO_HAS_* macros, outline the invariants for scheduler/service
construction (single scheduler per io_context, services must be
created/registered with that scheduler before use), and note threading/model
expectations (how threads interact with the scheduler and lifetime ordering for
create/destroy) so future maintainers can understand the backend wiring without
diving into implementation details.
In `@src/corosio/src/udp.cpp`:
- Around line 15-19: The udp::family() implementation currently constructs a
temporary native_udp each call; to avoid repeated temporaries either (A) add a
static/native helper like native_udp::family(bool v6) and change udp::family()
to return native_udp::family(v6_), or (B) store and return a cached int member
(e.g., cached_family_) computed once when udp is constructed or when v6_
changes; update the udp constructor or the v6_ setter to initialize
cached_family_ using native_udp(v6_ ? native_udp::v6() :
native_udp::v4()).family(), and have udp::family() simply return cached_family_;
references: udp::family(), native_udp, v6_.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 558412a4-dc87-4ebe-95de-cd2d726a8031
⛔ Files ignored due to path filters (8)
include/boost/corosio/test/socket_pair.hppis excluded by!**/test/**test/unit/buffer_param.cppis excluded by!**/test/**test/unit/ipv6_address.cppis excluded by!**/test/**test/unit/native/native_udp_socket.cppis excluded by!**/test/**test/unit/tcp_acceptor.cppis excluded by!**/test/**test/unit/tcp_socket.cppis excluded by!**/test/**test/unit/thread_pool.cppis excluded by!**/test/**test/unit/udp_socket.cppis excluded by!**/test/**
📒 Files selected for processing (66)
include/boost/corosio/backend.hppinclude/boost/corosio/detail/native_handle.hppinclude/boost/corosio/detail/tcp_acceptor_service.hppinclude/boost/corosio/detail/tcp_service.hppinclude/boost/corosio/detail/thread_pool.hppinclude/boost/corosio/detail/udp_service.hppinclude/boost/corosio/io/io_timer.hppinclude/boost/corosio/ipv6_address.hppinclude/boost/corosio/native/detail/epoll/epoll_op.hppinclude/boost/corosio/native/detail/epoll/epoll_tcp_acceptor.hppinclude/boost/corosio/native/detail/epoll/epoll_tcp_acceptor_service.hppinclude/boost/corosio/native/detail/epoll/epoll_tcp_service.hppinclude/boost/corosio/native/detail/epoll/epoll_tcp_socket.hppinclude/boost/corosio/native/detail/epoll/epoll_udp_service.hppinclude/boost/corosio/native/detail/epoll/epoll_udp_socket.hppinclude/boost/corosio/native/detail/iocp/win_resolver_service.hppinclude/boost/corosio/native/detail/iocp/win_tcp_acceptor.hppinclude/boost/corosio/native/detail/iocp/win_tcp_acceptor_service.hppinclude/boost/corosio/native/detail/iocp/win_tcp_service.hppinclude/boost/corosio/native/detail/iocp/win_tcp_socket.hppinclude/boost/corosio/native/detail/iocp/win_udp_service.hppinclude/boost/corosio/native/detail/iocp/win_udp_socket.hppinclude/boost/corosio/native/detail/kqueue/kqueue_op.hppinclude/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor.hppinclude/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_tcp_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_tcp_socket.hppinclude/boost/corosio/native/detail/kqueue/kqueue_udp_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_udp_socket.hppinclude/boost/corosio/native/detail/posix/posix_resolver_service.hppinclude/boost/corosio/native/detail/reactor/reactor_basic_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_datagram_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_descriptor_state.hppinclude/boost/corosio/native/detail/reactor/reactor_op.hppinclude/boost/corosio/native/detail/reactor/reactor_op_base.hppinclude/boost/corosio/native/detail/reactor/reactor_op_complete.hppinclude/boost/corosio/native/detail/reactor/reactor_scheduler.hppinclude/boost/corosio/native/detail/reactor/reactor_service_state.hppinclude/boost/corosio/native/detail/reactor/reactor_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_socket_service.hppinclude/boost/corosio/native/detail/reactor/reactor_stream_socket.hppinclude/boost/corosio/native/detail/select/select_op.hppinclude/boost/corosio/native/detail/select/select_scheduler.hppinclude/boost/corosio/native/detail/select/select_tcp_acceptor.hppinclude/boost/corosio/native/detail/select/select_tcp_acceptor_service.hppinclude/boost/corosio/native/detail/select/select_tcp_service.hppinclude/boost/corosio/native/detail/select/select_tcp_socket.hppinclude/boost/corosio/native/detail/select/select_udp_service.hppinclude/boost/corosio/native/detail/select/select_udp_socket.hppinclude/boost/corosio/native/native_socket_option.hppinclude/boost/corosio/native/native_tcp_acceptor.hppinclude/boost/corosio/native/native_tcp_socket.hppinclude/boost/corosio/native/native_udp.hppinclude/boost/corosio/native/native_udp_socket.hppinclude/boost/corosio/socket_option.hppinclude/boost/corosio/tcp_socket.hppinclude/boost/corosio/udp.hppinclude/boost/corosio/udp_socket.hppsrc/corosio/src/io_context.cppsrc/corosio/src/ipv6_address.cppsrc/corosio/src/socket_option.cppsrc/corosio/src/tcp_acceptor.cppsrc/corosio/src/tcp_server.cppsrc/corosio/src/tcp_socket.cppsrc/corosio/src/udp.cppsrc/corosio/src/udp_socket.cpp
💤 Files with no reviewable changes (1)
- include/boost/corosio/native/detail/reactor/reactor_socket.hpp
| using scheduler_type = detail::epoll_scheduler; | ||
| using tcp_socket_type = detail::epoll_tcp_socket; | ||
| using tcp_service_type = detail::epoll_tcp_service; | ||
| using udp_socket_type = detail::epoll_udp_socket; | ||
| using udp_service_type = detail::epoll_udp_service; | ||
| using tcp_acceptor_type = detail::epoll_tcp_acceptor; | ||
| using tcp_acceptor_service_type = detail::epoll_tcp_acceptor_service; |
There was a problem hiding this comment.
Keep the legacy backend aliases as compatibility shims.
These ranges replace the public socket_type / service_type / acceptor_type aliases with TCP-specific names only. That is a source-breaking change for existing backend-parameterized TCP code, even when it does not use UDP. Please add the new UDP aliases without dropping the old TCP names, and deprecate the legacy aliases on a later pass.
💡 Suggested fix
struct epoll_t
{
using scheduler_type = detail::epoll_scheduler;
using tcp_socket_type = detail::epoll_tcp_socket;
using tcp_service_type = detail::epoll_tcp_service;
using udp_socket_type = detail::epoll_udp_socket;
using udp_service_type = detail::epoll_udp_service;
using tcp_acceptor_type = detail::epoll_tcp_acceptor;
using tcp_acceptor_service_type = detail::epoll_tcp_acceptor_service;
+ using socket_type = tcp_socket_type;
+ using service_type = tcp_service_type;
+ using acceptor_type = tcp_acceptor_type;
+ using acceptor_service_type = tcp_acceptor_service_type;Mirror the same compatibility aliases in select_t, kqueue_t, and iocp_t.
Also applies to: 93-99, 138-144, 184-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/boost/corosio/backend.hpp` around lines 48 - 54, The new backend
header replaced generic aliases (socket_type, service_type, acceptor_type) with
TCP-specific names causing a source break; restore the legacy compatibility
aliases alongside the new UDP-specific ones. Update each backend typedef block
(e.g., scheduler_type, tcp_socket_type, tcp_service_type, udp_socket_type,
udp_service_type, tcp_acceptor_type, tcp_acceptor_service_type) to also define
the legacy aliases socket_type -> tcp_socket_type, service_type ->
tcp_service_type, and acceptor_type -> tcp_acceptor_type (for select_t,
kqueue_t, iocp_t and the other backend blocks noted) so existing code continues
to compile; leave a comment or mark them for later deprecation but do not remove
the legacy names now.
|
|
||
| namespace boost::corosio { | ||
|
|
||
| /// Represent a platform-specific socket descriptor (`int` on POSIX, `SOCKET` on Windows). |
There was a problem hiding this comment.
Use platform-neutral wording in this detail-header doc comment.
Line 20 mentions SOCKET; prefer neutral wording (for example, “native OS socket handle”) in this portable detail header.
As per coding guidelines, “Headers in include/boost/corosio/, include/boost/corosio/detail/, and include/boost/corosio/concept/ MUST NOT use platform-specific macros and types such as ... SOCKET.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/boost/corosio/detail/native_handle.hpp` at line 20, Update the doc
comment in include/boost/corosio/detail/native_handle.hpp that currently
mentions "SOCKET" to use platform-neutral wording such as "native OS socket
handle" or "platform-specific socket handle (e.g., int on POSIX)" so the detail
header avoids platform-specific symbols; modify the comment text near the
native_handle.hpp declaration to remove "SOCKET" and replace with the neutral
phrase.
| @param family Address family (e.g. `AF_INET`, `AF_INET6`). | ||
| @param type Socket type (`SOCK_DGRAM`). | ||
| @param protocol Protocol number (`IPPROTO_UDP`). |
There was a problem hiding this comment.
Remove platform macro names from this detail-header documentation.
At Line 43-Line 45, replace AF_INET/AF_INET6/SOCK_DGRAM/IPPROTO_UDP references with platform-neutral wording.
As per coding guidelines, “Headers in include/boost/corosio/, include/boost/corosio/detail/, and include/boost/corosio/concept/ MUST NOT use platform-specific macros and types such as AF_INET...”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/boost/corosio/detail/udp_service.hpp` around lines 43 - 45, The
documentation in detail::udp_service.hpp refers to platform-specific macros;
update the parameter descriptions for family, type, and protocol to
platform-neutral wording: change "Address family (e.g. `AF_INET`, `AF_INET6`)"
to something like "Address family (e.g. IPv4 or IPv6)", change "Socket type
(`SOCK_DGRAM`)" to "Socket type (datagram)" and change "Protocol number
(`IPPROTO_UDP`)" to "Protocol number (UDP)"; ensure you only edit the docstring
text around the parameters named family, type, and protocol and do not introduce
any platform macros or headers.
| auto* socket_svc = svc_.tcp_service(); | ||
| if (socket_svc) | ||
| { | ||
| auto& impl = | ||
| static_cast<epoll_socket&>(*socket_svc->construct()); | ||
| static_cast<epoll_tcp_socket&>(*socket_svc->construct()); | ||
| impl.set_socket(accepted); |
There was a problem hiding this comment.
Guard the accepted fd while constructing the peer wrapper.
accept4() has already returned a live peer fd here. If socket_svc->construct() throws, that connection leaks because ownership is not transferred until after the wrapper exists.
Suggested fix
+ struct accepted_fd_guard {
+ int fd;
+ ~accepted_fd_guard() { if (fd >= 0) ::close(fd); }
+ int release() noexcept { return std::exchange(fd, -1); }
+ } guard{accepted};
+
auto& impl =
static_cast<epoll_tcp_socket&>(*socket_svc->construct());
- impl.set_socket(accepted);
+ int accepted_fd = guard.release();
+ impl.set_socket(accepted_fd);
- impl.desc_state_.fd = accepted;
+ impl.desc_state_.fd = accepted_fd;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/boost/corosio/native/detail/epoll/epoll_tcp_acceptor_service.hpp`
around lines 149 - 154, Wrap the raw accepted file descriptor in an RAII guard
before calling socket_svc->construct() so the FD is closed if construct()
throws; then call socket_svc->construct() to create the epoll_tcp_socket wrapper
and only transfer the FD by calling impl.set_socket(accepted) after construction
succeeds (or release the RAII guard when transferring ownership). Update code
around epoll_tcp_acceptor_service where socket_svc->construct(),
epoll_tcp_socket, and set_socket are used to ensure no leak of the accepted FD
on exceptions.
| bool await_ready() const noexcept | ||
| { | ||
| return token_.stop_requested(); | ||
| } | ||
|
|
||
| capy::io_result<std::size_t> await_resume() const noexcept | ||
| { | ||
| if (token_.stop_requested()) | ||
| return {make_error_code(std::errc::operation_canceled), 0}; | ||
| return {ec_, bytes_transferred_}; | ||
| } | ||
|
|
||
| auto await_suspend(std::coroutine_handle<> h, capy::io_env const* env) | ||
| -> std::coroutine_handle<> | ||
| { | ||
| token_ = env->stop_token; | ||
| return self_.get_impl().send_to( | ||
| h, env->executor, buffers_, dest_, token_, &ec_, | ||
| &bytes_transferred_); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In C++20, is std::stop_tokena live view of shared stop state such thatstop_requested()can change betweenawait_suspendandawait_resume on different threads?
💡 Result:
Yes. In C++20, std::stop_token is a lightweight, copyable handle to a shared stop-state—“essentially a thread-safe view of the associated stop-state” [1]. If any std::stop_source associated with that same stop-state calls request_stop(), that stop request becomes visible to all stop_tokens for that stop-state [3], and the stop-state update is specified to be atomic with stop_requested() allowed to be called concurrently from other threads [2].
So if your coroutine’s await_suspend and await_resume run on different threads, token.stop_requested() can legitimately change from false to true in between (because another thread may call request_stop() during that interval) [2][3].
Sources: [1] cppreference std::stop_token overview, [2] cppreference stop_source::request_stop concurrency notes, [3] cppreference std::stop_source overview.
🏁 Script executed:
# Get the file and examine lines 100-235 to see both locations mentioned
head -235 include/boost/corosio/native/native_udp_socket.hpp | tail -136Repository: cppalliance/corosio
Length of output: 4467
🏁 Script executed:
# Check if the parent class udp_socket has the same cancellation bug
head -250 include/boost/corosio/udp_socket.hpp | tail -150 | grep -A 20 "await_ready\|await_suspend\|await_resume" | head -60Repository: cppalliance/corosio
Length of output: 45
🏁 Script executed:
# Find and search for the send_to/recv_from awaitable patterns in the parent class
find include -name "udp_socket.hpp" -not -path "*/native/*" | head -1 | xargs cat | grep -n "await_ready\|await_suspend\|await_resume" -A 5 | head -80Repository: cppalliance/corosio
Length of output: 2904
Latch cancellation in these native awaitables to prevent successful results from being masked by late stop requests.
Both native_send_to_awaitable (lines 105–124) and native_recv_from_awaitable (lines 147–166) repeat the same race condition as the parent class: await_ready() checks a default-constructed token_, await_suspend() assigns the live env->stop_token, and await_resume() re-checks it. If another thread calls request_stop() between await_suspend() and await_resume(), a successful I/O result will be overwritten with operation_canceled. Latch the token's state at suspension and check the latched result at resumption instead of re-polling the live token.
Additionally, the public send_to() and recv_from() wrapper methods (lines 169–189 and 191–211) lack the required async operation documentation: they must document completion conditions (when the coroutine resumes), cancellation semantics via stop_token, error codes including capy::cond::canceled, buffer ownership and lifetime requirements, and template parameter constraints. Currently they only state they "bypass virtual dispatch" and refer to the parent class, which is insufficient for public-header async operations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/boost/corosio/native/native_udp_socket.hpp` around lines 105 - 124,
Latch the stop_token state at suspension and consult that latched value at
resume to avoid a race that can overwrite a successful I/O with
operation_canceled: in native_send_to_awaitable and native_recv_from_awaitable,
capture env->stop_token.requested() (or equivalent bool) into a member (e.g.,
latched_requested_) inside await_suspend() after assigning token_, and change
await_ready()/await_resume() to consult that latched flag instead of polling the
live token_; keep the existing token_ assignment for use by the underlying
send_to/recv_from call. Also update the public send_to() and recv_from() wrapper
documentation (the overloads that bypass virtual dispatch) to explicitly state
completion conditions (when the coroutine resumes), cancellation semantics via
stop_token (including that cancellation may produce capy::cond::canceled or
std::errc::operation_canceled), precise error-code behavior, buffer
ownership/lifetime requirements, and template parameter constraints so users
know what guarantees send_to() and recv_from() provide.
| /** Enable loopback of outgoing multicast on IPv4 (IP_MULTICAST_LOOP). | ||
|
|
||
| @par Example | ||
| @code | ||
| sock.set_option( socket_option::multicast_loop_v4( true ) ); | ||
| @endcode | ||
| */ | ||
| class BOOST_COROSIO_DECL multicast_loop_v4 : public boolean_option | ||
| { | ||
| public: | ||
| using boolean_option::boolean_option; | ||
| using boolean_option::operator=; | ||
|
|
||
| /// Return the protocol level. | ||
| static int level() noexcept; | ||
|
|
||
| /// Return the option name. | ||
| static int name() noexcept; | ||
| }; | ||
|
|
||
| /** Enable loopback of outgoing multicast on IPv6 (IPV6_MULTICAST_LOOP). | ||
|
|
||
| @par Example | ||
| @code | ||
| sock.set_option( socket_option::multicast_loop_v6( true ) ); | ||
| @endcode | ||
| */ | ||
| class BOOST_COROSIO_DECL multicast_loop_v6 : public boolean_option | ||
| { | ||
| public: | ||
| using boolean_option::boolean_option; | ||
| using boolean_option::operator=; | ||
|
|
||
| /// Return the protocol level. | ||
| static int level() noexcept; | ||
|
|
||
| /// Return the option name. | ||
| static int name() noexcept; | ||
| }; | ||
|
|
||
| /** Set the multicast TTL for IPv4 (IP_MULTICAST_TTL). | ||
|
|
||
| @par Example | ||
| @code | ||
| sock.set_option( socket_option::multicast_hops_v4( 4 ) ); | ||
| @endcode | ||
| */ | ||
| class BOOST_COROSIO_DECL multicast_hops_v4 : public integer_option | ||
| { | ||
| public: | ||
| using integer_option::integer_option; | ||
| using integer_option::operator=; | ||
|
|
||
| /// Return the protocol level. | ||
| static int level() noexcept; | ||
|
|
||
| /// Return the option name. | ||
| static int name() noexcept; | ||
| }; | ||
|
|
||
| /** Set the multicast hop limit for IPv6 (IPV6_MULTICAST_HOPS). | ||
|
|
||
| @par Example | ||
| @code | ||
| sock.set_option( socket_option::multicast_hops_v6( 4 ) ); | ||
| @endcode | ||
| */ | ||
| class BOOST_COROSIO_DECL multicast_hops_v6 : public integer_option | ||
| { | ||
| public: | ||
| using integer_option::integer_option; | ||
| using integer_option::operator=; | ||
|
|
||
| /// Return the protocol level. | ||
| static int level() noexcept; | ||
|
|
||
| /// Return the option name. | ||
| static int name() noexcept; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the base class implementations
fd -t f "socket_option" --max-one-result | head -5Repository: cppalliance/corosio
Length of output: 296
🏁 Script executed:
# Look for boolean_option and integer_option definitions
rg "class boolean_option|class integer_option" -A 10Repository: cppalliance/corosio
Length of output: 1396
🏁 Script executed:
# Check the full structure of the socket_option header
wc -l include/boost/corosio/socket_option.hppRepository: cppalliance/corosio
Length of output: 106
🏁 Script executed:
# Get context around the lines in question (431-509)
sed -n '420,520p' include/boost/corosio/socket_option.hpp | cat -nRepository: cppalliance/corosio
Length of output: 3170
🏁 Script executed:
# Search for any platform-specific implementations (Windows/POSIX/macOS)
rg "IP_MULTICAST_LOOP|IPV6_MULTICAST_LOOP|IP_MULTICAST_TTL" -B 2 -A 2Repository: cppalliance/corosio
Length of output: 2059
🏁 Script executed:
# Check if there are any platform-specific includes or conditional compilation
rg "defined.*WIN|_WIN32|__APPLE__|linux" include/boost/corosio/socket_option.hppRepository: cppalliance/corosio
Length of output: 45
🏁 Script executed:
# Look for the implementation files (.cpp) for this header
fd "socket_option\\.cpp"Repository: cppalliance/corosio
Length of output: 96
🏁 Script executed:
# Check the native socket option header for template implementations
sed -n '1,100p' include/boost/corosio/native/native_socket_option.hpp | cat -nRepository: cppalliance/corosio
Length of output: 3649
🏁 Script executed:
# Look for the boolean and integer template definitions
rg "template.*boolean|template.*integer" include/boost/corosio/native/ -A 5Repository: cppalliance/corosio
Length of output: 45
🏁 Script executed:
# Check the implementation file
head -150 src/corosio/src/socket_option.cpp | cat -nRepository: cppalliance/corosio
Length of output: 3792
🏁 Script executed:
# Search for multicast_loop_v4 implementation
rg "multicast_loop_v4::level|multicast_loop_v4::name|multicast_hops_v4" src/ -A 3Repository: cppalliance/corosio
Length of output: 1393
🏁 Script executed:
# Check if there's any platform-specific handling for Windows vs POSIX
find include/boost/corosio/native -type f -name "*.hpp" | xargs cat | head -300 | cat -nRepository: cppalliance/corosio
Length of output: 11055
🏁 Script executed:
# Find the native multicast_loop_v4 definition
rg "multicast_loop_v4|multicast_hops_v4" include/boost/corosio/native/native_socket_option.hpp -B 1 -A 1Repository: cppalliance/corosio
Length of output: 324
🏁 Script executed:
# Get the full native_socket_option.hpp to see how boolean and integer templates work
wc -l include/boost/corosio/native/native_socket_option.hppRepository: cppalliance/corosio
Length of output: 120
🏁 Script executed:
# Look at the data() and size() methods of the boolean template
sed -n '78,150p' include/boost/corosio/native/native_socket_option.hpp | cat -nRepository: cppalliance/corosio
Length of output: 2167
🏁 Script executed:
# Check how portable socket_option classes use the native implementations
rg "data\(\)|size\(\)" include/boost/corosio/socket_option.hpp -B 2 -A 2 | head -50Repository: cppalliance/corosio
Length of output: 1084
🏁 Script executed:
# Look for the boolean_option size() and data() implementations in portable header
sed -n '1,300p' include/boost/corosio/socket_option.hpp | grep -A 10 "class boolean_option"Repository: cppalliance/corosio
Length of output: 305
🏁 Script executed:
# Check for any resize() or special handling in portable classes
sed -n '1,400p' include/boost/corosio/socket_option.hpp | grep -A 8 "class boolean_option"Repository: cppalliance/corosio
Length of output: 238
🏁 Script executed:
# Look for resize() in the portable header
rg "void resize" include/boost/corosio/socket_option.hpp -B 2 -A 3Repository: cppalliance/corosio
Length of output: 1297
🏁 Script executed:
# Check the complete boolean_option class to see if there's resize()
sed -n '100,200p' include/boost/corosio/socket_option.hpp | cat -nRepository: cppalliance/corosio
Length of output: 3177
🌐 Web query:
IP_MULTICAST_LOOP socket option type size macOS Linux Windows
💡 Result:
IP_MULTICAST_LOOP optval type / size (IPv4)
| OS | setsockopt() level/optname |
optval C type typically used |
optlen |
Notes |
|---|---|---|---|---|
| macOS (Darwin/BSD) | IPPROTO_IP, IP_MULTICAST_LOOP |
u_char (0 or 1) |
sizeof(u_char) (1 byte) |
Documented as u_char loop; /* 0=disable, 1=enable */ and passed with sizeof(loop) [1]. |
| Linux | IPPROTO_IP, IP_MULTICAST_LOOP |
int “boolean integer argument” |
sizeof(int) (commonly 4 bytes) |
Linux ip(7) specifies a boolean integer argument for this option [2]. |
| Windows (Winsock) | IPPROTO_IP, IP_MULTICAST_LOOP |
DWORD (boolean) |
sizeof(DWORD) (4 bytes) |
Microsoft documents the option value type as DWORD (boolean) [3]. Also note Windows’ behavior is not POSIX-compatible: it controls loopback on the receiving socket rather than the sending socket [3]. |
Sources: macOS ip(4) [1], Linux ip(7) [2], Microsoft Winsock IPPROTO_IP socket options [3].
🌐 Web query:
IP_MULTICAST_TTL socket option type DWORD int u_char platform differences
💡 Result:
IP_MULTICAST_TTL is one of the IPv4 multicast socket options whose optval type/size differs by platform:
- *BSD/POSIX (macOS, BSD, many Unix stacks):
optvalisu_char/unsigned char(1 byte) (TTL 0–255; default commonly 1). (manpagez.com) - Linux / glibc headers: the option is documented in headers as
u_charas well (1 byte). (sites.uclouvain.be) - Windows (Winsock):
IP_MULTICAST_TTLexpects aDWORD(4 bytes). (learn.microsoft.com)
Practical portability guidance
- If you pass a 1-byte
u_charTTL on Windows, you risksetsockopt()failing (or misbehaving) because Winsock documents a 4-byteDWORD. (learn.microsoft.com) - If you pass a 4-byte
int/DWORDTTL on BSD/POSIX stacks, you risk non-portable behavior because those stacks document a 1-byte value. (manpagez.com)
Portable pattern
Use a small wrapper that uses DWORD on Windows and uint8_t/unsigned char elsewhere, and pass the matching sizeof for each. (learn.microsoft.com)
Citations:
- 1: https://www.manpagez.com/man/4/ip/
- 2: https://sites.uclouvain.be/SystInfo/usr/include/bits/in.h.html
- 3: https://learn.microsoft.com/en-us/windows/win32/winsock/ipproto-ip-socket-options
- 4: https://learn.microsoft.com/en-us/windows/win32/winsock/ipproto-ip-socket-options
- 5: https://www.manpagez.com/man/4/ip/
- 6: https://learn.microsoft.com/en-us/windows/win32/winsock/ipproto-ip-socket-options
The IPv4 multicast wrappers need platform-specific storage and semantics.
multicast_loop_v4 / multicast_hops_v4 inherit generic boolean_option / integer_option bases, but platforms differ materially: macOS/BSD require 1-byte u_char values, Linux uses 4-byte integers, and Windows uses 4-byte DWORD while also defining IP_MULTICAST_LOOP on the receiving socket rather than the sender. The resize() method handles read-side normalization only; size() still reports 4 bytes, causing setsockopt() to send incorrect sizes on macOS/BSD. Introduce dedicated IPv4 multicast wrappers with platform-appropriate storage (and mirror them in native_socket_option) instead of reusing generic bases. Also document the Windows semantic difference (receive-side vs. POSIX send-side loopback).
| /** An asynchronous UDP socket for coroutine I/O. | ||
|
|
||
| This class provides asynchronous UDP datagram operations that | ||
| return awaitable types. Each operation participates in the affine | ||
| awaitable protocol, ensuring coroutines resume on the correct | ||
| executor. | ||
|
|
||
| Supports two modes of operation: | ||
|
|
||
| **Connectionless mode**: each `send_to` specifies a destination | ||
| endpoint, and each `recv_from` captures the source endpoint. | ||
| The socket must be opened (and optionally bound) before I/O. | ||
|
|
||
| **Connected mode**: call `connect()` to set a default peer, | ||
| then use `send()`/`recv()` without endpoint arguments. | ||
| The kernel filters incoming datagrams to those from the | ||
| connected peer. | ||
|
|
||
| @par Thread Safety | ||
| Distinct objects: Safe.@n | ||
| Shared objects: Unsafe. A socket must not have concurrent | ||
| operations of the same type (e.g., two simultaneous recv_from). | ||
| One send_to and one recv_from may be in flight simultaneously. | ||
|
|
||
| @par Example | ||
| @code | ||
| // Connectionless mode | ||
| io_context ioc; | ||
| udp_socket sock( ioc ); | ||
| sock.open( udp::v4() ); | ||
| sock.bind( endpoint( ipv4_address::any(), 9000 ) ); | ||
|
|
||
| char buf[1024]; | ||
| endpoint sender; | ||
| auto [ec, n] = co_await sock.recv_from( | ||
| capy::mutable_buffer( buf, sizeof( buf ) ), sender ); | ||
| if ( !ec ) | ||
| co_await sock.send_to( | ||
| capy::const_buffer( buf, n ), sender ); | ||
|
|
||
| // Connected mode | ||
| udp_socket csock( ioc ); | ||
| auto [cec] = co_await csock.connect( | ||
| endpoint( ipv4_address::loopback(), 9000 ) ); | ||
| if ( !cec ) | ||
| co_await csock.send( | ||
| capy::const_buffer( buf, n ) ); | ||
| @endcode | ||
| */ |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Finish the operation-level docs for this public UDP API.
The class overview is helpful, but the new public async surface still omits buffer lifetime/ownership, completion conditions, overlap rules, cancellation via stop_token/cancel(), the structure of the returned io_result, and the concept requirements on the templated buffer overloads. Line 550 also hard-codes a POSIX -1 sentinel for native_handle() even though the IOCP path uses INVALID_SOCKET.
As per coding guidelines "Docstrings are required for all classes and functions in public headers in non-detail namespaces" and async operations must document completion conditions, concurrency/overlap, cancellation, parameter ownership/lifetime, return values, and examples.
Also applies to: 89-235, 495-697
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/boost/corosio/udp_socket.hpp` around lines 37 - 85, Expand the public
API docs for class udp_socket and its async operations (recv_from, send_to,
send, connect, and any templated buffer overloads) to explicitly state: buffer
ownership and lifetime requirements (who must keep buffers alive until the
awaitable completes), the completion conditions and what constitutes success vs
error (structure and semantics of io_result including error_code and
byte_count), concurrency/overlap rules (which operations may run concurrently
and which must not), cancellation behavior and how stop_token/cancel() affect
in-flight operations, and the concept/type requirements for templated buffer
parameters; also audit and update other documented ranges noted (lines ~89-235,
495-697) to match this structure. Finally, replace the hard-coded POSIX sentinel
in native_handle() (-1) with a platform-neutral sentinel (use INVALID_SOCKET on
Windows or std::numeric_limits<socket_type>::min()/an appropriately named
constant) or document the platform-specific semantics in the native_handle()
docstring.
| bool await_ready() const noexcept | ||
| { | ||
| return token_.stop_requested(); | ||
| } | ||
|
|
||
| capy::io_result<std::size_t> await_resume() const noexcept | ||
| { | ||
| if (token_.stop_requested()) | ||
| return {make_error_code(std::errc::operation_canceled), 0}; | ||
| return {ec_, bytes_}; | ||
| } | ||
|
|
||
| auto await_suspend(std::coroutine_handle<> h, capy::io_env const* env) | ||
| -> std::coroutine_handle<> | ||
| { | ||
| token_ = env->stop_token; | ||
| return s_.get().send_to( | ||
| h, env->executor, buf_, dest_, token_, &ec_, &bytes_); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In C++20, is std::stop_tokena live view of shared stop state such thatstop_requested()can change betweenawait_suspendandawait_resume on different threads?
💡 Result:
Yes.
std::stop_token is a handle to a shared stop state; copies of the token (and any std::stop_sources for that same state) all observe the same underlying “stop requested” flag, and stop requests are visible across threads. [1][2]
The standard specifies that stop_requested() is executed atomically, concurrent calls to request_stop() / stop_requested() don’t data-race, and if stop_requested() returns true it synchronizes-with the first successful request_stop() on an associated stop_source. [1]
So, in a coroutine scenario, a stop_token you check in await_suspend can indeed report false, and then later (on another thread) request_stop() can occur, and then in await_resume a new stop_requested() call can report true. (Once it becomes true, it stays true.) [1][2]
Sources: [1] P0660R9 (stop_token/stop_source wording & synchronization), [2] cppreference std::stop_source (visibility to all tokens/sources sharing stop-state).
🏁 Script executed:
# First, let's verify the file exists and examine the exact code at the specified lines
cd /repo && find . -name "udp_socket.hpp" -type f | head -5Repository: cppalliance/corosio
Length of output: 118
🏁 Script executed:
# Read the file to understand the full context of the await implementations
wc -l include/boost/corosio/udp_socket.hppRepository: cppalliance/corosio
Length of output: 103
🏁 Script executed:
# Examine the lines mentioned in the review comment to confirm the issue
sed -n '250,280p' include/boost/corosio/udp_socket.hppRepository: cppalliance/corosio
Length of output: 948
🏁 Script executed:
# Check the other locations mentioned in "Also applies to"
sed -n '295,330p' include/boost/corosio/udp_socket.hppRepository: cppalliance/corosio
Length of output: 1104
🏁 Script executed:
# Get a broader view of the entire awaitable structures to understand the pattern
sed -n '1,100p' include/boost/corosio/udp_socket.hppRepository: cppalliance/corosio
Length of output: 3297
🏁 Script executed:
# Check lines 302-320, 341-358, 381-398, 421-438 to confirm the pattern
sed -n '302,320p' include/boost/corosio/udp_socket.hppRepository: cppalliance/corosio
Length of output: 697
🏁 Script executed:
sed -n '341,358p' include/boost/corosio/udp_socket.hppRepository: cppalliance/corosio
Length of output: 642
🏁 Script executed:
sed -n '381,398p' include/boost/corosio/udp_socket.hppRepository: cppalliance/corosio
Length of output: 665
🏁 Script executed:
sed -n '421,438p' include/boost/corosio/udp_socket.hppRepository: cppalliance/corosio
Length of output: 665
🏁 Script executed:
# Let's also check what token_ is - look for its member variable declaration
sed -n '150,280p' include/boost/corosio/udp_socket.hpp | grep -A 5 "send_to_awaitable"Repository: cppalliance/corosio
Length of output: 383
Fix systemic live-token race in all five awaitable types.
token_ is uninitialized when await_ready() runs (always returns false), then assigned in await_suspend(). Between that point and await_resume(), another thread can call request_stop() on the shared stop state. Because std::stop_token::stop_requested() is a live view of shared state, await_resume() will re-read it and return operation_canceled instead of the actual backend result, silently corrupting successful I/O.
Let the backend-populated ec_ (and bytes_ where applicable) drive the final result. Remove the live-token check from await_resume(), and change await_ready() to return false.
Fix pattern
bool await_ready() const noexcept
{
- return token_.stop_requested();
+ return false;
}
capy::io_result<std::size_t> await_resume() const noexcept
{
- if (token_.stop_requested())
- return {make_error_code(std::errc::operation_canceled), 0};
return {ec_, bytes_};
}Applies to send_to_awaitable (259–277), recv_from_awaitable (302–320), connect_awaitable (341–358), send_awaitable (381–398), and recv_awaitable (421–438).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/boost/corosio/udp_socket.hpp` around lines 259 - 277, The awaitables
use a live std::stop_token in await_ready()/await_resume(), causing a race where
token_ is uninitialized in await_ready() and stop_requested() re-reads shared
state in await_resume() and can override backend results; fix by changing
await_ready() to always return false (remove token_.stop_requested() there) and
remove the live-token check from await_resume() so it returns the
backend-populated ec_ (and bytes_ where applicable) directly; apply this change
to send_to_awaitable, recv_from_awaitable, connect_awaitable, send_awaitable and
recv_awaitable (adjust await_ready(), await_resume(), and keep await_suspend()
assigning token_ and calling the backend as-is, relying on ec_/bytes_ set by the
backend).
| /// Return the address family (AF_INET or AF_INET6). | ||
| int family() const noexcept; | ||
|
|
||
| /// Return the socket type (SOCK_DGRAM). | ||
| static int type() noexcept; | ||
|
|
||
| /// Return the IP protocol (IPPROTO_UDP). | ||
| static int protocol() noexcept; |
There was a problem hiding this comment.
Use platform-neutral return documentation for protocol constants.
Line 63-Line 70 should describe semantic meaning without naming OS macro constants directly.
As per coding guidelines, “Headers in include/boost/corosio/, include/boost/corosio/detail/, and include/boost/corosio/concept/ MUST NOT use platform-specific macros and types such as AF_INET...”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/boost/corosio/udp.hpp` around lines 63 - 70, The comments for
family(), type(), and protocol() currently reference platform-specific macro
names; update their documentation to be platform-neutral by describing the
semantics only: document family() as returning the address family indicating
whether the endpoint is IPv4 or IPv6, type() as returning the socket type used
for datagram sockets, and protocol() as returning the IP protocol number
corresponding to UDP; do not mention any OS-specific macros like AF_INET,
SOCK_DGRAM, or IPPROTO_UDP and leave the function signatures unchanged.
There was a problem hiding this comment.
Fixed (4 items)
- 5
win_udp_service.hpp— Fixed:recv_from_op::do_completenow populatessource_outfor zero-byte datagrams (removedbytes_transferred > 0guard). - 6
win_udp_service.hpp+reactor_datagram_socket.hpp— Fixed: Removed the empty-buffer short-circuit from UDP send paths. Zero-length datagrams are valid and should be sent viasendmsg/WSASendTo. - 8
kqueue_udp_service.hpp— Fixed:SO_NOSIGPIPEis now guarded with#ifdef SO_NOSIGPIPEand treated as non-fatal, matching the pattern inkqueue_tcp_acceptor_service.hpp. - 10
reactor_datagram_socket.hpp— Fixed: Inline fast path now assigns*sourcewhenn >= 0(wasn > 0), preserving the source endpoint for zero-byte datagrams.
Not fixing (14 items)
- 2
native_handle.hpp— The doc comment explains what the alias maps to on each platform, which is exactly what a reader needs at the definition site. Not changing. - 3
udp_service.hpp— This is a detail header for backend implementors. TheAF_INET/SOCK_DGRAM/IPPROTO_UDPnames precisely describe the values callers pass. Not changing. - 4
epoll_tcp_acceptor_service.hpp—construct()can only throwbad_alloc. On OOM the process is about to terminate anyway; adding try/catch here is noise. - 7
win_udp_service.hpp— Not a real issue. Even if the stop callback fires synchronously duringop.start(token),cancelledis set and the completion handler reports cancellation correctly. The connected socket state is harmless since the socket will be closed on error. This is the same pattern used throughout for TCP connect. - 9
reactor_basic_socket.hpp— The current order is intentional. If I/O completed on the cached ready path, we deliver the result even if cancellation was requested (last-writer-wins, same as Asio). Checking cancellation first would discard completed I/O. - 11
reactor_descriptor_state.hpp—ready_events_.exchange()is correctly inside the mutex. Moving it outside would create a window where a reactor could set new events between the exchange and lock acquisition, losing those events. The comment at lines 136-142 explains the ordering requirement. - 12
reactor_socket_service.hpp—<memory>is included and providesstd::movein practice. Pedantically adding<utility>is unnecessary. - 13
reactor_socket_service.hpp—destroy()runs from the io_object destructor.shutdown()runs during context destruction. These cannot race: context destruction happens after all io_objects are destroyed (they hold a handle referencing the context). - 14
native_udp_socket.hpp— The docs say "Otherwise identical to @ref udp_socket::send_to" — the cross-reference provides the full contract. Duplicating documentation is maintenance burden. - 15/18 Awaitable live-token race — Not a real race.
token_is default-constructed (stop_possible() == false), soawait_ready()returns false and falls through toawait_suspend()which assigns the real token. The backend populatesec_/bytes_before completion. Theawait_resume()re-check is belt-and-suspenders for tokens already stopped beforeawait_ready. TCP uses the identical pattern. - 16
socket_option.hpp—IP_MULTICAST_LOOPon macOS/BSD expectsu_char, butsetsockoptwith a 4-byteintstill works (kernel reads the first byte).getsockoptwrites 1 byte andresize()at line 106-110 handles that case. The tests pass on macOS CI. - 17
udp_socket.hpp— The POSIX-1sentinel is handled by the#if BOOST_COROSIO_HAS_IOCPblock inis_open(). Doc improvements are a style issue. - 19
udp.hpp—AF_INET,SOCK_DGRAM,IPPROTO_UDPare POSIX/Windows standard names, not obscure platform internals. They precisely describe what the functions return.
- Preserve source endpoint for zero-byte datagrams on both reactor inline path (n >= 0) and IOCP completion handler - Allow zero-length UDP sends instead of short-circuiting them - Guard SO_NOSIGPIPE with #ifdef on kqueue UDP service
There was a problem hiding this comment.
♻️ Duplicate comments (2)
include/boost/corosio/native/detail/iocp/win_udp_service.hpp (2)
194-200:⚠️ Potential issue | 🟠 MajorGuard
source_outassignment for empty-buffer receives.
recv_from_op::do_completenow writes*source_outon every successful completion, including the synthetic empty-buffer fast path. That can overwrite the caller’s endpoint with stale/default address data even though no datagram was received.Suggested fix
- if (success && op->source_out) + if (success && op->source_out && !op->empty_buffer) { *op->source_out = from_sockaddr(op->source_storage); }Also applies to: 447-453
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/native/detail/iocp/win_udp_service.hpp` around lines 194 - 200, In recv_from_op::do_complete ensure you only write *op->source_out when a real datagram was received: guard the assignment with a condition that transfer size > 0 (e.g. op->bytes_transferred > 0 or equivalent field indicating received bytes) in addition to the existing success check; update the two locations that perform the assignment (the block at the shown lines and the other instance around lines 447-453) so that if the receive completed with zero-length/empty-buffer (synthetic fast path) we do not overwrite the caller’s endpoint.
506-515:⚠️ Potential issue | 🟠 MajorSkip
WSAConnectwhen cancellation is already observed.After
op.start(token), cancellation may already be set, but::WSAConnectis still called unconditionally. That can leave the socket connected while the operation completes as canceled.Suggested fix
op.start(token); svc_.work_started(); + if (op.cancelled.load(std::memory_order_acquire)) + { + svc_.on_completion(&op, ERROR_OPERATION_ABORTED, 0); + return std::noop_coroutine(); + } sockaddr_storage storage{};#!/bin/bash python - <<'PY' from pathlib import Path p = Path("include/boost/corosio/native/detail/iocp/win_udp_service.hpp") lines = p.read_text().splitlines() start, end = 490, 521 seg = lines[start-1:end] print(f"Inspecting lines {start}-{end} in {p}:") for i, l in enumerate(seg, start): print(f"{i}: {l}") has_cancel_check = any("cancelled.load" in l for l in seg) print("\ncontains pre-WSAConnect cancellation check:", has_cancel_check) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/native/detail/iocp/win_udp_service.hpp` around lines 506 - 515, After calling op.start(token) and before invoking ::WSAConnect, check whether the cancellation token has already been signaled and, if so, avoid calling ::WSAConnect; instead complete the operation via the same cancellation pathway and tidy up the IOCP bookkeeping (mirror what svc_.work_started()/work_finished and the op completion code do). Concretely: after op.start(token) and svc_.work_started(), test the token (e.g., token.cancelled.load() or token.is_canceled()) and if true, call the operation's cancel-completion routine used elsewhere in this file (the same codepath that marks the op as canceled and releases any resources) and return without touching socket_ or calling ::WSAConnect.
🧹 Nitpick comments (3)
include/boost/corosio/native/detail/reactor/reactor_datagram_socket.hpp (2)
102-170: Expand async operation docblocks to include full coroutine contract sections.The operation comments are helpful but still miss required contract elements (
@paramlifetime/ownership,@returntuple elements, completion conditions list, concurrency/overlap, cancellation behavior, error conditions,@throws, examples,@see).As per coding guidelines
**/*.{h,hpp,cpp,cc,cxx}: async APIs should document completion conditions, concurrency/overlap, parameters, return aggregate, errors, cancellation, throws, examples, and related references.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/native/detail/reactor/reactor_datagram_socket.hpp` around lines 102 - 170, The docblocks for the async reactor operations (do_send_to, do_recv_from, do_connect, do_send, do_recv) are missing the required coroutine contract details; update each comment to include explicit `@param` entries describing lifetime/ownership for every argument (including the coroutine_handle, capy::executor_ref, buffer_param, endpoint/endpoint*, stop_token, error_code*, size_t*), an `@return` describing the returned std::coroutine_handle<> and any tuple/side-effect semantics, a clear list of completion conditions (success, partial success, hard errors, EAGAIN/EINPROGRESS), concurrency/overlap rules (whether operations may run concurrently on the same socket and what races are allowed), cancellation behavior and how stop_token is observed, all possible error conditions and whether errors are reported via *std::error_code or thrown, an `@throws` if exceptions can be propagated, a brief example usage snippet, and `@see` references to related methods (e.g., send_wr_, recv_rd_, reactor registration). Ensure each function's docblock follows the project header style and includes ownership/lifetime notes for pointer parameters and the semantics of inline vs queued completions.
20-25: Add a file-level implementation overview block after the includes.This header has non-trivial control flow (speculative syscalls, inline-budget dispatch, reactor fallback), but there is no
/* */high-level maintainer overview immediately after includes.As per coding guidelines
**/*.{cpp,hpp}: files with non-trivial implementation logic should include a/* */block comment after includes explaining tricky/non-obvious behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/native/detail/reactor/reactor_datagram_socket.hpp` around lines 20 - 25, Add a file-level block comment immediately after the includes in reactor_datagram_socket.hpp that gives a high-level maintainer overview of the datagram reactor implementation: briefly describe the speculative syscalls behavior, the inline-budget dispatch strategy, and the reactor fallback path (when/why it is used), and note any non-obvious control flow or threading/ordering assumptions; place this comment before the namespace boost::corosio::detail and reference the main conceptual pieces (speculative syscall logic, inline-budget dispatch, reactor fallback) so future readers can quickly understand the tricky behavior.include/boost/corosio/native/detail/iocp/win_udp_service.hpp (1)
17-34: Add a high-level implementation overview block after includes.This file has non-trivial orchestration/cancellation/completion logic; please add a short
/* ... */maintainer overview after the include section.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."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/native/detail/iocp/win_udp_service.hpp` around lines 17 - 34, Add a short /* ... */ maintainer overview block immediately after the includes in win_udp_service.hpp that summarizes the high-level orchestration: describe the role of the win_udp_service class and how it coordinates win_udp_socket, win_scheduler, win_completion_key and win_mutex; outline the flow for submitting operations, how IO completion events are routed (via win_completion_key and dispatch_coro), and how cancellation and error handling are performed (including use of win_wsa_init, endpoint_convert, and make_err). Keep it brief (3–6 sentences), mention key interactions and lifetime/cancellation semantics so future readers can quickly grasp the control flow without diving into implementation details.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@include/boost/corosio/native/detail/iocp/win_udp_service.hpp`:
- Around line 194-200: In recv_from_op::do_complete ensure you only write
*op->source_out when a real datagram was received: guard the assignment with a
condition that transfer size > 0 (e.g. op->bytes_transferred > 0 or equivalent
field indicating received bytes) in addition to the existing success check;
update the two locations that perform the assignment (the block at the shown
lines and the other instance around lines 447-453) so that if the receive
completed with zero-length/empty-buffer (synthetic fast path) we do not
overwrite the caller’s endpoint.
- Around line 506-515: After calling op.start(token) and before invoking
::WSAConnect, check whether the cancellation token has already been signaled
and, if so, avoid calling ::WSAConnect; instead complete the operation via the
same cancellation pathway and tidy up the IOCP bookkeeping (mirror what
svc_.work_started()/work_finished and the op completion code do). Concretely:
after op.start(token) and svc_.work_started(), test the token (e.g.,
token.cancelled.load() or token.is_canceled()) and if true, call the operation's
cancel-completion routine used elsewhere in this file (the same codepath that
marks the op as canceled and releases any resources) and return without touching
socket_ or calling ::WSAConnect.
---
Nitpick comments:
In `@include/boost/corosio/native/detail/iocp/win_udp_service.hpp`:
- Around line 17-34: Add a short /* ... */ maintainer overview block immediately
after the includes in win_udp_service.hpp that summarizes the high-level
orchestration: describe the role of the win_udp_service class and how it
coordinates win_udp_socket, win_scheduler, win_completion_key and win_mutex;
outline the flow for submitting operations, how IO completion events are routed
(via win_completion_key and dispatch_coro), and how cancellation and error
handling are performed (including use of win_wsa_init, endpoint_convert, and
make_err). Keep it brief (3–6 sentences), mention key interactions and
lifetime/cancellation semantics so future readers can quickly grasp the control
flow without diving into implementation details.
In `@include/boost/corosio/native/detail/reactor/reactor_datagram_socket.hpp`:
- Around line 102-170: The docblocks for the async reactor operations
(do_send_to, do_recv_from, do_connect, do_send, do_recv) are missing the
required coroutine contract details; update each comment to include explicit
`@param` entries describing lifetime/ownership for every argument (including the
coroutine_handle, capy::executor_ref, buffer_param, endpoint/endpoint*,
stop_token, error_code*, size_t*), an `@return` describing the returned
std::coroutine_handle<> and any tuple/side-effect semantics, a clear list of
completion conditions (success, partial success, hard errors,
EAGAIN/EINPROGRESS), concurrency/overlap rules (whether operations may run
concurrently on the same socket and what races are allowed), cancellation
behavior and how stop_token is observed, all possible error conditions and
whether errors are reported via *std::error_code or thrown, an `@throws` if
exceptions can be propagated, a brief example usage snippet, and `@see` references
to related methods (e.g., send_wr_, recv_rd_, reactor registration). Ensure each
function's docblock follows the project header style and includes
ownership/lifetime notes for pointer parameters and the semantics of inline vs
queued completions.
- Around line 20-25: Add a file-level block comment immediately after the
includes in reactor_datagram_socket.hpp that gives a high-level maintainer
overview of the datagram reactor implementation: briefly describe the
speculative syscalls behavior, the inline-budget dispatch strategy, and the
reactor fallback path (when/why it is used), and note any non-obvious control
flow or threading/ordering assumptions; place this comment before the namespace
boost::corosio::detail and reference the main conceptual pieces (speculative
syscall logic, inline-budget dispatch, reactor fallback) so future readers can
quickly understand the tricky behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cf965553-ad2e-437b-aa85-839395047929
📒 Files selected for processing (3)
include/boost/corosio/native/detail/iocp/win_udp_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_udp_service.hppinclude/boost/corosio/native/detail/reactor/reactor_datagram_socket.hpp
2 participants
Resolves #205.
Summary by CodeRabbit
New Features
Refactor