Add socket endpoint query methods (local_endpoint/remote_endpoint) #78
Conversation
📝 WalkthroughWalkthroughAdded noexcept endpoint accessors to public acceptor and socket APIs. Backends (epoll/iocp) cache, expose, set, and clear local/remote endpoints during open/bind/connect/accept/close flows. Some op operator() implementations moved to .cpp to enable endpoint propagation and caching. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Task
participant Socket as Socket (wrapper)
participant Op as ConnectOp
participant Impl as Socket Impl
participant Sys as System Calls
participant Coroutine as Awaiting Coroutine
Client->>Socket: async connect(target_endpoint)
Socket->>Op: create op (store target_endpoint)
Op->>Impl: start connect()
Impl->>Sys: non-blocking connect() syscall
Sys-->>Impl: connect completed (success)
Op->>Sys: getsockname()/getpeername()
Sys-->>Op: local/remote sockaddr
Op->>Impl: set_endpoints(local, target_endpoint)
Impl->>Impl: cache local_endpoint_, remote_endpoint_
Op->>Coroutine: resume with result
sequenceDiagram
participant Server as Acceptor/Task
participant Acceptor as Acceptor (wrapper)
participant AcceptOp as Accept Op
participant Sys as System Calls
participant NewImpl as Accepted Socket Impl
participant Coroutine as Awaiting Coroutine
Server->>Acceptor: async accept()
Acceptor->>AcceptOp: issue accept()
AcceptOp->>Sys: accept() syscall -> new socket fd
Sys-->>AcceptOp: new fd
AcceptOp->>Sys: getsockname(fd)
Sys-->>AcceptOp: local sockaddr
AcceptOp->>Sys: getpeername(fd)
Sys-->>AcceptOp: remote sockaddr
AcceptOp->>NewImpl: set_endpoints(local, remote)
NewImpl->>NewImpl: cache local_endpoint_, remote_endpoint_
AcceptOp->>Coroutine: resume with accepted socket
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
An automated preview of the documentation is available at https://78.corosio.prtest3.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-01-26 16:08:17 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: 2
🤖 Fix all issues with AI agents
In `@src/corosio/src/detail/epoll/sockets.hpp`:
- Around line 138-142: The endpoint members (local_endpoint_, remote_endpoint_)
are accessed concurrently without synchronization; update the implementation to
protect them with a synchronization primitive (e.g., add a std::mutex
endpoints_mutex_ and take a std::lock_guard in set_endpoints(), close_socket(),
and the accessors local_endpoint() and remote_endpoint()), or alternatively
switch the endpoint storage to std::atomic<endpoint> if endpoint is trivially
copyable; modify the methods set_endpoints(), close_socket(), local_endpoint(),
and remote_endpoint() to acquire the chosen lock/atomic access to eliminate data
races and preserve the documented "safe to call concurrently with I/O
operations" contract.
In `@src/corosio/src/detail/iocp/sockets.hpp`:
- Line 53: Add a file-level /* ... */ overview comment right after the header
includes describing the purpose and responsibilities of this header: that it
implements IOCP-based socket and acceptor helpers, manages endpoint caching
state (the endpoint member used for endpoint caching on success), and any
threading/ownership expectations; reference key symbols such as endpoint, the
socket/acceptor classes in this file, and note that the file contains
non-trivial IOCP logic and endpoint caching behavior to help future readers.
| { | ||
| win_socket_impl_internal& internal; | ||
| std::shared_ptr<win_socket_impl_internal> internal_ptr; // Keeps internal alive during I/O | ||
| endpoint target_endpoint; // Stored for endpoint caching on success |
There was a problem hiding this comment.
Add a file-level implementation overview block comment.
This header contains non-trivial IOCP socket/acceptor logic and now adds endpoint caching state; the guidelines call for a brief /* */ overview after the includes. Please add a short summary to keep it aligned. As per coding guidelines, please add the file overview.
💡 Suggested file header overview
`#include` <MSWSock.h>
`#include` <Ws2tcpip.h>
+/*
+ IOCP socket/acceptor internals
+ ------------------------------
+ - Defines overlapped operation state and internal wrappers.
+ - Caches local/remote endpoints for noexcept accessors.
+ - Exposes wrapper overrides used by socket/acceptor classes.
+*/🤖 Prompt for AI Agents
In `@src/corosio/src/detail/iocp/sockets.hpp` at line 53, Add a file-level /* ...
*/ overview comment right after the header includes describing the purpose and
responsibilities of this header: that it implements IOCP-based socket and
acceptor helpers, manages endpoint caching state (the endpoint member used for
endpoint caching on success), and any threading/ownership expectations;
reference key symbols such as endpoint, the socket/acceptor classes in this
file, and note that the file contains non-trivial IOCP logic and endpoint
caching behavior to help future readers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/corosio/src/detail/epoll/sockets.cpp`:
- Around line 52-70: On successful connect in epoll_connect_op::operator() (and
the similar block at lines ~187-202) always cache the remote endpoint
(target_endpoint) even if ::getsockname fails: attempt getsockname and if it
succeeds call set_endpoints(local_ep, target_endpoint) as today, but if
getsockname fails still call set_endpoints with a default-constructed/empty
local endpoint and target_endpoint so the remote endpoint is cached; update both
occurrences that currently skip caching entirely when getsockname fails to
follow this pattern.
🧹 Nitpick comments (1)
src/corosio/src/detail/epoll/sockets.cpp (1)
48-51: Add a file-level implementation overview comment.
This file has non-trivial logic but lacks a/* */overview after the includes. As per coding guidelines, please add a brief high-level description.
MungoG
force-pushed
the
local-endpoints
branch
from
7a5ab7d to
fe41ec9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
example/echo-server/echo_server.cpp (1)
10-18: Add a brief file-level overview comment after includes.This example contains non-trivial async flow; please add a short
/* */overview after the includes. As per coding guidelines, ...src/corosio/src/detail/epoll/sockets.cpp (1)
14-25: Add a brief file-level overview comment after includes.This file is non-trivial and should include a short
/* */overview after the includes. As per coding guidelines, ...
🤖 Fix all issues with AI agents
In `@src/corosio/src/detail/iocp/sockets.cpp`:
- Around line 273-285: The current logic only calls internal.set_endpoints(...)
when getsockname succeeds, so the remote endpoint (target_endpoint) isn't cached
if getsockname fails; change the flow so that when success is true you always
set the remote endpoint by calling internal.set_endpoints(...) or an equivalent
that accepts remote-only, and only populate the local endpoint from getsockname
when that call succeeds (use from_sockaddr_in(local_addr) to build the local
endpoint); update the use of set_endpoints/internal APIs accordingly so remote
(target_endpoint) is stored even if ::getsockname fails.
♻️ Duplicate comments (1)
src/corosio/src/detail/epoll/sockets.cpp (1)
60-70: Cache the remote endpoint even ifgetsocknamefails.Same issue previously noted: remote endpoint remains default when local lookup fails despite a successful connect.
Also applies to: 195-202
MungoG
force-pushed
the
local-endpoints
branch
from
fe41ec9 to
4c871c6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@include/boost/corosio/acceptor.hpp`:
- Around line 289-290: Add a concise contract docstring for the pure virtual
method local_endpoint() in the public header (the declaration "virtual endpoint
local_endpoint() const noexcept = 0;") consistent with other public APIs:
briefly describe what endpoint represents, whether it can throw (it’s noexcept),
what callers can expect if the acceptor is closed or not bound, and any
thread-safety or lifetime notes; place the comment immediately above
acceptor_impl::local_endpoint() in include/boost/corosio/acceptor.hpp using the
same style as neighboring method docs.
♻️ Duplicate comments (2)
src/corosio/src/detail/epoll/sockets.cpp (1)
60-70: Cache remote endpoint even ifgetsocknamefails.
On successful connect, the remote endpoint is already known; skippingset_endpointswhengetsocknamefails leavesremote_endpoint()empty.🐛 Suggested adjustment
- if (::getsockname(fd, reinterpret_cast<sockaddr*>(&local_addr), &local_len) == 0) - { - endpoint local_ep = from_sockaddr_in(local_addr); - static_cast<epoll_socket_impl*>(socket_impl_)->set_endpoints(local_ep, target_endpoint); - } + endpoint local_ep{}; + if (::getsockname(fd, reinterpret_cast<sockaddr*>(&local_addr), &local_len) == 0) + local_ep = from_sockaddr_in(local_addr); + static_cast<epoll_socket_impl*>(socket_impl_)->set_endpoints(local_ep, target_endpoint);- if (::getsockname(fd_, reinterpret_cast<sockaddr*>(&local_addr), &local_len) == 0) - { - local_endpoint_ = detail::from_sockaddr_in(local_addr); - remote_endpoint_ = ep; - } + endpoint local_ep{}; + if (::getsockname(fd_, reinterpret_cast<sockaddr*>(&local_addr), &local_len) == 0) + local_ep = detail::from_sockaddr_in(local_addr); + set_endpoints(local_ep, ep);Also applies to: 195-202
src/corosio/src/detail/iocp/sockets.cpp (1)
273-285: Cache remote endpoint even ifgetsocknamefails.
The remote endpoint is known fromtarget_endpoint, so skippingset_endpointson local query failure leavesremote_endpoint()empty after a successful connect.🐛 Suggested adjustment
- if (::getsockname(internal.native_handle(), - reinterpret_cast<sockaddr*>(&local_addr), &local_len) == 0) - { - endpoint local_ep = from_sockaddr_in(local_addr); - internal.set_endpoints(local_ep, target_endpoint); - } + endpoint local_ep{}; + if (::getsockname(internal.native_handle(), + reinterpret_cast<sockaddr*>(&local_addr), &local_len) == 0) + local_ep = from_sockaddr_in(local_addr); + internal.set_endpoints(local_ep, target_endpoint);
🧹 Nitpick comments (1)
src/corosio/src/detail/epoll/sockets.cpp (1)
48-51: Add a short file-level implementation overview after the includes.
This is a non-trivial implementation file and would benefit from a brief/* */overview describing the epoll socket/acceptor responsibilities and endpoint caching.As per coding guidelines, ...
MungoG
force-pushed
the
local-endpoints
branch
from
4c871c6 to
e96e9e2
Compare
MungoG
force-pushed
the
local-endpoints
branch
from
e96e9e2 to
943e9e1
Compare
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
|
manually rebased/merged. |
2 participants
Implement local_endpoint() and remote_endpoint() methods for the socket class, and local_endpoint() for the acceptor class, following the caching approach recommended in issue #63.
Design:
Implementation:
Tests:
Closes #63
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.