Use caf.net socket API in caf.io#2257
Closed
Neverlord wants to merge 1 commit into
Closed
Conversation
The I/O module predates `caf.net` and still ships several ad-hoc utility classes and functions that are available in `caf.net` with a much cleaner API. In this step, we establish a new dependency from `caf.io` to `caf.net` and replace some redundant legacy socket API in `caf.io` with the new primitives from `caf.net`. The downside to this new dependency is that users can no longer build `caf.io` without OpenSSL. However, keeping the duplicated code and maintaining two sets of socket abstractions just to avoid that one dependency does not seem to be a reasonable trade-off.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates caf.io (and related OpenSSL integration points) from its legacy, duplicated socket utilities to the newer caf.net socket API, making caf.io depend on caf.net and removing the old native_socket layer.
Changes:
- Switched
caf.ionetwork code paths fromio::network::native_socketutilities tonet::socket_idpluscaf::netsocket helpers. - Removed legacy socket abstraction files from
caf.io(native_socket.*,detail/socket_guard.*) and updated build configuration to requireCAF::net. - Updated related OpenSSL integration and documentation (changelog) to reflect the new dependency and API usage.
Reviewed changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| libcaf_openssl/caf/openssl/session.hpp | Replaces legacy native_socket alias with net::socket_id. |
| libcaf_openssl/caf/openssl/middleman_actor.cpp | Migrates OpenSSL middleman socket operations to caf::net helpers and guards. |
| libcaf_io/caf/policy/udp.hpp | Updates UDP policy API to take net::socket_id. |
| libcaf_io/caf/policy/udp.cpp | Switches UDP send/recv plumbing to caf::net socket aliases and error helpers. |
| libcaf_io/caf/policy/tcp.hpp | Updates TCP policy API to take net::socket_id. |
| libcaf_io/caf/policy/tcp.cpp | Switches TCP read/write/accept to caf::net primitives. |
| libcaf_io/caf/io/typed_broker.hpp | Updates typed broker overloads to accept net::socket_id. |
| libcaf_io/caf/io/network/stream_impl.hpp | Updates stream implementation ctor parameter type to net::socket_id. |
| libcaf_io/caf/io/network/stream.hpp | Updates stream ctor signature to use net::socket_id. |
| libcaf_io/caf/io/network/stream.cpp | Migrates shutdown write operation to caf::net. |
| libcaf_io/caf/io/network/scribe_impl.hpp | Updates scribe impl ctor signature to use net::socket_id. |
| libcaf_io/caf/io/network/scribe_impl.cpp | Migrates remote addr/port lookups to caf::net. |
| libcaf_io/caf/io/network/pipe_reader.hpp | Updates pipe reader init signature to use net::socket_id. |
| libcaf_io/caf/io/network/pipe_reader.cpp | Migrates pipe read/shutdown ops to caf::net pipe socket helpers. |
| libcaf_io/caf/io/network/native_socket.hpp | Removes legacy socket API header. |
| libcaf_io/caf/io/network/native_socket.cpp | Removes legacy socket API implementation. |
| libcaf_io/caf/io/network/multiplexer.hpp | Updates multiplexer virtual APIs to use net::socket_id. |
| libcaf_io/caf/io/network/ip_endpoint.cpp | Uses caf::net socket alias types for inet conversions. |
| libcaf_io/caf/io/network/event_handler.hpp | Migrates stored FD type to net::socket_id. |
| libcaf_io/caf/io/network/event_handler.cpp | Migrates socket close + FD flag setup to caf::net. |
| libcaf_io/caf/io/network/doorman_impl.hpp | Updates doorman impl ctor signature to use net::socket_id. |
| libcaf_io/caf/io/network/doorman_impl.cpp | Migrates local addr/port lookups to caf::net. |
| libcaf_io/caf/io/network/default_multiplexer.hpp | Updates FD types and helper declarations to net::socket_id. |
| libcaf_io/caf/io/network/default_multiplexer.cpp | Migrates pipe creation, socket guards, options, and error handling to caf::net. |
| libcaf_io/caf/io/network/datagram_servant_impl.hpp | Updates datagram servant impl ctor signature to use net::socket_id. |
| libcaf_io/caf/io/network/datagram_servant_impl.cpp | Migrates local port lookup to caf::net. |
| libcaf_io/caf/io/network/datagram_handler_impl.hpp | Updates datagram handler impl ctor signature to use net::socket_id. |
| libcaf_io/caf/io/network/datagram_handler.hpp | Migrates send-buffer sizing to caf::net::send_buffer_size. |
| libcaf_io/caf/io/network/datagram_handler.cpp | Migrates UDP connreset + send-buffer sizing to caf::net. |
| libcaf_io/caf/io/network/acceptor_impl.hpp | Updates acceptor impl ctor signature to use net::socket_id. |
| libcaf_io/caf/io/network/acceptor.hpp | Updates acceptor FD/result types to net::socket_id. |
| libcaf_io/caf/io/network/acceptor.cpp | Migrates shutdown ops to caf::net. |
| libcaf_io/caf/io/abstract_broker.hpp | Updates broker socket-based APIs to use net::socket_id. |
| libcaf_io/caf/io/abstract_broker.cpp | Migrates broker socket-based APIs to net::socket_id. |
| libcaf_io/caf/detail/socket_guard.hpp | Removes legacy caf::detail::socket_guard. |
| libcaf_io/caf/detail/socket_guard.cpp | Removes legacy caf::detail::socket_guard implementation. |
| libcaf_io/caf/detail/call_cfun.hpp | Updates syscall helpers to use caf::net error reporting + socket-id validity. |
| libcaf_io/CMakeLists.txt | Adds CAF::net dependency and removes deleted legacy sources. |
| CMakeLists.txt | Enforces that IO cannot be built without NET enabled. |
| CHANGELOG.md | Documents IO module dependency on NET and removal of legacy utilities. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+54
to
+68
| bool tcp::try_accept(net::socket_id& result, net::socket_id fd) { | ||
| auto lg = log::io::trace("fd = {}", fd); | ||
| sockaddr_storage addr; | ||
| std::memset(&addr, 0, sizeof(addr)); | ||
| socket_size_type addrlen = sizeof(addr); | ||
| result = ::accept(fd, reinterpret_cast<sockaddr*>(&addr), &addrlen); | ||
| // note accept4 is better to avoid races in setting CLOEXEC (but not posix) | ||
| if (result == invalid_native_socket) { | ||
| auto err = last_socket_error(); | ||
| if (!would_block_or_temporarily_unavailable(err)) { | ||
| log::io::error("accept failed {}", socket_error_as_string(err)); | ||
| auto accepted = net::accept(net::tcp_accept_socket{fd}); | ||
| if (accepted) { | ||
| if (auto err = net::child_process_inherit(*accepted, false); err.valid()) { | ||
| net::close(*accepted); | ||
| log::io::error("child process inherit failed: {}", err); | ||
| return false; | ||
| } | ||
| result = accepted->id; | ||
| log::io::debug("fd = {} result = {}", fd, result); | ||
| return true; | ||
| } | ||
| child_process_inherit(result, false); | ||
| log::io::debug("fd = {} result = {}", fd, result); | ||
| return true; | ||
| log::io::error("accept on fd {} failed: {}", fd, accepted.error()); | ||
| return false; |
There was a problem hiding this comment.
tcp::try_accept treats any net::accept error as a hard failure (logs an error and returns false). For non-blocking accept sockets, accept can legitimately fail with “would block/try again”; the previous implementation returned true with an invalid result in that case. Consider translating sec::unavailable_or_would_block (and/or checking net::last_socket_error_is_temporary() when net::accept fails) into return true while leaving result as net::invalid_socket_id, and only logging/returning false for non-temporary errors.
| static bool read_datagram(size_t& result, net::socket_id fd, void* buf, | ||
| size_t buf_len, io::network::ip_endpoint& ep); | ||
|
|
||
| /// Reveice a datagram of up to `len` bytes. Larger datagrams are truncated. |
There was a problem hiding this comment.
Spelling: "Reveice" should be "Receive".
Comment on lines
36
to
38
| log::io::warning( | ||
| "recvfrom cut of message, only received buf-len = {} of sres = {} bytes", | ||
| buf_len, sres); |
There was a problem hiding this comment.
Typo in log message: "recvfrom cut of message" reads like it should be "cut off".
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2257 +/- ##
==========================================
+ Coverage 71.32% 71.53% +0.21%
==========================================
Files 639 637 -2
Lines 30736 30639 -97
Branches 3331 3334 +3
==========================================
- Hits 21921 21918 -3
+ Misses 6936 6843 -93
+ Partials 1879 1878 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The I/O module predates
caf.netand still ships several ad-hoc utility classes and functions that are available incaf.netwith a much cleaner API.In this step, we establish a new dependency from
caf.iotocaf.netand replace some redundant legacy socket API incaf.iowith the new primitives fromcaf.net. The downside to this new dependency is that users can no longer buildcaf.iowithout OpenSSL. However, keeping the duplicated code and maintaining two sets of socket abstractions just to avoid that one dependency does not seem to be a reasonable trade-off.