Add clang-tidy CI, reformat codebase, and split acceptor_service#145
Add clang-tidy CI, reformat codebase, and split acceptor_service#145sgerbino wants to merge 3 commits intocppalliance:developfrom
Conversation
📝 WalkthroughWalkthroughAdds clang-format/clang-tidy configs and CI clang-tidy runs; introduces a new acceptor_service abstraction and updates backends; many formatting-only refactors across headers/impls; several destructor/override, cancel, scheduler/timer, and signal handling adjustments; removes an inline resume helper. Changes
Sequence Diagram(s)sequenceDiagram
participant Acceptor as tcp_acceptor
participant Service as acceptor_service
participant Backend as Scheduler/Backend (epoll/kqueue/select/iocp)
participant OS as Kernel Socket API
Acceptor->>Service: open_acceptor(impl, endpoint, backlog)
Service->>Backend: open_acceptor(impl, endpoint, backlog)
Backend->>OS: socket(), bind(), listen()
OS-->>Backend: fd / error
Backend-->>Service: std::error_code
Service-->>Acceptor: std::error_code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
An automated preview of the documentation is available at https://145.corosio.prtest3.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-02-16 03:30:46 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/corosio/src/detail/select/op.hpp (1)
340-395:⚠️ Potential issue | 🟡 MinorReplace
sockaddr_inwithsockaddr_storageto support both IPv4 and IPv6 addresses.The accept operation currently uses
sockaddr_in, which truncates peer addresses to IPv4. If the listener accepts an IPv6 connection, the peer address will be corrupted. The kqueue backend already usessockaddr_storagefor this; adopt the same pattern here.Proposed fix
- sockaddr_in addr{}; - socklen_t addrlen = sizeof(addr); + sockaddr_storage addr{}; + socklen_t addrlen = sizeof(addr);src/corosio/src/detail/epoll/acceptors.cpp (1)
355-402:⚠️ Potential issue | 🟠 Major
open_acceptoris hardcoded to IPv4 across all backends — IPv6 endpoints will silently misbehave.The socket is created with
AF_INET(line 362) and the endpoint is converted viato_sockaddr_in(line 369), regardless of the address family ofep. This same pattern appears identically in the kqueue and select backends.The
endpointclass explicitly supports both IPv4 and IPv6 viais_v4()andis_v6()methods, andendpoint_convert.hppprovides bothto_sockaddr_in()andto_sockaddr_in6()with documented preconditions: "Must be IPv4 (is_v4() == true)" and "Must be IPv6 (is_v6() == true)" respectively. Passing an IPv6 endpoint violates this precondition and causes silent data corruption at thebindcall.Branch on
ep.is_v6()to selectAF_INET6/to_sockaddr_in6(), or add a precondition check that rejects IPv6 endpoints with a clear error code.src/corosio/src/detail/posix/resolver_service.cpp (1)
790-815:⚠️ Potential issue | 🟠 MajorLifecycle inconsistency in
shutdown(): clearresolver_ptrs_immediately, unlike all other I/O services.All socket and acceptor services (epoll, kqueue, select) defer clearing their ptr maps to the destructor, with explicit reasoning: the scheduler shuts down after
shutdown()and drains queued ops, callingdestroy()on each. Releasing shared_ptrs before scheduler shutdown risks use-after-free if an op'sdestroy()is invoked after the impl is freed.The resolver service clears
resolver_ptrs_at line 807, before waiting for worker threads to finish at line 813. While detached worker threads holdshared_ptr<posix_resolver_impl>preventing immediate destruction, after a thread exits and releases its reference, the impl could be destroyed while its embeddedop_remains queued in the scheduler.Follow the pattern established by socket/acceptor services: defer clearing
resolver_ptrs_to the destructor, or document why the immediate release is safe relative to scheduler draining.src/corosio/src/detail/iocp/signals.hpp (1)
22-24:⚠️ Potential issue | 🟡 MinorDuplicate
#include <system_error>.
<system_error>is included on both line 22 and line 24. Remove one.Proposed fix
`#include` <system_error> -#include <system_error> - `#include` "src/detail/iocp/mutex.hpp"src/corosio/src/detail/select/acceptors.cpp (1)
116-125:⚠️ Potential issue | 🟡 MinorPre-existing: potential null dereference of
acceptor_impl_in cleanup path.At line 119,
acceptor_impl_is dereferenced viastatic_castwithout a null check. Unlike the success path (line 56 checksif (acceptor_impl_)), this cleanup path only checksif (peer_impl). Ifpeer_implis non-null whileacceptor_impl_is null, this would be a null-pointer dereference. This appears to be pre-existing rather than introduced by this PR, but worth hardening since you're touching this code.Proposed guard
if (peer_impl) { - auto* socket_svc_cleanup = - static_cast<select_acceptor_impl*>(acceptor_impl_) - ->service() - .socket_service(); - if (socket_svc_cleanup) - socket_svc_cleanup->destroy(peer_impl); - peer_impl = nullptr; + if (acceptor_impl_) + { + auto* socket_svc_cleanup = + static_cast<select_acceptor_impl*>(acceptor_impl_) + ->service() + .socket_service(); + if (socket_svc_cleanup) + socket_svc_cleanup->destroy(peer_impl); + } + peer_impl = nullptr; }
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 719-726: The pipeline step "Run clang-tidy" currently pipes grep
into xargs and can fail with exit code 1 when grep finds no matches; update the
command so an empty grep result doesn't kill the job—either append a no-op guard
like "grep ... || true" before piping into xargs, or use "xargs -r" (or both) so
clang-tidy is only invoked when files exist; adjust the existing grep and xargs
usage in the Run clang-tidy step to handle empty input gracefully.
In `@src/corosio/src/detail/epoll/sockets.cpp`:
- Around line 633-646: The posted op can be executed with an empty impl_ptr if
shared_from_this() throws; update the code so either (A) only post the op when
shared_from_this() succeeds by moving svc_.post(&op) and svc_.work_finished()
into the try block after setting op.impl_ptr (and call request_cancel() or skip
posting when shared_from_this() throws), or (B) add a null check inside
operator()() before dereferencing socket_impl_ (check op.impl_ptr or
socket_impl_ and return/abort the operation if null) to avoid the use-after-free
when impl_ptr is empty; adjust code paths around shared_from_this(),
op.impl_ptr, svc_.post, operator()(), socket_impl_->svc_, and request_cancel()
accordingly.
In `@src/corosio/src/detail/select/scheduler.cpp`:
- Around line 505-514: timer_timeout_us (from duration::count()) is a long long
and is narrowed to long without clamping, risking truncation on 32-bit
platforms; clamp timer_timeout_us to the [0, LONG_MAX] range before any cast and
use that clamped value in both the early return (when requested_timeout_us < 0)
and the final min-with-requested logic (replace
static_cast<long>(timer_timeout_us) with the clamped value); mirror the kqueue
backend approach (use std::min/std::max or explicit bounds check on
timer_timeout_us, refer to variables timer_timeout_us, requested_timeout_us,
nearest, now) and add `#include` <climits> if LONG_MAX is used.
In `@src/corosio/src/tls/context.cpp`:
- Around line 97-111: The two functions tls_context::use_pkcs12 and
tls_context::use_pkcs12_file currently return std::error_code(ENOTSUP, ...)
which is not portable to MSVC/Windows; replace that with a portable error such
as std::make_error_code(std::errc::operation_not_supported) or wrap the existing
ENOTSUP usage in an `#ifdef` ENOTSUP fallback to a safe alternative (e.g., ENOSYS)
so compilation succeeds on Windows while preserving the semantics that PKCS#12
is not implemented.
🧹 Nitpick comments (14)
src/corosio/src/detail/intrusive.hpp (1)
115-125:remove()does not clear stale linkage on the removed node, unlikepop_front().
pop_front()defensively zeroesw->next_andw->prev_(with a comment explaining why), butremove()leaves them dangling. A secondremove()call on the same node would silently corrupt the list. Consider adding the same defensive cleanup here for consistency.♻️ Proposed fix
void remove(T* w) noexcept { if (w->prev_) w->prev_->next_ = w->next_; else head_ = w->next_; if (w->next_) w->next_->prev_ = w->prev_; else tail_ = w->prev_; + // Defensive: clear stale linkage so a second + // remove() cannot corrupt the list. + w->next_ = nullptr; + w->prev_ = nullptr; }src/corosio/src/detail/make_err.cpp (1)
23-23: Add implementation overview comment.This file contains non-trivial error code mapping logic but lacks a block comment after the includes explaining the implementation approach. An overview comment should describe how the function maps platform-specific error codes (errno on POSIX, GetLastError codes on Windows) to std::error_code, and note the special handling for cancellation and EOF conditions.
📝 Suggested overview comment
`#endif` +/* + Implementation overview: + + make_err() maps platform-specific error codes to std::error_code. + On POSIX systems, errno values are mapped, with ECANCELED translated + to capy::error::canceled. On Windows, GetLastError() codes (DWORD) + are mapped, with ERROR_OPERATION_ABORTED and ERROR_CANCELLED translated + to capy::error::canceled, and ERROR_HANDLE_EOF to capy::error::eof. + All other error codes are wrapped with std::system_category(). +*/ + namespace boost::corosio::detail {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/iocp/timers_thread.cpp (2)
10-18: Consider adding a high-level overview comment.This file contains non-trivial implementation logic involving Windows waitable timers, thread management, and IOCP integration. A block comment after the includes explaining the implementation approach would help maintainers understand the timer dispatch mechanism and the coordination between the timer thread and the completion port.
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."
109-110: Consider adding error checking forPostQueuedCompletionStatus.The call to
PostQueuedCompletionStatusdoesn't check the return value. In contrast, the pattern inscheduler.cpp(lines 256-261) checks the return value and throws a system error if it fails. Consistent error handling would make failures more diagnosable and prevent silent timer dispatch issues.🛡️ Suggested error handling pattern
- ::InterlockedExchange(dispatch_required_, 1); - ::PostQueuedCompletionStatus( - static_cast<HANDLE>(iocp_), 0, key_wake_dispatch, nullptr); + ::InterlockedExchange(dispatch_required_, 1); + if (!::PostQueuedCompletionStatus( + static_cast<HANDLE>(iocp_), 0, key_wake_dispatch, nullptr)) + { + DWORD dwError = ::GetLastError(); + detail::throw_system_error(make_err(dwError)); + }.clang-format (1)
59-59:AlwaysBreakAfterReturnTypeis deprecated in clang-format ≥ 19 in favor ofBreakAfterReturnType.Update to the new option name to avoid deprecation warnings if targeting clang-format 19 or later.
include/boost/corosio/tcp_server.hpp (1)
591-594: Nit: constructor initializer list alignment.The member initializer list uses a mix of indentation styles —
impl_starts on the same line as the closing)whileex_is indented with a leading,. This is minor and may be dictated by the new clang-format config.template<capy::ExecutionContext Ctx, capy::Executor Ex> tcp_server(Ctx& ctx, Ex ex) : impl_(make_impl(ctx)) , ex_(std::move(ex))A more readable form might be:
template<capy::ExecutionContext Ctx, capy::Executor Ex> tcp_server(Ctx& ctx, Ex ex) : impl_(make_impl(ctx)) , ex_(std::move(ex))src/corosio/src/tls/context.cpp (1)
38-51: File I/O error reporting hardcodesENOENT.When
std::ifstreamfails to open, the cause may not always be "file not found" — it could be a permissions error, disk error, etc. ReturningENOENTunconditionally loses that distinction. This pattern is repeated inuse_certificate_chain_file,use_private_key_file,load_verify_file, andadd_crl_file.Consider using
errnoafter the failed open to capture the actual system error, or document that only ENOENT is returned.Proposed fix (example for one function)
std::error_code tls_context::use_certificate_file( std::string_view filename, tls_file_format format) { std::ifstream file(std::string(filename), std::ios::binary); if (!file) - return std::error_code(ENOENT, std::generic_category()); + return std::error_code(errno, std::generic_category()); std::ostringstream ss; ss << file.rdbuf(); impl_->entity_certificate = ss.str(); impl_->entity_cert_format = format; return {}; }src/corosio/src/tcp_socket.cpp (1)
72-78: Silently discarding the shutdown error code.
shutdown()capturesecbut discards it with[[maybe_unused]]. This is intentional (shutdown can fail on already-closed sockets, etc.), but it differs from the new pattern in the option methods where errors are thrown. Consider whether callers should know about shutdown failures — at minimum, a brief comment explaining why the error is intentionally swallowed would help future maintainers.src/wolfssl/src/wolfssl_stream.cpp (2)
486-490: Tautological check:total_read > 0is always true here.Inside the
if (ret > 0)block,total_readwas just incremented by a positive value, so theif (total_read > 0)guard on line 486 is always true. This is pre-existing logic (not introduced by this PR), so just flagging as a minor nit for a future cleanup.
612-612: Same tautology indo_write_some.Analogous to
do_read_some:total_written > 0on line 612 is always true inside theif (ret > 0)block.src/corosio/src/detail/epoll/scheduler.cpp (1)
955-967: NOLINT placement for multi-line statement.The
NOLINT(clang-analyzer-unix.BlockInCriticalSection)on line 955 and the one on line 967 suppress false positives for the::read()calls onevent_fd_andtimer_fd_— these reads are non-blocking (both fds useEFD_NONBLOCK/TFD_NONBLOCK) so the suppression is justified.Note the inconsistent placement: line 955 has the comment on the line before the
::read, while line 967 has it trailing after the closing paren. Both should work, but consider picking one style.src/corosio/src/detail/kqueue/sockets.cpp (1)
127-128: Minor inconsistency: kqueue usesstd::movefor saved handles, select backend uses copy.Here
saved_exandsaved_hare initialized viastd::move, while the equivalent code inselect_connect_op::operator()()(select/sockets.cpp, line 101–102) andselect_accept_op::operator()()(select/acceptors.cpp, line 132–133) uses plain copy construction. Since bothexecutor_refandcoroutine_handle<>are trivially copyable, this has no functional impact, but it would be cleaner to be consistent across backends.src/corosio/src/detail/select/sockets.cpp (1)
239-239: Pre-existing:readvmissing EINTR retry loop (unlike kqueue backend).The kqueue backend wraps
readvin ado { ... } while (n < 0 && errno == EINTR)loop (kqueue/sockets.cpp lines 330–334), but the select backend callsreadvonce with no EINTR handling. On systems with active signal handlers, a spurious EINTR could cause an unnecessary round-trip through the reactor. Not introduced by this PR, but worth noting since the backends are being reformatted together.src/corosio/src/detail/kqueue/scheduler.cpp (1)
444-467: Minor inconsistency:post_handlerdestructor lacksoverridehere but has it in the select backend.In
src/corosio/src/detail/select/scheduler.cpp(line 197), the identicalpost_handlerstruct uses~post_handler() override = default;, while here at line 450 it's~post_handler() = default;. Since both inherit fromscheduler_op(which presumably has a virtual destructor), addingoverridefor consistency is good practice even though the class isfinal.♻️ Suggested fix for consistency
- ~post_handler() = default; + ~post_handler() override = default;
| - name: Run clang-tidy | ||
| if: ${{ matrix.clang-tidy }} | ||
| run: | | ||
| python3 -c "import json; [print(e['file']) for e in json.load(open('__build_cmake_test__/compile_commands.json'))]" \ | ||
| | grep '/libs/corosio/src/' \ | ||
| | xargs clang-tidy-20 \ | ||
| -p __build_cmake_test__ \ | ||
| --warnings-as-errors='*' |
There was a problem hiding this comment.
Grep failure in pipeline could break the step even when clang-tidy itself would pass.
GitHub Actions bash steps run with set -eo pipefail. If grep finds no matching files (unlikely but possible if paths change), it exits with code 1, which kills the pipeline before xargs runs. Consider adding grep ... || true with a guard, or using xargs -r to handle empty input gracefully.
Proposed fix for robustness
- python3 -c "import json; [print(e['file']) for e in json.load(open('__build_cmake_test__/compile_commands.json'))]" \
- | grep '/libs/corosio/src/' \
- | xargs clang-tidy-20 \
- -p __build_cmake_test__ \
- --warnings-as-errors='*'
+ FILES=$(python3 -c "import json; [print(e['file']) for e in json.load(open('__build_cmake_test__/compile_commands.json'))]" \
+ | grep '/libs/corosio/src/' || true)
+ if [ -n "$FILES" ]; then
+ echo "$FILES" | xargs clang-tidy-20 \
+ -p __build_cmake_test__ \
+ --warnings-as-errors='*'
+ else
+ echo "No source files found for clang-tidy"
+ exit 1
+ fi📝 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.
| - name: Run clang-tidy | |
| if: ${{ matrix.clang-tidy }} | |
| run: | | |
| python3 -c "import json; [print(e['file']) for e in json.load(open('__build_cmake_test__/compile_commands.json'))]" \ | |
| | grep '/libs/corosio/src/' \ | |
| | xargs clang-tidy-20 \ | |
| -p __build_cmake_test__ \ | |
| --warnings-as-errors='*' | |
| - name: Run clang-tidy | |
| if: ${{ matrix.clang-tidy }} | |
| run: | | |
| FILES=$(python3 -c "import json; [print(e['file']) for e in json.load(open('__build_cmake_test__/compile_commands.json'))]" \ | |
| | grep '/libs/corosio/src/' || true) | |
| if [ -n "$FILES" ]; then | |
| echo "$FILES" | xargs clang-tidy-20 \ | |
| -p __build_cmake_test__ \ | |
| --warnings-as-errors='*' | |
| else | |
| echo "No source files found for clang-tidy" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
In @.github/workflows/ci.yml around lines 719 - 726, The pipeline step "Run
clang-tidy" currently pipes grep into xargs and can fail with exit code 1 when
grep finds no matches; update the command so an empty grep result doesn't kill
the job—either append a no-op guard like "grep ... || true" before piping into
xargs, or use "xargs -r" (or both) so clang-tidy is only invoked when files
exist; adjust the existing grep and xargs usage in the Run clang-tidy step to
handle empty input gracefully.
| std::error_code | ||
| tls_context:: | ||
| use_pkcs12( | ||
| std::string_view /*data*/, | ||
| std::string_view /*passphrase*/ ) | ||
| tls_context::use_pkcs12( | ||
| std::string_view /*data*/, std::string_view /*passphrase*/) | ||
| { | ||
| // TODO: Implement PKCS#12 parsing | ||
| return std::error_code( ENOTSUP, std::generic_category() ); | ||
| return std::error_code(ENOTSUP, std::generic_category()); | ||
| } | ||
|
|
||
| std::error_code | ||
| tls_context:: | ||
| use_pkcs12_file( | ||
| std::string_view /*filename*/, | ||
| std::string_view /*passphrase*/ ) | ||
| tls_context::use_pkcs12_file( | ||
| std::string_view /*filename*/, std::string_view /*passphrase*/) | ||
| { | ||
| // TODO: Implement PKCS#12 file loading | ||
| return std::error_code( ENOTSUP, std::generic_category() ); | ||
| return std::error_code(ENOTSUP, std::generic_category()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check all ENOTSUP usage in the codebase
rg -n 'ENOTSUP' --type=cpp --type=c -C 2Repository: cppalliance/corosio
Length of output: 642
🏁 Script executed:
# Look for other error code patterns and platform guards
rg -n '(ENOENT|EAGAIN|generic_category)' --type=cpp --type=c | head -20Repository: cppalliance/corosio
Length of output: 2134
🏁 Script executed:
# Check if there are platform-specific headers or guards in the tls directory
fd . src/corosio/src/tls/ --type f -x cat {} \; | head -100Repository: cppalliance/corosio
Length of output: 2888
🌐 Web query:
MSVC Visual Studio ENOTSUP cerrno.h availability C++ standard library
💡 Result:
ENOTSUPis not guaranteed to exist in MSVC’s<errno.h>/<cerrno>. It’s a POSIX errno name, and many POSIX errno macros are simply not defined on Windows toolchains (including mingw and older MSVC CRTs); Gnulib explicitly listsENOTSUPamong macros “not defined” on mingw/MSVC targets. [1]- What the C/C++ standards actually require: the C/C++ headers only require
errnoplus at leastEDOM,ERANGE, andEILSEQ; everything else (includingENOTSUP) is implementation-defined/optional. [2][3] <cerrno>in C++ includes many POSIX-style names, but they are not universally present across platforms/standard libraries (Windows/MSVC being a common exception). [4]- If this is about sockets on Windows: Winsock errors are not reported via
errno; you should useWSAGetLastError()and theWSA*codes (e.g.,WSAEOPNOTSUPP). [5]
Practical patterns (portable)
1) Prefer std::error_code / std::errc over raw errno names
std::error_code ec = /* ... */;
if (ec == std::errc::operation_not_supported || ec == std::errc::not_supported) {
// handle not supported
}2) If you must check errno, guard ENOTSUP and provide a fallback
if (
`#ifdef` ENOTSUP
errno == ENOTSUP ||
`#endif`
`#ifdef` EOPNOTSUPP
errno == EOPNOTSUPP ||
`#endif`
errno == ENOSYS // often "function not supported"
) { /* ... */ }Sources:
[1] Gnulib errno.h portability notes (lists ENOTSUP missing on mingw/MSVC) (gnu.org)
[2] <cerrno> requires EDOM, ERANGE, EILSEQ (cplusplus.com) (cplusplus.com)
[3] C standard summary of required <errno.h> macros (di-mgt) (di-mgt.com.au)
[4] cppreference note: many <cerrno> macros adopted from POSIX (not universally available) (en.cppreference.com)
[5] Microsoft Learn: Winsock uses WSAGetLastError, not errno (learn.microsoft.com)
Citations:
- 1: https://www.gnu.org/software/gnulib/manual/html_node/errno_002eh.html?utm_source=openai
- 2: https://cplusplus.com/reference/cerrno/?utm_source=openai
- 3: https://di-mgt.com.au/std_c/errno.html?utm_source=openai
- 4: https://en.cppreference.com/w/cpp/header/cerrno.html?utm_source=openai
- 5: https://learn.microsoft.com/en-us/windows/win32/winsock/error-codes-errno-h-errno-and-wsagetlasterror-2?utm_source=openai
Guard ENOTSUP for Windows portability, or use an alternative error code.
ENOTSUP is a POSIX errno value not guaranteed to be defined on Windows/MSVC toolchains. The web search confirms it is explicitly listed as missing on mingw/MSVC targets. If this code is compiled on Windows, wrap the usage in #ifdef ENOTSUP with a fallback (e.g., ENOSYS or a platform-agnostic error), or prefer std::errc::operation_not_supported for maximum portability. Note that ENOENT is used elsewhere in the file without guards; verify whether the overall codebase targets Windows or is Unix-only.
🤖 Prompt for AI Agents
In `@src/corosio/src/tls/context.cpp` around lines 97 - 111, The two functions
tls_context::use_pkcs12 and tls_context::use_pkcs12_file currently return
std::error_code(ENOTSUP, ...) which is not portable to MSVC/Windows; replace
that with a portable error such as
std::make_error_code(std::errc::operation_not_supported) or wrap the existing
ENOTSUP usage in an `#ifdef` ENOTSUP fallback to a safe alternative (e.g., ENOSYS)
so compilation succeeds on Windows while preserving the semantics that PKCS#12
is not implemented.
|
GCOVR code coverage report https://145.corosio.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-02-16 03:39:08 UTC |
sgerbino
force-pushed
the
pr/clang-tools
branch
from
3cd38af to
39ecf13
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 (9)
src/corosio/src/detail/intrusive.hpp (1)
107-110:⚠️ Potential issue | 🟡 MinorComment overpromises: clearing pointers prevents UB, not logical corruption.
If
remove()is called on a popped node whosenext_andprev_are bothnullptr, theremove()logic will unconditionally sethead_ = nullptrandtail_ = nullptr, wiping the list regardless of its actual contents. The defensive clearing prevents dangling-pointer dereferences (UB), not list corruption. Consider rewording the comment.📝 Suggested comment wording
- // Defensive: clear stale linkage so remove() on a - // popped node cannot corrupt the list. + // Defensive: clear stale linkage so a stale node + // cannot cause dangling-pointer dereferences.src/corosio/src/tcp_server.cpp (1)
40-51:⚠️ Potential issue | 🟠 MajorUse
std::moveforex_in move constructor and move assignment operator for proper move semantics.Lines 42 and 58 copy
ex_instead of moving it. All other members are properly moved viastd::exchangeorstd::move, and the template constructor (line 593) usesstd::move(ex)for the executor parameter. This inconsistency violates move semantics and may unnecessarily copycapy::any_executorwhen the moved-from object is otherwise left as an empty shell.Proposed fix
tcp_server::tcp_server(tcp_server&& o) noexcept : impl_(std::exchange(o.impl_, nullptr)) - , ex_(o.ex_) + , ex_(std::move(o.ex_)) , waiters_(std::exchange(o.waiters_, nullptr))tcp_server& tcp_server::operator=(tcp_server&& o) noexcept { delete impl_; impl_ = std::exchange(o.impl_, nullptr); - ex_ = o.ex_; + ex_ = std::move(o.ex_); waiters_ = std::exchange(o.waiters_, nullptr);src/corosio/src/detail/kqueue/acceptors.cpp (1)
197-206:⚠️ Potential issue | 🔴 CriticalAdd null check for
acceptor_impl_in failure path to match success path.At line 106, the success path guards
acceptor_impl_with a null check before dereferencing. The failure path at lines 197–206 does not. Thecancel()method (line 76) explicitly checksif (acceptor_impl_), indicating it can be null. Although the function unconditionally dereferences at line 87, the inconsistency between success and failure paths is a code smell and should be corrected for defensive consistency.Proposed fix
if (peer_impl) { - auto* socket_svc_cleanup = - static_cast<kqueue_acceptor_impl*>(acceptor_impl_) - ->service() - .socket_service(); - if (socket_svc_cleanup) - socket_svc_cleanup->destroy(peer_impl); + if (acceptor_impl_) + { + auto* socket_svc_cleanup = + static_cast<kqueue_acceptor_impl*>(acceptor_impl_) + ->service() + .socket_service(); + if (socket_svc_cleanup) + socket_svc_cleanup->destroy(peer_impl); + } peer_impl = nullptr; }src/corosio/src/detail/kqueue/scheduler.cpp (1)
1107-1119:⚠️ Potential issue | 🟠 MajorNit: task sentinel is not restored if
run_taskthrows.If
run_taskthrows (e.g., fromkeventreturning an unexpected error), thecatchblock re-throws after clearingtask_running_, buttask_op_is never pushed back ontocompleted_ops_(line 1118 is skipped). This means the reactor sentinel is permanently lost, and subsequentdo_oneiterations will never enter the reactor path — effectively halting I/O processing. Consider pushing the sentinel back before re-throwing:Proposed fix
catch (...) { task_running_ = false; + completed_ops_.push(&task_op_); throw; }src/openssl/src/openssl_stream.cpp (3)
111-113:⚠️ Potential issue | 🟠 Major
sni_ctx_data_indexhas a potential data race on lazy initialization.The static
sni_ctx_data_indexis read and written without synchronization (lines 113, 171-173). If two threads concurrently construct their firstopenssl_native_context, both may callSSL_CTX_get_ex_new_index, and the non-atomic write/read ofsni_ctx_data_indexis a data race (UB). Consider usingstd::call_onceorstd::atomic<int>.Also applies to: 171-173
720-733:⚠️ Potential issue | 🔴 Critical
make_implreturnsnullptron initialization failure, leading to null-pointer dereference in all public methods.If
init_ssl()fails,make_implreturnsnullptr(line 729). The template constructors in the header (lines 89, 106) assignimpl_directly without validation. Every public method (do_read_some,do_write_some,handshake,shutdown,reset) dereferencesimpl_unconditionally, causing undefined behavior.Throw an exception from
make_implon failure instead of returning null.Proposed fix
openssl_stream::impl* openssl_stream::make_impl(capy::any_stream& stream, tls_context const& ctx) { auto* p = new impl(stream, ctx); auto ec = p->init_ssl(); if (ec) { delete p; - return nullptr; + throw std::system_error(ec, "openssl_stream: SSL initialization failed"); } return p; }
740-758:⚠️ Potential issue | 🔴 CriticalMove operations cause
impl_->s_to reference a moved-from stream.After move,
impl_points to the old implementation that still holds a reference toother.stream_, notthis->stream_. All subsequent I/O throughimpl_->s_will operate on the wrong stream object (the moved-fromother.stream_).The proposed fix in the original comment (
impl_->s_ = stream_;) will not work—C++ reference members cannot be rebound after construction. Assignment to a reference member assigns to the referenced object, not the reference itself.The implementation must either:
- Recreate
impl_after the move viamake_impl(stream_, ctx_)(requires storingtls_contextseparately for the moved-to object)- Change
impl::s_from a reference to a pointer- Use placement new to reconstruct
implwith the new stream referencesrc/corosio/src/detail/select/sockets.cpp (1)
397-397:⚠️ Potential issue | 🟡 Minor
sendmsg()returning 0 falls through to the error path — intentional?When
sendmsg()returns 0 (which can happen with a zero-length write on some systems),n > 0is false anderrno == EAGAINis likely also false (errno may be 0). The code then falls to Line 397:op.complete(errno ? errno : EIO, 0). Sinceerrnomay be 0 after a successful zero-byte send, this synthesizesEIO.For
read_some()(Lines 249-255), then == 0case is handled explicitly. Consider adding an explicitn == 0case forwrite_sometoo, or document why it's intentionally treated as an error.src/corosio/src/detail/select/acceptors.cpp (1)
318-329:⚠️ Potential issue | 🟠 MajorUse-after-free risk:
post(&op)outside try-catch whenimpl_ptrmay not be set.If
shared_from_this()throwsbad_weak_ptr(Line 323),impl_ptrremains unset, but the op is still posted (Line 327) andwork_finished()called (Line 328). Since the op is embedded in the acceptor impl, if the impl is destroyed before the scheduler processes the posted op, the scheduler will dereference a dangling pointer.Compare with
sockets.cpp(Lines 594-604) wherepostandwork_finishedare inside the try block — the safer pattern.🐛 Proposed fix: move post/work_finished inside the try block
try { op.impl_ptr = shared_from_this(); + svc_.post(&op); + svc_.work_finished(); } // NOLINTNEXTLINE(bugprone-empty-catch) catch (const std::bad_weak_ptr&) { } - - svc_.post(&op); - svc_.work_finished(); } }
🤖 Fix all issues with AI agents
In `@src/corosio/src/detail/kqueue/acceptors.cpp`:
- Around line 388-399: The try/catch around shared_from_this() can leave
op.impl_ptr empty while still calling svc_.post(&op), risking dangling access in
operator()(); change the catch to refuse posting: inside the catch (const
std::bad_weak_ptr&) log or assert the error, and return (or otherwise abort
posting) while ensuring svc_.work_finished() is called only when an op was
successfully posted; move svc_.post(&op) and svc_.work_finished() into the try
block after successful assignment to op.impl_ptr. Apply the same change to the
close_socket() site so ops are never posted without a valid op.impl_ptr.
In `@src/corosio/src/detail/posix/signals.cpp`:
- Around line 761-771: The code mixes executor-level
on_work_started()/on_work_finished() with I/O-level
work_started()/work_finished(): posix_signals_impl::start_wait() currently calls
sched_->on_work_started() while signal_op::operator() ends up calling
posix_signals_impl::work_finished() via the wrapper. Fix by making the pair
consistent for I/O signal handling: change start_wait() to use the I/O API (call
posix_signals_impl::work_started()/or sched_->work_started()) instead of
on_work_started(), so start_wait() and signal_op::operator() use the same
work_started()/work_finished() semantics.
In `@src/corosio/src/detail/select/sockets.cpp`:
- Around line 594-604: The current sockets.cpp try block sets op.impl_ptr via
shared_from_this(), then calls svc_.post(&op) and svc_.work_finished(), which
skips both calls if shared_from_this() throws (avoiding a dangling post but
leaking work count if work_finished() isn't run); unify with
acceptors.cpp/cancel_single_op by ensuring we never post the op when
shared_from_this() fails but always decrement the work count. Modify the code
around shared_from_this(), op.impl_ptr, svc_.post(&op), and svc_.work_finished()
so that: attempt to set op.impl_ptr inside a try/catch, if it succeeds call
svc_.post(&op) and then svc_.work_finished(), and if it throws catch the
exception and still call svc_.work_finished() (without calling svc_.post) to
avoid leaking work; reference op.impl_ptr, shared_from_this(), svc_.post,
svc_.work_finished(), and the cancel_single_op pattern in acceptors.cpp when
making the change.
In `@src/corosio/src/tcp_socket.cpp`:
- Around line 71-78: tcp_socket::shutdown currently swallows errors (uses
[[maybe_unused]] on the ec from get().shutdown), which is inconsistent with
other non-noexcept methods; change it to capture the error_code (auto ec =
get().shutdown(what);) and if (ec) throw std::system_error(ec) so callers
receive exceptions like other methods (or alternatively mark the method noexcept
in the header if you intentionally want a best-effort API or add a
shutdown(error_code&) overload to expose the result)—update
tcp_socket::shutdown, the call to get().shutdown, and the function signature
accordingly to reflect the chosen behavior.
🧹 Nitpick comments (12)
src/corosio/src/ipv6_address.cpp (1)
352-352: Thestd::size_tcast is safe here but fragile — consider an assertion or comment.
std::size_t(7 - n)is safe because at least oneh16has been parsed before reaching the'.'branch (guaranteeingn <= 7). However, if future refactoring ever allowsn > 7to reach this line, the cast of a negative int tostd::size_twould silently wrap to a huge value, causing out-of-bounds access. A brief// n <= 7 guaranteed herecomment or a debug assert would make the invariant explicit.Note that lines 379–382 use
2 * (7 - n)withoutstd::size_tcasts — you may want to be consistent one way or the other.src/corosio/src/detail/intrusive.hpp (1)
114-124:remove()doesn't clear the removed node's linkage (asymmetric withpop_front).
pop_front()now defensively zeroes the popped node'snext_/prev_, butremove()leaves them stale. If the same defensive rationale applies, consider clearing them here too for consistency.♻️ Optional fix
void remove(T* w) noexcept { if (w->prev_) w->prev_->next_ = w->next_; else head_ = w->next_; if (w->next_) w->next_->prev_ = w->prev_; else tail_ = w->prev_; + w->next_ = nullptr; + w->prev_ = nullptr; }src/corosio/src/tcp_socket.cpp (1)
10-18: Consider adding a brief implementation overview comment after the includes.Per the project's documentation guidelines, files containing implementation logic should include a
/* */block comment after the includes summarizing how the implementation works. While this file is largely delegation, a one-liner such as:/* tcp_socket method definitions — thin wrappers that delegate to the platform-specific io_handle implementation, throwing on error. */would orient future readers quickly.
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."include/boost/corosio/tcp_server.hpp (2)
591-595: Constructor member-initializer formatting is fine, but consider a diagnostic formake_implfailure.
make_implperforms a barenew(per the relevant snippet intcp_server.cpp). If allocation fails,std::bad_allocpropagates beforeimpl_is initialized — this is safe because no resource has been acquired yet. However, there's no null-check or documented exception guarantee for callers. Consider adding a@throwsnote in the doc block (lines 575-590) mentioningstd::bad_allocifmake_implfails.
640-655:set_workersleaks ifidle_pushthrows (it won't, but the pattern is fragile).Line 650 does a raw
new StorageType(...), and line 651 wraps it in ashared_ptr. If anything betweennewand theshared_ptrconstruction threw, the allocation would leak. Currently that gap is a single statement so it's safe in practice, but wrapping the allocation directly insidestd::shared_ptrconstruction (or usingstd::make_sharedwith a type-erased deleter) would be more robust against future edits.That said,
idle_pushisnoexceptand theshared_ptrconstructor from a raw pointer doesn't throw (the control block is allocated duringshared_ptrconstruction, which can throw), so this is a theoretical concern.🛡️ Slightly safer alternative
- using StorageType = std::decay_t<Range>; - auto* p = new StorageType(std::forward<Range>(workers)); - storage_ = std::shared_ptr<void>( - p, [](void* ptr) { delete static_cast<StorageType*>(ptr); }); - for (auto&& elem : *static_cast<StorageType*>(p)) + using StorageType = std::decay_t<Range>; + auto sp = std::shared_ptr<StorageType>( + new StorageType(std::forward<Range>(workers)), + [](StorageType* ptr) { delete ptr; }); + for (auto&& elem : *sp) idle_push(std::to_address(elem)); + storage_ = std::move(sp);include/boost/corosio/basic_io_context.hpp (1)
203-207: Potentiallongoverflow on large durations (pre-existing).
static_cast<long>(…duration_cast<microseconds>(…).count())can overflow ifrel_timeexceeds ~35 minutes on platforms wherelongis 32-bit (e.g., Windows/MSVC). The 1-second clamp on line 200-201 prevents this today, but if that guard is ever removed or raised, this would silently wrap. Astatic_assertor narrowing check would future-proof it.This is pre-existing behavior — just flagging for awareness since the lines were touched for formatting.
src/corosio/src/detail/kqueue/acceptors.cpp (1)
212-216: Minor inconsistency with epoll backend:std::moveon trivially-copyable types.The epoll backend (see
epoll_op::operator()()) uses plain copy construction forexecutor_refandcoroutine_handle<>, while this kqueue path usesstd::move. Both are correct for trivially-copyable types, but the inconsistency may cause confusion when comparing backends.src/corosio/src/detail/posix/signals.cpp (1)
214-220: Verify that silently discardingclear()errors indestroy()is intentional.
[[maybe_unused]]suppresses the warning, butclear()can fail (e.g.,sigaction()failure). In a destroy/teardown path this is a common trade-off, but it means signal handlers may not be properly restored toSIG_DFLifsigactionfails during destruction. If this is intentional, a brief comment explaining why the error is ignored would be helpful.src/corosio/src/detail/iocp/scheduler.cpp (1)
110-157: Shutdown drain loop is a busy-wait when I/O completions are slow to arrive.The
while (outstanding_work_ > 0)loop callsGetQueuedCompletionStatuswith a zero timeout, making it a busy-spin if outstanding work exists but no completions are immediately available (e.g., pending socket I/O). This is acceptable for a shutdown path, but consider adding a briefSleep(0)orSleep(1)yield when the GQCS poll returns nothing, to avoid pegging a CPU core during a slow-draining shutdown.♻️ Optional: yield when GQCS returns nothing
if (overlapped) { ::InterlockedDecrement(&outstanding_work_); if (key == key_posted) { // Posted scheduler_op* auto* op = reinterpret_cast<scheduler_op*>(overlapped); op->destroy(); } else { // Actual I/O: convert OVERLAPPED* to overlapped_op* auto* op = overlapped_to_op(overlapped); op->destroy(); } } + else + { + // Nothing dequeued; yield to avoid busy-spin + ::Sleep(1); + }src/corosio/src/detail/select/op.hpp (1)
174-175: Copy-initialization ofsaved_exandsaved_his fine but inconsistent with kqueue backend.
capy::executor_refandstd::coroutine_handle<>are trivially copyable, so copy vs. move is semantically identical. However, the kqueue backend (e.g.,kqueue/acceptors.cpp:212-213,kqueue/sockets.cpp:126-127) usesstd::move(ex)/std::move(h)while select and epoll use plain copy. Consider unifying for consistency across backends.src/corosio/src/detail/select/acceptors.cpp (1)
440-441:SO_REUSEADDRreturn value silently ignored.While failing to set
SO_REUSEADDRis rarely fatal, silently ignoring the return value is inconsistent with the rest of the function which meticulously checks every syscall return. Consider at least logging or returning the error, especially since bind failure on address reuse is a common debugging headache.src/corosio/src/detail/select/scheduler.cpp (1)
596-601: Heap allocation under lock on every reactor iteration.
fds_to_checkis astd::vectorallocated while the mutex is held, which runs on every reactor pass. Consider using asmall_vectoror a pre-allocated member buffer to avoid repeated heap allocation on the hot path. This is a minor performance consideration and not urgent.
| void | ||
| tcp_socket:: | ||
| shutdown(shutdown_type what) | ||
| tcp_socket::shutdown(shutdown_type what) | ||
| { | ||
| if (is_open()) | ||
| get().shutdown(what); | ||
| { | ||
| [[maybe_unused]] auto ec = get().shutdown(what); | ||
| } | ||
| } |
There was a problem hiding this comment.
shutdown() silently discards the error code.
Every other non-noexcept method in this file throws on failure, but shutdown() captures and discards the error via [[maybe_unused]]. This creates an inconsistency: callers have no way to learn that the shutdown failed, yet the function signature (non-noexcept, non-error_code overload) implies it would throw.
If the intent is to make shutdown best-effort, consider either:
- Marking it
noexceptin the header to communicate the contract, or - Adding a
shutdown(error_code&)overload so callers who care can inspect the result, while keeping this throwing overload consistent with the rest of the API.
If this is a deliberate fire-and-forget for POSIX semantics where shutdown errors are rarely actionable, a brief inline comment explaining the rationale would help future readers.
🤖 Prompt for AI Agents
In `@src/corosio/src/tcp_socket.cpp` around lines 71 - 78, tcp_socket::shutdown
currently swallows errors (uses [[maybe_unused]] on the ec from get().shutdown),
which is inconsistent with other non-noexcept methods; change it to capture the
error_code (auto ec = get().shutdown(what);) and if (ec) throw
std::system_error(ec) so callers receive exceptions like other methods (or
alternatively mark the method noexcept in the header if you intentionally want a
best-effort API or add a shutdown(error_code&) overload to expose the
result)—update tcp_socket::shutdown, the call to get().shutdown, and the
function signature accordingly to reflect the chosen behavior.
39ecf13 to
434bb51
Compare
Add clang-tidy config and CI job. Update clang-format config (80-col limit, Allman-adjacent style) and reformat entire codebase. Split acceptor_service out of socket_service.hpp. Remove unused resume_coro.hpp.
sgerbino
force-pushed
the
pr/clang-tools
branch
from
434bb51 to
c935dde
Compare
Remove redundant on_work_started/on_work_finished from the scheduler interface. The executor retains on_work_* as thin wrappers that delegate to the unified work_started/work_finished. work_finished() now calls stop() when the count reaches zero on all backends, including IOCP which previously lacked this behavior.
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 (6)
src/corosio/src/detail/posix/signals.cpp (1)
666-691:⚠️ Potential issue | 🔴 CriticalFix work tracking mismatch in
cancel_wait(): changesched_->on_work_finished()towork_finished().
start_wait()at line 719 callssched_->work_started()andsignal_op::operator()()at line 356 callsservice->work_finished()(which delegates tosched_->work_finished()). However,cancel_wait()at line 689 callssched_->on_work_finished()— a different method pair. This breaks the work tracking balance, which controls whenio_context::run()exits. Usework_finished()instead to match the pair started bywork_started().Proposed fix
if (was_waiting) { if (op->ec_out) *op->ec_out = make_error_code(capy::error::canceled); if (op->signal_out) *op->signal_out = 0; op->d.post(op->h); - sched_->on_work_finished(); + work_finished(); }include/boost/corosio/signal_set.hpp (1)
22-27:⚠️ Potential issue | 🟡 MinorDuplicate
#include <system_error>.
<system_error>is included at both line 22 and line 27. One of them should be removed.Proposed fix
`#include` <boost/capy/concept/executor.hpp> `#include` <system_error> `#include` <concepts> `#include` <coroutine> `#include` <stop_token> -#include <system_error>src/corosio/src/detail/kqueue/sockets.cpp (1)
605-614:⚠️ Potential issue | 🟡 Minor
cancel()early-return on nullweak_from_this().lock()also skipsrequest_cancel()on all three ops.When
weak_from_this().lock()returns null (line 609), the method returns before callingrequest_cancel()onconn_,rd_, andwr_(lines 612-614). If any of those ops are in flight, their stop-token callbacks won't be unregistered. This is arguably acceptable since a null lock implies the object is in a broken ownership state, but it's worth noting this differs from the previous behavior.src/corosio/src/detail/iocp/signals.hpp (1)
22-24:⚠️ Potential issue | 🟡 MinorDuplicate
#include <system_error>.Lines 22 and 24 both include
<system_error>. One should be removed.Proposed fix
`#include` <boost/capy/ex/execution_context.hpp> `#include` "src/detail/intrusive.hpp" `#include` <system_error> -#include <system_error> - `#include` "src/detail/iocp/mutex.hpp"src/corosio/src/detail/kqueue/acceptors.hpp (1)
214-214:⚠️ Potential issue | 🟡 MinorMissing
overrideon~kqueue_acceptor_service()— inconsistent with epoll and select backends.Both
epoll_acceptor_service(line 116 ofepoll/acceptors.hpp) andselect_acceptor_service(line 115 ofselect/acceptors.hpp) declare their destructors withoverride. This one is missing it.Proposed fix
- ~kqueue_acceptor_service(); + ~kqueue_acceptor_service() override;src/corosio/src/detail/kqueue/scheduler.cpp (1)
1107-1115:⚠️ Potential issue | 🟡 Minorepoll backend lacks exception safety for
task_running_flag.Unlike the kqueue implementation at lines 1107–1115, the epoll scheduler does not wrap the
run_task()call (line 1032) in a try/catch block. Sincerun_task()can throw viadetail::throw_system_error()whenepoll_wait()fails (line 944), an exception would leavetask_running_permanently true, preventing any thread from becoming the reactor again.
🤖 Fix all issues with AI agents
In `@src/corosio/src/detail/epoll/sockets.cpp`:
- Around line 627-635: The early return in cancel_single_op can orphan a claimed
operation if weak_from_this().lock() yields null: ensure the
work_started()/work_finished() balance and the waiting coroutine is resumed by
either calling svc_.work_finished() before the early return or by still posting
the op via svc_.post(&op) even when op.impl_ptr is null and then making
operator()() tolerate a null impl_ptr; update cancel_single_op (around the
weak_from_this().lock() usage) to perform one of these two fixes and, if
choosing to post a null impl_ptr, add a null-check in operator()() to handle the
missing impl gracefully.
In `@src/corosio/src/detail/kqueue/sockets.cpp`:
- Around line 682-689: When an op is claimed but weak_from_this().lock() returns
null the function returns without balancing the work counter; ensure
svc_.work_finished() is called before returning when op.impl_ptr is null.
Specifically, in the code path that sets op.impl_ptr = weak_from_this().lock()
(and then checks if (!op.impl_ptr) return) add a call to svc_.work_finished()
immediately before returning to avoid leaking the parked coroutine and the
scheduler work count; apply the same fix in
kqueue_acceptor_impl::cancel_single_op() where the identical pattern occurs.
🧹 Nitpick comments (15)
src/corosio/src/select_context.cpp (1)
10-18: Consider adding a brief implementation overview comment.The coding guidelines recommend a
/* */block comment after the includes for files with non-trivial implementation logic. While this file is quite concise, a one-liner noting the select backend's context lifecycle (construct → register services, destruct → shutdown + destroy) could help future maintainers. 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/tls/context.cpp (1)
35-48: Consider extracting the repeated file-reading pattern into a helper.The open → check →
ostringstream→rdbuf()→.str()sequence is duplicated five times (use_certificate_file,use_certificate_chain_file,use_private_key_file,load_verify_file,add_crl_file). A small private helper likeread_file_contents(std::string_view filename) -> expected<std::string, std::error_code>(or a pair/result type) would eliminate the duplication and centralize the error handling.♻️ Example helper sketch
// In an anonymous namespace or as a static helper: static std::pair<std::string, std::error_code> read_file_contents(std::string_view filename) { std::ifstream file(std::string(filename), std::ios::binary); if (!file) return {{}, std::error_code(ENOENT, std::generic_category())}; std::ostringstream ss; ss << file.rdbuf(); return {ss.str(), {}}; }Then each caller becomes:
std::error_code tls_context::use_certificate_file( std::string_view filename, tls_file_format format) { auto [contents, ec] = read_file_contents(filename); if (ec) return ec; impl_->entity_certificate = std::move(contents); impl_->entity_cert_format = format; return {}; }Also applies to: 57-68, 79-92, 121-132, 231-242
src/corosio/src/ipv6_address.cpp (1)
352-352: Inconsistent use ofstd::size_tcasts in index expressions.Line 352 wraps the index arithmetic in
std::size_tcasts (std::size_t(2) * std::size_t(7 - n)), but the analogous indexing at lines 379–382 (2 * (7 - n) + 0, etc.) and lines 334–335/399–400 (2 * (8 - n) + 0) still use plainintarithmetic. Sincenisint, the cast at line 352 silently wraps if7 - nwere ever negative — though that shouldn't happen here, using a consistent style across all index expressions would be cleaner and avoid mixed-signedness warnings uniformly.src/corosio/src/detail/intrusive.hpp (1)
114-124:remove()does not clear stale linkage on the removed node, unlikepop_front().
pop_front()defensively nulls out the popped node'snext_/prev_pointers (with a comment explaining why), butremove()leaves them dangling. A subsequent accidentalremove()on the same node would silently corrupt the list. Consider the same defensive cleanup here for consistency.♻️ Suggested diff
void remove(T* w) noexcept { if (w->prev_) w->prev_->next_ = w->next_; else head_ = w->next_; if (w->next_) w->next_->prev_ = w->prev_; else tail_ = w->prev_; + // Defensive: clear stale linkage so a second remove() + // on the same node cannot corrupt the list. + w->next_ = nullptr; + w->prev_ = nullptr; }src/corosio/src/tcp_socket.cpp (1)
10-18: Missing high-level implementation overview comment after includes.Per coding guidelines, files with non-trivial implementation logic should include a
/* */block comment after the includes summarizing how the implementation works (e.g., the ec-checked accessor pattern, platform dispatch, and the fire-and-forget shutdown contract). 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."include/boost/corosio/wolfssl_stream.hpp (1)
119-138: Public override methods lack docstrings.
handshake,shutdown,reset,name, and the move operations are undocumented here. If the base classtls_streamalready documents the contract, a brief/**@copydoctls_stream::handshake */(or similar) would satisfy the guideline without duplicating prose. As per coding guidelines, "Docstrings are required for all classes and functions in public headers in non-detail namespaces.".clang-tidy (1)
1-35: Good conservative clang-tidy configuration.Well-chosen check set with sensible exclusions for noisy checks. The
HeaderFilterRegexcorrectly scopes analysis to project headers.One minor note: consider whether
performance-noexcept-move-constructorshould remain excluded long-term, since noexcept move constructors are important for STL container efficiency (e.g.,std::vectorreallocation). The exclusion is understandable for an initial rollout to reduce noise, but it may be worth re-enabling once the codebase stabilizes.src/corosio/src/detail/select/op.hpp (1)
197-231: NOLINTNEXTLINE suppressions forstd::stop_tokenby-value parameters are appropriate.
std::stop_tokenis designed to be passed by value (small, ref-counted handle). Theperformance-unnecessary-value-paramwarning is a false positive here since the token is consumed bystop_cb.emplace(). Note that the equivalentstart()overloads inkqueue/op.hpp(lines 236–256) takestd::stop_tokenby value without these suppressions — consider adding them there too for consistency when clang-tidy is enabled on that backend.include/boost/corosio/tcp_server.hpp (1)
591-595: Constructor member initializer list formatting with leading commas.Minor nit: the initializer list places
impl_andex_indentation aligned with the colon, which is fine. However, the colon-prefixedimpl_starts quite far to the right due to aligning after the closing parenthesis. If clang-format produced this, it's acceptable; just noting it reads a bit unusually.src/corosio/src/tcp_server.cpp (2)
40-51: Move constructor copiesex_instead of moving it.Line 42 uses
ex_(o.ex_)(copy) rather thanex_(std::move(o.ex_)). Similarly, line 58 in the move assignment usesex_ = o.ex_. Sinceany_executoris type-erased, copying likely involves a reference-count bump or allocation, whereas moving would be cheaper. All other members are properly moved/exchanged fromo.Proposed fix
tcp_server::tcp_server(tcp_server&& o) noexcept : impl_(std::exchange(o.impl_, nullptr)) - , ex_(o.ex_) + , ex_(std::move(o.ex_)) , waiters_(std::exchange(o.waiters_, nullptr))
53-67: Move assignment also copiesex_instead of moving it.Same issue as the move constructor —
ex_ = o.ex_should beex_ = std::move(o.ex_)for consistency and efficiency.Proposed fix
impl_ = std::exchange(o.impl_, nullptr); - ex_ = o.ex_; + ex_ = std::move(o.ex_); waiters_ = std::exchange(o.waiters_, nullptr);include/boost/corosio/basic_io_context.hpp (1)
370-373: Consider removingoperator!=for C++20.In C++20,
operator!=is automatically synthesized fromoperator==. Since the codebase uses<coroutine>(a C++20 header), this is redundant. However, keeping it for explicit compatibility with C++17-era tooling is harmless.src/corosio/src/detail/epoll/sockets.cpp (1)
675-682: Empty destructor body—consider= default.Line 682 uses an empty body
{}for~epoll_socket_service(). For consistency with line 177 (~epoll_socket_impl() = default;), consider using= default.Proposed change
-epoll_socket_service::~epoll_socket_service() {} +epoll_socket_service::~epoll_socket_service() = default;src/corosio/src/detail/select/scheduler.cpp (2)
452-457:interrupt_reactor()lacks idempotency guard unlike epoll/kqueue backends.The epoll backend guards
interrupt_reactor()witheventfd_armed_.compare_exchange_strong(...)and kqueue usesuser_event_armed_.compare_exchange_strong(...)to avoid redundant writes. The select backend writes to the pipe unconditionally on every call. While functionally safe (non-blocking pipe + drain loop at lines 587–589), this generates more syscalls under contention — everywake_one_thread_and_unlock,work_finished, andstopcall triggers awrite(). Consider adding a similarstd::atomic<bool>guard for consistency and to reduce unnecessary syscalls.
596-601: Heap allocation per reactor iteration for fd iteration.
fds_to_checkis astd::vector<int>allocated each timerun_reactor()processes I/O completions. Sinceregistered_fds_.size()is bounded byFD_SETSIZE(~1024), consider using a small stack buffer (e.g.,boost::container::small_vectoror a fixed-size array) to avoid the heap allocation on every reactor cycle.
If weak_from_this().lock() returns null after claiming an op from the descriptor slot, the early return skipped work_finished(), leaking the work_started() from register_op(). This could cause run() to hang. Fix all four instances: epoll/kqueue sockets and acceptors.
sgerbino
force-pushed
the
pr/clang-tools
branch
from
be3a4a6 to
3c33570
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 (6)
include/boost/corosio/signal_set.hpp (1)
22-27:⚠️ Potential issue | 🟡 MinorDuplicate
#include <system_error>.Lines 22 and 27 both include
<system_error>. One of them should be removed.Proposed fix
`#include` <boost/capy/concept/executor.hpp> -#include <system_error> `#include` <concepts> `#include` <coroutine> `#include` <stop_token> `#include` <system_error>src/corosio/src/detail/posix/resolver_service.cpp (2)
220-222:⚠️ Potential issue | 🔴 CriticalBug:
errnoread on wrong thread forEAI_SYSTEM.
make_gai_erroris invoked fromresolve_op::operator()()/reverse_resolve_op::operator()(), which execute on the scheduler thread. However,getaddrinfo/getnameinforun on a detached worker thread. Sinceerrnois thread-local on POSIX, line 222 captures the scheduler thread'serrno, not the worker's.Fix: capture
errnoalongsidegai_errorin the worker thread and pass it through.Proposed fix
Add a field to both op structs to store the system errno:
// Result storage (populated by worker thread) resolver_results stored_results; int gai_error = 0; + int sys_errno = 0;In the worker thread, capture errno immediately after
getaddrinfo:int result = ::getaddrinfo( op_.host.empty() ? nullptr : op_.host.c_str(), op_.service.empty() ? nullptr : op_.service.c_str(), &hints, &ai); + int captured_errno = errno; if (!op_.cancelled.load(std::memory_order_acquire)) { if (result == 0 && ai) { op_.stored_results = convert_results(ai, op_.host, op_.service); op_.gai_error = 0; } else { op_.gai_error = result; + op_.sys_errno = captured_errno; } }Then in
make_gai_error, accept the stored errno instead of reading the global:-std::error_code -make_gai_error(int gai_err) +std::error_code +make_gai_error(int gai_err, int sys_errno = 0) { ... case EAI_SYSTEM: - return std::error_code(errno, std::generic_category()); + return std::error_code(sys_errno, std::generic_category()); ... }And update the call sites in
operator()():- *ec_out = make_gai_error(gai_error); + *ec_out = make_gai_error(gai_error, sys_errno);Apply the same pattern to
reverse_resolve_opand its worker thread.
700-761:⚠️ Potential issue | 🔴 CriticalSame
errnocapture issue applies toreverse_resolveworker thread.The
getnameinfocall at line 725 may also returnEAI_SYSTEM, and the same thread-localerrnoproblem described above applies here. Ensureerrnois captured in this worker thread as well.src/corosio/src/timer.cpp (1)
41-52:⚠️ Potential issue | 🟡 MinorMove assignment should delegate to base class
io_object::operator=for consistency.The move constructor delegates to
io_object(std::move(other)), but the move assignment operator directly accessesh_instead of callingio_object::operator=. While both currently move the same member, this asymmetry is fragile: ifio_objectever adds state, logic, or lifecycle management to its move assignment,timerwill silently diverge. Useio_object::operator=(std::move(other))to maintain consistent patterns and protect against future evolution of the base class.src/corosio/src/detail/select/acceptors.cpp (1)
116-125:⚠️ Potential issue | 🟡 MinorPotential null-pointer dereference of
acceptor_impl_in cleanup path.In the failure branch (lines 108–129),
peer_implis checked for non-null at line 116, butacceptor_impl_is dereferenced at line 119 without a null check. Ifpeer_implis non-null whileacceptor_impl_happens to be null, this is a crash.🛡️ Proposed fix
if (peer_impl) { - auto* socket_svc_cleanup = - static_cast<select_acceptor_impl*>(acceptor_impl_) - ->service() - .socket_service(); - if (socket_svc_cleanup) - socket_svc_cleanup->destroy(peer_impl); - peer_impl = nullptr; + if (acceptor_impl_) + { + auto* socket_svc_cleanup = + static_cast<select_acceptor_impl*>(acceptor_impl_) + ->service() + .socket_service(); + if (socket_svc_cleanup) + socket_svc_cleanup->destroy(peer_impl); + } + peer_impl = nullptr; }src/corosio/src/detail/epoll/acceptors.cpp (1)
49-56:⚠️ Potential issue | 🟠 MajorGuard
ec_outbefore dereferencing — it may be null.Lines 51–56 and line 92 dereference
*ec_outunconditionally. The kqueue backend (kqueue/acceptors.cpplines 94–102) and select backend (select/acceptors.cpplines 44–52, 92–93) both guard withif (ec_out)before dereferencing. The inconsistency indicatesec_outis treated as a potentially nullable parameter; epoll must follow suit to avoid a null-pointer dereference crash.Wrap the error-code assignments (lines 51–56 and line 92) with an
if (ec_out)guard.
🤖 Fix all issues with AI agents
In `@src/corosio/src/detail/kqueue/sockets.cpp`:
- Around line 682-689: The cancellation path leaks coroutine handles when
weak_from_this().lock() yields null: op.impl_ptr remains null so svc_.post(&op)
is skipped and kqueue_op::operator()() never runs to resolve op.h; fix by either
posting the op unconditionally and guarding uses of kqueue_op::socket_impl_
inside kqueue_op::operator()() (check socket_impl_ for null before deref), or
explicitly destroy the stored coroutine handle (call h.destroy() on op.h after
verifying it's a non-empty handle) when impl_ptr is null; apply the same change
to the identical pattern in acceptors.cpp and ensure svc_.work_finished()
remains balanced.
🧹 Nitpick comments (12)
src/corosio/src/endpoint.cpp (1)
10-13: Consider adding a high-level implementation overview comment.Per coding guidelines, files with non-trivial implementation logic should include a
/* */block comment after the includes describing how the implementation works. This file contains endpoint-parsing logic (format detection, IPv4/IPv6 with optional port, bracketed IPv6) that would benefit from a brief overview for maintainers.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/intrusive.hpp (1)
114-124:remove()doesn't clear stale pointers, unlikepop_front().
pop_front()defensively zeroes the popped node'snext_/prev_(with an explanatory comment), butremove()leaves them dangling. A secondremove()call on the same node would silently corrupt the list. Consider applying the same defensive cleanup here for consistency.♻️ Proposed fix
void remove(T* w) noexcept { if (w->prev_) w->prev_->next_ = w->next_; else head_ = w->next_; if (w->next_) w->next_->prev_ = w->prev_; else tail_ = w->prev_; + // Defensive: clear stale linkage so a second remove() + // on the same node cannot corrupt the list. + w->next_ = nullptr; + w->prev_ = nullptr; }src/corosio/src/ipv6_address.cpp (1)
264-267: Narrowing note: result of the arithmetic isint, notunsigned short.
static_cast<unsigned short>(p[0]) * 256promotes back tointdue to integer promotion rules, sowordis implicitly narrowed on assignment. This is harmless here (values are bounded by the subsequent checks), but if the goal of the explicit casts was to keep the arithmetic inunsigned shortland, it doesn't achieve that. A single cast of the full expression (matching the style on Lines 168/190) would be more consistent:Optional: match the style used elsewhere in this file
- unsigned short word = static_cast<unsigned short>(p[0]) * 256 + - static_cast<unsigned short>(p[1]); + unsigned short word = static_cast<unsigned short>(p[0] * 256U + p[1]);src/corosio/src/tcp_socket.cpp (1)
10-18: Missing file-level overview comment after includes.Per coding guidelines, files containing non-trivial implementation logic should include a
/* */block comment after the includes providing a high-level overview. This file has platform-branching logic and a consistent error-propagation pattern across accessors that would benefit from a brief summary (e.g., noting the throwing vs. noexcept contract split between option accessors and endpoint queries).src/corosio/src/detail/posix/resolver_service.cpp (1)
856-860: Remove unusedis_shutting_down()method.The
is_shutting_down()method is defined and declared but never called anywhere in the codebase. While the underlyingshutting_down_atomic flag is actively used during shutdown, this accessor method is dead code and should be removed. The same applies to the parallel implementation in the IOCP resolver service.src/corosio/src/timer.cpp (1)
16-16: Consider adding a brief file-level overview comment after the includes.The move-assignment operator contains a non-trivial cross-context safety invariant. A short
/* */block comment after the includes summarizing this would help future maintainers. 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/kqueue/sockets.hpp (1)
211-211: Addoverrideto kqueue_socket_service destructor for consistency.
kqueue_socket_servicedestructor (line 211) lacksoverride, whileselect_socket_service(line 193) correctly includes it. The base classsocket_servicedeclares a virtual destructor, so both implementations should useoverridefor consistency and to enable clang-tidy'smodernize-use-overrideto catch future mismatches.Proposed fix
- ~kqueue_socket_service(); + ~kqueue_socket_service() override;src/corosio/src/detail/select/acceptors.cpp (1)
343-343: Minor: prefer= defaultover empty destructor body.This is consistent with how the base class destructor is declared (
~acceptor_service() override = default).♻️ Suggested change
-select_acceptor_service::~select_acceptor_service() {} +select_acceptor_service::~select_acceptor_service() = default;src/corosio/src/detail/select/sockets.cpp (1)
622-622: Minor: prefer= defaultover empty destructor body.Same as the
select_acceptor_servicedestructor.♻️ Suggested change
-select_socket_service::~select_socket_service() {} +select_socket_service::~select_socket_service() = default;src/corosio/src/detail/kqueue/acceptors.cpp (1)
348-369:cancel()early-returns whenweak_from_this().lock()fails — verify this doesn't skip essential cleanup.When
selfis null (line 352), the method returns without callingacc_.request_cancel()or claiming any ops fromdesc_state_. If the acceptor impl isn't managed byshared_ptr, all cancel logic is silently skipped. This is intentional (the object is being torn down), but it differs from the epoll backend'scancel()(line 234–237 inepoll/acceptors.cpp) which unconditionally delegates tocancel_single_opwithout aweak_from_this()guard.The inconsistency is benign in practice (both backends handle the edge case safely), but worth noting for future alignment.
src/corosio/src/detail/epoll/acceptors.cpp (1)
10-27: Missing implementation overview block comment.The kqueue counterpart (
kqueue/acceptors.cpp, lines 22–63) has a comprehensive block comment explaining the async accept control flow, completion, lifetime management, and cancellation. This file lacks a similar overview. As per coding guidelines, files containing non-trivial implementation logic should include a block comment after the includes providing a high-level overview.src/corosio/src/detail/select/scheduler.cpp (1)
569-661: I/O completion processing performsperform_io()andgetsockoptwhile holding the mutex — potential latency concern.In
run_reactor(), after re-acquiring the lock at line 558, the I/O processing loop (lines 579–661) callsop->perform_io()(lines 616, 650) and::getsockopt()(lines 607–608, 641–642) while holdingmutex_. These syscalls block under the lock, which could delay other threads trying topost()work or register/deregister fds.The epoll backend avoids this by processing I/O in
descriptor_state::operator()()outside the main scheduler mutex. The select backend's simpler design makes this harder to avoid, but it's worth noting as a scalability consideration if contention becomes observable.
| if (claimed) | ||
| { | ||
| try { | ||
| op.impl_ptr = shared_from_this(); | ||
| } catch (const std::bad_weak_ptr&) {} | ||
| svc_.post(&op); | ||
| op.impl_ptr = weak_from_this().lock(); | ||
| if (op.impl_ptr) | ||
| svc_.post(&op); | ||
| svc_.work_finished(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check kqueue_op::operator()() for socket_impl_ dereferences
echo "=== kqueue_op::operator()() implementation (first 50 lines after definition) ==="
rg -n -A50 'kqueue_op::operator\(\)' src/corosio/src/detail/kqueue/sockets.cpp | head -60
echo ""
echo "=== Check line 105 and surrounding context ==="
sed -n '100,110p' src/corosio/src/detail/kqueue/sockets.cppRepository: cppalliance/corosio
Length of output: 2761
🏁 Script executed:
#!/bin/bash
# Check for kqueue_op destructor or any cleanup
echo "=== Search for kqueue_op destructor or cleanup ==="
rg -n 'kqueue_op' src/corosio/src/detail/kqueue/sockets.cpp | grep -E '(~|destructor|cleanup)'
echo ""
echo "=== Check kqueue_op structure definition ==="
rg -n -B5 -A15 'struct kqueue_op' src/corosio/src/detail/kqueue/sockets.cpp | head -50
echo ""
echo "=== Check acceptors.cpp for similar weak_from_this pattern ==="
rg -n -B5 -A10 'impl_ptr.*weak_from_this' src/corosio/src/detail/kqueue/acceptors.cpp
echo ""
echo "=== Check how acceptors.cpp handles null impl_ptr case ==="
rg -n -B10 -A10 'if.*impl_ptr\)' src/corosio/src/detail/kqueue/acceptors.cpp | head -40Repository: cppalliance/corosio
Length of output: 1206
🏁 Script executed:
#!/bin/bash
# Find the function that contains lines 682-689
echo "=== Context around lines 682-689 (find the function) ==="
sed -n '670,695p' src/corosio/src/detail/kqueue/sockets.cpp
echo ""
echo "=== Look at more context to find function start ==="
sed -n '650,695p' src/corosio/src/detail/kqueue/sockets.cpp | cat -n
echo ""
echo "=== Check kqueue_op full structure/class definition ==="
rg -n 'class kqueue_op|struct kqueue_op' -A30 src/corosio/src/detail/kqueue/sockets.cpp | head -60Repository: cppalliance/corosio
Length of output: 2485
🏁 Script executed:
#!/bin/bash
# Search for kqueue_op in header files
echo "=== Search for kqueue_op class definition in headers ==="
find src/corosio -name '*.hpp' -o -name '*.h' | xargs rg -n 'class kqueue_op|struct kqueue_op' -A20
echo ""
echo "=== Look for coroutine handle member (h, handle, coro_handle) ==="
find src/corosio/src/detail/kqueue -name '*.hpp' -o -name '*.h' | xargs rg -n 'std::coroutine_handle|coroutine handle' -B2 -A2Repository: cppalliance/corosio
Length of output: 7697
🏁 Script executed:
#!/bin/bash
# Check for kqueue_op member initialization and destruction
echo "=== Check full kqueue_op struct definition (including destructor/cleanup) ==="
rg -n -A50 'struct kqueue_op :' src/corosio/src/detail/kqueue/op.hpp
echo ""
echo "=== Search for impl_ptr in kqueue_op ==="
rg -n 'impl_ptr' src/corosio/src/detail/kqueue/op.hpp
echo ""
echo "=== Check if socket_impl destructor handles pending ops ==="
rg -n -A20 '~kqueue_socket_impl' src/corosio/src/detail/kqueue/sockets.cppRepository: cppalliance/corosio
Length of output: 2650
Address coroutine handle leak when weak_from_this().lock() fails during cancellation.
When impl_ptr is null (line 685), the operation is not posted, causing kqueue_op::operator()() to never execute and the coroutine handle stored in op.h to remain unresolved. This leak occurs if a stop_token callback races with socket destruction.
The unconditional socket_impl_ dereference at line 105 of kqueue_op::operator()() prevents posting when impl_ptr is null without additional null-checking. Consider either:
- Posting the op and adding a null-check in
operator()()forsocket_impl_, or - Explicitly destroying the coroutine handle via
h.destroy()whenimpl_ptris null (and ensuringhis valid—only call on default-constructed handles if uninitialized).
The identical pattern in acceptors.cpp has the same issue.
🤖 Prompt for AI Agents
In `@src/corosio/src/detail/kqueue/sockets.cpp` around lines 682 - 689, The
cancellation path leaks coroutine handles when weak_from_this().lock() yields
null: op.impl_ptr remains null so svc_.post(&op) is skipped and
kqueue_op::operator()() never runs to resolve op.h; fix by either posting the op
unconditionally and guarding uses of kqueue_op::socket_impl_ inside
kqueue_op::operator()() (check socket_impl_ for null before deref), or
explicitly destroy the stored coroutine handle (call h.destroy() on op.h after
verifying it's a non-empty handle) when impl_ptr is null; apply the same change
to the identical pattern in acceptors.cpp and ensure svc_.work_finished()
remains balanced.
2 participants
Add clang-tidy config and CI job. Update clang-format config (80-col limit, Allman-adjacent style) and reformat entire codebase. Split acceptor_service out of socket_service.hpp. Remove unused resume_coro.hpp.
Summary by CodeRabbit
Refactor
Bug Fixes
Chores