Draft
Conversation
Replace 42 near-identical per-backend files (14 each for epoll, kqueue, select) with shared parameterized templates and per-backend traits + types files. Each reactor backend drops from 14 files to 3 (scheduler, traits, types). Net: +2,624 / -7,980 lines across 64 files. Per-backend traits structs capture all platform-specific behavior (socket creation flags, write/accept policies, fd configuration, SO_LINGER hooks, descriptor_state) in one location per backend. Shared templates (reactor_socket_finals, reactor_service_finals, reactor_stream _ops, reactor_datagram_ops, reactor_backend) provide a single implementation of socket/service/acceptor/op logic that all three backends instantiate via per-backend types files.
The old per-backend service classes had BOOST_COROSIO_DECL which ensures RTTI typeinfo symbols are exported with correct visibility. Without it, find_service<>() fails on FreeBSD (clang) because typeid comparisons across translation units don't match.
CRTP base classes used in a two-level hierarchy (base -> intermediate -> final) need protected constructors so the intermediate template can call them. Add NOLINTNEXTLINE suppression since the tidy check doesn't account for multi-level CRTP. Intermediate "finals" templates use private constructors with friend Derived as the check recommends.
close_socket() is not virtual in the acceptor implementation bases (tcp_acceptor::implementation, local_stream_acceptor::implementation). It was misleadingly placed inside a "Virtual method overrides" comment block. Move it out and document it as a non-virtual service helper.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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://232.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-15 21:57:30 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/boost/corosio/native/detail/reactor/reactor_datagram_socket.hpp (1)
690-692:⚠️ Potential issue | 🟠 MajorPass the write-direction flag for datagram
connect()too.
do_connect()also waits on writability, so leavingis_write_directionat its defaultfalsemeans select schedulers may not interrupt the currentselect()loop when this op is registered after the fd-set snapshot. That can stall nonblocking datagram connects until an unrelated event arrives.Suggested fix
this->register_op( op, this->desc_state_.connect_op, this->desc_state_.write_ready, - this->desc_state_.connect_cancel_pending); + this->desc_state_.connect_cancel_pending, true);Based on learnings: In the select backend, write/connect ops registered after the snapshot must notify the reactor so the next iteration picks them up in
write_fds.🤖 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 690 - 692, The datagram connect registration omits the write-direction flag so the select backend may not wake for connect operations; update the register_op call in the datagram connect path (the place calling this->register_op with this->desc_state_.connect_op, this->desc_state_.write_ready, this->desc_state_.connect_cancel_pending) to pass is_write_direction=true (matching do_connect’s writability wait) so the reactor/select backend will mark the op as write-directed and notify the reactor for correct write_fds handling.
🧹 Nitpick comments (1)
include/boost/corosio/native/detail/kqueue/kqueue_types.hpp (1)
17-28: Place the file overview block after the include block.The high-level
/* */overview is useful, but this guideline expects it after includes for implementation files with non-trivial logic.♻️ Suggested placement update
-/* Named per-backend types for the kqueue reactor. - - Each class is a final, named wrapper around the parameterized - reactor_*_impl templates. Forward-declarable from backend.hpp - so the concrete layer never pulls in platform headers. -*/ - `#include` <boost/corosio/detail/config.hpp> `#include` <boost/corosio/native/detail/kqueue/kqueue_traits.hpp> `#include` <boost/corosio/native/detail/kqueue/kqueue_scheduler.hpp> `#include` <boost/corosio/native/detail/reactor/reactor_backend.hpp> + +/* Named per-backend types for the kqueue reactor. + + Each class is a final, named wrapper around the parameterized + reactor_*_impl templates. Forward-declarable from backend.hpp + so the concrete layer never pulls in platform headers. +*/As per coding guidelines: “Files containing non-trivial implementation logic should include a
/* */block comment after the includes that provides a high-level overview…”.🤖 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_types.hpp` around lines 17 - 28, Move the top-level block comment that describes "Named per-backend types for the kqueue reactor" so it appears immediately after the include directives (the lines including kqueue_traits.hpp, kqueue_scheduler.hpp, and reactor_backend.hpp) instead of before them; update the file-level overview placement in kqueue_types.hpp so the /* ... */ overview follows the `#include` block and precedes the rest of the implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/boost/corosio/native/detail/reactor/reactor_stream_socket.hpp`:
- Around line 233-242: The current override of do_release_socket() clears
remote_endpoint_ after calling base_type::do_release_socket(), which breaks
established reactor release semantics; update the override in
reactor_stream_socket::do_release_socket() to call
base_type::do_release_socket() and return its fd without modifying
remote_endpoint_, and ensure only reactor_basic_socket resets local_endpoint_
(leave remote_endpoint_ intact) so the cached remote endpoint is preserved
across release().
In `@include/boost/corosio/native/detail/select/select_traits.hpp`:
- Around line 142-152: The current code treats a failed setsockopt(SO_NOSIGPIPE,
...) as fatal by closing new_fd and returning -1; instead make SO_NOSIGPIPE
best-effort in the select backend by ignoring errors from ::setsockopt for
SO_NOSIGPIPE — do not close new_fd or return on failure (optionally log/debug
the errno), and proceed normally after the setsockopt call; apply the same
change to the other SO_NOSIGPIPE branch(s) in select_traits.hpp that handle
new_fd/accept.
---
Outside diff comments:
In `@include/boost/corosio/native/detail/reactor/reactor_datagram_socket.hpp`:
- Around line 690-692: The datagram connect registration omits the
write-direction flag so the select backend may not wake for connect operations;
update the register_op call in the datagram connect path (the place calling
this->register_op with this->desc_state_.connect_op,
this->desc_state_.write_ready, this->desc_state_.connect_cancel_pending) to pass
is_write_direction=true (matching do_connect’s writability wait) so the
reactor/select backend will mark the op as write-directed and notify the reactor
for correct write_fds handling.
---
Nitpick comments:
In `@include/boost/corosio/native/detail/kqueue/kqueue_types.hpp`:
- Around line 17-28: Move the top-level block comment that describes "Named
per-backend types for the kqueue reactor" so it appears immediately after the
include directives (the lines including kqueue_traits.hpp, kqueue_scheduler.hpp,
and reactor_backend.hpp) instead of before them; update the file-level overview
placement in kqueue_types.hpp so the /* ... */ overview follows the `#include`
block and precedes the rest of the implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3da70766-115f-4d50-b2c3-5cd5aeafb10c
📒 Files selected for processing (64)
include/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_scheduler.hppinclude/boost/corosio/native/detail/epoll/epoll_tcp_acceptor.hppinclude/boost/corosio/native/detail/epoll/epoll_tcp_acceptor_service.hppinclude/boost/corosio/native/detail/epoll/epoll_tcp_service.hppinclude/boost/corosio/native/detail/epoll/epoll_tcp_socket.hppinclude/boost/corosio/native/detail/epoll/epoll_traits.hppinclude/boost/corosio/native/detail/epoll/epoll_types.hppinclude/boost/corosio/native/detail/epoll/epoll_udp_service.hppinclude/boost/corosio/native/detail/epoll/epoll_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_scheduler.hppinclude/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor.hppinclude/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_tcp_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_tcp_socket.hppinclude/boost/corosio/native/detail/kqueue/kqueue_traits.hppinclude/boost/corosio/native/detail/kqueue/kqueue_types.hppinclude/boost/corosio/native/detail/kqueue/kqueue_udp_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_udp_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_acceptor.hppinclude/boost/corosio/native/detail/reactor/reactor_acceptor_service.hppinclude/boost/corosio/native/detail/reactor/reactor_backend.hppinclude/boost/corosio/native/detail/reactor/reactor_basic_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_datagram_ops.hppinclude/boost/corosio/native/detail/reactor/reactor_datagram_socket.hppinclude/boost/corosio/native/detail/reactor/reactor_op_complete.hppinclude/boost/corosio/native/detail/reactor/reactor_service_finals.hppinclude/boost/corosio/native/detail/reactor/reactor_socket_finals.hppinclude/boost/corosio/native/detail/reactor/reactor_socket_service.hppinclude/boost/corosio/native/detail/reactor/reactor_stream_ops.hppinclude/boost/corosio/native/detail/reactor/reactor_stream_socket.hppinclude/boost/corosio/native/detail/select/select_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_scheduler.hppinclude/boost/corosio/native/detail/select/select_tcp_acceptor.hppinclude/boost/corosio/native/detail/select/select_tcp_acceptor_service.hppinclude/boost/corosio/native/detail/select/select_tcp_service.hppinclude/boost/corosio/native/detail/select/select_tcp_socket.hppinclude/boost/corosio/native/detail/select/select_traits.hppinclude/boost/corosio/native/detail/select/select_types.hppinclude/boost/corosio/native/detail/select/select_udp_service.hppinclude/boost/corosio/native/detail/select/select_udp_socket.hppinclude/boost/corosio/native/native_tcp_acceptor.hppinclude/boost/corosio/native/native_tcp_socket.hppinclude/boost/corosio/native/native_udp_socket.hppsrc/corosio/src/io_context.cpp
💤 Files with no reviewable changes (39)
- include/boost/corosio/native/detail/epoll/epoll_udp_socket.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_socket.hpp
- include/boost/corosio/native/detail/select/select_local_stream_acceptor.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_udp_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_udp_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor.hpp
- include/boost/corosio/native/detail/select/select_tcp_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_local_stream_acceptor.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_acceptor.hpp
- include/boost/corosio/native/detail/select/select_local_datagram_socket.hpp
- include/boost/corosio/native/detail/epoll/epoll_local_stream_acceptor.hpp
- include/boost/corosio/native/detail/select/select_tcp_acceptor.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_local_datagram_service.hpp
- include/boost/corosio/native/detail/select/select_local_datagram_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_socket.hpp
- include/boost/corosio/native/detail/epoll/epoll_local_stream_acceptor_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_local_datagram_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_op.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_local_stream_service.hpp
- include/boost/corosio/native/detail/epoll/epoll_local_stream_service.hpp
- include/boost/corosio/native/detail/select/select_tcp_socket.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_op.hpp
- include/boost/corosio/native/detail/select/select_local_stream_service.hpp
- include/boost/corosio/native/detail/select/select_local_stream_acceptor_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_local_stream_acceptor_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_tcp_acceptor_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_udp_socket.hpp
- include/boost/corosio/native/detail/epoll/epoll_local_datagram_socket.hpp
- include/boost/corosio/native/detail/select/select_udp_socket.hpp
- include/boost/corosio/native/detail/select/select_op.hpp
- include/boost/corosio/native/detail/epoll/epoll_tcp_acceptor_service.hpp
- include/boost/corosio/native/detail/select/select_tcp_acceptor_service.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_local_datagram_socket.hpp
- include/boost/corosio/native/detail/kqueue/kqueue_local_stream_socket.hpp
- include/boost/corosio/native/detail/select/select_local_stream_socket.hpp
- include/boost/corosio/native/detail/epoll/epoll_local_stream_socket.hpp
- include/boost/corosio/native/detail/select/select_udp_service.hpp
Comment on lines
+233
to
+242
| /** Release the socket without closing the fd. | ||
|
|
||
| Extends the base do_release_socket() to also reset | ||
| the remote endpoint. | ||
| */ | ||
| native_handle_type do_release_socket() noexcept | ||
| { | ||
| auto fd = base_type::do_release_socket(); | ||
| remote_endpoint_ = Endpoint{}; | ||
| return fd; |
There was a problem hiding this comment.
Preserve the cached remote endpoint across release().
This newly changes post-release() behavior by zeroing remote_endpoint_, even though base_type::do_release_socket() already transitions the object to the released state. That makes stream sockets diverge from the established reactor release semantics for no safety gain.
Suggested fix
native_handle_type do_release_socket() noexcept
{
auto fd = base_type::do_release_socket();
- remote_endpoint_ = Endpoint{};
return fd;
}Based on learnings: do_release_socket() should only reset local_endpoint_ in reactor_basic_socket and intentionally leave the derived remote_endpoint_ cache untouched.
📝 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
| /** Release the socket without closing the fd. | |
| Extends the base do_release_socket() to also reset | |
| the remote endpoint. | |
| */ | |
| native_handle_type do_release_socket() noexcept | |
| { | |
| auto fd = base_type::do_release_socket(); | |
| remote_endpoint_ = Endpoint{}; | |
| return fd; | |
| native_handle_type do_release_socket() noexcept | |
| { | |
| auto fd = base_type::do_release_socket(); | |
| return fd; | |
| } |
🤖 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 233 - 242, The current override of do_release_socket() clears
remote_endpoint_ after calling base_type::do_release_socket(), which breaks
established reactor release semantics; update the override in
reactor_stream_socket::do_release_socket() to call
base_type::do_release_socket() and return its fd without modifying
remote_endpoint_, and ensure only reactor_basic_socket resets local_endpoint_
(leave remote_endpoint_ intact) so the cached remote endpoint is preserved
across release().
Comment on lines
+142
to
+152
| #ifdef SO_NOSIGPIPE | ||
| int one = 1; | ||
| if (::setsockopt( | ||
| new_fd, SOL_SOCKET, SO_NOSIGPIPE, | ||
| &one, sizeof(one)) == -1) | ||
| { | ||
| int err = errno; | ||
| ::close(new_fd); | ||
| errno = err; | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
Keep SO_NOSIGPIPE best-effort on the select backend.
These branches now fail accept() / socket setup when SO_NOSIGPIPE cannot be applied. That is a behavior change from the existing select services, which intentionally treat this option as non-fatal. On affected platforms, sockets that previously opened or accepted successfully will now fail instead.
Suggested fix
`#ifdef` SO_NOSIGPIPE
int one = 1;
- if (::setsockopt(
- new_fd, SOL_SOCKET, SO_NOSIGPIPE,
- &one, sizeof(one)) == -1)
- {
- int err = errno;
- ::close(new_fd);
- errno = err;
- return -1;
- }
+ ::setsockopt(
+ new_fd, SOL_SOCKET, SO_NOSIGPIPE,
+ &one, sizeof(one));
`#endif` `#ifdef` SO_NOSIGPIPE
- // SO_NOSIGPIPE is the primary defense against SIGPIPE on
- // platforms that don't support MSG_NOSIGNAL. A silent failure
- // here would leave the fd vulnerable to crashing the process
- // on a closed-peer write, so propagate the error.
{
int one = 1;
- if (::setsockopt(
- fd, SOL_SOCKET, SO_NOSIGPIPE, &one, sizeof(one)) != 0)
- return make_err(errno);
+ ::setsockopt(
+ fd, SOL_SOCKET, SO_NOSIGPIPE, &one, sizeof(one));
}
`#endif`Based on learnings: In the select backend, setsockopt(SO_NOSIGPIPE, ...) failures are intentionally ignored for consistency with the existing select services.
Also applies to: 180-190
🤖 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_traits.hpp` around lines
142 - 152, The current code treats a failed setsockopt(SO_NOSIGPIPE, ...) as
fatal by closing new_fd and returning -1; instead make SO_NOSIGPIPE best-effort
in the select backend by ignoring errors from ::setsockopt for SO_NOSIGPIPE — do
not close new_fd or return on failure (optionally log/debug the errno), and
proceed normally after the setsockopt call; apply the same change to the other
SO_NOSIGPIPE branch(s) in select_traits.hpp that handle new_fd/accept.
|
GCOVR code coverage report https://232.corosio.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-04-15 22:05:19 UTC |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #232 +/- ##
========================================
Coverage 77.73% 77.73%
========================================
Files 96 96
Lines 7306 7306
Branches 1787 1787
========================================
Hits 5679 5679
Misses 1109 1109
Partials 518 518
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
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