Feat/tcp connecotr hardening#179
Conversation
…rt and connection handling improvements
thaodt
left a comment
thaodt
left a comment
There was a problem hiding this comment.
Approved with some small changes then we're good to go.
…prove time scaling in tests
…atchdog The loopback harness polls two non-terminating embassy-net stacks in the background, so a transport regression or a dropped crossover packet would leave `block_on` pending until the outer CI timeout instead of failing the test. Race the foreground/background `select` against a 20s wall-clock watchdog that re-arms its waker each poll (so the deadline is observed even when the stacks would otherwise park) and panics with a clear message. Addresses review feedback on #179. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ed N=2 path The Embassy loopback concurrency test used two unrelated `TcpListener<1>` on different ports, so it never touched `with_buffers` construction or same-port fan-out. Rework it to stand up one `TcpListener::<2>::with_buffers(...)` on a single port and dial it with two clients. Reaching the N>1 pool from an integration test required a public accept: the `Listener` impl is `N=1`-only and the pooled path (`into_server_futures`/ `serve_socket_slot`) was private, reachable only through `TcpServer<N>` + the AimX session engine. Add `TcpListener::<N>::accept_on(index)` and factor the accept body — previously duplicated between the `N=1` `Listener` impl and the server workers — into a shared `accept_on_slot`. Side benefit: `with_buffers` can now be driven directly instead of only through `TcpServer`. Addresses review feedback on #179. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| pub fn accept_on( | ||
| &self, | ||
| index: usize, | ||
| ) -> impl Future<Output = TransportResult<Box<dyn Connection>>> + Send + '_ { |
There was a problem hiding this comment.
When a checked-out slot has 2 separate tasks waiting on accept_on(index), both call poll_take, but TcpSocketSlot stores only one Option<Waker>, the 2nd registration overwrites the first, so after the socket returns only one task wakes while the other can remain pending forever.
Because this new method takes &self, safe callers can create this state, so enforce one waiter per index or support multiple waiters.
There was a problem hiding this comment.
Right, a &self accept that can touch many slots shouldn't be public. So:
accept_onis now test-only, hidden behind
#[cfg(feature = "_test-embassy-loopback")]+#[doc(hidden)].Listener<1>::acceptcallsaccept_on_slotdirectly and the real pooled
path isTcpServer<N>.
That means nothing we ship can put two waiters on one slot anymore. The only
caller left is the test and it does one accept per slot.
| let mut socket = poll_fn(|cx| slot.poll_take(cx)).await; | ||
| socket.abort(); | ||
| match socket.accept(local_endpoint).await { |
There was a problem hiding this comment.
When an accept_on future is dropped after taking the socket but while TcpSocket::accept is pending - for e.g., when a timeout or shutdown select wins, the async local drops the TcpSocket instead of calling slot.put. The slot remains None, so every later accept on that index waits forever, you may want to retain the socket in a drop guard or otherwise return it on cancellation.
There was a problem hiding this comment.
Right, I reworked it so that a SlotReturn guard holds the socket while accept() runs. On success it hands the socket off. If the accept is dropped first, the guard puts the socket back in the slot instead of losing it. Added cancelled_accept_recycles_socket to cover it.
|
Your are right, but I think this change will lead to a broader api change. Let me check and come back! |
…ng in accept logic
|
Rather than reshaping the public API, I opted to feature-gate I also kept the transport-level fan-out test rather than replacing it with a full |
2 participants
Changes
accept()errors.embassy-net'sTcpSocket::accept()can fail synchronously (a port-0local_endpointrejected asInvalidPort), and theErrarms ofserve_socket_slotand theListener<1>compat path had no await point — a tight loop with zero yields that starves the cooperative executor. Both nowembassy_futures::yield_now().await, degrading a static misconfig to a debuggable warn-loop. New optionalembassy-futuresdep._test-embassy-loopback). The socket pool is welded to a concreteembassy_net::tcp::TcpSocket, so its recycling and waker handoff were review-only. Now exercised over two realembassy-netstacks wired by an in-memory driver-channel crossover: recycle → re-accept, concurrent accept slots, and dialer redial after a failed connect / dropped link. Uses a wall-clock host time driver (the frozen stub clock stalls the delayed-ACK timer and hangs the firstflush()). Kept offembassy-runtimeand out of[dev-dependencies]; wired intomake check.TcpSocketSlotSAFETY comment (dialer/worker/listener-owned, not just "dialer-owned") and documented the intentional double-abort()on the recycle path.aimdb-tcp-connector/CHANGELOG.md(initial) + global changelog index link.