Add Unix domain socket support and collapse reactor backends#228
Open
mvandeberg wants to merge 1 commit intocppalliance:developfrom
Open
Add Unix domain socket support and collapse reactor backends#228mvandeberg wants to merge 1 commit intocppalliance:developfrom
mvandeberg wants to merge 1 commit intocppalliance:developfrom
Conversation
📝 WalkthroughWalkthroughUmbrella header extended; reactor backend refactored into trait-driven parameterized reactor templates and finals; POSIX local stream/datagram public APIs and services added; many legacy per-backend op/service headers removed; awaitable/op base, message_flags, and shutdown_type introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Sock as local_datagram_socket
participant Reactor as reactor_dgram_socket_final (Traits)
participant Scheduler as Scheduler (epoll/select/kqueue)
participant OS as OS Kernel
App->>+Sock: co_await send_to(buffers, endpoint, flags)
Sock->>+Reactor: dispatch(h, executor) with flags
Reactor->>OS: sendmsg(fd, iov, native_flags)
OS-->>Reactor: result / EAGAIN
alt immediate completion
Reactor->>Scheduler: complete inline -> resume coroutine
else would-block
Reactor->>Scheduler: register_op(write)
Scheduler->>OS: wait for writable
OS-->>Scheduler: fd writable
Scheduler->>Reactor: operator()() -> resume coroutine
end
sequenceDiagram
participant App as Application
participant Acceptor as local_stream_acceptor
participant ReactorAcc as reactor_acceptor_final (Traits)
participant AcceptorSvc as acceptor service
participant Scheduler as Scheduler
participant OS as OS Kernel
App->>+Acceptor: open(local_stream)
Acceptor->>+AcceptorSvc: open_acceptor_socket(family,type,protocol)
AcceptorSvc->>OS: socket + configure
App->>+Acceptor: co_await accept()
Acceptor->>+ReactorAcc: accept(h, executor)
ReactorAcc->>OS: accept syscall
OS-->>ReactorAcc: peer_fd / EAGAIN
alt accepted
ReactorAcc->>Scheduler: setup accepted descriptor -> resume coroutine with peer_socket
else would-block
ReactorAcc->>Scheduler: link accept_op to read_op -> wait -> resume upon readable
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
GCOVR code coverage report https://228.corosio.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-04-10 22:40:32 UTC |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
include/boost/corosio/detail/io_op_base.hpp (1)
13-22: Add the file-level overview block here.This header now centralizes the stop-token, error-code, and completion-state plumbing for multiple awaitables, but it jumps straight from the includes into declarations. A short
/* */overview after the includes would make the dispatch/lifetime model much easier to reason about when touching the backend call sites later.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/detail/io_op_base.hpp` around lines 13 - 22, Add a file-level /* */ overview comment immediately after the includes that explains the header's role in centralizing stop-token handling, std::error_code propagation, and completion-state dispatch for awaitables in namespace boost::corosio::detail; describe the lifecycle model (who owns the stop_token, how/when errors are set and propagated, which path signals completion vs cancellation), note any expectations callers must meet (e.g., thread/context for resumption), and reference the key abstractions used by this file (stop_token, std::error_code, completion-state/awaitable dispatch and any io_op_base-style types) so future readers can quickly understand dispatch and lifetime responsibilities before diving into the implementations.src/corosio/src/io_context.cpp (1)
18-19: Add the required high-level/* */file overview.This file now coordinates backend selection, service pre-creation, and scheduler tuning, but it still only has a terse
//note after the includes. Please replace it with the repository-standard block overview so the construction order and backend split are documented in one place.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 18 - 19, Add a repository-standard multi-line C-style comment block after the includes in io_context.cpp that documents the file's high-level purpose: explain that this translation unit coordinates backend selection (via reactor_backend.hpp / backend.hpp and the IOCP-specific includes), describes the construction order (service pre-creation before scheduler/loop start), and summarizes scheduler tuning decisions and where backend-specific split points occur; place the block immediately after the include section and reference the relevant symbols (reactor_backend.hpp, backend.hpp, IOCP) and the functions/classes that perform service pre-creation and scheduler setup so future readers can quickly understand the file flow.src/corosio/src/local_socket_pair.cpp (1)
28-38: Uncheckedfcntl()return values may hide configuration failures.The fallback path for platforms without
SOCK_NONBLOCKignoresfcntl()return values. While failures here are rare, silently ignoring them could leave sockets in blocking mode, causing unexpected hangs in async operations.♻️ Proposed fix to propagate fcntl errors
void make_nonblock_cloexec(int fd) { `#ifndef` SOCK_NONBLOCK int fl = ::fcntl(fd, F_GETFL, 0); + if (fl == -1) + throw std::system_error( + std::error_code(errno, std::system_category()), + "fcntl F_GETFL"); - ::fcntl(fd, F_SETFL, fl | O_NONBLOCK); - ::fcntl(fd, F_SETFD, FD_CLOEXEC); + if (::fcntl(fd, F_SETFL, fl | O_NONBLOCK) == -1) + throw std::system_error( + std::error_code(errno, std::system_category()), + "fcntl F_SETFL"); + if (::fcntl(fd, F_SETFD, FD_CLOEXEC) == -1) + throw std::system_error( + std::error_code(errno, std::system_category()), + "fcntl F_SETFD"); `#else` (void)fd; `#endif` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/corosio/src/local_socket_pair.cpp` around lines 28 - 38, The make_nonblock_cloexec fallback currently ignores ::fcntl() return values; update it to check each ::fcntl() call (F_GETFL, F_SETFL, F_SETFD) for -1 and propagate the error instead of silently continuing—e.g., on failure capture errno and either throw a std::system_error (or return/abort according to the surrounding error model) with a descriptive message including the operation and fd so callers know configuration failed; ensure this change is applied inside make_nonblock_cloexec so any platform lacking SOCK_NONBLOCK surfaces fcntl failures.include/boost/corosio/native/detail/reactor/reactor_op_complete.hpp (1)
122-162:setup_accepted_socketusessizeof(peer_storage)instead of actual peer address length.The function receives
sockaddr_storage const& peer_storagebut not the actualsocklen_treturned byaccept(). Usingsizeof(peer_storage)(Line 155) works for most cases but is inconsistent with howcomplete_datagram_opcorrectly usesop.source_addrlen. ForAF_UNIXwith abstract sockets, the actual length is significant.Consider passing
socklen_t peer_addrlenas a parameter for consistency with the datagram path.♻️ Proposed signature change
template<typename SocketImpl, typename AcceptorImpl> bool setup_accepted_socket( AcceptorImpl* acceptor_impl, int& accepted_fd, sockaddr_storage const& peer_storage, + socklen_t peer_addrlen, io_object::implementation** impl_out, std::error_code* ec_out) { // ... impl.set_endpoints( acceptor_impl->local_endpoint(), from_sockaddr_as( peer_storage, - static_cast<socklen_t>(sizeof(peer_storage)), + peer_addrlen, ep_type{}));🤖 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_op_complete.hpp` around lines 122 - 162, The setup_accepted_socket function uses sizeof(peer_storage) when calling from_sockaddr_as which loses the actual peer address length (important for AF_UNIX abstract sockets); change the signature of setup_accepted_socket to accept a socklen_t peer_addrlen parameter (e.g. setup_accepted_socket(..., sockaddr_storage const& peer_storage, socklen_t peer_addrlen, ...)), pass peer_addrlen into from_sockaddr_as instead of sizeof(peer_storage), and update all callers (e.g. where accept() results are processed) to forward the actual addrlen they receive; also validate non-zero/within-bounds peer_addrlen where appropriate before calling from_sockaddr_as.include/boost/corosio/native/detail/reactor/reactor_basic_socket.hpp (1)
419-484: Consider extracting shared cancel/claim logic.
do_release_socket()duplicates the cancel/claim pattern fromdo_close_socket(). This is intentional (release doesn't call::close()), but the ~35 lines of identical cancel/claim logic could be extracted into a helper to reduce duplication.Since both methods share the same cancellation semantics and the only difference is the final fd handling, a private helper like
cancel_and_claim_ops()could be factored out.🤖 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_basic_socket.hpp` around lines 419 - 484, Extract the duplicated cancel/claim logic from do_release_socket() and do_close_socket() into a private helper (suggest name cancel_and_claim_ops()) that captures the sequence: lock weak_from_this()->lock(), call Derived::for_each_op to request_cancel(), lock desc_state_.mutex, move reactor_op_base* entries out of descriptor slots into a local claimed array, reset desc_state_ flags (read_ready, write_ready, read_cancel_pending, write_cancel_pending, connect_cancel_pending), set impl_ref_ when is_enqueued_ is true, then after the mutex loop set each claimed[i].base->impl_ptr = self and call svc_.post(...) followed by svc_.work_finished(); replace the duplicated blocks in both do_release_socket() and do_close_socket() with calls to this helper and keep the socket-specific behavior (do_release_socket must NOT close fd_ while do_close_socket still closes) so only the final fd handling remains in each function.include/boost/corosio/local_stream_acceptor.hpp (1)
159-169: Consider throwing instead of asserting on cross-context move-assign.The
asserton line 161 will only trigger in debug builds. In release builds, moving from an acceptor with a differentexecution_contextwould silently corrupt state. Consider throwingstd::logic_errorfor a consistent debug/release behavior.💡 Suggested fix: Throw instead of assert
local_stream_acceptor& operator=(local_stream_acceptor&& other) noexcept { - assert(&ctx_ == &other.ctx_ && - "move-assign requires the same execution_context"); + if (&ctx_ != &other.ctx_) + detail::throw_logic_error( + "move-assign requires the same execution_context"); if (this != &other) { close(); io_object::operator=(std::move(other)); } return *this; }Note: This would require removing
noexceptfrom the signature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/local_stream_acceptor.hpp` around lines 159 - 169, The move-assignment operator for local_stream_acceptor currently uses assert to enforce same execution_context (assert(&ctx_ == &other.ctx_)), which only fires in debug builds; change this to throw a std::logic_error when &ctx_ != &other.ctx_ to ensure consistent behavior in release builds, remove noexcept from local_stream_acceptor& operator=(local_stream_acceptor&& other) noexcept so it can throw, and keep the existing close() and io_object::operator=(std::move(other)) logic unchanged after the check; update any exception-safe invariants or documentation for local_stream_acceptor accordingly.include/boost/corosio/native/detail/reactor/reactor_datagram_socket.hpp (1)
27-36: Consider using named constants for portable flag values.The magic numbers
1,2,4work correctly but could be more self-documenting by referencing themessage_flagsenum values directly or adding a brief comment mapping them.💡 Optional: Add brief comment or use named constants
inline int to_native_msg_flags(int flags) noexcept { int native = 0; - if (flags & 1) native |= MSG_PEEK; - if (flags & 2) native |= MSG_OOB; - if (flags & 4) native |= MSG_DONTROUTE; + if (flags & 1) native |= MSG_PEEK; // message_flags::peek + if (flags & 2) native |= MSG_OOB; // message_flags::out_of_band + if (flags & 4) native |= MSG_DONTROUTE; // message_flags::dont_route return native; }🤖 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 27 - 36, The function to_native_msg_flags currently uses magic numbers (1, 2, 4); replace those literals with the corresponding portable message_flags enum constants (or define named constants) and/or add a one-line comment mapping each portable flag to its MSG_* counterpart so the intent is clear (e.g., use message_flags::peek, message_flags::oob, message_flags::dontroute or named constexprs in the checks and then set MSG_PEEK, MSG_OOB, MSG_DONTROUTE accordingly).
🤖 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/io/io_read_stream.hpp`:
- Around line 50-64: Update the public documentation for
io_read_stream::read_some to state its cancellation contract: explain that the
operation supports cancellation via the stop_token propagated through the
IoAwaitable protocol and via the i/o object's cancel() member, and that when
cancelled the resulting error_code (the first result) compares equal to
capy::cond::canceled; reference the awaitable type read_some_awaitable and the
shared base detail::io_bytes_op_base which now handles stop-token plumbing so
readers know cancellation can originate from the propagated stop_token or
io_read_stream::cancel().
In `@include/boost/corosio/io/io_write_stream.hpp`:
- Around line 50-64: Update the public documentation for write_some to state the
cancellation contract: document that write_some (and its awaitable type
write_some_awaitable used by io_write_stream) supports cancellation via the
propagated stop_token through the IoAwaitable protocol and via the i/o object's
cancel() member, and that when cancelled the operation completes with an error
value that compares equal to capy::cond::canceled; reference the involved
symbols in the docs (write_some, write_some_awaitable, io_write_stream,
do_write_some, and the shared io_bytes_op_base/token_ mechanism) so callers know
where cancellation is handled and what to expect.
In `@include/boost/corosio/local_endpoint.hpp`:
- Around line 78-89: The doc comment for is_abstract() incorrectly shows a
double-escaped null character ('\\0'); update the `@return` line in the comment
above bool is_abstract() const noexcept to display the single-escaped null
character representation ('\0') so generated docs render correctly (no code
changes to is_abstract(), just fix the text in the comment referencing path_ and
len_).
In `@include/boost/corosio/local_stream_socket.hpp`:
- Around line 136-144: The move assignment operator for local_stream_socket
currently assigns h_ directly and bypasses base-class logic; change
local_stream_socket::operator=(local_stream_socket&& other) noexcept to call the
base-class move assignment (io_object::operator=(std::move(other))) instead of
assigning h_ directly, mirroring how the move constructor delegates to
io_object(std::move(other)) and matching the pattern used by
local_datagram_socket.
In `@include/boost/corosio/local_stream.hpp`:
- Around line 20-49: Replace the plain block comment above class local_stream
with a proper API docstring that briefly describes its purpose (encapsulates the
Unix domain stream protocol), documents the return semantics of family(),
type(), and protocol(), lists the associated types socket and acceptor
(local_stream_socket, local_stream_acceptor), and explicitly states the
POSIX-only precondition (this protocol is meaningful only on POSIX
builds/AF_UNIX-supporting platforms); also mention relation to
native_local_stream as an inline variant. Ensure the docstring documents
preconditions (POSIX only) and return values for family()/type()/protocol().
In `@include/boost/corosio/message_flags.hpp`:
- Around line 15-47: Add public header docstrings for the message_flags enum and
its bitmask operators to satisfy the documentation guideline: replace the block
comment with Doxygen-style (or project-standard) brief and detailed descriptions
for enum class message_flags explaining each flag's meaning (none, peek,
out_of_band, do_not_route) and that values are platform-agnostic and mapped at
the syscall boundary, and add docstrings for the exported bitwise operators
operator|(message_flags, message_flags), operator&(message_flags, message_flags)
and operator~(message_flags) describing that they perform bitmask combination,
intersection, and bitwise inversion respectively and return a message_flags
result; ensure the documentation is placed in the public header (not a detail
namespace) and follows the project's docstring style so generated references
capture the API and semantics.
---
Nitpick comments:
In `@include/boost/corosio/detail/io_op_base.hpp`:
- Around line 13-22: Add a file-level /* */ overview comment immediately after
the includes that explains the header's role in centralizing stop-token
handling, std::error_code propagation, and completion-state dispatch for
awaitables in namespace boost::corosio::detail; describe the lifecycle model
(who owns the stop_token, how/when errors are set and propagated, which path
signals completion vs cancellation), note any expectations callers must meet
(e.g., thread/context for resumption), and reference the key abstractions used
by this file (stop_token, std::error_code, completion-state/awaitable dispatch
and any io_op_base-style types) so future readers can quickly understand
dispatch and lifetime responsibilities before diving into the implementations.
In `@include/boost/corosio/local_stream_acceptor.hpp`:
- Around line 159-169: The move-assignment operator for local_stream_acceptor
currently uses assert to enforce same execution_context (assert(&ctx_ ==
&other.ctx_)), which only fires in debug builds; change this to throw a
std::logic_error when &ctx_ != &other.ctx_ to ensure consistent behavior in
release builds, remove noexcept from local_stream_acceptor&
operator=(local_stream_acceptor&& other) noexcept so it can throw, and keep the
existing close() and io_object::operator=(std::move(other)) logic unchanged
after the check; update any exception-safe invariants or documentation for
local_stream_acceptor accordingly.
In `@include/boost/corosio/native/detail/reactor/reactor_basic_socket.hpp`:
- Around line 419-484: Extract the duplicated cancel/claim logic from
do_release_socket() and do_close_socket() into a private helper (suggest name
cancel_and_claim_ops()) that captures the sequence: lock
weak_from_this()->lock(), call Derived::for_each_op to request_cancel(), lock
desc_state_.mutex, move reactor_op_base* entries out of descriptor slots into a
local claimed array, reset desc_state_ flags (read_ready, write_ready,
read_cancel_pending, write_cancel_pending, connect_cancel_pending), set
impl_ref_ when is_enqueued_ is true, then after the mutex loop set each
claimed[i].base->impl_ptr = self and call svc_.post(...) followed by
svc_.work_finished(); replace the duplicated blocks in both do_release_socket()
and do_close_socket() with calls to this helper and keep the socket-specific
behavior (do_release_socket must NOT close fd_ while do_close_socket still
closes) so only the final fd handling remains in each function.
In `@include/boost/corosio/native/detail/reactor/reactor_datagram_socket.hpp`:
- Around line 27-36: The function to_native_msg_flags currently uses magic
numbers (1, 2, 4); replace those literals with the corresponding portable
message_flags enum constants (or define named constants) and/or add a one-line
comment mapping each portable flag to its MSG_* counterpart so the intent is
clear (e.g., use message_flags::peek, message_flags::oob,
message_flags::dontroute or named constexprs in the checks and then set
MSG_PEEK, MSG_OOB, MSG_DONTROUTE accordingly).
In `@include/boost/corosio/native/detail/reactor/reactor_op_complete.hpp`:
- Around line 122-162: The setup_accepted_socket function uses
sizeof(peer_storage) when calling from_sockaddr_as which loses the actual peer
address length (important for AF_UNIX abstract sockets); change the signature of
setup_accepted_socket to accept a socklen_t peer_addrlen parameter (e.g.
setup_accepted_socket(..., sockaddr_storage const& peer_storage, socklen_t
peer_addrlen, ...)), pass peer_addrlen into from_sockaddr_as instead of
sizeof(peer_storage), and update all callers (e.g. where accept() results are
processed) to forward the actual addrlen they receive; also validate
non-zero/within-bounds peer_addrlen where appropriate before calling
from_sockaddr_as.
In `@src/corosio/src/io_context.cpp`:
- Around line 18-19: Add a repository-standard multi-line C-style comment block
after the includes in io_context.cpp that documents the file's high-level
purpose: explain that this translation unit coordinates backend selection (via
reactor_backend.hpp / backend.hpp and the IOCP-specific includes), describes the
construction order (service pre-creation before scheduler/loop start), and
summarizes scheduler tuning decisions and where backend-specific split points
occur; place the block immediately after the include section and reference the
relevant symbols (reactor_backend.hpp, backend.hpp, IOCP) and the
functions/classes that perform service pre-creation and scheduler setup so
future readers can quickly understand the file flow.
In `@src/corosio/src/local_socket_pair.cpp`:
- Around line 28-38: The make_nonblock_cloexec fallback currently ignores
::fcntl() return values; update it to check each ::fcntl() call (F_GETFL,
F_SETFL, F_SETFD) for -1 and propagate the error instead of silently
continuing—e.g., on failure capture errno and either throw a std::system_error
(or return/abort according to the surrounding error model) with a descriptive
message including the operation and fd so callers know configuration failed;
ensure this change is applied inside make_nonblock_cloexec so any platform
lacking SOCK_NONBLOCK surfaces fcntl failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f6f51880-71b6-4bcd-bd67-f593153ee059
⛔ Files ignored due to path filters (19)
doc/modules/ROOT/nav.adocis excluded by!**/doc/**doc/modules/ROOT/pages/4.guide/4a.tcp-networking.adocis excluded by!**/doc/**doc/modules/ROOT/pages/4.guide/4p.unix-sockets.adocis excluded by!**/doc/**doc/modules/ROOT/pages/glossary.adocis excluded by!**/doc/**doc/modules/ROOT/pages/index.adocis excluded by!**/doc/**perf/bench/CMakeLists.txtis excluded by!**/bench/**perf/bench/asio/callback/benchmarks.hppis excluded by!**/bench/**perf/bench/asio/callback/unix_socket_latency_bench.cppis excluded by!**/bench/**perf/bench/asio/callback/unix_socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/asio/coroutine/benchmarks.hppis excluded by!**/bench/**perf/bench/asio/coroutine/unix_socket_latency_bench.cppis excluded by!**/bench/**perf/bench/asio/coroutine/unix_socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/asio/unix_socket_utils.hppis excluded by!**/bench/**perf/bench/corosio/benchmarks.hppis excluded by!**/bench/**perf/bench/corosio/unix_socket_latency_bench.cppis excluded by!**/bench/**perf/bench/corosio/unix_socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/main.cppis excluded by!**/bench/**test/unit/local_datagram_socket.cppis excluded by!**/test/**test/unit/local_stream_socket.cppis excluded by!**/test/**
📒 Files selected for processing (74)
include/boost/corosio.hppinclude/boost/corosio/backend.hppinclude/boost/corosio/detail/io_op_base.hppinclude/boost/corosio/detail/local_datagram_service.hppinclude/boost/corosio/detail/local_stream_acceptor_service.hppinclude/boost/corosio/detail/local_stream_service.hppinclude/boost/corosio/io/io_object.hppinclude/boost/corosio/io/io_read_stream.hppinclude/boost/corosio/io/io_write_stream.hppinclude/boost/corosio/local_datagram.hppinclude/boost/corosio/local_datagram_socket.hppinclude/boost/corosio/local_endpoint.hppinclude/boost/corosio/local_socket_pair.hppinclude/boost/corosio/local_stream.hppinclude/boost/corosio/local_stream_acceptor.hppinclude/boost/corosio/local_stream_socket.hppinclude/boost/corosio/message_flags.hppinclude/boost/corosio/native/detail/endpoint_convert.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_traits.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_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_traits.hppinclude/boost/corosio/native/detail/kqueue/kqueue_udp_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_udp_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_acceptor.hppinclude/boost/corosio/native/detail/reactor/reactor_acceptor_service.hppinclude/boost/corosio/native/detail/reactor/reactor_backend.hppinclude/boost/corosio/native/detail/reactor/reactor_basic_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_datagram_ops.hppinclude/boost/corosio/native/detail/reactor/reactor_datagram_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_op.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_finals.hppinclude/boost/corosio/native/detail/reactor/reactor_socket_finals.hppinclude/boost/corosio/native/detail/reactor/reactor_socket_service.hppinclude/boost/corosio/native/detail/reactor/reactor_stream_ops.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_traits.hppinclude/boost/corosio/native/detail/select/select_udp_service.hppinclude/boost/corosio/native/detail/select/select_udp_socket.hppinclude/boost/corosio/native/native_tcp_acceptor.hppinclude/boost/corosio/native/native_tcp_socket.hppinclude/boost/corosio/native/native_udp_socket.hppinclude/boost/corosio/shutdown_type.hppinclude/boost/corosio/tcp_socket.hppinclude/boost/corosio/udp_socket.hppperf/common/native_includes.hppsrc/corosio/src/io_context.cppsrc/corosio/src/local_datagram.cppsrc/corosio/src/local_datagram_socket.cppsrc/corosio/src/local_endpoint.cppsrc/corosio/src/local_socket_pair.cppsrc/corosio/src/local_stream.cppsrc/corosio/src/local_stream_acceptor.cppsrc/corosio/src/local_stream_socket.cpp
💤 Files with no reviewable changes (23)
- include/boost/corosio/native/native_tcp_acceptor.hpp
- include/boost/corosio/native/native_tcp_socket.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor.hpp
- include/boost/corosio/native/detail/select/select_tcp_acceptor.hpp
- include/boost/corosio/native/detail/select/select_tcp_acceptor_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_acceptor.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_service.hpp
- include/boost/corosio/native/detail/select/select_udp_socket.hpp
- include/boost/corosio/native/detail/epoll/epoll_udp_service.hpp
- include/boost/corosio/native/detail/select/select_tcp_socket.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor_service.hpp
- include/boost/corosio/native/detail/select/select_udp_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_socket.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_socket.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_udp_socket.hpp
- include/boost/corosio/native/detail/select/select_op.hpp
- include/boost/corosio/native/detail/select/select_tcp_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_op.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_udp_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_acceptor_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_udp_socket.hpp
- include/boost/corosio/native/detail/epoll/epoll_op.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_service.hpp
mvandeberg
force-pushed
the
pr/122-unix-sockets-2
branch
from
77fc9aa to
d63af20
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/boost/corosio/native/detail/reactor/reactor_stream_socket.hpp (1)
219-228:⚠️ Potential issue | 🟡 MinorClear
remote_endpoint_on release as well.
do_close_socket()resets the cached peer, but the new release path comes fromreactor_basic_socket::do_release_socket()and only clears the local endpoint. Afterrelease_socket()the object is no longer open, yetremote_endpoint()can still report the previous peer.🤖 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 219 - 228, The release path forgets to clear the cached peer: add an override for do_release_socket() in this class that calls the base implementation (reactor_basic_socket::do_release_socket() / base_type::do_release_socket()) and then resets remote_endpoint_ = Endpoint{} so that after release_socket() remote_endpoint() no longer reports the previous peer; keep the method noexcept and mirror the pattern used in do_close_socket().
♻️ Duplicate comments (1)
include/boost/corosio/local_stream_socket.hpp (1)
136-142:⚠️ Potential issue | 🟠 MajorUse base move assignment instead of directly moving
h_.Line 141 bypasses
io_object::operator=(...), which can skip base-class move logic and diverges from the move-constructor pattern.Proposed fix
local_stream_socket& operator=(local_stream_socket&& other) noexcept { if (this != &other) { - close(); - h_ = std::move(other.h_); + io_object::operator=(std::move(other)); } return *this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/local_stream_socket.hpp` around lines 136 - 142, The move-assignment for local_stream_socket currently assigns h_ directly, bypassing base-class move logic; update local_stream_socket::operator=(local_stream_socket&&) noexcept to invoke the base class move-assignment (io_object::operator=(std::move(other))) before or instead of moving h_ so the io_object state is transferred consistently with the move-constructor, then handle any socket-specific cleanup (close()) and avoid direct assignment to h_ after the base move.
🧹 Nitpick comments (3)
include/boost/corosio/native/detail/epoll/epoll_traits.hpp (1)
103-134: Consider logging ignoredsetsockopterrors inconfigure_ip_*functions.The
configure_ip_socket()andconfigure_ip_acceptor()functions ignore the return value ofsetsockopt()forIPV6_V6ONLY. While this is likely intentional (the option is non-fatal), silent failures could complicate debugging dual-stack connectivity issues.If this is intentional design, a brief comment explaining why errors are ignored would help future maintainers.
🤖 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_traits.hpp` around lines 103 - 134, Check the return value of ::setsockopt calls in configure_ip_socket and configure_ip_acceptor (the calls that set IPV6_V6ONLY for AF_INET6); if setsockopt fails, either log the error (including errno) or convert it to and return a std::error_code (e.g., from errno using std::error_code(errno, std::generic_category())) so failures aren’t silently dropped — if swallowing is intentional, add a short comment in configure_ip_socket and configure_ip_acceptor explaining why the error is intentionally ignored.include/boost/corosio/local_stream_socket.hpp (1)
212-219: Document theshutdown_sendread-side EOF postcondition explicitly.Please state the expected post-shutdown read behavior (explicit EOF error condition) in this API contract so behavior is unambiguous for callers and tests.
Based on learnings: In boost::corosio,
socket::shutdown(shutdown_send)requires subsequent reads to reportcapy::cond::eofexplicitly, not only zero-byte success.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/local_stream_socket.hpp` around lines 212 - 219, The shutdown overloads lack an explicit postcondition for shutdown_send; update the API doc for void shutdown(shutdown_type what) and void shutdown(shutdown_type what, std::error_code& ec) noexcept to state that calling shutdown(shutdown_send) will cause subsequent read operations to report an explicit EOF condition (capy::cond::eof) rather than merely succeeding with zero bytes, so callers/tests know to expect capy::cond::eof on reads after shutdown_send.include/boost/corosio/native/detail/kqueue/kqueue_traits.hpp (1)
101-113: Consider adding a brief comment explaining the writev choice.Unlike
select_traitswhich usessendmsg()withMSG_NOSIGNAL, this useswritev()because macOS lacksMSG_NOSIGNALand relies onSO_NOSIGPIPEinstead (set inset_fd_options). A one-line comment would help maintainers understand this platform-specific choice.📝 Suggested comment
struct write_policy { + // writev() used instead of sendmsg(MSG_NOSIGNAL); macOS relies on SO_NOSIGPIPE. static ssize_t write(int fd, iovec* iovecs, int count) noexcept {🤖 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_traits.hpp` around lines 101 - 113, Add a one-line comment above the write_policy::write implementation explaining why writev() is used instead of sendmsg() (macOS lacks MSG_NOSIGNAL and the code relies on SO_NOSIGPIPE set in set_fd_options), referencing the platform-specific choice and that select_traits uses sendmsg()+MSG_NOSIGNAL for contrast; this will help future maintainers understand the rationale without changing behavior.
🤖 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/local_stream_acceptor.hpp`:
- Around line 224-242: The two accept overloads (accept(local_stream_socket&
peer) and accept()) currently guard only with is_open(), which allows calling
accept after open()/bind() but before listen(), leading to backend null-deref;
change the guard to check the listening state (use is_listening() or equivalent)
instead of is_open(), and keep the same detail::throw_logic_error("accept:
acceptor not listening") behavior; update both callers that return
accept_awaitable(*this, peer) and move_accept_awaitable(*this) to be behind the
is_listening() check so accept() throws as documented when not listening.
- Around line 36-58: Add full public API documentation for local_stream_acceptor
and its operations (e.g., bind(), accept()) in this header: include completion
conditions (what success and error outcomes complete the awaitable),
cancellation behavior (what happens on coroutine cancellation or reactor
shutdown), notable error cases (EADDRINUSE, ENOENT, EACCES, ECONNABORTED, etc.),
and a short `@par` Example showing typical bind/unlink/accept usage; ensure
class-level docstring and each public function (including enum bind_option)
include these sections to meet the repository guideline that all public headers
in non-detail namespaces have docstrings documenting Completion conditions,
Cancellation, and Example.
In `@include/boost/corosio/local_stream_socket.hpp`:
- Around line 206-209: The release() documentation is contradictory about
whether it returns -1 when not open or throws std::logic_error; decide on a
single contract and make docs and implementation consistent: either (A) keep the
non-throwing behavior—remove the "@throws std::logic_error" sentence, ensure
local_stream_socket::release() returns -1 when not open and document that
explicitly, or (B) keep the throwing behavior—remove the "or -1 if not open"
note, modify local_stream_socket::release() to throw std::logic_error when the
socket is not open, and update the docblock to state it throws; apply the chosen
change to the release() docstring and to the release() implementation and any
callers that rely on its behavior.
In `@include/boost/corosio/native/detail/iocp/win_udp_service.hpp`:
- Around line 395-398: The IOCP UDP backend in win_udp_service.hpp passes
portable message_flags directly to WinSock calls (WSASendTo, WSARecvFrom,
WSASend, WSARecv) causing peek/out_of_band to be inverted; add a
to_native_msg_flags(unsigned flags) helper (mirroring reactor's implementation)
that maps portable peek=1 -> MSG_PEEK and out_of_band=2 -> MSG_OOB and use it to
transform the flags argument before calling
WSASendTo/WSARecvFrom/WSASend/WSARecv (e.g., replace static_cast<DWORD>(flags)
with static_cast<DWORD>(to_native_msg_flags(flags))) so WinSock receives the
correct native constants.
In `@include/boost/corosio/native/detail/reactor/reactor_backend.hpp`:
- Around line 54-89: The code reconstructs the peer endpoint using
sizeof(peer_storage) instead of the actual addrlen returned by accept; update
Traits::accept_policy::do_accept (and the reactor_accept_op that calls it) to
return or output the real socklen_t (e.g., via an out-param or pair return)
alongside the accepted fd, then use that real addrlen when calling
from_sockaddr_as(...) instead of static_cast<socklen_t>(sizeof(peer_storage)) so
Unix-domain socket paths (abstract or non-null-terminated) are preserved; ensure
the new return/out parameter is propagated from accept_policy::do_accept through
reactor_accept_op to the call site where peer_storage and from_sockaddr_as are
used.
---
Outside diff comments:
In `@include/boost/corosio/native/detail/reactor/reactor_stream_socket.hpp`:
- Around line 219-228: The release path forgets to clear the cached peer: add an
override for do_release_socket() in this class that calls the base
implementation (reactor_basic_socket::do_release_socket() /
base_type::do_release_socket()) and then resets remote_endpoint_ = Endpoint{} so
that after release_socket() remote_endpoint() no longer reports the previous
peer; keep the method noexcept and mirror the pattern used in do_close_socket().
---
Duplicate comments:
In `@include/boost/corosio/local_stream_socket.hpp`:
- Around line 136-142: The move-assignment for local_stream_socket currently
assigns h_ directly, bypassing base-class move logic; update
local_stream_socket::operator=(local_stream_socket&&) noexcept to invoke the
base class move-assignment (io_object::operator=(std::move(other))) before or
instead of moving h_ so the io_object state is transferred consistently with the
move-constructor, then handle any socket-specific cleanup (close()) and avoid
direct assignment to h_ after the base move.
---
Nitpick comments:
In `@include/boost/corosio/local_stream_socket.hpp`:
- Around line 212-219: The shutdown overloads lack an explicit postcondition for
shutdown_send; update the API doc for void shutdown(shutdown_type what) and void
shutdown(shutdown_type what, std::error_code& ec) noexcept to state that calling
shutdown(shutdown_send) will cause subsequent read operations to report an
explicit EOF condition (capy::cond::eof) rather than merely succeeding with zero
bytes, so callers/tests know to expect capy::cond::eof on reads after
shutdown_send.
In `@include/boost/corosio/native/detail/epoll/epoll_traits.hpp`:
- Around line 103-134: Check the return value of ::setsockopt calls in
configure_ip_socket and configure_ip_acceptor (the calls that set IPV6_V6ONLY
for AF_INET6); if setsockopt fails, either log the error (including errno) or
convert it to and return a std::error_code (e.g., from errno using
std::error_code(errno, std::generic_category())) so failures aren’t silently
dropped — if swallowing is intentional, add a short comment in
configure_ip_socket and configure_ip_acceptor explaining why the error is
intentionally ignored.
In `@include/boost/corosio/native/detail/kqueue/kqueue_traits.hpp`:
- Around line 101-113: Add a one-line comment above the write_policy::write
implementation explaining why writev() is used instead of sendmsg() (macOS lacks
MSG_NOSIGNAL and the code relies on SO_NOSIGPIPE set in set_fd_options),
referencing the platform-specific choice and that select_traits uses
sendmsg()+MSG_NOSIGNAL for contrast; this will help future maintainers
understand the rationale without changing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 30d53a5d-2df5-4996-a88b-a16c9feaa3ee
⛔ Files ignored due to path filters (19)
doc/modules/ROOT/nav.adocis excluded by!**/doc/**doc/modules/ROOT/pages/4.guide/4a.tcp-networking.adocis excluded by!**/doc/**doc/modules/ROOT/pages/4.guide/4p.unix-sockets.adocis excluded by!**/doc/**doc/modules/ROOT/pages/glossary.adocis excluded by!**/doc/**doc/modules/ROOT/pages/index.adocis excluded by!**/doc/**perf/bench/CMakeLists.txtis excluded by!**/bench/**perf/bench/asio/callback/benchmarks.hppis excluded by!**/bench/**perf/bench/asio/callback/unix_socket_latency_bench.cppis excluded by!**/bench/**perf/bench/asio/callback/unix_socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/asio/coroutine/benchmarks.hppis excluded by!**/bench/**perf/bench/asio/coroutine/unix_socket_latency_bench.cppis excluded by!**/bench/**perf/bench/asio/coroutine/unix_socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/asio/unix_socket_utils.hppis excluded by!**/bench/**perf/bench/corosio/benchmarks.hppis excluded by!**/bench/**perf/bench/corosio/unix_socket_latency_bench.cppis excluded by!**/bench/**perf/bench/corosio/unix_socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/main.cppis excluded by!**/bench/**test/unit/local_datagram_socket.cppis excluded by!**/test/**test/unit/local_stream_socket.cppis excluded by!**/test/**
📒 Files selected for processing (74)
include/boost/corosio.hppinclude/boost/corosio/backend.hppinclude/boost/corosio/detail/local_datagram_service.hppinclude/boost/corosio/detail/local_stream_acceptor_service.hppinclude/boost/corosio/detail/local_stream_service.hppinclude/boost/corosio/detail/op_base.hppinclude/boost/corosio/io/io_object.hppinclude/boost/corosio/io/io_read_stream.hppinclude/boost/corosio/io/io_write_stream.hppinclude/boost/corosio/local_datagram.hppinclude/boost/corosio/local_datagram_socket.hppinclude/boost/corosio/local_endpoint.hppinclude/boost/corosio/local_socket_pair.hppinclude/boost/corosio/local_stream.hppinclude/boost/corosio/local_stream_acceptor.hppinclude/boost/corosio/local_stream_socket.hppinclude/boost/corosio/message_flags.hppinclude/boost/corosio/native/detail/endpoint_convert.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_traits.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_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_traits.hppinclude/boost/corosio/native/detail/kqueue/kqueue_udp_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_udp_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_acceptor.hppinclude/boost/corosio/native/detail/reactor/reactor_acceptor_service.hppinclude/boost/corosio/native/detail/reactor/reactor_backend.hppinclude/boost/corosio/native/detail/reactor/reactor_basic_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_datagram_ops.hppinclude/boost/corosio/native/detail/reactor/reactor_datagram_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_op.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_finals.hppinclude/boost/corosio/native/detail/reactor/reactor_socket_finals.hppinclude/boost/corosio/native/detail/reactor/reactor_socket_service.hppinclude/boost/corosio/native/detail/reactor/reactor_stream_ops.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_traits.hppinclude/boost/corosio/native/detail/select/select_udp_service.hppinclude/boost/corosio/native/detail/select/select_udp_socket.hppinclude/boost/corosio/native/native_tcp_acceptor.hppinclude/boost/corosio/native/native_tcp_socket.hppinclude/boost/corosio/native/native_udp_socket.hppinclude/boost/corosio/shutdown_type.hppinclude/boost/corosio/tcp_socket.hppinclude/boost/corosio/udp_socket.hppperf/common/native_includes.hppsrc/corosio/src/io_context.cppsrc/corosio/src/local_datagram.cppsrc/corosio/src/local_datagram_socket.cppsrc/corosio/src/local_endpoint.cppsrc/corosio/src/local_socket_pair.cppsrc/corosio/src/local_stream.cppsrc/corosio/src/local_stream_acceptor.cppsrc/corosio/src/local_stream_socket.cpp
💤 Files with no reviewable changes (23)
- include/boost/corosio/native/native_tcp_acceptor.hpp
- include/boost/corosio/native/native_tcp_socket.hpp
- include/boost/corosio/native/detail/select/select_tcp_acceptor_service.hpp
- include/boost/corosio/native/detail/select/select_tcp_acceptor.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor.hpp
- include/boost/corosio/native/detail/select/select_op.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_socket.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_service.hpp
- include/boost/corosio/native/detail/select/select_tcp_socket.hpp
- include/boost/corosio/native/detail/select/select_udp_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_udp_socket.hpp
- include/boost/corosio/native/detail/epoll/epoll_op.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_socket.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_op.hpp
- include/boost/corosio/native/detail/select/select_tcp_service.hpp
- include/boost/corosio/native/detail/select/select_udp_socket.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_acceptor.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_udp_socket.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_udp_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_acceptor_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_udp_service.hpp
✅ Files skipped from review due to trivial changes (17)
- include/boost/corosio.hpp
- include/boost/corosio/native/detail/reactor/reactor_scheduler.hpp
- include/boost/corosio/shutdown_type.hpp
- perf/common/native_includes.hpp
- include/boost/corosio/native/detail/select/select_scheduler.hpp
- src/corosio/src/local_stream.cpp
- include/boost/corosio/message_flags.hpp
- include/boost/corosio/local_stream.hpp
- include/boost/corosio/detail/local_stream_service.hpp
- include/boost/corosio/local_datagram.hpp
- include/boost/corosio/detail/local_datagram_service.hpp
- include/boost/corosio/detail/local_stream_acceptor_service.hpp
- include/boost/corosio/local_socket_pair.hpp
- include/boost/corosio/native/detail/reactor/reactor_stream_ops.hpp
- src/corosio/src/local_datagram.cpp
- include/boost/corosio/native/detail/reactor/reactor_op_complete.hpp
- include/boost/corosio/native/detail/reactor/reactor_datagram_socket.hpp
🚧 Files skipped from review as they are similar to previous changes (16)
- include/boost/corosio/io/io_object.hpp
- include/boost/corosio/native/detail/reactor/reactor_socket_service.hpp
- include/boost/corosio/native/native_udp_socket.hpp
- src/corosio/src/local_socket_pair.cpp
- include/boost/corosio/io/io_write_stream.hpp
- src/corosio/src/local_stream_acceptor.cpp
- include/boost/corosio/native/detail/iocp/win_udp_socket.hpp
- src/corosio/src/local_stream_socket.cpp
- src/corosio/src/local_datagram_socket.cpp
- src/corosio/src/local_endpoint.cpp
- include/boost/corosio/native/detail/endpoint_convert.hpp
- src/corosio/src/io_context.cpp
- include/boost/corosio/native/detail/reactor/reactor_acceptor_service.hpp
- include/boost/corosio/native/detail/reactor/reactor_datagram_ops.hpp
- include/boost/corosio/backend.hpp
- include/boost/corosio/native/detail/reactor/reactor_socket_finals.hpp
include/boost/corosio/native/detail/reactor/reactor_backend.hpp
Outdated
Show resolved
Hide resolved
mvandeberg
force-pushed
the
pr/122-unix-sockets-2
branch
from
d63af20 to
1e6c449
Compare
|
An automated preview of the documentation is available at https://228.corosio.prtest3.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-04-10 22:32:24 UTC |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #228 +/- ##
===========================================
- Coverage 77.85% 77.78% -0.07%
===========================================
Files 96 96
Lines 7350 7289 -61
Branches 1791 1787 -4
===========================================
- Hits 5722 5670 -52
+ Misses 1110 1101 -9
Partials 518 518
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
include/boost/corosio/native/detail/reactor/reactor_service_finals.hpp (1)
66-74:⚠️ Potential issue | 🔴 CriticalRun backend fd normalization before
init_and_register().
do_assign_fd()skips the same trait-specific setup thatdo_open_socket()applies. An adopted descriptor can therefore stay blocking, missFD_CLOEXEC, and bypass select’sFD_SETSIZEguard, which is enough to stall speculative I/O on the reactor thread.🤖 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_service_finals.hpp` around lines 66 - 74, do_assign_fd currently closes and immediately calls socket_impl->init_and_register(fd), skipping the trait-specific backend fd normalization that do_open_socket applies; update do_assign_fd to perform the same normalization step(s) before init_and_register (i.e., run the trait/backend fd setup that enforces non-blocking, FD_CLOEXEC and FD_SETSIZE checks used by do_open_socket) so an adopted descriptor cannot remain blocking or miss CLOEXEC and so select()/reactor guards are preserved; keep calls to socket_impl->close_socket() and socket_impl->init_and_register(fd) but insert the same normalization routine (the trait/backend normalization used by do_open_socket) between them.include/boost/corosio/local_stream_acceptor.hpp (1)
210-243:⚠️ Potential issue | 🟠 MajorSemantic mismatch:
is_open()vs listening state.The comment on line 210 says "Check if the acceptor is listening" but the method name is
is_open()and it only checks if the fd exists. This allowsaccept()to be called afteropen()/bind()but beforelisten(), leading to a backend null-pointer crash (per learnings, this is intentional misuse detection).However, the documentation at line 222/236 promises a
std::logic_error"if the acceptor is not listening", which contradicts the actual crash behavior. Consider either:
- Rename
is_open()tois_listening()if it truly checks listening state- Add a separate
is_listening()check inaccept()that throws as documented- Update the documentation to reflect the actual misuse behavior
Based on learnings: "Calling accept() before listen() is API misuse and will intentionally crash (nullptr dereference) as a loud programming error, not a bug to defensively guard."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/local_stream_acceptor.hpp` around lines 210 - 243, The comments promise accept() throws if the acceptor is not listening, but is_open() only checks the fd and allows calling accept() before listen(), causing a crash; fix by adding a proper is_listening() predicate (e.g., bool is_listening() const noexcept that queries the underlying handle/state) and use is_listening() in both accept(local_stream_socket& peer) and accept() before returning accept_awaitable/move_accept_awaitable so they throw the documented std::logic_error, leaving is_open() unchanged (or alternatively update the docstrings if you opt to keep the current behavior).include/boost/corosio/local_stream_socket.hpp (2)
200-210:⚠️ Potential issue | 🟡 MinorResolve contradictory
release()contract wording.Line 206 says "or -1 if not open", while Line 208 says it throws
std::logic_errorif not open. Please keep a single behavior contract in the docs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/local_stream_socket.hpp` around lines 200 - 210, The documentation for local_stream_socket::release() contains contradictory behavior; update the comment to a single contract: remove "or -1 if not open" and state that release() returns the native_handle_type on success and throws std::logic_error if the socket is not open; ensure the `@return` and `@throws` lines consistently describe that behavior for the release() method.
136-144:⚠️ Potential issue | 🟠 MajorInconsistent move assignment bypasses base class.
The move constructor (line 132) correctly delegates to
io_object(std::move(other)), but this move assignment directly assignsh_instead of calling the base class operator. This could skip any logic inio_object::operator=.🐛 Proposed fix for consistency with base class
local_stream_socket& operator=(local_stream_socket&& other) noexcept { if (this != &other) { close(); - h_ = std::move(other.h_); + io_object::operator=(std::move(other)); } return *this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/local_stream_socket.hpp` around lines 136 - 144, The move assignment in local_stream_socket (local_stream_socket& operator=(local_stream_socket&& other) noexcept) bypasses the base io_object by assigning h_ directly; change it to call the base move-assignment (e.g., io_object::operator=(std::move(other))) after closing the current socket (keep the close() call) and then let the base assign its members instead of manually moving h_, ensuring any io_object::operator= logic is executed.
🧹 Nitpick comments (3)
include/boost/corosio/local_stream_socket.hpp (1)
64-99: Add documentation parity with tcp_socket::implementation.The
implementationinterface methods lack the detailed parameter documentation present intcp_socket::implementation(see context snippet 3, lines 89-158). At minimum,connect()should document its parameters (h,ex,ep,token,ec) and return value semantics.📝 Example documentation for connect
struct implementation : io_stream::implementation { + /** Initiate an asynchronous connect to the given endpoint. + + `@param` h Coroutine handle to resume on completion. + `@param` ex Executor for dispatching the completion. + `@param` ep The local endpoint (path) to connect to. + `@param` token Stop token for cancellation. + `@param` ec Output error code. + + `@return` Coroutine handle to resume immediately. + */ virtual std::coroutine_handle<> connect( std::coroutine_handle<> h, capy::executor_ref ex, corosio::local_endpoint ep, std::stop_token token, std::error_code* ec) = 0;As per coding guidelines: "Docstrings are required for all classes and functions in public headers in non-detail namespaces."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/local_stream_socket.hpp` around lines 64 - 99, The implementation struct lacks docstrings matching tcp_socket::implementation; add Doxygen-style comments for implementation and for each virtual method (at minimum connect) documenting parameters and return semantics: for connect(document h, ex, ep, token, ec and what the returned std::coroutine_handle<> represents and error reporting via ec), and similar brief descriptions for shutdown, native_handle, release_socket, cancel, set_option, get_option, local_endpoint and remote_endpoint so the public header has parity with tcp_socket::implementation.include/boost/corosio/local_stream_acceptor.hpp (1)
286-313: Consider adding parameter documentation to implementation interface.The
implementationstruct methods lack parameter documentation. While this is an internal backend extension point, adding brief docs for theaccept()method signature would help backend implementers understand the contract (coroutine handle semantics, output parameters, etc.).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/local_stream_acceptor.hpp` around lines 286 - 313, The implementation interface currently lacks parameter documentation which makes the backend contract unclear; add brief Doxygen-style comments to the struct implementation and specifically document the accept(...) method parameters (the incoming coroutine handle, capy::executor_ref executor, std::stop_token for cancellation, std::error_code* out_error for reporting, and io_object::implementation** out_impl for swapping implementations), plus short notes on return value semantics (what the returned std::coroutine_handle<> represents and when it is resumed) and similar one-line docs for local_endpoint(), is_open(), release_socket(), cancel(), set_option() and get_option() so implementers know expected behavior and ownership/cancellation guarantees.src/corosio/src/io_context.cpp (1)
11-20: Add the required file-level overview block.This file now coordinates backend selection, service pre-creation, and post-construction scheduler tuning, but there is still no
/* */overview comment after the includes. Replacing the ad-hoc include note with the required high-level map would make this much easier to maintain.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 - 20, Add a file-level /* */ overview block immediately after the include section in src/corosio/src/io_context.cpp that summarizes the file's responsibilities: backend selection logic (reactor/backend choice), service pre-creation behavior, and post-construction scheduler tuning for io_context and related classes (mention io_context, reactor/backend selection, detail::thread_pool or scheduler tuning points). Keep the comment concise but high-level, mapping the main control flow (how backends are chosen, which services are pre-created, and where scheduler tuning occurs) so maintainers can quickly understand the implementation without reading all functions.
🤖 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/local_datagram_socket.hpp`:
- Around line 581-615: The docs for release() and shutdown(shutdown_type) must
be made consistent with the implementation: update the release() comment to
state it throws std::logic_error if the socket is not open and otherwise returns
the native_handle_type (remove the “returns -1” claim), and update the
shutdown(shutdown_type) comment to reflect the implementation’s behavior (it
does not propagate service errors—use the noexcept overload
shutdown(shutdown_type, std::error_code&) to observe errors) while documenting
parameters, return behavior, and the thrown exception for release(); reference
the functions release() and shutdown(shutdown_type) in the header so the
contract matches the code.
In `@include/boost/corosio/native/detail/reactor/reactor_basic_socket.hpp`:
- Around line 419-483: do_release_socket in reactor_basic_socket only clears
local_endpoint_, so add a CRTP release hook call so derived sockets can clear
their endpoint state (e.g. remote_endpoint_) at release time: introduce a
protected no-op hook method (name it something like on_release_endpoint_state or
release_endpoint_state) on the base CRTP type and call it from
reactor_basic_socket<Derived,...>::do_release_socket before or when clearing
local_endpoint_; update reactor_stream_socket (or the stream-derived class) to
override that hook and clear its remote_endpoint_ there. Ensure the hook has
noexcept and is callable via
static_cast<Derived*>(this)->on_release_endpoint_state() so derived cleanup runs
when the fd is handed off.
In `@include/boost/corosio/native/detail/reactor/reactor_service_finals.hpp`:
- Around line 66-74: do_assign_fd currently just registers the new fd so sockets
that cache endpoints (reactor_basic_socket / reactor_stream_socket via
local_endpoint_ / remote_endpoint_) remain empty; after
socket_impl->init_and_register(fd) refresh the cached endpoints by querying the
socket (e.g. using getsockname/getpeername or an existing helper like
refresh_endpoints on SocketFinal) and store the results into
socket_impl->local_endpoint_ and socket_impl->remote_endpoint_ (handle and
propagate any syscall errors into the returned std::error_code rather than
leaving caches empty), ensuring do_assign_fd updates endpoint caches so
local_endpoint()/remote_endpoint() return correct values.
---
Duplicate comments:
In `@include/boost/corosio/local_stream_acceptor.hpp`:
- Around line 210-243: The comments promise accept() throws if the acceptor is
not listening, but is_open() only checks the fd and allows calling accept()
before listen(), causing a crash; fix by adding a proper is_listening()
predicate (e.g., bool is_listening() const noexcept that queries the underlying
handle/state) and use is_listening() in both accept(local_stream_socket& peer)
and accept() before returning accept_awaitable/move_accept_awaitable so they
throw the documented std::logic_error, leaving is_open() unchanged (or
alternatively update the docstrings if you opt to keep the current behavior).
In `@include/boost/corosio/local_stream_socket.hpp`:
- Around line 200-210: The documentation for local_stream_socket::release()
contains contradictory behavior; update the comment to a single contract: remove
"or -1 if not open" and state that release() returns the native_handle_type on
success and throws std::logic_error if the socket is not open; ensure the
`@return` and `@throws` lines consistently describe that behavior for the release()
method.
- Around line 136-144: The move assignment in local_stream_socket
(local_stream_socket& operator=(local_stream_socket&& other) noexcept) bypasses
the base io_object by assigning h_ directly; change it to call the base
move-assignment (e.g., io_object::operator=(std::move(other))) after closing the
current socket (keep the close() call) and then let the base assign its members
instead of manually moving h_, ensuring any io_object::operator= logic is
executed.
In `@include/boost/corosio/native/detail/reactor/reactor_service_finals.hpp`:
- Around line 66-74: do_assign_fd currently closes and immediately calls
socket_impl->init_and_register(fd), skipping the trait-specific backend fd
normalization that do_open_socket applies; update do_assign_fd to perform the
same normalization step(s) before init_and_register (i.e., run the trait/backend
fd setup that enforces non-blocking, FD_CLOEXEC and FD_SETSIZE checks used by
do_open_socket) so an adopted descriptor cannot remain blocking or miss CLOEXEC
and so select()/reactor guards are preserved; keep calls to
socket_impl->close_socket() and socket_impl->init_and_register(fd) but insert
the same normalization routine (the trait/backend normalization used by
do_open_socket) between them.
---
Nitpick comments:
In `@include/boost/corosio/local_stream_acceptor.hpp`:
- Around line 286-313: The implementation interface currently lacks parameter
documentation which makes the backend contract unclear; add brief Doxygen-style
comments to the struct implementation and specifically document the accept(...)
method parameters (the incoming coroutine handle, capy::executor_ref executor,
std::stop_token for cancellation, std::error_code* out_error for reporting, and
io_object::implementation** out_impl for swapping implementations), plus short
notes on return value semantics (what the returned std::coroutine_handle<>
represents and when it is resumed) and similar one-line docs for
local_endpoint(), is_open(), release_socket(), cancel(), set_option() and
get_option() so implementers know expected behavior and ownership/cancellation
guarantees.
In `@include/boost/corosio/local_stream_socket.hpp`:
- Around line 64-99: The implementation struct lacks docstrings matching
tcp_socket::implementation; add Doxygen-style comments for implementation and
for each virtual method (at minimum connect) documenting parameters and return
semantics: for connect(document h, ex, ep, token, ec and what the returned
std::coroutine_handle<> represents and error reporting via ec), and similar
brief descriptions for shutdown, native_handle, release_socket, cancel,
set_option, get_option, local_endpoint and remote_endpoint so the public header
has parity with tcp_socket::implementation.
In `@src/corosio/src/io_context.cpp`:
- Around line 11-20: Add a file-level /* */ overview block immediately after the
include section in src/corosio/src/io_context.cpp that summarizes the file's
responsibilities: backend selection logic (reactor/backend choice), service
pre-creation behavior, and post-construction scheduler tuning for io_context and
related classes (mention io_context, reactor/backend selection,
detail::thread_pool or scheduler tuning points). Keep the comment concise but
high-level, mapping the main control flow (how backends are chosen, which
services are pre-created, and where scheduler tuning occurs) so maintainers can
quickly understand the implementation without reading all functions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0888560c-1441-4912-86e9-078fe04b66c3
⛔ Files ignored due to path filters (20)
doc/modules/ROOT/nav.adocis excluded by!**/doc/**doc/modules/ROOT/pages/4.guide/4a.tcp-networking.adocis excluded by!**/doc/**doc/modules/ROOT/pages/4.guide/4p.unix-sockets.adocis excluded by!**/doc/**doc/modules/ROOT/pages/glossary.adocis excluded by!**/doc/**doc/modules/ROOT/pages/index.adocis excluded by!**/doc/**perf/bench/CMakeLists.txtis excluded by!**/bench/**perf/bench/asio/callback/benchmarks.hppis excluded by!**/bench/**perf/bench/asio/callback/unix_socket_latency_bench.cppis excluded by!**/bench/**perf/bench/asio/callback/unix_socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/asio/coroutine/benchmarks.hppis excluded by!**/bench/**perf/bench/asio/coroutine/unix_socket_latency_bench.cppis excluded by!**/bench/**perf/bench/asio/coroutine/unix_socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/asio/unix_socket_utils.hppis excluded by!**/bench/**perf/bench/corosio/benchmarks.hppis excluded by!**/bench/**perf/bench/corosio/unix_socket_latency_bench.cppis excluded by!**/bench/**perf/bench/corosio/unix_socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/main.cppis excluded by!**/bench/**test/unit/local_datagram_socket.cppis excluded by!**/test/**test/unit/local_stream_socket.cppis excluded by!**/test/**test/unit/tcp_socket.cppis excluded by!**/test/**
📒 Files selected for processing (75)
include/boost/corosio.hppinclude/boost/corosio/backend.hppinclude/boost/corosio/detail/config.hppinclude/boost/corosio/detail/local_datagram_service.hppinclude/boost/corosio/detail/local_stream_acceptor_service.hppinclude/boost/corosio/detail/local_stream_service.hppinclude/boost/corosio/detail/op_base.hppinclude/boost/corosio/io/io_object.hppinclude/boost/corosio/io/io_read_stream.hppinclude/boost/corosio/io/io_write_stream.hppinclude/boost/corosio/local_datagram.hppinclude/boost/corosio/local_datagram_socket.hppinclude/boost/corosio/local_endpoint.hppinclude/boost/corosio/local_socket_pair.hppinclude/boost/corosio/local_stream.hppinclude/boost/corosio/local_stream_acceptor.hppinclude/boost/corosio/local_stream_socket.hppinclude/boost/corosio/message_flags.hppinclude/boost/corosio/native/detail/endpoint_convert.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_traits.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_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_traits.hppinclude/boost/corosio/native/detail/kqueue/kqueue_udp_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_udp_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_acceptor.hppinclude/boost/corosio/native/detail/reactor/reactor_acceptor_service.hppinclude/boost/corosio/native/detail/reactor/reactor_backend.hppinclude/boost/corosio/native/detail/reactor/reactor_basic_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_datagram_ops.hppinclude/boost/corosio/native/detail/reactor/reactor_datagram_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_op.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_finals.hppinclude/boost/corosio/native/detail/reactor/reactor_socket_finals.hppinclude/boost/corosio/native/detail/reactor/reactor_socket_service.hppinclude/boost/corosio/native/detail/reactor/reactor_stream_ops.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_traits.hppinclude/boost/corosio/native/detail/select/select_udp_service.hppinclude/boost/corosio/native/detail/select/select_udp_socket.hppinclude/boost/corosio/native/native_tcp_acceptor.hppinclude/boost/corosio/native/native_tcp_socket.hppinclude/boost/corosio/native/native_udp_socket.hppinclude/boost/corosio/shutdown_type.hppinclude/boost/corosio/tcp_socket.hppinclude/boost/corosio/udp_socket.hppperf/common/native_includes.hppsrc/corosio/src/io_context.cppsrc/corosio/src/local_datagram.cppsrc/corosio/src/local_datagram_socket.cppsrc/corosio/src/local_endpoint.cppsrc/corosio/src/local_socket_pair.cppsrc/corosio/src/local_stream.cppsrc/corosio/src/local_stream_acceptor.cppsrc/corosio/src/local_stream_socket.cpp
💤 Files with no reviewable changes (23)
- include/boost/corosio/native/native_tcp_socket.hpp
- include/boost/corosio/native/native_tcp_acceptor.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_udp_socket.hpp
- include/boost/corosio/native/detail/select/select_op.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor.hpp
- include/boost/corosio/native/detail/select/select_tcp_acceptor.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_socket.hpp
- include/boost/corosio/native/detail/epoll/epoll_udp_service.hpp
- include/boost/corosio/native/detail/select/select_tcp_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_acceptor.hpp
- include/boost/corosio/native/detail/select/select_tcp_acceptor_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_socket.hpp
- include/boost/corosio/native/detail/select/select_udp_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_op.hpp
- include/boost/corosio/native/detail/select/select_tcp_socket.hpp
- include/boost/corosio/native/detail/epoll/epoll_udp_socket.hpp
- include/boost/corosio/native/detail/select/select_udp_socket.hpp
- include/boost/corosio/native/detail/epoll/epoll_op.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_udp_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_acceptor_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor_service.hpp
✅ Files skipped from review due to trivial changes (18)
- include/boost/corosio/detail/config.hpp
- include/boost/corosio.hpp
- include/boost/corosio/detail/local_datagram_service.hpp
- include/boost/corosio/shutdown_type.hpp
- src/corosio/src/local_stream.cpp
- include/boost/corosio/detail/local_stream_service.hpp
- include/boost/corosio/local_datagram.hpp
- src/corosio/src/local_datagram.cpp
- include/boost/corosio/local_stream.hpp
- include/boost/corosio/local_socket_pair.hpp
- src/corosio/src/local_endpoint.cpp
- include/boost/corosio/native/detail/reactor/reactor_op_complete.hpp
- include/boost/corosio/detail/op_base.hpp
- include/boost/corosio/local_endpoint.hpp
- include/boost/corosio/detail/local_stream_acceptor_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_traits.hpp
- include/boost/corosio/tcp_socket.hpp
- include/boost/corosio/native/detail/reactor/reactor_datagram_socket.hpp
🚧 Files skipped from review as they are similar to previous changes (18)
- include/boost/corosio/native/detail/select/select_scheduler.hpp
- include/boost/corosio/native/detail/reactor/reactor_scheduler.hpp
- perf/common/native_includes.hpp
- include/boost/corosio/io/io_object.hpp
- include/boost/corosio/io/io_read_stream.hpp
- include/boost/corosio/native/native_udp_socket.hpp
- src/corosio/src/local_socket_pair.cpp
- include/boost/corosio/native/detail/reactor/reactor_op.hpp
- src/corosio/src/local_stream_socket.cpp
- include/boost/corosio/native/detail/iocp/win_udp_service.hpp
- include/boost/corosio/native/detail/reactor/reactor_stream_ops.hpp
- include/boost/corosio/native/detail/reactor/reactor_datagram_ops.hpp
- include/boost/corosio/native/detail/reactor/reactor_backend.hpp
- include/boost/corosio/native/detail/iocp/win_udp_socket.hpp
- include/boost/corosio/backend.hpp
- include/boost/corosio/io/io_write_stream.hpp
- include/boost/corosio/udp_socket.hpp
- include/boost/corosio/native/detail/endpoint_convert.hpp
mvandeberg
force-pushed
the
pr/122-unix-sockets-2
branch
from
1e6c449 to
25f6507
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/boost/corosio/udp_socket.hpp (1)
505-510:⚠️ Potential issue | 🟡 MinorFix the enum name in the
send_todocs.Line 509 references
message_flags::dont_route, but the enum exported byinclude/boost/corosio/message_flags.hppismessage_flags::do_not_route.🤖 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 505 - 510, The documentation for send_to incorrectly references message_flags::dont_route; update the doc comment for the send_to function (in udp_socket.hpp) to use the correct enum name message_flags::do_not_route as exported by include/boost/corosio/message_flags.hpp, ensuring any mention of the flag in the parameter description and examples is replaced with message_flags::do_not_route.
♻️ Duplicate comments (3)
include/boost/corosio/io/io_read_stream.hpp (1)
97-113:⚠️ Potential issue | 🟡 MinorDocument
read_somecancellation explicitly.The public contract still omits that cancellation can come from the propagated
stop_tokenorcancel(), and that the first result compares equal tocapy::cond::canceled.As per coding guidelines, "Cancellation — State that the operation supports cancellation via stop_token propagated through the IoAwaitable protocol, or via the i/o object's cancel() member; state that the resulting error compares equal to capy::cond::canceled."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/io/io_read_stream.hpp` around lines 97 - 113, Update the read_some documentation in io_read_stream.hpp to explicitly state that the operation supports cancellation via the stop_token propagated through the IoAwaitable protocol and via the stream's cancel() member (or cancel()), and clarify that when cancelled the awaitable resumes with an error_code that compares equal to capy::cond::canceled as the first element of the returned (error_code, std::size_t) tuple; reference the template function read_some (template<capy::MutableBufferSequence MB>) so readers can find the documented behavior.include/boost/corosio/udp_socket.hpp (1)
505-533:⚠️ Potential issue | 🟡 MinorDocument cancellation on the new message-flags overloads.
These overloads are now part of the public async API, but their docs still omit stop-token /
cancel()behavior and thecapy::cond::canceledresult contract.As per coding guidelines, "Cancellation — State that the operation supports cancellation via stop_token propagated through the IoAwaitable protocol, or via the i/o object's cancel() member; state that the resulting error compares equal to capy::cond::canceled."
Also applies to: 535-564, 585-635
🤖 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 505 - 533, Update the documentation for the new message-flags overloads (e.g., the template send_to(Buffers const& buf, endpoint dest, corosio::message_flags flags) and its two-argument overload calling it) to state that these async operations support cancellation via a propagated stop_token (IoAwaitable protocol) and via the socket's cancel()/i/o object cancel() method, and that when cancelled the awaitable completes with an error that compares equal to capy::cond::canceled; apply the same documentation wording to the other newly added message_flags overloads referenced (the similar overloads around the send_to_awaitable/receive_from_awaitable functions at the other ranges).include/boost/corosio/local_endpoint.hpp (1)
78-85:⚠️ Potential issue | 🟡 MinorFix the rendered null-character example.
Line 84 still shows the escaped form incorrectly in the docs; it should render as
'\0'.📝 Proposed fix
- `@return` `true` if the path starts with '\\0'. + `@return` `true` if the path starts with `'\0'`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/local_endpoint.hpp` around lines 78 - 85, The documentation comment in include/boost/corosio/local_endpoint.hpp displays the null-character example as '\\0' instead of the intended '\0'; update the docstring to render the character literal correctly by replacing the escaped form with a single-quoted null char `'\0'` (i.e., change the `\\0` instance in that comment block to `\0` inside quotes/backticks) so the generated docs show '\0' as intended.
🧹 Nitpick comments (4)
include/boost/corosio/local_endpoint.hpp (1)
26-42: Document the new public type with a real API docstring.This is a new public header, but the class is still described with a plain block comment and the constructor docs omit key contract details like the portable length limit, abstract-path semantics, and the lifetime expectations of the returned
path()view.As per coding guidelines, "Docstrings are required for all classes and functions in public headers in non-detail namespaces. Docstrings should include a brief description of purpose, document all parameters, return values, and any preconditions, postconditions, or exceptions."
Also applies to: 56-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/local_endpoint.hpp` around lines 26 - 42, Add a proper API docstring for the public class local_endpoint documenting its purpose as a Unix domain socket endpoint, the constructor(s) parameters and behavior (including the portable pathname length limit and how abstract Linux paths starting with '\0' are handled), the semantics of path() (that it returns a non-owning view into an internal fixed-size buffer and the lifetime is tied to the local_endpoint object), the fact the class avoids heap allocation and is trivially copyable, and the cleanup responsibility (library does NOT unlink filesystem socket paths); update docstrings for the constructors and path() accessor to state preconditions (path length limits), postconditions, and platform-specific notes for abstract paths.include/boost/corosio/native/detail/reactor/reactor_socket_finals.hpp (1)
21-38: Include the standard headers this file relies on.This header directly uses
std::conditional_t,std::is_same_v,std::coroutine_handle,std::stop_token, andstd::error_code, but it never includes the standard headers that define them. Right now it only works as long as one of the project headers pulls them in transitively.🧩 Suggested include set
+#include <coroutine> +#include <stop_token> +#include <system_error> +#include <type_traits>🤖 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_finals.hpp` around lines 21 - 38, The header reactor_socket_finals.hpp uses standard library types (std::conditional_t, std::is_same_v, std::coroutine_handle, std::stop_token, std::error_code) but doesn't include the corresponding standard headers; add the minimal required includes (e.g. <type_traits>, <coroutine> or <coroutine> shim used in this project, <stop_token>, and <system_error>) at the top of the file so these symbols are always defined without relying on transitive includes.include/boost/corosio/native/detail/reactor/reactor_acceptor.hpp (2)
13-28: Add the required file-level overview block.This header now owns the acceptor fd lifecycle, descriptor registration, cancellation, close, and release paths, but it still jumps straight from includes into declarations. A short
/* ... */overview after the includes would make the ownership and synchronization model much easier to reason about.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_acceptor.hpp` around lines 13 - 28, Add a file-level overview block comment immediately after the includes in reactor_acceptor.hpp that briefly describes ownership of the acceptor file descriptor, how descriptor registration/unregistration is handled, cancellation/close/release paths, and the synchronization model used to protect the FD and internal state (e.g., which mutex(es) guard what and whether operations may be called concurrently). Reference reactor_acceptor.hpp and the key responsibilities (acceptor fd lifecycle, descriptor registration, cancellation, close, release) so readers can quickly understand the module boundaries and threading assumptions before reading class/struct declarations and functions that implement those behaviors.
252-344: Extract the shared teardown path.
do_close_socket()anddo_release_socket()duplicate the same lock/claim/post/reset sequence. Keeping two copies of this concurrency-sensitive cleanup makes it easy for the next descriptor-state fix to land in one path and be missed in the other.🤖 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_acceptor.hpp` around lines 252 - 344, Both do_close_socket() and do_release_socket() duplicate the same lock/claim/post/reset sequence; extract that shared teardown into a single helper (e.g., teardown_descriptor() or release_descriptor_resources()) and call it from both functions. The helper should capture the weak_from_this() lock, call acc_.request_cancel(), take the desc_state_.mutex, exchange desc_state_.read_op to nullptr, set read_ready/write_ready false, set desc_state_.impl_ref_ if is_enqueued_.load(...), and if a claimed op exists set acc_.impl_ptr = self then svc_.post(&acc_) and svc_.work_finished(); after calling the helper each caller should continue its remaining socket-specific logic (closing/deregistering fd_, resetting desc_state_.fd and registered_events, and clearing local_endpoint_). Ensure the helper references acc_, desc_state_, svc_, and weak_from_this() to preserve the exact existing behavior.
🤖 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/local_datagram_socket.hpp`:
- Around line 80-123: The docblocks for the backend hook methods now include a
flags parameter but the comments for implementation::send_to,
implementation::recv_from and implementation::send (and the corresponding
docblocks at lines ~141-159) do not document it; update each method's docblock
to add an `@param` flags description describing its purpose and expected
values/behavior, ensure the description is placed with the other parameter
entries for send_to, recv_from and send, and keep wording consistent with
existing param style used in these header comments.
- Around line 437-565: The public async methods connect(), send_to(),
recv_from(), send(), and recv() are missing per-operation docs describing
completion conditions, cancellation behavior, and overlap/serialization rules;
update each method's comment to include: (1) Completion conditions — a bulleted
list of when the awaitable completes (success, remote shutdown/peer closed,
EAGAIN/EWOULDBLOCK, payload bytes transferred, or error codes) referencing the
corresponding awaitable types (connect_awaitable, send_to_awaitable,
recv_from_awaitable, send_awaitable, recv_awaitable); (2) Cancellation — state
that the operation supports cancellation via the stop_token propagated through
the IoAwaitable protocol and/or by calling the socket's cancel() member, and
describe what result/error the awaitable yields on cancellation; and (3)
Overlap/serialization — explicitly state whether multiple concurrent operations
of the same kind are allowed on the socket or must be serialized (e.g., only one
concurrent send/send_to or recv/recv_from), and mention any interactions between
connected vs. unconnected operations (connect() vs. send()/send_to()). Ensure
these notes are added to each method's Doxygen block using the same phrasing
pattern as the class-level thread-safety note.
In `@include/boost/corosio/local_stream_socket.hpp`:
- Around line 212-230: Update the documentation for
local_stream_socket::shutdown(shutdown_type) and its noexcept overload to state
the stronger postcondition for shutdown_send: when called with shutdown_send,
the socket guarantees that subsequent read operations will not merely return
zero bytes but will report an explicit EOF condition (capy::cond::eof). Mention
this contrasts with close(), and reference the shutdown_type/shutdown_send
values and capy::cond::eof so readers know the expected behavior for
shutdown(shutdown_send) vs close().
- Around line 169-185: Update the connect() documentation to state that the
parameter ep is the peer endpoint (path) to connect to, not the local endpoint,
and expand the awaitable contract for connect_awaitable: list completion
conditions (successful connection completion, failures such as permission denied
/ path not found / connection refused / interrupted, and exceptions thrown when
open() fails), declare cancellation behavior (supports stop_token propagated via
the IoAwaitable protocol and/or can be cancelled via the socket's cancel()
member which will cause the awaitable to complete with operation_aborted),
specify overlap/serialization rules (only one concurrent connect() is allowed
per socket; concurrent connect calls must be serialized by the caller), and
enumerate notable error conditions (std::system_error from open(),
std::errc::connection_refused, std::errc::no_such_file_or_directory,
operation_aborted on cancellation). Ensure the doc references the
functions/types connect(), connect_awaitable, local_endpoint, is_open(), and
open() so readers can locate the implementation.
In `@include/boost/corosio/native/detail/select/select_traits.hpp`:
- Around line 177-181: The setsockopt calls for SO_NOSIGPIPE and IPV6_V6ONLY are
currently ignored; check the return value of ::setsockopt where these options
are set (the blocks that set SO_NOSIGPIPE and IPV6_V6ONLY) and propagate
failures instead of swallowing them: if ::setsockopt returns -1, return or throw
an appropriate error from the surrounding open() / socket-creation routine (or
convert errno to the function's error return) so socket configuration failures
abort socket setup and surface to the caller; apply the same change to both
occurrences around SO_NOSIGPIPE and IPV6_V6ONLY.
In `@include/boost/corosio/tcp_socket.hpp`:
- Around line 398-400: The docstring incorrectly references a non-existent
overload shutdown(shutdown_type,std::error_code&); fix by adding a matching
overload declaration for tcp_socket::shutdown(shutdown_type, std::error_code&)
to the class (and implement/forward it to the existing shutdown(shutdown_type)
behavior while populating the std::error_code on failure), or alternatively
change the documentation text to point to the actual error-reporting API that
exists; update the doc/comment at the same spot (around the existing shutdown
documentation) to reflect whichever approach you take so the reference and the
code stay consistent.
In `@src/corosio/src/local_stream_acceptor.cpp`:
- Around line 53-64: The code currently unconditionally calls ::unlink on
ep.path() when bind_option::unlink_existing is set, which can delete arbitrary
filesystem entries; instead, null-terminate the copied path buffer and call
::lstat on that buffer, verify the existing file is a socket (S_ISSOCK /
S_IFSOCK) before calling ::unlink, treat lstat ENOENT as fine and skip unlink on
any non-socket result; apply this change in the block handling
bind_option::unlink_existing that uses ep.path(), char
buf[local_endpoint::max_path_length + 1], and ::unlink so bind() only removes
stale socket nodes.
---
Outside diff comments:
In `@include/boost/corosio/udp_socket.hpp`:
- Around line 505-510: The documentation for send_to incorrectly references
message_flags::dont_route; update the doc comment for the send_to function (in
udp_socket.hpp) to use the correct enum name message_flags::do_not_route as
exported by include/boost/corosio/message_flags.hpp, ensuring any mention of the
flag in the parameter description and examples is replaced with
message_flags::do_not_route.
---
Duplicate comments:
In `@include/boost/corosio/io/io_read_stream.hpp`:
- Around line 97-113: Update the read_some documentation in io_read_stream.hpp
to explicitly state that the operation supports cancellation via the stop_token
propagated through the IoAwaitable protocol and via the stream's cancel() member
(or cancel()), and clarify that when cancelled the awaitable resumes with an
error_code that compares equal to capy::cond::canceled as the first element of
the returned (error_code, std::size_t) tuple; reference the template function
read_some (template<capy::MutableBufferSequence MB>) so readers can find the
documented behavior.
In `@include/boost/corosio/local_endpoint.hpp`:
- Around line 78-85: The documentation comment in
include/boost/corosio/local_endpoint.hpp displays the null-character example as
'\\0' instead of the intended '\0'; update the docstring to render the character
literal correctly by replacing the escaped form with a single-quoted null char
`'\0'` (i.e., change the `\\0` instance in that comment block to `\0` inside
quotes/backticks) so the generated docs show '\0' as intended.
In `@include/boost/corosio/udp_socket.hpp`:
- Around line 505-533: Update the documentation for the new message-flags
overloads (e.g., the template send_to(Buffers const& buf, endpoint dest,
corosio::message_flags flags) and its two-argument overload calling it) to state
that these async operations support cancellation via a propagated stop_token
(IoAwaitable protocol) and via the socket's cancel()/i/o object cancel() method,
and that when cancelled the awaitable completes with an error that compares
equal to capy::cond::canceled; apply the same documentation wording to the other
newly added message_flags overloads referenced (the similar overloads around the
send_to_awaitable/receive_from_awaitable functions at the other ranges).
---
Nitpick comments:
In `@include/boost/corosio/local_endpoint.hpp`:
- Around line 26-42: Add a proper API docstring for the public class
local_endpoint documenting its purpose as a Unix domain socket endpoint, the
constructor(s) parameters and behavior (including the portable pathname length
limit and how abstract Linux paths starting with '\0' are handled), the
semantics of path() (that it returns a non-owning view into an internal
fixed-size buffer and the lifetime is tied to the local_endpoint object), the
fact the class avoids heap allocation and is trivially copyable, and the cleanup
responsibility (library does NOT unlink filesystem socket paths); update
docstrings for the constructors and path() accessor to state preconditions (path
length limits), postconditions, and platform-specific notes for abstract paths.
In `@include/boost/corosio/native/detail/reactor/reactor_acceptor.hpp`:
- Around line 13-28: Add a file-level overview block comment immediately after
the includes in reactor_acceptor.hpp that briefly describes ownership of the
acceptor file descriptor, how descriptor registration/unregistration is handled,
cancellation/close/release paths, and the synchronization model used to protect
the FD and internal state (e.g., which mutex(es) guard what and whether
operations may be called concurrently). Reference reactor_acceptor.hpp and the
key responsibilities (acceptor fd lifecycle, descriptor registration,
cancellation, close, release) so readers can quickly understand the module
boundaries and threading assumptions before reading class/struct declarations
and functions that implement those behaviors.
- Around line 252-344: Both do_close_socket() and do_release_socket() duplicate
the same lock/claim/post/reset sequence; extract that shared teardown into a
single helper (e.g., teardown_descriptor() or release_descriptor_resources())
and call it from both functions. The helper should capture the weak_from_this()
lock, call acc_.request_cancel(), take the desc_state_.mutex, exchange
desc_state_.read_op to nullptr, set read_ready/write_ready false, set
desc_state_.impl_ref_ if is_enqueued_.load(...), and if a claimed op exists set
acc_.impl_ptr = self then svc_.post(&acc_) and svc_.work_finished(); after
calling the helper each caller should continue its remaining socket-specific
logic (closing/deregistering fd_, resetting desc_state_.fd and
registered_events, and clearing local_endpoint_). Ensure the helper references
acc_, desc_state_, svc_, and weak_from_this() to preserve the exact existing
behavior.
In `@include/boost/corosio/native/detail/reactor/reactor_socket_finals.hpp`:
- Around line 21-38: The header reactor_socket_finals.hpp uses standard library
types (std::conditional_t, std::is_same_v, std::coroutine_handle,
std::stop_token, std::error_code) but doesn't include the corresponding standard
headers; add the minimal required includes (e.g. <type_traits>, <coroutine> or
<coroutine> shim used in this project, <stop_token>, and <system_error>) at the
top of the file so these symbols are always defined without relying on
transitive includes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 92a35d76-28f6-4ac1-acf9-c7fb1f4c2151
⛔ Files ignored due to path filters (20)
doc/modules/ROOT/nav.adocis excluded by!**/doc/**doc/modules/ROOT/pages/4.guide/4a.tcp-networking.adocis excluded by!**/doc/**doc/modules/ROOT/pages/4.guide/4p.unix-sockets.adocis excluded by!**/doc/**doc/modules/ROOT/pages/glossary.adocis excluded by!**/doc/**doc/modules/ROOT/pages/index.adocis excluded by!**/doc/**perf/bench/CMakeLists.txtis excluded by!**/bench/**perf/bench/asio/callback/benchmarks.hppis excluded by!**/bench/**perf/bench/asio/callback/unix_socket_latency_bench.cppis excluded by!**/bench/**perf/bench/asio/callback/unix_socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/asio/coroutine/benchmarks.hppis excluded by!**/bench/**perf/bench/asio/coroutine/unix_socket_latency_bench.cppis excluded by!**/bench/**perf/bench/asio/coroutine/unix_socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/asio/unix_socket_utils.hppis excluded by!**/bench/**perf/bench/corosio/benchmarks.hppis excluded by!**/bench/**perf/bench/corosio/unix_socket_latency_bench.cppis excluded by!**/bench/**perf/bench/corosio/unix_socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/main.cppis excluded by!**/bench/**test/unit/local_datagram_socket.cppis excluded by!**/test/**test/unit/local_stream_socket.cppis excluded by!**/test/**test/unit/tcp_socket.cppis excluded by!**/test/**
📒 Files selected for processing (75)
include/boost/corosio.hppinclude/boost/corosio/backend.hppinclude/boost/corosio/detail/config.hppinclude/boost/corosio/detail/local_datagram_service.hppinclude/boost/corosio/detail/local_stream_acceptor_service.hppinclude/boost/corosio/detail/local_stream_service.hppinclude/boost/corosio/detail/op_base.hppinclude/boost/corosio/io/io_object.hppinclude/boost/corosio/io/io_read_stream.hppinclude/boost/corosio/io/io_write_stream.hppinclude/boost/corosio/local_datagram.hppinclude/boost/corosio/local_datagram_socket.hppinclude/boost/corosio/local_endpoint.hppinclude/boost/corosio/local_socket_pair.hppinclude/boost/corosio/local_stream.hppinclude/boost/corosio/local_stream_acceptor.hppinclude/boost/corosio/local_stream_socket.hppinclude/boost/corosio/message_flags.hppinclude/boost/corosio/native/detail/endpoint_convert.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_traits.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_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_traits.hppinclude/boost/corosio/native/detail/kqueue/kqueue_udp_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_udp_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_acceptor.hppinclude/boost/corosio/native/detail/reactor/reactor_acceptor_service.hppinclude/boost/corosio/native/detail/reactor/reactor_backend.hppinclude/boost/corosio/native/detail/reactor/reactor_basic_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_datagram_ops.hppinclude/boost/corosio/native/detail/reactor/reactor_datagram_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_op.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_finals.hppinclude/boost/corosio/native/detail/reactor/reactor_socket_finals.hppinclude/boost/corosio/native/detail/reactor/reactor_socket_service.hppinclude/boost/corosio/native/detail/reactor/reactor_stream_ops.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_traits.hppinclude/boost/corosio/native/detail/select/select_udp_service.hppinclude/boost/corosio/native/detail/select/select_udp_socket.hppinclude/boost/corosio/native/native_tcp_acceptor.hppinclude/boost/corosio/native/native_tcp_socket.hppinclude/boost/corosio/native/native_udp_socket.hppinclude/boost/corosio/shutdown_type.hppinclude/boost/corosio/tcp_socket.hppinclude/boost/corosio/udp_socket.hppperf/common/native_includes.hppsrc/corosio/src/io_context.cppsrc/corosio/src/local_datagram.cppsrc/corosio/src/local_datagram_socket.cppsrc/corosio/src/local_endpoint.cppsrc/corosio/src/local_socket_pair.cppsrc/corosio/src/local_stream.cppsrc/corosio/src/local_stream_acceptor.cppsrc/corosio/src/local_stream_socket.cpp
💤 Files with no reviewable changes (23)
- include/boost/corosio/native/native_tcp_socket.hpp
- include/boost/corosio/native/native_tcp_acceptor.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_udp_socket.hpp
- include/boost/corosio/native/detail/select/select_op.hpp
- include/boost/corosio/native/detail/select/select_tcp_acceptor.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_acceptor.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_udp_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_socket.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_service.hpp
- include/boost/corosio/native/detail/select/select_tcp_socket.hpp
- include/boost/corosio/native/detail/epoll/epoll_udp_socket.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_op.hpp
- include/boost/corosio/native/detail/epoll/epoll_op.hpp
- include/boost/corosio/native/detail/epoll/epoll_udp_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_service.hpp
- include/boost/corosio/native/detail/select/select_tcp_service.hpp
- include/boost/corosio/native/detail/select/select_udp_socket.hpp
- include/boost/corosio/native/detail/select/select_udp_service.hpp
- include/boost/corosio/native/detail/select/select_tcp_acceptor_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_socket.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_acceptor_service.hpp
✅ Files skipped from review due to trivial changes (14)
- include/boost/corosio.hpp
- include/boost/corosio/native/detail/reactor/reactor_scheduler.hpp
- include/boost/corosio/detail/config.hpp
- include/boost/corosio/shutdown_type.hpp
- include/boost/corosio/local_stream.hpp
- include/boost/corosio/local_datagram.hpp
- include/boost/corosio/message_flags.hpp
- include/boost/corosio/local_socket_pair.hpp
- src/corosio/src/local_datagram.cpp
- include/boost/corosio/detail/local_stream_acceptor_service.hpp
- src/corosio/src/local_stream.cpp
- include/boost/corosio/detail/op_base.hpp
- include/boost/corosio/native/detail/reactor/reactor_stream_ops.hpp
- include/boost/corosio/local_stream_acceptor.hpp
🚧 Files skipped from review as they are similar to previous changes (18)
- include/boost/corosio/native/detail/select/select_scheduler.hpp
- include/boost/corosio/native/detail/reactor/reactor_socket_service.hpp
- include/boost/corosio/io/io_object.hpp
- include/boost/corosio/native/native_udp_socket.hpp
- perf/common/native_includes.hpp
- src/corosio/src/local_socket_pair.cpp
- src/corosio/src/io_context.cpp
- include/boost/corosio/detail/local_stream_service.hpp
- src/corosio/src/local_endpoint.cpp
- include/boost/corosio/detail/local_datagram_service.hpp
- include/boost/corosio/io/io_write_stream.hpp
- include/boost/corosio/native/detail/reactor/reactor_op_complete.hpp
- include/boost/corosio/native/detail/reactor/reactor_datagram_ops.hpp
- include/boost/corosio/native/detail/iocp/win_udp_socket.hpp
- src/corosio/src/local_stream_socket.cpp
- src/corosio/src/local_datagram_socket.cpp
- include/boost/corosio/backend.hpp
- include/boost/corosio/native/detail/reactor/reactor_service_finals.hpp
mvandeberg
force-pushed
the
pr/122-unix-sockets-2
branch
from
25f6507 to
85079b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/corosio/src/local_stream_acceptor.cpp (1)
14-20: Add the file-level implementation overview while this logic is fresh.This new
.cpphas non-trivial open/bind/listen behavior and a subtleunlink_existingsafety path, but it still lacks the high-level/* */maintainer note required for implementation files.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/local_stream_acceptor.cpp` around lines 14 - 20, Add a file-level /* */ block comment immediately after the includes that summarizes the implementation of boost::corosio::local_stream_acceptor: describe the open/bind/listen sequence, how errors are handled (referencing detail::except), the role of detail::local_stream_acceptor_service in performing operations, and clearly explain the unlink_existing safety path (when it unlinks, why, and any race/permission caveats) so maintainers can quickly grasp the high-level behavior without reading the code.src/corosio/src/local_datagram_socket.cpp (1)
14-20: Add a file-level overview block here.This new source file contains non-trivial lifecycle and state-management logic, but it goes straight from the includes into the implementation. Please add a short
/* */overview after the includes describing howlocal_datagram_socketdelegates todetail::local_datagram_serviceand what the open/close/assign/query state model is.♻️ Suggested addition
`#include` <boost/corosio/detail/local_datagram_service.hpp> `#include` <sys/ioctl.h> +/* Implements the POSIX local_datagram_socket facade by delegating socket + creation, binding, assignment, closure, and cancellation to + detail::local_datagram_service. The implementation keeps the public state + machine simple: open/close/cancel are idempotent, invalid state transitions + raise logic_error, and endpoint / availability queries degrade safely when + the socket is not open. +*/ + namespace boost::corosio {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/local_datagram_socket.cpp` around lines 14 - 20, Add a file-level block comment after the includes that summarizes the purpose and high-level design of this translation unit: explain that class local_datagram_socket delegates operations (open/close/assign/query) to detail::local_datagram_service, describe the socket lifecycle/state model (how open/close transitions work, what assign does, and how query/readable state is reported), and call out any important invariants or thread/coroutine-safety expectations for methods like local_datagram_socket::open, ::close, ::assign and the service helpers in detail::local_datagram_service so readers can quickly understand ownership and state transitions 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.
Inline comments:
In `@include/boost/corosio/local_datagram_socket.hpp`:
- Around line 40-66: The comment block describing the public contract for
local_datagram_socket is currently a C-style /* ... */ comment and will be
omitted from generated docs; replace it with a public Doxygen-style doc comment
/** ... */ placed immediately above the local_datagram_socket class declaration
so the thread-safety, overlap rules (recv/recv_from and send/send_to sharing
slots), supported modes (connected vs connectionless), and usage notes are
included in generated documentation; ensure the text remains intact and follows
the coding guideline that public classes in non-detail headers have docstrings.
In `@include/boost/corosio/local_endpoint.hpp`:
- Around line 26-41: Convert the leading block comment for the local_endpoint
class into a public docstring so it appears in generated docs: replace the plain
/* ... */ comment with a Doxygen-style /** ... */ comment immediately preceding
the local_endpoint class declaration, preserving the text about path storage,
abstract-socket semantics, no-auto-unlink, and thread-safety; mirror the style
used for local_datagram_socket to ensure it meets the project's public-header
docstring guideline.
In `@include/boost/corosio/local_stream_acceptor.hpp`:
- Around line 237-272: Update the documentation to match the implementation:
change the is_open() summary to state it only indicates the native acceptor
handle exists (not that it is listening), and update the `@throws` clauses on both
overloads accept(local_stream_socket& peer) and accept() to say they only throw
std::logic_error when the native acceptor handle is absent (i.e., !is_open()),
and that calling accept on an open-but-not-listening acceptor will not throw but
will complete with a normal kernel error (e.g., EINVAL) from the backend.
In `@include/boost/corosio/native/detail/endpoint_convert.hpp`:
- Around line 295-316: The code computes path_len from the incoming len even
though only up to sizeof(sockaddr_un) bytes were copied into sockaddr_un sa;
clamp the derived path_len to the actual bytes copied to avoid reading past
sa.sun_path. Change the logic around path_offset/path_len so you first compute
bytes_copied = std::min<std::size_t>(static_cast<std::size_t>(len), sizeof(sa)),
then set path_len = (bytes_copied > path_offset) ? bytes_copied - path_offset :
0 before doing the memchr and constructing local_endpoint
ep(std::string_view(sa.sun_path, path_len), ec). This ensures memchr and the
string_view only examine valid bytes in sa.sun_path.
In `@include/boost/corosio/native/detail/reactor/reactor_service_finals.hpp`:
- Around line 72-113: Before calling socket_impl->init_and_register(fd) validate
the FD's address family and socket type and return an error so ownership stays
with the caller: use ::getsockname(fd, ...) to read sockaddr.sa_family and
::getsockopt(fd, SOL_SOCKET, SO_TYPE, ...) to get the socket type, then check
that the family == AF_UNIX and the SO_TYPE matches the expected type for this
SocketFinal (stream vs datagram); if either check fails, convert the failure
into an appropriate std::error_code and return it (do this after
Traits::configure_local_socket(fd) but before
socket_impl->init_and_register(fd)). Ensure the checks reference do_assign_fd,
socket_impl and init_and_register so the wrong-protocol FD is rejected before
adoption.
---
Nitpick comments:
In `@src/corosio/src/local_datagram_socket.cpp`:
- Around line 14-20: Add a file-level block comment after the includes that
summarizes the purpose and high-level design of this translation unit: explain
that class local_datagram_socket delegates operations (open/close/assign/query)
to detail::local_datagram_service, describe the socket lifecycle/state model
(how open/close transitions work, what assign does, and how query/readable state
is reported), and call out any important invariants or thread/coroutine-safety
expectations for methods like local_datagram_socket::open, ::close, ::assign and
the service helpers in detail::local_datagram_service so readers can quickly
understand ownership and state transitions without diving into implementation
details.
In `@src/corosio/src/local_stream_acceptor.cpp`:
- Around line 14-20: Add a file-level /* */ block comment immediately after the
includes that summarizes the implementation of
boost::corosio::local_stream_acceptor: describe the open/bind/listen sequence,
how errors are handled (referencing detail::except), the role of
detail::local_stream_acceptor_service in performing operations, and clearly
explain the unlink_existing safety path (when it unlinks, why, and any
race/permission caveats) so maintainers can quickly grasp the high-level
behavior without reading the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2deee44-3bd7-40de-bad7-46a07995380d
⛔ Files ignored due to path filters (20)
doc/modules/ROOT/nav.adocis excluded by!**/doc/**doc/modules/ROOT/pages/4.guide/4a.tcp-networking.adocis excluded by!**/doc/**doc/modules/ROOT/pages/4.guide/4p.unix-sockets.adocis excluded by!**/doc/**doc/modules/ROOT/pages/glossary.adocis excluded by!**/doc/**doc/modules/ROOT/pages/index.adocis excluded by!**/doc/**perf/bench/CMakeLists.txtis excluded by!**/bench/**perf/bench/asio/callback/benchmarks.hppis excluded by!**/bench/**perf/bench/asio/callback/unix_socket_latency_bench.cppis excluded by!**/bench/**perf/bench/asio/callback/unix_socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/asio/coroutine/benchmarks.hppis excluded by!**/bench/**perf/bench/asio/coroutine/unix_socket_latency_bench.cppis excluded by!**/bench/**perf/bench/asio/coroutine/unix_socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/asio/unix_socket_utils.hppis excluded by!**/bench/**perf/bench/corosio/benchmarks.hppis excluded by!**/bench/**perf/bench/corosio/unix_socket_latency_bench.cppis excluded by!**/bench/**perf/bench/corosio/unix_socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/main.cppis excluded by!**/bench/**test/unit/local_datagram_socket.cppis excluded by!**/test/**test/unit/local_stream_socket.cppis excluded by!**/test/**test/unit/tcp_socket.cppis excluded by!**/test/**
📒 Files selected for processing (76)
include/boost/corosio.hppinclude/boost/corosio/backend.hppinclude/boost/corosio/detail/config.hppinclude/boost/corosio/detail/local_datagram_service.hppinclude/boost/corosio/detail/local_stream_acceptor_service.hppinclude/boost/corosio/detail/local_stream_service.hppinclude/boost/corosio/detail/op_base.hppinclude/boost/corosio/io/io_object.hppinclude/boost/corosio/io/io_read_stream.hppinclude/boost/corosio/io/io_write_stream.hppinclude/boost/corosio/local_datagram.hppinclude/boost/corosio/local_datagram_socket.hppinclude/boost/corosio/local_endpoint.hppinclude/boost/corosio/local_socket_pair.hppinclude/boost/corosio/local_stream.hppinclude/boost/corosio/local_stream_acceptor.hppinclude/boost/corosio/local_stream_socket.hppinclude/boost/corosio/message_flags.hppinclude/boost/corosio/native/detail/endpoint_convert.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_traits.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_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_traits.hppinclude/boost/corosio/native/detail/kqueue/kqueue_udp_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_udp_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_acceptor.hppinclude/boost/corosio/native/detail/reactor/reactor_acceptor_service.hppinclude/boost/corosio/native/detail/reactor/reactor_backend.hppinclude/boost/corosio/native/detail/reactor/reactor_basic_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_datagram_ops.hppinclude/boost/corosio/native/detail/reactor/reactor_datagram_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_op.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_finals.hppinclude/boost/corosio/native/detail/reactor/reactor_socket_finals.hppinclude/boost/corosio/native/detail/reactor/reactor_socket_service.hppinclude/boost/corosio/native/detail/reactor/reactor_stream_ops.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_traits.hppinclude/boost/corosio/native/detail/select/select_udp_service.hppinclude/boost/corosio/native/detail/select/select_udp_socket.hppinclude/boost/corosio/native/native_tcp_acceptor.hppinclude/boost/corosio/native/native_tcp_socket.hppinclude/boost/corosio/native/native_udp_socket.hppinclude/boost/corosio/shutdown_type.hppinclude/boost/corosio/tcp_socket.hppinclude/boost/corosio/udp_socket.hppperf/common/native_includes.hppsrc/corosio/src/io_context.cppsrc/corosio/src/local_datagram.cppsrc/corosio/src/local_datagram_socket.cppsrc/corosio/src/local_endpoint.cppsrc/corosio/src/local_socket_pair.cppsrc/corosio/src/local_stream.cppsrc/corosio/src/local_stream_acceptor.cppsrc/corosio/src/local_stream_socket.cppsrc/corosio/src/tcp_socket.cpp
💤 Files with no reviewable changes (23)
- include/boost/corosio/native/native_tcp_socket.hpp
- include/boost/corosio/native/native_tcp_acceptor.hpp
- include/boost/corosio/native/detail/select/select_op.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_acceptor.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_service.hpp
- include/boost/corosio/native/detail/select/select_tcp_acceptor.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor.hpp
- include/boost/corosio/native/detail/select/select_tcp_socket.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_udp_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_socket.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_udp_socket.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_socket.hpp
- include/boost/corosio/native/detail/epoll/epoll_op.hpp
- include/boost/corosio/native/detail/epoll/epoll_udp_socket.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor_service.hpp
- include/boost/corosio/native/detail/select/select_udp_socket.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_acceptor_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_udp_service.hpp
- include/boost/corosio/native/detail/select/select_tcp_acceptor_service.hpp
- include/boost/corosio/native/detail/select/select_udp_service.hpp
- include/boost/corosio/native/detail/select/select_tcp_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_op.hpp
✅ Files skipped from review due to trivial changes (16)
- include/boost/corosio.hpp
- include/boost/corosio/detail/config.hpp
- include/boost/corosio/shutdown_type.hpp
- src/corosio/src/local_datagram.cpp
- include/boost/corosio/local_datagram.hpp
- include/boost/corosio/local_stream.hpp
- include/boost/corosio/message_flags.hpp
- include/boost/corosio/local_socket_pair.hpp
- include/boost/corosio/detail/local_stream_acceptor_service.hpp
- src/corosio/src/local_stream.cpp
- include/boost/corosio/detail/local_datagram_service.hpp
- include/boost/corosio/udp_socket.hpp
- include/boost/corosio/native/detail/iocp/win_udp_service.hpp
- include/boost/corosio/detail/op_base.hpp
- include/boost/corosio/native/detail/reactor/reactor_stream_ops.hpp
- include/boost/corosio/detail/local_stream_service.hpp
🚧 Files skipped from review as they are similar to previous changes (14)
- include/boost/corosio/native/detail/select/select_scheduler.hpp
- include/boost/corosio/native/detail/reactor/reactor_scheduler.hpp
- perf/common/native_includes.hpp
- src/corosio/src/local_endpoint.cpp
- src/corosio/src/local_socket_pair.cpp
- src/corosio/src/io_context.cpp
- include/boost/corosio/native/detail/reactor/reactor_op_complete.hpp
- include/boost/corosio/native/native_udp_socket.hpp
- include/boost/corosio/io/io_write_stream.hpp
- src/corosio/src/local_stream_socket.cpp
- include/boost/corosio/native/detail/iocp/win_udp_socket.hpp
- include/boost/corosio/backend.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_traits.hpp
- include/boost/corosio/native/detail/reactor/reactor_acceptor.hpp
mvandeberg
force-pushed
the
pr/122-unix-sockets-2
branch
from
85079b1 to
733f64d
Compare
Contributor
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/boost/corosio/native/native_udp_socket.hpp (1)
73-155:⚠️ Potential issue | 🟠 MajorAdd flag overloads to
native_udp_socket::send_to()andnative_udp_socket::recv_from().The base
udp_socketnow exposessend_to(buffers, dest, message_flags)andrecv_from(buffers, source, message_flags)overloads, allowing callers to usemessage_flags::peek,dont_route, and other flag values. However,native_udp_socketonly exposes the 2-argument forms, hiding the base overload set. Users of the native wrapper cannot access message flags unless they upcast toudp_socket, which defeats the purpose of using the native interface.Add public
send_toandrecv_fromoverloads that acceptmessage_flags, update both awaitables to store the flags parameter, and delegate the no-flags overloads to the flagged versions (matching the pattern inudp_socket.hpplines 537 and 573).🤖 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 73 - 155, The native wrapper is missing message_flags overloads, so add public overloads native_udp_socket::send_to(ConstBufferSequence, endpoint, capy::message_flags) and native_udp_socket::recv_from(MutableBufferSequence, endpoint&, capy::message_flags) that forward to get_impl().send_to/get_impl().recv_from with the flags; update native_send_to_awaitable and native_recv_from_awaitable to store a capy::message_flags member (add to their constructors) and have await_suspend pass that flags value into self_.get_impl().send_to(..., flags, &ec_, &bytes_transferred_) and recv_from(..., flags, &ec_, &bytes_transferred_); finally make the existing two-argument native_udp_socket::send_to/recv_from delegate to the new three-argument overloads (matching the udp_socket pattern).
🧹 Nitpick comments (3)
include/boost/corosio/native/detail/reactor/reactor_datagram_socket.hpp (1)
27-35: Use the enum values here instead of hard-coded bits.Replace the raw bit patterns (1, 2, 4) with
static_cast<int>(message_flags::peek),static_cast<int>(message_flags::out_of_band), andstatic_cast<int>(message_flags::do_not_route)to make the mapping explicit and keep it in sync if the enum values change.🤖 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 27 - 35, Replace the hard-coded mask literals in to_native_msg_flags with the corresponding message_flags enum values: use static_cast<int>(message_flags::peek) instead of 1, static_cast<int>(message_flags::out_of_band) instead of 2, and static_cast<int>(message_flags::do_not_route) instead of 4; update the three if checks in to_native_msg_flags to test against those enum-derived ints so the mapping to native MSG_PEEK, MSG_OOB and MSG_DONTROUTE remains explicit and resilient to enum value changes.include/boost/corosio/local_stream_acceptor.hpp (1)
189-199: Move assignment assertion could be stronger.The assert at line 191-192 validates same context, but this is a debug-only check. Consider whether a release-mode check with a more informative error would help users diagnose cross-context moves in production.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/local_stream_acceptor.hpp` around lines 189 - 199, The move-assignment for local_stream_acceptor currently only asserts the contexts match (assert(&ctx_ == &other.ctx_)) which disappears in release builds; change operator=(local_stream_acceptor&& other) to perform a runtime check of ctx_ vs other.ctx_ and handle mismatch deterministically (e.g., throw std::invalid_argument or call std::terminate with a clear diagnostic) before calling close() and io_object::operator=(std::move(other)); keep the assert for debug builds but add the release-safe check and an explanatory error message referencing ctx_ and the operation (local_stream_acceptor::operator=).include/boost/corosio/native/detail/reactor/reactor_acceptor.hpp (1)
254-291: Consider extracting shared cancellation logic.
do_close_socket()anddo_release_socket()share nearly identical cancellation and cleanup logic (lines 258-276 vs 308-326). The only difference is thatdo_close_socket()calls::close(fd_)whiledo_release_socket()returns the fd. This duplication increases maintenance burden.♻️ Possible extraction
// Extract shared logic into a helper: void prepare_for_close_or_release() noexcept { auto self = this->weak_from_this().lock(); if (self) { acc_.request_cancel(); reactor_op_base* claimed = nullptr; { std::lock_guard lock(desc_state_.mutex); claimed = std::exchange(desc_state_.read_op, nullptr); desc_state_.read_ready = false; desc_state_.write_ready = false; if (desc_state_.is_enqueued_.load(std::memory_order_acquire)) desc_state_.impl_ref_ = self; } if (claimed) { acc_.impl_ptr = self; svc_.post(&acc_); svc_.work_finished(); } } }Also applies to: 293-344
🤖 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_acceptor.hpp` around lines 254 - 291, do_close_socket() and do_release_socket() duplicate the cancellation/cleanup sequence (the acc_.request_cancel, claiming/exchanging desc_state_.read_op, resetting read_ready/write_ready, setting impl_ref_, and posting acc_) — extract that shared logic into a noexcept helper (e.g., prepare_for_close_or_release()) and have both do_close_socket() and do_release_socket() call it; keep the helper using this->weak_from_this().lock(), acc_, desc_state_.mutex, std::exchange on desc_state_.read_op, desc_state_.is_enqueued_.load, svc_.post(&acc_) and svc_.work_finished(), and leave only the fd-specific behavior (::close(fd_) vs returning fd) in the two original functions.
🤖 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/native/detail/kqueue/kqueue_traits.hpp`:
- Around line 57-99: stream_socket_hook's user_set_linger_ remains sticky and
can affect unrelated fds later; ensure the flag is cleared when the fd lifecycle
ends by resetting user_set_linger_ to false after clearing SO_LINGER (update
reset_linger to set user_set_linger_ = false after calling ::setsockopt), and
also ensure any code path that bypasses these hooks (e.g. release_socket)
explicitly clears the hook state on the stream_socket_hook instance so a reused
socket won't inherit the previous linger setting.
In `@include/boost/corosio/native/detail/reactor/reactor_datagram_ops.hpp`:
- Around line 66-104: reactor_dgram_recv_op currently inherits
reactor_dgram_base_op::operator() which calls complete_io_op and treats zero
bytes as EOF; override operator() in reactor_dgram_recv_op to call
complete_datagram_op instead (e.g. implement reactor_dgram_recv_op::operator()
that invokes complete_datagram_op(*this)) so connected recv() completions treat
zero-length datagrams as successful datagrams rather than EOF; keep references
to reactor_dgram_recv_op, reactor_dgram_base_op::operator(),
complete_datagram_op, and complete_io_op to locate the change.
In `@include/boost/corosio/native/detail/select/select_traits.hpp`:
- Around line 174-175: The check that returns make_err(EMFILE) when fd >=
FD_SETSIZE should return make_err(EINVAL) instead so the backend consistently
reports "invalid argument / unsupported fd" (same as
accept_policy::do_accept()); locate the branch using FD_SETSIZE and replace the
EMFILE error with EINVAL by changing the call to make_err(...) accordingly,
ensuring select_traits.hpp's overflow path uses EINVAL.
---
Outside diff comments:
In `@include/boost/corosio/native/native_udp_socket.hpp`:
- Around line 73-155: The native wrapper is missing message_flags overloads, so
add public overloads native_udp_socket::send_to(ConstBufferSequence, endpoint,
capy::message_flags) and native_udp_socket::recv_from(MutableBufferSequence,
endpoint&, capy::message_flags) that forward to
get_impl().send_to/get_impl().recv_from with the flags; update
native_send_to_awaitable and native_recv_from_awaitable to store a
capy::message_flags member (add to their constructors) and have await_suspend
pass that flags value into self_.get_impl().send_to(..., flags, &ec_,
&bytes_transferred_) and recv_from(..., flags, &ec_, &bytes_transferred_);
finally make the existing two-argument native_udp_socket::send_to/recv_from
delegate to the new three-argument overloads (matching the udp_socket pattern).
---
Nitpick comments:
In `@include/boost/corosio/local_stream_acceptor.hpp`:
- Around line 189-199: The move-assignment for local_stream_acceptor currently
only asserts the contexts match (assert(&ctx_ == &other.ctx_)) which disappears
in release builds; change operator=(local_stream_acceptor&& other) to perform a
runtime check of ctx_ vs other.ctx_ and handle mismatch deterministically (e.g.,
throw std::invalid_argument or call std::terminate with a clear diagnostic)
before calling close() and io_object::operator=(std::move(other)); keep the
assert for debug builds but add the release-safe check and an explanatory error
message referencing ctx_ and the operation (local_stream_acceptor::operator=).
In `@include/boost/corosio/native/detail/reactor/reactor_acceptor.hpp`:
- Around line 254-291: do_close_socket() and do_release_socket() duplicate the
cancellation/cleanup sequence (the acc_.request_cancel, claiming/exchanging
desc_state_.read_op, resetting read_ready/write_ready, setting impl_ref_, and
posting acc_) — extract that shared logic into a noexcept helper (e.g.,
prepare_for_close_or_release()) and have both do_close_socket() and
do_release_socket() call it; keep the helper using
this->weak_from_this().lock(), acc_, desc_state_.mutex, std::exchange on
desc_state_.read_op, desc_state_.is_enqueued_.load, svc_.post(&acc_) and
svc_.work_finished(), and leave only the fd-specific behavior (::close(fd_) vs
returning fd) in the two original functions.
In `@include/boost/corosio/native/detail/reactor/reactor_datagram_socket.hpp`:
- Around line 27-35: Replace the hard-coded mask literals in to_native_msg_flags
with the corresponding message_flags enum values: use
static_cast<int>(message_flags::peek) instead of 1,
static_cast<int>(message_flags::out_of_band) instead of 2, and
static_cast<int>(message_flags::do_not_route) instead of 4; update the three if
checks in to_native_msg_flags to test against those enum-derived ints so the
mapping to native MSG_PEEK, MSG_OOB and MSG_DONTROUTE remains explicit and
resilient to enum value changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e0cb93f7-a33c-48fa-9786-8e3548cc7e10
⛔ Files ignored due to path filters (20)
doc/modules/ROOT/nav.adocis excluded by!**/doc/**doc/modules/ROOT/pages/4.guide/4a.tcp-networking.adocis excluded by!**/doc/**doc/modules/ROOT/pages/4.guide/4p.unix-sockets.adocis excluded by!**/doc/**doc/modules/ROOT/pages/glossary.adocis excluded by!**/doc/**doc/modules/ROOT/pages/index.adocis excluded by!**/doc/**perf/bench/CMakeLists.txtis excluded by!**/bench/**perf/bench/asio/callback/benchmarks.hppis excluded by!**/bench/**perf/bench/asio/callback/unix_socket_latency_bench.cppis excluded by!**/bench/**perf/bench/asio/callback/unix_socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/asio/coroutine/benchmarks.hppis excluded by!**/bench/**perf/bench/asio/coroutine/unix_socket_latency_bench.cppis excluded by!**/bench/**perf/bench/asio/coroutine/unix_socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/asio/unix_socket_utils.hppis excluded by!**/bench/**perf/bench/corosio/benchmarks.hppis excluded by!**/bench/**perf/bench/corosio/unix_socket_latency_bench.cppis excluded by!**/bench/**perf/bench/corosio/unix_socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/main.cppis excluded by!**/bench/**test/unit/local_datagram_socket.cppis excluded by!**/test/**test/unit/local_stream_socket.cppis excluded by!**/test/**test/unit/tcp_socket.cppis excluded by!**/test/**
📒 Files selected for processing (76)
include/boost/corosio.hppinclude/boost/corosio/backend.hppinclude/boost/corosio/detail/config.hppinclude/boost/corosio/detail/local_datagram_service.hppinclude/boost/corosio/detail/local_stream_acceptor_service.hppinclude/boost/corosio/detail/local_stream_service.hppinclude/boost/corosio/detail/op_base.hppinclude/boost/corosio/io/io_object.hppinclude/boost/corosio/io/io_read_stream.hppinclude/boost/corosio/io/io_write_stream.hppinclude/boost/corosio/local_datagram.hppinclude/boost/corosio/local_datagram_socket.hppinclude/boost/corosio/local_endpoint.hppinclude/boost/corosio/local_socket_pair.hppinclude/boost/corosio/local_stream.hppinclude/boost/corosio/local_stream_acceptor.hppinclude/boost/corosio/local_stream_socket.hppinclude/boost/corosio/message_flags.hppinclude/boost/corosio/native/detail/endpoint_convert.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_traits.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_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_traits.hppinclude/boost/corosio/native/detail/kqueue/kqueue_udp_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_udp_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_acceptor.hppinclude/boost/corosio/native/detail/reactor/reactor_acceptor_service.hppinclude/boost/corosio/native/detail/reactor/reactor_backend.hppinclude/boost/corosio/native/detail/reactor/reactor_basic_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_datagram_ops.hppinclude/boost/corosio/native/detail/reactor/reactor_datagram_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_op.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_finals.hppinclude/boost/corosio/native/detail/reactor/reactor_socket_finals.hppinclude/boost/corosio/native/detail/reactor/reactor_socket_service.hppinclude/boost/corosio/native/detail/reactor/reactor_stream_ops.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_traits.hppinclude/boost/corosio/native/detail/select/select_udp_service.hppinclude/boost/corosio/native/detail/select/select_udp_socket.hppinclude/boost/corosio/native/native_tcp_acceptor.hppinclude/boost/corosio/native/native_tcp_socket.hppinclude/boost/corosio/native/native_udp_socket.hppinclude/boost/corosio/shutdown_type.hppinclude/boost/corosio/tcp_socket.hppinclude/boost/corosio/udp_socket.hppperf/common/native_includes.hppsrc/corosio/src/io_context.cppsrc/corosio/src/local_datagram.cppsrc/corosio/src/local_datagram_socket.cppsrc/corosio/src/local_endpoint.cppsrc/corosio/src/local_socket_pair.cppsrc/corosio/src/local_stream.cppsrc/corosio/src/local_stream_acceptor.cppsrc/corosio/src/local_stream_socket.cppsrc/corosio/src/tcp_socket.cpp
💤 Files with no reviewable changes (23)
- include/boost/corosio/native/native_tcp_socket.hpp
- include/boost/corosio/native/detail/select/select_tcp_acceptor.hpp
- include/boost/corosio/native/native_tcp_acceptor.hpp
- include/boost/corosio/native/detail/select/select_op.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_socket.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_acceptor.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor.hpp
- include/boost/corosio/native/detail/select/select_tcp_socket.hpp
- include/boost/corosio/native/detail/epoll/epoll_udp_socket.hpp
- include/boost/corosio/native/detail/select/select_udp_socket.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_op.hpp
- include/boost/corosio/native/detail/select/select_tcp_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_socket.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_udp_service.hpp
- include/boost/corosio/native/detail/select/select_udp_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_op.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_acceptor_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_udp_service.hpp
- include/boost/corosio/native/detail/select/select_tcp_acceptor_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_udp_socket.hpp
✅ Files skipped from review due to trivial changes (12)
- include/boost/corosio/native/detail/reactor/reactor_scheduler.hpp
- include/boost/corosio.hpp
- include/boost/corosio/shutdown_type.hpp
- src/corosio/src/local_datagram.cpp
- include/boost/corosio/local_stream.hpp
- include/boost/corosio/local_socket_pair.hpp
- include/boost/corosio/local_datagram.hpp
- include/boost/corosio/detail/local_stream_acceptor_service.hpp
- include/boost/corosio/detail/local_datagram_service.hpp
- include/boost/corosio/detail/op_base.hpp
- include/boost/corosio/message_flags.hpp
- include/boost/corosio/io/io_write_stream.hpp
🚧 Files skipped from review as they are similar to previous changes (13)
- include/boost/corosio/native/detail/select/select_scheduler.hpp
- include/boost/corosio/native/detail/reactor/reactor_socket_service.hpp
- src/corosio/src/tcp_socket.cpp
- include/boost/corosio/io/io_object.hpp
- perf/common/native_includes.hpp
- src/corosio/src/io_context.cpp
- src/corosio/src/local_stream.cpp
- src/corosio/src/local_socket_pair.cpp
- include/boost/corosio/native/detail/iocp/win_udp_service.hpp
- include/boost/corosio/native/detail/reactor/reactor_op_complete.hpp
- include/boost/corosio/local_endpoint.hpp
- include/boost/corosio/native/detail/reactor/reactor_service_finals.hpp
- include/boost/corosio/udp_socket.hpp
✅ Actions performedReviews resumed. |
mvandeberg
force-pushed
the
pr/122-unix-sockets-2
branch
from
733f64d to
b968d3c
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
include/boost/corosio/native/native_udp_socket.hpp (1)
187-222: Consider addingmessage_flagsoverloads to matchudp_socketAPI.The base
udp_socketnow provides overloads acceptingmessage_flagsforsend_to,recv_from,send, andrecv. Thenative_udp_socketmethods shadow these without offering the flags variants, so users ofnative_udp_socketcannot specify message flags.For API consistency, consider adding:
template<capy::ConstBufferSequence CB> auto send_to(CB const& buffers, endpoint dest, message_flags flags) { ... }This is not blocking since the default (no flags) still works.
🤖 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 187 - 222, native_udp_socket currently lacks the message_flags overloads that udp_socket exposes, so add new overloads for send_to, recv_from, send and recv that accept a message_flags parameter; each overload should mirror the existing checks (call is_open and throw_logic_error) and then forward to the corresponding backend awaitable (native_send_to_awaitable, native_recv_from_awaitable, native_send_awaitable, native_recv_awaitable) passing the flags along, using the same template constraints (capy::ConstBufferSequence for send/send_to and capy::MutableBufferSequence for recv/recv_from) and returning the same awaitable type as the no-flags versions.src/corosio/src/local_socket_pair.cpp (1)
10-24: Add the missing file-level implementation overview.This file now owns the
socketpaircreation path, the nonblocking/CLOEXEC fallback, and the exception-safe FD handoff logic, so it qualifies as non-trivial implementation code and should have the required post-include/* */summary block. 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/local_socket_pair.cpp` around lines 10 - 24, Add a file-level block comment immediately after the include section in local_socket_pair.cpp that briefly summarizes the implementation: explain that this file implements the POSIX socketpair creation path, attempts SOCK_NONBLOCK|SOCK_CLOEXEC atomically and falls back to setting O_NONBLOCK and FD_CLOEXEC if not supported, and describes the exception-safe file-descriptor handoff logic used to ensure no FD leaks on errors; reference the socketpair creation/fallback and the exception-safe handoff so reviewers can quickly understand functions handling those concerns.
🤖 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/detail/local_stream_acceptor_service.hpp`:
- Around line 35-41: The documentation comment in
local_stream_acceptor_service.hpp exposes POSIX macros AF_UNIX and SOCK_STREAM;
update the comment for the acceptor creation (the block describing the
parameters for the acceptor/open operation) to use Corosio-local semantics
instead of platform constants: describe the address family as "local (domain)
socket", the type as "stream socket semantics", and the protocol as "protocol
number (default 0)"/or "implementation-defined", removing any direct reference
to AF_UNIX or SOCK_STREAM so the header no longer contains platform-specific
macro names.
In `@include/boost/corosio/local_datagram_socket.hpp`:
- Around line 691-700: Update the assign(int fd) docblock to document the
precondition that the socket must not already be open and that violating this
precondition results in a std::logic_error; keep the existing note that
std::system_error is thrown on other failures. Reference the assign(int)
declaration and ensure the docstring mentions the parameter, the precondition
("socket must not already be open"), and both exception types (std::logic_error
for already-open sockets and std::system_error for other errors).
In `@include/boost/corosio/local_stream_acceptor.hpp`:
- Around line 124-130: The await_suspend implementation in local_stream_acceptor
should short-circuit if the provided env->stop_token is already requested:
assign token_ = env->stop_token, check token_.stop_requested(), and if true set
ec_ to an operation-cancelled error (e.g. std::errc::operation_canceled or
boost::asio::error::operation_aborted equivalent) and return the incoming
coroutine handle (h) to resume immediately instead of calling
implementation::accept(); otherwise call acc_.get().accept(...) as before. Apply
this same pre-cancellation guard to both accept_awaitable and
move_accept_awaitable await_suspend implementations, and ensure peer_impl_ is
not consumed when short-circuiting.
In `@include/boost/corosio/native/detail/reactor/reactor_basic_socket.hpp`:
- Around line 128-140: init_and_register() now always calls
svc_.scheduler().register_descriptor(fd, &desc_state_), so make the
close/release paths symmetric by always calling
svc_.scheduler().deregister_descriptor(fd_, &desc_state_) for any descriptor
that was registered (e.g., whenever fd_ or desc_state_.fd is valid), instead of
only when registered_events != 0; after deregistering clear fd_/desc_state_.fd
and reset descriptor pointers under the same mutex to avoid races. Apply the
same change to both existing close/release paths that currently check
registered_events (the other two deregistration sites) so every path that undoes
init_and_register performs an unconditional deregister followed by proper
cleanup.
In `@src/corosio/src/local_socket_pair.cpp`:
- Around line 28-55: The fallback make_nonblock_cloexec currently ignores
failures and returns partially-configured sockets; modify it to check the return
values of each ::fcntl call (F_GETFL, F_SETFL, F_SETFD) and propagate failures
back to create_pair (e.g., by returning an errno or throwing std::system_error).
In create_pair, after socketpair succeeds but before returning, call
make_nonblock_cloexec for both fds and if either call fails, close any opened
descriptors (close fds[0] and fds[1] if valid) and throw a std::system_error
using the failing errno so callers never receive blocking or CLOEXEC-missing
sockets; reference functions make_nonblock_cloexec and create_pair for locating
changes.
In `@src/corosio/src/local_stream_acceptor.cpp`:
- Around line 18-19: This translation unit calls std::memcpy (the usage around
the memcpy call) but doesn't include the <cstring> header; add `#include`
<cstring> to the includes at the top of local_stream_acceptor.cpp so std::memcpy
is declared reliably (keep the existing `#include` <sys/stat.h> and `#include`
<unistd.h> and insert <cstring> alongside them).
---
Nitpick comments:
In `@include/boost/corosio/native/native_udp_socket.hpp`:
- Around line 187-222: native_udp_socket currently lacks the message_flags
overloads that udp_socket exposes, so add new overloads for send_to, recv_from,
send and recv that accept a message_flags parameter; each overload should mirror
the existing checks (call is_open and throw_logic_error) and then forward to the
corresponding backend awaitable (native_send_to_awaitable,
native_recv_from_awaitable, native_send_awaitable, native_recv_awaitable)
passing the flags along, using the same template constraints
(capy::ConstBufferSequence for send/send_to and capy::MutableBufferSequence for
recv/recv_from) and returning the same awaitable type as the no-flags versions.
In `@src/corosio/src/local_socket_pair.cpp`:
- Around line 10-24: Add a file-level block comment immediately after the
include section in local_socket_pair.cpp that briefly summarizes the
implementation: explain that this file implements the POSIX socketpair creation
path, attempts SOCK_NONBLOCK|SOCK_CLOEXEC atomically and falls back to setting
O_NONBLOCK and FD_CLOEXEC if not supported, and describes the exception-safe
file-descriptor handoff logic used to ensure no FD leaks on errors; reference
the socketpair creation/fallback and the exception-safe handoff so reviewers can
quickly understand functions handling those concerns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 882cae7a-a017-488d-b5b6-01a69b2f5ed4
⛔ Files ignored due to path filters (21)
doc/modules/ROOT/nav.adocis excluded by!**/doc/**doc/modules/ROOT/pages/4.guide/4a.tcp-networking.adocis excluded by!**/doc/**doc/modules/ROOT/pages/4.guide/4p.unix-sockets.adocis excluded by!**/doc/**doc/modules/ROOT/pages/glossary.adocis excluded by!**/doc/**doc/modules/ROOT/pages/index.adocis excluded by!**/doc/**perf/bench/CMakeLists.txtis excluded by!**/bench/**perf/bench/asio/callback/benchmarks.hppis excluded by!**/bench/**perf/bench/asio/callback/unix_socket_latency_bench.cppis excluded by!**/bench/**perf/bench/asio/callback/unix_socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/asio/coroutine/benchmarks.hppis excluded by!**/bench/**perf/bench/asio/coroutine/unix_socket_latency_bench.cppis excluded by!**/bench/**perf/bench/asio/coroutine/unix_socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/asio/unix_socket_utils.hppis excluded by!**/bench/**perf/bench/corosio/benchmarks.hppis excluded by!**/bench/**perf/bench/corosio/unix_socket_latency_bench.cppis excluded by!**/bench/**perf/bench/corosio/unix_socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/main.cppis excluded by!**/bench/**test/unit/local_datagram_socket.cppis excluded by!**/test/**test/unit/local_stream_socket.cppis excluded by!**/test/**test/unit/tcp_socket.cppis excluded by!**/test/**test/unit/udp_socket.cppis excluded by!**/test/**
📒 Files selected for processing (76)
include/boost/corosio.hppinclude/boost/corosio/backend.hppinclude/boost/corosio/detail/config.hppinclude/boost/corosio/detail/local_datagram_service.hppinclude/boost/corosio/detail/local_stream_acceptor_service.hppinclude/boost/corosio/detail/local_stream_service.hppinclude/boost/corosio/detail/op_base.hppinclude/boost/corosio/io/io_object.hppinclude/boost/corosio/io/io_read_stream.hppinclude/boost/corosio/io/io_write_stream.hppinclude/boost/corosio/local_datagram.hppinclude/boost/corosio/local_datagram_socket.hppinclude/boost/corosio/local_endpoint.hppinclude/boost/corosio/local_socket_pair.hppinclude/boost/corosio/local_stream.hppinclude/boost/corosio/local_stream_acceptor.hppinclude/boost/corosio/local_stream_socket.hppinclude/boost/corosio/message_flags.hppinclude/boost/corosio/native/detail/endpoint_convert.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_traits.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_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_traits.hppinclude/boost/corosio/native/detail/kqueue/kqueue_udp_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_udp_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_acceptor.hppinclude/boost/corosio/native/detail/reactor/reactor_acceptor_service.hppinclude/boost/corosio/native/detail/reactor/reactor_backend.hppinclude/boost/corosio/native/detail/reactor/reactor_basic_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_datagram_ops.hppinclude/boost/corosio/native/detail/reactor/reactor_datagram_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_op.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_finals.hppinclude/boost/corosio/native/detail/reactor/reactor_socket_finals.hppinclude/boost/corosio/native/detail/reactor/reactor_socket_service.hppinclude/boost/corosio/native/detail/reactor/reactor_stream_ops.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_traits.hppinclude/boost/corosio/native/detail/select/select_udp_service.hppinclude/boost/corosio/native/detail/select/select_udp_socket.hppinclude/boost/corosio/native/native_tcp_acceptor.hppinclude/boost/corosio/native/native_tcp_socket.hppinclude/boost/corosio/native/native_udp_socket.hppinclude/boost/corosio/shutdown_type.hppinclude/boost/corosio/tcp_socket.hppinclude/boost/corosio/udp_socket.hppperf/common/native_includes.hppsrc/corosio/src/io_context.cppsrc/corosio/src/local_datagram.cppsrc/corosio/src/local_datagram_socket.cppsrc/corosio/src/local_endpoint.cppsrc/corosio/src/local_socket_pair.cppsrc/corosio/src/local_stream.cppsrc/corosio/src/local_stream_acceptor.cppsrc/corosio/src/local_stream_socket.cppsrc/corosio/src/tcp_socket.cpp
💤 Files with no reviewable changes (23)
- include/boost/corosio/native/native_tcp_socket.hpp
- include/boost/corosio/native/native_tcp_acceptor.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_socket.hpp
- include/boost/corosio/native/detail/select/select_op.hpp
- include/boost/corosio/native/detail/select/select_tcp_acceptor.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_op.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_acceptor.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_udp_socket.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_socket.hpp
- include/boost/corosio/native/detail/select/select_tcp_socket.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_udp_socket.hpp
- include/boost/corosio/native/detail/select/select_tcp_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_op.hpp
- include/boost/corosio/native/detail/select/select_udp_socket.hpp
- include/boost/corosio/native/detail/select/select_udp_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_acceptor_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_udp_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_udp_service.hpp
- include/boost/corosio/native/detail/select/select_tcp_acceptor_service.hpp
✅ Files skipped from review due to trivial changes (12)
- include/boost/corosio/detail/config.hpp
- include/boost/corosio.hpp
- include/boost/corosio/native/detail/reactor/reactor_scheduler.hpp
- include/boost/corosio/shutdown_type.hpp
- src/corosio/src/local_datagram.cpp
- include/boost/corosio/detail/local_stream_service.hpp
- include/boost/corosio/local_datagram.hpp
- include/boost/corosio/local_stream.hpp
- include/boost/corosio/message_flags.hpp
- src/corosio/src/local_stream.cpp
- include/boost/corosio/detail/local_datagram_service.hpp
- include/boost/corosio/detail/op_base.hpp
🚧 Files skipped from review as they are similar to previous changes (10)
- include/boost/corosio/native/detail/select/select_scheduler.hpp
- include/boost/corosio/native/detail/reactor/reactor_socket_service.hpp
- src/corosio/src/tcp_socket.cpp
- perf/common/native_includes.hpp
- src/corosio/src/io_context.cpp
- include/boost/corosio/native/detail/iocp/win_udp_service.hpp
- include/boost/corosio/native/detail/reactor/reactor_acceptor_service.hpp
- include/boost/corosio/backend.hpp
- include/boost/corosio/native/detail/reactor/reactor_stream_ops.hpp
- include/boost/corosio/native/detail/reactor/reactor_service_finals.hpp
Add local_stream_socket, local_stream_acceptor, local_datagram_socket,
local_endpoint, and local_socket_pair for Unix domain socket I/O.
The public API mirrors TCP/UDP with protocol-appropriate differences
(path-based endpoints, socketpair, release_socket for fd passing).
Replace 42 per-backend reactor files (14 each for epoll, kqueue,
select) with parameterized templates instantiated from per-backend
traits structs. Each backend provides a traits type capturing its
platform-specific socket creation, write policy, and accept policy.
The reactor_types<Traits> template in reactor_backend.hpp generates
all concrete socket, service, acceptor, and op types. Virtual method
overrides move into the CRTP bases, and select's notify_reactor()
moves into register_op() via a compile-time trait, eliminating all
select-specific method overrides.
New files:
- {epoll,kqueue,select}_traits.hpp: per-backend policies
- reactor_{stream,datagram}_ops.hpp: parameterized op types
- reactor_socket_finals.hpp: final socket/acceptor types
- reactor_service_finals.hpp: per-protocol service types
- reactor_backend.hpp: accept impl and reactor_types bundle
mvandeberg
force-pushed
the
pr/122-unix-sockets-2
branch
from
b968d3c to
64cbfc7
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/boost/corosio/native/detail/reactor/reactor_acceptor.hpp (1)
279-289:⚠️ Potential issue | 🟠 MajorDeregister the acceptor even before the first accept op arms events.
do_listen()registers the fd immediately, but both teardown paths still useregistered_events != 0as a proxy for “registered”. If the acceptor is closed or released right afterlisten()and before anyaccept()starts, the descriptor stays in the scheduler registry with a dead fd.💡 Suggested fix
protected: Service& svc_; int fd_ = -1; Endpoint local_endpoint_; + bool descriptor_registered_ = false; @@ std::error_code do_listen(int backlog); @@ std::error_code reactor_acceptor<Derived, Service, Op, AcceptOp, DescState, ImplBase, Endpoint>:: do_listen(int backlog) { if (::listen(fd_, backlog) < 0) return make_err(errno); svc_.scheduler().register_descriptor(fd_, &desc_state_); + descriptor_registered_ = true; return {}; } @@ if (fd_ >= 0) { - if (desc_state_.registered_events != 0) + if (descriptor_registered_) svc_.scheduler().deregister_descriptor(fd_); ::close(fd_); fd_ = -1; } desc_state_.fd = -1; desc_state_.registered_events = 0; + descriptor_registered_ = false; @@ if (fd_ >= 0) { - if (desc_state_.registered_events != 0) + if (descriptor_registered_) svc_.scheduler().deregister_descriptor(fd_); fd_ = -1; } desc_state_.fd = -1; desc_state_.registered_events = 0; + descriptor_registered_ = false;Also applies to: 331-341, 385-389
🤖 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_acceptor.hpp` around lines 279 - 289, When tearing down the acceptor, don't rely on desc_state_.registered_events to decide whether to call svc_.scheduler().deregister_descriptor because do_listen() registers the fd immediately; instead, deregister whenever the descriptor was registered (e.g., check desc_state_.fd >= 0 or fd_ >= 0) before closing the fd. Update the teardown blocks that currently check desc_state_.registered_events (the block using fd_, desc_state_.registered_events and ::close, and the similar blocks at the other noted sites) to call svc_.scheduler().deregister_descriptor when the descriptor exists (fd/desc_state_.fd >= 0), then clear fd_ and desc_state_ as before so the descriptor is removed from the scheduler even if no accept() had yet armed events.
♻️ Duplicate comments (2)
include/boost/corosio/io/io_read_stream.hpp (1)
109-112:⚠️ Potential issue | 🟡 MinorUse
capy::cond::canceledin the cancellation contract.These docs still say cancellation yields
errc::operation_canceled. The public contract should describe the propagatedstop_token/cancel()behavior in terms of the library’s error condition and tell callers to compare conditions, not exact values.📝 Suggested wording
- Supports cancellation via the awaitable's stop_token or by - calling the owning stream's `cancel()` member. On cancellation, - yields `errc::operation_canceled`. + Supports cancellation via the stop_token propagated through the + IoAwaitable protocol or by calling the owning stream's + `cancel()` member. On cancellation, the first result compares + equal to `capy::cond::canceled`; compare error codes to error + conditions rather than specific values.As per coding guidelines, "Cancellation — State that the operation supports cancellation via stop_token propagated through the IoAwaitable protocol, or via the i/o object's cancel() member; state that the resulting error compares equal to capy::cond::canceled."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/io/io_read_stream.hpp` around lines 109 - 112, Update the cancellation paragraph in io_read_stream.hpp to reference capy::cond::canceled instead of errc::operation_canceled and state that cancellation is propagated via the IoAwaitable stop_token or the stream's cancel() member and that callers should compare the resulting error with capy::cond::canceled (i.e., use condition comparison, not exact value). Locate the text around the IO read stream documentation block that currently mentions "yields `errc::operation_canceled`" and replace that phrase and surrounding wording to match the coding guideline wording describing stop_token propagation and comparison to capy::cond::canceled.include/boost/corosio/local_stream_socket.hpp (1)
39-57:⚠️ Potential issue | 🟡 MinorFinish the async contract for
connect().The new docs are better, but they still don't spell out the completion conditions, the caller-side serialization rule for initiating operations, or the portable cancellation contract (
capy::cond::canceledrather than a specific error value). That leaves the public awaitable contract incomplete.As per coding guidelines, "Completion conditions — Bulleted
@lilist of conditions under which the operation completes and the coroutine resumes" and "Cancellation — State that the resulting error compares equal to capy::cond::canceled."Also applies to: 169-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/local_stream_socket.hpp` around lines 39 - 57, Update the documentation for local_stream_socket::connect() to fully specify the async awaitable contract: list explicit completion conditions (success on connected peer, failure with corresponding system error on connect failure, and aborted with capy::cond::canceled for cancellation), state the caller-side serialization rule (no concurrent connect() calls; operations of the same type must be serialized; one read and one write may be in flight), and declare the portable cancellation result compares equal to capy::cond::canceled rather than a platform-specific errno; apply the same additions to the duplicate doc block at lines ~169-190 to keep both docs consistent.
🧹 Nitpick comments (1)
include/boost/corosio/native/detail/reactor/reactor_stream_socket.hpp (1)
26-42: Add a file-level overview for the consolidated reactor core.This header now owns the shared speculative-I/O and reactor-registration flow for every backend, but the only prose here is the CRTP type comment. A short
/* */overview after the includes would make the backend/trait invariants much easier to audit and maintain.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 26 - 42, Add a file-level /* ... */ overview comment immediately after the `#include` section in reactor_stream_socket.hpp that summarizes the consolidated reactor core responsibilities: ownership of speculative I/O, reactor-registration/cancellation flow, relationship with reactor_basic_socket, and invariants expected by CRTP types (Derived, Service, ConnOp, ReadOp, WriteOp, DescState, ImplBase, Endpoint). Keep it short (5–8 lines) and describe how connect/read/write/shutdown dispatch and descriptor_state management are coordinated so reviewers can quickly audit backend/trait expectations without diving into implementation.
🤖 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 16-30: The public header backend.hpp is including
reactor_backend.hpp and platform-specific *_traits.hpp which pull in
POSIX/native headers; to fix, remove those direct includes from
include/boost/corosio/backend.hpp and instead create a lightweight non-native
shim (e.g., a new header like
include/boost/corosio/detail/reactor_types_shim.hpp) that only contains the
alias-only mapping (reactor_types) or forward declarations without including
native headers, then have reactor_backend.hpp and the platform-specific
*_traits.hpp include the real native headers and the shim include the alias
mapping; update backend.hpp to include only the shim (or forward declaration) so
it no longer transitively pulls in <sys/socket.h> etc., and ensure symbols
referenced in backend.hpp (reactor_types) are resolved via the shim in
compilation units that need the real traits.
In `@include/boost/corosio/local_stream_acceptor.hpp`:
- Around line 110-121: The current await_resume() can turn a completed accept
into cancellation by checking token_.stop_requested() before verifying ec_ and
peer_impl_; change the logic so a successful accept (ec_ indicates success and
peer_impl_ is non-null) is honored even if stop_requested() became true
afterward: first test if (ec_ || !peer_impl_) and only if that indicates failure
consider returning the operation_canceled when token_.stop_requested();
otherwise construct the local_stream_socket via reset_peer_impl(peer,
peer_impl_) and return the successful result. Apply the same fix to the other
await_resume() variant around lines 158–165.
In `@include/boost/corosio/local_stream_socket.hpp`:
- Around line 64-99: Document the public backend hook struct implementation and
each pure virtual method (connect, shutdown, native_handle, release_socket,
cancel, set_option, get_option, local_endpoint, remote_endpoint): add a brief
docstring for the struct describing its role as the platform backend interface,
then for each method document parameters and return values, ownership and
lifetime rules (who owns/relinquishes sockets or handles), error-reporting
semantics (how std::error_code is used and when it must be set), cancellation
behavior and thread-safety guarantees for cancel and connect
(preconditions/postconditions around stop tokens and coroutine handles), and any
platform-specific expectations for option calls (valid levels/optnames, how size
is used/updated); ensure the docs are placed in this public header near struct
implementation so backend implementers follow the same contract.
In `@include/boost/corosio/native/detail/reactor/reactor_backend.hpp`:
- Around line 67-104: The inline-accept fast path in reactor_backend.hpp skips
complete_accept_op(), leaving op.stop_cb set and allowing a stale stop callback
to fire against this->acc_ later; fix by clearing the stop callback before
returning from the inline branch—e.g., reset op.stop_cb (or call the op/accept
stop-clear helper) after setting *impl_out and before op.cont_op.cont.h = h /
return dispatch_coro(...), ensuring the accept object's stop state is cleared
for this accept.
---
Outside diff comments:
In `@include/boost/corosio/native/detail/reactor/reactor_acceptor.hpp`:
- Around line 279-289: When tearing down the acceptor, don't rely on
desc_state_.registered_events to decide whether to call
svc_.scheduler().deregister_descriptor because do_listen() registers the fd
immediately; instead, deregister whenever the descriptor was registered (e.g.,
check desc_state_.fd >= 0 or fd_ >= 0) before closing the fd. Update the
teardown blocks that currently check desc_state_.registered_events (the block
using fd_, desc_state_.registered_events and ::close, and the similar blocks at
the other noted sites) to call svc_.scheduler().deregister_descriptor when the
descriptor exists (fd/desc_state_.fd >= 0), then clear fd_ and desc_state_ as
before so the descriptor is removed from the scheduler even if no accept() had
yet armed events.
---
Duplicate comments:
In `@include/boost/corosio/io/io_read_stream.hpp`:
- Around line 109-112: Update the cancellation paragraph in io_read_stream.hpp
to reference capy::cond::canceled instead of errc::operation_canceled and state
that cancellation is propagated via the IoAwaitable stop_token or the stream's
cancel() member and that callers should compare the resulting error with
capy::cond::canceled (i.e., use condition comparison, not exact value). Locate
the text around the IO read stream documentation block that currently mentions
"yields `errc::operation_canceled`" and replace that phrase and surrounding
wording to match the coding guideline wording describing stop_token propagation
and comparison to capy::cond::canceled.
In `@include/boost/corosio/local_stream_socket.hpp`:
- Around line 39-57: Update the documentation for local_stream_socket::connect()
to fully specify the async awaitable contract: list explicit completion
conditions (success on connected peer, failure with corresponding system error
on connect failure, and aborted with capy::cond::canceled for cancellation),
state the caller-side serialization rule (no concurrent connect() calls;
operations of the same type must be serialized; one read and one write may be in
flight), and declare the portable cancellation result compares equal to
capy::cond::canceled rather than a platform-specific errno; apply the same
additions to the duplicate doc block at lines ~169-190 to keep both docs
consistent.
---
Nitpick comments:
In `@include/boost/corosio/native/detail/reactor/reactor_stream_socket.hpp`:
- Around line 26-42: Add a file-level /* ... */ overview comment immediately
after the `#include` section in reactor_stream_socket.hpp that summarizes the
consolidated reactor core responsibilities: ownership of speculative I/O,
reactor-registration/cancellation flow, relationship with reactor_basic_socket,
and invariants expected by CRTP types (Derived, Service, ConnOp, ReadOp,
WriteOp, DescState, ImplBase, Endpoint). Keep it short (5–8 lines) and describe
how connect/read/write/shutdown dispatch and descriptor_state management are
coordinated so reviewers can quickly audit backend/trait expectations without
diving into implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f1b8d525-9117-449c-8dd0-9e405a04f41f
⛔ Files ignored due to path filters (21)
doc/modules/ROOT/nav.adocis excluded by!**/doc/**doc/modules/ROOT/pages/4.guide/4a.tcp-networking.adocis excluded by!**/doc/**doc/modules/ROOT/pages/4.guide/4p.unix-sockets.adocis excluded by!**/doc/**doc/modules/ROOT/pages/glossary.adocis excluded by!**/doc/**doc/modules/ROOT/pages/index.adocis excluded by!**/doc/**perf/bench/CMakeLists.txtis excluded by!**/bench/**perf/bench/asio/callback/benchmarks.hppis excluded by!**/bench/**perf/bench/asio/callback/unix_socket_latency_bench.cppis excluded by!**/bench/**perf/bench/asio/callback/unix_socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/asio/coroutine/benchmarks.hppis excluded by!**/bench/**perf/bench/asio/coroutine/unix_socket_latency_bench.cppis excluded by!**/bench/**perf/bench/asio/coroutine/unix_socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/asio/unix_socket_utils.hppis excluded by!**/bench/**perf/bench/corosio/benchmarks.hppis excluded by!**/bench/**perf/bench/corosio/unix_socket_latency_bench.cppis excluded by!**/bench/**perf/bench/corosio/unix_socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/main.cppis excluded by!**/bench/**test/unit/local_datagram_socket.cppis excluded by!**/test/**test/unit/local_stream_socket.cppis excluded by!**/test/**test/unit/tcp_socket.cppis excluded by!**/test/**test/unit/udp_socket.cppis excluded by!**/test/**
📒 Files selected for processing (82)
include/boost/corosio.hppinclude/boost/corosio/backend.hppinclude/boost/corosio/detail/config.hppinclude/boost/corosio/detail/local_datagram_service.hppinclude/boost/corosio/detail/local_stream_acceptor_service.hppinclude/boost/corosio/detail/local_stream_service.hppinclude/boost/corosio/detail/op_base.hppinclude/boost/corosio/detail/tcp_acceptor_service.hppinclude/boost/corosio/detail/tcp_service.hppinclude/boost/corosio/detail/udp_service.hppinclude/boost/corosio/io/io_object.hppinclude/boost/corosio/io/io_read_stream.hppinclude/boost/corosio/io/io_write_stream.hppinclude/boost/corosio/local_datagram.hppinclude/boost/corosio/local_datagram_socket.hppinclude/boost/corosio/local_endpoint.hppinclude/boost/corosio/local_socket_pair.hppinclude/boost/corosio/local_stream.hppinclude/boost/corosio/local_stream_acceptor.hppinclude/boost/corosio/local_stream_socket.hppinclude/boost/corosio/message_flags.hppinclude/boost/corosio/native/detail/endpoint_convert.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_traits.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_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_traits.hppinclude/boost/corosio/native/detail/kqueue/kqueue_udp_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_udp_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_acceptor.hppinclude/boost/corosio/native/detail/reactor/reactor_acceptor_service.hppinclude/boost/corosio/native/detail/reactor/reactor_backend.hppinclude/boost/corosio/native/detail/reactor/reactor_basic_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_datagram_ops.hppinclude/boost/corosio/native/detail/reactor/reactor_datagram_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_op.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_finals.hppinclude/boost/corosio/native/detail/reactor/reactor_socket_finals.hppinclude/boost/corosio/native/detail/reactor/reactor_socket_service.hppinclude/boost/corosio/native/detail/reactor/reactor_stream_ops.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_traits.hppinclude/boost/corosio/native/detail/select/select_udp_service.hppinclude/boost/corosio/native/detail/select/select_udp_socket.hppinclude/boost/corosio/native/native_tcp_acceptor.hppinclude/boost/corosio/native/native_tcp_socket.hppinclude/boost/corosio/native/native_udp_socket.hppinclude/boost/corosio/shutdown_type.hppinclude/boost/corosio/socket_option.hppinclude/boost/corosio/tcp.hppinclude/boost/corosio/tcp_socket.hppinclude/boost/corosio/udp.hppinclude/boost/corosio/udp_socket.hppperf/common/native_includes.hppsrc/corosio/src/io_context.cppsrc/corosio/src/local_datagram.cppsrc/corosio/src/local_datagram_socket.cppsrc/corosio/src/local_endpoint.cppsrc/corosio/src/local_socket_pair.cppsrc/corosio/src/local_stream.cppsrc/corosio/src/local_stream_acceptor.cppsrc/corosio/src/local_stream_socket.cppsrc/corosio/src/tcp_socket.cpp
💤 Files with no reviewable changes (23)
- include/boost/corosio/native/native_tcp_acceptor.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_op.hpp
- include/boost/corosio/native/native_tcp_socket.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_socket.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_acceptor.hpp
- include/boost/corosio/native/detail/select/select_tcp_acceptor.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_service.hpp
- include/boost/corosio/native/detail/select/select_tcp_socket.hpp
- include/boost/corosio/native/detail/select/select_tcp_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_udp_service.hpp
- include/boost/corosio/native/detail/select/select_op.hpp
- include/boost/corosio/native/detail/epoll/epoll_op.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_udp_socket.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_socket.hpp
- include/boost/corosio/native/detail/select/select_udp_service.hpp
- include/boost/corosio/native/detail/select/select_udp_socket.hpp
- include/boost/corosio/native/detail/epoll/epoll_udp_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor_service.hpp
- include/boost/corosio/native/detail/select/select_tcp_acceptor_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_acceptor_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_udp_socket.hpp
✅ Files skipped from review due to trivial changes (22)
- include/boost/corosio/socket_option.hpp
- include/boost/corosio/tcp.hpp
- include/boost/corosio.hpp
- include/boost/corosio/detail/config.hpp
- include/boost/corosio/detail/udp_service.hpp
- include/boost/corosio/udp.hpp
- include/boost/corosio/detail/tcp_service.hpp
- include/boost/corosio/shutdown_type.hpp
- include/boost/corosio/detail/tcp_acceptor_service.hpp
- include/boost/corosio/local_stream.hpp
- include/boost/corosio/local_datagram.hpp
- include/boost/corosio/local_socket_pair.hpp
- include/boost/corosio/detail/local_stream_service.hpp
- src/corosio/src/local_datagram.cpp
- include/boost/corosio/detail/local_stream_acceptor_service.hpp
- include/boost/corosio/message_flags.hpp
- src/corosio/src/local_stream.cpp
- include/boost/corosio/detail/local_datagram_service.hpp
- include/boost/corosio/udp_socket.hpp
- include/boost/corosio/detail/op_base.hpp
- include/boost/corosio/io/io_write_stream.hpp
- include/boost/corosio/native/detail/reactor/reactor_datagram_socket.hpp
🚧 Files skipped from review as they are similar to previous changes (9)
- include/boost/corosio/native/detail/select/select_scheduler.hpp
- include/boost/corosio/native/detail/reactor/reactor_scheduler.hpp
- src/corosio/src/tcp_socket.cpp
- src/corosio/src/io_context.cpp
- include/boost/corosio/local_endpoint.hpp
- include/boost/corosio/native/detail/iocp/win_udp_socket.hpp
- include/boost/corosio/native/detail/iocp/win_udp_service.hpp
- include/boost/corosio/native/detail/reactor/reactor_datagram_ops.hpp
- include/boost/corosio/local_datagram_socket.hpp
Comment on lines
+16
to
+30
| #if BOOST_COROSIO_HAS_EPOLL || BOOST_COROSIO_HAS_SELECT || BOOST_COROSIO_HAS_KQUEUE | ||
| #include <boost/corosio/native/detail/reactor/reactor_backend.hpp> | ||
| #endif | ||
|
|
||
| #if BOOST_COROSIO_HAS_EPOLL | ||
| #include <boost/corosio/native/detail/epoll/epoll_traits.hpp> | ||
| #endif | ||
|
|
||
| #if BOOST_COROSIO_HAS_SELECT | ||
| #include <boost/corosio/native/detail/select/select_traits.hpp> | ||
| #endif | ||
|
|
||
| #if BOOST_COROSIO_HAS_KQUEUE | ||
| #include <boost/corosio/native/detail/kqueue/kqueue_traits.hpp> | ||
| #endif |
There was a problem hiding this comment.
Keep backend.hpp free of native POSIX headers.
This refactor makes the top-level public header depend on reactor_backend.hpp and the backend *_traits.hpp files, which transitively pull in native headers like <sys/socket.h> and <netinet/in.h>. That breaks the portable-header boundary for include/boost/corosio/backend.hpp. Please move the alias-only reactor_types mapping behind a lightweight shim, or otherwise keep the native traits out of this header.
As per coding guidelines "Headers in include/boost/corosio/, include/boost/corosio/detail/, and include/boost/corosio/concept/ MUST NOT include Unix/POSIX-specific headers such as <sys/socket.h>, <netinet/in.h>, <arpa/inet.h>, <unistd.h>, <sys/types.h>, or platform-specific <errno.h> constants".
🤖 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 16 - 30, The public header
backend.hpp is including reactor_backend.hpp and platform-specific *_traits.hpp
which pull in POSIX/native headers; to fix, remove those direct includes from
include/boost/corosio/backend.hpp and instead create a lightweight non-native
shim (e.g., a new header like
include/boost/corosio/detail/reactor_types_shim.hpp) that only contains the
alias-only mapping (reactor_types) or forward declarations without including
native headers, then have reactor_backend.hpp and the platform-specific
*_traits.hpp include the real native headers and the shim include the alias
mapping; update backend.hpp to include only the shim (or forward declaration) so
it no longer transitively pulls in <sys/socket.h> etc., and ensure symbols
referenced in backend.hpp (reactor_types) are resolved via the shim in
compilation units that need the real traits.
Comment on lines
+110
to
+121
| capy::io_result<local_stream_socket> await_resume() const noexcept | ||
| { | ||
| if (token_.stop_requested()) | ||
| return {make_error_code(std::errc::operation_canceled), | ||
| local_stream_socket()}; | ||
|
|
||
| if (ec_ || !peer_impl_) | ||
| return {ec_, local_stream_socket()}; | ||
|
|
||
| local_stream_socket peer(acc_.ctx_); | ||
| reset_peer_impl(peer, peer_impl_); | ||
| return {ec_, std::move(peer)}; |
There was a problem hiding this comment.
Don’t let a late stop request overwrite a completed accept.
await_suspend() already records pre-cancel into ec_, and the backend completion path records in-flight cancellation there too. Re-checking token_.stop_requested() in await_resume() means a stop request that arrives after a successful accept but before resumption turns success into cancellation and drops peer_impl_.
💡 Suggested fix
capy::io_result<local_stream_socket> await_resume() const noexcept
{
- if (token_.stop_requested())
- return {make_error_code(std::errc::operation_canceled),
- local_stream_socket()};
-
if (ec_ || !peer_impl_)
return {ec_, local_stream_socket()};
local_stream_socket peer(acc_.ctx_);
reset_peer_impl(peer, peer_impl_);
return {ec_, std::move(peer)};
}
@@
capy::io_result<> await_resume() const noexcept
{
- if (token_.stop_requested())
- return {make_error_code(std::errc::operation_canceled)};
-
if (!ec_ && peer_impl_)
peer_.h_.reset(peer_impl_);
return {ec_};
}Also applies to: 158-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/boost/corosio/local_stream_acceptor.hpp` around lines 110 - 121, The
current await_resume() can turn a completed accept into cancellation by checking
token_.stop_requested() before verifying ec_ and peer_impl_; change the logic so
a successful accept (ec_ indicates success and peer_impl_ is non-null) is
honored even if stop_requested() became true afterward: first test if (ec_ ||
!peer_impl_) and only if that indicates failure consider returning the
operation_canceled when token_.stop_requested(); otherwise construct the
local_stream_socket via reset_peer_impl(peer, peer_impl_) and return the
successful result. Apply the same fix to the other await_resume() variant around
lines 158–165.
Comment on lines
+64
to
+99
| /** Define backend hooks for local stream socket operations. | ||
|
|
||
| Platform backends (epoll, kqueue, select) derive from this | ||
| to implement socket I/O, connection, and option management. | ||
| */ | ||
| struct implementation : io_stream::implementation | ||
| { | ||
| virtual std::coroutine_handle<> connect( | ||
| std::coroutine_handle<> h, | ||
| capy::executor_ref ex, | ||
| corosio::local_endpoint ep, | ||
| std::stop_token token, | ||
| std::error_code* ec) = 0; | ||
|
|
||
| virtual std::error_code shutdown(shutdown_type what) noexcept = 0; | ||
|
|
||
| virtual native_handle_type native_handle() const noexcept = 0; | ||
|
|
||
| virtual native_handle_type release_socket() noexcept = 0; | ||
|
|
||
| virtual void cancel() noexcept = 0; | ||
|
|
||
| virtual std::error_code set_option( | ||
| int level, | ||
| int optname, | ||
| void const* data, | ||
| std::size_t size) noexcept = 0; | ||
|
|
||
| virtual std::error_code | ||
| get_option(int level, int optname, void* data, std::size_t* size) | ||
| const noexcept = 0; | ||
|
|
||
| virtual corosio::local_endpoint local_endpoint() const noexcept = 0; | ||
|
|
||
| virtual corosio::local_endpoint remote_endpoint() const noexcept = 0; | ||
| }; |
There was a problem hiding this comment.
Document the implementation hook contract.
These pure virtuals are a public extension surface, but their ownership, error-reporting, and cancellation semantics are still implicit. Please add brief docs for each method so backend implementations agree on what connect, release_socket, endpoint accessors, and option calls must guarantee.
As per coding guidelines, "Docstrings are required for all classes and functions in public headers in non-detail namespaces. Docstrings should include a brief description of purpose, document all parameters, return values, and any preconditions, postconditions, or exceptions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/boost/corosio/local_stream_socket.hpp` around lines 64 - 99, Document
the public backend hook struct implementation and each pure virtual method
(connect, shutdown, native_handle, release_socket, cancel, set_option,
get_option, local_endpoint, remote_endpoint): add a brief docstring for the
struct describing its role as the platform backend interface, then for each
method document parameters and return values, ownership and lifetime rules (who
owns/relinquishes sockets or handles), error-reporting semantics (how
std::error_code is used and when it must be set), cancellation behavior and
thread-safety guarantees for cancel and connect (preconditions/postconditions
around stop tokens and coroutine handles), and any platform-specific
expectations for option calls (valid levels/optnames, how size is used/updated);
ensure the docs are placed in this public header near struct implementation so
backend implementers follow the same contract.
Comment on lines
+67
to
+104
| if (this->svc_.scheduler().try_consume_inline_budget()) | ||
| { | ||
| auto* socket_svc = this->svc_.stream_service(); | ||
| if (socket_svc) | ||
| { | ||
| auto& impl = | ||
| static_cast<socket_final&>(*socket_svc->construct()); | ||
| impl.set_socket(accepted); | ||
|
|
||
| impl.desc_state_.fd = accepted; | ||
| { | ||
| std::lock_guard lock(impl.desc_state_.mutex); | ||
| impl.desc_state_.read_op = nullptr; | ||
| impl.desc_state_.write_op = nullptr; | ||
| impl.desc_state_.connect_op = nullptr; | ||
| } | ||
| socket_svc->scheduler().register_descriptor( | ||
| accepted, &impl.desc_state_); | ||
|
|
||
| impl.set_endpoints( | ||
| this->local_endpoint_, | ||
| from_sockaddr_as( | ||
| peer_storage, peer_addrlen, Endpoint{})); | ||
|
|
||
| *ec = {}; | ||
| if (impl_out) | ||
| *impl_out = &impl; | ||
| } | ||
| else | ||
| { | ||
| ::close(accepted); | ||
| *ec = make_err(ENOENT); | ||
| if (impl_out) | ||
| *impl_out = nullptr; | ||
| } | ||
| op.cont_op.cont.h = h; | ||
| return dispatch_coro(ex, op.cont_op.cont); | ||
| } |
There was a problem hiding this comment.
Reset the stop callback on the inline accept fast path.
This branch resumes directly instead of going through complete_accept_op(), so op.stop_cb never gets cleared. A later stop request can still fire the stale callback against this->acc_ after the accept has already completed, which can cancel the next accept on the same object.
💡 Suggested fix
if (this->svc_.scheduler().try_consume_inline_budget())
{
+ op.stop_cb.reset();
auto* socket_svc = this->svc_.stream_service();
if (socket_svc)
{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (this->svc_.scheduler().try_consume_inline_budget()) | |
| { | |
| auto* socket_svc = this->svc_.stream_service(); | |
| if (socket_svc) | |
| { | |
| auto& impl = | |
| static_cast<socket_final&>(*socket_svc->construct()); | |
| impl.set_socket(accepted); | |
| impl.desc_state_.fd = accepted; | |
| { | |
| std::lock_guard lock(impl.desc_state_.mutex); | |
| impl.desc_state_.read_op = nullptr; | |
| impl.desc_state_.write_op = nullptr; | |
| impl.desc_state_.connect_op = nullptr; | |
| } | |
| socket_svc->scheduler().register_descriptor( | |
| accepted, &impl.desc_state_); | |
| impl.set_endpoints( | |
| this->local_endpoint_, | |
| from_sockaddr_as( | |
| peer_storage, peer_addrlen, Endpoint{})); | |
| *ec = {}; | |
| if (impl_out) | |
| *impl_out = &impl; | |
| } | |
| else | |
| { | |
| ::close(accepted); | |
| *ec = make_err(ENOENT); | |
| if (impl_out) | |
| *impl_out = nullptr; | |
| } | |
| op.cont_op.cont.h = h; | |
| return dispatch_coro(ex, op.cont_op.cont); | |
| } | |
| if (this->svc_.scheduler().try_consume_inline_budget()) | |
| { | |
| op.stop_cb.reset(); | |
| auto* socket_svc = this->svc_.stream_service(); | |
| if (socket_svc) | |
| { | |
| auto& impl = | |
| static_cast<socket_final&>(*socket_svc->construct()); | |
| impl.set_socket(accepted); | |
| impl.desc_state_.fd = accepted; | |
| { | |
| std::lock_guard lock(impl.desc_state_.mutex); | |
| impl.desc_state_.read_op = nullptr; | |
| impl.desc_state_.write_op = nullptr; | |
| impl.desc_state_.connect_op = nullptr; | |
| } | |
| socket_svc->scheduler().register_descriptor( | |
| accepted, &impl.desc_state_); | |
| impl.set_endpoints( | |
| this->local_endpoint_, | |
| from_sockaddr_as( | |
| peer_storage, peer_addrlen, Endpoint{})); | |
| *ec = {}; | |
| if (impl_out) | |
| *impl_out = &impl; | |
| } | |
| else | |
| { | |
| ::close(accepted); | |
| *ec = make_err(ENOENT); | |
| if (impl_out) | |
| *impl_out = nullptr; | |
| } | |
| op.cont_op.cont.h = h; | |
| return dispatch_coro(ex, op.cont_op.cont); | |
| } |
🤖 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_backend.hpp` around lines
67 - 104, The inline-accept fast path in reactor_backend.hpp skips
complete_accept_op(), leaving op.stop_cb set and allowing a stale stop callback
to fire against this->acc_ later; fix by clearing the stop callback before
returning from the inline branch—e.g., reset op.stop_cb (or call the op/accept
stop-clear helper) after setting *impl_out and before op.cont_op.cont.h = h /
return dispatch_coro(...), ensuring the accept object's stop state is cleared
for this accept.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add local_stream_socket, local_stream_acceptor, local_datagram_socket, local_endpoint, and local_socket_pair for Unix domain socket I/O. The public API mirrors TCP/UDP with protocol-appropriate differences (path-based endpoints, socketpair, release_socket for fd passing).
Replace 42 per-backend reactor files (14 each for epoll, kqueue, select) with parameterized templates instantiated from per-backend traits structs. Each backend provides a traits type capturing its platform-specific socket creation, write policy, and accept policy.
The reactor_types template in reactor_backend.hpp generates all concrete socket, service, acceptor, and op types. Virtual method overrides move into the CRTP bases, and select's notify_reactor() moves into register_op() via a compile-time trait, eliminating all select-specific method overrides.
New files:
Benchmarks
Summary by CodeRabbit
New Features
API
Refactor
Chores