Local Sockets (Unix & Winsock)#229
Open
mvandeberg wants to merge 1 commit intocppalliance:developfrom
Open
Conversation
📝 WalkthroughWalkthroughComprehensive addition of Unix domain socket support (AF_UNIX) to Boost.Corosio, introducing local stream and datagram socket types with full async/coroutine support across epoll, select, kqueue, and IOCP backends. Includes new protocol types, endpoint representation, message flags, abstract service interfaces, platform-specific implementations, and reactor framework generalizations to support endpoint-agnostic socket types. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant SS as local_stream_socket
participant Svc as local_stream_service
participant Impl as backend::implementation
participant Sched as scheduler
rect rgba(100, 150, 200, 0.5)
Note over App,Sched: Local Stream Socket Connection & I/O
App->>SS: connect(local_endpoint)
SS->>SS: create connect_awaitable
App->>SS: co_await
SS->>SS: await_suspend(h, env)
SS->>Svc: get_impl().connect(h, executor, ep, ...)
Svc->>Impl: dispatch to native backend
Impl->>Sched: register_op(connect_op)
Note over Sched: Async operation pending
Sched->>Impl: operation complete / canceled
Impl->>Svc: call completion handler
Svc->>SS: resume coroutine with result
SS->>App: return io_result
end
rect rgba(150, 100, 200, 0.5)
Note over App,Sched: Accepted Socket from Acceptor
App->>Svc: accept() awaitable
Svc->>Impl: do_accept(...)
Impl->>Sched: accept4/accept (blocking or async)
Sched->>Impl: peer socket accepted
Impl->>Svc: construct socket impl from peer fd
Svc->>Svc: resolve local/remote endpoints
Svc->>App: resume with accepted socket
end
sequenceDiagram
participant App as Application
participant DS as local_datagram_socket
participant Svc as local_datagram_service
participant Impl as backend::implementation
participant OS as OS Socket (AF_UNIX)
rect rgba(200, 150, 100, 0.5)
Note over App,OS: Datagram Send with Message Flags
App->>DS: send_to(buffer, endpoint, msg_flags)
DS->>DS: create send_to_awaitable(flags)
App->>DS: co_await
DS->>Svc: dispatch(h, executor)
Svc->>Impl: send_to(h, ex, buf, ep, flags, token, ...)
Impl->>OS: sendmsg(fd, msg, MSG_* | flags)
OS-->>Impl: bytes_sent or error
Impl->>Svc: completion callback
Svc->>DS: resume with io_result<size_t>
DS->>App: return {bytes_sent} or error
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~180+ minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
An automated preview of the documentation is available at https://229.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-14 22:37:24 UTC |
|
GCOVR code coverage report https://229.corosio.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-04-14 22:44:42 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: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
include/boost/corosio/native/detail/reactor/reactor_basic_socket.hpp (1)
13-29:⚠️ Potential issue | 🟡 MinorAdd a file-level overview for the shared reactor teardown rules.
This header now carries a lot of subtle shared socket logic, including bind-time endpoint caching and the new release path, but it still has no
/* ... */overview after the includes explaining the descriptor-state synchronization model and howdo_close_socket()differs fromdo_release_socket(). A short maintainer-facing summary would help a lot here.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_basic_socket.hpp` around lines 13 - 29, Add a file-level /* ... */ comment block right after the includes that summarizes the reactor_basic_socket.hpp descriptor-state synchronization model and the shared teardown rules: describe the socket state machine (owned vs released vs closed), how bind-time endpoint caching is stored and used, and clearly explain the difference between do_close_socket() (immediate close and resource reclamation) and do_release_socket() (transfer of ownership/descriptor without closing), including any locking expectations and which thread/context must call each path; reference the reactor_basic_socket class and the functions do_close_socket() and do_release_socket() so maintainers can map the prose to the implementation.include/boost/corosio/udp_socket.hpp (1)
505-635:⚠️ Potential issue | 🟡 MinorDocument the new flagged awaitables with the full async contract.
These overloads add new public awaitable entry points, but the docs still only cover the immediate parameters. Please add completion conditions, cancellation semantics, serialization/concurrency constraints, and the returned tuple elements so the new overloads match the contract expected for public coroutine I/O APIs in this repo.
As per coding guidelines, "Docstrings are required for all classes and functions in public headers in non-detail namespaces" and async operations must document completion conditions and cancellation.
🤖 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 - 635, Add full async contract docstrings for the new public overloads send_to(Buffers const&,...), recv_from(Buffers const&,...), send(Buffers const&,...), and recv(Buffers const&,...): document completion conditions (when the awaitable completes successfully vs error), the exact returned io_result tuple elements (size and error info), cancellation behavior (what happens if the coroutine is cancelled or the socket is closed), and any serialization/concurrency constraints (whether operations are safe concurrently on the same socket or must be serialized). Match wording/structure to existing coroutine I/O API docs in this header (same sections for "return", "throws", "completion", "cancellation", "concurrency") so the overloads have identical contract detail as other public awaitables.
🟡 Minor comments (4)
include/boost/corosio/local_endpoint.hpp-26-76 (1)
26-76:⚠️ Potential issue | 🟡 MinorComplete the public
local_endpointAPI docs.
local_endpointis a public type, but the class header uses a plain block comment and the constructor/accessor docs still miss the importantstd::string_viewsemantics here: construction copies the path bytes into internal storage, whilepath()returns a view that is only valid while the endpoint object remains alive. That detail should be explicit in the generated API docs.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_endpoint.hpp` around lines 26 - 76, Add full Doxygen-style public API docs for the class local_endpoint and its public members: document that local_endpoint stores at most max_path_length bytes by copying the contents of the input std::string_view into internal storage (so the caller may free/modify the original string), describe that path() returns a std::string_view that refers to the internal buffer and is only valid while the local_endpoint object exists, document the default constructor (creates an empty/unbound endpoint), document the throwing constructor local_endpoint(std::string_view) (throws std::system_error if path length > max_path_length), document the non-throwing constructor local_endpoint(std::string_view, std::error_code&) (sets ec on error and is noexcept), and document the max_path_length constant and thread-safety guarantees; use Doxygen tags for parameters, return values, exceptions, and remarks so these details appear in generated API docs.src/corosio/src/local_stream_socket.cpp-18-22 (1)
18-22:⚠️ Potential issue | 🟡 MinorInclude
<cerrno>explicitly before usingerrnoon the POSIX path.The
available()method readserrnowhen the::ioctl()call fails, but this file does not explicitly include<cerrno>. The declaration is currently dependent on<sys/ioctl.h>transitively providing it, which is not portable.Suggested change
`#if` BOOST_COROSIO_POSIX +#include <cerrno> `#include` <sys/ioctl.h> `#elif` BOOST_COROSIO_HAS_IOCP `#include` <boost/corosio/native/detail/iocp/win_windows.hpp> `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/corosio/src/local_stream_socket.cpp` around lines 18 - 22, Add an explicit `#include` <cerrno> to the POSIX branch so errno is declared before it is read in the available() method; update the preprocessor include block that currently has `#if` BOOST_COROSIO_POSIX / `#include` <sys/ioctl.h> to also `#include` <cerrno> (referencing the available() method in local_stream_socket.cpp which reads errno after ::ioctl()).src/corosio/src/local_datagram_socket.cpp-18-22 (1)
18-22:⚠️ Potential issue | 🟡 MinorAdd explicit
#include <cerrno>before usingerrnoon the POSIX path.The
available()function at line 149 readserrno, but this file only includes<sys/ioctl.h>. While this may build due to transitive includes from system headers, the C++ standard requires an explicit#include <cerrno>to guarantee access toerrno. The same issue exists inlocal_stream_socket.cppandlocal_socket_pair.cpp.Suggested change
`#if` BOOST_COROSIO_POSIX +#include <cerrno> `#include` <sys/ioctl.h> `#elif` BOOST_COROSIO_HAS_IOCP `#include` <boost/corosio/native/detail/iocp/win_windows.hpp> `#endif`🤖 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 18 - 22, The POSIX branch uses errno in the available() function but doesn't include <cerrno>; add an explicit `#include` <cerrno> inside the BOOST_COROSIO_POSIX include block in local_datagram_socket.cpp so errno is defined, and apply the same fix to the POSIX include blocks in local_stream_socket.cpp and local_socket_pair.cpp; update the include near the existing `#include` <sys/ioctl.h> so available() and any other uses of errno reference the proper header.include/boost/corosio/native/detail/epoll/epoll_local_stream_service.hpp-26-31 (1)
26-31:⚠️ Potential issue | 🟡 MinorInclude
<mutex>explicitly for clarity.This header uses
std::lock_guardon lines 183 and 208 but does not directly include<mutex>. Although<mutex>is transitively available viaepoll_scheduler.hpp,reactor_socket_service.hpp, andreactor_op_complete.hpp(included earlier), explicit inclusion follows best practices and makes the header's dependencies clearer.Suggested fix
`#include` <coroutine> +#include <mutex> `#include` <errno.h> `#include` <sys/socket.h>🤖 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_local_stream_service.hpp` around lines 26 - 31, This header uses std::lock_guard (used around lines referencing the epoll_local_stream_service implementation at lock sites) but doesn't include <mutex> directly; fix it by adding `#include` <mutex> near the top of include directives in epoll_local_stream_service.hpp so the dependency is explicit and build-safe for symbols like std::lock_guard and std::mutex used in the class/implementation.
🧹 Nitpick comments (9)
include/boost/corosio/detail/io_op_base.hpp (1)
13-23: Add a file-level implementation overview block after includes.This header has non-trivial coroutine/cancellation mechanics, but it lacks the required top-level
/* ... */maintainer overview right after the includes.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."Suggested patch
`#include` <stop_token> `#include` <system_error> +/* + Provides shared CRTP awaitable bases for coroutine I/O operations. + The bases centralize stop-token propagation from `capy::io_env`, + cancellation short-circuit behavior, and standardized `capy::io_result` + construction, while delegating backend-specific initiation to + `Derived::dispatch(...)`. +*/ + namespace boost::corosio::detail {🤖 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 - 23, Add a file-level /* ... */ maintainer overview comment immediately after the includes and before the namespace boost::corosio::detail in io_op_base.hpp that summarizes the coroutine and cancellation design used by this header (how coroutines are resumed, how stop_token/cancellation is observed and propagated, ownership/lifetime assumptions for any io operation objects, and any important invariants). Place the block at the top of the file (right after the last `#include` and before namespace boost::corosio::detail) and reference the key components used in the file (e.g., the coroutine machinery, stop_token usage, and any io operation types such as io_op_base or similarly named types) so maintainers can quickly understand the control flow and cancellation semantics without reading the full implementation.src/corosio/src/local_socket_pair.cpp (1)
10-22: Add the file-level overview comment after the includes.This file now has non-obvious fallback and ownership-transfer behavior around
socketpair,fcntl, and partial-failure cleanup. A short/* */maintainer overview right after the includes would make that flow 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 `@src/corosio/src/local_socket_pair.cpp` around lines 10 - 22, Add a file-level /* */ block comment immediately after the includes that gives a concise maintainer overview of the local socket pair implementation: explain that boost::corosio::local_socket_pair creates a POSIX socketpair, may set descriptors non-blocking via fcntl, transfers ownership of fds into the RAII wrapper, and contains specific fallback and partial-failure cleanup paths (close on error, handle partial success when one fd creation or fcntl fails); mention the key syscalls used (socketpair, fcntl, close) and the intended semantics on success vs. failure so future readers understand ownership transfer and error-handling behavior.include/boost/corosio/native/detail/reactor/reactor_basic_socket.hpp (1)
332-458: Extract the close/release teardown into one helper.
do_close_socket()anddo_release_socket()now duplicate the same cancellation, claim, and descriptor-state cleanup sequence. That block is concurrency-sensitive, so keeping two copies makes future fixes easy to apply inconsistently. Please centralize the shared teardown and keep the close-vs-release behavior as the small tail difference.Based on learnings, accesses to
impl_ref_andis_enqueued_in reactor close paths must stay underdesc_state_.mutexwith the established ordering.🤖 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 332 - 458, Both do_close_socket and do_release_socket duplicate the concurrency-sensitive cancellation/claim/descriptor-state cleanup; extract that shared sequence into a single helper (e.g., teardown_cancel_and_claim) and call it from both functions, leaving only the close-vs-release tails in each. The helper should: invoke for_each_op(request_cancel) on the Derived (use static_cast<Derived*>(this)), lock desc_state_.mutex while calling for_each_desc_entry and while reading/updating desc_state_.is_enqueued_ and desc_state_.impl_ref_ (preserving the existing ordering), clear the desc_state flags (read_ready/write_ready/read_cancel_pending/write_cancel_pending/connect_cancel_pending) and collect claimed reactor_op_base pointers, then after releasing the mutex set each claimed[i]->impl_ptr = self and call svc_.post(...) and svc_.work_finished(). Ensure fd_/registered_events cleanup and ::close(fd_) (for close) or leaving fd for return (for release) remain as the small function-specific tails in do_close_socket and do_release_socket respectively.src/corosio/src/local_datagram_socket.cpp (1)
14-24: Add a file-level overview block after the includes.This file now contains the platform-conditional lifecycle, shutdown, and
available()behavior for a new socket type, so it would benefit from the short maintainer-facing/* */overview the repo asks for on non-trivial 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_datagram_socket.cpp` around lines 14 - 24, Add a file-level C-style block comment immediately after the existing includes in local_datagram_socket.cpp that briefly summarizes the file's purpose and high-level behavior: mention that this file implements the platform-conditional lifecycle, shutdown, and available() behavior for the local_datagram_socket type (including POSIX vs IOCP differences), note any important invariants or threading/ownership expectations, and call out interactions with detail::local_datagram_service and detail::except; keep it short and maintainer-focused per repo guidelines.src/corosio/src/local_stream_socket.cpp (1)
14-24: Add a file-level overview block after the includes.This file carries the platform-conditional lifecycle and readiness-query logic for the new local stream socket, so a short
/* */maintainer overview would make the implementation 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 `@src/corosio/src/local_stream_socket.cpp` around lines 14 - 24, Add a file-level C-style comment block immediately after the includes that gives a short high-level overview of this translation unit: describe that src/corosio/src/local_stream_socket.cpp implements the platform-conditional lifecycle and readiness-query logic for local_stream_socket, mention the role of boost::corosio::detail::local_stream_service and the POSIX vs IOCP branches (BOOST_COROSIO_POSIX / BOOST_COROSIO_HAS_IOCP), and note any important invariants or threading/async expectations callers must respect; place this /* ... */ block before the namespace boost::corosio { line.src/corosio/src/io_context.cpp (1)
18-57: Add a short file-level overview comment.This file now wires four backend families plus the pre/post-construction option flow, so the control flow is no longer obvious from the includes and factory calls alone. A brief
/* ... */overview after the includes would make the service graph and construction order 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."Also applies to: 61-135
🤖 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 - 57, Add a brief file-level /* ... */ block immediately after the include section that summarizes the overall design: list the four backend families (epoll/select/kqueue/iocp) and key scheduler/service classes (e.g., epoll_scheduler, select_scheduler, kqueue_scheduler, win_scheduler and the corresponding *_tcp_service / *_udp_service / *_local_*_service classes), describe the pre/post-construction option flow and the service graph (which scheduler is selected and how acceptor/stream/datagram services are wired), and note construction/order constraints to follow when modifying io_context creation. This comment should be concise (2–5 sentences) and placed after the grouped includes so future readers can quickly see backend selection and construction order.include/boost/corosio/native/detail/select/select_local_datagram_service.hpp (1)
17-35: Add a file-level overview block comment.This header now owns enough select-specific socket lifecycle and completion wiring that it needs a short post-include
/* */overview, similar toselect_local_stream_service.hpp, to capture the reactor/registration model 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 `@include/boost/corosio/native/detail/select/select_local_datagram_service.hpp` around lines 17 - 35, Add a file-level /* */ overview comment immediately after the includes in select_local_datagram_service.hpp that summarizes the select-based reactor/registration model and socket lifecycle handled by this header (similar to the comment in select_local_stream_service.hpp); mention the roles of select_local_datagram_socket, select_scheduler, reactor_socket_service and reactor_op_complete, how sockets are registered/unregistered with the scheduler, where completion is posted, and any important threading/coroutine considerations so readers can quickly understand the high-level flow implemented in this file.include/boost/corosio/native/detail/epoll/epoll_local_stream_service.hpp (1)
17-31: Add a file-level overview block comment.This new service header has enough backend-specific lifecycle and registration logic that it should carry the same post-include
/* */overview as the select variant, so the invariants are obvious to the next maintainer.As per coding guidelines, "Files containing non-trivial implementation logic should include a
/* */block comment after the includes that provides a high-level overview of how the implementation works."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/native/detail/epoll/epoll_local_stream_service.hpp` around lines 17 - 31, The header lacks a file-level overview comment after the includes; add a C-style block comment immediately after the include section in epoll_local_stream_service.hpp that summarizes the service's high-level design, lifecycle and registration flow (how epoll_local_stream_service interacts with epoll_local_stream_socket, epoll_scheduler and reactor_socket_service, and how reactor_op_complete is used), states key invariants and any threading/ownership expectations, and mirrors the style/content of the select variant overview so future maintainers can quickly understand the backend-specific logic.src/corosio/src/local_stream_acceptor.cpp (1)
10-16: Add the file-level implementation overview block.This new
.cppnow contains the lifecycle/bind/listen glue for a public type, but it still lacks the required/* ... */overview after the includes. A short note covering the service-backed handle flow and the POSIXunlink_existingstep would make the control flow much easier to reason about 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 `@src/corosio/src/local_stream_acceptor.cpp` around lines 10 - 16, Add a file-level C-style block comment immediately after the `#include` section of local_stream_acceptor.cpp that summarizes the implementation: describe that this translation unit implements the lifecycle (construct/destruct), bind/listen operations for the public local_stream_acceptor handle, explain that it delegates work to detail::local_stream_acceptor_service, outline the service-backed handle flow (handle -> service calls -> platform specifics), and note the POSIX-specific step to unlink_existing socket paths before bind. Keep it brief (3-6 lines) and mention any error/cleanup interaction between unlink_existing and the service.
🤖 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_datagram_service.hpp`:
- Around line 57-71: Change the assign_socket signature to take the
platform-native handle type instead of int: replace the parameter type in
local_datagram_service::assign_socket with native_handle_type and update all
overriding methods to match (epoll_local_datagram_service::assign_socket,
select_local_datagram_service::assign_socket,
kqueue_local_datagram_service::assign_socket,
win_local_dgram_service::assign_socket). Ensure all implementations use the same
native_handle_type typedef/alias (or fully qualify it) and adjust any call
sites, error checks, and ownership/close logic that assumed an int to work with
the new type.
In `@include/boost/corosio/detail/local_stream_service.hpp`:
- Around line 53-68: Change the assign_socket interface to accept the
platform-neutral native_handle_type instead of int: update the pure virtual
method assign_socket(local_stream_socket::implementation& impl, int fd) to use
local_stream_socket::native_handle_type (or the appropriate using alias
native_handle_type) for the fd/handle parameter, and then update all
implementations (e.g. win_local_stream_service and any POSIX implementations)
and callers to match the new signature and to forward/return native_handle_type
where needed (ensure local_stream_socket::native_handle() and related
stream_file::assign()/random_access_file::assign() usage remain consistent).
In `@include/boost/corosio/local_datagram_socket.hpp`:
- Around line 40-66: Update the header docs for the asynchronous Unix datagram
socket class to fully specify the coroutine contract: for public APIs like
send_to, recv_from, send, recv, connect and the class itself, add explicit
completion conditions (when the awaitable resumes and what it guarantees on
success), cancellation semantics (how cancellation tokens or coroutine
cancellation are handled and what partial progress is possible),
concurrency/serialization rules (which operations share read/write slots and
which may not overlap), parameter/return value descriptions, notable error
conditions and errno mappings, and at least one short example showing connected
vs connectionless usage; place this information adjacent to the existing class
comment and mirror the same coverage for the other public method groups
referenced in the file (lines ~441-569) so callers can rely on precise
resumption/cancellation/error behavior.
- Around line 418-425: The assign() overloads currently take an int which can
truncate native_handle_type on some platforms; change all assign(int fd)
signatures to assign(native_handle_type fd) and update their doc comments to
reflect native_handle_type semantics (e.g., "invalid handle represented by
~native_handle_type(0)" or similar) so they match release()/native_handle()
return types and the is_open() checks; update all three assign overloads and
their documentation to use native_handle_type and ensure any internal
comparisons/assignments use native_handle_type as well so no narrowing occurs on
IOCP/64-bit builds.
In `@include/boost/corosio/local_stream_acceptor.hpp`:
- Around line 36-58: Update the public API documentation for
local_stream_acceptor and its related hooks: add full docstrings for the class
and every public method/overload (including accept() overloads, bind(),
release(), close(), and lifecycle methods) that specify extended description,
completion conditions, cancellation semantics, serialization/in-flight rules
(concurrency/overlap), params/returns, throws, and examples; also document the
public implementation hook implementation::accept() with its contract and
completion/cancellation behavior, and fix the contradictory release() docs
(clarify whether it returns -1 or throws logic_error and make behavior and
throws section consistent with implementation). Ensure the new comments follow
the existing header style and cover the listed ranges (including lines around
174-262 and 298-325) so all public awaitable surface matches the project's
docstring standards.
In `@include/boost/corosio/message_flags.hpp`:
- Around line 43-46: Add Doxygen documentation to the public enum message_flags
and its bitwise operators: document the enum's purpose (mapping portable flags
to platform MSG_* constants), describe each enumerator (peek, out_of_band,
do_not_route, none), and explain usage patterns (compose with operator|, test
with operator&). For operator~ specifically, change its semantics to stay within
the declared flag bits (do not produce out-of-domain values) by masking the
inverted value with the defined flag-bit mask (e.g., use a message_flags_mask
constant representing the OR of peek|out_of_band|do_not_route) and document that
all bitwise operator results may produce combinations of these bits; also add
docstrings for operator|, operator&, operator^ and operator~ describing their
behavior and return values.
In `@include/boost/corosio/native/detail/endpoint_convert.hpp`:
- Line 35: The header endpoint_convert.hpp uses std::min but doesn’t include
<algorithm>, causing reliance on transitive includes; add `#include` <algorithm>
near the top of the file (alongside the existing `#include` <cstddef>) so calls to
std::min (used in this header at the locations where endpoint conversion logic
computes minimums) are well-defined and the header no longer depends on include
order.
In `@include/boost/corosio/native/detail/iocp/win_local_dgram_service.hpp`:
- Around line 52-89: The win_local_dgram_service exposes AF_UNIX datagram
sockets on Windows even though SOCK_DGRAM is unsupported; update the
implementation to either compile-gate the entire win_local_dgram_service for
Windows platforms without AF_UNIX datagram support or explicitly reject
SOCK_DGRAM early. Specifically, either wrap the class and its registration in an
`#if` guard that excludes it on Windows without datagram AF_UNIX, or add a check
in win_local_dgram_service::open_socket and/or open_socket_internal that returns
a suitable std::error_code (e.g., std::errc::operation_not_supported or
WSAEOPNOTSUPP mapped) when type == SOCK_DGRAM (mirroring the existing
abstract-socket rejection behavior), and ensure the constructor or service
registration does not advertise datagram support on Windows.
In `@include/boost/corosio/native/detail/iocp/win_local_dgram_socket.hpp`:
- Around line 151-155: The IOCP implementation declares separate op slots (wr_,
send_wr_, rd_, recv_rd_) in win_local_dgram_socket but does not enforce the
public contract that send/send_to and recv/recv_from must not overlap; either
update the public doc or enforce mutual exclusion. To fix: in the methods
send(), send_to(), recv(), and recv_from() on win_local_dgram_socket, add
precondition checks that reject/start-fail if a conflicting operation is already
pending (e.g., use an atomic<bool> or existing pending-state on
local_dgram_send_op/local_dgram_recv_op) so send and send_to cannot both start
and recv and recv_from cannot both start; reference the member slots wr_,
send_wr_, rd_, recv_rd_ and the operation types
local_dgram_send_op/local_dgram_recv_op when implementing the guard. Ensure any
rejection returns the same error behavior documented by the public API (e.g.,
starting error or throwing) so the contract is enforced consistently.
In
`@include/boost/corosio/native/detail/iocp/win_local_stream_acceptor_service.hpp`:
- Around line 268-276: The acceptor lifetime guard is being set on
acc_.acceptor_ptr before calling op.reset(), which immediately clears it and can
lead to a null owner during the completion path; to fix, call op.reset() first,
then assign acc_.acceptor_ptr = shared_from_this(), and finally set op.h, op.ex,
op.ec_out, op.impl_out and call op.start(token) so the in-flight accept holds a
valid acceptor_ptr for the entire operation (refer to acc_.acceptor_ptr,
op.reset(), shared_from_this(), and op.start(token)).
In `@include/boost/corosio/native/detail/iocp/win_local_stream_service.hpp`:
- Around line 21-22: win_local_stream_service must not reuse TCP provider
extension pointers (tcp_svc_.connect_ex / tcp_svc_.accept_ex) because
ConnectEx/AcceptEx are for AF_INET/AF_INET6; update win_local_stream_service to
either load ConnectEx/AcceptEx separately for the AF_UNIX provider or, simpler,
call the standard Winsock APIs (connect/accept) for AF_UNIX operations: locate
methods in class win_local_stream_service that currently delegate to
tcp_svc_.connect_ex()/tcp_svc_.accept_ex and replace delegation with
AF_UNIX-safe calls (explicit use of ::connect/::accept or loading per-provider
extension pointers) and ensure error handling preserves returning WSAEOPNOTSUPP
only when appropriate; keep changes scoped to win_local_stream_service and its
connect/accept path so tcp_svc_ remains unchanged.
In
`@include/boost/corosio/native/detail/kqueue/kqueue_local_datagram_service.hpp`:
- Around line 300-321: The assign_socket function must normalize an adopted fd
like open_socket does: before storing fd into kqueue_local_datagram_socket::fd_
and registering the descriptor, use fcntl to set O_NONBLOCK and FD_CLOEXEC (get
current flags, add O_NONBLOCK; set FD_CLOEXEC via F_SETFD) and handle fcntl
failures by returning the appropriate errno (similar to open_socket); then
proceed to set desc_state_.fd, clear ops, and call
scheduler().register_descriptor. Ensure you reference and update
kqueue_local_datagram_service::assign_socket and mirror the normalization
behavior of open_socket.
- Around line 133-136: The operator() in kqueue_local_dgram_recv_op currently
calls complete_io_op which converts bytes_transferred==0 into capy::error::eof;
instead, implement and call a datagram-specific completion that preserves
zero-length datagrams (e.g. complete_datagram_io_op or a variant of
complete_io_op that accepts a datagram flag) from
kqueue_local_dgram_recv_op::operator() so its deferred recv() path returns
success with 0 bytes like reactor_datagram_socket::do_recv(); ensure the chosen
completion routine does not translate zero bytes into capy::error::eof and
updates the same completion state used by reactor_datagram_socket::do_recv().
- Around line 275-283: The code calls setsockopt(fd, SOL_SOCKET, SO_NOSIGPIPE,
...) unconditionally which may be undefined on non-BSD platforms; wrap the
SO_NOSIGPIPE block in an `#ifdef` SO_NOSIGPIPE guard like other kqueue services so
the setsockopt call and its error-handling (the int one = 1; the
if(::setsockopt(...) { int errn = errno; ::close(fd); return make_err(errn); })
are compiled only when SO_NOSIGPIPE is defined, preserving the existing behavior
otherwise.
In
`@include/boost/corosio/native/detail/kqueue/kqueue_local_stream_acceptor_service.hpp`:
- Around line 150-162: The setsockopt call that applies SO_NOSIGPIPE in the
accept path should be guarded by the same preprocessor check used elsewhere;
wrap the block that references SO_NOSIGPIPE (the int one = 1;
::setsockopt(accepted, SOL_SOCKET, SO_NOSIGPIPE, &one, sizeof(one)) ... error
handling) in `#ifdef` SO_NOSIGPIPE / `#endif` to avoid referencing the symbol on
platforms that lack it (mirror the guard used in open_acceptor_socket()); keep
the existing error handling (close(accepted), op.complete(...), op.impl_ptr =
shared_from_this(), svc_.post(&op), return std::noop_coroutine()) unchanged.
In `@include/boost/corosio/native/detail/kqueue/kqueue_local_stream_service.hpp`:
- Around line 166-250: The assigned socket path is incomplete: update
kqueue_local_stream_service::assign_socket to normalize the incoming fd the same
way open_socket does — set O_NONBLOCK (via fcntl F_GETFL/F_SETFL), set
FD_CLOEXEC (F_SETFD) and apply SO_NOSIGPIPE (setsockopt) with the same error
handling and close-on-failure behavior, and only set kq_impl->fd_,
desc_state_.fd and call scheduler().register_descriptor(fd, ...) after those
operations succeed; reference make_local_stream_pair and
local_stream_socket::assign() in tests/docs to ensure call sites relying on
assign() are covered or document the precondition if you choose not to normalize
here.
In `@include/boost/corosio/native/detail/reactor/reactor_op_complete.hpp`:
- Around line 150-156: The code currently calls from_sockaddr_as(peer_storage,
static_cast<socklen_t>(sizeof(peer_storage)), ep_type{}) which hardcodes the
peer buffer size and loses the actual addrlen returned by accept(); update
setup_accepted_socket(...) to accept and return (or store) the accept()-returned
socklen_t (addrlen) for the accepted peer, propagate that addrlen to where
impl.set_endpoints(...) is called, and replace the sizeof(peer_storage) argument
with the preserved addrlen so from_sockaddr_as(...) receives the real
kernel-reported length (preserve existing symbols: setup_accepted_socket,
acceptor_impl->local_endpoint(), impl.set_endpoints, peer_storage,
from_sockaddr_as, ep_type).
In `@include/boost/corosio/native/native_udp_socket.hpp`:
- Around line 121-123: The native fast-path currently hardcodes flags=0 when
calling self_.get_impl().send_to (and similarly in the recv_from case), which
drops UDP message-flag support; update the native awaitable API to carry a
message_flags parameter through native_udp_socket (and its awaitables) and add
matching send_to/recv_from overloads that accept a message_flags argument, then
pass that message_flags to self_.get_impl().send_to / recv_from instead of 0
while preserving existing parameters (h, env->executor, buffers_, dest_, token_,
&ec_, &bytes_transferred_); update call sites and constructors that build the
awaitable to accept and forward message_flags so native_udp_socket remains
feature-parity with udp_socket.
In `@src/corosio/src/local_socket_pair.cpp`:
- Around line 50-64: create_pair() can leak the two descriptors from
::socketpair() if make_nonblock_cloexec(fds[0]) or make_nonblock_cloexec(fds[1])
throws on the `#ifndef` SOCK_NONBLOCK path; modify create_pair to close fds[0] and
fds[1] if any subsequent operation throws (e.g., wrap the calls to
make_nonblock_cloexec in a try/catch, close both descriptors on exception, then
rethrow), ensuring socketpair() failure handling remains unchanged and no
descriptors are leaked.
---
Outside diff comments:
In `@include/boost/corosio/native/detail/reactor/reactor_basic_socket.hpp`:
- Around line 13-29: Add a file-level /* ... */ comment block right after the
includes that summarizes the reactor_basic_socket.hpp descriptor-state
synchronization model and the shared teardown rules: describe the socket state
machine (owned vs released vs closed), how bind-time endpoint caching is stored
and used, and clearly explain the difference between do_close_socket()
(immediate close and resource reclamation) and do_release_socket() (transfer of
ownership/descriptor without closing), including any locking expectations and
which thread/context must call each path; reference the reactor_basic_socket
class and the functions do_close_socket() and do_release_socket() so maintainers
can map the prose to the implementation.
In `@include/boost/corosio/udp_socket.hpp`:
- Around line 505-635: Add full async contract docstrings for the new public
overloads send_to(Buffers const&,...), recv_from(Buffers const&,...),
send(Buffers const&,...), and recv(Buffers const&,...): document completion
conditions (when the awaitable completes successfully vs error), the exact
returned io_result tuple elements (size and error info), cancellation behavior
(what happens if the coroutine is cancelled or the socket is closed), and any
serialization/concurrency constraints (whether operations are safe concurrently
on the same socket or must be serialized). Match wording/structure to existing
coroutine I/O API docs in this header (same sections for "return", "throws",
"completion", "cancellation", "concurrency") so the overloads have identical
contract detail as other public awaitables.
---
Minor comments:
In `@include/boost/corosio/local_endpoint.hpp`:
- Around line 26-76: Add full Doxygen-style public API docs for the class
local_endpoint and its public members: document that local_endpoint stores at
most max_path_length bytes by copying the contents of the input std::string_view
into internal storage (so the caller may free/modify the original string),
describe that path() returns a std::string_view that refers to the internal
buffer and is only valid while the local_endpoint object exists, document the
default constructor (creates an empty/unbound endpoint), document the throwing
constructor local_endpoint(std::string_view) (throws std::system_error if path
length > max_path_length), document the non-throwing constructor
local_endpoint(std::string_view, std::error_code&) (sets ec on error and is
noexcept), and document the max_path_length constant and thread-safety
guarantees; use Doxygen tags for parameters, return values, exceptions, and
remarks so these details appear in generated API docs.
In `@include/boost/corosio/native/detail/epoll/epoll_local_stream_service.hpp`:
- Around line 26-31: This header uses std::lock_guard (used around lines
referencing the epoll_local_stream_service implementation at lock sites) but
doesn't include <mutex> directly; fix it by adding `#include` <mutex> near the top
of include directives in epoll_local_stream_service.hpp so the dependency is
explicit and build-safe for symbols like std::lock_guard and std::mutex used in
the class/implementation.
In `@src/corosio/src/local_datagram_socket.cpp`:
- Around line 18-22: The POSIX branch uses errno in the available() function but
doesn't include <cerrno>; add an explicit `#include` <cerrno> inside the
BOOST_COROSIO_POSIX include block in local_datagram_socket.cpp so errno is
defined, and apply the same fix to the POSIX include blocks in
local_stream_socket.cpp and local_socket_pair.cpp; update the include near the
existing `#include` <sys/ioctl.h> so available() and any other uses of errno
reference the proper header.
In `@src/corosio/src/local_stream_socket.cpp`:
- Around line 18-22: Add an explicit `#include` <cerrno> to the POSIX branch so
errno is declared before it is read in the available() method; update the
preprocessor include block that currently has `#if` BOOST_COROSIO_POSIX / `#include`
<sys/ioctl.h> to also `#include` <cerrno> (referencing the available() method in
local_stream_socket.cpp which reads errno after ::ioctl()).
---
Nitpick comments:
In `@include/boost/corosio/detail/io_op_base.hpp`:
- Around line 13-23: Add a file-level /* ... */ maintainer overview comment
immediately after the includes and before the namespace boost::corosio::detail
in io_op_base.hpp that summarizes the coroutine and cancellation design used by
this header (how coroutines are resumed, how stop_token/cancellation is observed
and propagated, ownership/lifetime assumptions for any io operation objects, and
any important invariants). Place the block at the top of the file (right after
the last `#include` and before namespace boost::corosio::detail) and reference the
key components used in the file (e.g., the coroutine machinery, stop_token
usage, and any io operation types such as io_op_base or similarly named types)
so maintainers can quickly understand the control flow and cancellation
semantics without reading the full implementation.
In `@include/boost/corosio/native/detail/epoll/epoll_local_stream_service.hpp`:
- Around line 17-31: The header lacks a file-level overview comment after the
includes; add a C-style block comment immediately after the include section in
epoll_local_stream_service.hpp that summarizes the service's high-level design,
lifecycle and registration flow (how epoll_local_stream_service interacts with
epoll_local_stream_socket, epoll_scheduler and reactor_socket_service, and how
reactor_op_complete is used), states key invariants and any threading/ownership
expectations, and mirrors the style/content of the select variant overview so
future maintainers can quickly understand the backend-specific logic.
In `@include/boost/corosio/native/detail/reactor/reactor_basic_socket.hpp`:
- Around line 332-458: Both do_close_socket and do_release_socket duplicate the
concurrency-sensitive cancellation/claim/descriptor-state cleanup; extract that
shared sequence into a single helper (e.g., teardown_cancel_and_claim) and call
it from both functions, leaving only the close-vs-release tails in each. The
helper should: invoke for_each_op(request_cancel) on the Derived (use
static_cast<Derived*>(this)), lock desc_state_.mutex while calling
for_each_desc_entry and while reading/updating desc_state_.is_enqueued_ and
desc_state_.impl_ref_ (preserving the existing ordering), clear the desc_state
flags
(read_ready/write_ready/read_cancel_pending/write_cancel_pending/connect_cancel_pending)
and collect claimed reactor_op_base pointers, then after releasing the mutex set
each claimed[i]->impl_ptr = self and call svc_.post(...) and
svc_.work_finished(). Ensure fd_/registered_events cleanup and ::close(fd_) (for
close) or leaving fd for return (for release) remain as the small
function-specific tails in do_close_socket and do_release_socket respectively.
In
`@include/boost/corosio/native/detail/select/select_local_datagram_service.hpp`:
- Around line 17-35: Add a file-level /* */ overview comment immediately after
the includes in select_local_datagram_service.hpp that summarizes the
select-based reactor/registration model and socket lifecycle handled by this
header (similar to the comment in select_local_stream_service.hpp); mention the
roles of select_local_datagram_socket, select_scheduler, reactor_socket_service
and reactor_op_complete, how sockets are registered/unregistered with the
scheduler, where completion is posted, and any important threading/coroutine
considerations so readers can quickly understand the high-level flow implemented
in this file.
In `@src/corosio/src/io_context.cpp`:
- Around line 18-57: Add a brief file-level /* ... */ block immediately after
the include section that summarizes the overall design: list the four backend
families (epoll/select/kqueue/iocp) and key scheduler/service classes (e.g.,
epoll_scheduler, select_scheduler, kqueue_scheduler, win_scheduler and the
corresponding *_tcp_service / *_udp_service / *_local_*_service classes),
describe the pre/post-construction option flow and the service graph (which
scheduler is selected and how acceptor/stream/datagram services are wired), and
note construction/order constraints to follow when modifying io_context
creation. This comment should be concise (2–5 sentences) and placed after the
grouped includes so future readers can quickly see backend selection and
construction order.
In `@src/corosio/src/local_datagram_socket.cpp`:
- Around line 14-24: Add a file-level C-style block comment immediately after
the existing includes in local_datagram_socket.cpp that briefly summarizes the
file's purpose and high-level behavior: mention that this file implements the
platform-conditional lifecycle, shutdown, and available() behavior for the
local_datagram_socket type (including POSIX vs IOCP differences), note any
important invariants or threading/ownership expectations, and call out
interactions with detail::local_datagram_service and detail::except; keep it
short and maintainer-focused per repo guidelines.
In `@src/corosio/src/local_socket_pair.cpp`:
- Around line 10-22: Add a file-level /* */ block comment immediately after the
includes that gives a concise maintainer overview of the local socket pair
implementation: explain that boost::corosio::local_socket_pair creates a POSIX
socketpair, may set descriptors non-blocking via fcntl, transfers ownership of
fds into the RAII wrapper, and contains specific fallback and partial-failure
cleanup paths (close on error, handle partial success when one fd creation or
fcntl fails); mention the key syscalls used (socketpair, fcntl, close) and the
intended semantics on success vs. failure so future readers understand ownership
transfer and error-handling behavior.
In `@src/corosio/src/local_stream_acceptor.cpp`:
- Around line 10-16: Add a file-level C-style block comment immediately after
the `#include` section of local_stream_acceptor.cpp that summarizes the
implementation: describe that this translation unit implements the lifecycle
(construct/destruct), bind/listen operations for the public
local_stream_acceptor handle, explain that it delegates work to
detail::local_stream_acceptor_service, outline the service-backed handle flow
(handle -> service calls -> platform specifics), and note the POSIX-specific
step to unlink_existing socket paths before bind. Keep it brief (3-6 lines) and
mention any error/cleanup interaction between unlink_existing and the service.
In `@src/corosio/src/local_stream_socket.cpp`:
- Around line 14-24: Add a file-level C-style comment block immediately after
the includes that gives a short high-level overview of this translation unit:
describe that src/corosio/src/local_stream_socket.cpp implements the
platform-conditional lifecycle and readiness-query logic for
local_stream_socket, mention the role of
boost::corosio::detail::local_stream_service and the POSIX vs IOCP branches
(BOOST_COROSIO_POSIX / BOOST_COROSIO_HAS_IOCP), and note any important
invariants or threading/async expectations callers must respect; place this /*
... */ block before the namespace boost::corosio { line.
🪄 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: 3bec5a83-8ac6-4a0d-be12-aff783e39b9e
⛔ 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 (75)
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_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_local_datagram_service.hppinclude/boost/corosio/native/detail/epoll/epoll_local_datagram_socket.hppinclude/boost/corosio/native/detail/epoll/epoll_local_stream_acceptor.hppinclude/boost/corosio/native/detail/epoll/epoll_local_stream_acceptor_service.hppinclude/boost/corosio/native/detail/epoll/epoll_local_stream_service.hppinclude/boost/corosio/native/detail/epoll/epoll_local_stream_socket.hppinclude/boost/corosio/native/detail/epoll/epoll_tcp_acceptor_service.hppinclude/boost/corosio/native/detail/epoll/epoll_tcp_socket.hppinclude/boost/corosio/native/detail/epoll/epoll_udp_service.hppinclude/boost/corosio/native/detail/epoll/epoll_udp_socket.hppinclude/boost/corosio/native/detail/iocp/win_local_dgram_service.hppinclude/boost/corosio/native/detail/iocp/win_local_dgram_socket.hppinclude/boost/corosio/native/detail/iocp/win_local_stream_acceptor.hppinclude/boost/corosio/native/detail/iocp/win_local_stream_acceptor_service.hppinclude/boost/corosio/native/detail/iocp/win_local_stream_service.hppinclude/boost/corosio/native/detail/iocp/win_local_stream_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_local_datagram_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_local_datagram_socket.hppinclude/boost/corosio/native/detail/kqueue/kqueue_local_stream_acceptor.hppinclude/boost/corosio/native/detail/kqueue/kqueue_local_stream_acceptor_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_local_stream_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_local_stream_socket.hppinclude/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_tcp_socket.hppinclude/boost/corosio/native/detail/kqueue/kqueue_udp_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_udp_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_acceptor.hppinclude/boost/corosio/native/detail/reactor/reactor_acceptor_service.hppinclude/boost/corosio/native/detail/reactor/reactor_basic_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_datagram_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_op.hppinclude/boost/corosio/native/detail/reactor/reactor_op_complete.hppinclude/boost/corosio/native/detail/reactor/reactor_stream_socket.hppinclude/boost/corosio/native/detail/select/select_local_datagram_service.hppinclude/boost/corosio/native/detail/select/select_local_datagram_socket.hppinclude/boost/corosio/native/detail/select/select_local_stream_acceptor.hppinclude/boost/corosio/native/detail/select/select_local_stream_acceptor_service.hppinclude/boost/corosio/native/detail/select/select_local_stream_service.hppinclude/boost/corosio/native/detail/select/select_local_stream_socket.hppinclude/boost/corosio/native/detail/select/select_tcp_acceptor_service.hppinclude/boost/corosio/native/detail/select/select_tcp_socket.hppinclude/boost/corosio/native/detail/select/select_udp_service.hppinclude/boost/corosio/native/detail/select/select_udp_socket.hppinclude/boost/corosio/native/native_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
include/boost/corosio/native/detail/kqueue/kqueue_local_stream_acceptor_service.hpp
Show resolved
Hide resolved
include/boost/corosio/native/detail/kqueue/kqueue_local_stream_service.hpp
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #229 +/- ##
===========================================
- Coverage 77.85% 77.73% -0.12%
===========================================
Files 96 96
Lines 7350 7306 -44
Branches 1791 1787 -4
===========================================
- Hits 5722 5679 -43
+ Misses 1110 1109 -1
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 (2)
include/boost/corosio/native/detail/kqueue/kqueue_local_stream_service.hpp (1)
203-211:⚠️ Potential issue | 🟠 MajorGuard
SO_NOSIGPIPEhere too.This references
SO_NOSIGPIPEunconditionally, while the other kqueue local services in this PR already treat that option as optional. On a kqueue target where the macro is absent, this file stops compiling.Proposed fix
- // Suppress SIGPIPE on this socket; writev() has no MSG_NOSIGNAL - // equivalent, so SO_NOSIGPIPE is required on macOS/FreeBSD. - int one = 1; - if (::setsockopt(fd, SOL_SOCKET, SO_NOSIGPIPE, &one, sizeof(one)) != 0) - { - int errn = errno; - ::close(fd); - return make_err(errn); - } + // Suppress SIGPIPE on this socket; writev() has no MSG_NOSIGNAL + // equivalent, so SO_NOSIGPIPE is required where available. +#ifdef SO_NOSIGPIPE + int one = 1; + if (::setsockopt(fd, SOL_SOCKET, SO_NOSIGPIPE, &one, sizeof(one)) != 0) + { + int errn = errno; + ::close(fd); + return make_err(errn); + } +#endif🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/native/detail/kqueue/kqueue_local_stream_service.hpp` around lines 203 - 211, The code unconditionally references SO_NOSIGPIPE in the setsockopt call inside the kqueue local stream service; wrap that block with a compile-time guard (e.g. `#ifdef` SO_NOSIGPIPE / `#endif`) so platforms without SO_NOSIGPIPE still compile, and if the macro is absent simply skip the setsockopt call; keep the existing error handling around ::close(fd) and return make_err(errn) unchanged for the guarded branch and ensure no other references to SO_NOSIGPIPE remain unguarded in this file (look for the setsockopt(fd, SOL_SOCKET, SO_NOSIGPIPE, &one, sizeof(one)) invocation and the local variables fd, one, errn, make_err).include/boost/corosio/local_datagram_socket.hpp (1)
676-692:⚠️ Potential issue | 🟡 MinorAvoid
-1in the closed-handle docs here too.
is_open()already uses~native_handle_type(0)on IOCP, so documentingnative_handle()/release()as returning-1when closed is still POSIX-specific and misleading for Windows builds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/local_datagram_socket.hpp` around lines 676 - 692, The docs for native_handle() and release() incorrectly state they return "-1" when closed (POSIX-specific); update their return descriptions to reference the type's invalid sentinel used by is_open() (e.g., the value produced by ~native_handle_type(0) / "invalid native handle") or phrase as "an invalid handle value (implementation-defined, e.g. -1 on POSIX)" so it is correct for IOCP/Windows as well; update both function comments to match is_open()'s invalid-handle semantics and remove the POSIX-only "-1" wording.
🧹 Nitpick comments (5)
include/boost/corosio/detail/local_stream_service.hpp (1)
20-27: Refresh the overview to include the Windows backend.This block still says the concrete implementations are only epoll/select/kqueue and frames the contract as Unix-only, but this service now also underpins the IOCP/Winsock local-stream backend in this PR. Leaving the overview stale here makes the backend contract harder to reason about 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 ... Warn if such files lack this overview or if the overview appears stale relative to the code."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/detail/local_stream_service.hpp` around lines 20 - 27, The top comment for the abstract local_stream_service is stale: update the overview to mention that concrete implementations include Windows IOCP/Winsock in addition to epoll/select/kqueue and clarify the service contract is cross-platform (not Unix-only); specifically edit the block that references "Concrete implementations (epoll, select, kqueue)" and the sentences mentioning "Unix domain sockets" so they reference the Windows local-stream backend and indicate that make_service installs the platform-specific backend and local_stream_socket retrieves it via use_service<local_stream_service>().include/boost/corosio/native/detail/reactor/reactor_op.hpp (1)
265-272: Refresh the accept-policy contract comment.This docblock still describes
AcceptPolicy::do_accept(fd, peer_storage)even though the actual interface now requirespeer_addrlenas well. Since this header defines the reusable backend contract, leaving the old signature here makes the new requirement easy to miss.As per coding guidelines "
Files containing non-trivial implementation logic should include a /* */ block comment after the includes that provides a high-level overview ... Warn if such files lack this overview or if the overview appears stale relative to the code."🤖 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.hpp` around lines 265 - 272, Update the stale docblock in reactor_op.hpp that describes the accept-policy contract: change the description of AcceptPolicy::do_accept to match the current signature (e.g., AcceptPolicy::do_accept(int fd, sockaddr_storage& peer_storage, socklen_t& peer_addrlen)) and state that it returns the accepted fd or -1 with errno set, and document how peer_addrlen is used/updated; keep the template parameter notes for Base and AcceptPolicy and ensure the file-level /* */ overview after the includes is present or refreshed to mention this updated contract and any non-trivial behavior.include/boost/corosio/native/detail/epoll/epoll_local_datagram_service.hpp (1)
17-33: Add a file-level overview block after the includes.This header has enough reactor-specific behavior that it benefits from a short
/* */summary covering the open/adopt lifecycle, descriptor registration, and why connected datagram recv uses a distinct completion path.As per coding guidelines, "Files containing non-trivial implementation logic should include a
/* */block comment after the includes that provides a high-level overview of how the implementation works."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/native/detail/epoll/epoll_local_datagram_service.hpp` around lines 17 - 33, Add a file-level /* ... */ overview comment immediately after the includes in epoll_local_datagram_service.hpp that briefly explains the module's responsibilities: describe the open/adopt lifecycle for local datagram sockets, how descriptors are registered/unregistered with epoll via epoll_scheduler and reactor_socket_service, and explain why connected datagram receive operations follow a different completion path (referencing epoll_local_datagram_socket and reactor_op_complete) so readers understand the reactor-specific behavior and completion flow.include/boost/corosio/native/detail/kqueue/kqueue_local_datagram_service.hpp (1)
17-34: Add a file-level overview block after the includes.This file has the same kind of non-obvious reactor behavior as the select-backed version, but it lacks the overview comment that explains the socket lifecycle, descriptor registration, and the distinct datagram completion paths.
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/kqueue/kqueue_local_datagram_service.hpp` around lines 17 - 34, Add a file-level /* ... */ overview block immediately after the includes in kqueue_local_datagram_service.hpp that describes the high-level behavior: explain the socket lifecycle for kqueue_local_datagram_socket, how descriptors are registered/unregistered with kqueue_scheduler/reactor_socket_service, and outline the two distinct datagram completion paths (immediate completion vs reactor-driven completion via reactor_op_complete) mirroring the explanatory comment used in the select-backed implementation so readers understand registration, events, and completion flow.include/boost/corosio/local_stream_socket.hpp (1)
90-105: Documentconnect()to the same async-contract level as the rest of the public API.
connect()andimplementation::connect(...)still miss the usual completion conditions, notable error surface, and serialization guidance for a public awaitable operation.As per coding guidelines, “public async operations in
*.hppshould document completion conditions, concurrency/serialization, cancellation, parameters/returns, error conditions, and examples.”Also applies to: 284-299
🤖 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 90 - 105, Update the public API documentation for connect() (and the corresponding implementation::connect(...)) to the same async-contract level as other public awaits: add clear "Completion conditions" describing when the coroutine resumes (success or failure and which errors may be set), "Concurrency / serialization" stating whether concurrent connect calls on the same socket are allowed or must be serialized, "Cancellation" explaining how std::stop_token cancels the operation and what error (e.g. operation_aborted) is reported, explicit Parameter/Return descriptions for h, ex, ep, token, and ec, and an "Error conditions" list enumerating notable error_codes returned (e.g. not_found, permission_denied, connection_refused, operation_aborted). Also include a short usage example or reference to an example and mirror the same text at the implementation::connect signature to keep header/impl docs consistent.
🤖 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 567-583: The documentation for the datagram receive awaitable in
include/boost/corosio/local_datagram_socket.hpp incorrectly states that
bytes_transferred is “the datagram size”; update the comment for the
receive/awaitable that captures the sender endpoint so it clearly states that
bytes_transferred is the number of bytes copied into the provided buffer (i.e.,
truncated if the buffer is smaller) and not the original sender datagram length,
and optionally mention that excess bytes are discarded per datagram semantics.
In `@include/boost/corosio/local_stream_socket.hpp`:
- Around line 57-60: The class documentation in local_stream_socket.hpp still
documents a POSIX-only model and promises native_handle()/release() return -1
for closed sockets; update the comment around the class and the paragraphs at
lines 309-337 to mention both Unix-domain (epoll/kqueue/select) and Windows
IOCP/Winsock backends, and replace the hard-coded "-1" sentinel with a
platform-neutral "invalid native handle" description (e.g. use
native_handle_type(0) as the invalid sentinel on Windows/Winsock and -1 for
POSIX) and state that native_handle()/release() return that invalid sentinel
when closed; reference the native_handle_type, native_handle(), and release()
symbols so readers can find the exact API contract.
In `@include/boost/corosio/native/detail/select/select_local_stream_service.hpp`:
- Around line 218-223: In select_local_stream_service::open_socket() the
SO_NOSIGPIPE setsockopt call is currently ignored; change it to check the return
value and treat -1 as a failure by closing the socket and returning an error
(mirroring kqueue_local_stream_service::open_socket and kqueue_tcp_service.hpp
behavior). Specifically, after ::setsockopt(fd, SOL_SOCKET, SO_NOSIGPIPE, &one,
sizeof(one)) detect a -1 return, call the same socket-close/cleanup path used on
other configuration failures and propagate the error out of open_socket (so
callers like select_op accept handlers see the failure).
---
Duplicate comments:
In `@include/boost/corosio/local_datagram_socket.hpp`:
- Around line 676-692: The docs for native_handle() and release() incorrectly
state they return "-1" when closed (POSIX-specific); update their return
descriptions to reference the type's invalid sentinel used by is_open() (e.g.,
the value produced by ~native_handle_type(0) / "invalid native handle") or
phrase as "an invalid handle value (implementation-defined, e.g. -1 on POSIX)"
so it is correct for IOCP/Windows as well; update both function comments to
match is_open()'s invalid-handle semantics and remove the POSIX-only "-1"
wording.
In `@include/boost/corosio/native/detail/kqueue/kqueue_local_stream_service.hpp`:
- Around line 203-211: The code unconditionally references SO_NOSIGPIPE in the
setsockopt call inside the kqueue local stream service; wrap that block with a
compile-time guard (e.g. `#ifdef` SO_NOSIGPIPE / `#endif`) so platforms without
SO_NOSIGPIPE still compile, and if the macro is absent simply skip the
setsockopt call; keep the existing error handling around ::close(fd) and return
make_err(errn) unchanged for the guarded branch and ensure no other references
to SO_NOSIGPIPE remain unguarded in this file (look for the setsockopt(fd,
SOL_SOCKET, SO_NOSIGPIPE, &one, sizeof(one)) invocation and the local variables
fd, one, errn, make_err).
---
Nitpick comments:
In `@include/boost/corosio/detail/local_stream_service.hpp`:
- Around line 20-27: The top comment for the abstract local_stream_service is
stale: update the overview to mention that concrete implementations include
Windows IOCP/Winsock in addition to epoll/select/kqueue and clarify the service
contract is cross-platform (not Unix-only); specifically edit the block that
references "Concrete implementations (epoll, select, kqueue)" and the sentences
mentioning "Unix domain sockets" so they reference the Windows local-stream
backend and indicate that make_service installs the platform-specific backend
and local_stream_socket retrieves it via use_service<local_stream_service>().
In `@include/boost/corosio/local_stream_socket.hpp`:
- Around line 90-105: Update the public API documentation for connect() (and the
corresponding implementation::connect(...)) to the same async-contract level as
other public awaits: add clear "Completion conditions" describing when the
coroutine resumes (success or failure and which errors may be set), "Concurrency
/ serialization" stating whether concurrent connect calls on the same socket are
allowed or must be serialized, "Cancellation" explaining how std::stop_token
cancels the operation and what error (e.g. operation_aborted) is reported,
explicit Parameter/Return descriptions for h, ex, ep, token, and ec, and an
"Error conditions" list enumerating notable error_codes returned (e.g.
not_found, permission_denied, connection_refused, operation_aborted). Also
include a short usage example or reference to an example and mirror the same
text at the implementation::connect signature to keep header/impl docs
consistent.
In `@include/boost/corosio/native/detail/epoll/epoll_local_datagram_service.hpp`:
- Around line 17-33: Add a file-level /* ... */ overview comment immediately
after the includes in epoll_local_datagram_service.hpp that briefly explains the
module's responsibilities: describe the open/adopt lifecycle for local datagram
sockets, how descriptors are registered/unregistered with epoll via
epoll_scheduler and reactor_socket_service, and explain why connected datagram
receive operations follow a different completion path (referencing
epoll_local_datagram_socket and reactor_op_complete) so readers understand the
reactor-specific behavior and completion flow.
In
`@include/boost/corosio/native/detail/kqueue/kqueue_local_datagram_service.hpp`:
- Around line 17-34: Add a file-level /* ... */ overview block immediately after
the includes in kqueue_local_datagram_service.hpp that describes the high-level
behavior: explain the socket lifecycle for kqueue_local_datagram_socket, how
descriptors are registered/unregistered with
kqueue_scheduler/reactor_socket_service, and outline the two distinct datagram
completion paths (immediate completion vs reactor-driven completion via
reactor_op_complete) mirroring the explanatory comment used in the select-backed
implementation so readers understand registration, events, and completion flow.
In `@include/boost/corosio/native/detail/reactor/reactor_op.hpp`:
- Around line 265-272: Update the stale docblock in reactor_op.hpp that
describes the accept-policy contract: change the description of
AcceptPolicy::do_accept to match the current signature (e.g.,
AcceptPolicy::do_accept(int fd, sockaddr_storage& peer_storage, socklen_t&
peer_addrlen)) and state that it returns the accepted fd or -1 with errno set,
and document how peer_addrlen is used/updated; keep the template parameter notes
for Base and AcceptPolicy and ensure the file-level /* */ overview after the
includes is present or refreshed to mention this updated contract and any
non-trivial 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: 6fd59889-d559-4200-9c5f-670c0c42eca1
⛔ Files ignored due to path filters (2)
test/unit/local_stream_socket.cppis excluded by!**/test/**test/unit/tcp_socket.cppis excluded by!**/test/**
📒 Files selected for processing (38)
include/boost/corosio/backend.hppinclude/boost/corosio/detail/local_datagram_service.hppinclude/boost/corosio/detail/local_stream_service.hppinclude/boost/corosio/detail/op_base.hppinclude/boost/corosio/io/io_read_stream.hppinclude/boost/corosio/io/io_write_stream.hppinclude/boost/corosio/local_datagram_socket.hppinclude/boost/corosio/local_endpoint.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_local_datagram_service.hppinclude/boost/corosio/native/detail/epoll/epoll_local_stream_acceptor_service.hppinclude/boost/corosio/native/detail/epoll/epoll_local_stream_service.hppinclude/boost/corosio/native/detail/epoll/epoll_op.hppinclude/boost/corosio/native/detail/epoll/epoll_tcp_acceptor_service.hppinclude/boost/corosio/native/detail/iocp/win_local_dgram_service.hppinclude/boost/corosio/native/detail/iocp/win_local_stream_acceptor_service.hppinclude/boost/corosio/native/detail/iocp/win_local_stream_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_local_datagram_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_local_stream_acceptor_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_local_stream_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_op.hppinclude/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor_service.hppinclude/boost/corosio/native/detail/reactor/reactor_op.hppinclude/boost/corosio/native/detail/reactor/reactor_op_complete.hppinclude/boost/corosio/native/detail/select/select_local_datagram_service.hppinclude/boost/corosio/native/detail/select/select_local_stream_acceptor_service.hppinclude/boost/corosio/native/detail/select/select_local_stream_service.hppinclude/boost/corosio/native/detail/select/select_op.hppinclude/boost/corosio/native/detail/select/select_tcp_acceptor_service.hppinclude/boost/corosio/native/native_udp_socket.hppinclude/boost/corosio/tcp_socket.hppinclude/boost/corosio/udp_socket.hppsrc/corosio/src/local_datagram_socket.cppsrc/corosio/src/local_socket_pair.cppsrc/corosio/src/local_stream_socket.cpp
✅ Files skipped from review due to trivial changes (2)
- include/boost/corosio/message_flags.hpp
- include/boost/corosio/native/detail/iocp/win_local_stream_acceptor_service.hpp
🚧 Files skipped from review as they are similar to previous changes (9)
- include/boost/corosio/tcp_socket.hpp
- include/boost/corosio/io/io_write_stream.hpp
- src/corosio/src/local_socket_pair.cpp
- include/boost/corosio/io/io_read_stream.hpp
- include/boost/corosio/detail/local_datagram_service.hpp
- src/corosio/src/local_stream_socket.cpp
- include/boost/corosio/backend.hpp
- include/boost/corosio/native/detail/iocp/win_local_stream_service.hpp
- include/boost/corosio/native/detail/select/select_local_stream_acceptor_service.hpp
include/boost/corosio/native/detail/select/select_local_stream_service.hpp
Show resolved
Hide resolved
mvandeberg
force-pushed
the
pr/122-local-sockets
branch
from
3f7f2a9 to
35aa44a
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 (3)
include/boost/corosio/native/detail/reactor/reactor_op.hpp (1)
265-272:⚠️ Potential issue | 🟡 MinorUpdate the
AcceptPolicycontract in the template docs.The comment still advertises
do_accept(int, sockaddr_storage&), butperform_io()now requiresdo_accept(int, sockaddr_storage&, socklen_t&). Leaving the signature stale here will send the next backend implementation toward the wrong policy shape.🤖 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.hpp` around lines 265 - 272, The template doc for AcceptPolicy in reactor_op.hpp is stale: update the comment to reflect the new required do_accept signature used by perform_io; state that AcceptPolicy must provide a static int do_accept(int, sockaddr_storage&, socklen_t&) which returns the accepted fd or -1 with errno set (i.e., include the socklen_t& parameter and the return contract). Mention the related symbols AcceptPolicy, perform_io(), and reactor_op to make locating the docstring easy.include/boost/corosio/native/native_udp_socket.hpp (1)
205-263:⚠️ Potential issue | 🟡 MinorDocument the new flags-aware overloads to the public async-API standard.
These are public async entry points, but the added docs still omit the required coroutine contract details: completion/resumption conditions, cancellation behavior, overlap/serialization rules, buffer lifetime requirements, and notable error conditions for the
message_flagsvariants. Please flesh out bothsend_toandrecv_fromoverload sets so the native wrapper stays at parity with the documentedudp_socketsurface. As per coding guidelines: "Docstrings are required for all classes and functions in public headers in non-detail namespaces" and async-operation docs for**/*.{h,hpp,cpp,cc,cxx}must cover completion conditions, concurrency/overlap, parameter lifetime, return values, error conditions, throws, cancellation, and examples.🤖 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 205 - 263, The send_to and recv_from overloads (and their message_flags variants) lack the required coroutine-contract docs; update the docblocks for template send_to(CB,const endpoint,corosio::message_flags) / send_to(CB,endpoint) and recv_from(MB,endpoint&,corosio::message_flags) / recv_from(MB,endpoint&) to match udp_socket's public async API: specify completion/resumption conditions, cancellation behavior, overlap/serialization rules (concurrent calls allowed or serialized), buffer lifetime requirements, notable error conditions and exceptions thrown, the exact return tuple (error_code, size_t), and include a brief usage example; reference the backend awaitables native_send_to_awaitable and native_recv_from_awaitable and the message_flags parameter in the text so readers know these overloads forward to those implementations.include/boost/corosio/udp_socket.hpp (1)
517-647:⚠️ Potential issue | 🟡 MinorComplete the new flags-overload docs and fix the enum name.
These four new public async overloads only document
@param flags, but they still omit the buffer-lifetime contract, cancellation behavior, completion conditions, and returned aggregate details. Also, Line 521 refers tomessage_flags::dont_route, while the enum ismessage_flags::do_not_route.As per coding guidelines, "Docstrings are required for all classes and functions in public headers in non-detail namespaces" and async docs must cover cancellation, return values, and ownership/lifetime semantics.
🤖 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 517 - 647, Update the docs for the four public async overloads send_to(Buffers,const endpoint,corosio::message_flags), recv_from(Buffers,endpoint&,corosio::message_flags), send(Buffers,corosio::message_flags), and recv(Buffers,corosio::message_flags) to include the buffer-lifetime/ownership contract (caller must keep Buffers alive and unmodified until the returned awaitable completes), cancellation behavior (what happens if the awaiting coroutine is cancelled or the socket is closed), the precise completion conditions and error semantics (what io_result contains on success/failure), and the returned aggregate details (that the awaitable completes with io_result<std::size_t> or io_result<> as appropriate); also fix the example/param text that currently references message_flags::dont_route to the correct enum name message_flags::do_not_route.
🧹 Nitpick comments (6)
include/boost/corosio/io/io_read_stream.hpp (1)
50-64: Expandread_some's public async-op docs to match the new base-driven behavior.Since cancellation and result propagation now come from
bytes_op_base, theread_somedocs should explicitly cover completion conditions, cancellation (stop_token/cancel()andcapy::cond::canceled), and the allowed in-flight overlap/serialization contract.As per coding guidelines, "Completion conditions — Bulleted
@lilist of conditions under which the operation completes and the coroutine resumes," "Concurrency and overlap — State which operations may be simultaneously in flight," and "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 50 - 64, Update the public documentation for read_some to reflect the new base-driven behavior: state that completion conditions are governed by bytes_op_base (e.g., successful read bytes_ set, or ec_ set on error), list explicit completion bullets (successful bytes read, EOF, error, or cancellation), document cancellation semantics (supports stop_token propagation via the IoAwaitable protocol and io_read_stream::cancel(), and that the resulting error compares equal to capy::cond::canceled), and clarify concurrency/overlap rules (which read_some/other ops on io_read_stream may be in flight or must be serialized). Reference read_some, read_some_awaitable, bytes_op_base, io_read_stream, cancel(), stop_token/IoAwaitable protocol, and capy::cond::canceled in the documentation so readers can map behavior to the implementation.include/boost/corosio/io/io_write_stream.hpp (1)
50-64: Documentwrite_some's cancellation and overlap contract explicitly.Now that stop-token/error handling lives in
bytes_op_base, the publicwrite_somedocs should call out the required completion conditions, cancellation behavior (stop_token/cancel()andcapy::cond::canceled), and the overlap/serialization rules for callers.As per coding guidelines, "Completion conditions — Bulleted
@lilist of conditions under which the operation completes and the coroutine resumes," "Concurrency and overlap — State which operations may be simultaneously in flight," and "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_write_stream.hpp` around lines 50 - 64, Update the public documentation for write_some to explicitly state its completion conditions, cancellation behavior, and concurrency/overlap rules: document that write_some (and its awaitable type write_some_awaitable) completes when the underlying do_write_some finishes with either success, error, or cancellation, that cancellation is supported via the stop_token propagated through bytes_op_base or by calling io_write_stream::cancel(), and that a cancellation results in an error comparing equal to capy::cond::canceled; also state which operations may be in flight concurrently (serialization/overlap rules for io_write_stream operations) and reference the related symbols write_some, write_some_awaitable, bytes_op_base, io_write_stream::do_write_some, stop_token, cancel(), and capy::cond::canceled in the docs.include/boost/corosio/native/detail/reactor/reactor_basic_socket.hpp (1)
394-459: Consider extracting shared cancellation logic.
do_release_socket()anddo_close_socket()share ~45 lines of identical cancellation/deregistration code (lines 399-441 vs 337-378). A private helper likecancel_and_deregister_internal()could reduce duplication and ensure both paths stay in sync.🤖 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 394 - 459, The cancellation and deregistration block duplicated between reactor_basic_socket::do_release_socket and reactor_basic_socket::do_close_socket should be extracted into a private helper (e.g., cancel_and_deregister_internal) to centralize logic; implement a new private method that takes no parameters (or takes a flag if behavior differs) which performs the weak_from_this lock, for_each_op cancel, the claimed_entry array handling, the locked modification of desc_state_ (mutex, clearing desc slots, resetting read_/write_/connect_cancel_pending and possibly impl_ref_), posting claimed ops via svc_.post and svc_.work_finished, and deregistration of descriptor via svc_.scheduler().deregister_descriptor; then replace the duplicated blocks in do_release_socket and do_close_socket with calls to this helper and adjust any slight semantic differences (ownership of fd_ or "Do NOT close" behavior) outside the helper so both functions remain correct.include/boost/corosio/native/detail/kqueue/kqueue_local_stream_acceptor_service.hpp (1)
321-325: Note:SO_NOSIGPIPEreturn value unchecked inopen_acceptor_socket().Unlike the accept path (lines 150-164) where the return value is checked, here it's ignored. This creates an inconsistency within the same file. The kqueue backend generally checks
SO_NOSIGPIPEresults per learnings, so this may warrant alignment.♻️ Proposed fix for consistency
`#ifdef` SO_NOSIGPIPE int nosig = 1; - ::setsockopt(fd, SOL_SOCKET, SO_NOSIGPIPE, &nosig, sizeof(nosig)); + if (::setsockopt(fd, SOL_SOCKET, SO_NOSIGPIPE, &nosig, sizeof(nosig)) == -1) + { + int errn = errno; + ::close(fd); + return make_err(errn); + } `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/native/detail/kqueue/kqueue_local_stream_acceptor_service.hpp` around lines 321 - 325, In open_acceptor_socket(), the SO_NOSIGPIPE setsockopt call is currently unchecked; capture its return value (e.g., int rv = ::setsockopt(...)) and handle failures the same way as the accept path does: on rv == -1 inspect errno, close the socket fd, and return/propagate an error status (or false) so the caller knows the option failed. Ensure you reference SO_NOSIGPIPE and ::setsockopt in the fix and follow the same cleanup/logging behavior used by the accept path to keep behavior consistent.include/boost/corosio/native/detail/reactor/reactor_datagram_socket.hpp (1)
26-35: Consider adding a comment referencing the portablemessage_flagsenum values, or refactor to use named constants.The
to_native_msg_flags()function maps hardcoded bit patterns1,2,4toMSG_PEEK,MSG_OOB,MSG_DONTROUTE. The values are correct and match themessage_flagsenum definition (peek = 1,out_of_band = 2,do_not_route = 4), but adding an explicit comment or reference to the enum would improve code clarity and maintainability for future reviewers.🤖 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 26 - 35, The bitmask literals in to_native_msg_flags(int flags) are unclear; update the function to reference the portable message_flags enum (e.g., peek = 1, out_of_band = 2, do_not_route = 4) or replace the literals with those enum constants and add a short inline comment stating the mapping to native MSG_PEEK/MSG_OOB/MSG_DONTROUTE; locate to_native_msg_flags and either use the message_flags symbols instead of 1/2/4 or add a one-line comment above the ifs that documents "message_flags: peek=1, out_of_band=2, do_not_route=4" and their mapping to the native constants.include/boost/corosio/local_datagram_socket.hpp (1)
677-693: Update documentation to reflect platform-specific invalid handle sentinels.The
@returndocumentation states "-1 if not open" but this is misleading on Windows/IOCP wherenative_handle_typeisstd::uintptr_t(unsigned) and the invalid sentinel is~native_handle_type(0)(i.e.,INVALID_SOCKET). This is consistent with the check inis_open()at line 487.Consider documenting as "an invalid handle sentinel if not open" or adding a note about platform differences.
📝 Suggested documentation fix
/** Get the native socket handle. - `@return` The native socket handle, or -1 if not open. + `@return` The native socket handle, or an invalid sentinel + (~native_handle_type(0) on Windows, -1 on POSIX) if not open. */ native_handle_type native_handle() const noexcept; /** Release ownership of the native socket handle. Deregisters the socket from the reactor and cancels pending operations without closing the fd. The caller takes ownership of the returned descriptor. - `@return` The native handle, or -1 if not open. + `@return` The native handle, or an invalid sentinel if not open. `@throws` std::logic_error if the socket is not open. */ native_handle_type release();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/local_datagram_socket.hpp` around lines 677 - 693, Update the return documentation for native_handle() and release() to reference the platform-specific invalid-handle sentinel instead of hardcoding "-1 if not open"; specifically mention that on POSIX the sentinel is -1 but on Windows/IOCP native_handle_type is unsigned and the invalid sentinel is ~native_handle_type(0) (e.g. INVALID_SOCKET), and note that this matches the is_open() check used elsewhere (see is_open()); keep both functions' `@return` descriptions consistent and indicate that callers should compare against the platform-specific invalid sentinel.
🤖 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_datagram_service.hpp`:
- Around line 73-80: Update the precondition on bind_socket to acknowledge that
impl may be created via either open_socket() or assign_socket() (e.g., by
local_datagram_socket::assign()), or more simply state that impl "represents a
valid local datagram socket"; change the sentence that currently reads "impl was
opened via open_socket()" to "impl was opened or assigned (e.g., via
open_socket() or assign_socket())," or the alternative phrasing so both opened
and adopted/assigned implementations are treated as valid inputs to bind_socket.
In `@include/boost/corosio/native/detail/iocp/win_local_dgram_service.hpp`:
- Around line 507-530: After installing the stop callback and starting the
operation (the block that sets conn_.internal_ptr, initializes op via
op.reset(), sets op.h/op.ex/op.ec_out/op.target_endpoint and calls
op.start(token) and svc_.work_started()), check op.cancelled; if it's true, do
not call ::WSAConnect and instead call svc_.on_completion(&op,
WSA_OPERATION_ABORTED, 0) (or the appropriate platform error constant) and
return std::noop_coroutine(); otherwise proceed to call ::WSAConnect as before.
This ensures the socket is not synchronously connected when the operation was
already cancelled.
In
`@include/boost/corosio/native/detail/iocp/win_local_stream_acceptor_service.hpp`:
- Around line 268-276: The accept path can arm the overlapped op with
op.start(token) before listen_socket is assigned, so if the token is already
canceled the operation may never be resumed; modify the sequence around
acc_.acceptor_ptr / op.reset() / op.start(token) in the accept routine (symbols:
acc_, op.start(token), listen_socket, on_pending(), CancelIoEx) to check token
cancellation after arming and, if canceled, perform the same cancellation
handoff used elsewhere (invoke on_pending() and call CancelIoEx or post a
cancellation) so the pending accept is resumed immediately; ensure this logic is
also applied at the other occurrence referenced (lines ~350-351) and preserve
assignment of acc_.acceptor_ptr and listen_socket before or immediately after
arming as necessary to avoid race conditions.
In `@include/boost/corosio/native/detail/iocp/win_local_stream_service.hpp`:
- Around line 347-400: connect() currently starts the ConnectEx operation via
conn_ (op), calls op.start(token) and svc_.on_pending() but fails to perform the
same CancelIoEx handoff used for read/write when op.cancelled is already true;
update the connect() flow (around conn_/op, op.start(token), the connect_ex()
invocation and right after svc_.on_pending()) to mirror the read/write
cancellation logic: after arming the operation and calling svc_.on_pending(),
check op.cancelled and if set call ::CancelIoEx(socket_, &op) so the kernel is
asked to cancel the pending ConnectEx, letting the existing on_completion path
resume with the cancellation result. Ensure you reference conn_/op,
op.start(token), svc_.on_pending(), connect_ex(), and the socket_ handle when
making this change.
---
Outside diff comments:
In `@include/boost/corosio/native/detail/reactor/reactor_op.hpp`:
- Around line 265-272: The template doc for AcceptPolicy in reactor_op.hpp is
stale: update the comment to reflect the new required do_accept signature used
by perform_io; state that AcceptPolicy must provide a static int do_accept(int,
sockaddr_storage&, socklen_t&) which returns the accepted fd or -1 with errno
set (i.e., include the socklen_t& parameter and the return contract). Mention
the related symbols AcceptPolicy, perform_io(), and reactor_op to make locating
the docstring easy.
In `@include/boost/corosio/native/native_udp_socket.hpp`:
- Around line 205-263: The send_to and recv_from overloads (and their
message_flags variants) lack the required coroutine-contract docs; update the
docblocks for template send_to(CB,const endpoint,corosio::message_flags) /
send_to(CB,endpoint) and recv_from(MB,endpoint&,corosio::message_flags) /
recv_from(MB,endpoint&) to match udp_socket's public async API: specify
completion/resumption conditions, cancellation behavior, overlap/serialization
rules (concurrent calls allowed or serialized), buffer lifetime requirements,
notable error conditions and exceptions thrown, the exact return tuple
(error_code, size_t), and include a brief usage example; reference the backend
awaitables native_send_to_awaitable and native_recv_from_awaitable and the
message_flags parameter in the text so readers know these overloads forward to
those implementations.
In `@include/boost/corosio/udp_socket.hpp`:
- Around line 517-647: Update the docs for the four public async overloads
send_to(Buffers,const endpoint,corosio::message_flags),
recv_from(Buffers,endpoint&,corosio::message_flags),
send(Buffers,corosio::message_flags), and recv(Buffers,corosio::message_flags)
to include the buffer-lifetime/ownership contract (caller must keep Buffers
alive and unmodified until the returned awaitable completes), cancellation
behavior (what happens if the awaiting coroutine is cancelled or the socket is
closed), the precise completion conditions and error semantics (what io_result
contains on success/failure), and the returned aggregate details (that the
awaitable completes with io_result<std::size_t> or io_result<> as appropriate);
also fix the example/param text that currently references
message_flags::dont_route to the correct enum name message_flags::do_not_route.
---
Nitpick comments:
In `@include/boost/corosio/io/io_read_stream.hpp`:
- Around line 50-64: Update the public documentation for read_some to reflect
the new base-driven behavior: state that completion conditions are governed by
bytes_op_base (e.g., successful read bytes_ set, or ec_ set on error), list
explicit completion bullets (successful bytes read, EOF, error, or
cancellation), document cancellation semantics (supports stop_token propagation
via the IoAwaitable protocol and io_read_stream::cancel(), and that the
resulting error compares equal to capy::cond::canceled), and clarify
concurrency/overlap rules (which read_some/other ops on io_read_stream may be in
flight or must be serialized). Reference read_some, read_some_awaitable,
bytes_op_base, io_read_stream, cancel(), stop_token/IoAwaitable protocol, and
capy::cond::canceled in the documentation so readers can map behavior to the
implementation.
In `@include/boost/corosio/io/io_write_stream.hpp`:
- Around line 50-64: Update the public documentation for write_some to
explicitly state its completion conditions, cancellation behavior, and
concurrency/overlap rules: document that write_some (and its awaitable type
write_some_awaitable) completes when the underlying do_write_some finishes with
either success, error, or cancellation, that cancellation is supported via the
stop_token propagated through bytes_op_base or by calling
io_write_stream::cancel(), and that a cancellation results in an error comparing
equal to capy::cond::canceled; also state which operations may be in flight
concurrently (serialization/overlap rules for io_write_stream operations) and
reference the related symbols write_some, write_some_awaitable, bytes_op_base,
io_write_stream::do_write_some, stop_token, cancel(), and capy::cond::canceled
in the docs.
In `@include/boost/corosio/local_datagram_socket.hpp`:
- Around line 677-693: Update the return documentation for native_handle() and
release() to reference the platform-specific invalid-handle sentinel instead of
hardcoding "-1 if not open"; specifically mention that on POSIX the sentinel is
-1 but on Windows/IOCP native_handle_type is unsigned and the invalid sentinel
is ~native_handle_type(0) (e.g. INVALID_SOCKET), and note that this matches the
is_open() check used elsewhere (see is_open()); keep both functions' `@return`
descriptions consistent and indicate that callers should compare against the
platform-specific invalid sentinel.
In
`@include/boost/corosio/native/detail/kqueue/kqueue_local_stream_acceptor_service.hpp`:
- Around line 321-325: In open_acceptor_socket(), the SO_NOSIGPIPE setsockopt
call is currently unchecked; capture its return value (e.g., int rv =
::setsockopt(...)) and handle failures the same way as the accept path does: on
rv == -1 inspect errno, close the socket fd, and return/propagate an error
status (or false) so the caller knows the option failed. Ensure you reference
SO_NOSIGPIPE and ::setsockopt in the fix and follow the same cleanup/logging
behavior used by the accept path to keep behavior consistent.
In `@include/boost/corosio/native/detail/reactor/reactor_basic_socket.hpp`:
- Around line 394-459: The cancellation and deregistration block duplicated
between reactor_basic_socket::do_release_socket and
reactor_basic_socket::do_close_socket should be extracted into a private helper
(e.g., cancel_and_deregister_internal) to centralize logic; implement a new
private method that takes no parameters (or takes a flag if behavior differs)
which performs the weak_from_this lock, for_each_op cancel, the claimed_entry
array handling, the locked modification of desc_state_ (mutex, clearing desc
slots, resetting read_/write_/connect_cancel_pending and possibly impl_ref_),
posting claimed ops via svc_.post and svc_.work_finished, and deregistration of
descriptor via svc_.scheduler().deregister_descriptor; then replace the
duplicated blocks in do_release_socket and do_close_socket with calls to this
helper and adjust any slight semantic differences (ownership of fd_ or "Do NOT
close" behavior) outside the helper so both functions remain correct.
In `@include/boost/corosio/native/detail/reactor/reactor_datagram_socket.hpp`:
- Around line 26-35: The bitmask literals in to_native_msg_flags(int flags) are
unclear; update the function to reference the portable message_flags enum (e.g.,
peek = 1, out_of_band = 2, do_not_route = 4) or replace the literals with those
enum constants and add a short inline comment stating the mapping to native
MSG_PEEK/MSG_OOB/MSG_DONTROUTE; locate to_native_msg_flags and either use the
message_flags symbols instead of 1/2/4 or add a one-line comment above the ifs
that documents "message_flags: peek=1, out_of_band=2, do_not_route=4" and their
mapping to the native constants.
🪄 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: f7940b74-edc5-4723-a46c-4a1a891fee49
⛔ 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 (78)
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_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_local_datagram_service.hppinclude/boost/corosio/native/detail/epoll/epoll_local_datagram_socket.hppinclude/boost/corosio/native/detail/epoll/epoll_local_stream_acceptor.hppinclude/boost/corosio/native/detail/epoll/epoll_local_stream_acceptor_service.hppinclude/boost/corosio/native/detail/epoll/epoll_local_stream_service.hppinclude/boost/corosio/native/detail/epoll/epoll_local_stream_socket.hppinclude/boost/corosio/native/detail/epoll/epoll_op.hppinclude/boost/corosio/native/detail/epoll/epoll_tcp_acceptor_service.hppinclude/boost/corosio/native/detail/epoll/epoll_tcp_socket.hppinclude/boost/corosio/native/detail/epoll/epoll_udp_service.hppinclude/boost/corosio/native/detail/epoll/epoll_udp_socket.hppinclude/boost/corosio/native/detail/iocp/win_local_dgram_service.hppinclude/boost/corosio/native/detail/iocp/win_local_dgram_socket.hppinclude/boost/corosio/native/detail/iocp/win_local_stream_acceptor.hppinclude/boost/corosio/native/detail/iocp/win_local_stream_acceptor_service.hppinclude/boost/corosio/native/detail/iocp/win_local_stream_service.hppinclude/boost/corosio/native/detail/iocp/win_local_stream_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_local_datagram_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_local_datagram_socket.hppinclude/boost/corosio/native/detail/kqueue/kqueue_local_stream_acceptor.hppinclude/boost/corosio/native/detail/kqueue/kqueue_local_stream_acceptor_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_local_stream_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_local_stream_socket.hppinclude/boost/corosio/native/detail/kqueue/kqueue_op.hppinclude/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_tcp_socket.hppinclude/boost/corosio/native/detail/kqueue/kqueue_udp_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_udp_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_acceptor.hppinclude/boost/corosio/native/detail/reactor/reactor_acceptor_service.hppinclude/boost/corosio/native/detail/reactor/reactor_basic_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_datagram_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_op.hppinclude/boost/corosio/native/detail/reactor/reactor_op_complete.hppinclude/boost/corosio/native/detail/reactor/reactor_stream_socket.hppinclude/boost/corosio/native/detail/select/select_local_datagram_service.hppinclude/boost/corosio/native/detail/select/select_local_datagram_socket.hppinclude/boost/corosio/native/detail/select/select_local_stream_acceptor.hppinclude/boost/corosio/native/detail/select/select_local_stream_acceptor_service.hppinclude/boost/corosio/native/detail/select/select_local_stream_service.hppinclude/boost/corosio/native/detail/select/select_local_stream_socket.hppinclude/boost/corosio/native/detail/select/select_op.hppinclude/boost/corosio/native/detail/select/select_tcp_acceptor_service.hppinclude/boost/corosio/native/detail/select/select_tcp_socket.hppinclude/boost/corosio/native/detail/select/select_udp_service.hppinclude/boost/corosio/native/detail/select/select_udp_socket.hppinclude/boost/corosio/native/native_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 skipped from review due to trivial changes (12)
- include/boost/corosio.hpp
- include/boost/corosio/shutdown_type.hpp
- include/boost/corosio/detail/local_stream_service.hpp
- include/boost/corosio/local_socket_pair.hpp
- include/boost/corosio/local_stream.hpp
- include/boost/corosio/detail/local_stream_acceptor_service.hpp
- src/corosio/src/local_stream.cpp
- include/boost/corosio/native/detail/kqueue/kqueue_local_stream_acceptor.hpp
- include/boost/corosio/native/detail/iocp/win_udp_service.hpp
- include/boost/corosio/message_flags.hpp
- include/boost/corosio/local_datagram.hpp
- include/boost/corosio/native/detail/iocp/win_local_stream_socket.hpp
🚧 Files skipped from review as they are similar to previous changes (24)
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_socket.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_socket.hpp
- include/boost/corosio/native/detail/epoll/epoll_op.hpp
- include/boost/corosio/native/detail/select/select_tcp_socket.hpp
- perf/common/native_includes.hpp
- src/corosio/src/local_datagram.cpp
- include/boost/corosio/native/detail/select/select_udp_service.hpp
- include/boost/corosio/detail/op_base.hpp
- include/boost/corosio/native/detail/epoll/epoll_udp_socket.hpp
- src/corosio/src/local_socket_pair.cpp
- include/boost/corosio/native/detail/select/select_udp_socket.hpp
- include/boost/corosio/local_endpoint.hpp
- include/boost/corosio/native/detail/endpoint_convert.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_udp_socket.hpp
- include/boost/corosio/tcp_socket.hpp
- include/boost/corosio/native/detail/reactor/reactor_stream_socket.hpp
- include/boost/corosio/backend.hpp
- include/boost/corosio/native/detail/epoll/epoll_local_datagram_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor_service.hpp
- include/boost/corosio/native/detail/reactor/reactor_op_complete.hpp
- include/boost/corosio/native/detail/select/select_local_datagram_socket.hpp
- include/boost/corosio/native/detail/epoll/epoll_local_datagram_socket.hpp
- include/boost/corosio/native/detail/iocp/win_udp_socket.hpp
- include/boost/corosio/native/detail/iocp/win_local_stream_acceptor.hpp
include/boost/corosio/native/detail/iocp/win_local_stream_acceptor_service.hpp
Show resolved
Hide resolved
mvandeberg
force-pushed
the
pr/122-local-sockets
branch
from
35aa44a to
f7ea297
Compare
Introduce local_stream_socket, local_stream_acceptor, local_datagram_socket, local_endpoint, and local_socket_pair for AF_UNIX communication across all backends: epoll, kqueue, select, and Windows IOCP. local_stream_socket satisfies the Stream concept and supports connect, read/write, shutdown, cancel, release, and assign. local_datagram_socket supports both connectionless (send_to/recv_from) and connected (send/recv) modes with message_flags (peek, out_of_band, do_not_route). local_stream_acceptor provides reference-based and move-returning accept overloads, plus bind_option::unlink_existing for automatic socket path cleanup. local_endpoint stores paths in a fixed char[108] buffer with operator<=>, operator<<, and abstract socket path support. shutdown_type and message_flags are shared enums used across all socket types. IOCP backend (Windows): - win_local_stream_socket/service/acceptor using ConnectEx and AcceptEx (reuses function pointers from win_tcp_service) - win_local_dgram_socket/service using WSASendTo/WSARecvFrom - Platform-agnostic sockaddr_un alias (un_sa_t) in endpoint_convert.hpp, avoiding <afunix.h> dependency - Abstract sockets rejected with operation_not_supported - assign_socket returns operation_not_supported (no socketpair) Reactor backends: - Per-backend concrete files for each protocol, with op types defined inline in socket headers (matching UDP convention) - CRTP acceptor service base eliminates per-backend boilerplate - Accept path threads kernel-returned socklen_t through to from_sockaddr_as for correct AF_UNIX path extraction Changes to existing code: - udp_socket: add message_flags parameter to implementation virtuals (send_to, recv_from, send, recv) and forward through native_udp_socket awaitables - reactor_op: accept operations preserve peer addrlen from the kernel for correct endpoint extraction - reactor_op_complete: add complete_dgram_recv_op that does not map zero-length datagrams to EOF - endpoint_convert.hpp: platform-agnostic AF_UNIX conversions using un_sa_t (sockaddr_un on POSIX, private struct on Windows matching Boost.Asio's approach) - tcp_acceptor_service (all backends): propagate peer_addrlen in deferred accept path - SO_NOSIGPIPE applied in select local socket services Includes Antora documentation, benchmarks with Asio comparison, and comprehensive tests covering stream I/O, datagram boundaries, abstract sockets, MSG_PEEK, move semantics, and error paths.
mvandeberg
force-pushed
the
pr/122-local-sockets
branch
from
f7ea297 to
16c363f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
include/boost/corosio/local_datagram_socket.hpp (1)
677-693:⚠️ Potential issue | 🟡 MinorUse
native_handle_typeterminology consistently in the public contract.These docs still promise
-1and talk about a “file descriptor”, but this API now exposesnative_handle_type; on IOCP the invalid sentinel is not documented that way, andrelease()also contradicts itself by both returning “-1 if not open” and throwing when not open. Please align these comments to “native handle” semantics throughout.As per coding guidelines, public headers should accurately document parameters and return values.
Also applies to: 769-778
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/local_datagram_socket.hpp` around lines 677 - 693, Update the public comments for native_handle() and release() to consistently use "native handle" and native_handle_type semantics instead of referring to "-1" or "file descriptor"; explicitly state the sentinel for an invalid handle as the default-constructed native_handle_type (or "invalid native handle") rather than -1 and avoid platform-specific fd wording (e.g., IOCP). For release(), remove the contradictory "returns -1 if not open" — document the single behavior: either it throws std::logic_error when the socket is not open and otherwise returns the native_handle_type (transferring ownership), or (if you prefer non-throwing semantics) document that it returns an invalid native_handle_type when not open; pick one consistent choice and apply it to both the release() comment and the native_handle() comment (and mirror the same wording in the other occurrence noted around lines 769-778). Ensure all references use native_handle_type and say that pending operations are deregistered/cancelled, without mentioning file descriptors or -1.include/boost/corosio/native/detail/kqueue/kqueue_local_stream_service.hpp (1)
203-211:⚠️ Potential issue | 🟠 MajorGuard
SO_NOSIGPIPElike the other kqueue services.Line 206 uses
SO_NOSIGPIPEunconditionally. The sibling kqueue services already wrap this option in#ifdef SO_NOSIGPIPE, so this version is less portable and can fail to compile on platforms where the macro is absent.🛠️ Proposed fix
// Suppress SIGPIPE on this socket; writev() has no MSG_NOSIGNAL // equivalent, so SO_NOSIGPIPE is required on macOS/FreeBSD. +#ifdef SO_NOSIGPIPE int one = 1; if (::setsockopt(fd, SOL_SOCKET, SO_NOSIGPIPE, &one, sizeof(one)) != 0) { int errn = errno; ::close(fd); return make_err(errn); } +#endifRun this to confirm the inconsistency against the sibling kqueue services:
#!/bin/bash set -euo pipefail sed -n '203,211p' include/boost/corosio/native/detail/kqueue/kqueue_local_stream_service.hpp echo sed -n '276,286p' include/boost/corosio/native/detail/kqueue/kqueue_local_datagram_service.hpp echo sed -n '144,165p' include/boost/corosio/native/detail/kqueue/kqueue_local_stream_acceptor_service.hpp echo rg -n -C2 'SO_NOSIGPIPE' include/boost/corosio/native/detail/kqueue --glob '*.hpp'🤖 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_local_stream_service.hpp` around lines 203 - 211, Guard the unconditional use of SO_NOSIGPIPE with an `#ifdef` SO_NOSIGPIPE like the other kqueue services: wrap the setsockopt call and its error handling in a preprocessor check so platforms without SO_NOSIGPIPE won't try to compile that branch; keep the existing behavior (call ::setsockopt(fd, SOL_SOCKET, SO_NOSIGPIPE, &one, sizeof(one)), capture errno into errn, ::close(fd), return make_err(errn)) inside the `#ifdef` and leave no-op or alternate handling when the macro is absent so compilation matches sibling services (see the block around setsockopt in kqueue_local_stream_service.hpp and compare to kqueue_local_datagram_service.hpp / kqueue_local_stream_acceptor_service.hpp).
🧹 Nitpick comments (7)
include/boost/corosio/native/detail/reactor/reactor_acceptor_service.hpp (1)
115-123: Documentstream_svc_initialization expectation.The
stream_svc_pointer is initialized tonullptrhere, but derived classes must set it (typically in their constructor after callinguse_service<StreamService>()). Consider adding a brief comment clarifying this contract for maintainers.📝 Suggested documentation
protected: capy::execution_context& ctx_; std::unique_ptr<state_type> state_; + /// Derived classes must initialize via use_service<StreamService>(). StreamService* stream_svc_ = nullptr;🤖 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_service.hpp` around lines 115 - 123, Add a short comment documenting the contract that stream_svc_ is initialized to nullptr and must be assigned by derived classes (typically after calling use_service<StreamService>() in their constructor) before any use; reference stream_svc_, the accessor stream_service(), and the expected initialization point (use_service<StreamService>()) so maintainers know derived classes must set stream_svc_ (state_type and ctx_ can remain unchanged).include/boost/corosio/native/detail/iocp/win_local_dgram_service.hpp (1)
36-45: Avoid hard-codingmessage_flagsbit positions.This helper is coupled to the current
message_flagsunderlying values via1/2/4, so a future enum reshuffle would silently remap flags here. Masking against the named enum constants would keep the backend tied to the public API contract instead of its current layout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/native/detail/iocp/win_local_dgram_service.hpp` around lines 36 - 45, The function local_dgram_to_native_msg_flags hard-codes bit positions (1/2/4); replace those magic literals by masking against the public message_flags enum constants (e.g., message_flags::peek, message_flags::oob, message_flags::dont_route) inside local_dgram_to_native_msg_flags and map each named flag to the corresponding MSG_* constant (MSG_PEEK, MSG_OOB, MSG_DONTROUTE); also ensure the header that defines message_flags is included or visible so the function compiles against the enum symbols rather than numeric literals.src/corosio/src/local_datagram_socket.cpp (1)
14-22: Add a high-level file overview after the includes.This file now combines lifecycle rules, service delegation, and POSIX/IOCP-specific
available()behavior. A short/* */maintainer note would make the wrapper contract easier to audit.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 - 22, Add a short /* */ file-level comment immediately after the includes in local_datagram_socket.cpp that explains the high-level design: describe the wrapper's lifecycle rules (ownership/close semantics), how operations delegate to detail::local_datagram_service, and note platform-specific behavior for available() under BOOST_COROSIO_POSIX vs BOOST_COROSIO_HAS_IOCP (ioctl vs IOCP query). Mention any invariants callers should expect (threading/async context) and reference functions/classes used in the file such as available(), detail::local_datagram_service, and the socket wrapper itself so maintainers can quickly audit the wrapper contract.include/boost/corosio/native/detail/reactor/reactor_acceptor.hpp (1)
237-275: Extract the shared close/release teardown path.
do_close_socket()anddo_release_socket()now duplicate the same op-cancellation, descriptor reset, and endpoint reset logic. Pulling that into one helper will make future lifecycle fixes land in both paths consistently.Also applies to: 277-327
🤖 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 237 - 275, The do_close_socket() and do_release_socket() functions duplicate the same op-cancellation, descriptor reset and endpoint reset logic; create a single helper method (e.g., perform_socket_teardown() or release_descriptor_resources()) that encapsulates the block that locks desc_state_.mutex, calls acc_.request_cancel(), clears desc_state_.read_op/read_ready/write_ready, conditionally sets desc_state_.impl_ref_, posts acc_ via svc_.post(&acc_) if claimed, calls svc_.work_finished(), deregisters and closes fd_, resets fd_, desc_state_.fd/registered_events and clears local_endpoint_; then replace the duplicated blocks in do_close_socket() and do_release_socket() to call this new helper and leave any function-specific behavior outside it.include/boost/corosio/native/detail/reactor/reactor_stream_socket.hpp (1)
92-119: Avoid raw0/1/2shutdown mapping.This helper now depends on every caller’s shutdown enum keeping the same underlying values and ordering. A typed overload or small traits mapping would keep the contract explicit and avoid silent misrouting if either enum changes later.
🤖 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 92 - 119, The helper do_shutdown(int what) relies on magic integers 0/1/2 and risks mismatches with caller enums; replace this with a typed API and explicit mapping: introduce or accept a specific enum (e.g., an enum class ShutdownMode { Receive, Send, Both }) and add an overload do_shutdown(ShutdownMode mode) that switches on those enum cases and maps to SHUT_RD/SHUT_WR/SHUT_RDWR, update callers to pass the enum instead of raw ints, or alternatively implement a small traits/function (e.g., shutdown_mode_to_how(YourCallerEnum)) that performs the explicit mapping and is referenced from do_shutdown; ensure the original do_shutdown(int) is removed or marked deprecated so callers can't pass raw integers.include/boost/corosio/native/detail/kqueue/kqueue_local_stream_acceptor_service.hpp (1)
321-324: Consider checking SO_NOSIGPIPE return value for consistency.The accept() path (lines 154-163) strictly checks the SO_NOSIGPIPE setsockopt return value and closes the socket on failure, but open_acceptor_socket() ignores it. While best-effort is acceptable here, the inconsistency within the same kqueue backend file is worth noting for future cleanup. Based on learnings, the kqueue backend uses stricter error handling than select.
🔧 Optional: Match the accept() error handling pattern
// SO_NOSIGPIPE on macOS (where MSG_NOSIGNAL doesn't exist) `#ifdef` SO_NOSIGPIPE int nosig = 1; - ::setsockopt(fd, SOL_SOCKET, SO_NOSIGPIPE, &nosig, sizeof(nosig)); + if (::setsockopt(fd, SOL_SOCKET, SO_NOSIGPIPE, &nosig, sizeof(nosig)) == -1) + { + int errn = errno; + ::close(fd); + return make_err(errn); + } `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/native/detail/kqueue/kqueue_local_stream_acceptor_service.hpp` around lines 321 - 324, open_acceptor_socket() sets SO_NOSIGPIPE without checking the return value, which is inconsistent with the stricter handling in the accept() path; update open_acceptor_socket() to check the ::setsockopt return value for SO_NOSIGPIPE and on failure behave like the accept() path (close the socket fd and return/propagate an error or false), and include a matching error/log message so behavior is consistent with the existing accept() code path that inspects setsockopt result.include/boost/corosio/native/detail/select/select_local_datagram_socket.hpp (1)
31-35: Acceptor template parameter mismatch.
select_local_datagram_opusesselect_tcp_acceptoras the acceptor template argument, but this is for local (AF_UNIX) datagram sockets. While datagram sockets don't use accept operations (soacceptor_impl_is never populated), usingselect_tcp_acceptoris conceptually incorrect and could cause confusion during maintenance.Consider using a local-specific acceptor type or a placeholder type that makes the "not applicable" semantics explicit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/native/detail/select/select_local_datagram_socket.hpp` around lines 31 - 35, select_local_datagram_op incorrectly instantiates reactor_op with select_tcp_acceptor (TCP acceptor) even though local datagram sockets don't accept, so replace the acceptor template argument with a local-specific or explicit "not applicable" type: either create/use a select_local_acceptor (or a select_null_acceptor/placeholder like std::monostate) and change the inheritance to reactor_op<select_local_datagram_socket, select_local_acceptor> (or reactor_op<select_local_datagram_socket, select_null_acceptor>), update any forward declarations/includes so the new acceptor type is visible, and ensure any code that referenced acceptor_impl_ handles the placeholder appropriately (no-op or absent).
🤖 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/select/select_local_stream_acceptor_service.hpp`:
- Around line 118-126: The loop in select_local_stream_acceptor_service::...
calls ::accept(fd_, reinterpret_cast<sockaddr*>(&peer_storage), &addrlen) but
fails to reinitialize addrlen on each retry, which can lead to truncated peer
addresses after an EINTR; update the accept retry loop to reset socklen_t
addrlen = sizeof(peer_storage) (or assign addrlen = sizeof(peer_storage))
immediately before each ::accept call (mirroring
select_accept_policy::do_accept()), so each iteration passes a fresh in/out
length for peer_storage and prevents caching an incorrect local_endpoint.
---
Duplicate comments:
In `@include/boost/corosio/local_datagram_socket.hpp`:
- Around line 677-693: Update the public comments for native_handle() and
release() to consistently use "native handle" and native_handle_type semantics
instead of referring to "-1" or "file descriptor"; explicitly state the sentinel
for an invalid handle as the default-constructed native_handle_type (or "invalid
native handle") rather than -1 and avoid platform-specific fd wording (e.g.,
IOCP). For release(), remove the contradictory "returns -1 if not open" —
document the single behavior: either it throws std::logic_error when the socket
is not open and otherwise returns the native_handle_type (transferring
ownership), or (if you prefer non-throwing semantics) document that it returns
an invalid native_handle_type when not open; pick one consistent choice and
apply it to both the release() comment and the native_handle() comment (and
mirror the same wording in the other occurrence noted around lines 769-778).
Ensure all references use native_handle_type and say that pending operations are
deregistered/cancelled, without mentioning file descriptors or -1.
In `@include/boost/corosio/native/detail/kqueue/kqueue_local_stream_service.hpp`:
- Around line 203-211: Guard the unconditional use of SO_NOSIGPIPE with an
`#ifdef` SO_NOSIGPIPE like the other kqueue services: wrap the setsockopt call and
its error handling in a preprocessor check so platforms without SO_NOSIGPIPE
won't try to compile that branch; keep the existing behavior (call
::setsockopt(fd, SOL_SOCKET, SO_NOSIGPIPE, &one, sizeof(one)), capture errno
into errn, ::close(fd), return make_err(errn)) inside the `#ifdef` and leave no-op
or alternate handling when the macro is absent so compilation matches sibling
services (see the block around setsockopt in kqueue_local_stream_service.hpp and
compare to kqueue_local_datagram_service.hpp /
kqueue_local_stream_acceptor_service.hpp).
---
Nitpick comments:
In `@include/boost/corosio/native/detail/iocp/win_local_dgram_service.hpp`:
- Around line 36-45: The function local_dgram_to_native_msg_flags hard-codes bit
positions (1/2/4); replace those magic literals by masking against the public
message_flags enum constants (e.g., message_flags::peek, message_flags::oob,
message_flags::dont_route) inside local_dgram_to_native_msg_flags and map each
named flag to the corresponding MSG_* constant (MSG_PEEK, MSG_OOB,
MSG_DONTROUTE); also ensure the header that defines message_flags is included or
visible so the function compiles against the enum symbols rather than numeric
literals.
In
`@include/boost/corosio/native/detail/kqueue/kqueue_local_stream_acceptor_service.hpp`:
- Around line 321-324: open_acceptor_socket() sets SO_NOSIGPIPE without checking
the return value, which is inconsistent with the stricter handling in the
accept() path; update open_acceptor_socket() to check the ::setsockopt return
value for SO_NOSIGPIPE and on failure behave like the accept() path (close the
socket fd and return/propagate an error or false), and include a matching
error/log message so behavior is consistent with the existing accept() code path
that inspects setsockopt result.
In `@include/boost/corosio/native/detail/reactor/reactor_acceptor_service.hpp`:
- Around line 115-123: Add a short comment documenting the contract that
stream_svc_ is initialized to nullptr and must be assigned by derived classes
(typically after calling use_service<StreamService>() in their constructor)
before any use; reference stream_svc_, the accessor stream_service(), and the
expected initialization point (use_service<StreamService>()) so maintainers know
derived classes must set stream_svc_ (state_type and ctx_ can remain unchanged).
In `@include/boost/corosio/native/detail/reactor/reactor_acceptor.hpp`:
- Around line 237-275: The do_close_socket() and do_release_socket() functions
duplicate the same op-cancellation, descriptor reset and endpoint reset logic;
create a single helper method (e.g., perform_socket_teardown() or
release_descriptor_resources()) that encapsulates the block that locks
desc_state_.mutex, calls acc_.request_cancel(), clears
desc_state_.read_op/read_ready/write_ready, conditionally sets
desc_state_.impl_ref_, posts acc_ via svc_.post(&acc_) if claimed, calls
svc_.work_finished(), deregisters and closes fd_, resets fd_,
desc_state_.fd/registered_events and clears local_endpoint_; then replace the
duplicated blocks in do_close_socket() and do_release_socket() to call this new
helper and leave any function-specific behavior outside it.
In `@include/boost/corosio/native/detail/reactor/reactor_stream_socket.hpp`:
- Around line 92-119: The helper do_shutdown(int what) relies on magic integers
0/1/2 and risks mismatches with caller enums; replace this with a typed API and
explicit mapping: introduce or accept a specific enum (e.g., an enum class
ShutdownMode { Receive, Send, Both }) and add an overload
do_shutdown(ShutdownMode mode) that switches on those enum cases and maps to
SHUT_RD/SHUT_WR/SHUT_RDWR, update callers to pass the enum instead of raw ints,
or alternatively implement a small traits/function (e.g.,
shutdown_mode_to_how(YourCallerEnum)) that performs the explicit mapping and is
referenced from do_shutdown; ensure the original do_shutdown(int) is removed or
marked deprecated so callers can't pass raw integers.
In `@include/boost/corosio/native/detail/select/select_local_datagram_socket.hpp`:
- Around line 31-35: select_local_datagram_op incorrectly instantiates
reactor_op with select_tcp_acceptor (TCP acceptor) even though local datagram
sockets don't accept, so replace the acceptor template argument with a
local-specific or explicit "not applicable" type: either create/use a
select_local_acceptor (or a select_null_acceptor/placeholder like
std::monostate) and change the inheritance to
reactor_op<select_local_datagram_socket, select_local_acceptor> (or
reactor_op<select_local_datagram_socket, select_null_acceptor>), update any
forward declarations/includes so the new acceptor type is visible, and ensure
any code that referenced acceptor_impl_ handles the placeholder appropriately
(no-op or absent).
In `@src/corosio/src/local_datagram_socket.cpp`:
- Around line 14-22: Add a short /* */ file-level comment immediately after the
includes in local_datagram_socket.cpp that explains the high-level design:
describe the wrapper's lifecycle rules (ownership/close semantics), how
operations delegate to detail::local_datagram_service, and note
platform-specific behavior for available() under BOOST_COROSIO_POSIX vs
BOOST_COROSIO_HAS_IOCP (ioctl vs IOCP query). Mention any invariants callers
should expect (threading/async context) and reference functions/classes used in
the file such as available(), detail::local_datagram_service, and the socket
wrapper itself so maintainers can quickly audit the wrapper contract.
🪄 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: 9246cf85-47c2-4121-87f2-e1ee2bfa3744
⛔ 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/local_socket_latency_bench.cppis excluded by!**/bench/**perf/bench/asio/callback/local_socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/asio/coroutine/benchmarks.hppis excluded by!**/bench/**perf/bench/asio/coroutine/local_socket_latency_bench.cppis excluded by!**/bench/**perf/bench/asio/coroutine/local_socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/asio/local_socket_utils.hppis excluded by!**/bench/**perf/bench/corosio/benchmarks.hppis excluded by!**/bench/**perf/bench/corosio/local_socket_latency_bench.cppis excluded by!**/bench/**perf/bench/corosio/local_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 (78)
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_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_local_datagram_service.hppinclude/boost/corosio/native/detail/epoll/epoll_local_datagram_socket.hppinclude/boost/corosio/native/detail/epoll/epoll_local_stream_acceptor.hppinclude/boost/corosio/native/detail/epoll/epoll_local_stream_acceptor_service.hppinclude/boost/corosio/native/detail/epoll/epoll_local_stream_service.hppinclude/boost/corosio/native/detail/epoll/epoll_local_stream_socket.hppinclude/boost/corosio/native/detail/epoll/epoll_op.hppinclude/boost/corosio/native/detail/epoll/epoll_tcp_acceptor_service.hppinclude/boost/corosio/native/detail/epoll/epoll_tcp_socket.hppinclude/boost/corosio/native/detail/epoll/epoll_udp_service.hppinclude/boost/corosio/native/detail/epoll/epoll_udp_socket.hppinclude/boost/corosio/native/detail/iocp/win_local_dgram_service.hppinclude/boost/corosio/native/detail/iocp/win_local_dgram_socket.hppinclude/boost/corosio/native/detail/iocp/win_local_stream_acceptor.hppinclude/boost/corosio/native/detail/iocp/win_local_stream_acceptor_service.hppinclude/boost/corosio/native/detail/iocp/win_local_stream_service.hppinclude/boost/corosio/native/detail/iocp/win_local_stream_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_local_datagram_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_local_datagram_socket.hppinclude/boost/corosio/native/detail/kqueue/kqueue_local_stream_acceptor.hppinclude/boost/corosio/native/detail/kqueue/kqueue_local_stream_acceptor_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_local_stream_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_local_stream_socket.hppinclude/boost/corosio/native/detail/kqueue/kqueue_op.hppinclude/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_tcp_socket.hppinclude/boost/corosio/native/detail/kqueue/kqueue_udp_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_udp_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_acceptor.hppinclude/boost/corosio/native/detail/reactor/reactor_acceptor_service.hppinclude/boost/corosio/native/detail/reactor/reactor_basic_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_datagram_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_op.hppinclude/boost/corosio/native/detail/reactor/reactor_op_complete.hppinclude/boost/corosio/native/detail/reactor/reactor_stream_socket.hppinclude/boost/corosio/native/detail/select/select_local_datagram_service.hppinclude/boost/corosio/native/detail/select/select_local_datagram_socket.hppinclude/boost/corosio/native/detail/select/select_local_stream_acceptor.hppinclude/boost/corosio/native/detail/select/select_local_stream_acceptor_service.hppinclude/boost/corosio/native/detail/select/select_local_stream_service.hppinclude/boost/corosio/native/detail/select/select_local_stream_socket.hppinclude/boost/corosio/native/detail/select/select_op.hppinclude/boost/corosio/native/detail/select/select_tcp_acceptor_service.hppinclude/boost/corosio/native/detail/select/select_tcp_socket.hppinclude/boost/corosio/native/detail/select/select_udp_service.hppinclude/boost/corosio/native/detail/select/select_udp_socket.hppinclude/boost/corosio/native/native_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 skipped from review due to trivial changes (11)
- include/boost/corosio.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_socket.hpp
- include/boost/corosio/io/io_read_stream.hpp
- include/boost/corosio/shutdown_type.hpp
- include/boost/corosio/local_datagram.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_local_stream_socket.hpp
- include/boost/corosio/local_stream.hpp
- include/boost/corosio/message_flags.hpp
- include/boost/corosio/detail/op_base.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_local_stream_acceptor.hpp
- include/boost/corosio/native/detail/iocp/win_local_stream_acceptor.hpp
🚧 Files skipped from review as they are similar to previous changes (21)
- include/boost/corosio/native/detail/epoll/epoll_tcp_socket.hpp
- include/boost/corosio/native/detail/select/select_tcp_socket.hpp
- perf/common/native_includes.hpp
- include/boost/corosio/native/detail/epoll/epoll_op.hpp
- src/corosio/src/local_stream.cpp
- src/corosio/src/local_datagram.cpp
- include/boost/corosio/native/detail/reactor/reactor_basic_socket.hpp
- include/boost/corosio/local_socket_pair.hpp
- src/corosio/src/local_socket_pair.cpp
- include/boost/corosio/native/detail/iocp/win_udp_service.hpp
- include/boost/corosio/local_endpoint.hpp
- include/boost/corosio/native/detail/reactor/reactor_datagram_socket.hpp
- include/boost/corosio/native/detail/select/select_local_stream_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_udp_socket.hpp
- include/boost/corosio/udp_socket.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_local_datagram_socket.hpp
- include/boost/corosio/backend.hpp
- include/boost/corosio/native/detail/epoll/epoll_local_datagram_socket.hpp
- include/boost/corosio/native/detail/epoll/epoll_local_stream_acceptor_service.hpp
- include/boost/corosio/native/detail/iocp/win_local_stream_socket.hpp
- include/boost/corosio/native/detail/iocp/win_local_dgram_socket.hpp
Comment on lines
+118
to
+126
| sockaddr_storage peer_storage{}; | ||
| socklen_t addrlen = sizeof(peer_storage); | ||
| int accepted; | ||
| do | ||
| { | ||
| accepted = | ||
| ::accept(fd_, reinterpret_cast<sockaddr*>(&peer_storage), &addrlen); | ||
| } | ||
| while (accepted < 0 && errno == EINTR); |
There was a problem hiding this comment.
Reset addrlen before each accept() retry.
accept() uses addrlen as an in/out parameter. After an EINTR, the kernel may have already modified it, so reusing the old value on the retry can truncate the peer address and later cache the wrong local_endpoint. This fast path should mirror select_accept_policy::do_accept() and reinitialize addrlen on each loop iteration.
Suggested fix
- socklen_t addrlen = sizeof(peer_storage);
+ socklen_t addrlen;
int accepted;
do
{
+ addrlen = sizeof(peer_storage);
accepted =
::accept(fd_, reinterpret_cast<sockaddr*>(&peer_storage), &addrlen);
}
while (accepted < 0 && errno == EINTR);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@include/boost/corosio/native/detail/select/select_local_stream_acceptor_service.hpp`
around lines 118 - 126, The loop in select_local_stream_acceptor_service::...
calls ::accept(fd_, reinterpret_cast<sockaddr*>(&peer_storage), &addrlen) but
fails to reinitialize addrlen on each retry, which can lead to truncated peer
addresses after an EINTR; update the accept retry loop to reset socklen_t
addrlen = sizeof(peer_storage) (or assign addrlen = sizeof(peer_storage))
immediately before each ::accept call (mirroring
select_accept_policy::do_accept()), so each iteration passes a fresh in/out
length for peer_storage and prevents caching an incorrect local_endpoint.
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.
Summary by CodeRabbit
New Features
local_stream_socketandlocal_datagram_sockettypes across all backends (epoll, kqueue, select, IOCP).local_endpointfor addressing Unix domain sockets.local_stream_acceptorfor accepting local stream connections.message_flagsenum and flag parameters to UDP send/receive operations.Refactoring