Add IPv6 socket support with dual-stack, lazy fd creation, and generi…#175
Merged
sgerbino merged 1 commit intocppalliance:developfrom Feb 25, 2026
Merged
Add IPv6 socket support with dual-stack, lazy fd creation, and generi…#175sgerbino merged 1 commit intocppalliance:developfrom
sgerbino merged 1 commit intocppalliance:developfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (13)
📒 Files selected for processing (37)
📝 WalkthroughWalkthroughThis pull request refactors socket and acceptor lifecycle management by splitting combined operations into discrete steps, replaces socket-specific option methods with a generic level/optname interface, introduces type-safe option abstractions, and adds comprehensive IPv4/IPv6 dual-stack support with proper address family handling throughout the codebase. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Socket as tcp_socket
participant Impl as socket_service
participant Native as OS
User->>Socket: open(tcp::v6())
Socket->>Impl: open_socket(impl, AF_INET6, SOCK_STREAM, IPPROTO_TCP)
Impl->>Native: socket(AF_INET6, SOCK_STREAM, IPPROTO_TCP)
Native-->>Impl: fd
Impl->>Native: setsockopt(IPV6_V6ONLY, 1)
Impl-->>Socket: error_code
User->>Socket: set_option(no_delay(true))
Socket->>Impl: set_option(IPPROTO_TCP, TCP_NODELAY, data, size)
Impl->>Native: setsockopt(IPPROTO_TCP, TCP_NODELAY, 1)
Native-->>Impl: result
Impl-->>Socket: void
sequenceDiagram
actor User
participant Acceptor as tcp_acceptor
participant Service as acceptor_service
participant Native as OS
User->>Acceptor: open(tcp::v6())
Acceptor->>Service: open_acceptor_socket(impl, AF_INET6, SOCK_STREAM, IPPROTO_TCP)
Service->>Native: socket(AF_INET6, SOCK_STREAM, IPPROTO_TCP)
Native-->>Service: fd
Service->>Native: setsockopt(IPV6_V6ONLY, 1)
Service-->>Acceptor: error_code
User->>Acceptor: bind(endpoint)
Acceptor->>Service: bind_acceptor(impl, endpoint)
Service->>Native: bind(fd, sockaddr, len)
Service->>Native: getsockname(fd, storage)
Service-->>Acceptor: error_code
User->>Acceptor: listen(128)
Acceptor->>Service: listen_acceptor(impl, 128)
Service->>Native: listen(fd, 128)
Service->>Native: register with epoll/kqueue/iocp
Service-->>Acceptor: error_code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
An automated preview of the documentation is available at https://175.corosio.prtest3.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-02-25 01:25:47 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: 5
🧹 Nitpick comments (10)
include/boost/corosio/io/io_read_stream.hpp (1)
108-111: Add doc comments for changed special members in this public header.Line 108 through Line 111 modify ownership/movability semantics, but these function declarations are undocumented. Please add brief docstrings describing the intent (movable, non-copyable, move-assignment disabled and why).
Proposed documentation update
+ /** Move-construct the stream base. */ io_read_stream(io_read_stream&&) noexcept = default; + /** Disable move assignment to prevent unsafe reassignment of shared base state. */ io_read_stream& operator=(io_read_stream&&) noexcept = delete; + /** Disable copy construction; stream bases are non-copyable. */ io_read_stream(io_read_stream const&) = delete; + /** Disable copy assignment; stream bases are non-copyable. */ io_read_stream& operator=(io_read_stream const&) = delete;As per coding guidelines, "Docstrings are required for all classes and functions in public headers in non-detail namespaces."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/io/io_read_stream.hpp` around lines 108 - 111, Add brief doc comments above the four special-member declarations in the public header for io_read_stream: document that the move constructor io_read_stream(io_read_stream&&) noexcept enables move semantics (transfers ownership of underlying I/O resources), the move-assignment operator io_read_stream& operator=(io_read_stream&&) noexcept = delete is intentionally deleted (explain why move-assignment is unsafe or would require complex state management), and that both the copy constructor io_read_stream(io_read_stream const&) = delete and copy-assignment io_read_stream& operator=(io_read_stream const&) = delete disable copying to prevent duplicate ownership of resources; keep comments short, on one or two lines each, and place them directly above the corresponding declarations.include/boost/corosio/socket_option.hpp (2)
55-60: Fill in full contract docs for public mutators (@param/preconditions).A few public methods are documented tersely (e.g., assignment/mutator overloads) but don’t consistently describe parameter constraints/preconditions in the style required for public headers.
As per coding guidelines: "Docstrings are required for all classes and functions in public headers in non-detail namespaces... document all parameters, return values, and any preconditions, postconditions, or exceptions."
Also applies to: 113-117, 335-341
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/socket_option.hpp` around lines 55 - 60, The assignment operator boolean_option& operator=(bool v) noexcept is missing full doc comments; add a Doxygen-style docblock that documents the parameter (`@param` v true to set the option on, false to set it off), the return value (`@return` reference to *this), and any preconditions/postconditions (no preconditions; postcondition: value() equals v), and note noexcept behavior; apply the same treatment to the other public mutators in this header (the other assignment/setter overloads and public setter methods) so every public mutator documents parameters, return value, and pre/postconditions consistently.
329-341: Guardlingertimeout against negative and out-of-range values before platform conversion.The
timeout( int v )setter (line 340) and constructor (line 329) cast the signedinttimeout directly tol_lingerviastatic_cast<decltype(value_.l_linger)>(timeout)without validation. On platforms wherel_lingeris narrower (e.g.,unsigned short) or unsigned, negative or oversized values will silently wrap and alter close behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/socket_option.hpp` around lines 329 - 341, The linger(bool, int) constructor and the timeout(int v) setter currently cast the signed int directly into value_.l_linger which can wrap on platforms where l_linger is narrower or unsigned; change both to validate and clamp the incoming timeout: if timeout is negative set to 0, if timeout exceeds std::numeric_limits<decltype(value_.l_linger)>::max() clamp to that max, then perform the safe static_cast to value_.l_linger; update the implementation of linger(bool,int) and timeout(int) to use this guarded clamp (and add `#include` <limits> if needed) so negative or out-of-range values cannot wrap value_.l_linger.include/boost/corosio/native/detail/iocp/win_acceptor.hpp (1)
130-135: Document the newly added option methods.Please add concise docs for
set_optionandget_option, includingdatapointer expectations andsizehandling, to keep IOCP backend API contracts explicit.As per coding guidelines, parameter lifetime/ownership and return behavior should be documented for each function.
🤖 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_acceptor.hpp` around lines 130 - 135, Add concise Doxygen-style comments above the set_option and get_option declarations (functions set_option and get_option) that specify: the purpose of each method, that data is a pointer to option-specific bytes owned by the caller (no ownership transferred), how size is interpreted on input (number of bytes available) and output (for get_option, set *size to the number of bytes written or required), how null pointers are treated, and that the returned std::error_code indicates success or failure (no exceptions). Also document noexcept and that callers must ensure the pointed-to memory remains valid for the duration of the call.include/boost/corosio/native/detail/kqueue/kqueue_acceptor.hpp (1)
84-89: Add doc comments for the new option overrides.Please document
set_optionandget_optionsimilarly to nearby methods, including pointer ownership/validity expectations fordataand in-out semantics forsize.As per coding guidelines, each parameter and return value should be explicitly documented, including lifetime semantics for buffer/pointer-like parameters.
🤖 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_acceptor.hpp` around lines 84 - 89, Add Doxygen-style comments for the two new overrides set_option and get_option in kqueue_acceptor.hpp mirroring nearby documented methods: document each parameter (level, optname, data, size) and the return value; state that data is a non-owning pointer whose lifetime must extend for the call, that for set_option data is read-only and size is the data length, and for get_option data is an output buffer with size as an in-out parameter (input = buffer capacity, output = actual bytes written); also document noexcept override behavior and error_code meanings on failure/success.include/boost/corosio/native/detail/iocp/win_sockets.hpp (1)
98-100: Updateopen_socketdocs for the new parameters.The signature now includes
family,type, andprotocol, but the existing doc block does not describe them yet.As per coding guidelines, every parameter should be documented with clear semantics.
🤖 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_sockets.hpp` around lines 98 - 100, Update the documentation block for the function open_socket to describe the new parameters: document win_socket_internal& impl (purpose and ownership/lifetime expectations), int family (address family semantics, e.g. AF_INET/AF_INET6 and effect on returned socket), int type (socket type semantics, e.g. SOCK_STREAM/SOCK_DGRAM), and int protocol (protocol number or 0 for default), and note any constraints or error behavior related to these values; keep the doc style consistent with surrounding comments and reference the open_socket and win_socket_internal symbols so readers can locate the implementation easily.include/boost/corosio/native/detail/select/select_acceptor.hpp (1)
62-67: Add method contracts for the new option APIs.
set_option/get_optionare newly exposed overrides, but they currently lack parameter and return documentation (especiallydata/sizein-out semantics). Please add concise@paramand@returndocs to prevent misuse.As per coding guidelines, "
@param documentation for each parameter... and@return documentation" are required elements.🤖 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_acceptor.hpp` around lines 62 - 67, Add Doxygen-style method contracts for the newly exposed overrides set_option(...) and get_option(...): document each parameter with `@param` (level - protocol level, optname - option identifier, data - pointer to option value with explicit in/out semantics, size - size of buffer or, for get_option, in: pointer to available buffer size, out: actual bytes written or required size) and describe ownership/validity expectations and that data may be null for size-query operations; add `@return` describing the std::error_code result (success vs specific error codes on failure) and note noexcept behavior. Ensure both functions include concise `@param/`@return lines and any special cases (e.g., when size is updated on error or when data is ignored) to match coding guidelines.include/boost/corosio/native/detail/epoll/epoll_acceptor.hpp (1)
61-66: Documentset_option/get_optionusage contracts.These new declarations need brief API docs for argument semantics (
level,optname,data,size) and returned error behavior to match the rest of the public backend surface.As per coding guidelines, function docs should include complete
@paramand@returncoverage.🤖 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_acceptor.hpp` around lines 61 - 66, Add Doxygen-style API docs above the declarations of set_option and get_option in epoll_acceptor.hpp: describe each `@param` (level as socket/option level such as SOL_SOCKET or IPPROTO_TCP, optname as the platform-specific option identifier, data as pointer to the option value buffer, size as the size of the value for set_option and as an in/out pointer for get_option where it receives available buffer size and on return is set to the number of bytes written), and the `@return` contract (std::error_code: default-constructed on success, platform-specific errno mapped to std::errc or std::error_code on failure). Also note noexcept/const semantics and that data must point to valid memory of at least the supplied size and that callers are responsible for platform-specific value formats.include/boost/corosio/native/native_socket_option.hpp (1)
32-39: Platform headers are intentional here, but technically violate the guideline.This file includes
<sys/socket.h>,<netinet/in.h>, etc., which falls under theinclude/boost/corosio/**/*.{h,hpp}glob that prohibits platform-specific headers. However, the file is clearly designed as the "native" variant with the portablesocket_option.hppalternative documented at lines 18–20. If this is the accepted convention for thenative/subdirectory, consider adding an exception to the guideline to avoid future confusion.As per coding guidelines: "Public headers in
include/boost/corosio/and all subdirectories MUST NOT include platform-specific headers"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/native/native_socket_option.hpp` around lines 32 - 39, The native header native_socket_option.hpp intentionally includes platform-specific headers which violates the general rule banning platform headers under include/boost/corosio/*; fix this by either (A) adding a short top-of-file comment in native_socket_option.hpp explaining that the native/ variant is an allowed exception and referencing the alternate portable socket_option.hpp, or (B) updating the project coding-guidelines file to explicitly permit platform headers under include/boost/corosio/native/** so future reviewers understand the convention; ensure the comment or guideline change mentions native_socket_option.hpp and socket_option.hpp so the exception is discoverable.src/corosio/src/socket_option.cpp (1)
62-64:static_assertinside the constructor body should be hoisted to class or namespace scope.A
static_assertnested in a constructor is only checked when that specific constructor is reachable from the TU being compiled. Moving it to namespace scope (or as astatic_assertdata member in the class definition) ensures it is evaluated unconditionally and catches regressions immediately on all platforms rather than only when the constructor is instantiated.♻️ Proposed refactor
Add to the class definition in the header (preferred), or at the top of the namespace block here:
+static_assert( + sizeof( native_socket_option::linger ) <= sizeof( linger::storage_ ), + "platform linger exceeds socket_option::linger storage" ); + linger::linger( bool enabled, int timeout ) noexcept { native_socket_option::linger native( enabled, timeout ); - static_assert( - sizeof( native ) <= sizeof( storage_ ), - "platform linger exceeds socket_option::linger storage" ); std::memcpy( storage_, native.data(), native.size() ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/corosio/src/socket_option.cpp` around lines 62 - 64, Move the static_assert out of the constructor body so it is evaluated unconditionally: remove the static_assert( sizeof(native) <= sizeof(storage_), "platform linger exceeds socket_option::linger storage" ) from the constructor in socket_option.cpp and place an equivalent static_assert at namespace or class scope (e.g., in the socket_option class definition or top of the namespace) so the check for native vs storage_ size (and the socket_option::linger storage assumption) is performed at compile time for all translation units.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/boost/corosio/native/detail/kqueue/kqueue_socket_service.hpp`:
- Around line 599-618: The generic kqueue_socket::set_option path fails to
update the user_set_linger_ flag after the dedicated linger setter was removed,
breaking close-time SO_LINGER logic; inside kqueue_socket::set_option, detect
when level == SOL_SOCKET and optname == SO_LINGER, extract the linger::l_onoff
value from the supplied data (safely casting the void* to const linger* or
reading sizeof checks), set user_set_linger_ = (l_onoff != 0) only after a
successful ::setsockopt call, and leave other behavior unchanged so
shutdown/destroy sees the correct user-set linger state.
In `@include/boost/corosio/native/native_tcp.hpp`:
- Around line 30-36: The public header native_tcp.hpp is including
platform-specific socket headers and using platform macros (e.g. _WIN32,
winsock2/ws2tcpip or netinet/in.h and sys/socket.h) which violates portability;
remove all platform-specific `#includes` and platform-specific macros/usages from
the header and replace them with a platform-agnostic API (e.g. forward-declare
opaque types or use std::intptr_t/portable typedefs) in native_tcp.hpp, then
implement the OS-specific includes and macro logic in a corresponding .cpp or
platform-specific implementation unit (implementations referenced by the
header's functions/classes such as any socket wrapper class or functions
declared around lines ~70-80) so consumers of the public header no longer
transitively include OS headers.
In `@src/corosio/src/socket_option.cpp`:
- Around line 10-13: Add a file-level /* ... */ block comment immediately after
the includes in src/corosio/src/socket_option.cpp that documents the
memcpy-based round-trip serialization approach for linger: explain why a raw
storage_ buffer is used, what invariants storage_ must maintain (size,
alignment, and raw bytes mapping), and why each accessor constructs a temporary
native_socket_option::linger for materialization; reference the storage_ member,
the native_socket_option::linger type, and the accessor/serializer functions so
future maintainers understand the design and safety assumptions.
- Around line 42-44: The fallback for reuse_port (the `#else` block returning
SOL_SOCKET and name() = -1) must be removed because it silently produces invalid
socket options; instead guard the entire reuse_port implementation/class with
`#ifdef` SO_REUSEPORT (or equivalent platform macro) so reuse_port,
reuse_port::level() and reuse_port::name() only exist when SO_REUSEPORT is
defined; if you must keep a fallback, explicitly expose an unsupported state
(e.g., reuse_port::is_supported() or similar) and include the proper platform
socket header rather than relying on transitive includes (reference symbols:
reuse_port, level(), name(), SO_REUSEPORT, setsockopt).
In `@src/corosio/src/tcp_server.cpp`:
- Around line 94-103: The catch currently only handles std::system_error so a
std::bad_alloc from impl_->ports.emplace_back(...) can escape a function that
should return std::error_code; change the error handling in the function that
calls impl_->ports.emplace_back to catch all exceptions (e.g., catch
(std::exception const& e) or catch (...) after the std::system_error handler)
and convert allocation or other unexpected exceptions into an appropriate
std::error_code (for example std::errc::not_enough_memory or a generic
std::errc::state_not_recoverable) before returning, while keeping the existing
std::system_error handling for tcp_acceptor constructor-specific errors. Ensure
the code references impl_->ports.emplace_back, the tcp_acceptor constructor
behavior, and the function's std::error_code return contract when implementing
the additional catch-and-convert logic.
---
Nitpick comments:
In `@include/boost/corosio/io/io_read_stream.hpp`:
- Around line 108-111: Add brief doc comments above the four special-member
declarations in the public header for io_read_stream: document that the move
constructor io_read_stream(io_read_stream&&) noexcept enables move semantics
(transfers ownership of underlying I/O resources), the move-assignment operator
io_read_stream& operator=(io_read_stream&&) noexcept = delete is intentionally
deleted (explain why move-assignment is unsafe or would require complex state
management), and that both the copy constructor io_read_stream(io_read_stream
const&) = delete and copy-assignment io_read_stream& operator=(io_read_stream
const&) = delete disable copying to prevent duplicate ownership of resources;
keep comments short, on one or two lines each, and place them directly above the
corresponding declarations.
In `@include/boost/corosio/native/detail/epoll/epoll_acceptor.hpp`:
- Around line 61-66: Add Doxygen-style API docs above the declarations of
set_option and get_option in epoll_acceptor.hpp: describe each `@param` (level as
socket/option level such as SOL_SOCKET or IPPROTO_TCP, optname as the
platform-specific option identifier, data as pointer to the option value buffer,
size as the size of the value for set_option and as an in/out pointer for
get_option where it receives available buffer size and on return is set to the
number of bytes written), and the `@return` contract (std::error_code:
default-constructed on success, platform-specific errno mapped to std::errc or
std::error_code on failure). Also note noexcept/const semantics and that data
must point to valid memory of at least the supplied size and that callers are
responsible for platform-specific value formats.
In `@include/boost/corosio/native/detail/iocp/win_acceptor.hpp`:
- Around line 130-135: Add concise Doxygen-style comments above the set_option
and get_option declarations (functions set_option and get_option) that specify:
the purpose of each method, that data is a pointer to option-specific bytes
owned by the caller (no ownership transferred), how size is interpreted on input
(number of bytes available) and output (for get_option, set *size to the number
of bytes written or required), how null pointers are treated, and that the
returned std::error_code indicates success or failure (no exceptions). Also
document noexcept and that callers must ensure the pointed-to memory remains
valid for the duration of the call.
In `@include/boost/corosio/native/detail/iocp/win_sockets.hpp`:
- Around line 98-100: Update the documentation block for the function
open_socket to describe the new parameters: document win_socket_internal& impl
(purpose and ownership/lifetime expectations), int family (address family
semantics, e.g. AF_INET/AF_INET6 and effect on returned socket), int type
(socket type semantics, e.g. SOCK_STREAM/SOCK_DGRAM), and int protocol (protocol
number or 0 for default), and note any constraints or error behavior related to
these values; keep the doc style consistent with surrounding comments and
reference the open_socket and win_socket_internal symbols so readers can locate
the implementation easily.
In `@include/boost/corosio/native/detail/kqueue/kqueue_acceptor.hpp`:
- Around line 84-89: Add Doxygen-style comments for the two new overrides
set_option and get_option in kqueue_acceptor.hpp mirroring nearby documented
methods: document each parameter (level, optname, data, size) and the return
value; state that data is a non-owning pointer whose lifetime must extend for
the call, that for set_option data is read-only and size is the data length, and
for get_option data is an output buffer with size as an in-out parameter (input
= buffer capacity, output = actual bytes written); also document noexcept
override behavior and error_code meanings on failure/success.
In `@include/boost/corosio/native/detail/select/select_acceptor.hpp`:
- Around line 62-67: Add Doxygen-style method contracts for the newly exposed
overrides set_option(...) and get_option(...): document each parameter with
`@param` (level - protocol level, optname - option identifier, data - pointer to
option value with explicit in/out semantics, size - size of buffer or, for
get_option, in: pointer to available buffer size, out: actual bytes written or
required size) and describe ownership/validity expectations and that data may be
null for size-query operations; add `@return` describing the std::error_code
result (success vs specific error codes on failure) and note noexcept behavior.
Ensure both functions include concise `@param/`@return lines and any special cases
(e.g., when size is updated on error or when data is ignored) to match coding
guidelines.
In `@include/boost/corosio/native/native_socket_option.hpp`:
- Around line 32-39: The native header native_socket_option.hpp intentionally
includes platform-specific headers which violates the general rule banning
platform headers under include/boost/corosio/*; fix this by either (A) adding a
short top-of-file comment in native_socket_option.hpp explaining that the
native/ variant is an allowed exception and referencing the alternate portable
socket_option.hpp, or (B) updating the project coding-guidelines file to
explicitly permit platform headers under include/boost/corosio/native/** so
future reviewers understand the convention; ensure the comment or guideline
change mentions native_socket_option.hpp and socket_option.hpp so the exception
is discoverable.
In `@include/boost/corosio/socket_option.hpp`:
- Around line 55-60: The assignment operator boolean_option& operator=(bool v)
noexcept is missing full doc comments; add a Doxygen-style docblock that
documents the parameter (`@param` v true to set the option on, false to set it
off), the return value (`@return` reference to *this), and any
preconditions/postconditions (no preconditions; postcondition: value() equals
v), and note noexcept behavior; apply the same treatment to the other public
mutators in this header (the other assignment/setter overloads and public setter
methods) so every public mutator documents parameters, return value, and
pre/postconditions consistently.
- Around line 329-341: The linger(bool, int) constructor and the timeout(int v)
setter currently cast the signed int directly into value_.l_linger which can
wrap on platforms where l_linger is narrower or unsigned; change both to
validate and clamp the incoming timeout: if timeout is negative set to 0, if
timeout exceeds std::numeric_limits<decltype(value_.l_linger)>::max() clamp to
that max, then perform the safe static_cast to value_.l_linger; update the
implementation of linger(bool,int) and timeout(int) to use this guarded clamp
(and add `#include` <limits> if needed) so negative or out-of-range values cannot
wrap value_.l_linger.
In `@src/corosio/src/socket_option.cpp`:
- Around line 62-64: Move the static_assert out of the constructor body so it is
evaluated unconditionally: remove the static_assert( sizeof(native) <=
sizeof(storage_), "platform linger exceeds socket_option::linger storage" ) from
the constructor in socket_option.cpp and place an equivalent static_assert at
namespace or class scope (e.g., in the socket_option class definition or top of
the namespace) so the check for native vs storage_ size (and the
socket_option::linger storage assumption) is performed at compile time for all
translation units.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (13)
doc/design/physical-structure.mdis excluded by!**/doc/**include/boost/corosio/test/mocket.hppis excluded by!**/test/**include/boost/corosio/test/socket_pair.hppis excluded by!**/test/**perf/bench/corosio/accept_churn_bench.cppis excluded by!**/bench/**perf/bench/corosio/fan_out_bench.cppis excluded by!**/bench/**perf/bench/corosio/http_server_bench.cppis excluded by!**/bench/**perf/bench/corosio/socket_latency_bench.cppis excluded by!**/bench/**test/unit/acceptor.cppis excluded by!**/test/**test/unit/native/native_io.cppis excluded by!**/test/**test/unit/native/native_tcp_acceptor.cppis excluded by!**/test/**test/unit/socket.cppis excluded by!**/test/**test/unit/socket_stress.cppis excluded by!**/test/**test/unit/tcp_server.cppis excluded by!**/test/**
📒 Files selected for processing (37)
include/boost/corosio/detail/acceptor_service.hppinclude/boost/corosio/detail/endpoint_convert.hppinclude/boost/corosio/detail/socket_service.hppinclude/boost/corosio/io/io_read_stream.hppinclude/boost/corosio/io/io_write_stream.hppinclude/boost/corosio/ipv6_address.hppinclude/boost/corosio/native/detail/epoll/epoll_acceptor.hppinclude/boost/corosio/native/detail/epoll/epoll_acceptor_service.hppinclude/boost/corosio/native/detail/epoll/epoll_op.hppinclude/boost/corosio/native/detail/epoll/epoll_socket.hppinclude/boost/corosio/native/detail/epoll/epoll_socket_service.hppinclude/boost/corosio/native/detail/iocp/win_acceptor.hppinclude/boost/corosio/native/detail/iocp/win_acceptor_service.hppinclude/boost/corosio/native/detail/iocp/win_socket.hppinclude/boost/corosio/native/detail/iocp/win_sockets.hppinclude/boost/corosio/native/detail/kqueue/kqueue_acceptor.hppinclude/boost/corosio/native/detail/kqueue/kqueue_acceptor_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_socket.hppinclude/boost/corosio/native/detail/kqueue/kqueue_socket_service.hppinclude/boost/corosio/native/detail/select/select_acceptor.hppinclude/boost/corosio/native/detail/select/select_acceptor_service.hppinclude/boost/corosio/native/detail/select/select_op.hppinclude/boost/corosio/native/detail/select/select_socket.hppinclude/boost/corosio/native/detail/select/select_socket_service.hppinclude/boost/corosio/native/native_socket_option.hppinclude/boost/corosio/native/native_tcp.hppinclude/boost/corosio/socket_option.hppinclude/boost/corosio/tcp.hppinclude/boost/corosio/tcp_acceptor.hppinclude/boost/corosio/tcp_socket.hppperf/profile/concurrent_io_bench.cppperf/profile/small_io_bench.cppsrc/corosio/src/socket_option.cppsrc/corosio/src/tcp.cppsrc/corosio/src/tcp_acceptor.cppsrc/corosio/src/tcp_server.cppsrc/corosio/src/tcp_socket.cpp
Comment on lines
+30
to
+36
| #ifdef _WIN32 | ||
| #include <winsock2.h> | ||
| #include <ws2tcpip.h> | ||
| #else | ||
| #include <netinet/in.h> | ||
| #include <sys/socket.h> | ||
| #endif |
There was a problem hiding this comment.
Public header currently violates the portability rule.
native_tcp.hpp pulls in platform socket headers and inline-uses platform socket macros in a public header. This couples user translation units to OS socket headers and conflicts with the repository portability constraints.
🔧 Suggested direction
-#ifdef _WIN32
-#include <winsock2.h>
-#include <ws2tcpip.h>
-#else
-#include <netinet/in.h>
-#include <sys/socket.h>
-#endif
+// Keep this public header platform-neutral.
- int family() const noexcept
- {
- return v6_ ? AF_INET6 : AF_INET;
- }
+ int family() const noexcept;
- static constexpr int type() noexcept { return SOCK_STREAM; }
- static constexpr int protocol() noexcept { return IPPROTO_TCP; }
+ static int type() noexcept;
+ static int protocol() noexcept;Then implement these in a .cpp/platform abstraction implementation unit.
As per coding guidelines: "Public headers in include/boost/corosio/ and all subdirectories MUST NOT include platform-specific headers" and "Platform-specific macros and types that require platform-specific headers are prohibited in public headers".
Also applies to: 70-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/boost/corosio/native/native_tcp.hpp` around lines 30 - 36, The public
header native_tcp.hpp is including platform-specific socket headers and using
platform macros (e.g. _WIN32, winsock2/ws2tcpip or netinet/in.h and
sys/socket.h) which violates portability; remove all platform-specific `#includes`
and platform-specific macros/usages from the header and replace them with a
platform-agnostic API (e.g. forward-declare opaque types or use
std::intptr_t/portable typedefs) in native_tcp.hpp, then implement the
OS-specific includes and macro logic in a corresponding .cpp or
platform-specific implementation unit (implementations referenced by the
header's functions/classes such as any socket wrapper class or functions
declared around lines ~70-80) so consumers of the public header no longer
transitively include OS headers.
Comment on lines
+10
to
+13
| #include <boost/corosio/socket_option.hpp> | ||
| #include <boost/corosio/native/native_socket_option.hpp> | ||
|
|
||
| #include <cstring> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing file-level overview block comment for the non-trivial linger implementation.
Per coding guidelines, implementation files with non-trivial logic should include a /* */ block comment after the includes explaining the approach. The memcpy-based round-trip serialization strategy for linger (why a raw storage_ buffer is used, what invariants storage_ must satisfy, why every accessor materialises a temporary native_socket_option::linger) is not self-evident to a maintainer.
📝 Suggested addition
`#include` <cstring>
+/*
+ * Implements the portable socket_option wrappers by delegating to the
+ * native_socket_option layer, which owns all platform-specific constants
+ * (IPPROTO_TCP, SOL_SOCKET, TCP_NODELAY, …).
+ *
+ * linger is the only option that carries value state. Its payload is kept
+ * in a raw byte buffer (storage_) whose size is statically asserted to be
+ * at least as large as the platform's native ::linger struct. Every accessor
+ * round-trips through a temporary native_socket_option::linger via std::memcpy
+ * so that field encoding/decoding is encapsulated in the native layer and
+ * does not leak platform struct layout knowledge here.
+ */
+
namespace boost::corosio::socket_option {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/corosio/src/socket_option.cpp` around lines 10 - 13, Add a file-level /*
... */ block comment immediately after the includes in
src/corosio/src/socket_option.cpp that documents the memcpy-based round-trip
serialization approach for linger: explain why a raw storage_ buffer is used,
what invariants storage_ must maintain (size, alignment, and raw bytes mapping),
and why each accessor constructs a temporary native_socket_option::linger for
materialization; reference the storage_ member, the native_socket_option::linger
type, and the accessor/serializer functions so future maintainers understand the
design and safety assumptions.
Comment on lines
+94
to
103
| try | ||
| { | ||
| impl_->ports.emplace_back(impl_->ctx, ep); | ||
| return {}; | ||
| } | ||
| catch (std::system_error const& e) | ||
| { | ||
| return e.code(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for tcp_acceptor class definition and constructor
fd -e hpp -e h | xargs rg -l "tcp_acceptor" | head -5Repository: cppalliance/corosio
Length of output: 234
🏁 Script executed:
#!/bin/bash
# Find tcp_acceptor constructor definition
rg "tcp_acceptor\s*\(" --type=hpp -A 10 -B 2Repository: cppalliance/corosio
Length of output: 90
🏁 Script executed:
#!/bin/bash
# Check the tcp_server.cpp file context around line 96 to see the full function
cat -n src/corosio/src/tcp_server.cpp | sed -n '80,110p'Repository: cppalliance/corosio
Length of output: 832
🏁 Script executed:
#!/bin/bash
# Find tcp_acceptor class definition - search in headers
find . -name "*.hpp" -o -name "*.h" | xargs rg -l "class tcp_acceptor" 2>/dev/null | head -10Repository: cppalliance/corosio
Length of output: 179
🏁 Script executed:
#!/bin/bash
# Also check what impl_->ports is declared as
rg "ports" include/boost/corosio/tcp_server.hpp -B 3 -A 3Repository: cppalliance/corosio
Length of output: 326
🏁 Script executed:
#!/bin/bash
# Find tcp_acceptor definition broadly
rg "class tcp_acceptor|struct tcp_acceptor" -A 15Repository: cppalliance/corosio
Length of output: 2960
🏁 Script executed:
#!/bin/bash
# Get the tcp_acceptor class definition from tcp.hpp
cat include/boost/corosio/tcp.hppRepository: cppalliance/corosio
Length of output: 2352
🏁 Script executed:
#!/bin/bash
# Search for tcp_acceptor class definition
rg "class tcp_acceptor\s*\{|class tcp_acceptor\s*:" -A 30Repository: cppalliance/corosio
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Look for tcp_acceptor header file
find include -name "*tcp_acceptor*" -o -name "*acceptor*"Repository: cppalliance/corosio
Length of output: 726
🏁 Script executed:
#!/bin/bash
# Search for tcp_acceptor constructor
rg "tcp_acceptor\s*\(" -B 5 -A 10 | head -100Repository: cppalliance/corosio
Length of output: 6165
🏁 Script executed:
#!/bin/bash
# Get full tcp_acceptor.hpp to see complete constructor documentation
cat include/boost/corosio/tcp_acceptor.hppRepository: cppalliance/corosio
Length of output: 15464
Incomplete catch leaves std::bad_alloc escaping from a function that promises std::error_code return.
vector::emplace_back can throw std::bad_alloc during reallocation, and while the tcp_acceptor constructor only throws std::system_error (per its documented contract), the vector operation is independent. The current catch block only handles std::system_error, allowing std::bad_alloc to propagate uncaught to callers who expect only an error_code return value. This violates the API contract and could terminate the process if unhandled upstream.
🛡️ Proposed fix
try
{
impl_->ports.emplace_back(impl_->ctx, ep);
return {};
}
catch (std::system_error const& e)
{
return e.code();
}
+ catch (std::bad_alloc const&)
+ {
+ return std::make_error_code(std::errc::not_enough_memory);
+ }
+ catch (...)
+ {
+ return std::make_error_code(std::errc::operation_not_permitted);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try | |
| { | |
| impl_->ports.emplace_back(impl_->ctx, ep); | |
| return {}; | |
| } | |
| catch (std::system_error const& e) | |
| { | |
| return e.code(); | |
| } | |
| } | |
| try | |
| { | |
| impl_->ports.emplace_back(impl_->ctx, ep); | |
| return {}; | |
| } | |
| catch (std::system_error const& e) | |
| { | |
| return e.code(); | |
| } | |
| catch (std::bad_alloc const&) | |
| { | |
| return std::make_error_code(std::errc::not_enough_memory); | |
| } | |
| catch (...) | |
| { | |
| return std::make_error_code(std::errc::operation_not_permitted); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/corosio/src/tcp_server.cpp` around lines 94 - 103, The catch currently
only handles std::system_error so a std::bad_alloc from
impl_->ports.emplace_back(...) can escape a function that should return
std::error_code; change the error handling in the function that calls
impl_->ports.emplace_back to catch all exceptions (e.g., catch (std::exception
const& e) or catch (...) after the std::system_error handler) and convert
allocation or other unexpected exceptions into an appropriate std::error_code
(for example std::errc::not_enough_memory or a generic
std::errc::state_not_recoverable) before returning, while keeping the existing
std::system_error handling for tcp_acceptor constructor-specific errors. Ensure
the code references impl_->ports.emplace_back, the tcp_acceptor constructor
behavior, and the function's std::error_code return contract when implementing
the additional catch-and-convert logic.
|
GCOVR code coverage report https://175.corosio.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-02-25 01:30:12 UTC |
…c socket options
New public types:
- tcp: compiled protocol type (family/type/protocol without
platform headers); inline native_tcp variant for zero-overhead
- socket_option: type-safe wrappers (no_delay, keep_alive,
reuse_address, reuse_port, v6_only, linger, send/receive
buffer sizes); inline native_socket_option variant
Acceptor API:
- Split monolithic listen(endpoint) into open() / bind() / listen()
so socket options can be set between open and listen
- Add convenience constructor: tcp_acceptor(ctx, ep, backlog)
for the common open+reuse_address+bind+listen pattern
- Add set_option() / get_option() on tcp_acceptor and its
implementation interface (all 4 backends)
Socket changes:
- tcp_socket::open() takes tcp protocol type (defaults to v4)
- connect() on a closed socket auto-opens with the endpoint's
address family via open(tcp::v6()) / open(tcp::v4())
- Service layer passes (family, type, protocol) triple through
socket_service, acceptor_service, and all 4 backends; backends
use the caller-provided triple in ::socket() / WSASocketW()
- IOCP backend stores address family in win_socket_internal
for the ephemeral bind required by ConnectEx
- Delete move assignment on io_read_stream / io_write_stream
to prevent double-move of virtual base in diamond hierarchy
IPv6 support across all backends:
- Sockets: IPV6_V6ONLY=1 (v6-only by default, user can clear)
- Acceptors: IPV6_V6ONLY=0 (dual-stack by default)
- endpoint_convert: IPv4-mapped IPv6 conversion when an IPv4
endpoint connects through an AF_INET6 socket
Tests:
- IPv6 connect, read/write, v6_only option, dual-stack connect
- Lazy open (connect auto-opens), preserves pre-set options
- Acceptor: open/bind/listen lifecycle, set/get_option,
convenience constructor, IPv6 accept
- All existing tests updated for the new open/bind/listen API
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.
…c socket options
New public types:
Acceptor API:
Socket changes:
IPv6 support across all backends:
Tests:
Resolves #128.
Summary by CodeRabbit
New Features
tcp::v4()andtcp::v6().set_option()andget_option()methods.API Changes
open(),bind(), andlisten()are now separate steps.