-
Notifications
You must be signed in to change notification settings - Fork 16
Feat/tcp connecotr hardening #179
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
0bac710
1e0a957
7219360
eae447c
2c463d5
73f96c5
83df852
337d202
6f04f68
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| # Changelog - aimdb-tcp-connector | ||
|
|
||
| All notable changes to the `aimdb-tcp-connector` crate will be documented in this file. | ||
|
|
||
| The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), | ||
| and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). | ||
|
|
||
| ## [Unreleased] | ||
|
|
||
| ### Added | ||
|
|
||
| - **New crate — the length-prefixed TCP transport for AimDB remote access (AimX over TCP, refs #121).** Contributes the `Dialer`/`Listener`/`Connection` transport triple plus thin `TcpClient`/`TcpServer` sugar; the AimX codec + dispatch and the runtime-neutral session engines (`run_client`/`serve`) are reused from `aimdb-core`. Every AimX envelope is framed as a `u32` big-endian length prefix. Two runtime halves: | ||
| - **`tokio-runtime`** (std, host/gateway) — TCP transport over `tokio::net`. | ||
| - **`embassy-runtime`** (`no_std + alloc`, MCU) — an explicit pool of caller-buffered `embassy-net` sockets, one accept/session worker per slot (`TcpServer::<N>::with_buffers`), with socket recycling across reconnects. A synchronous `accept()` failure (e.g. a port-0 endpoint rejected as `InvalidPort`) yields instead of spinning the cooperative executor. | ||
| - **Embassy TCP runtime smoke test** (`_test-embassy-loopback`) — exercises the socket pool over two real `embassy-net` stacks wired by an in-memory driver-channel crossover: recycle → re-accept, concurrent accept slots, and dialer redial after a failed connect / dropped link. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,7 @@ use aimdb_core::session::{ | |
| }; | ||
| use aimdb_embassy_adapter::connectors::{EmbassySessionClient, OneShotCell}; | ||
| use aimdb_embassy_adapter::SendFutureWrapper; | ||
| use embassy_futures::yield_now; | ||
| use embassy_net::tcp::TcpSocket; | ||
| use embassy_net::{IpEndpoint, IpListenEndpoint, Stack}; | ||
| use embedded_io_async::Write; | ||
|
|
@@ -114,6 +115,8 @@ impl Connection for TcpConnection { | |
| impl Drop for TcpConnection { | ||
| fn drop(&mut self) { | ||
| if let Some(mut socket) = self.socket.take() { | ||
| // Double abort is intentional: this one resets the link promptly on | ||
| // drop; the next taker re-aborts before reuse for a clean socket. | ||
| socket.abort(); | ||
| if let Some(recycler) = &self.recycler { | ||
| recycler.put(socket); | ||
|
|
@@ -147,8 +150,9 @@ struct TcpSocketSlot { | |
| // SAFETY: single-core cooperative Embassy executor; the socket and stack stay on | ||
| // the same executor task set, and `RefCell` is never borrowed from another core. | ||
| unsafe impl Send for TcpSocketSlot {} | ||
| // SAFETY: same invariant. This is only shared so the dropped connection can | ||
| // return its socket to the dialer-owned slot. | ||
| // SAFETY: same invariant. Shared only so a dropped connection can return its | ||
| // socket to the slot — owned by a dialer, an accept/session worker, or the | ||
| // `Listener` compat path, never touched from more than one at a time. | ||
| unsafe impl Sync for TcpSocketSlot {} | ||
|
|
||
| impl TcpSocketSlot { | ||
|
|
@@ -288,6 +292,23 @@ impl<const N: usize> TcpListener<N> { | |
| } | ||
| } | ||
|
|
||
| /// Accept one connection on pooled socket `index`, recycling that socket | ||
| /// back into its slot when the returned connection drops. | ||
| /// | ||
| /// The [`Listener`] impl (`N = 1`) and the `TcpServer` workers each accept a | ||
| /// single slot at a time; this exposes the same per-slot accept so a caller | ||
| /// can keep several pooled sockets in `accept()` on one endpoint at once | ||
| /// (the concurrent-listener property `with_buffers` exists for). Panics if | ||
| /// `index >= N`. | ||
| pub fn accept_on( | ||
| &self, | ||
| index: usize, | ||
| ) -> impl Future<Output = TransportResult<Box<dyn Connection>>> + Send + '_ { | ||
| let slot = self.slots[index].clone(); | ||
| let local_endpoint = self.local_endpoint; | ||
| SendFutureWrapper(async move { accept_on_slot(&slot, local_endpoint).await }) | ||
| } | ||
|
|
||
| fn into_server_futures( | ||
| self, | ||
| codec: Arc<AimxCodec>, | ||
|
|
@@ -312,22 +333,33 @@ impl<const N: usize> TcpListener<N> { | |
|
|
||
| impl Listener for TcpListener<1> { | ||
| fn accept(&mut self) -> BoxFut<'_, TransportResult<Box<dyn Connection>>> { | ||
| Box::pin(SendFutureWrapper(async move { | ||
| let slot = &self.slots[0]; | ||
| let mut socket = poll_fn(|cx| slot.poll_take(cx)).await; | ||
| Box::pin(self.accept_on(0)) | ||
| } | ||
| } | ||
|
|
||
| /// Take the socket from `slot`, accept one inbound connection on it, and hand | ||
| /// back a recyclable [`TcpConnection`]. Shared by [`TcpListener::accept_on`] and | ||
| /// the per-slot server workers so all pooled accept paths behave identically. | ||
| async fn accept_on_slot( | ||
| slot: &Arc<TcpSocketSlot>, | ||
| local_endpoint: IpListenEndpoint, | ||
| ) -> TransportResult<Box<dyn Connection>> { | ||
| let mut socket = poll_fn(|cx| slot.poll_take(cx)).await; | ||
| socket.abort(); | ||
| match socket.accept(local_endpoint).await { | ||
|
Comment on lines
+347
to
+349
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When an |
||
| Ok(()) => { | ||
| Ok(Box::new(TcpConnection::reusable(socket, slot.clone())) as Box<dyn Connection>) | ||
| } | ||
| Err(_) => { | ||
| socket.abort(); | ||
| match socket.accept(self.local_endpoint).await { | ||
| Ok(()) => { | ||
| Ok(Box::new(TcpConnection::reusable(socket, slot.clone())) | ||
| as Box<dyn Connection>) | ||
| } | ||
| Err(_) => { | ||
| socket.abort(); | ||
| slot.put(socket); | ||
| Err(TransportError::Io) | ||
| } | ||
| } | ||
| })) | ||
| slot.put(socket); | ||
| // `accept()` can fail synchronously (e.g. port-0 `InvalidPort`); | ||
| // without this await the caller re-enters `accept()` immediately with | ||
| // no yield point, starving the executor. Yield so a misconfig | ||
| // warn-loops instead of hanging. | ||
| yield_now().await; | ||
| Err(TransportError::Io) | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -339,18 +371,10 @@ async fn serve_socket_slot( | |
| config: SessionConfig, | ||
| ) { | ||
| loop { | ||
| let mut socket = poll_fn(|cx| slot.poll_take(cx)).await; | ||
| socket.abort(); | ||
| match socket.accept(local_endpoint).await { | ||
| Ok(()) => { | ||
| let conn = | ||
| Box::new(TcpConnection::reusable(socket, slot.clone())) as Box<dyn Connection>; | ||
| run_session(conn, codec.as_ref(), dispatch.as_ref(), &config).await; | ||
| } | ||
| Err(_) => { | ||
| socket.abort(); | ||
| slot.put(socket); | ||
| } | ||
| // `accept_on_slot` already yields on the synchronous-failure path, so a | ||
| // misconfig warn-loops here instead of starving the executor. | ||
| if let Ok(conn) = accept_on_slot(&slot, local_endpoint).await { | ||
| run_session(conn, codec.as_ref(), dispatch.as_ref(), &config).await; | ||
| } | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a checked-out slot has 2 separate tasks waiting on
accept_on(index), both callpoll_take, butTcpSocketSlotstores only oneOption<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.