Add clang-tidy CI, reformat codebase, and split acceptor_service#145
Add clang-tidy CI, reformat codebase, and split acceptor_service#145sgerbino merged 1 commit intocppalliance:developfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds clang-format/clang-tidy and CI clang-tidy runs; introduces an acceptor_service abstraction and wires it into backends; wide-reaching formatting and qualifier changes (many destructors now Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,220,255,0.5)
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
end
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #145 +/- ##
===========================================
- Coverage 81.23% 80.97% -0.26%
===========================================
Files 64 65 +1
Lines 5696 5713 +17
===========================================
- Hits 4627 4626 -1
- Misses 1069 1087 +18
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
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 16:15:40 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;
|
GCOVR code coverage report https://145.corosio.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-02-16 16:24:45 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.
sgerbino
force-pushed
the
pr/clang-tools
branch
2 times, most recently
from
434bb51 to
c935dde
Compare
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.
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.
There was a problem hiding this comment.
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/select/acceptors.cpp (1)
116-125:⚠️ Potential issue | 🟠 MajorPotential null-pointer dereference of
acceptor_impl_in cleanup path.Line 119 dereferences
acceptor_impl_without a null check, but this member can be null — the code checks it for null at line 56 in the success path. Ifpeer_implis non-null whileacceptor_impl_has been cleared (e.g., the acceptor was destroyed before the op's completion handler runs), this will crash.Proposed fix: guard the dereference
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); + 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; }
🧹 Nitpick comments (9)
src/corosio/src/tcp_socket.cpp (1)
205-219:local_endpoint/remote_endpointsilently return a defaultendpoint{}on error — verify this is intentional given the ec-checked shift elsewhere.These two methods are marked
noexceptand return a default-constructedendpoint{}when the socket is not open, but they also delegate toget().local_endpoint()/get().remote_endpoint()which are declarednoexceptin the header and do not surface anerror_code. If the underlying OS call fails (e.g.,getsocknamereturns an error), the error is silently swallowed inside the implementation.Every other accessor in this file was just upgraded to propagate errors via
throw_system_error. These two are the odd ones out. If thenoexceptcontract is intentional (e.g., best-effort likeshutdown), a brief comment similar to line 76 would make the intent explicit and prevent future reviewers from flagging the asymmetry.src/corosio/src/detail/select/acceptors.cpp (1)
351-359: Empty destructor body — consider= default.Line 359 defines an empty destructor. Using
= defaultis more idiomatic and signals intent more clearly.Suggestion
-select_acceptor_service::~select_acceptor_service() {} +select_acceptor_service::~select_acceptor_service() = default;src/corosio/src/detail/select/sockets.cpp (1)
634-641: Same nit: prefer= defaultfor empty destructor.Suggestion
-select_socket_service::~select_socket_service() {} +select_socket_service::~select_socket_service() = default;src/corosio/src/detail/epoll/acceptors.cpp (2)
10-27: Missing high-level implementation overview comment after includes.This file contains non-trivial accept/cancel/close lifecycle logic and the acceptor service implementation. A brief
/* */block comment after the includes summarizing the key design points (e.g., the weak_from_this guard pattern, the inline-budget fast path inaccept, and why cancel is inlined intoclose_socket) would help future maintainers orient quickly. As per coding guidelines, files with non-trivial implementation logic should include such an overview.
59-95: Duplicate socket-setup logic betweenoperator()()andaccept()fast path.The block that constructs an
epoll_socket_impl, initializesdesc_state_, registers the descriptor, and sets endpoints is repeated almost verbatim inaccept()(lines 157-186). Consider extracting a shared helper (e.g.,setup_accepted_socket(int fd, const sockaddr_in& addr) -> epoll_socket_impl*) to reduce duplication and the risk of the two copies drifting apart.src/corosio/src/detail/epoll/sockets.cpp (2)
31-34: Consider adding a high-level implementation overview comment.This file contains non-trivial logic (speculative I/O with EAGAIN fallback, reactor registration,
weak_from_this-based lifetime management in cancel/close paths, edge-triggered event caching). A/* */block comment after the includes summarizing these design choices would help maintainers reason about the code. 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."
640-697: Substantial duplication betweenclose_socket()andcancel().Lines 646–682 replicate the request-cancel → lock → claim → post pattern from
cancel()(lines 552–597). The differences are: (a)close_socketunconditionally claims all slots and resetsready/cancel_pendingflags, and (b) it pins the impl viaimpl_ref_if enqueued.Extracting the shared claim-and-post logic into a private helper (e.g.,
drain_pending_ops(self, bool reset_flags)) would reduce the duplication and make future adjustments safer.src/corosio/src/detail/kqueue/acceptors.cpp (1)
248-269:shared_from_this()in synchronous accept paths can throw if preconditions are violated.Lines 257, 266, and 277 use
shared_from_this()(notweak_from_this().lock()). This is acceptable here sinceaccept()is called by the user on a live object, so the shared_ptr guarantee holds. However, unlikecancel()/close_socket()which switched toweak_from_this().lock(), an unmanaged object would get astd::bad_weak_ptrexception here with no catch. This is likely fine as a precondition violation, but worth noting for consistency.src/corosio/src/detail/kqueue/sockets.cpp (1)
656-693: Consider movingop.request_cancel()before theweak_from_this()guard.When
weak_from_this().lock()returns null at line 659, the function returns beforeop.request_cancel()at line 662 is reached. The callers (e.g.,kqueue_connect_op::cancel()) only fall through to their ownrequest_cancel()whensocket_impl_is null — they have no fallback forsocket_impl_being non-null butcancel_single_op()bailing early.In practice this is safe because
close_socket()always callsrequest_cancel()on all three ops before claiming them (lines 701–703), andclose_socket()/destroy()runs before the lastshared_ptris released. Still, hoistingrequest_cancel()above the guard costs nothing (it's an atomic store on a member of the still-alive impl during this call) and removes the dependency on that ordering invariant.♻️ Proposed change
void kqueue_socket_impl::cancel_single_op(kqueue_op& op) noexcept { + op.request_cancel(); + auto self = weak_from_this().lock(); if (!self) return; - op.request_cancel(); - kqueue_op** desc_op_ptr = nullptr;
sgerbino
force-pushed
the
pr/clang-tools
branch
from
1aecbbe to
e624e31
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 (3)
src/corosio/src/tcp_server.cpp (1)
41-52:⚠️ Potential issue | 🟠 MajorMove constructor and move-assignment operator copy
ex_instead of moving it.The template constructor (tcp_server.hpp:593) explicitly uses
std::move(ex)to move the executor, confirming thatcapy::any_executorhas move semantics. However, the move constructor (line 43) and move-assignment operator (line 59) both copyex_from the source object while usingstd::exchangeorstd::movefor all other members. This leaves the moved-from object in an inconsistent state with a still-valid executor.Change lines 43 and 59 to use
std::move: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))impl_ = std::exchange(o.impl_, nullptr); - ex_ = o.ex_; + ex_ = std::move(o.ex_); waiters_ = std::exchange(o.waiters_, nullptr);src/corosio/src/detail/epoll/acceptors.hpp (1)
112-116:⚠️ Potential issue | 🟡 MinorDestructor
overrideis correct — but kqueue counterpart is missing it.
~epoll_acceptor_service() overridecorrectly marks the destructor sinceacceptor_servicehas a virtual destructor. However, insrc/corosio/src/detail/kqueue/acceptors.hppline 214, the kqueue counterpart declares~kqueue_acceptor_service();withoutoverride. This inconsistency should be addressed for uniformity and to catch accidental base-class changes at compile time.src/corosio/src/detail/kqueue/acceptors.hpp (1)
214-214:⚠️ Potential issue | 🟡 MinorMissing
overrideon~kqueue_acceptor_service()destructor.The epoll counterpart at
src/corosio/src/detail/epoll/acceptors.hppline 116 uses~epoll_acceptor_service() override;, and the base classacceptor_servicehas a virtual destructor. Addoverridefor consistency and to let the compiler catch base-class changes.Proposed fix
- ~kqueue_acceptor_service(); + ~kqueue_acceptor_service() override;
🤖 Fix all issues with AI agents
In `@src/corosio/src/detail/iocp/scheduler.cpp`:
- Around line 320-322: The conversion from microseconds to milliseconds in the
timeout calculation can overflow a 32-bit long when evaluating (usec + 999) on
Windows; modify the code around timeout_ms to perform the add/divide in a wider
signed integer (e.g., int64_t or long long) by casting usec before adding 999,
then divide by 1000 and clamp the result to the target unsigned long range (or
INFINITE) before static_cast<unsigned long>(); update the code that computes
timeout_ms (the usec, timeout_ms and do_one(...) call) so the intermediate
arithmetic cannot overflow and the final value is safely cast for do_one.
🧹 Nitpick comments (8)
src/corosio/src/iocp_context.cpp (1)
10-18: Consider adding a brief 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. A one- or two-liner noting that this file wires up the IOCP backend services (scheduler, sockets, acceptor, signals) would help future maintainers. This is minor given the file's brevity.Suggested addition after the includes
`#include` <thread> +/* Wires the IOCP backend: creates the Win32 scheduler, socket, acceptor, + and signal services in the order required by their dependencies. */ + namespace boost::corosio {As per coding guidelines: "Files containing non-trivial implementation logic should include a
/* */block comment after the includes that provides a high-level overview."src/corosio/src/detail/intrusive.hpp (1)
114-124:remove()doesn't clear stale pointers on the removed node, unlikepop_front().
pop_front()(line 109-110) defensively zeroesw->next_andw->prev_after unlinking, with a comment explaining why.remove()does not. A doubleremove()on the same node, or aremove()followed by apop_front()that returns it, would silently corrupt the list. Consider applying the same defensive pattern 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 is a harmless no-op. + w->next_ = nullptr; + w->prev_ = nullptr; }src/corosio/src/tls/context.cpp (1)
36-49: File-reading helper is duplicated five times and silently ignores read errors.The open-read-store pattern (ifstream → ostringstream →
ss.str()) is repeated verbatim inuse_certificate_file,use_certificate_chain_file,use_private_key_file,load_verify_file, andadd_crl_file. Beyond DRY, none of these call sites checkfile.bad()orss.fail()afterss << file.rdbuf()— if the read encounters an I/O error after successfully opening the file, the caller silently receives partial/empty data with a successerror_code.Consider extracting a small helper and adding a post-read check:
♻️ Suggested helper
// In an anonymous namespace or as a private static helper namespace { std::pair<std::string, std::error_code> read_file(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(); if (file.bad() || ss.fail()) return {{}, std::error_code(EIO, std::generic_category())}; return {ss.str(), {}}; } } // namespaceEach call site then becomes a two-liner:
auto [contents, ec] = read_file(filename); if (ec) return ec;include/boost/corosio/tcp_server.hpp (1)
640-655:set_workersdoesn't guard against being called while running.The reset at lines 643–646 clears
storage_,idle_head_,active_head_, andactive_tail_, but doesn't resetwaiters_. If called while the server is running (coroutines suspended inpop_awaitablewaiting onwaiters_), those waiters become dangling. The existing doc says "Replaces any existing workers", but a debug-mode assertion on!running_(and/orwaiters_ == nullptr) would make the precondition fail-fast rather than silently corrupt state.Proposed assertion
void set_workers(Range&& workers) { + BOOST_ASSERT(!running_); + BOOST_ASSERT(waiters_ == nullptr); // Clear existing state storage_.reset();src/corosio/src/tcp_server.cpp (1)
11-15: Consider adding a high-level implementation overview comment after the includes.Per coding guidelines, files with non-trivial implementation logic should include a
/* */block comment after the includes explaining how the implementation works and highlighting anything tricky (e.g., the coroutine-based accept loop, stop/join synchronization viacondition_variable, the relationship betweenactive_accepts_and the join gate).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/timer_service.cpp (1)
792-798:timer_service_direct— bypassesfind_service()for hot-path timer access.This relies on
static_cast<basic_io_context&>(ctx)being valid. Since this is an internal detail function only called from timer construction paths where the context is always abasic_io_context, this is safe — but there's no compile-time or runtime guard.Consider adding a brief comment noting the precondition, e.g.,
// Precondition: ctx is a basic_io_context, which is already implied by the access pattern but would help future maintainers.📝 Suggested comment
// Bypass find_service() mutex by reading the scheduler's cached pointer +// Precondition: ctx must be a basic_io_context (UB otherwise) io_object::io_service& timer_service_direct(capy::execution_context& ctx) noexceptsrc/corosio/src/detail/kqueue/sockets.hpp (1)
207-233: Minor inconsistency:~kqueue_socket_service()missingoverride.The select backend's destructor was updated to
~select_socket_service() override;in this PR, but the kqueue counterpart at line 211 still lacksoverride. Consider adding it for consistency across backends.♻️ Suggested fix
- ~kqueue_socket_service(); + ~kqueue_socket_service() override;src/corosio/src/detail/iocp/scheduler.hpp (1)
47-48: Consider addingoverrideto~win_scheduler()for consistency.The epoll backend's destructor was updated to
~epoll_scheduler() override;in this PR. Thewin_schedulerdestructor should follow suit sincecapy::execution_context::servicehas a virtual destructor.Proposed fix
- ~win_scheduler(); + ~win_scheduler() override;
| unsigned long timeout_ms = | ||
| usec < 0 ? INFINITE : static_cast<unsigned long>((usec + 999) / 1000); | ||
| return do_one(timeout_ms); |
There was a problem hiding this comment.
Potential long overflow in microsecond-to-millisecond conversion on Windows.
On MSVC, long is always 32 bits. If usec is near LONG_MAX (~2.1 billion), usec + 999 overflows before the division. This is an edge case (would require a ~35-minute timeout), but could produce an incorrect (negative) timeout value.
🛡️ Suggested defensive fix
- unsigned long timeout_ms =
- usec < 0 ? INFINITE : static_cast<unsigned long>((usec + 999) / 1000);
+ unsigned long timeout_ms =
+ usec < 0 ? INFINITE
+ : static_cast<unsigned long>(
+ (static_cast<long long>(usec) + 999) / 1000);🤖 Prompt for AI Agents
In `@src/corosio/src/detail/iocp/scheduler.cpp` around lines 320 - 322, The
conversion from microseconds to milliseconds in the timeout calculation can
overflow a 32-bit long when evaluating (usec + 999) on Windows; modify the code
around timeout_ms to perform the add/divide in a wider signed integer (e.g.,
int64_t or long long) by casting usec before adding 999, then divide by 1000 and
clamp the result to the target unsigned long range (or INFINITE) before
static_cast<unsigned long>(); update the code that computes timeout_ms (the
usec, timeout_ms and do_one(...) call) so the intermediate arithmetic cannot
overflow and the final value is safely cast for do_one.
e624e31 to
99cecc4
Compare
Tooling - Add .clang-tidy config and CI step (clang-20, warnings-as-errors) - Add .clang-format and reformat entire codebase - Harden clang-tidy CI step with xargs -r for empty input - Enable performance-noexcept-move-constructor check Code quality - Mark 42 leaf implementation classes final for devirtualization (~8% ping-pong latency improvement, ~4% timer fire rate gain) - Make all move assignment operators noexcept, removing unnecessary cross-context checks - Add missing override on virtual destructors across public headers - Add NOLINTNEXTLINE annotations for intentional value params - Replace ENOTSUP (POSIX-only) with portable std::errc in PKCS#12 stubs - Split acceptor_service out of socket_service.hpp Bug fixes - Unify work tracking to single work_started/work_finished pair, removing redundant on_work_started/on_work_finished virtual methods across all backends (epoll, select, kqueue, IOCP) - Fix cancel_single_op work count imbalance when impl is dying - Inline cancel into close_socket and guard cancel_single_op lifetime with weak_from_this().lock() to prevent use-after-free - Fix IOCP wait_one timeout overflow: widen usec to long long before adding 999 to prevent signed overflow on Windows (32-bit long) Performance - Bypass find_service() mutex in timer constructor by reading the scheduler's cached timer_svc_ pointer directly (22 → 57 Mops/s)
sgerbino
force-pushed
the
pr/clang-tools
branch
from
99cecc4 to
e1172c1
Compare
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