Add portable select() reactor backend for POSIX platforms #87
Conversation
📝 WalkthroughWalkthroughThis pull request implements a select-based reactor backend as a fallback for POSIX platforms without epoll, replaces compile-time backend macros with runtime service polymorphism, and updates the IOCP backend to use new guard patterns. Additionally, CI workflows are enhanced with macOS support and WolfSSL configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Socket as socket::open()
participant ExecutionContext as execution_context
participant SocketService as socket_service
participant Impl as socket_impl
Client->>Socket: open(endpoint)
Socket->>ExecutionContext: find<socket_service>()
ExecutionContext-->>Socket: service instance (epoll/select/iocp)
Socket->>SocketService: create_impl()
SocketService-->>Socket: socket_impl&
Socket->>SocketService: open_socket(impl)
SocketService->>Impl: initialize (fd, etc.)
Impl-->>SocketService: success
SocketService-->>Socket: success
Socket-->>Client: open complete
sequenceDiagram
participant Op as select_op
participant Scheduler as select_scheduler
participant Reactor as reactor (select loop)
participant FDSet as fd_set
Op->>Scheduler: register_fd(fd, op, events)
Scheduler->>Scheduler: store op in fd_state
Scheduler->>Reactor: add fd to FDSet
Reactor->>Reactor: select(timeout)
Reactor->>FDSet: check readiness
FDSet-->>Reactor: fd ready
Reactor->>Op: dispatch(error_code, bytes)
Op->>Op: operator() - complete coroutine
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/corosio/src/acceptor.cpp`:
- Around line 13-20: Add a short file-level overview comment (C-style /* ... */)
immediately after the include block that explains the acceptor.cpp role and the
backend selection logic driven by the _WIN32 macro; reference the preprocessor
branches (`#if` defined(_WIN32) ... `#else` ... `#endif`), the included headers
("src/detail/iocp/sockets.hpp" and "src/detail/socket_service.hpp"), and that
this file wires platform-specific acceptor behavior into the overall acceptor
implementation (mention acceptor.cpp and boost/corosio/detail/except.hpp as
context). Keep the comment concise (one or two sentences) and placed directly
after the shown includes.
In `@src/corosio/src/detail/select/scheduler.hpp`:
- Around line 16-31: Add a concise file-level C-style block comment immediately
after the include block and before the namespace declaration in scheduler.hpp
that summarizes the purpose of this header (scheduler implementation and
integration with scheduler_op.hpp and timer_service.hpp), notes key
responsibilities (managing execution contexts, timers, and synchronization
primitives such as std::mutex/std::condition_variable), and any important
invariants or usage constraints; place it above "namespace
boost::corosio::detail" so readers see the overview before the declarations and
implementations that follow.
- Around line 97-105: Add a direct include of the header that defines FD_SETSIZE
to fix compile failures: in the scheduler.hpp header, add `#include`
<sys/select.h> near the other includes so the constexpr function max_fd() can
reference FD_SETSIZE; ensure the include is placed in the header (not only in a
.cpp) so the macro is available wherever scheduler.hpp is included.
In `@src/corosio/src/socket.cpp`:
- Around line 14-19: Add a concise file-level comment after the include block in
socket.cpp that explains the purpose of the file, specifically mentioning the
backend selection logic for Windows and POSIX platforms and the use of different
socket service headers based on the platform. This overview should clearly
communicate to maintainers that the file handles platform-specific socket
functionality through conditional includes.
🧹 Nitpick comments (5)
src/corosio/src/detail/iocp/scheduler.hpp (1)
38-41: Consider adding a class-level docstring forwin_scheduler.While this is internal detail code,
win_scheduleris a key component implementing the IOCP-based event loop. A brief docstring documenting its purpose and thread-safety guarantees would improve maintainability, similar to the documentation style used forwin_resolver_servicein this PR.src/corosio/src/detail/select/op.hpp (1)
324-327: Consider logging or handling fcntl F_SETFL failure.The code checks if
flags != -1before callingF_SETFL, but doesn't check the return value offcntl(new_fd, F_SETFL, ...). IfF_SETFLfails, the socket will remain blocking, which could cause unexpected behavior later.However, this matches the pattern used elsewhere in the codebase (e.g., in sockets.cpp), so this is a consistency observation rather than a blocker.
src/corosio/src/detail/select/scheduler.cpp (2)
591-596: Vector allocation in hot path.Creating a vector and reserving space on each reactor iteration introduces allocation overhead. For a portable fallback backend with known O(n) characteristics, this is acceptable, but could be optimized by reusing a member vector if performance becomes a concern.
673-681: Thread wakeup strategy is correct but could be simplified.The logic to wake idle threads uses a loop of
notify_one()calls. While correct, usingnotify_all()unconditionally would be simpler and the overhead difference is negligible given the select backend's inherent O(n) characteristics.src/corosio/src/detail/select/sockets.hpp (1)
10-30: Make the header self‑contained for coroutine/stop_token types.
This header usesstd::coroutine_handleandstd::stop_tokenin declarations; add the standard headers here to avoid reliance on transitive includes.♻️ Proposed include fix
`#include` <memory> `#include` <mutex> `#include` <unordered_map> +#include <coroutine> +#include <stop_token>
sgerbino
force-pushed
the
feature/select-reactor
branch
from
29ace1f to
927881c
Compare
|
An automated preview of the documentation is available at https://87.corosio.prtest3.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-01-28 03:18:22 UTC |
There was a problem hiding this comment.
Actionable comments posted: 5
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/io_context.hpp (1)
44-44: Documentation is stale: remove[future]tag.The comment says
select_context (select) [future]but this PR implementsselect_context, so the[future]tag should be removed to accurately reflect the current state.📝 Suggested fix
- - Other POSIX: `select_context` (select) [future] + - Other POSIX: `select_context` (select)
🤖 Fix all issues with AI agents
In `@include/boost/corosio/socket.hpp`:
- Around line 104-110: The cancel() documentation is contradictory: update the
comment on virtual void cancel() noexcept to make the cancellation guarantee
backend-dependent — e.g., change the opening sentence to indicate that when a
backend supports explicit cancellation (and will override cancel()), all
outstanding operations complete with operation_canceled, but the default
implementation does nothing for backends that don't support it; ensure the new
text references cancel() and the "default implementation" so readers know the
postcondition is not universal unless the backend overrides it.
In `@src/corosio/src/detail/epoll/sockets.cpp`:
- Around line 210-216: The epoll edge-case is caused by setting op.registered
(op.registered.store) after calling svc_.scheduler().register_fd, allowing
run_reactor() to see registered==false and drop an event; change the
registration protocol to a tri-state or handshake: add a "REGISTERING" state and
set op.registered to REGISTERING before calling
svc_.scheduler().register_fd(fd_, &op, ...), then after register_fd returns set
it to REGISTERED (using atomic operations); update run_reactor() to treat
REGISTERING as already registered (or wait/handle it) so events delivered
between register_fd and the final store are not ignored; apply the same change
to the connect/read/write/accept paths and use compare_exchange/atomic ordering
to avoid races.
In `@src/corosio/src/detail/select/op.hpp`:
- Around line 323-337: The code currently ignores failures from the fcntl calls
when configuring new_fd; update the sequence around F_GETFL, F_SETFL and F_SETFD
to check each return value, on any failure capture errno, close(new_fd), and
call complete(errno, 0) instead of continuing; only set accepted_fd, call
create_peer/create_peer(..) and complete(0, 0) after all fcntl calls succeed
(use the existing symbols new_fd, accepted_fd, create_peer, peer_impl, and
complete()).
In `@src/corosio/src/detail/select/sockets.cpp`:
- Around line 677-707: The fcntl calls that set O_NONBLOCK and FD_CLOEXEC on the
accepted socket (the accepted variable) currently ignore errors; update the
accept-handling path to check the return values of both ::fcntl(accepted,
F_SETFL, ...) and ::fcntl(accepted, F_SETFD, ...), capture errno on failure, and
treat a failure as a fatal accept: close(accepted), set op.accepted_fd = -1 and
op.peer_impl = nullptr, call op.complete(errno, 0) (or appropriate error code),
set op.impl_ptr = shared_from_this(), svc_.post(&op) and return; only proceed to
create select_socket_impl and post a successful op when both fcntl calls
succeed.
In `@src/corosio/src/select_context.cpp`:
- Around line 10-18: Add a brief /* ... */ file-level overview comment
immediately after the include block in select_context.cpp describing the file's
purpose (wiring scheduler creation and service installation for non-Windows
builds, referencing the scheduler and sockets helpers in src/detail/select), so
readers can quickly understand the role of this translation unit; place the
comment right after the last `#include` and keep it concise (one or two sentences)
mentioning scheduler creation and service installation.
🧹 Nitpick comments (8)
src/corosio/src/detail/epoll/sockets.cpp (1)
11-25: Add a file-level implementation overview comment.This file has non-trivial logic but doesn’t include the required
/* */overview after the includes.
As per coding guidelines, please add a short block comment summarizing the epoll socket/acceptor service flow and cancellation model.✍️ Suggested doc block
+#/* + Overview: epoll socket/acceptor service implementation, op lifecycle, + cancellation handling, and shared-state ownership. +*/src/corosio/src/detail/select/scheduler.cpp (1)
592-596: Consider reserving or reusing the fd vector to reduce allocations.A
std::vectoris allocated on every reactor iteration to avoid iterator invalidation. While correct, for high-throughput scenarios this could add allocation overhead. Consider makingfds_to_checka member variable that's cleared and reused, or using a small-buffer-optimized container.src/corosio/src/detail/select/sockets.cpp (2)
1-25: Missing implementation overview comment.Per coding guidelines, files with non-trivial implementation logic should include a block comment after the includes providing a high-level overview. Consider adding a comment similar to the one in
scheduler.cppthat explains the operation flow, cancellation handling, and service architecture.
1038-1039: SO_REUSEADDR failure silently ignored.The return value of
setsockopt(..., SO_REUSEADDR, ...)is intentionally not checked, which is common for "best-effort" options. Consider adding a brief comment to clarify this is intentional.📝 Suggested documentation
int reuse = 1; - ::setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &reuse, sizeof(reuse)); + // Best-effort: failure to set SO_REUSEADDR is not fatal + (void)::setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &reuse, sizeof(reuse));src/corosio/src/detail/select/sockets.hpp (4)
191-191: Redundant forward declaration.
select_acceptor_serviceis already forward-declared at line 67.🧹 Suggested fix
//------------------------------------------------------------------------------ -class select_acceptor_service; /** Shared state between select socket and acceptor services.
142-144: Public operation state members.The
conn_,rd_, andwr_members are public. If this is intentional (e.g., for direct access by completion handlers), consider adding a brief comment. Otherwise, these could be made private with appropriate friend declarations.
198-216: Consider usingstructfor all-public type.
select_sockets_stateis declared asclassbut has only public members. Usingstructwould be more idiomatic and make the intent clearer.📝 Suggested change
-class select_sockets_state +struct select_sockets_state { -public: explicit select_sockets_state(select_scheduler& sched) noexcept
218-247: Consider documenting thread-safety guarantees.Per coding guidelines, classes intended for concurrent use should document their thread-safety guarantees. The presence of
mutex_inselect_sockets_statesuggests concurrent access is expected. A brief note on which operations are thread-safe would help users of these services.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #87 +/- ##
===========================================
- Coverage 84.99% 81.72% -3.28%
===========================================
Files 43 49 +6
Lines 3266 4454 +1188
===========================================
+ Hits 2776 3640 +864
- Misses 490 814 +324
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
sgerbino
force-pushed
the
feature/select-reactor
branch
from
927881c to
ec0e6bb
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 (2)
src/corosio/src/detail/iocp/wsa_init.cpp (1)
13-18: Add required file overview block comment after includes.This file contains non-trivial initialization logic (refcounted WSAStartup/WSACleanup) and should include a brief /* */ high‑level overview per guidelines.
📝 Suggested comment placement
`#include` <boost/corosio/detail/except.hpp> +/* + * High-level overview: + * win_wsa_init maintains a process-wide refcount and performs WSAStartup on the + * first instance and WSACleanup on the last, using interlocked ops for safety. + */ namespace boost::corosio::detail {include/boost/corosio/io_context.hpp (1)
43-44: Documentation is slightly stale.The comment on line 44 says
select_context (select) [future]but select_context is now implemented and actively used as the fallback for BSD/macOS and other POSIX platforms. Consider updating to reflect the current state.📝 Suggested fix
- - Other POSIX: `select_context` (select) [future] + - Other POSIX: `select_context` (select)
🤖 Fix all issues with AI agents
In `@src/corosio/src/detail/epoll/sockets.cpp`:
- Around line 210-223: The CAS failure after setting op.registered to
registration_state::registering can leave the fd registered with no active op if
cancel() won the race; after the compare_exchange_strong on op.registered
(checking registration_state::registering -> registration_state::registered)
fails, call the reactor unregister method (the counterpart to
svc_.scheduler().register_fd, e.g. svc_.scheduler().unregister_fd or similar) to
remove the fd registration before returning, and apply the same fix to the
read/write/accept paths that use op.registered and svc_.scheduler().register_fd
so they also unregister when the CAS fails.
In `@src/corosio/src/detail/select/scheduler.cpp`:
- Around line 384-401: After updating registered_fds_ and max_fd_ inside
select_scheduler::register_fd (protected by mutex_), wake the reactor so a
thread currently blocked in select() sees the new fd; i.e., after setting
state.read_op/state.write_op and updating max_fd_, invoke the reactor
wake/interrupt mechanism (for example call the class's interrupt() or write to
the reactor wake pipe) while still holding or immediately after releasing mutex_
so the select loop re-evaluates fd_sets and includes the newly registered fd.
Ensure this interrupt call is placed in register_fd right after the max_fd_
update and references the select_scheduler instance.
In `@src/corosio/src/detail/select/sockets.cpp`:
- Around line 13-26: Add a short file‑level block comment immediately after the
include block in src/detail/select/sockets.cpp that summarizes the purpose and
responsibilities of this translation unit (e.g., implements socket operations
and helpers used by the select-based backend, such as socket creation, option
setup, and error conversion utilities used in namespace boost::corosio::detail).
Keep it brief (2–4 lines) and mention the key responsibilities and intended
audience (maintainers), then leave the existing includes and namespace
declaration unchanged so the comment sits directly before namespace
boost::corosio::detail.
- Around line 701-769: After accepting a socket, ensure its descriptor is less
than FD_SETSIZE and immediately reject (close the fd, set op.accepted_fd = -1,
op.peer_impl = nullptr, call op.complete(...) and svc_.post(&op)) if it is >=
FD_SETSIZE to avoid later failures in register_fd() and select-based monitoring;
add this check right after int accepted = ::accept(...) and before any fcntl()
calls or before constructing select_socket_impl/peer_impl, using the same
cleanup/error-path pattern as the existing fcntl failure branches.
🧹 Nitpick comments (4)
include/boost/corosio/acceptor.hpp (1)
295-303: Clarify the comment about backends without explicit cancellation support.The default no-op is never actually called in practice—all backends (epoll, select, iocp) have working
cancel()implementations. The comment suggesting "backends that don't support explicit cancellation" is misleading. Consider updating the docstring to more accurately reflect that all supported backends do implement cancellation, or remove this comment entirely.Additionally, there's a design inconsistency:
epoll_acceptor_implandselect_acceptor_implboth declarevoid cancel() noexcept override;(following the virtual override pattern), butwin_acceptor_impldoes not. For consistency and clarity,win_acceptor_implshould also declarecancel() noexcept override;even though the Windows dispatch path (acceptor::cancel()) directly callsget_internal()->cancel().src/corosio/src/detail/epoll/sockets.cpp (1)
11-17: Add a post-include implementation overview comment.This file contains substantial reactor/service logic but lacks a brief /* */ overview right after the includes.
As per coding guidelines, please add a high-level implementation summary comment.
src/corosio/src/detail/epoll/sockets.hpp (2)
210-210: Remove the duplicate forward declaration.
class epoll_acceptor_service;is declared earlier already.
228-231: Update the overview comment to match current containers.The overview still references a “vector” but the implementation now uses
unordered_map+intrusive_list.As per coding guidelines, please refresh the overview to avoid staleness.
sgerbino
force-pushed
the
feature/select-reactor
branch
from
ec0e6bb to
24caddf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/corosio/src/detail/epoll/sockets.cpp`:
- Around line 1061-1066: The scheduler() accessor currently dereferences state_
without a null check; update epoll_acceptor_service::scheduler() to mirror other
methods by either asserting state_ or performing if (!state_) { /* handle or
throw */ } before returning state_->sched_, e.g., use an assert or throw to make
the invariant explicit; reference epoll_acceptor_service::scheduler(), state_,
and epoll_scheduler to locate and fix the code.
🧹 Nitpick comments (4)
src/corosio/src/detail/iocp/timers_thread.cpp (1)
11-15: Add a high-level implementation overview comment after the includes.
This file contains non-trivial threading/timer logic but lacks the required/* */overview block right after the includes. As per coding guidelines, please add a brief description of how the timer thread and wakeup logic work and any tricky behavior.include/boost/corosio/io_context.hpp (1)
82-86: select_context as fallback for non-Linux POSIX is appropriate.Using
select_contextfor BSD/macOS and other POSIX systems provides a working portable backend while kqueue support is pending. The documentation comment on line 44 should be updated to reflect that select_context is now available (not "[future]").📝 Suggested documentation update
- Windows: `iocp_context` (I/O Completion Ports) - Linux: `epoll_context` (epoll) - BSD/macOS: `kqueue_context` (kqueue) [future] - - Other POSIX: `select_context` (select) [future] + - BSD/macOS: `select_context` (select) [fallback until kqueue implemented] + - Other POSIX: `select_context` (select)src/corosio/src/detail/select/sockets.cpp (1)
384-433: Verify handling of sendmsg() returning 0.If
sendmsg()returns 0 (no bytes written) anderrnois notEAGAIN/EWOULDBLOCK, the code falls through to line 430 witherrno ? errno : EIO. This is defensive, but for non-empty buffers, a return of 0 is unusual. Consider whether this warrants logging or a more specific error code.src/corosio/src/detail/select/sockets.hpp (1)
155-188: Consider usingnative_handle_typefor acceptor'snative_handle().The
select_socket_impl::native_handle()returnsnative_handle_type(line 111), butselect_acceptor_impl::native_handle()returnsint(line 174). For API consistency, consider using the same type alias. This is a minor stylistic concern.
sgerbino
force-pushed
the
feature/select-reactor
branch
2 times, most recently
from
2db8ec1 to
3eaf2bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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/io_context.hpp (1)
41-47: Update documentation to reflect current implementation.The docstring lists
select_contextas "[future]" on line 44, but this PR implements it. Consider updating the documentation:📝 Suggested documentation update
This is a type alias for the platform's default I/O backend: - Windows: `iocp_context` (I/O Completion Ports) - Linux: `epoll_context` (epoll) - BSD/macOS: `kqueue_context` (kqueue) [future] - - Other POSIX: `select_context` (select) [future] + - Other POSIX: `select_context` (select)src/corosio/src/detail/epoll/op.hpp (1)
52-151: Add<cstdint>forstd::uint8_t.The
registration_stateenum at line 102 usesstd::uint8_t, but<cstdint>is not explicitly included. While transitive includes may provide it, explicitly including<cstdint>is required for portability and best practices.Suggested include
`#include` <atomic> +#include <cstdint> `#include` <cstddef>
🤖 Fix all issues with AI agents
In `@src/corosio/src/detail/epoll/sockets.cpp`:
- Around line 11-18: Add a brief file-level /* ... */ overview comment
immediately after the includes in src/detail/epoll/sockets.cpp describing the
purpose and high-level responsibilities of this translation unit (e.g.,
"epoll-based socket reactor/service implementation for Linux, handling event
registration, readiness callbacks, and error conversion via
endpoint_convert/make_err"). Reference the relationship to the corresponding
header sockets.hpp and the helpers endpoint_convert.hpp and make_err.hpp, and
mention that the file contains reactor/service logic for epoll event loop and
socket lifecycle so maintainers know what to expect.
🧹 Nitpick comments (3)
.github/workflows/ci.yml (1)
538-582: Verify WolfSSL library filename on macOS triplets.
libwolfssl.ais hard-coded, but vcpkg’s default macOS triplets often produce.dylib. If the linkage is dynamic, this will break CMake/WolfSSL detection on macOS. Consider resolving the actual file at runtime.💡 Suggested robust resolution
- # Output for CMake steps (to pass WolfSSL paths explicitly) - echo "wolfssl_include=${vcpkg_installed}/include" >> $GITHUB_OUTPUT - echo "wolfssl_library=${vcpkg_installed}/lib/libwolfssl.a" >> $GITHUB_OUTPUT + # Output for CMake steps (to pass WolfSSL paths explicitly) + echo "wolfssl_include=${vcpkg_installed}/include" >> $GITHUB_OUTPUT + if [ -f "${vcpkg_installed}/lib/libwolfssl.dylib" ]; then + wolfssl_lib="${vcpkg_installed}/lib/libwolfssl.dylib" + else + wolfssl_lib="${vcpkg_installed}/lib/libwolfssl.a" + fi + echo "wolfssl_library=${wolfssl_lib}" >> $GITHUB_OUTPUTsrc/corosio/src/detail/select/sockets.hpp (2)
191-191: Remove duplicate forward declaration.
class select_acceptor_service;is declared twice (Line 67 and Line 191). This is harmless but redundant.♻️ Suggested cleanup
-class select_acceptor_service;
214-215: Defensive null-check forsocket_svc_is already in place; consider nullifying during shutdown.The code already safely handles the raw pointer through null-checks before dereference (lines 705–708). Additionally,
select_acceptor_servicerequiressocket_serviceto be installed first (enforced at construction, throwing if absent), and both services share the samestate_as ashared_ptr, ensuring the shared state stays alive as long as either service holds a reference. However, for extra safety during shutdown, consider explicitly nullifyingsocket_svc_inselect_socket_service::shutdown()to prevent any accidental use if the service is accessed after destruction.Also, remove the duplicate forward declaration of
select_acceptor_serviceat line 191 (already declared at line 67).
sgerbino
force-pushed
the
feature/select-reactor
branch
from
3eaf2bd to
01d1ef2
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/corosio/src/detail/epoll/op.hpp (1)
95-150: Add explicit#include <cstdint>forstd::uint8_t.The enum
registration_stateusesstd::uint8_tas its underlying type. Per C++ standard,std::uint8_tis defined only in<cstdint>. The current includes (<atomic>,<cstddef>,<memory>,<optional>,<stop_token>) do not guarantee<cstdint>is available. Some toolchains will fail to compile without an explicit include.Proposed fix
`#include` <atomic> +#include <cstdint> `#include` <cstddef>src/corosio/src/detail/epoll/sockets.hpp (1)
31-80: Refresh the overview comment to match the new shared-state design.The block still refers to vector ownership and a single epoll_sockets owner, but the implementation now uses shared state and unordered_map with split services. Please update the overview to avoid misleading maintainers.
✍️ Suggested comment refresh
- epoll_sockets owns all socket impls. destroy_impl() removes the shared_ptr - from the vector, but the impl may survive if ops still hold impl_ptr refs. - shutdown() closes all sockets and clears the vectors; any in-flight ops + epoll_socket_service owns socket impls and epoll_acceptor_service owns acceptor + impls via shared state (unordered_map). destroy_impl() removes the shared_ptr + from the map, but the impl may survive if ops still hold impl_ptr refs. + shutdown() closes all sockets/acceptors and clears the maps; any in-flight opsAs per coding guidelines, please keep the file-level overview in sync with the implementation.
🤖 Fix all issues with AI agents
In `@CMakeLists.txt`:
- Around line 265-268: The CMake file unconditionally links CoreFoundation and
Security for target_link_libraries(boost_corosio_wolfssl ...) on APPLE even
though the WolfSSL Apple native cert validation macros may not be enabled;
either add target_compile_definitions(boost_corosio_wolfssl PUBLIC
WOLFSSL_APPLE_NATIVE_CERT_VALIDATION WOLFSSL_SYS_CA_CERTS) to enable the feature
alongside those framework links, or gate the target_link_libraries call on those
compile definitions being set (e.g., check a cached/option var or use
if(WOLFSSL_APPLE_NATIVE_CERT_VALIDATION AND WOLFSSL_SYS_CA_CERTS) before
linking); additionally ensure you enforce or document a minimum WolfSSL version
>= 5.8.2 when enabling WOLFSSL_APPLE_NATIVE_CERT_VALIDATION to avoid
CVE-2025-7395.
In `@src/corosio/src/detail/select/scheduler.cpp`:
- Around line 164-181: select_scheduler::shutdown currently clears
completed_ops_, sets shutdown_ and notifies wakeup_event_ but may leave a
reactor thread blocked; after draining completed_ops_ and releasing the mutex
(i.e., after the final lock.unlock() or just before returning from shutdown()),
explicitly interrupt the reactor so a thread blocked in the reactor wait will
wake—call the reactor interrupt API (e.g., reactor_.interrupt() or
reactor->interrupt(), checking for a valid reactor handle) and then notify_all
on wakeup_event_ and set outstanding_work_.store(0) to ensure shutdown cannot
hang.
In `@src/corosio/src/detail/select/sockets.hpp`:
- Around line 193-216: The raw pointer select_sockets_state::socket_svc_ is
captured by the create_peer lambda and can dangle if the select_socket_service
is destroyed before the deferred lambda runs; change the ownership/validation:
replace socket_svc_ with a std::weak_ptr<select_socket_service> (and ensure
select_socket_service is held by a shared_ptr) and have create_peer lock the
weak_ptr before use, or alternatively clear socket_svc_ in
select_socket_service's destructor and ensure pending operations are
completed/cancelled before destruction; update all references in create_peer and
any accept paths (see select_sockets_state::socket_svc_ and the create_peer
lambda invocation) to use the weak_ptr lock+null-check pattern or the
destructor-clearing approach to avoid use-after-free.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
538-582: Ensure vcpkg triplet matches the macOS runner architecture.This step picks the triplet from
uname -m, butrun-vcpkgexecutes earlier. If vcpkg’s default triplet ever diverges (e.g., arm64 runner but x64-osx install), this block will fail to find the install dir. Consider exportingVCPKG_DEFAULT_TRIPLETfor macOS beforeSetup vcpkgso install and lookup stay aligned.Suggested pre-vcpkg step
+ - name: Set vcpkg triplet (macOS) + if: runner.os == 'macOS' + shell: bash + run: | + if [[ "$(uname -m)" == "arm64" ]]; then + echo "VCPKG_DEFAULT_TRIPLET=arm64-osx" >> $GITHUB_ENV + else + echo "VCPKG_DEFAULT_TRIPLET=x64-osx" >> $GITHUB_ENV + fi
| # WolfSSL on macOS uses Apple Security framework for certificate validation | ||
| if (APPLE) | ||
| target_link_libraries(boost_corosio_wolfssl PUBLIC "-framework CoreFoundation" "-framework Security") | ||
| endif () |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
WolfSSL macOS certificate validation frameworks required CoreFoundation Security
💡 Result:
On macOS, wolfSSL only needs Apple’s CoreFoundation and Security frameworks if you enable Apple native certificate validation / system trust store support (i.e., using wolfSSL_CTX_load_system_CA_certs() with the system trust APIs).
What to enable in wolfSSL
- Define
WOLFSSL_SYS_CA_CERTSandWOLFSSL_APPLE_NATIVE_CERT_VALIDATION(wolfSSL uses Apple Security framework trust APIs when this is enabled). (wolfssl.com) - On macOS,
WOLFSSL_APPLE_NATIVE_CERT_VALIDATIONis not enabled by default (unlike some other Apple targets), so you must enable it explicitly if you want native verification. (wolfssl.com)
What to link
- Link these Apple frameworks:
-framework CoreFoundation-framework Security(wolfssl.com)
Security note (important)
- If you use this configuration on Apple platforms, update wolfSSL to v5.8.2 or later due to CVE-2025-7395 affecting builds with
WOLFSSL_SYS_CA_CERTS+WOLFSSL_APPLE_NATIVE_CERT_VALIDATION. (wolfssl.com)
Citations:
- 1: https://www.wolfssl.com/wolfssl-extends-support-for-system-ca-certificates-to-apple-devices/?utm_source=openai
- 2: https://www.wolfssl.com/wolfssl-extends-support-for-system-ca-certificates-to-apple-devices/?utm_source=openai
- 3: https://www.wolfssl.com/wolfssl-extends-support-for-system-ca-certificates-to-apple-devices/?utm_source=openai
- 4: https://www.wolfssl.com/vulnerability-disclosure-wolfssl-cve-2025-7395/?utm_source=openai
🏁 Script executed:
# Search for any other WolfSSL-related configuration in CMakeLists.txt
rg -n "wolfssl|WolfSSL" CMakeLists.txt -A 3 -B 1Repository: cppalliance/corosio
Length of output: 2479
🏁 Script executed:
# Check if there are any build or configuration files related to WolfSSL
fd -e cmake -e txt -e md | xargs rg -l "WolfSSL\|wolfssl" 2>/dev/null | head -20Repository: cppalliance/corosio
Length of output: 45
🏁 Script executed:
# Search for WOLFSSL_APPLE_NATIVE_CERT_VALIDATION and WOLFSSL_SYS_CA_CERTS in CMakeLists.txt
rg "WOLFSSL_APPLE_NATIVE_CERT_VALIDATION|WOLFSSL_SYS_CA_CERTS" CMakeLists.txtRepository: cppalliance/corosio
Length of output: 45
🏁 Script executed:
# Search entire codebase for these defines
rg "WOLFSSL_APPLE_NATIVE_CERT_VALIDATION|WOLFSSL_SYS_CA_CERTS" --type cmake --type cpp --type hppRepository: cppalliance/corosio
Length of output: 90
🏁 Script executed:
# Check if there's any WolfSSL target configuration or find_package handling
rg -n "target_compile_definitions.*WOLFSSL|WolfSSL::WolfSSL" CMakeLists.txt -A 2 -B 2Repository: cppalliance/corosio
Length of output: 1161
Framework linking should be conditional on WolfSSL's Apple native certificate validation feature being enabled.
According to WolfSSL documentation, CoreFoundation and Security frameworks are only required if WOLFSSL_APPLE_NATIVE_CERT_VALIDATION and WOLFSSL_SYS_CA_CERTS are explicitly enabled—these are not enabled by default on macOS. The CMakeLists.txt unconditionally links these frameworks on all macOS builds (lines 266–268), but no corresponding compile definitions to enable the Apple native cert validation feature are present in this file.
Either:
- Add
target_compile_definitions(boost_corosio_wolfssl PUBLIC WOLFSSL_APPLE_NATIVE_CERT_VALIDATION WOLFSSL_SYS_CA_CERTS)to enable the feature, or - Make the framework linking conditional on whether these features are actually enabled.
Critical: If Apple native cert validation is enabled, ensure the project uses WolfSSL v5.8.2 or later to mitigate CVE-2025-7395, which affects builds with this configuration.
🤖 Prompt for AI Agents
In `@CMakeLists.txt` around lines 265 - 268, The CMake file unconditionally links
CoreFoundation and Security for target_link_libraries(boost_corosio_wolfssl ...)
on APPLE even though the WolfSSL Apple native cert validation macros may not be
enabled; either add target_compile_definitions(boost_corosio_wolfssl PUBLIC
WOLFSSL_APPLE_NATIVE_CERT_VALIDATION WOLFSSL_SYS_CA_CERTS) to enable the feature
alongside those framework links, or gate the target_link_libraries call on those
compile definitions being set (e.g., check a cached/option var or use
if(WOLFSSL_APPLE_NATIVE_CERT_VALIDATION AND WOLFSSL_SYS_CA_CERTS) before
linking); additionally ensure you enforce or document a minimum WolfSSL version
>= 5.8.2 when enabling WOLFSSL_APPLE_NATIVE_CERT_VALIDATION to avoid
CVE-2025-7395.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/corosio/src/detail/select/sockets.hpp`:
- Around line 119-143: Add a shutdown-state flag to select_socket_impl
(analogous to empty_buffer_read) and set it inside
select_socket_impl::shutdown(socket::shutdown_type) when shutdown_send (or both
send/rdwr as appropriate) is requested; then modify
select_socket_impl::read_some so that when ::readv() returns 0 it checks that
shutdown flag and calls op.complete(0, capy::cond::eof) (rather than
op.complete(0, 0)) to report explicit EOF after a local shutdown_send; ensure
all references use the existing select_socket_impl, shutdown(), read_some(),
shutdown_send and op.complete symbols so the new flag is consistently
read/written and used to decide the returned error code.
In `@src/corosio/src/detail/socket_service.hpp`:
- Around line 57-85: The abstract base class socket_service lacks a virtual
destructor which can cause undefined behavior when derived instances are deleted
via a socket_service pointer; add a virtual destructor (e.g. virtual
~socket_service() noexcept = default;) to the socket_service class (alongside
the existing protected default constructor) so derived destructors are invoked
correctly.
- Around line 97-131: The acceptor_service abstract base class is missing a
virtual destructor which can cause undefined behavior when deleting derived
instances via base pointers; add a virtual destructor to acceptor_service (e.g.,
declare virtual ~acceptor_service() = default;) matching the visibility used in
socket_service so derived implementations are properly destroyed.
🧹 Nitpick comments (4)
src/corosio/src/detail/iocp/timers.cpp (1)
32-32: Consider adding a trailing comment to#endiffor consistency.The header files in this PR use
#endif // _WIN32for clarity. This#endifcould follow the same convention.Suggested change
-#endif +#endif // _WIN32include/boost/corosio/select_context.hpp (1)
43-45: Clarify thread safety guarantee for shared objects.The thread safety note states shared objects are safe "if using a concurrency hint greater than 1", but this wording may mislead users into thinking concurrent operations on a single context are automatically safe. The concurrency hint typically enables internal synchronization for the scheduler, but users should still understand what operations can safely overlap.
Consider clarifying:
`@par` Thread Safety Distinct objects: Safe.@n - Shared objects: Safe, if using a concurrency hint greater than 1. + Shared objects: Safe for concurrent calls to run() from multiple threads + when concurrency_hint > 1. Individual I/O objects (sockets, acceptors) + must not have concurrent operations.src/corosio/src/detail/select/scheduler.cpp (1)
598-603: Consider reserving vector capacity upfront.The vector is reserved with
registered_fds_.size()but then iterated to copy keys. This is correct, but for very high fd counts, consider whether the allocation could be avoided by iterating twice (once to collect ready fds).src/corosio/src/detail/epoll/sockets.hpp (1)
210-240: Drop the redundant forward declaration.Line 210 repeats
class epoll_acceptor_service;already declared at Line 85; removing it reduces clutter.
| system::error_code shutdown(socket::shutdown_type what) noexcept override; | ||
|
|
||
| native_handle_type native_handle() const noexcept override { return fd_; } | ||
|
|
||
| // Socket options | ||
| system::error_code set_no_delay(bool value) noexcept override; | ||
| bool no_delay(system::error_code& ec) const noexcept override; | ||
|
|
||
| system::error_code set_keep_alive(bool value) noexcept override; | ||
| bool keep_alive(system::error_code& ec) const noexcept override; | ||
|
|
||
| system::error_code set_receive_buffer_size(int size) noexcept override; | ||
| int receive_buffer_size(system::error_code& ec) const noexcept override; | ||
|
|
||
| system::error_code set_send_buffer_size(int size) noexcept override; | ||
| int send_buffer_size(system::error_code& ec) const noexcept override; | ||
|
|
||
| system::error_code set_linger(bool enabled, int timeout) noexcept override; | ||
| socket::linger_options linger(system::error_code& ec) const noexcept override; | ||
|
|
||
| endpoint local_endpoint() const noexcept override { return local_endpoint_; } | ||
| endpoint remote_endpoint() const noexcept override { return remote_endpoint_; } | ||
| bool is_open() const noexcept { return fd_ >= 0; } | ||
| void cancel() noexcept override; | ||
| void cancel_single_op(select_op& op) noexcept; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find src/corosio/src/detail/select -name "*.cpp" -o -name "*.hpp"Repository: cppalliance/corosio
Length of output: 271
🏁 Script executed:
rg -n "select_socket_impl::shutdown|select_socket_impl::read_some" src/corosio/src/detail/select/ -A 10Repository: cppalliance/corosio
Length of output: 45
🏁 Script executed:
rg -n "cond::eof|shutdown_send" src/corosio/src/detail/select/ -B 2 -A 2Repository: cppalliance/corosio
Length of output: 488
🏁 Script executed:
# Check if select_socket_impl implements shutdown properly
rg -n "void shutdown|error_code shutdown" src/corosio/src/detail/select/sockets.cpp -A 15Repository: cppalliance/corosio
Length of output: 45
🏁 Script executed:
# Read the shutdown implementation context in sockets.cpp
sed -n '430,480p' src/corosio/src/detail/select/sockets.cppRepository: cppalliance/corosio
Length of output: 1319
🏁 Script executed:
# Search for read_some or read implementations
rg -n "read_some|::read\(" src/corosio/src/detail/select/sockets.cpp -A 20 | head -100Repository: cppalliance/corosio
Length of output: 712
🏁 Script executed:
# Look for EOF or error code handling in read operations
rg -n "eof|cond::" src/corosio/src/detail/select/sockets.cpp -B 2 -A 2Repository: cppalliance/corosio
Length of output: 3637
🏁 Script executed:
# Get file size to understand scope
wc -l src/corosio/src/detail/select/sockets.cppRepository: cppalliance/corosio
Length of output: 109
🏁 Script executed:
# Read the complete read_some function
sed -n '250,320p' src/corosio/src/detail/select/sockets.cppRepository: cppalliance/corosio
Length of output: 2316
🏁 Script executed:
# Search for select_read_op structure and how it handles EOF
rg -n "select_read_op" src/corosio/src/detail/select/ -A 5 | head -80Repository: cppalliance/corosio
Length of output: 1498
🏁 Script executed:
# Check if there's any EOF/zero-byte handling after shutdown in read
rg -n "read.*shutdown|shutdown.*read" src/corosio/src/detail/select/sockets.cpp -iRepository: cppalliance/corosio
Length of output: 45
🏁 Script executed:
# Look at op.hpp to understand how operations complete
sed -n '1,100p' src/corosio/src/detail/select/op.hppRepository: cppalliance/corosio
Length of output: 3402
🏁 Script executed:
# Search for cond::eof in the entire codebase
rg -n "cond::eof" --type cpp --type hppRepository: cppalliance/corosio
Length of output: 90
🏁 Script executed:
# Check the epoll backend for comparison on how it handles EOF after shutdown
rg -n "epoll_socket_impl::read_some" src/corosio/src/detail/epoll/sockets.cpp -A 30 | head -50Repository: cppalliance/corosio
Length of output: 45
🏁 Script executed:
# Search for shutdown handling in epoll
rg -n "epoll_socket_impl::shutdown" src/corosio/src/detail/epoll/sockets.cpp -A 15Repository: cppalliance/corosio
Length of output: 45
🏁 Script executed:
# Check if there's a shutdown state tracked anywhere in select backend
rg -n "shutdown|SHUT_" src/corosio/src/detail/select/sockets.hppRepository: cppalliance/corosio
Length of output: 289
🏁 Script executed:
# Search for eof without namespace
rg -n "eof" src/corosio/src/detail/select/sockets.cpp -iRepository: cppalliance/corosio
Length of output: 1204
🏁 Script executed:
# Check the socket interface/header for shutdown contract
fd src/corosio/include -name "*.hpp" -o -name "*.h" 2>/dev/null | head -20Repository: cppalliance/corosio
Length of output: 45
🏁 Script executed:
# Look at the learning source - test file
rg -n "shutdown_send|cond::eof" test/unit/socket.cpp -B 3 -A 3Repository: cppalliance/corosio
Length of output: 1181
🏁 Script executed:
# Search for how error codes are created/returned in select
rg -n "make_err|error_code" src/corosio/src/detail/select/sockets.cpp | head -30Repository: cppalliance/corosio
Length of output: 1156
🏁 Script executed:
# Find capy::cond::eof definition
rg -n "namespace cond|cond::eof" --type cppRepository: cppalliance/corosio
Length of output: 905
🏁 Script executed:
# Check op.complete() signature to understand error passing
rg -n "void complete\(" src/corosio/src/detail/select/op.hpp -A 5Repository: cppalliance/corosio
Length of output: 209
🏁 Script executed:
# Verify if select backend's read_some tracks shutdown state
rg -n "shutdown_send|shutdown_receive|shutdown_both" src/corosio/src/detail/select/sockets.hpp -B 2 -A 2Repository: cppalliance/corosio
Length of output: 45
🏁 Script executed:
# Check if there's shutdown state tracking in select_socket_impl
sed -n '1,200p' src/corosio/src/detail/select/sockets.hpp | grep -n "shutdown\|private:"Repository: cppalliance/corosio
Length of output: 247
🏁 Script executed:
# Find the definition of capy::cond::eof to understand the error code value
rg -n "enum.*cond|eof.*=" -B 2 -A 2 | grep -A 5 -B 5 "eof"Repository: cppalliance/corosio
Length of output: 9218
🏁 Script executed:
# Check the include/boost/corosio/socket.hpp for the full contract documentation
sed -n '350,365p' include/boost/corosio/socket.hppRepository: cppalliance/corosio
Length of output: 589
Track shutdown state and return explicit EOF error code on reads after shutdown_send.
The select backend's read_some implementation returns op.complete(0, 0) when ::readv() yields zero bytes, signaling EOF as error_code(0) rather than the documented capy::cond::eof. Per the public API contract (documented in include/boost/corosio/socket.hpp), reads following shutdown_send must report an explicit EOF error code. The select backend currently lacks shutdown state tracking to distinguish shutdown-initiated EOF from peer-initiated EOF. Add a shutdown state flag to select_socket_impl (similar to the existing empty_buffer_read pattern), track it in shutdown(), and return the appropriate error code in read_some when EOF occurs after shutdown_send.
🤖 Prompt for AI Agents
In `@src/corosio/src/detail/select/sockets.hpp` around lines 119 - 143, Add a
shutdown-state flag to select_socket_impl (analogous to empty_buffer_read) and
set it inside select_socket_impl::shutdown(socket::shutdown_type) when
shutdown_send (or both send/rdwr as appropriate) is requested; then modify
select_socket_impl::read_some so that when ::readv() returns 0 it checks that
shutdown flag and calls op.complete(0, capy::cond::eof) (rather than
op.complete(0, 0)) to report explicit EOF after a local shutdown_send; ensure
all references use the existing select_socket_impl, shutdown(), read_some(),
shutdown_send and op.complete symbols so the new flag is consistently
read/written and used to decide the returned error code.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the select backend's shutdown implementation
echo "=== Checking shutdown implementation ==="
sed -n '430,480p' src/corosio/src/detail/select/sockets.cpp
echo -e "\n=== Checking read_some implementation around EOF handling ==="
sed -n '250,320p' src/corosio/src/detail/select/sockets.cpp
echo -e "\n=== Search for shutdown state tracking in select_socket_impl ==="
grep -n "shutdown" src/corosio/src/detail/select/sockets.hpp | head -20
echo -e "\n=== Check if capy::cond::eof is used in select backend ==="
grep -n "cond::eof" src/corosio/src/detail/select/sockets.cpp
Length of output: 3967
🏁 Script executed:
#!/bin/bash
# Check if epoll backend tracks shutdown state and returns cond::eof
echo "=== Check epoll backend for shutdown state tracking ==="
grep -n "shutdown_" src/corosio/src/detail/epoll/sockets.hpp | head -20
echo -e "\n=== Check epoll read_some for cond::eof usage ==="
grep -n "cond::eof" src/corosio/src/detail/epoll/sockets.cpp
echo -e "\n=== Check the test that documents this contract ==="
sed -n '703,720p' test/unit/socket.cpp
Length of output: 306
🏁 Script executed:
#!/bin/bash
# Find the socket test file
echo "=== Find socket test files ==="
fd -t f socket test/ 2>/dev/null || find test -name "*socket*" -type f 2>/dev/null
echo -e "\n=== Search for shutdown_send test in all test files ==="
rg -n "shutdown_send|shutdown\(shutdown_send\)" test/ -A 3 -B 1 2>/dev/null | head -50
echo -e "\n=== Check if cond::eof is used anywhere in tests ==="
rg -n "cond::eof" test/ 2>/dev/null | head -20
Length of output: 141
🏁 Script executed:
#!/bin/bash
# Search entire codebase for shutdown_send and cond::eof
echo "=== Search for shutdown_send usage anywhere ==="
rg -n "shutdown_send" --type cpp --type hpp -C 2 | head -50
echo -e "\n=== Search for cond::eof definition or usage ==="
rg -n "cond::eof" --type cpp --type hpp | head -30
echo -e "\n=== Check socket.hpp for documented shutdown contract ==="
rg -n "shutdown.*contract|postcondition|subsequent read" include/boost/corosio/socket.hpp -B 2 -A 2
Length of output: 467
🏁 Script executed:
#!/bin/bash
# Check the full shutdown documentation context
echo "=== Full shutdown documentation from socket.hpp ==="
sed -n '340,380p' include/boost/corosio/socket.hpp
echo -e "\n=== Check select backend's read_some for any EOF handling ==="
sed -n '285,310p' src/corosio/src/detail/select/sockets.cpp
echo -e "\n=== Search for cond::eof anywhere in codebase ==="
find . -name "*.cpp" -o -name "*.h" -o -name "*.hpp" | xargs grep -l "cond::eof" 2>/dev/null
Length of output: 2643
🏁 Script executed:
#!/bin/bash
# Check what op.complete does with its parameters
echo "=== Check op.complete signature and implementation ==="
rg -n "void complete\(" src/corosio/src/detail/select/op.hpp -A 10
echo -e "\n=== Check how cond::eof is used in io_stream.hpp ==="
sed -n '1,100p' include/boost/corosio/io_stream.hpp | grep -B 5 -A 5 "cond::eof"
echo -e "\n=== Find where capy::cond::eof is defined ==="
find . -path ./build -prune -o -name "*.hpp" -exec grep -l "namespace cond" {} \; 2>/dev/null | head -5
Length of output: 1117
@sgerbino, I can see why you're questioning this. Let me share what I found:
The documentation states (socket.hpp:353-355):
"When the peer shuts down their send direction (sends a FIN), subsequent read operations will complete with
capy::cond::eof."
The current select backend implementation (sockets.cpp:295-300):
if (n == 0)
{
op.complete(0, 0); // passes errno=0, bytes=0
op.impl_ptr = shared_from_this();
svc_.post(&op);
return;
}This passes error code 0 (no error) when readv() returns 0, not capy::cond::eof.
However, I cannot find where op.errn (the int error number) gets converted to the final system::error_code that's returned to users. There might be a translation layer that converts error 0 + bytes 0 into capy::cond::eof that I'm not seeing in the code I examined.
Could you clarify:
- Is there error code translation happening somewhere that converts errno 0 with 0 bytes into
capy::cond::eof? - Or does the current behavior (returning error code 0 for EOF) match the intended design?
The learning I have references test code from PR #40 that doesn't exist in this PR, so it's possible the contract has changed or the learning is outdated.
🧠 Learnings used
Learnt from: vinniefalco
Repo: cppalliance/corosio PR: 40
File: test/unit/socket.cpp:703-706
Timestamp: 2026-01-21T00:31:28.371Z
Learning: In boost::corosio, `socket::shutdown(shutdown_send)` has a contract that requires subsequent read operations to return an explicit EOF error code (`capy::cond::eof`), not just zero bytes. This is a stricter postcondition than `socket::close()`, which may signal EOF either through an error code or by returning zero bytes.
Learnt from: CR
Repo: cppalliance/corosio PR: 0
File: .cursor/rules/portable-headers.mdc:0-0
Timestamp: 2026-01-22T19:54:36.343Z
Learning: Applies to include/boost/corosio/**/*.{h,hpp} : Public headers in `include/boost/corosio/` and all subdirectories MUST NOT include platform-specific headers (Windows headers like `<windows.h>`, `<WinSock2.h>`, `<Ws2tcpip.h>`, `<MSWSock.h>` or Unix/POSIX headers like `<sys/socket.h>`, `<netinet/in.h>`, `<arpa/inet.h>`, `<unistd.h>`, `<sys/types.h>`)
bc37ec9 to
77cfa39
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/corosio/src/detail/win/signals.cpp (1)
474-500: Fix work-tracking counter mismatch in signal completion path.The work-tracking implementation has a critical bug due to mismatched counter types:
start_wait()(line 530) callssched_.on_work_started()to track signal wait as executor work (counter that stops scheduler when zero)cancel_wait()(line 498) correctly callssched_.on_work_finished()to balance the executor work counter- However, when a signal arrives normally, the signal_op callback (line 164) calls
service->work_finished(), which delegates tosched_.work_finished()— the I/O work counter (which does not stop the scheduler)These are two separate counters with different semantics. The scheduler's executor work counter remains incremented after signal completion, potentially preventing graceful shutdown or causing the scheduler to hang.
The fix is to ensure signal completion uses
on_work_finished()(executor counter) instead ofwork_finished()(I/O counter) to properly balance theon_work_started()call fromstart_wait().
🤖 Fix all issues with AI agents
In `@include/boost/corosio/acceptor.hpp`:
- Around line 296-303: Update the documentation for the virtual method cancel()
to make the "all outstanding operations complete with operation_canceled error"
guarantee conditional on backends that override cancel; keep the current default
implementation virtual void cancel() noexcept {} as a no-op but clarify in the
comment that only backends which provide an override will cancel outstanding
operations and produce operation_canceled errors, and mention that backends that
do not override cancel will not perform explicit cancellation.
🧹 Nitpick comments (5)
src/corosio/src/detail/iocp/timers_nt.cpp (1)
13-14: Consider adding a high-level implementation overview.This file contains non-trivial logic for integrating timers with IOCP via undocumented NT wait completion packet APIs. A brief block comment after the includes explaining the approach (why NT APIs are used, how the wait packet associates with the waitable timer, etc.) would help future maintainers understand the design.
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."src/corosio/src/acceptor.cpp (1)
87-92: Consider unifying cancel() via the virtual hook to drop the platform branch.Now that win_acceptor_impl overrides cancel(), you can just call get().cancel() for all platforms.
♻️ Proposed simplification
void acceptor:: cancel() { if (!impl_) return; -#if defined(_WIN32) - static_cast<detail::win_acceptor_impl*>(impl_)->get_internal()->cancel(); -#else - // acceptor_impl has virtual cancel() method get().cancel(); -#endif }src/corosio/src/detail/epoll/sockets.cpp (1)
1053-1059: Consider adding a comment about dynamic_cast safety.The
dynamic_cast<epoll_socket_service*>is correct for returning the concrete type, but a brief comment noting that this returnsnullptrif a different backend's socket service is installed would clarify the intent.💡 Suggested comment
epoll_socket_service* epoll_acceptor_service:: socket_service() const noexcept { + // Returns nullptr if a non-epoll socket service is installed auto* svc = ctx_.find_service<detail::socket_service>(); return svc ? dynamic_cast<epoll_socket_service*>(svc) : nullptr; }src/corosio/src/detail/select/scheduler.cpp (1)
599-698: Consider processing only a bounded number of completions per reactor tick.The reactor processes all ready fds in a single iteration. Under high load with many ready fds, this could starve other threads waiting on the condition variable. Consider limiting completions per tick to improve fairness.
This is a minor optimization concern and can be deferred if not needed for current use cases.
src/corosio/src/detail/select/sockets.cpp (1)
953-994: FD_SETSIZE check should occur before fcntl calls for efficiency.In
open_socket, the FD_SETSIZE check (lines 986-990) happens after the fcntl calls (lines 965-983). If the fd exceeds the limit, the fcntl work was wasted. Consider moving the check earlier.♻️ Suggested improvement
int fd = ::socket(AF_INET, SOCK_STREAM, 0); if (fd < 0) return make_err(errno); + // Check fd is within select() limits before configuring + if (fd >= FD_SETSIZE) + { + ::close(fd); + return make_err(EMFILE); + } + // Set non-blocking and close-on-exec int flags = ::fcntl(fd, F_GETFL, 0); // ... fcntl calls ... - - // Check fd is within select() limits - if (fd >= FD_SETSIZE) - { - ::close(fd); - return make_err(EMFILE); // Too many open files - }
sgerbino
force-pushed
the
feature/select-reactor
branch
from
77cfa39 to
792e22e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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/io_context.hpp (1)
34-47: Update backend list—select_context is no longer “future.”
The doc still labels select as “future,” but it’s now the default/fallback on several POSIX targets. Please update the bullet list (and examples) to avoid misleading users.📘 Suggested doc tweak
- - BSD/macOS: `kqueue_context` (kqueue) [future] - - Other POSIX: `select_context` (select) [future] + - BSD/macOS: `select_context` (select) fallback until kqueue_context exists + - Other POSIX: `select_context` (select) @@ - For explicit backend selection, use the concrete context types - directly (e.g., `epoll_context`, `iocp_context`). + For explicit backend selection, use the concrete context types + directly (e.g., `epoll_context`, `select_context`, `iocp_context`).Also applies to: 82-86
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 111-139: The macOS matrix entries for the Apple-Clang jobs (the
entries with name "Apple-Clang (macOS 15, asan+ubsan): C++20" and "Apple-Clang
(macOS 14): C++20" / runs-on "macos-15" and "macos-14") don’t set a vcpkg
triplet so vcpkg defaults to x64; add vcpkg-triplet: "arm64-osx" to both of
those job matrix entries so the vcpkg install and later macOS path lookup use
the same arm64-osx triplet.
🧹 Nitpick comments (6)
src/corosio/src/detail/iocp/wsa_init.cpp (1)
13-17: Add a brief implementation overview comment after includes.This file contains non-trivial initialization/cleanup logic and should include a short
/* */high-level overview block to explain the lifecycle and error-handling rationale.
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.src/corosio/src/detail/epoll/op.hpp (1)
31-35: Add<cstdint>forstd::uint8_t.The enum
registration_stateat line 102 usesstd::uint8_t, which requires<cstdint>. This explicit include prevents builds from breaking on stricter toolchains that don't provide transitive includes.Proposed change
`#include` <atomic> `#include` <cstddef> +#include <cstdint> `#include` <memory>Also applies to: 102-107
src/corosio/src/detail/epoll/sockets.cpp (1)
1053-1059: Consider caching the socket service pointer.The
dynamic_caston everysocket_service()call has runtime overhead. Since the socket service is typically installed once at context creation, consider caching the result after the first successful lookup.However, the current implementation is acceptable given this is only called during accept operations.
src/corosio/src/detail/epoll/sockets.hpp (2)
264-264: Document the state_ non-null invariant.The
scheduler()accessor dereferencesstate_without a null check. While the constructor always initializesstate_, a brief comment documenting this invariant would clarify the assumption for maintainers.- epoll_scheduler& scheduler() const noexcept { return state_->sched_; } + /// Returns the scheduler. Precondition: service is constructed (state_ is never null). + epoll_scheduler& scheduler() const noexcept { return state_->sched_; }
296-296: Same applies to acceptor service's scheduler() accessor.Consider adding the same invariant documentation for consistency.
src/corosio/src/detail/select/sockets.hpp (1)
202-203: Remove redundant forward declaration.
select_acceptor_serviceis already forward-declared on line 77.//------------------------------------------------------------------------------ -class select_acceptor_service; - /** State for select socket service. */
| - compiler: "apple-clang" | ||
| version: "*" | ||
| cxxstd: "20" | ||
| latest-cxxstd: "20" | ||
| cxx: "clang++" | ||
| cc: "clang" | ||
| runs-on: "macos-15" | ||
| b2-toolset: "clang" | ||
| is-latest: true | ||
| name: "Apple-Clang (macOS 15, asan+ubsan): C++20" | ||
| shared: true | ||
| build-type: "RelWithDebInfo" | ||
| asan: true | ||
| ubsan: true | ||
| cxxflags: "-fexperimental-library" | ||
|
|
||
| - compiler: "apple-clang" | ||
| version: "*" | ||
| cxxstd: "20" | ||
| latest-cxxstd: "20" | ||
| cxx: "clang++" | ||
| cc: "clang" | ||
| runs-on: "macos-14" | ||
| b2-toolset: "clang" | ||
| name: "Apple-Clang (macOS 14): C++20" | ||
| shared: true | ||
| build-type: "Release" | ||
| build-cmake: true | ||
| cxxflags: "-fexperimental-library" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
vcpkg default triplet Apple silicon arm64 auto-detection
💡 Result:
On Apple Silicon (arm64) macOS, vcpkg’s behavior is split into target triplet vs host triplet:
-
Default target triplet (what
vcpkg install zlibuses if you don’t specify one): vcpkg’s documented platform default for macOS is stillx64-osxunless you override it withVCPKG_DEFAULT_TRIPLET(or pass--triplet). [1]- So there is no automatic “arm64-osx” default-target auto-detection in the general CLI default described by the docs; you typically must set it yourself.
-
Default host triplet (used for host tools / host dependencies): vcpkg chooses the host triplet based on the host OS + CPU architecture (so on Apple Silicon this will be
arm64-osx, unless overridden). [2]
How to force arm64 on Apple Silicon:
- One-off (CLI):
vcpkg install <port> --triplet arm64-osx[1] - Global default: set environment variable
VCPKG_DEFAULT_TRIPLET=arm64-osx[1] - CMake integration: set
VCPKG_TARGET_TRIPLET=arm64-osx(so CMake drives the selection rather than relying on the CLI default). [1]
Sources:
[1] vcpkg Triplets (default triplet remarks; macOS default x64-osx) https://learn.microsoft.com/en-us/vcpkg/concepts/triplets
[2] vcpkg Host dependencies (host triplet chosen from host arch/OS; override options) https://learn.microsoft.com/en-us/vcpkg/users/host-dependencies
🏁 Script executed:
# Inspect the workflow file to understand vcpkg configuration
cd .github/workflows && wc -l ci.ymlRepository: cppalliance/corosio
Length of output: 73
🏁 Script executed:
# Check the macOS matrix entries (lines 111-139) and vcpkg-related steps
sed -n '80,150p' .github/workflows/ci.ymlRepository: cppalliance/corosio
Length of output: 2237
🏁 Script executed:
# Search for vcpkg-triplet and VCPKG_DEFAULT_TRIPLET in the entire workflow
grep -n "vcpkg-triplet\|VCPKG_DEFAULT_TRIPLET" .github/workflows/ci.ymlRepository: cppalliance/corosio
Length of output: 756
🏁 Script executed:
# Find the run-vcpkg step and surrounding context
grep -n "run-vcpkg" .github/workflows/ci.yml | head -20Repository: cppalliance/corosio
Length of output: 340
Ensure vcpkg triplet is set for macOS runners before vcpkg install.
run-vcpkg executes before you infer the triplet later. On macOS 14/15 (arm64), vcpkg's default triplet is x64-osx, which installs into a different directory and causes the later macOS path step to fail. Add vcpkg-triplet: "arm64-osx" to both macOS matrix entries so installation and lookup match.
Suggested adjustment
- compiler: "apple-clang"
version: "*"
cxxstd: "20"
latest-cxxstd: "20"
cxx: "clang++"
cc: "clang"
runs-on: "macos-15"
b2-toolset: "clang"
is-latest: true
name: "Apple-Clang (macOS 15, asan+ubsan): C++20"
shared: true
build-type: "RelWithDebInfo"
asan: true
ubsan: true
cxxflags: "-fexperimental-library"
+ vcpkg-triplet: "arm64-osx"
- compiler: "apple-clang"
version: "*"
cxxstd: "20"
latest-cxxstd: "20"
cxx: "clang++"
cc: "clang"
runs-on: "macos-14"
b2-toolset: "clang"
name: "Apple-Clang (macOS 14): C++20"
shared: true
build-type: "Release"
build-cmake: true
cxxflags: "-fexperimental-library"
+ vcpkg-triplet: "arm64-osx"📝 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.
| - compiler: "apple-clang" | |
| version: "*" | |
| cxxstd: "20" | |
| latest-cxxstd: "20" | |
| cxx: "clang++" | |
| cc: "clang" | |
| runs-on: "macos-15" | |
| b2-toolset: "clang" | |
| is-latest: true | |
| name: "Apple-Clang (macOS 15, asan+ubsan): C++20" | |
| shared: true | |
| build-type: "RelWithDebInfo" | |
| asan: true | |
| ubsan: true | |
| cxxflags: "-fexperimental-library" | |
| - compiler: "apple-clang" | |
| version: "*" | |
| cxxstd: "20" | |
| latest-cxxstd: "20" | |
| cxx: "clang++" | |
| cc: "clang" | |
| runs-on: "macos-14" | |
| b2-toolset: "clang" | |
| name: "Apple-Clang (macOS 14): C++20" | |
| shared: true | |
| build-type: "Release" | |
| build-cmake: true | |
| cxxflags: "-fexperimental-library" | |
| - compiler: "apple-clang" | |
| version: "*" | |
| cxxstd: "20" | |
| latest-cxxstd: "20" | |
| cxx: "clang++" | |
| cc: "clang" | |
| runs-on: "macos-15" | |
| b2-toolset: "clang" | |
| is-latest: true | |
| name: "Apple-Clang (macOS 15, asan+ubsan): C++20" | |
| shared: true | |
| build-type: "RelWithDebInfo" | |
| asan: true | |
| ubsan: true | |
| cxxflags: "-fexperimental-library" | |
| vcpkg-triplet: "arm64-osx" | |
| - compiler: "apple-clang" | |
| version: "*" | |
| cxxstd: "20" | |
| latest-cxxstd: "20" | |
| cxx: "clang++" | |
| cc: "clang" | |
| runs-on: "macos-14" | |
| b2-toolset: "clang" | |
| name: "Apple-Clang (macOS 14): C++20" | |
| shared: true | |
| build-type: "Release" | |
| build-cmake: true | |
| cxxflags: "-fexperimental-library" | |
| vcpkg-triplet: "arm64-osx" |
🤖 Prompt for AI Agents
In @.github/workflows/ci.yml around lines 111 - 139, The macOS matrix entries
for the Apple-Clang jobs (the entries with name "Apple-Clang (macOS 15,
asan+ubsan): C++20" and "Apple-Clang (macOS 14): C++20" / runs-on "macos-15" and
"macos-14") don’t set a vcpkg triplet so vcpkg defaults to x64; add
vcpkg-triplet: "arm64-osx" to both of those job matrix entries so the vcpkg
install and later macOS path lookup use the same arm64-osx triplet.
Implement a select()-based I/O multiplexing backend that provides runtime backend selection on platforms where multiple backends are available. On Linux, users can now choose between epoll_context (best performance) and select_context (portable) at runtime. Architecture changes: - Add abstract socket_service and acceptor_service base classes to enable runtime polymorphism between epoll and select backends - Split epoll socket/acceptor services to avoid diamond inheritance with execution_context::service - Update socket.cpp and acceptor.cpp to use abstract service interfaces - Delete config_backend.hpp entirely: use platform macros directly (_WIN32, __linux__, !defined(_WIN32)) instead of custom macros Key design decisions: - Self-pipe pattern for reactor interruption (portable, no eventfd) - FD_SETSIZE limit enforced with graceful error (no undefined behavior) - Single-reactor thread model matching epoll for consistent behavior - Level-triggered semantics (inherent to select) Test updates: - Add multi-backend test parameterization to acceptor, socket, timer, and signal_set tests using template pattern - Tests run for both io_context and select_context on POSIX platforms - Fix testWriteAfterPeerClose to write more data for macOS compatibility (macOS has larger socket buffers, needs more data to trigger EPIPE) Benchmark updates: - Add --backend option to io_context_bench for runtime backend selection - Add --list option to show available backends on the platform - Benchmarks can now compare epoll vs select performance on Linux CI updates: - Enable macOS builds using select backend (kqueue planned for future) - Add vcpkg.json and vcpkg paths setup for macOS builds - Add -fexperimental-library flag for std::stop_token support on macOS Build fixes: - Link CoreFoundation and Security frameworks for WolfSSL on macOS (vcpkg-built WolfSSL uses Apple certificate APIs)
| }; | ||
|
|
||
| capy::coro h; | ||
| capy::executor_ref d; |
There was a problem hiding this comment.
should be
capy::executor_ref ex;
| if (bytes_out) | ||
| *bytes_out = bytes_transferred; | ||
|
|
||
| auto saved_d = d; |
There was a problem hiding this comment.
This needs a comment that we are moving things to the stack because we are about to destroy the frame. Also I prefer seeing the whole type at sensitive places like this:
executor_ref ex( std::move(ex_) );
| acceptor_impl_ = impl; | ||
|
|
||
| if (token.stop_possible()) | ||
| stop_cb.emplace(token, canceller{this}); |
There was a problem hiding this comment.
why not just always store the token and always check it
optionals are an eyesore
sgerbino
force-pushed
the
feature/select-reactor
branch
from
792e22e to
b994557
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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/io_context.hpp (1)
40-45: Sync the backend list with the new POSIX default.The alias now selects
select_contexton BSD/macOS and other POSIX platforms, but the bullet list still marks select as “future” and lists kqueue as the default.📝 Suggested doc update
- - BSD/macOS: `kqueue_context` (kqueue) [future] - - Other POSIX: `select_context` (select) [future] + - BSD/macOS: `select_context` (select) until `kqueue_context` is available + - Other POSIX: `select_context` (select)Also applies to: 82-86
🤖 Fix all issues with AI agents
In `@src/corosio/src/epoll_context.cpp`:
- Around line 10-17: Add a brief file-level block comment (/* ... */)
immediately after the include directives in epoll_context.cpp that summarizes
the implementation: explain that this translation unit wires up the epoll
scheduler (reference to scheduler creation in src/detail/epoll/scheduler.hpp)
and installs the epoll service and socket helpers (reference to sockets
utilities in src/detail/epoll/sockets.hpp), describe the high-level flow of
scheduler creation and service installation, and note any threading or lifecycle
assumptions the implementation expects.
🧹 Nitpick comments (2)
src/corosio/src/detail/iocp/timers_thread.cpp (1)
11-15: Add a brief implementation overview block after the includes.This file has non-trivial threading/timer logic but lacks the required post-include overview comment.
♻️ Proposed addition
`#include` "src/detail/iocp/timers_thread.hpp" `#include` "src/detail/iocp/windows.hpp" +/* + Windows waitable-timer thread + ----------------------------- + Uses a waitable timer + IOCP repost to wake the scheduler thread. + The timer thread exits on shutdown_ and is woken via immediate timer fire. +*/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.”
src/corosio/src/acceptor.cpp (1)
87-92: Consider using virtualcancel()on Windows for consistency.The Windows path uses an explicit
static_castwhile POSIX uses the virtualcancel(). Sincewin_acceptor_implnow hascancel() noexcept override, you could unify both paths:void acceptor:: cancel() { if (!impl_) return; get().cancel(); }This mirrors the
close()method which uses virtualrelease()on both platforms.
| #include <boost/corosio/epoll_context.hpp> | ||
|
|
||
| #include "src/detail/config_backend.hpp" | ||
|
|
||
| #if defined(BOOST_COROSIO_BACKEND_EPOLL) | ||
| #if defined(__linux__) | ||
|
|
||
| #include "src/detail/epoll/scheduler.hpp" | ||
| #include "src/detail/epoll/sockets.hpp" | ||
|
|
There was a problem hiding this comment.
Add a brief file-level implementation overview.
This translation unit wires scheduler creation and service installation but lacks the required /* */ overview comment after the includes.
📌 Suggested update
`#include` <boost/corosio/epoll_context.hpp>
+/*
+ epoll_context implementation overview
+ - Installs the epoll scheduler
+ - Registers epoll socket/acceptor services for runtime backend selection
+*/
+
`#if` defined(__linux__)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.
📝 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.
| #include <boost/corosio/epoll_context.hpp> | |
| #include "src/detail/config_backend.hpp" | |
| #if defined(BOOST_COROSIO_BACKEND_EPOLL) | |
| #if defined(__linux__) | |
| #include "src/detail/epoll/scheduler.hpp" | |
| #include "src/detail/epoll/sockets.hpp" | |
| `#include` <boost/corosio/epoll_context.hpp> | |
| /* | |
| epoll_context implementation overview | |
| - Installs the epoll scheduler | |
| - Registers epoll socket/acceptor services for runtime backend selection | |
| */ | |
| `#if` defined(__linux__) | |
| `#include` "src/detail/epoll/scheduler.hpp" | |
| `#include` "src/detail/epoll/sockets.hpp" |
🤖 Prompt for AI Agents
In `@src/corosio/src/epoll_context.cpp` around lines 10 - 17, Add a brief
file-level block comment (/* ... */) immediately after the include directives in
epoll_context.cpp that summarizes the implementation: explain that this
translation unit wires up the epoll scheduler (reference to scheduler creation
in src/detail/epoll/scheduler.hpp) and installs the epoll service and socket
helpers (reference to sockets utilities in src/detail/epoll/sockets.hpp),
describe the high-level flow of scheduler creation and service installation, and
note any threading or lifecycle assumptions the implementation expects.
3 participants
Implement a select()-based I/O multiplexing backend that provides runtime backend selection on platforms where multiple backends are available. On Linux, users can now choose between epoll_context (best performance) and select_context (portable) at runtime.
Architecture changes:
Key design decisions:
Bug fixes (applied to both select and epoll backends):
Test updates:
Benchmark updates:
CI updates:
Build fixes:
Resolves #61.
Summary by CodeRabbit
New Features
cancel()method to socket and acceptor classes for operation cancellation.Improvements
✏️ Tip: You can customize this high-level summary in your review settings.