io_object lifecycle through handles#144
Conversation
Remove impl_ and ctx_ from io_object, making h_ (handle) the sole data member. All I/O objects (timer, signal_set, resolver, tcp_socket, tcp_acceptor) now use handles on every platform. Each service implements io_service::construct()/destroy()/open()/close() for lifecycle management. Eliminate all create_impl() indirection — bodies inlined into construct().
The handle-based refactor uses find_service (not use_service), so services must be pre-registered. The IOCP context was missing this, causing "service not installed" on Windows.
handle::~handle() and operator= now call svc_->close() before destroy(), preventing fd leaks if a derived class forgets to close explicitly. The context-only handle/io_object/io_stream constructors were unused and removed.
Without CancelIoEx(), closesocket() does not reliably complete pending overlapped operations with ERROR_OPERATION_ABORTED, causing close-while-reading tests to get the wrong error code.
open() was only meaningful for socket services — timers, signals, resolvers, and acceptors all had empty bodies. tcp_socket::open() now casts directly to the concrete service and calls open_socket(), matching the pattern tcp_acceptor::listen() already uses. close() gets a default empty body since most services (timers, signals, resolvers) don't need it, eliminating boilerplate.
Consistent with kqueue which already uses dispatch_coro for inline completions.
Unify naming: io_object_impl, timer_impl, signal_set_impl, resolver_impl, io_stream_impl, acceptor_impl, and socket_impl all become nested implementation structs.
📝 WalkthroughWalkthroughRefactors io_object ownership to a move-only, handle-centric model: adds nested implementation and io_service types, a handle class, and create_handle(). Replaces create/destroy/release per-backend APIs with construct()/destroy()/close(handle), and updates all platform backends and consumers accordingly. Changes
Sequence Diagram(s)mermaid User->>Context: create_handle(ctx) 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)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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://144.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-15 04:05:36 UTC |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #144 +/- ##
===========================================
- Coverage 81.18% 81.02% -0.17%
===========================================
Files 64 64
Lines 5724 5696 -28
===========================================
- Hits 4647 4615 -32
- Misses 1077 1081 +4
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (11)
src/corosio/src/iocp_context.cpp (1)
28-38: 🛠️ Refactor suggestion | 🟠 MajorAdd implementation comment explaining the Windows-specific service registration pattern.
The service initialization order (scheduler → sockets → acceptor → signals) is correct for the IOCP context. However, this file requires an implementation-level overview comment per the coding guidelines. Unlike the POSIX contexts (epoll, kqueue, select), the Windows IOCP implementation registers a signals service and passes the sockets reference to the acceptor service due to Windows-specific architecture. Add a brief
/* */block comment after the includes explaining this pattern.src/corosio/src/tcp_socket.cpp (1)
85-98:⚠️ Potential issue | 🟡 MinorConsider using
INVALID_SOCKETconstant or a platform-abstracted sentinel instead of magic value~0ull.While
~0ullcast tostd::uintptr_tproduces the correct sentinel value (equivalent toINVALID_SOCKET), the codebase already defines and usesINVALID_SOCKETextensively throughout the IOCP implementation. Using the actual constant or defining a platform-abstracted sentinel (e.g.,platform::invalid_socket()) would improve clarity and consistency with the rest of the Windows socket handling code. This would also align with howis_open()uses~native_handle_type(0)for comparisons.src/corosio/src/detail/kqueue/sockets.hpp (1)
66-72:⚠️ Potential issue | 🟡 MinorStale block comment:
destroy_impl()should bedestroy().Line 68 reads "destroy_impl() removes the shared_ptr from the map" but this method has been renamed to
destroy()in this PR. The select backend's equivalent comment (select/sockets.hpp, line 63) was correctly updated.📝 Suggested fix
- kqueue_socket_service owns all socket impls. destroy_impl() removes the + kqueue_socket_service owns all socket impls. destroy() removes thesrc/corosio/src/detail/epoll/sockets.hpp (1)
73-79:⚠️ Potential issue | 🟡 MinorStale block comment:
destroy_impl()should bedestroy().Same issue as in the kqueue backend — line 75 still references
destroy_impl(). The select backend was correctly updated.📝 Suggested fix
- epoll_socket_service owns all socket impls. destroy_impl() removes the + epoll_socket_service owns all socket impls. destroy() removes thesrc/corosio/src/detail/select/sockets.hpp (1)
68-68:⚠️ Potential issue | 🟡 MinorFix stale comment in kqueue backend: "destroy_impl()" should be "destroy()".
The block comment in
src/corosio/src/detail/kqueue/sockets.hppat line 68 incorrectly referencesdestroy_impl()instead ofdestroy()in the Service Ownership description. The select backend's equivalent comment was correctly updated to usedestroy().include/boost/corosio/resolver.hpp (1)
341-351:⚠️ Potential issue | 🟡 MinorMove assignment differs from tcp_socket in how it handles cleanup.
The docstring states "Cancels any existing operations and transfers ownership," but unlike
tcp_socket::operator=which callsclose()before moving, resolver relies on the implicit destruction of the replaced handle to trigger cleanup. While the handle destructor does callsvc_->close()andsvc_->destroy(), making this implicit cleanup functionally correct, the pattern is inconsistent between similar I/O object types. For clarity and consistency withtcp_socket, consider explicitly callingclose()or documenting why the implicit pattern is preferred here.src/corosio/src/detail/epoll/acceptors.cpp (1)
63-83:⚠️ Potential issue | 🟠 MajorAccepted fd leaks if
construct()throwsstd::bad_alloc.
socket_svc->construct()callsstd::make_sharedwhich can throw. If it does,accepted_fdis never closed — the cleanup at lines 93–102 is not reached because the exception propagates out ofoperator()(). The same issue exists at line 155 in the inline accept path.Consider wrapping the construct + setup block in a try/catch, or use a scope guard on
accepted_fdto ensure it's closed on any exit path.🐛 Proposed fix sketch
if (success && accepted_fd >= 0 && acceptor_impl_) { auto* socket_svc = static_cast<epoll_acceptor_impl*>(acceptor_impl_) ->service().socket_service(); if (socket_svc) { - auto& impl = static_cast<epoll_socket_impl&>(*socket_svc->construct()); + io_object::implementation* raw = nullptr; + try { + raw = socket_svc->construct(); + } catch (...) { + ::close(accepted_fd); + accepted_fd = -1; + *ec_out = make_err(ENOMEM); + success = false; + } + if (!raw) goto cleanup; + auto& impl = static_cast<epoll_socket_impl&>(*raw);src/corosio/src/timer.cpp (1)
49-61:⚠️ Potential issue | 🟡 MinorMove-assign from a moved-from timer will dereference null in
other.context().Line 55 calls
other.context()which ultimately dereferencesother.h_.ctx_. Ifotherwas previously moved-from,ctx_is null. While move-assigning from a moved-from object is arguably a precondition violation, the same pattern insignal_set::operator=(line 888 of signals.cpp) has the same issue. A defensiveif (!other.h_)early-return (or assert) would prevent the crash.src/corosio/src/detail/posix/signals.cpp (1)
882-893:⚠️ Potential issue | 🟡 MinorSame moved-from null dereference risk as
timer::operator=.Line 888 calls
other.context()which dereferencesother.h_.ctx_. Ifotheris moved-from, this is a null dereference. See the same comment ontimer::operator=.src/corosio/src/detail/select/acceptors.cpp (1)
56-101:⚠️ Potential issue | 🟡 MinorAccept success path doesn't handle
construct()failure (out of memory).Line 64:
socket_svc->construct()allocates viastd::make_shared. If this throwsstd::bad_alloc, the exception propagates out ofoperator()()withaccepted_fdstill open, leaking the file descriptor.The kqueue backend (line 112 of
kqueue/acceptors.cpp) has the same issue. Consider wrapping in a try/catch to closeaccepted_fdon allocation failure.src/corosio/src/detail/kqueue/acceptors.cpp (1)
104-136:⚠️ Potential issue | 🔴 CriticalRemove the explicit
::close(accepted_fd)call; letdestroy()handle fd closure.Line 131 closes
accepted_fd, butimpl.fd_still references the same (now-closed) descriptor. Whendestroy(&impl)is called on line 133, it invokesclose_socket()which attempts to closeimpl.fd_again, resulting in a double-close.Fix
if (::setsockopt(accepted_fd, SOL_SOCKET, SO_NOSIGPIPE, &one, sizeof(one)) == -1) { if (ec_out) *ec_out = make_err(errno); - ::close(accepted_fd); - accepted_fd = -1; socket_svc->destroy(&impl); + accepted_fd = -1; if (impl_out) *impl_out = nullptr; }
🧹 Nitpick comments (9)
src/corosio/src/tcp_socket.cpp (1)
39-57: IOCPopen()path has a deep cast chain — consider a comment explaining the layering.The IOCP path (lines 46–49) performs three static_casts to navigate from the abstract handle through
tcp_socket::implementationdown towin_socket_impl::get_internal(). This is correct given the type hierarchy, but the indirection is non-obvious.A brief
// win_sockets creates win_socket_impl which wraps the IOCP-specific internalstyle comment would help future maintainers reason about the cast chain.src/corosio/src/detail/posix/signals.hpp (1)
52-53: Emptypublic:access specifier left behind after removingcreate_impl().The
public:on line 52 has no declarations beforeprotected:on line 53. This is likely a leftover from removing thecreate_impl()pure virtual.🧹 Remove the empty access specifier
{ -public: protected: posix_signals() = default; };src/corosio/src/detail/iocp/signals.hpp (1)
22-24: Duplicate#include <system_error>.Lines 22 and 24 both include
<system_error>.🧹 Remove duplicate include
`#include` "src/detail/intrusive.hpp" `#include` <system_error> -#include <system_error> - `#include` "src/detail/iocp/mutex.hpp"include/boost/corosio/timer.hpp (1)
100-114: Consider adding a brief docstring for the publicimplementationstruct.
implementationis a public nested type in a public header (include/boost/corosio/timer.hpp), in a non-detail namespace. While it's clearly an internal extension point, a one-line summary would satisfy the documentation guidelines for public declarations.As per coding guidelines: "Docstrings are required for all classes and functions in public headers in non-detail namespaces."
📝 Suggested docstring
+ /** Internal timer implementation base with heap-indexed expiry state. */ struct implementation : io_object::implementation {include/boost/corosio/io_object.hpp (1)
158-170:reset()has no safety net if called on a moved-from handle with a non-nullp.If
reset(p)is called on a default-constructed or moved-from handle (wheresvc_isnullptr) withp != nullptr, the handle will store a non-nullimpl_with a nullsvc_, leading to a null-pointer dereference in the destructor or a subsequentreset().Consider adding an assertion or a precondition check:
🛡️ Proposed defensive assertion
void reset(implementation* p) noexcept { if (impl_) { svc_->close(*this); svc_->destroy(impl_); } + // Precondition: if p is non-null, this handle must own a service. + BOOST_ASSERT(!p || svc_); impl_ = p; }src/corosio/src/detail/timer_service.cpp (1)
167-187:implementationname shadows the enclosingtimer::implementationin this TU.The local
struct implementation : timer::implementationin thedetailnamespace works correctly since it's only used internally, but the name collision could confuse maintainers reading the file. The block comment at line 33 and thestructdeclaration make this clear enough, though a brief// detail::implementation — concrete timer implcomment at the declaration would help.src/corosio/src/detail/iocp/sockets.cpp (2)
645-664: Two separate lock acquisitions inconstruct()are intentional but could be consolidated.The lock is released between adding to
socket_list_(line 653) andsocket_wrapper_list_(line 660), keepingnew win_socket_impl(...)outside the critical section. This is a reasonable tradeoff, but since the wrapper allocation is typically fast and holding the lock would simplify reasoning about invariants, consider a single lock scope.
784-803:win_acceptor_service::construct()takes locks onsvc_.mutex_— document the friend relationship dependency.This method accesses
svc_.mutex_,svc_.acceptor_list_, andsvc_.acceptor_wrapper_list_directly, relying on thefriend class win_acceptor_servicedeclaration inwin_sockets(line 620 of sockets.hpp). This tight coupling is acceptable for a platform-specific implementation, but a brief comment noting the friend dependency would help maintainers.src/corosio/src/detail/iocp/sockets.hpp (1)
642-687:win_acceptor_service::shutdown()is empty — relies onwin_sockets::shutdown()for acceptor cleanup.This works because
win_socketsdirectly iteratesacceptor_list_in its ownshutdown(). However, theexecution_contextshuts down services in reverse creation order, andwin_acceptor_serviceis created afterwin_sockets(since its constructor receives awin_sockets&). Sowin_acceptor_service::shutdown()runs first — which is fine because it's a no-op. Thenwin_sockets::shutdown()cleans up everything. Correct ordering, but worth a brief comment in the emptyshutdown()explaining why.📝 Suggested comment
- void shutdown() override {} + // No-op: win_sockets::shutdown() handles acceptor cleanup. + // This service shuts down first (reverse creation order), so + // win_sockets is still alive to perform the actual teardown. + void shutdown() override {}
| implementation& get() const noexcept | ||
| { | ||
| return *static_cast<signal_set_impl*>(impl_); | ||
| return *static_cast<implementation*>(h_.get()); | ||
| } |
There was a problem hiding this comment.
get() dereferences h_.get() without a null check.
If the signal_set has been moved-from, h_.get() may return null, making this UB. Other io_objects (e.g., tcp_acceptor) guard public methods with is_open() before calling get(). The wait_awaitable::await_suspend at line 197 calls s_.get().wait(...) without such a guard. This is likely a pre-existing concern, but worth noting now that the accessor has changed.
|
GCOVR code coverage report https://144.corosio.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-02-15 04:09:03 UTC |
|
GCOVR code coverage report https://144.corosio.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-02-15 04:14:01 UTC |
2 participants
Summary by CodeRabbit
New Features
Refactor