From 27db35a8a769c0a7bba7f96a25c388f9a59eb81b Mon Sep 17 00:00:00 2001 From: Justin Kovacich Date: Wed, 17 Jun 2026 09:43:27 -0400 Subject: [PATCH 01/12] docs: implementation plan for PR 2 (#125 client async-state reduction) 8 TDD tasks: BufferPool/BufferLease primitive + BufferProvider trait (mirroring the channel-pool machinery), thread the provider through ClientDeps -> BindDispatch -> bind_*, move per-socket buffers out of the spawned loop futures into caller-sized pooled storage, kill the second E2E scratch buffer, and flatten handle_control_message. Every memory change is gated on the PR-0 future-size witnesses. Stacked on PR1. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...17-pr2-125-client-async-state-reduction.md | 765 ++++++++++++++++++ 1 file changed, 765 insertions(+) create mode 100644 docs/simple_someip/plans/2026-06-17-pr2-125-client-async-state-reduction.md diff --git a/docs/simple_someip/plans/2026-06-17-pr2-125-client-async-state-reduction.md b/docs/simple_someip/plans/2026-06-17-pr2-125-client-async-state-reduction.md new file mode 100644 index 00000000..3dc80182 --- /dev/null +++ b/docs/simple_someip/plans/2026-06-17-pr2-125-client-async-state-reduction.md @@ -0,0 +1,765 @@ +# PR 2 — #125 Client Async-State Reduction Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Move the client's per-socket `[u8; UDP_BUFFER_SIZE]` buffers out of the spawned socket-loop futures and into caller-sized pooled storage, and flatten the control-message handler, so the client's Embassy-arena (TaskStorage) footprint drops and becomes consumer-sized — closing the client half of issue #125. + +**Architecture:** Three moves. (1) Add a `BufferPool` static-storage primitive and a `BufferProvider` trait that mirror the existing channel-pool machinery (`OneshotPool`/`MpscPool` + `ChannelFactory`); a claim returns a RAII `BufferLease` that derefs to `&'static mut [u8]` and returns its slot on drop. (2) Thread a `BufferProvider` through `ClientDeps` → `BindDispatch` → `SocketManager::bind_*`; the socket loop receives its buffer by slice instead of owning a stack array, so the buffer lives in consumer `.bss` (or the tokio heap pool), not in the future. (3) Flatten `handle_control_message` into a synchronous decode/decide returning a small action value, with awaits hoisted to shallow helpers — kept only where the PR-0 witnesses show it moves the number. + +**Tech Stack:** Rust (edition 2024, crate stays stable-buildable), `heapless`, `embassy-sync` (bare-metal channel backend), `critical-section` (no_std slot synchronization), `tokio` (std path only), cargo, `cargo-nextest`. + +**Spec:** `docs/simple_someip/plans/2026-06-09-phase22-125-memory-reduction-design.md` (PR 2 section) and its rev-2 caller-sized-buffers decision. + +## Global Constraints + +- **Crate stays stable-buildable.** No nightly-only features in `simple-someip` itself. (CI may use nightly only for `-Zprint-type-sizes` / `-Zbuild-std` measurement jobs.) +- **Wire format untouched.** No change to any byte emitted or parsed. +- **Public tokio/std API unchanged.** Callers using the tokio-defaulted `ClientDeps` constructor see no signature change; the buffer pool is provisioned internally (8 × `UDP_BUFFER_SIZE`). +- **Only these behavioral changes are allowed** (all from the design doc's PR-2 section; everything else is behavior-preserving): + - (a) An inbound datagram larger than the claimed receive buffer is **dropped with a log**, not truncated/panicked. + - (b) The oversize-send rejection compares against the **claimed buffer length** (`buf.len()`), not the `UDP_BUFFER_SIZE` constant. +- **Existing suite stays green on every commit** (~543 tests as of #114 plus #124's additions; the `client,bare_metal` no-alloc witness and the embassy-net loopback live-wire test included). +- **Every memory change is validated against PR-0's future-size witnesses** (`tests/bare_metal_e2e.rs`). A change that does not move a witness number is dropped, not merged on faith. +- **No `&'static mut` aliasing.** A buffer slot is handed out to exactly one lease at a time; the pool enforces this and is covered by a witness test. +- **Exact constants (copy verbatim):** `UDP_BUFFER_SIZE = 1500` (`src/lib.rs:158`); `UNICAST_SOCKETS_CAP = 8` (`src/client/inner.rs:40`). + +--- + +## File Structure + +| File | Action | Responsibility | +|---|---|---| +| `src/static_channels/buffer_pool.rs` | Create | `BufferPool` static primitive + `BufferLease` RAII handle (claim/release, no aliasing). | +| `src/static_channels/mod.rs` | Modify | `mod buffer_pool; pub use buffer_pool::{BufferPool, BufferLease};` | +| `src/transport.rs` | Modify | `BufferProvider` trait (`claim(&self) -> Option`), next to the `*Pooled` traits. | +| `src/client/socket_manager.rs` | Modify | `socket_loop_future` takes the buffer by lease; oversize-send check keys off `buf.len()`; inbound-oversize drop+log; `bind_*` claim a buffer before spawn. | +| `src/client/bind_dispatch.rs` | Modify | Thread the `BufferProvider` from `SpawnerDispatch` into the `SocketManager::bind_*` calls. | +| `src/client/inner.rs` | Modify | Hold the provider; flatten `handle_control_message` into a sync decide + hoisted awaits. | +| `src/client/mod.rs` | Modify | Add `buffer_provider` field/generic to `ClientDeps`; rewrite the `:1-30` memory-footprint doc. | +| `src/tokio_transport.rs` | Modify | Heap-backed `BufferProvider` (leak a `[u8; UDP_BUFFER_SIZE] × 8` store) wired into the tokio-defaulted `ClientDeps` constructor. | +| `tests/buffer_pool.rs` | Create | Unit + witness tests for claim-to-exhaustion, release-on-drop, no double-claim. | +| `tests/bare_metal_e2e.rs` | Modify | Extend/retighten the future-size witnesses after extraction. | + +--- + +### Task 1: `BufferPool` static primitive + `BufferLease` + +**Files:** +- Create: `src/static_channels/buffer_pool.rs` +- Modify: `src/static_channels/mod.rs` +- Test: `tests/buffer_pool.rs` + +**Interfaces:** +- Produces: + - `pub struct BufferPool` with `pub const fn new() -> Self` and `pub fn claim(&'static self) -> Option`. + - `pub struct BufferLease { /* private */ }` implementing `Deref`, `DerefMut`, `Drop` (returns the slot), and `Send`. + +- [ ] **Step 1: Write the failing test** + +```rust +// tests/buffer_pool.rs +use simple_someip::static_channels::BufferPool; + +static POOL: BufferPool<2, 4> = BufferPool::new(); + +#[test] +fn claim_returns_distinct_zeroed_slices_until_exhausted() { + let mut a = POOL.claim().expect("slot 0"); + let b = POOL.claim().expect("slot 1"); + assert_eq!(a.len(), 4); + assert_eq!(&*b, &[0u8; 4]); // freshly handed-out slot is zeroed + a[0] = 0xAB; // writable + assert_eq!(a[0], 0xAB); + assert!(POOL.claim().is_none(), "pool of 2 must refuse a 3rd claim"); +} + +#[test] +fn dropping_a_lease_returns_its_slot() { + let a = POOL.claim().expect("slot"); + drop(a); + assert!(POOL.claim().is_some(), "slot must be reusable after the lease drops"); +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `cargo test --test buffer_pool` +Expected: FAIL — `BufferPool` is not found / unresolved import. + +- [ ] **Step 3: Write the implementation** + +```rust +// src/static_channels/buffer_pool.rs +//! Fixed-capacity pool of `&'static mut [u8]` buffers with claim/release +//! semantics, mirroring the channel pools in this module. A `BufferPool` +//! is declared as a `static` by the consumer; each `claim()` hands out one +//! slot as a [`BufferLease`] that returns the slot to the pool on drop. +//! +//! Synchronization uses `critical-section` so the same code is valid on the +//! bare-metal (single-core, no atomics-guarantee) target and on std. + +use core::cell::UnsafeCell; +use core::ops::{Deref, DerefMut}; + +/// Backing storage for a pool: `SLOTS` independent `LEN`-byte buffers plus a +/// claimed-flag per slot. +pub struct BufferPool { + // `UnsafeCell` because `claim()` hands out `&'static mut` into this store; + // the `claimed` flags guarantee at most one live `&mut` per slot. + store: UnsafeCell<[[u8; LEN]; SLOTS]>, + claimed: UnsafeCell<[bool; SLOTS]>, +} + +// SAFETY: all access to `store`/`claimed` is funneled through a +// `critical_section::with`, which provides mutual exclusion on the targets +// we support; a slot is only aliased once (its `claimed` flag gates it). +unsafe impl Sync for BufferPool {} + +impl BufferPool { + #[must_use] + pub const fn new() -> Self { + Self { + store: UnsafeCell::new([[0u8; LEN]; SLOTS]), + claimed: UnsafeCell::new([false; SLOTS]), + } + } + + /// Claim a free slot, or `None` if all `SLOTS` are in use. The returned + /// buffer is zeroed before hand-out so a reused slot never leaks the + /// previous tenant's bytes. + pub fn claim(&'static self) -> Option { + critical_section::with(|_| { + // SAFETY: inside the critical section we hold exclusive access to + // both arrays; we take a raw pointer and only form one `&mut` for + // the chosen, not-yet-claimed slot. + let claimed = unsafe { &mut *self.claimed.get() }; + let idx = claimed.iter().position(|&c| !c)?; + claimed[idx] = true; + let store = unsafe { &mut *self.store.get() }; + let slot: &'static mut [u8; LEN] = unsafe { &mut *(&mut store[idx] as *mut [u8; LEN]) }; + slot.fill(0); + Some(BufferLease { + buf: slot.as_mut_slice(), + claimed_flag: claimed.as_mut_ptr(), + idx, + }) + }) + } +} + +impl Default for BufferPool { + fn default() -> Self { + Self::new() + } +} + +/// RAII handle to one claimed buffer. Derefs to the `&'static mut [u8]`; +/// returns the slot to its pool on drop. +pub struct BufferLease { + buf: &'static mut [u8], + claimed_flag: *mut bool, + idx: usize, +} + +// SAFETY: `BufferLease` owns exclusive access to its slot (gated by the +// pool's `claimed` flag) and the backing store is `'static`. +unsafe impl Send for BufferLease {} + +impl Deref for BufferLease { + type Target = [u8]; + fn deref(&self) -> &[u8] { + self.buf + } +} + +impl DerefMut for BufferLease { + fn deref_mut(&mut self) -> &mut [u8] { + self.buf + } +} + +impl Drop for BufferLease { + fn drop(&mut self) { + critical_section::with(|_| { + // SAFETY: `claimed_flag` points into the owning pool's `'static` + // `claimed` array; only this lease writes `idx`'s flag. + unsafe { + *self.claimed_flag.add(self.idx) = false; + } + }); + } +} +``` + +```rust +// src/static_channels/mod.rs — add near the other `mod`/`pub use` lines +mod buffer_pool; +pub use buffer_pool::{BufferLease, BufferPool}; +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `cargo test --test buffer_pool` +Expected: PASS (both tests). + +- [ ] **Step 5: Confirm no_std-clean and lint-clean** + +Run: `cargo clippy -p simple-someip --no-default-features --features client,bare_metal -- -D warnings -D clippy::pedantic` +Expected: no warnings. (`critical-section` is already a transitive dep via `embassy-sync` per `Cargo.toml`; if the bare-metal build cannot find it, add `critical-section = { version = "1", optional = true }` and include it in the `bare_metal` feature.) + +- [ ] **Step 6: Commit** + +```bash +git add src/static_channels/buffer_pool.rs src/static_channels/mod.rs tests/buffer_pool.rs +git commit -m "feat(static_channels): BufferPool + BufferLease claim/release primitive (#125)" +``` + +--- + +### Task 2: `BufferProvider` trait + static & tokio impls + +**Files:** +- Modify: `src/transport.rs` (next to `OneshotPooled`/`BoundedPooled`, ~`:1342`) +- Modify: `src/tokio_transport.rs` (after the `TokioChannels` `*Pooled` impls, ~`:550`) +- Test: `tests/buffer_pool.rs` + +**Interfaces:** +- Consumes: `BufferLease`, `BufferPool` (Task 1). +- Produces: + - `pub trait BufferProvider: Clone + Send + Sync + 'static { fn claim(&self) -> Option; }` + - `pub struct StaticBufferProvider(pub &'static BufferPool);` impl `BufferProvider`. + - `pub struct TokioBufferProvider;` impl `BufferProvider` (heap-backed, `UDP_BUFFER_SIZE`-sized). + +- [ ] **Step 1: Write the failing test** + +```rust +// tests/buffer_pool.rs (append) +use simple_someip::static_channels::{BufferPool, BufferLease}; +use simple_someip::transport::{BufferProvider, StaticBufferProvider}; + +static PROV_POOL: BufferPool<2, 8> = BufferPool::new(); + +#[test] +fn static_provider_claims_through_a_shared_pool() { + let prov = StaticBufferProvider(&PROV_POOL); + let _a = prov.claim().expect("first"); + let _b = prov.claim().expect("second"); + assert!(prov.claim().is_none(), "provider exposes the pool's capacity"); +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `cargo test --test buffer_pool static_provider_claims_through_a_shared_pool` +Expected: FAIL — `BufferProvider` / `StaticBufferProvider` unresolved. + +- [ ] **Step 3: Write the trait and static impl** + +```rust +// src/transport.rs — near the *Pooled traits +use crate::static_channels::{BufferLease, BufferPool}; + +/// Source of `&'static mut [u8]` receive/scratch buffers for the client's +/// socket loops. Mirrors [`ChannelFactory`]'s role for channels: the +/// bare-metal path is backed by a consumer-declared `static BufferPool`; +/// the tokio path is heap-backed and provisioned internally. +pub trait BufferProvider: Clone + Send + Sync + 'static { + /// Claim one buffer, or `None` when the pool is exhausted. + fn claim(&self) -> Option; +} + +/// `BufferProvider` backed by a `'static` [`BufferPool`] (bare-metal path). +#[derive(Clone, Copy, Debug)] +pub struct StaticBufferProvider( + pub &'static BufferPool, +); + +impl BufferProvider + for StaticBufferProvider +{ + fn claim(&self) -> Option { + self.0.claim() + } +} +``` + +```rust +// src/tokio_transport.rs — heap-backed provider +use crate::static_channels::{BufferLease, BufferPool}; +use crate::transport::BufferProvider; +use crate::UDP_BUFFER_SIZE; + +/// Tokio-path buffer provider: a single leaked `BufferPool` sized at +/// `UNICAST_SOCKETS_CAP + 1` × `UDP_BUFFER_SIZE` (one per possible socket +/// plus discovery). Leaking is fine — a client process holds one for its +/// lifetime; the API hides it entirely from callers. +#[derive(Clone, Copy, Debug)] +pub struct TokioBufferProvider(&'static BufferPool<9, UDP_BUFFER_SIZE>); + +impl TokioBufferProvider { + #[must_use] + pub fn new() -> Self { + Self(Box::leak(Box::new(BufferPool::new()))) + } +} + +impl Default for TokioBufferProvider { + fn default() -> Self { + Self::new() + } +} + +impl BufferProvider for TokioBufferProvider { + fn claim(&self) -> Option { + self.0.claim() + } +} +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `cargo test --test buffer_pool static_provider_claims_through_a_shared_pool` +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add src/transport.rs src/tokio_transport.rs tests/buffer_pool.rs +git commit -m "feat(transport): BufferProvider trait + static/tokio impls (#125)" +``` + +--- + +### Task 3: `socket_loop_future` consumes the buffer; oversize behaviors + +**Files:** +- Modify: `src/client/socket_manager.rs:551` (signature), `:569` (drop local `buf`), `:447-458` (oversize-send check), the receive path (inbound-oversize drop+log) +- Test: `tests/bare_metal_e2e.rs` (a focused inbound-oversize test) + +**Interfaces:** +- Consumes: `BufferLease` (Task 1). +- Produces: `socket_loop_future(socket, rx_tx, tx_rx, e2e_registry, buf: BufferLease)` — the buffer is now an explicit parameter. + +- [ ] **Step 1: Write the failing test** (inbound datagram larger than the claimed buffer is dropped with a log, loop survives) + +```rust +// tests/bare_metal_e2e.rs (new test; reuse the file's existing harness types) +#[tokio::test] +async fn inbound_datagram_larger_than_claimed_buffer_is_dropped_not_fatal() { + // Claim a deliberately tiny 8-byte buffer for the loop, deliver a 64-byte + // datagram, then deliver a valid small one. The oversized datagram must be + // dropped (no panic, loop still running) and the valid one delivered. + let outcome = run_socket_loop_with_buffer_len(8, &[ + Datagram::raw(vec![0xFF; 64]), // oversized -> dropped + Datagram::valid_small(), // must still arrive + ]) + .await; + assert_eq!(outcome.delivered.len(), 1, "only the in-budget datagram is delivered"); + assert!(outcome.loop_alive, "loop must survive an oversized datagram"); +} +``` + +*(If `run_socket_loop_with_buffer_len` / `Datagram` helpers don't exist, add a thin harness in this test module that builds a `SocketManager` over a mock `TransportSocket` whose `recv` yields the scripted datagrams and a `BufferPool<1, 8>` for the lease. Model it on the existing `future_size_witness_bare_metal_channels` setup at `tests/bare_metal_e2e.rs:600`.)* + +- [ ] **Step 2: Run test to verify it fails** + +Run: `cargo test --features client,server,bare_metal --test bare_metal_e2e inbound_datagram_larger` +Expected: FAIL — signature mismatch (`socket_loop_future` takes no buffer) / helper missing. + +- [ ] **Step 3: Change the signature and drop the local array** + +```rust +// src/client/socket_manager.rs:551 — add the buffer parameter +#[allow(clippy::too_many_lines)] +async fn socket_loop_future( + socket: T, + rx_tx: C::BoundedSender, Error>, 16>, + mut tx_rx: C::BoundedReceiver, 16>, + e2e_registry: R, + mut buf: crate::static_channels::BufferLease, // ← was `let mut buf = [0u8; UDP_BUFFER_SIZE];` +) where + T: TransportSocket + 'static, + R: E2ERegistryHandle, +{ + const MAX_CONSECUTIVE_RECV_ERRORS: u32 = 16; + let mut consecutive_recv_errors: u32 = 0; + // (delete the old `let mut buf = [0u8; UDP_BUFFER_SIZE];` at :569) +``` + +- [ ] **Step 4: Inbound-oversize drop+log in the receive path** + +In the receive arm (where `socket.recv(&mut buf)` is awaited), the transport already truncates to `buf.len()`; add the guard that a datagram reported larger than `buf.len()` is dropped with a log rather than parsed from a truncated buffer: + +```rust +// receive path, after a successful recv reporting `n` bytes: +let n = match socket.recv(&mut buf).await { + Ok(n) => n, + Err(e) => { /* existing consecutive-error handling, unchanged */ } +}; +if n > buf.len() { + crate::log::warn!( + "inbound datagram ({n} B) exceeds claimed buffer ({} B); dropping", + buf.len() + ); + continue; +} +let datagram = &buf[..n]; +// ... existing parse/forward of `datagram`, unchanged +``` + +- [ ] **Step 5: Oversize-send check keys off `buf.len()`** + +```rust +// src/client/socket_manager.rs:447 — was `if required > UDP_BUFFER_SIZE {` +if required > buf.len() { + warn!( + "outgoing message size {required} exceeds claimed buffer ({}); rejecting with Capacity(\"udp_buffer\")", + buf.len() + ); + return Err(Error::Capacity("udp_buffer")); +} +``` + +For the E2E `protected` scratch at `:632` (`let mut protected = [0u8; UDP_BUFFER_SIZE];`): leave it for **Task 5** (measurement-gated). For now, keep it as a local so this task stays focused on the receive buffer + the two oversize behaviors. + +- [ ] **Step 6: Run the focused test + the full client/server suite** + +Run: `cargo test --features client,server,bare_metal --test bare_metal_e2e inbound_datagram_larger` +Expected: PASS. +Run: `cargo nextest run --no-default-features --features client-tokio,server-tokio` +Expected: all existing client/server tests still green (behavior-preserving except the two allowed changes). + +- [ ] **Step 7: Commit** + +```bash +git add src/client/socket_manager.rs tests/bare_metal_e2e.rs +git commit -m "feat(client): socket loop receives buffer by lease; oversize drop/reject on buf.len() (#125)" +``` + +--- + +### Task 4: Thread `BufferProvider` through deps → bind → spawn + +**Files:** +- Modify: `src/client/mod.rs:278-296` (`ClientDeps` gains a `buffer_provider` + generic `BP`), and the tokio-defaulted constructor +- Modify: `src/client/bind_dispatch.rs:34-117` (`BindDispatch` + `SpawnerDispatch` carry/forward the provider) +- Modify: `src/client/socket_manager.rs:248-249,355-397` (claim a buffer before spawn; pass it into `socket_loop_future`) +- Modify: `src/client/inner.rs` (store the provider; nothing extra at eviction — release is RAII on loop exit) +- Test: `tests/bare_metal_e2e.rs` (bind-to-capacity claims/releases) + +**Interfaces:** +- Consumes: `BufferProvider` (Task 2), `socket_loop_future(.., buf)` (Task 3). +- Produces: `ClientDeps` with field `pub buffer_provider: BP`. + +- [ ] **Step 1: Write the failing test** (binding N unicast sockets claims N buffers; closing a socket releases its buffer) + +```rust +// tests/bare_metal_e2e.rs +#[tokio::test] +async fn each_bound_socket_claims_one_buffer_and_releases_on_close() { + // Pool with exactly 2 slots; provider shared into ClientDeps. + static POOL: simple_someip::static_channels::BufferPool<2, 1500> = + simple_someip::static_channels::BufferPool::new(); + let provider = simple_someip::transport::StaticBufferProvider(&POOL); + + let client = build_test_client_with_buffer_provider(provider).await; + client.bind_unicast_for_test(40000).await.expect("1st bind claims slot 0"); + client.bind_unicast_for_test(40001).await.expect("2nd bind claims slot 1"); + // 3rd bind must fail: pool exhausted. + let third = client.bind_unicast_for_test(40002).await; + assert!(matches!(third, Err(Error::Capacity("udp_buffer")))); + + client.close_unicast_for_test(40000).await; // releases slot 0 + client.bind_unicast_for_test(40002).await.expect("slot freed -> bind succeeds"); +} +``` + +*(`build_test_client_with_buffer_provider` / `bind_unicast_for_test` / `close_unicast_for_test` are thin test shims over `new_with_deps` + the existing control-message API; build them in the test module mirroring `tests/bare_metal_client.rs:257`.)* + +- [ ] **Step 2: Run test to verify it fails** + +Run: `cargo test --features client,server,bare_metal --test bare_metal_e2e each_bound_socket_claims` +Expected: FAIL — `ClientDeps` has no `buffer_provider`. + +- [ ] **Step 3: Add the provider to `ClientDeps`** + +```rust +// src/client/mod.rs:278 — add generic BP and field +pub struct ClientDeps +where + F: TransportFactory, + Tm: Timer, + R: E2ERegistryHandle, + I: InterfaceHandle, + BP: crate::transport::BufferProvider, +{ + pub factory: F, + pub timer: Tm, + pub e2e_registry: R, + pub interface: I, + pub spawner: Sp, + /// Source of `&'static mut [u8]` socket-loop buffers (caller-sized on + /// bare-metal; internally heap-provisioned on the tokio path). + pub buffer_provider: BP, +} +``` + +Update `Client::new_with_deps` and the `Inner` constructor to store `buffer_provider` (alongside `dispatch`/`spawner`). The tokio-defaulted constructor (the phase-21 `ClientDeps`/`Deps` convenience builder) sets `buffer_provider: TokioBufferProvider::new()` so tokio callers are unaffected. + +- [ ] **Step 4: Forward the provider through `bind_dispatch` and claim at bind** + +In `SpawnerDispatch` add a `buffer_provider: BP` field; in its `BindDispatch::bind_unicast`/`bind_discovery` impls (`src/client/bind_dispatch.rs:87-117`), claim a buffer and pass it to the `SocketManager::bind_*` calls. In `SocketManager::bind_with_transport` / `bind_discovery_seeded_with_transport` (`socket_manager.rs:355-397` / `:248`), accept the lease and forward it into the spawned loop: + +```rust +// src/client/bind_dispatch.rs — bind_unicast impl +fn bind_unicast(&self, port: u16, e2e_registry: R) -> impl Future, Error>> + '_ { + async move { + let buf = self + .buffer_provider + .claim() + .ok_or(Error::Capacity("udp_buffer"))?; // pool exhausted -> typed error + SocketManager::::bind_with_transport( + &self.factory, + &self.spawner, + port, + e2e_registry, + buf, // ← moved into the loop + ) + .await + } +} +``` + +```rust +// src/client/socket_manager.rs:389 — pass buf into the spawned future +let fut = Self::socket_loop_future(socket, rx_tx, tx_rx, e2e_registry, buf); +spawner.spawn(fut); +``` + +Release needs **no** new code: the lease is owned by the loop future, so when a socket closes and the loop returns, the future drops, dropping the lease and freeing the slot — the eviction at `inner.rs:639` already removes the handle. (Add a one-line comment there noting the buffer is released via the loop future's drop.) + +- [ ] **Step 5: Run the test + the no-alloc witness** + +Run: `cargo test --features client,server,bare_metal --test bare_metal_e2e each_bound_socket_claims` +Expected: PASS. +Run: `cargo test --features client,bare_metal --test no_alloc_witness` +Expected: PASS — the buffer pool introduces no allocator symbols on the client path. + +- [ ] **Step 6: Commit** + +```bash +git add src/client/mod.rs src/client/bind_dispatch.rs src/client/socket_manager.rs src/client/inner.rs tests/bare_metal_e2e.rs +git commit -m "feat(client): thread BufferProvider through deps/bind; release on loop drop (#125)" +``` + +--- + +### Task 5: E2E scratch buffer — measurement-gated + +**Files:** +- Modify: `src/client/socket_manager.rs:629-632` (the `protected` E2E buffer) +- Reference: `tests/bare_metal_e2e.rs` witnesses + +**Interfaces:** consumes the per-loop receive `BufferLease` (Task 3/4). + +The design doc leaves this as measure-and-decide. Two candidate implementations; pick by the witness numbers. + +- [ ] **Step 1: Capture the current `bm_client_socket_loop` witness number** + +Run: `cargo test --features client,server,bare_metal --test bare_metal_e2e future_size_witness_bare_metal_channels -- --nocapture | grep FUTURE_SIZE` +Record the `bm_client_socket_loop` value (post-Task-4 baseline). + +- [ ] **Step 2: Implement Option A — reuse the loop's receive buffer for E2E protect** + +The receive buffer and the E2E send-scratch are never live at the same instant (receive and send are distinct `select` arms processed one at a time). Reuse the single leased `buf` for protection instead of a second 1500-byte array: + +```rust +// src/client/socket_manager.rs:629 — was `let mut protected = [0u8; UDP_BUFFER_SIZE];` +// Reuse the loop's leased buffer; nothing inbound is pending across a send. +let protected = &mut buf[..]; +let protected_len = e2e_registry.protect(&key, &outgoing, protected)?; // adjust to actual protect() sig +socket.send_to(&protected[..protected_len], dest).await?; +``` + +If `protect()` requires the input and output to be disjoint slices (it may, depending on its signature), fall back to **Option B**: claim a second `BufferLease` from the same provider at send time (`provider.claim()`), use it for `protected`, and let it drop at the end of the send arm. Decide A-vs-B by which keeps `bm_client_socket_loop` smaller in the witness. + +- [ ] **Step 3: Re-measure and verify the suite** + +Run: `cargo test --features client,server,bare_metal --test bare_metal_e2e -- --nocapture | grep FUTURE_SIZE` +Expected: `bm_client_socket_loop` ≤ the Step-1 number (strictly smaller if the second array was the dominant term). +Run: `cargo nextest run --no-default-features --features client-tokio,server-tokio` +Expected: green, including any E2E send test. + +- [ ] **Step 4: Commit** + +```bash +git add src/client/socket_manager.rs +git commit -m "perf(client): eliminate the second E2E scratch buffer from the socket loop (#125)" +``` + +--- + +### Task 6: Flatten `handle_control_message` (secondary, keep only if it moves the number) + +**Files:** +- Modify: `src/client/inner.rs:661-970` (`handle_control_message`), `:1034-1235` (`run_future`) +- Test: existing control-message tests + the run-future witness + +**Interfaces:** introduces a private `enum ControlAction` describing the post-decode work; awaits are hoisted to `run_future`'s top level. + +- [ ] **Step 1: Capture the current `bm_client_run_future` witness number** + +Run: `cargo test --features client,server,bare_metal --test bare_metal_e2e future_size_witness_bare_metal_channels -- --nocapture | grep bm_client_run_future` +Record the value. + +- [ ] **Step 2: Add the action enum and split decode from await** + +Split the 10-variant `match` (`inner.rs:661-970`) into (i) a synchronous `decide_control_action(&mut self, msg) -> ControlAction` that does all the lock/registry mutation and returns a small value, and (ii) shallow `async` helpers invoked from `run_future` for the arms that must await (`bind_*`, `SendToService`, `SendSD`, `Subscribe`, `QueryRebootFlag`): + +```rust +// src/client/inner.rs — new private enum +enum ControlAction { + None, + BindDiscovery(C::OneshotSender>), + BindUnicastThenSend { service_id: u16, instance_id: u16, message: /*…*/, /* + responders */ }, + SendSd { target: SocketAddrV4, header: SdHeader, response: /*…*/ }, + Subscribe { /* the Subscribe fields */ }, + QueryRebootFlag(C::OneshotSender>), + // …one variant per arm that currently awaits +} +``` + +```rust +// run_future loop tail (was `self.handle_control_message().await;` at :1234) +let action = self.decide_control_action(); // synchronous; drops all locals before awaiting +match action { + ControlAction::None => {} + ControlAction::BindDiscovery(resp) => { + let r = self.bind_discovery().await; // shallow, single await + let _ = resp.send(r); + } + ControlAction::BindUnicastThenSend { .. } => { /* hoisted await */ } + // … +} +``` + +The awaited sub-futures still appear in `run_future`'s layout; the win is that the per-variant locals (decoded headers, buffers, responder handles) are no longer held across the awaits, and the variants overlap better. **This task is kept only if Step 4 shows the witness moved.** + +- [ ] **Step 3: Run the full control-message suite (behavior must be identical)** + +Run: `cargo nextest run --no-default-features --features client-tokio,server-tokio` +Expected: all green — every control path (bind/unbind, send, subscribe, reboot-flag query, set-interface) behaves exactly as before. + +- [ ] **Step 4: Re-measure the run-future witness** + +Run: `cargo test --features client,server,bare_metal --test bare_metal_e2e -- --nocapture | grep bm_client_run_future` +Expected: value ≤ Step-1. **If it did not drop, revert this task** (`git checkout -- src/client/inner.rs`) per the "dropped, not merged on faith" constraint, and note it in the PR description. + +- [ ] **Step 5: Commit (only if kept)** + +```bash +git add src/client/inner.rs +git commit -m "perf(client): flatten control-message handler to shrink run_future state (#125)" +``` + +--- + +### Task 7: Tighten witness budgets + rewrite the footprint doc + +**Files:** +- Modify: `tests/bare_metal_e2e.rs:596-598` (budgets) +- Modify: `src/client/mod.rs:1-30` (doc) + +- [ ] **Step 1: Set budgets to the new sizes + 25% headroom** + +Using the post-Task-6 `FUTURE_SIZE` prints, set each budget to `ceil64(measured × 1.25)`: + +```rust +// tests/bare_metal_e2e.rs:596 — replace with the new measured baselines +const BM_CLIENT_RUN_FUTURE_BUDGET: usize = /* ceil64(new bm_client_run_future × 1.25) */; +const BM_CLIENT_SOCKET_LOOP_BUDGET: usize = /* ceil64(new bm_client_socket_loop × 1.25) */; +const BM_SERVER_RUN_FUTURE_BUDGET: usize = 9664; // unchanged — server is PR 3 +``` + +- [ ] **Step 2: Rewrite the memory-footprint doc** (`src/client/mod.rs:1-30`) to describe pooled, caller-sized buffers instead of the old "12 KiB always-live / 24 KiB peak in-future" math: + +```rust +//! SOME/IP client. +//! +//! # Memory footprint +//! +//! The client's `Inner` state is allocated inline. The per-socket +//! `UDP_BUFFER_SIZE` receive buffers are **not** part of the spawned +//! socket-loop futures: each loop claims a `&'static mut [u8]` from a +//! [`BufferProvider`] at bind and releases it when the socket closes. On +//! the bare-metal path the consumer declares the backing `BufferPool` as a +//! `static`, choosing both the slot count and the per-slot length (e.g. +//! 2 × 512 B), so the buffer budget lives in `.bss` and is sized by the +//! caller rather than fixed at `UNICAST_SOCKETS_CAP × UDP_BUFFER_SIZE`. On +//! `std + tokio` the provider is heap-backed and provisioned internally +//! (`UDP_BUFFER_SIZE`-sized slots), invisible to callers. +//! +//! See `docs/simple_someip/plans/2026-06-09-phase22-125-memory-reduction-design.md`. +``` + +- [ ] **Step 3: Verify witnesses pass at the new budgets + docs build** + +Run: `cargo test --features client,server,bare_metal --test bare_metal_e2e` +Expected: PASS at the tightened budgets. +Run: `RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --no-default-features --features client` +Expected: no broken intra-doc links (the `[`BufferProvider`]` link resolves). + +- [ ] **Step 4: Commit** + +```bash +git add tests/bare_metal_e2e.rs src/client/mod.rs +git commit -m "docs+test(client): pooled-buffer footprint doc + tightened future-size budgets (#125)" +``` + +--- + +### Task 8: Full verification + before/after numbers + +**Files:** none (verification + PR description) + +- [ ] **Step 1: Full suite, all shipped feature combos** + +```bash +cargo nextest run --no-default-features --features client-tokio,server-tokio +cargo test --features client,bare_metal --test no_alloc_witness +cargo clippy --workspace --all-features -- -D warnings -D clippy::pedantic +cargo clippy -p simple-someip --no-default-features --features client,bare_metal -- -D warnings -D clippy::pedantic +cargo build --target thumbv7em-none-eabihf --no-default-features --features client,bare_metal +``` +Expected: all green; client+bare_metal still alloc-free. + +- [ ] **Step 2: Capture authoritative thumb numbers** (if `tools/capture_type_sizes.sh` from PR 0 exists) + +Run: `tools/capture_type_sizes.sh` +Record the client run-future / socket-loop TaskStorage rows. + +- [ ] **Step 3: Record before/after** in the PR description — the PR-0 baseline vs. the post-PR-2 `FUTURE_SIZE` prints and thumb table, calling out the arena bytes moved to caller `.bss`. + +- [ ] **Step 4: Finish the branch** + +Announce: "I'm using the finishing-a-development-branch skill to complete this work." Then follow superpowers:finishing-a-development-branch — verify the suite, push `feature/pr2_125_client_async_state`, and open the draft PR **based on `feature/pr1_124_followups`** (keep it stacked, never merge-down). + +--- + +## Self-Review + +**Spec coverage (design doc PR-2 section):** +- Buffer extraction via claim/release pool, `&'static mut [u8]`, caller-sized → Tasks 1, 2, 3, 4. ✓ +- Tokio path provisions internally, API unchanged → Task 2 (`TokioBufferProvider`) + Task 4 (defaulted constructor). ✓ +- Inbound-oversize drop+log; oversize-send keyed on `buf.len()` → Task 3. ✓ +- E2E `protected` buffer pooled-or-restructured, measured → Task 5. ✓ +- Handler-tree flattening, kept only if numbers move → Task 6. ✓ +- Doc debt rewrite (`mod.rs:12-30`) → Task 7. ✓ +- Validate against PR-0 numbers; drop changes that don't move it → Steps in Tasks 5, 6 + budget retighten in Task 7. ✓ +- Deferred readiness-split receive → out of scope, recorded in the design doc; not a task here. ✓ + +**Placeholder scan:** Task 5/6 contain measurement-derived values (budgets, the A/B E2E choice) that are *intentionally* resolved at implementation time against live witness output — each has an explicit capture-then-decide step, not a vague "TBD." The `protect()` call shape in Task 5 and the `ControlAction` field lists in Task 6 must be matched to the real signatures in `inner.rs`/the E2E registry when those tasks run; flagged inline. + +**Type consistency:** `BufferPool`/`BufferLease` (Task 1) → `BufferProvider`/`StaticBufferProvider`/`TokioBufferProvider` (Task 2) → `ClientDeps.buffer_provider: BP` (Task 4) → `socket_loop_future(.., buf: BufferLease)` (Task 3) are consistent across tasks. `Error::Capacity("udp_buffer")` is reused verbatim from the existing send-path error (`socket_manager.rs:447`) for the pool-exhaustion case. + +**Risk note carried from the design doc:** the pool hands out `&'static mut [u8]`; Task 1's tests cover claim-to-exhaustion, release-on-drop, and no-double-claim. If `critical-section` is not already satisfiable on the bare-metal build, Task 1 Step 5 adds it to the `bare_metal` feature. From 9124c1b39a36f26ea3558b2081e5ec736f4ea676 Mon Sep 17 00:00:00 2001 From: Justin Kovacich Date: Wed, 17 Jun 2026 09:52:22 -0400 Subject: [PATCH 02/12] feat(static_channels): BufferPool + BufferLease claim/release primitive (#125) Co-Authored-By: Claude Opus 4.8 (1M context) --- Cargo.toml | 4 + src/static_channels/buffer_pool.rs | 126 +++++++++++++++++++++++++++++ src/static_channels/mod.rs | 3 + tests/buffer_pool.rs | 21 +++++ 4 files changed, 154 insertions(+) create mode 100644 src/static_channels/buffer_pool.rs create mode 100644 tests/buffer_pool.rs diff --git a/Cargo.toml b/Cargo.toml index 43053c7a..34189df5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -184,3 +184,7 @@ harness = false [[test]] name = "bare_metal_e2e" required-features = ["client", "server", "bare_metal"] + +[[test]] +name = "buffer_pool" +required-features = ["bare_metal"] diff --git a/src/static_channels/buffer_pool.rs b/src/static_channels/buffer_pool.rs new file mode 100644 index 00000000..3e7d8dcc --- /dev/null +++ b/src/static_channels/buffer_pool.rs @@ -0,0 +1,126 @@ +//! Fixed-capacity pool of `&'static mut [u8]` buffers with claim/release +//! semantics, mirroring the channel pools in this module. A `BufferPool` +//! is declared as a `static` by the consumer; each `claim()` hands out one +//! slot as a [`BufferLease`] that returns the slot to the pool on drop. +//! +//! Synchronization uses per-slot `AtomicBool` compare-exchange so the same +//! code is valid on the bare-metal target and on std without requiring a +//! `critical-section` implementation. + +use core::cell::UnsafeCell; +use core::ops::{Deref, DerefMut}; +use core::sync::atomic::{AtomicBool, Ordering}; + +/// Fixed-capacity pool of `LEN`-byte buffers. Declare as a `static` and call +/// [`Self::claim`] to obtain a [`BufferLease`]. +/// +/// # Const-constructible +/// +/// `BufferPool::new()` is `const fn`, so pools can be declared as `static` +/// items initialized at link time with no runtime cost. +/// +/// # Synchronization +/// +/// Each slot has an independent `AtomicBool` claimed flag. `claim()` scans +/// for the first free slot and atomically claims it via +/// `compare_exchange(false, true, AcqRel, Acquire)`. `Drop` releases via +/// `store(false, Release)`. No global lock is taken; claim and release are +/// individually linearizable. +pub struct BufferPool { + // `UnsafeCell` because `claim()` hands out `&'static mut` slices into + // this store. The `claimed` flags ensure at most one live `&mut` per slot. + store: UnsafeCell<[[u8; LEN]; SLOTS]>, + // One atomic flag per slot; `true` = slot is currently claimed. + claimed: [AtomicBool; SLOTS], +} + +// SAFETY: `BufferPool` is Sync because: +// - `claimed` is an array of `AtomicBool`, which is already Sync. +// - Access to `store` is strictly gated: a slot's bytes are only touched +// while its `claimed` flag is held (compare_exchange'd to true), which +// ensures at most one live `&mut` per slot at any time. +unsafe impl Sync for BufferPool {} + +impl BufferPool { + /// Create a new, empty pool. All slots are free. + #[must_use] + pub const fn new() -> Self { + Self { + store: UnsafeCell::new([[0u8; LEN]; SLOTS]), + claimed: [const { AtomicBool::new(false) }; SLOTS], + } + } + + /// Claim a free slot, returning a [`BufferLease`], or `None` if all + /// `SLOTS` are in use. + /// + /// The returned buffer is zeroed before hand-out so a reused slot never + /// leaks the previous tenant's bytes. + pub fn claim(&'static self) -> Option { + for (idx, flag) in self.claimed.iter().enumerate() { + // Attempt to atomically claim this slot. + if flag + .compare_exchange(false, true, Ordering::AcqRel, Ordering::Acquire) + .is_ok() + { + // SAFETY: we just won the compare_exchange on `claimed[idx]`, + // so no other `claim()` call holds a reference to slot `idx`. + // We derive the slot pointer by raw-pointer arithmetic to avoid + // forming a `&mut` to the whole array (which would alias already- + // claimed slots). The resulting `&'static mut [u8]` is valid for + // the lifetime of the `BufferPool` `static`. + let slot_ptr = + unsafe { self.store.get().cast::<[u8; LEN]>().add(idx) }; + let slot: &'static mut [u8] = unsafe { (*slot_ptr).as_mut_slice() }; + slot.fill(0); + return Some(BufferLease { + buf: slot, + claimed_flag: &self.claimed[idx], + }); + } + } + None + } +} + +impl Default for BufferPool { + fn default() -> Self { + Self::new() + } +} + +/// RAII handle to one claimed buffer from a [`BufferPool`]. +/// +/// Derefs to `[u8]` for read/write access. Returns the slot to its pool on +/// drop. +pub struct BufferLease { + buf: &'static mut [u8], + /// Back-pointer to this slot's claimed flag in the owning pool. + claimed_flag: &'static AtomicBool, +} + +// SAFETY: `BufferLease` owns exclusive access to its slot (enforced by the +// pool's per-slot `AtomicBool`). Both `&'static AtomicBool` and +// `&'static mut [u8]` are Send. +unsafe impl Send for BufferLease {} + +impl Deref for BufferLease { + type Target = [u8]; + fn deref(&self) -> &[u8] { + self.buf + } +} + +impl DerefMut for BufferLease { + fn deref_mut(&mut self) -> &mut [u8] { + self.buf + } +} + +impl Drop for BufferLease { + fn drop(&mut self) { + // Release the slot atomically. Any subsequent `claim()` that acquires + // this flag will see the updated store state. + self.claimed_flag.store(false, Ordering::Release); + } +} diff --git a/src/static_channels/mod.rs b/src/static_channels/mod.rs index 9bdc16f2..2e20d049 100644 --- a/src/static_channels/mod.rs +++ b/src/static_channels/mod.rs @@ -48,6 +48,9 @@ #![allow(clippy::module_name_repetitions)] +mod buffer_pool; +pub use buffer_pool::{BufferLease, BufferPool}; + use core::cell::{Cell, RefCell}; use core::future::{Future, poll_fn}; use core::pin::Pin; diff --git a/tests/buffer_pool.rs b/tests/buffer_pool.rs new file mode 100644 index 00000000..b386d476 --- /dev/null +++ b/tests/buffer_pool.rs @@ -0,0 +1,21 @@ +use simple_someip::static_channels::BufferPool; + +static POOL: BufferPool<2, 4> = BufferPool::new(); + +#[test] +fn claim_returns_distinct_zeroed_slices_until_exhausted() { + let mut a = POOL.claim().expect("slot 0"); + let b = POOL.claim().expect("slot 1"); + assert_eq!(a.len(), 4); + assert_eq!(&*b, &[0u8; 4]); // freshly handed-out slot is zeroed + a[0] = 0xAB; // writable + assert_eq!(a[0], 0xAB); + assert!(POOL.claim().is_none(), "pool of 2 must refuse a 3rd claim"); +} + +#[test] +fn dropping_a_lease_returns_its_slot() { + let a = POOL.claim().expect("slot"); + drop(a); + assert!(POOL.claim().is_some(), "slot must be reusable after the lease drops"); +} From 34b1d0775ac5df09ff2913466fa6d89c40806070 Mon Sep 17 00:00:00 2001 From: Justin Kovacich Date: Wed, 17 Jun 2026 10:01:10 -0400 Subject: [PATCH 03/12] test(static_channels): isolate buffer_pool tests per-pool; doc unsafe lifetime (#125) Addresses Task 1 review: shared static could flake under parallel libtest. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/static_channels/buffer_pool.rs | 3 +++ tests/buffer_pool.rs | 15 +++++++++------ 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/static_channels/buffer_pool.rs b/src/static_channels/buffer_pool.rs index 3e7d8dcc..6cc173c1 100644 --- a/src/static_channels/buffer_pool.rs +++ b/src/static_channels/buffer_pool.rs @@ -71,6 +71,9 @@ impl BufferPool { // the lifetime of the `BufferPool` `static`. let slot_ptr = unsafe { self.store.get().cast::<[u8; LEN]>().add(idx) }; + // `'static` is sound because `self: &'static BufferPool`, so the + // backing store outlives the lease; the annotation, not a + // transmute, carries the lifetime. let slot: &'static mut [u8] = unsafe { (*slot_ptr).as_mut_slice() }; slot.fill(0); return Some(BufferLease { diff --git a/tests/buffer_pool.rs b/tests/buffer_pool.rs index b386d476..97c2a8de 100644 --- a/tests/buffer_pool.rs +++ b/tests/buffer_pool.rs @@ -1,21 +1,24 @@ use simple_someip::static_channels::BufferPool; -static POOL: BufferPool<2, 4> = BufferPool::new(); +// One pool per test: a shared `static` would let libtest's parallel threads +// race (one test claiming both slots makes the other's claim spuriously fail). +static POOL_EXHAUST: BufferPool<2, 4> = BufferPool::new(); +static POOL_RETURN: BufferPool<2, 4> = BufferPool::new(); #[test] fn claim_returns_distinct_zeroed_slices_until_exhausted() { - let mut a = POOL.claim().expect("slot 0"); - let b = POOL.claim().expect("slot 1"); + let mut a = POOL_EXHAUST.claim().expect("slot 0"); + let b = POOL_EXHAUST.claim().expect("slot 1"); assert_eq!(a.len(), 4); assert_eq!(&*b, &[0u8; 4]); // freshly handed-out slot is zeroed a[0] = 0xAB; // writable assert_eq!(a[0], 0xAB); - assert!(POOL.claim().is_none(), "pool of 2 must refuse a 3rd claim"); + assert!(POOL_EXHAUST.claim().is_none(), "pool of 2 must refuse a 3rd claim"); } #[test] fn dropping_a_lease_returns_its_slot() { - let a = POOL.claim().expect("slot"); + let a = POOL_RETURN.claim().expect("slot"); drop(a); - assert!(POOL.claim().is_some(), "slot must be reusable after the lease drops"); + assert!(POOL_RETURN.claim().is_some(), "slot must be reusable after the lease drops"); } From e4b19e9999831ae262827e8090e4e294d677c7f1 Mon Sep 17 00:00:00 2001 From: Justin Kovacich Date: Wed, 17 Jun 2026 10:09:23 -0400 Subject: [PATCH 04/12] feat(transport): BufferProvider trait + static/tokio impls; relocate BufferPool to ungated module (#125) Co-Authored-By: Claude Opus 4.8 (1M context) --- Cargo.toml | 1 - src/{static_channels => }/buffer_pool.rs | 9 +++++++ src/lib.rs | 5 ++++ src/static_channels/mod.rs | 6 +++-- src/tokio_transport.rs | 33 ++++++++++++++++++++++++ src/transport.rs | 27 +++++++++++++++++++ tests/buffer_pool.rs | 13 +++++++++- 7 files changed, 90 insertions(+), 4 deletions(-) rename src/{static_channels => }/buffer_pool.rs (94%) diff --git a/Cargo.toml b/Cargo.toml index 34189df5..04c8af80 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -187,4 +187,3 @@ required-features = ["client", "server", "bare_metal"] [[test]] name = "buffer_pool" -required-features = ["bare_metal"] diff --git a/src/static_channels/buffer_pool.rs b/src/buffer_pool.rs similarity index 94% rename from src/static_channels/buffer_pool.rs rename to src/buffer_pool.rs index 6cc173c1..7f1fd231 100644 --- a/src/static_channels/buffer_pool.rs +++ b/src/buffer_pool.rs @@ -92,6 +92,15 @@ impl Default for BufferPool { } } +impl core::fmt::Debug for BufferPool { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + f.debug_struct("BufferPool") + .field("slots", &SLOTS) + .field("len", &LEN) + .finish_non_exhaustive() + } +} + /// RAII handle to one claimed buffer from a [`BufferPool`]. /// /// Derefs to `[u8]` for read/write access. Returns the slot to its pool on diff --git a/src/lib.rs b/src/lib.rs index 05047a61..84d6911d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -157,6 +157,11 @@ extern crate alloc; /// smaller link MTU may want to lower this by forking. pub const UDP_BUFFER_SIZE: usize = 1500; +/// Fixed-capacity pool of `&'static mut [u8]` receive/scratch buffers. +/// Pure `no_std` (uses only `core::`). Exposed without a feature gate so +/// both the bare-metal and std/tokio paths can reach [`buffer_pool::BufferPool`] +/// and [`buffer_pool::BufferLease`]. +pub mod buffer_pool; /// SOME/IP client for discovering services and exchanging messages. #[cfg(feature = "client")] pub mod client; diff --git a/src/static_channels/mod.rs b/src/static_channels/mod.rs index 2e20d049..b778acf1 100644 --- a/src/static_channels/mod.rs +++ b/src/static_channels/mod.rs @@ -48,8 +48,10 @@ #![allow(clippy::module_name_repetitions)] -mod buffer_pool; -pub use buffer_pool::{BufferLease, BufferPool}; +// `BufferPool` and `BufferLease` live in `crate::buffer_pool` (ungated, so +// the tokio path can reach them without the `bare_metal` feature). Re-export +// them here so existing `static_channels::BufferPool` paths keep working. +pub use crate::buffer_pool::{BufferLease, BufferPool}; use core::cell::{Cell, RefCell}; use core::future::{Future, poll_fn}; diff --git a/src/tokio_transport.rs b/src/tokio_transport.rs index 2ee097da..67c5be34 100644 --- a/src/tokio_transport.rs +++ b/src/tokio_transport.rs @@ -549,6 +549,39 @@ impl crate::transport::UnboundedPooled for T { } } +// ── TokioBufferProvider ─────────────────────────────────────────────────── + +use std::boxed::Box; + +use crate::buffer_pool::{BufferLease, BufferPool}; +use crate::transport::BufferProvider; + +/// Tokio-path buffer provider: a single leaked `BufferPool` sized at +/// `UNICAST_SOCKETS_CAP + 1` × `UDP_BUFFER_SIZE` (one per possible socket +/// plus discovery). Leaking is fine — a client process holds one for its +/// lifetime; the API hides it entirely from callers. +#[derive(Clone, Copy, Debug)] +pub struct TokioBufferProvider(&'static BufferPool<9, { crate::UDP_BUFFER_SIZE }>); + +impl TokioBufferProvider { + #[must_use] + pub fn new() -> Self { + Self(Box::leak(Box::new(BufferPool::new()))) + } +} + +impl Default for TokioBufferProvider { + fn default() -> Self { + Self::new() + } +} + +impl BufferProvider for TokioBufferProvider { + fn claim(&self) -> Option { + self.0.claim() + } +} + // ── EmbassySyncChannels (extracted) ────────────────────────────────────── // // The bare-metal `ChannelFactory` impl previously lived here as a sub- diff --git a/src/transport.rs b/src/transport.rs index fbc49a77..1f137d79 100644 --- a/src/transport.rs +++ b/src/transport.rs @@ -1360,6 +1360,33 @@ pub trait UnboundedPooled: Send + Sized + 'static { fn unbounded_pair() -> (C::UnboundedSender, C::UnboundedReceiver); } +// ── BufferProvider ──────────────────────────────────────────────────────── + +use crate::buffer_pool::{BufferLease, BufferPool}; + +/// Source of `&'static mut [u8]` receive/scratch buffers for the client's +/// socket loops. Mirrors [`ChannelFactory`]'s role for channels: the +/// bare-metal path is backed by a consumer-declared `static BufferPool`; +/// the tokio path is heap-backed and provisioned internally. +pub trait BufferProvider: Clone + Send + Sync + 'static { + /// Claim one buffer, or `None` when the pool is exhausted. + fn claim(&self) -> Option; +} + +/// `BufferProvider` backed by a `'static` [`BufferPool`] (bare-metal path). +#[derive(Clone, Copy, Debug)] +pub struct StaticBufferProvider( + pub &'static BufferPool, +); + +impl BufferProvider + for StaticBufferProvider +{ + fn claim(&self) -> Option { + self.0.claim() + } +} + /// Zero-behavior implementations of the client- and server-side /// dependency traits. Two uses: (1) compile-time proof the trait /// signatures are implementable without async machinery, (2) diff --git a/tests/buffer_pool.rs b/tests/buffer_pool.rs index 97c2a8de..dba27940 100644 --- a/tests/buffer_pool.rs +++ b/tests/buffer_pool.rs @@ -1,4 +1,5 @@ -use simple_someip::static_channels::BufferPool; +use simple_someip::buffer_pool::BufferPool; +use simple_someip::transport::{BufferProvider, StaticBufferProvider}; // One pool per test: a shared `static` would let libtest's parallel threads // race (one test claiming both slots makes the other's claim spuriously fail). @@ -22,3 +23,13 @@ fn dropping_a_lease_returns_its_slot() { drop(a); assert!(POOL_RETURN.claim().is_some(), "slot must be reusable after the lease drops"); } + +static PROV_POOL: BufferPool<2, 8> = BufferPool::new(); + +#[test] +fn static_provider_claims_through_a_shared_pool() { + let prov = StaticBufferProvider(&PROV_POOL); + let _a = prov.claim().expect("first"); + let _b = prov.claim().expect("second"); + assert!(prov.claim().is_none(), "provider exposes the pool's capacity"); +} From 2f6552cfca8e4f23f81e2ad2a12fc0b05e367018 Mon Sep 17 00:00:00 2001 From: Justin Kovacich Date: Wed, 17 Jun 2026 10:14:12 -0400 Subject: [PATCH 05/12] style(transport): blank line after buffer_pool mod; document required Box import (#125) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task 2 review: lib.rs blank-line consistency. The reviewer's 'redundant Box import' finding was a false positive — the crate is no_std, so std-gated modules import Box explicitly; verified client-tokio fails without it. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/lib.rs | 1 + src/tokio_transport.rs | 2 ++ 2 files changed, 3 insertions(+) diff --git a/src/lib.rs b/src/lib.rs index 84d6911d..410d8b56 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -162,6 +162,7 @@ pub const UDP_BUFFER_SIZE: usize = 1500; /// both the bare-metal and std/tokio paths can reach [`buffer_pool::BufferPool`] /// and [`buffer_pool::BufferLease`]. pub mod buffer_pool; + /// SOME/IP client for discovering services and exchanging messages. #[cfg(feature = "client")] pub mod client; diff --git a/src/tokio_transport.rs b/src/tokio_transport.rs index 67c5be34..915ffa0d 100644 --- a/src/tokio_transport.rs +++ b/src/tokio_transport.rs @@ -551,6 +551,8 @@ impl crate::transport::UnboundedPooled for T { // ── TokioBufferProvider ─────────────────────────────────────────────────── +// `Box` is not in scope by default: the crate is `#![no_std]`, so std-gated +// modules still import it explicitly (it is NOT a redundant import). use std::boxed::Box; use crate::buffer_pool::{BufferLease, BufferPool}; From bca75a0570057213ab2660d4277d991c99b55756 Mon Sep 17 00:00:00 2001 From: Justin Kovacich Date: Wed, 17 Jun 2026 10:41:27 -0400 Subject: [PATCH 06/12] feat(client): move socket-loop buffer out of the future via BufferProvider; oversize drop/reject (#125) Threads a BufferProvider through ClientDeps -> BindDispatch -> bind_*; the socket loop receives its buffer by lease (released on loop-future drop). Inbound datagrams over the claimed length are dropped+logged; oversize-send rejection keys off buf.len(). Tokio path provisions one leaked pool per client. bare_metal socket-loop future: 2224 B -> 776 B. (Tasks 3+4 of the #125 plan.) Co-Authored-By: Claude Opus 4.8 (1M context) --- examples/bare_metal_client/src/main.rs | 14 +- examples/embassy_net_client/src/main.rs | 6 +- simple-someip-embassy-net/tests/loopback.rs | 11 +- src/client/bind_dispatch.rs | 130 +++++--- src/client/inner.rs | 39 ++- src/client/mod.rs | 90 +++++- src/client/socket_manager.rs | 95 +++++- tests/bare_metal_client.rs | 10 +- tests/bare_metal_client_local.rs | 10 +- tests/bare_metal_e2e.rs | 309 +++++++++++++++++++- tests/static_channels_alloc_witness.rs | 10 +- tools/size_probe/src/lib.rs | 6 +- 12 files changed, 636 insertions(+), 94 deletions(-) diff --git a/examples/bare_metal_client/src/main.rs b/examples/bare_metal_client/src/main.rs index 383841a9..37e32cdc 100644 --- a/examples/bare_metal_client/src/main.rs +++ b/examples/bare_metal_client/src/main.rs @@ -59,12 +59,13 @@ use simple_someip::client::{ClientUpdate, ControlMessage, ReceivedMessage, SendM use simple_someip::define_static_channels; use simple_someip::e2e::E2ERegistry; use simple_someip::protocol::sd::RebootFlag; +use simple_someip::static_channels::BufferPool; use simple_someip::transport::{ - ReceivedDatagram, SocketOptions, Spawner, Timer, TransportError, TransportFactory, - TransportSocket, + ReceivedDatagram, SocketOptions, Spawner, StaticBufferProvider, Timer, TransportError, + TransportFactory, TransportSocket, }; use simple_someip::{AtomicInterfaceHandle, StaticE2EHandle, StaticE2EStorage}; -use simple_someip::{Client, ClientDeps, RawPayload}; +use simple_someip::{Client, ClientDeps, RawPayload, UDP_BUFFER_SIZE}; // ── Static-pool channel factory ─────────────────────────────────────── // @@ -298,6 +299,13 @@ async fn main() { timer: MockTimer, e2e_registry: e2e, interface: iface, + // Caller-declared static buffer pool (#125): one slot per + // possible socket. On real firmware this is a `static`; here it + // is a function-local `static` for the example. + buffer_provider: { + static POOL: BufferPool<9, UDP_BUFFER_SIZE> = BufferPool::new(); + StaticBufferProvider(&POOL) + }, }, false, // multicast_loopback ); diff --git a/examples/embassy_net_client/src/main.rs b/examples/embassy_net_client/src/main.rs index 61024c38..b79e3e79 100644 --- a/examples/embassy_net_client/src/main.rs +++ b/examples/embassy_net_client/src/main.rs @@ -58,7 +58,8 @@ use simple_someip::define_static_channels; use simple_someip::e2e::E2ERegistry; use simple_someip::protocol::sd::RebootFlag; use simple_someip::server::{ServerConfig, SubscribeError, Subscriber, SubscriptionHandle}; -use simple_someip::transport::{LocalSpawner, Timer}; +use simple_someip::static_channels::BufferPool; +use simple_someip::transport::{LocalSpawner, StaticBufferProvider, Timer}; use simple_someip::{Client, ClientDeps, RawPayload, Server, ServerDeps}; use simple_someip_embassy_net::{EmbassyNetFactory, EmbassyNetSocket, LINK_MTU, SocketPool}; @@ -410,12 +411,15 @@ async fn main() { let client_e2e: Arc> = Arc::new(Mutex::new(E2ERegistry::new())); let client_iface: Arc> = Arc::new(RwLock::new(IP_B)); + let buf_pool: &'static BufferPool<8, LINK_MTU> = + Box::leak(Box::new(BufferPool::new())); let client_deps = ClientDeps { factory: client_factory, spawner: LocalTokioSpawner, timer: LocalTimer, e2e_registry: client_e2e, interface: client_iface, + buffer_provider: StaticBufferProvider(buf_pool), }; let (client, mut updates, run_fut) = Client::< diff --git a/simple-someip-embassy-net/tests/loopback.rs b/simple-someip-embassy-net/tests/loopback.rs index bc9f571c..0f7e571f 100644 --- a/simple-someip-embassy-net/tests/loopback.rs +++ b/simple-someip-embassy-net/tests/loopback.rs @@ -42,7 +42,10 @@ use std::sync::{Arc, Mutex}; use embassy_net::driver::{Capabilities, Driver, HardwareAddress, LinkState, RxToken, TxToken}; use embassy_net::{Config, Stack, StackResources, StaticConfigV4}; -use simple_someip::transport::{SocketOptions, TransportFactory, TransportSocket}; +use simple_someip::static_channels::BufferPool; +use simple_someip::transport::{ + SocketOptions, StaticBufferProvider, TransportFactory, TransportSocket, +}; use simple_someip_embassy_net::{EmbassyNetFactory, LINK_MTU, SocketPool}; // ── LoopbackDriver pair ────────────────────────────────────────────── @@ -640,12 +643,15 @@ async fn client_receives_server_sd_announcement() { Arc::new(std::sync::Mutex::new(E2ERegistry::new())); let client_iface: Arc> = Arc::new(RwLock::new(IP_B)); + let buf_pool: &'static BufferPool<2, LINK_MTU> = + Box::leak(Box::new(BufferPool::new())); let client_deps = ClientDeps { factory: client_factory, spawner: LocalTokioSpawner, timer: LocalTimer, e2e_registry: client_e2e, interface: client_iface, + buffer_provider: StaticBufferProvider(buf_pool), }; let (client, mut updates, run_fut) = @@ -762,12 +768,15 @@ async fn client_send_request_server_runloop_stable() { Arc::new(std::sync::Mutex::new(E2ERegistry::new())); let client_iface: Arc> = Arc::new(RwLock::new(IP_B)); + let buf_pool: &'static BufferPool<8, LINK_MTU> = + Box::leak(Box::new(BufferPool::new())); let client_deps = ClientDeps { factory: client_factory, spawner: LocalTokioSpawner, timer: LocalTimer, e2e_registry: client_e2e, interface: client_iface, + buffer_provider: StaticBufferProvider(buf_pool), }; let (client, _updates, run_fut) = Client::< diff --git a/src/client/bind_dispatch.rs b/src/client/bind_dispatch.rs index 39d8977c..cb90b71b 100644 --- a/src/client/bind_dispatch.rs +++ b/src/client/bind_dispatch.rs @@ -26,7 +26,8 @@ use super::error::Error; use super::socket_manager::SocketManager; use crate::traits::PayloadWireFormat; use crate::transport::{ - ChannelFactory, E2ERegistryHandle, LocalSpawner, Spawner, TransportFactory, TransportSocket, + BufferProvider, ChannelFactory, E2ERegistryHandle, LocalSpawner, Spawner, TransportFactory, + TransportSocket, }; /// Crate-private bind-and-spawn abstraction shared by Send and `!Send` @@ -43,6 +44,10 @@ where { /// Bind a discovery socket and submit its I/O loop to the /// configured task executor. + // `async move` body (rather than `async fn`) is required: the trait + // method returns `impl Future`, and the block must capture `&self` to + // claim a buffer (#125) before delegating to `SocketManager::bind_*`. + #[allow(clippy::manual_async_fn)] fn bind_discovery( &self, interface: Ipv4Addr, @@ -63,12 +68,18 @@ where /// `BindDispatch` for the multi-threaded path: requires a /// [`Spawner`] and a `Send + Sync` transport socket. -pub(super) struct SpawnerDispatch { +/// +/// Carries a [`BufferProvider`] (`#125`): each `bind_*` claims one +/// socket-loop buffer from it and moves the lease into the spawned loop +/// future. The lease frees its pool slot when that future drops (i.e. when +/// the socket closes), so no explicit release is needed at eviction. +pub(super) struct SpawnerDispatch { pub factory: F, pub spawner: S, + pub buffer_provider: BP, } -impl BindDispatch for SpawnerDispatch +impl BindDispatch for SpawnerDispatch where MD: PayloadWireFormat + Clone + core::fmt::Debug + Send + 'static, C: ChannelFactory, @@ -79,11 +90,16 @@ where for<'a> ::SendFuture<'a>: Send, for<'a> ::RecvFuture<'a>: Send, S: Spawner + Send + Sync + 'static, + BP: BufferProvider, Result, Error>: crate::transport::BoundedPooled, super::socket_manager::SendMessage: crate::transport::BoundedPooled, Result<(), Error>: crate::transport::OneshotPooled, { + // `async move` body (rather than `async fn`) is required: the trait + // method returns `impl Future`, and the block must capture `&self` to + // claim a buffer (#125) before delegating to `SocketManager::bind_*`. + #[allow(clippy::manual_async_fn)] fn bind_discovery( &self, interface: Ipv4Addr, @@ -92,40 +108,62 @@ where session_has_wrapped: bool, multicast_loopback: bool, ) -> impl Future, Error>> + '_ { - SocketManager::::bind_discovery_seeded_with_transport( - &self.factory, - &self.spawner, - interface, - e2e_registry, - session_id, - session_has_wrapped, - multicast_loopback, - ) + async move { + let buf = self + .buffer_provider + .claim() + .ok_or(Error::Capacity("udp_buffer"))?; + SocketManager::::bind_discovery_seeded_with_transport( + &self.factory, + &self.spawner, + interface, + e2e_registry, + session_id, + session_has_wrapped, + multicast_loopback, + buf, + ) + .await + } } + #[allow(clippy::manual_async_fn)] fn bind_unicast( &self, port: u16, e2e_registry: R, ) -> impl Future, Error>> + '_ { - SocketManager::::bind_with_transport( - &self.factory, - &self.spawner, - port, - e2e_registry, - ) + async move { + let buf = self + .buffer_provider + .claim() + .ok_or(Error::Capacity("udp_buffer"))?; + SocketManager::::bind_with_transport( + &self.factory, + &self.spawner, + port, + e2e_registry, + buf, + ) + .await + } } } /// `BindDispatch` for the single-threaded path: requires a /// [`LocalSpawner`] and `'static` transport socket. The socket and its /// GAT futures are not required to be `Send`. -pub(super) struct LocalSpawnerDispatch { +/// +/// Carries a [`BufferProvider`] for the same reason as [`SpawnerDispatch`]: +/// each `bind_*` claims one socket-loop buffer and moves the lease into the +/// spawned loop future, which frees the slot on drop. +pub(super) struct LocalSpawnerDispatch { pub factory: F, pub spawner: S, + pub buffer_provider: BP, } -impl BindDispatch for LocalSpawnerDispatch +impl BindDispatch for LocalSpawnerDispatch where MD: PayloadWireFormat + Clone + core::fmt::Debug + Send + 'static, C: ChannelFactory, @@ -133,11 +171,16 @@ where F: TransportFactory + 'static, F::Socket: 'static, S: LocalSpawner + 'static, + BP: BufferProvider, Result, Error>: crate::transport::BoundedPooled, super::socket_manager::SendMessage: crate::transport::BoundedPooled, Result<(), Error>: crate::transport::OneshotPooled, { + // `async move` body (rather than `async fn`) is required: the trait + // method returns `impl Future`, and the block must capture `&self` to + // claim a buffer (#125) before delegating to `SocketManager::bind_*`. + #[allow(clippy::manual_async_fn)] fn bind_discovery( &self, interface: Ipv4Addr, @@ -146,27 +189,44 @@ where session_has_wrapped: bool, multicast_loopback: bool, ) -> impl Future, Error>> + '_ { - SocketManager::::bind_discovery_seeded_with_transport_local( - &self.factory, - &self.spawner, - interface, - e2e_registry, - session_id, - session_has_wrapped, - multicast_loopback, - ) + async move { + let buf = self + .buffer_provider + .claim() + .ok_or(Error::Capacity("udp_buffer"))?; + SocketManager::::bind_discovery_seeded_with_transport_local( + &self.factory, + &self.spawner, + interface, + e2e_registry, + session_id, + session_has_wrapped, + multicast_loopback, + buf, + ) + .await + } } + #[allow(clippy::manual_async_fn)] fn bind_unicast( &self, port: u16, e2e_registry: R, ) -> impl Future, Error>> + '_ { - SocketManager::::bind_with_transport_local( - &self.factory, - &self.spawner, - port, - e2e_registry, - ) + async move { + let buf = self + .buffer_provider + .claim() + .ok_or(Error::Capacity("udp_buffer"))?; + SocketManager::::bind_with_transport_local( + &self.factory, + &self.spawner, + port, + e2e_registry, + buf, + ) + .await + } } } diff --git a/src/client/inner.rs b/src/client/inner.rs index 38d3a47a..405a0e40 100644 --- a/src/client/inner.rs +++ b/src/client/inner.rs @@ -10,7 +10,9 @@ use std::sync::{Arc, Mutex}; #[cfg(all(test, feature = "client-tokio"))] use crate::e2e::E2ERegistry; #[cfg(all(test, feature = "client-tokio"))] -use crate::tokio_transport::{TokioChannels, TokioSpawner, TokioTimer, TokioTransport}; +use crate::tokio_transport::{ + TokioBufferProvider, TokioChannels, TokioSpawner, TokioTimer, TokioTransport, +}; use crate::{ Timer, client::{ @@ -636,6 +638,10 @@ where } } for port in &dead_ports { + // Removing the `SocketManager` drops its channel ends, so the + // spawned socket-loop future returns and is dropped. That drop + // releases its `BufferLease` (#125), freeing the pool slot for + // the next bind — no explicit buffer release is needed here. unicast_sockets.remove(port); crate::log::warn!("Unicast socket on port {port} closed; evicted from registry"); } @@ -1256,6 +1262,7 @@ mod tests { crate::client::bind_dispatch::SpawnerDispatch< crate::tokio_transport::TokioTransport, TokioSpawner, + crate::tokio_transport::TokioBufferProvider, >, >; @@ -1428,6 +1435,7 @@ mod tests { dispatch: crate::client::bind_dispatch::SpawnerDispatch { factory: TokioTransport, spawner: TokioSpawner, + buffer_provider: TokioBufferProvider::new(), }, timer: TokioTimer, phantom: core::marker::PhantomData, @@ -1647,7 +1655,11 @@ mod tests { TokioTimer, Arc>, TokioChannels, - crate::client::bind_dispatch::SpawnerDispatch, + crate::client::bind_dispatch::SpawnerDispatch< + TokioTransport, + CountingSpawner, + TokioBufferProvider, + >, > = Inner { control_receiver, request_queue: Deque::new(), @@ -1668,6 +1680,7 @@ mod tests { dispatch: crate::client::bind_dispatch::SpawnerDispatch { factory: TokioTransport, spawner, + buffer_provider: TokioBufferProvider::new(), }, timer: TokioTimer, phantom: core::marker::PhantomData, @@ -1699,6 +1712,7 @@ mod tests { crate::client::bind_dispatch::SpawnerDispatch { factory: TokioTransport, spawner: TokioSpawner, + buffer_provider: TokioBufferProvider::new(), }, TokioTimer, ); @@ -1744,6 +1758,7 @@ mod tests { crate::client::bind_dispatch::SpawnerDispatch { factory: TokioTransport, spawner: TokioSpawner, + buffer_provider: TokioBufferProvider::new(), }, TokioTimer, ); @@ -1766,6 +1781,7 @@ mod tests { crate::client::bind_dispatch::SpawnerDispatch { factory: TokioTransport, spawner: TokioSpawner, + buffer_provider: TokioBufferProvider::new(), }, TokioTimer, ); @@ -1788,6 +1804,7 @@ mod tests { crate::client::bind_dispatch::SpawnerDispatch { factory: TokioTransport, spawner: TokioSpawner, + buffer_provider: TokioBufferProvider::new(), }, TokioTimer, ); @@ -1812,6 +1829,7 @@ mod tests { crate::client::bind_dispatch::SpawnerDispatch { factory: TokioTransport, spawner: TokioSpawner, + buffer_provider: TokioBufferProvider::new(), }, TokioTimer, ); @@ -1847,6 +1865,7 @@ mod tests { crate::client::bind_dispatch::SpawnerDispatch { factory: TokioTransport, spawner: TokioSpawner, + buffer_provider: TokioBufferProvider::new(), }, TokioTimer, ); @@ -1923,6 +1942,7 @@ mod tests { crate::client::bind_dispatch::SpawnerDispatch { factory: TokioTransport, spawner: TokioSpawner, + buffer_provider: TokioBufferProvider::new(), }, TokioTimer, ); @@ -1946,6 +1966,7 @@ mod tests { crate::client::bind_dispatch::SpawnerDispatch { factory: TokioTransport, spawner: TokioSpawner, + buffer_provider: TokioBufferProvider::new(), }, TokioTimer, ); @@ -1968,6 +1989,7 @@ mod tests { crate::client::bind_dispatch::SpawnerDispatch { factory: TokioTransport, spawner: TokioSpawner, + buffer_provider: TokioBufferProvider::new(), }, TokioTimer, ); @@ -2000,6 +2022,7 @@ mod tests { crate::client::bind_dispatch::SpawnerDispatch { factory: TokioTransport, spawner: TokioSpawner, + buffer_provider: TokioBufferProvider::new(), }, TokioTimer, ); @@ -2020,6 +2043,7 @@ mod tests { crate::client::bind_dispatch::SpawnerDispatch { factory: TokioTransport, spawner: TokioSpawner, + buffer_provider: TokioBufferProvider::new(), }, TokioTimer, ); @@ -2045,6 +2069,7 @@ mod tests { crate::client::bind_dispatch::SpawnerDispatch { factory: TokioTransport, spawner: TokioSpawner, + buffer_provider: TokioBufferProvider::new(), }, TokioTimer, ); @@ -2071,6 +2096,7 @@ mod tests { crate::client::bind_dispatch::SpawnerDispatch { factory: TokioTransport, spawner: TokioSpawner, + buffer_provider: TokioBufferProvider::new(), }, TokioTimer, ); @@ -2101,6 +2127,7 @@ mod tests { crate::client::bind_dispatch::SpawnerDispatch { factory: TokioTransport, spawner: TokioSpawner, + buffer_provider: TokioBufferProvider::new(), }, TokioTimer, ); @@ -2137,6 +2164,7 @@ mod tests { crate::client::bind_dispatch::SpawnerDispatch { factory: TokioTransport, spawner: TokioSpawner, + buffer_provider: TokioBufferProvider::new(), }, TokioTimer, ); @@ -2167,6 +2195,7 @@ mod tests { crate::client::bind_dispatch::SpawnerDispatch { factory: TokioTransport, spawner: TokioSpawner, + buffer_provider: TokioBufferProvider::new(), }, TokioTimer, ); @@ -2191,6 +2220,7 @@ mod tests { crate::client::bind_dispatch::SpawnerDispatch { factory: TokioTransport, spawner: TokioSpawner, + buffer_provider: TokioBufferProvider::new(), }, TokioTimer, ); @@ -2231,6 +2261,7 @@ mod tests { crate::client::bind_dispatch::SpawnerDispatch { factory: TokioTransport, spawner: TokioSpawner, + buffer_provider: TokioBufferProvider::new(), }, TokioTimer, ); @@ -2254,6 +2285,7 @@ mod tests { crate::client::bind_dispatch::SpawnerDispatch { factory: TokioTransport, spawner: TokioSpawner, + buffer_provider: TokioBufferProvider::new(), }, TokioTimer, ); @@ -2284,6 +2316,7 @@ mod tests { crate::client::bind_dispatch::SpawnerDispatch { factory: TokioTransport, spawner: TokioSpawner, + buffer_provider: TokioBufferProvider::new(), }, TokioTimer, ); @@ -2320,6 +2353,7 @@ mod tests { crate::client::bind_dispatch::SpawnerDispatch { factory: TokioTransport, spawner: TokioSpawner, + buffer_provider: TokioBufferProvider::new(), }, TokioTimer, ); @@ -2372,6 +2406,7 @@ mod tests { crate::client::bind_dispatch::SpawnerDispatch { factory: TokioTransport, spawner: TokioSpawner, + buffer_provider: TokioBufferProvider::new(), }, TokioTimer, ); diff --git a/src/client/mod.rs b/src/client/mod.rs index 6724d43b..601d0542 100644 --- a/src/client/mod.rs +++ b/src/client/mod.rs @@ -275,12 +275,13 @@ impl /// /// All five fields are public so callers can construct the struct /// inline; there's no builder ceremony beyond the field assignments. -pub struct ClientDeps +pub struct ClientDeps where F: TransportFactory, Tm: Timer, R: E2ERegistryHandle, I: InterfaceHandle, + BP: crate::transport::BufferProvider, { /// Transport factory used by `bind_*` to construct sockets. pub factory: F, @@ -293,6 +294,11 @@ where pub interface: I, /// Task-spawner used by `bind_*` to drive per-socket I/O loops. pub spawner: Sp, + /// Source of `&'static mut [u8]` socket-loop buffers (`#125`): + /// caller-sized on bare-metal (a `static BufferPool`), internally + /// heap-provisioned on the tokio path. One provider per client, + /// reused for every `bind_*`. + pub buffer_provider: BP, } /// Tokio-defaulted constructor. @@ -323,9 +329,16 @@ impl Arc>, Arc>, TokioSpawner, + crate::tokio_transport::TokioBufferProvider, > { /// Build a `ClientDeps` with the tokio defaults. + /// + /// `buffer_provider` is a single `TokioBufferProvider::new()` + /// constructed here exactly once. `TokioBufferProvider::new()` does + /// a `Box::leak`, so it MUST be one-per-client and never called on a + /// per-bind / hot path — this constructor is the canonical single + /// call site for the tokio path. #[must_use] pub fn tokio(interface: Ipv4Addr) -> Self { Self { @@ -334,6 +347,7 @@ impl e2e_registry: Arc::new(Mutex::new(E2ERegistry::new())), interface: Arc::new(RwLock::new(interface)), spawner: TokioSpawner, + buffer_provider: crate::tokio_transport::TokioBufferProvider::new(), } } } @@ -358,34 +372,40 @@ impl /// # let _ = deps; /// # } /// ``` -impl ClientDeps +impl ClientDeps where F: TransportFactory, Tm: Timer, R: E2ERegistryHandle, I: InterfaceHandle, + BP: crate::transport::BufferProvider, { /// Replace the `factory` field, returning a `ClientDeps` over the /// new factory type. - pub fn with_factory(self, factory: F2) -> ClientDeps { + pub fn with_factory( + self, + factory: F2, + ) -> ClientDeps { ClientDeps { factory, timer: self.timer, e2e_registry: self.e2e_registry, interface: self.interface, spawner: self.spawner, + buffer_provider: self.buffer_provider, } } /// Replace the `timer` field, returning a `ClientDeps` over the new /// timer type. - pub fn with_timer(self, timer: Tm2) -> ClientDeps { + pub fn with_timer(self, timer: Tm2) -> ClientDeps { ClientDeps { factory: self.factory, timer, e2e_registry: self.e2e_registry, interface: self.interface, spawner: self.spawner, + buffer_provider: self.buffer_provider, } } @@ -394,13 +414,14 @@ where pub fn with_e2e_registry( self, e2e_registry: R2, - ) -> ClientDeps { + ) -> ClientDeps { ClientDeps { factory: self.factory, timer: self.timer, e2e_registry, interface: self.interface, spawner: self.spawner, + buffer_provider: self.buffer_provider, } } @@ -409,13 +430,14 @@ where pub fn with_interface( self, interface: I2, - ) -> ClientDeps { + ) -> ClientDeps { ClientDeps { factory: self.factory, timer: self.timer, e2e_registry: self.e2e_registry, interface, spawner: self.spawner, + buffer_provider: self.buffer_provider, } } @@ -427,13 +449,14 @@ where /// [`Client::new_with_deps_local`] expects a `LocalSpawner` and /// the bound is enforced here at the builder call site rather /// than deferred to construction. - pub fn with_spawner(self, spawner: Sp2) -> ClientDeps { + pub fn with_spawner(self, spawner: Sp2) -> ClientDeps { ClientDeps { factory: self.factory, timer: self.timer, e2e_registry: self.e2e_registry, interface: self.interface, spawner, + buffer_provider: self.buffer_provider, } } @@ -446,13 +469,32 @@ where pub fn with_local_spawner( self, spawner: Sp2, - ) -> ClientDeps { + ) -> ClientDeps { ClientDeps { factory: self.factory, timer: self.timer, e2e_registry: self.e2e_registry, interface: self.interface, spawner, + buffer_provider: self.buffer_provider, + } + } + + /// Replace the `buffer_provider` field, returning a `ClientDeps` + /// over the new provider type. Bare-metal callers use this to supply + /// a [`StaticBufferProvider`](crate::transport::StaticBufferProvider) + /// backed by a consumer-declared `static BufferPool`. + pub fn with_buffer_provider( + self, + buffer_provider: BP2, + ) -> ClientDeps { + ClientDeps { + factory: self.factory, + timer: self.timer, + e2e_registry: self.e2e_registry, + interface: self.interface, + spawner: self.spawner, + buffer_provider, } } } @@ -642,6 +684,10 @@ where e2e_registry: Arc::new(Mutex::new(E2ERegistry::new())), interface: Arc::new(RwLock::new(interface)), spawner, + // One `TokioBufferProvider::new()` per client construction. + // It `Box::leak`s internally, so it must not be moved to a + // per-bind path; this single call covers every `bind_*`. + buffer_provider: crate::tokio_transport::TokioBufferProvider::new(), }, multicast_loopback, ) @@ -691,8 +737,8 @@ where /// `LocalSet`-style spawner shim. #[allow(clippy::type_complexity)] #[must_use = "the returned run-loop future must be spawned (e.g. via the Spawner) for the client to make progress"] - pub fn new_with_deps( - deps: ClientDeps, + pub fn new_with_deps( + deps: ClientDeps, multicast_loopback: bool, ) -> ( Self, @@ -708,6 +754,7 @@ where Sp: Spawner + Send + Sync + 'static, Tm: Timer + Send + Sync + 'static, for<'a> Tm::SleepFuture<'a>: Send, + BP: crate::transport::BufferProvider, { let ClientDeps { factory, @@ -715,11 +762,16 @@ where e2e_registry, interface, spawner, + buffer_provider, } = deps; let initial_addr = interface.get(); - let dispatch = bind_dispatch::SpawnerDispatch { factory, spawner }; + let dispatch = bind_dispatch::SpawnerDispatch { + factory, + spawner, + buffer_provider, + }; let (control_sender, update_receiver, run_future) = - Inner::>::build( + Inner::>::build( initial_addr, e2e_registry.clone(), multicast_loopback, @@ -752,8 +804,8 @@ where /// [`Spawner`]: crate::transport::Spawner #[allow(clippy::type_complexity)] #[must_use = "the returned run-loop future must be spawned (e.g. via the LocalSpawner) for the client to make progress"] - pub fn new_with_deps_local( - deps: ClientDeps, + pub fn new_with_deps_local( + deps: ClientDeps, multicast_loopback: bool, ) -> ( Self, @@ -765,6 +817,7 @@ where F::Socket: 'static, Sp: crate::transport::LocalSpawner + 'static, Tm: Timer + 'static, + BP: crate::transport::BufferProvider, { let ClientDeps { factory, @@ -772,15 +825,20 @@ where e2e_registry, interface, spawner, + buffer_provider, } = deps; let initial_addr = interface.get(); - let dispatch = bind_dispatch::LocalSpawnerDispatch { factory, spawner }; + let dispatch = bind_dispatch::LocalSpawnerDispatch { + factory, + spawner, + buffer_provider, + }; let (control_sender, update_receiver, run_future) = Inner::< MessageDefinitions, Tm, R, C, - bind_dispatch::LocalSpawnerDispatch, + bind_dispatch::LocalSpawnerDispatch, >::build( initial_addr, e2e_registry.clone(), diff --git a/src/client/socket_manager.rs b/src/client/socket_manager.rs index 72fc1b12..4236c7d9 100644 --- a/src/client/socket_manager.rs +++ b/src/client/socket_manager.rs @@ -42,6 +42,7 @@ use crate::{ UDP_BUFFER_SIZE, + buffer_pool::BufferLease, e2e::{E2ECheckStatus, E2EKey}, protocol::{Message, MessageView, sd}, traits::{PayloadWireFormat, WireFormat}, @@ -182,7 +183,11 @@ where session_has_wrapped: bool, multicast_loopback: bool, ) -> Result { - use crate::tokio_transport::{TokioSpawner, TokioTransport}; + use crate::tokio_transport::{TokioBufferProvider, TokioSpawner, TokioTransport}; + use crate::transport::BufferProvider; + let buf = TokioBufferProvider::new() + .claim() + .ok_or(Error::Capacity("udp_buffer"))?; Self::bind_discovery_seeded_with_transport( &TokioTransport, &TokioSpawner, @@ -191,6 +196,7 @@ where session_id, session_has_wrapped, multicast_loopback, + buf, ) .await } @@ -228,6 +234,7 @@ where /// (e.g. wrapping `embassy_net::udp::UdpSocket`) as long as it is /// `Send + Sync + 'static` and its `SendFuture` / `RecvFuture` GAT /// projections are `Send` for every borrow lifetime. + #[allow(clippy::too_many_arguments)] // +1 for the #125 caller-provided buffer lease pub async fn bind_discovery_seeded_with_transport( factory: &F, spawner: &S, @@ -236,6 +243,7 @@ where session_id: u16, session_has_wrapped: bool, multicast_loopback: bool, + buf: BufferLease, ) -> Result where F: TransportFactory, @@ -269,7 +277,7 @@ where let socket = factory.bind(bind_addr, &options).await?; socket.join_multicast_v4(sd::MULTICAST_IP, interface)?; - let fut = Self::socket_loop_future(socket, rx_tx, tx_rx, e2e_registry); + let fut = Self::socket_loop_future(socket, rx_tx, tx_rx, e2e_registry, buf); spawner.spawn(fut); Ok(Self { receiver: rx_rx, @@ -284,6 +292,7 @@ where /// /// Called by [`super::bind_dispatch::LocalSpawnerDispatch`] which is /// wired through [`super::Client::new_with_deps_local`]. + #[allow(clippy::too_many_arguments)] // +1 for the #125 caller-provided buffer lease pub async fn bind_discovery_seeded_with_transport_local( factory: &F, spawner: &S, @@ -292,6 +301,7 @@ where session_id: u16, session_has_wrapped: bool, multicast_loopback: bool, + buf: BufferLease, ) -> Result where F: TransportFactory, @@ -312,7 +322,7 @@ where let bind_addr = SocketAddrV4::new(Ipv4Addr::UNSPECIFIED, sd::MULTICAST_PORT); let socket = factory.bind(bind_addr, &options).await?; socket.join_multicast_v4(sd::MULTICAST_IP, interface)?; - let fut = Self::socket_loop_future(socket, rx_tx, tx_rx, e2e_registry); + let fut = Self::socket_loop_future(socket, rx_tx, tx_rx, e2e_registry, buf); spawner.spawn_local(fut); Ok(Self { receiver: rx_rx, @@ -337,8 +347,12 @@ where /// behind it. #[cfg(all(test, feature = "client-tokio"))] pub async fn bind(port: u16, e2e_registry: R) -> Result { - use crate::tokio_transport::{TokioSpawner, TokioTransport}; - Self::bind_with_transport(&TokioTransport, &TokioSpawner, port, e2e_registry).await + use crate::tokio_transport::{TokioBufferProvider, TokioSpawner, TokioTransport}; + use crate::transport::BufferProvider; + let buf = TokioBufferProvider::new() + .claim() + .ok_or(Error::Capacity("udp_buffer"))?; + Self::bind_with_transport(&TokioTransport, &TokioSpawner, port, e2e_registry, buf).await } /// Variant of [`Self::bind`] that constructs the underlying socket @@ -357,6 +371,7 @@ where spawner: &S, port: u16, e2e_registry: R, + buf: BufferLease, ) -> Result where F: TransportFactory, @@ -386,7 +401,7 @@ where let socket = factory.bind(bind_addr, &options).await?; let port = socket.local_addr()?.port(); - let fut = Self::socket_loop_future(socket, rx_tx, tx_rx, e2e_registry); + let fut = Self::socket_loop_future(socket, rx_tx, tx_rx, e2e_registry, buf); spawner.spawn(fut); Ok(Self { receiver: rx_rx, @@ -410,6 +425,7 @@ where spawner: &S, port: u16, e2e_registry: R, + buf: BufferLease, ) -> Result where F: TransportFactory, @@ -427,7 +443,7 @@ where let bind_addr = SocketAddrV4::new(Ipv4Addr::UNSPECIFIED, port); let socket = factory.bind(bind_addr, &options).await?; let port = socket.local_addr()?.port(); - let fut = Self::socket_loop_future(socket, rx_tx, tx_rx, e2e_registry); + let fut = Self::socket_loop_future(socket, rx_tx, tx_rx, e2e_registry, buf); spawner.spawn_local(fut); Ok(Self { receiver: rx_rx, @@ -553,6 +569,7 @@ where rx_tx: C::BoundedSender, Error>, 16>, mut tx_rx: C::BoundedReceiver, 16>, e2e_registry: R, + mut buf: BufferLease, ) where T: TransportSocket + 'static, R: E2ERegistryHandle, @@ -566,7 +583,11 @@ where // tight `error!` log loop with no exit; this counter caps that. const MAX_CONSECUTIVE_RECV_ERRORS: u32 = 16; let mut consecutive_recv_errors: u32 = 0; - let mut buf = [0u8; UDP_BUFFER_SIZE]; + // The receive/scratch buffer is now leased from a caller-provided + // `BufferProvider` (see `#125`): on bare-metal it is a slot of a + // consumer-declared `static BufferPool`; on tokio it is heap-backed. + // The lease is owned by this future and frees its pool slot on drop + // when the loop exits. // Iteration counter used solely to flip `select_biased!` arm // priority each turn so a sustained one-sided load (only-send // or only-recv) cannot starve the other arm. We can't use @@ -585,7 +606,7 @@ where // below runs, so the body can re-borrow `buf` freely. let outcome: Outcome = { let send_fut = MpscRecv::recv(&mut tx_rx).fuse(); - let recv_fut = socket.recv_from(&mut buf).fuse(); + let recv_fut = socket.recv_from(&mut buf[..]).fuse(); pin_mut!(send_fut, recv_fut); if prefer_recv_first { select_biased! { @@ -604,8 +625,24 @@ where match outcome { Outcome::Send(Some(send_message)) => { trace!("Sending: {:?}", &send_message); + // Oversize-send rejection keys off the claimed buffer's + // length (`#125`), not the compile-time `UDP_BUFFER_SIZE`: + // a caller-sized bare-metal pool may hand out a buffer + // smaller than `UDP_BUFFER_SIZE`, and the message must fit + // the buffer we actually encode into. + let required = send_message.message.required_size(); + if required > buf.len() { + warn!( + "outgoing message size {required} exceeds claimed buffer ({}); rejecting with Capacity(\"udp_buffer\")", + buf.len() + ); + let _ = send_message + .response + .send(Err(Error::Capacity("udp_buffer"))); + continue; + } let mut message_length = - match send_message.message.encode(&mut buf.as_mut_slice()) { + match send_message.message.encode(&mut &mut buf[..]) { Ok(length) => length, Err(e) => { error!("Failed to encode message: {:?}", e); @@ -707,6 +744,18 @@ where truncated, })) => { consecutive_recv_errors = 0; + if bytes_received > buf.len() { + // A backend reported a datagram larger than the + // claimed buffer. Parsing `&buf[..bytes_received]` + // would index out of bounds (and the bytes past + // `buf.len()` were never written), so drop it + // rather than parse a truncated buffer. + warn!( + "inbound datagram ({bytes_received} B) exceeds claimed buffer ({} B); dropping", + buf.len() + ); + continue; + } if truncated { // A truncated datagram cannot be parsed reliably; // the length field in the SOME/IP header will not @@ -809,6 +858,16 @@ mod tests { Arc::new(Mutex::new(E2ERegistry::new())) } + /// Claim a single socket-loop buffer for a direct `bind_with_transport` + /// call in these unit tests. Each call leaks one small `BufferPool` + /// (acceptable in a test); production paths claim from one shared + /// provider per client. + fn test_buf() -> crate::buffer_pool::BufferLease { + use crate::tokio_transport::TokioBufferProvider; + use crate::transport::BufferProvider; + TokioBufferProvider::new().claim().expect("fresh pool slot") + } + async fn bind_ephemeral_spawned() -> TestSocketManager { TestSocketManager::bind(0, test_registry()).await.unwrap() } @@ -1132,10 +1191,15 @@ mod tests { calls: AtomicUsize::new(0), }; - let sm = - TestSocketManager::bind_with_transport(&factory, &TokioSpawner, 0, test_registry()) - .await - .expect("bind via custom factory"); + let sm = TestSocketManager::bind_with_transport( + &factory, + &TokioSpawner, + 0, + test_registry(), + test_buf(), + ) + .await + .expect("bind via custom factory"); assert_eq!( factory.calls.load(Ordering::SeqCst), 1, @@ -1184,6 +1248,7 @@ mod tests { &TokioSpawner, 0, test_registry(), + test_buf(), ) .await .expect("bind via custom factory"); @@ -1297,6 +1362,7 @@ mod tests { &TokioSpawner, 0, test_registry(), + test_buf(), ) .await .expect("bind via wrapping factory"); @@ -1355,6 +1421,7 @@ mod tests { &TokioSpawner, 0, test_registry(), + test_buf(), ) .await .expect_err("factory returned Err, bind must surface it"); diff --git a/tests/bare_metal_client.rs b/tests/bare_metal_client.rs index 3de10d3d..02e0cc7d 100644 --- a/tests/bare_metal_client.rs +++ b/tests/bare_metal_client.rs @@ -44,11 +44,12 @@ use simple_someip::client::{ClientUpdate, ControlMessage, ReceivedMessage, SendM use simple_someip::define_static_channels; use simple_someip::e2e::E2ERegistry; use simple_someip::protocol::sd::RebootFlag; +use simple_someip::static_channels::BufferPool; use simple_someip::transport::{ - ReceivedDatagram, SocketOptions, Spawner, Timer, TransportError, TransportFactory, - TransportSocket, + ReceivedDatagram, SocketOptions, Spawner, StaticBufferProvider, Timer, TransportError, + TransportFactory, TransportSocket, }; -use simple_someip::{Client, ClientDeps, RawPayload}; +use simple_someip::{Client, ClientDeps, RawPayload, UDP_BUFFER_SIZE}; // ── Static-pool channel factory declared via the macro ──────────────── // @@ -249,6 +250,8 @@ async fn client_constructible_without_client_tokio_feature() { Arc::new(std::sync::RwLock::new(Ipv4Addr::LOCALHOST)); let e2e_handle: Arc> = Arc::new(Mutex::new(E2ERegistry::new())); + static POOL: BufferPool<9, UDP_BUFFER_SIZE> = BufferPool::new(); + let (client, _updates, run_fut) = Client::< RawPayload, Arc>, @@ -261,6 +264,7 @@ async fn client_constructible_without_client_tokio_feature() { timer: MockTimer, e2e_registry: e2e_handle, interface: interface_handle, + buffer_provider: StaticBufferProvider(&POOL), }, false, ); diff --git a/tests/bare_metal_client_local.rs b/tests/bare_metal_client_local.rs index b670436a..03cf2cc4 100644 --- a/tests/bare_metal_client_local.rs +++ b/tests/bare_metal_client_local.rs @@ -18,11 +18,12 @@ use simple_someip::client::{ClientUpdate, ControlMessage, ReceivedMessage, SendM use simple_someip::define_static_channels; use simple_someip::e2e::E2ERegistry; use simple_someip::protocol::sd::RebootFlag; +use simple_someip::static_channels::BufferPool; use simple_someip::transport::{ - LocalSpawner, ReceivedDatagram, SocketOptions, Timer, TransportError, TransportFactory, - TransportSocket, + LocalSpawner, ReceivedDatagram, SocketOptions, StaticBufferProvider, Timer, TransportError, + TransportFactory, TransportSocket, }; -use simple_someip::{Client, ClientDeps, RawPayload}; +use simple_someip::{Client, ClientDeps, RawPayload, UDP_BUFFER_SIZE}; define_static_channels! { name: LocalChannels, @@ -196,6 +197,8 @@ async fn client_constructible_with_local_spawner() { Arc::new(std::sync::RwLock::new(Ipv4Addr::LOCALHOST)); let e2e_handle: Arc> = Arc::new(Mutex::new(E2ERegistry::new())); + static POOL: BufferPool<9, UDP_BUFFER_SIZE> = BufferPool::new(); + let (client, _updates, run_fut) = Client::< RawPayload, Arc>, @@ -208,6 +211,7 @@ async fn client_constructible_with_local_spawner() { timer: MockTimer, e2e_registry: e2e_handle, interface: interface_handle, + buffer_provider: StaticBufferProvider(&POOL), }, false, ); diff --git a/tests/bare_metal_e2e.rs b/tests/bare_metal_e2e.rs index dedeb6f4..31776ec2 100644 --- a/tests/bare_metal_e2e.rs +++ b/tests/bare_metal_e2e.rs @@ -36,21 +36,25 @@ use simple_someip::protocol::{ Header, Message, MessageId, MessageType, MessageTypeField, ReturnCode, }; use simple_someip::server::{ServerConfig, SubscribeError, Subscriber, SubscriptionHandle}; +use simple_someip::WireFormat; +use simple_someip::static_channels::BufferPool; use simple_someip::transport::{ - ReceivedDatagram, SocketOptions, Spawner, Timer, TransportError, TransportFactory, - TransportSocket, + ReceivedDatagram, SocketOptions, Spawner, StaticBufferProvider, Timer, TransportError, + TransportFactory, TransportSocket, }; -use simple_someip::{Client, ClientDeps, RawPayload, Server, ServerDeps}; +use simple_someip::{Client, ClientDeps, RawPayload, Server, ServerDeps, UDP_BUFFER_SIZE}; // ── Static-pool channel factory ─────────────────────────────────────── // // Pool budget: each `Client::new_with_deps` claims one `ControlMessage` // bounded slot and one `ClientUpdate` unbounded slot for the lifetime -// of the client. Both pools hold 4. A plain parallel `cargo test` runs -// every test in this file in ONE process, so concurrent tests share -// these pools — currently 3 client-constructing tests worst-case. If a -// new test pushes that past 4, grow the two pool counts below or the -// exhaustion panic will land in whichever test loses the race. +// of the client. A plain parallel `cargo test` runs every test in this +// file in ONE process, so concurrent tests share these pools. As of the +// #125 buffer-provider work there are 6 client-constructing tests +// worst-case (the two new buffer tests join the original four), so both +// pools hold 8. If a new test pushes past 8, grow the two pool counts +// below or the exhaustion panic will land in whichever test loses the +// race. // // NOTE: `tools/size_probe`'s `ProbeChannels` mirrors this entry list // for thumbv7em layout capture. If you change the entries here, @@ -64,12 +68,12 @@ define_static_channels! { (Result, 8), ], bounded: [ - ((ControlMessage, 4), 4), - ((SendMessage, 16), 8), - ((Result, ClientError>, 16), 8), + ((ControlMessage, 4), 8), + ((SendMessage, 16), 12), + ((Result, ClientError>, 16), 12), ], unbounded: [ - (ClientUpdate, 4), + (ClientUpdate, 8), ], } @@ -393,12 +397,14 @@ async fn client_receives_server_sd_announcement() { let client_e2e: Arc> = Arc::new(Mutex::new(E2ERegistry::new())); let client_iface: Arc> = Arc::new(RwLock::new(Ipv4Addr::LOCALHOST)); + static POOL_SD: BufferPool<9, UDP_BUFFER_SIZE> = BufferPool::new(); let client_deps = ClientDeps { factory: client_factory, spawner: TokioBackedSpawner, timer: MockTimer, e2e_registry: client_e2e, interface: client_iface, + buffer_provider: StaticBufferProvider(&POOL_SD), }; let (client, mut updates, run_fut) = Client::< @@ -492,12 +498,14 @@ async fn client_send_request_server_runloop_stable() { let client_e2e: Arc> = Arc::new(Mutex::new(E2ERegistry::new())); let client_iface: Arc> = Arc::new(RwLock::new(Ipv4Addr::LOCALHOST)); + static POOL_REQ: BufferPool<9, UDP_BUFFER_SIZE> = BufferPool::new(); let client_deps = ClientDeps { factory: client_factory, spawner: TokioBackedSpawner, timer: MockTimer, e2e_registry: client_e2e, interface: client_iface, + buffer_provider: StaticBufferProvider(&POOL_REQ), }; let (client, mut updates, client_run_fut) = Client::< @@ -650,6 +658,7 @@ async fn future_size_witness_bare_metal_channels() { next_port: Arc::new(Mutex::new(100)), }; let max_spawned = Arc::new(AtomicUsize::new(0)); + static POOL_WITNESS: BufferPool<9, UDP_BUFFER_SIZE> = BufferPool::new(); let client_deps = ClientDeps { factory: client_factory, spawner: SizeRecordingSpawner { @@ -658,6 +667,7 @@ async fn future_size_witness_bare_metal_channels() { timer: MockTimer, e2e_registry: Arc::new(Mutex::new(E2ERegistry::new())), interface: Arc::new(RwLock::new(Ipv4Addr::LOCALHOST)), + buffer_provider: StaticBufferProvider(&POOL_WITNESS), }; let (client, _updates, run_fut) = Client::< RawPayload, @@ -690,3 +700,278 @@ async fn future_size_witness_bare_metal_channels() { ); client.shut_down(); } + +// ── Task 3 harness: a recv that reports an UNCLAMPED datagram length ─── +// +// `MockSocket` above clamps `bytes_received` to `buf.len()` and flags +// `truncated`, which would exercise the pre-existing truncation drop. To +// exercise the NEW `bytes_received > buf.len()` guard in +// `socket_loop_future` directly, this harness reports the datagram's +// ORIGINAL length (possibly larger than the loop's claimed buffer) with +// `truncated: false`. `SocketManager` is a private module, so the guard is +// driven end-to-end through the public `Client` discovery socket. + +/// Scripted inbound queue: each entry is `(bytes_to_copy, reported_len)`. +/// `reported_len` is what the socket reports as `bytes_received`, which may +/// exceed the caller's buffer to simulate an oversized datagram. +#[derive(Default)] +struct ScriptPipe { + queue: Mutex, usize, SocketAddrV4)>>, + waker: Mutex>, +} + +impl ScriptPipe { + fn push(&self, bytes: Vec, reported_len: usize, source: SocketAddrV4) { + self.queue + .lock() + .unwrap() + .push_back((bytes, reported_len, source)); + if let Some(w) = self.waker.lock().unwrap().take() { + w.wake(); + } + } +} + +#[derive(Clone)] +struct ScriptFactory { + rx: Arc, +} + +impl TransportFactory for ScriptFactory { + type Socket = ScriptSocket; + type BindFuture<'a> = + core::pin::Pin> + Send + 'a>>; + fn bind<'a>(&'a self, addr: SocketAddrV4, _options: &'a SocketOptions) -> Self::BindFuture<'a> { + let rx = Arc::clone(&self.rx); + let local = SocketAddrV4::new(*addr.ip(), if addr.port() == 0 { 41000 } else { addr.port() }); + Box::pin(async move { Ok(ScriptSocket { rx, local }) }) + } +} + +struct ScriptSocket { + rx: Arc, + local: SocketAddrV4, +} + +struct ScriptRecvFut<'a> { + rx: Arc, + buf: &'a mut [u8], +} + +impl Future for ScriptRecvFut<'_> { + type Output = Result; + fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { + let me = self.get_mut(); + if let Some((bytes, reported_len, source)) = me.rx.queue.lock().unwrap().pop_front() { + // Copy only what fits (a real backend would never write past the + // buffer), but report the ORIGINAL length so the loop's + // `bytes_received > buf.len()` guard can fire. + let n = bytes.len().min(me.buf.len()); + me.buf[..n].copy_from_slice(&bytes[..n]); + return Poll::Ready(Ok(ReceivedDatagram { + bytes_received: reported_len, + source, + truncated: false, + })); + } + *me.rx.waker.lock().unwrap() = Some(cx.waker().clone()); + Poll::Pending + } +} + +impl TransportSocket for ScriptSocket { + type SendFuture<'a> = MockSendFut; + type RecvFuture<'a> = ScriptRecvFut<'a>; + fn send_to<'a>(&'a self, _buf: &'a [u8], _target: SocketAddrV4) -> Self::SendFuture<'a> { + // Sends are unused by this test; resolve immediately into a dead pipe. + MockSendFut { + pipe: Arc::new(MockPipe::default()), + bytes: None, + source: self.local, + } + } + fn recv_from<'a>(&'a self, buf: &'a mut [u8]) -> Self::RecvFuture<'a> { + ScriptRecvFut { + rx: Arc::clone(&self.rx), + buf, + } + } + fn local_addr(&self) -> Result { + Ok(self.local) + } + fn join_multicast_v4(&self, _g: Ipv4Addr, _i: Ipv4Addr) -> Result<(), TransportError> { + Ok(()) + } + fn leave_multicast_v4(&self, _g: Ipv4Addr, _i: Ipv4Addr) -> Result<(), TransportError> { + Ok(()) + } +} + +/// Task 3: an inbound datagram reported larger than the loop's claimed +/// buffer must be dropped (not parsed from a truncated buffer), the loop +/// must survive, and a subsequent in-budget datagram must still be +/// delivered. Driven through the public `Client` discovery socket because +/// `socket_loop_future` / `SocketManager` are private. +#[tokio::test] +async fn inbound_datagram_larger_than_claimed_buffer_is_dropped_not_fatal() { + // Claim buffers of exactly 64 bytes: big enough for a small SD message + // (16-byte SOME/IP header + a short SD payload), too small for the + // scripted 256-byte oversized datagram. + const BUF_LEN: usize = 64; + static POOL: BufferPool<2, BUF_LEN> = BufferPool::new(); + + let rx = Arc::new(ScriptPipe::default()); + let factory = ScriptFactory { rx: Arc::clone(&rx) }; + let source = SocketAddrV4::new(Ipv4Addr::new(127, 0, 0, 1), 30490); + + // Oversized datagram first: 256 reported bytes into a 64-byte buffer. + rx.push(vec![0xFFu8; BUF_LEN], 256, source); + + // Then a valid small SD message that fits the 64-byte buffer. + let sd_msg = Message::::new_sd(1, &empty_vec_sd_header()); + let mut wire = vec![0u8; BUF_LEN]; + let len = sd_msg.encode(&mut wire.as_mut_slice()).expect("encode sd"); + assert!(len <= BUF_LEN, "valid SD message must fit the claimed buffer"); + rx.push(wire[..len].to_vec(), len, source); + + let client_e2e: Arc> = Arc::new(Mutex::new(E2ERegistry::new())); + let client_iface: Arc> = Arc::new(RwLock::new(Ipv4Addr::LOCALHOST)); + let deps = ClientDeps { + factory, + spawner: TokioBackedSpawner, + timer: MockTimer, + e2e_registry: client_e2e, + interface: client_iface, + buffer_provider: StaticBufferProvider(&POOL), + }; + let (client, mut updates, run_fut) = Client::< + RawPayload, + Arc>, + Arc>, + E2ETestChannels, + >::new_with_deps(deps, false); + let run_handle = tokio::spawn(run_fut); + + client.bind_discovery().await.expect("bind_discovery"); + + // The oversized datagram must be dropped and the loop must survive long + // enough to deliver the valid SD message as a discovery update. + let got = tokio::time::timeout(Duration::from_secs(2), async { + while let Some(update) = updates.recv().await { + if let ClientUpdate::DiscoveryUpdated(_) = update { + return true; + } + } + false + }) + .await; + + assert!( + got.unwrap_or(false), + "loop must drop the oversized datagram, survive, and deliver the in-budget SD message" + ); + + client.shut_down(); + run_handle.abort(); +} + +/// Task 4: binding N unicast sockets claims N buffers from the shared pool; +/// a pool with exactly 2 slots rejects the 3rd distinct bind with +/// `Error::Capacity("udp_buffer")`. Driven through the public `Client` +/// `send_to_service` path, which binds one unicast socket per distinct +/// endpoint `local_port` and surfaces the bind error to the caller. +/// +/// Note on release-on-close: `socket_loop_future` owns the `BufferLease` +/// and frees the pool slot when the loop exits (RAII on future drop). That +/// release is exercised at the unit level in `tests/buffer_pool.rs` +/// (`BufferLease::drop`); the public `Client` API exposes no per-socket +/// close, so the integration test here focuses on the claim + exhaustion +/// contract, which is the new bind-path behavior #125 introduces. +#[tokio::test] +async fn each_bound_socket_claims_one_buffer_and_releases_on_close() { + // Exactly 2 slots so the 3rd concurrent unicast bind exhausts the pool. + static POOL: BufferPool<2, UDP_BUFFER_SIZE> = BufferPool::new(); + + let network = SharedNetwork::new(); + let client_factory = MockFactory { + tx_pipe: Arc::clone(&network.client_to_server), + rx_pipe: Arc::clone(&network.server_to_client), + next_port: Arc::new(Mutex::new(0)), + }; + + let client_e2e: Arc> = Arc::new(Mutex::new(E2ERegistry::new())); + let client_iface: Arc> = Arc::new(RwLock::new(Ipv4Addr::LOCALHOST)); + let deps = ClientDeps { + factory: client_factory, + spawner: TokioBackedSpawner, + timer: MockTimer, + e2e_registry: client_e2e, + interface: client_iface, + buffer_provider: StaticBufferProvider(&POOL), + }; + let (client, _updates, run_fut) = Client::< + RawPayload, + Arc>, + Arc>, + E2ETestChannels, + >::new_with_deps(deps, false); + let run_handle = tokio::spawn(run_fut); + + let target = SocketAddrV4::new(Ipv4Addr::LOCALHOST, 30700); + + // Each distinct (service, local_port) forces a distinct unicast bind, + // each of which claims one buffer from the 2-slot pool. `send_to_service` + // with a non-zero `local_port` binds that exact source port (no discovery + // auto-bind, unlike `subscribe`). + let bind_via_send = |svc: u16, port: u16| { + let client = client.clone(); + async move { + client + .add_endpoint(svc, 1, target, port) + .await + .expect("add_endpoint"); + let msg_id = MessageId::new_from_service_and_method(svc, 0x0001); + let payload = RawPayload::from_payload_bytes(msg_id, &[0u8; 4]).expect("payload"); + let request = Message::::new( + Header::new( + msg_id, + 0x0001_0001, + 1, + 1, + MessageTypeField::new(MessageType::Request, false), + ReturnCode::Ok, + 4, + ), + payload, + ); + client.send_to_service(svc, 1, request).await.map(|_| ()) + } + }; + + bind_via_send(0x4001, 40000) + .await + .expect("1st bind claims slot 0"); + bind_via_send(0x4002, 40001) + .await + .expect("2nd bind claims slot 1"); + + // 3rd distinct port: pool exhausted -> typed capacity error surfaces. + let third = bind_via_send(0x4003, 40002).await; + assert!( + matches!(third, Err(ClientError::Capacity("udp_buffer"))), + "3rd bind must fail with Capacity(\"udp_buffer\"), got {third:?}" + ); + + client.shut_down(); + run_handle.abort(); +} + +/// An empty `VecSdHeader` for building a minimal valid SD message. +fn empty_vec_sd_header() -> simple_someip::VecSdHeader { + use simple_someip::protocol::sd::{Flags, RebootFlag}; + simple_someip::VecSdHeader { + flags: Flags::new_sd(RebootFlag::RecentlyRebooted), + entries: vec![], + options: vec![], + } +} diff --git a/tests/static_channels_alloc_witness.rs b/tests/static_channels_alloc_witness.rs index 6db9ea5e..654968de 100644 --- a/tests/static_channels_alloc_witness.rs +++ b/tests/static_channels_alloc_witness.rs @@ -52,11 +52,12 @@ use simple_someip::client::{ClientUpdate, ControlMessage, ReceivedMessage, SendM use simple_someip::define_static_channels; use simple_someip::e2e::E2ERegistry; use simple_someip::protocol::sd::RebootFlag; +use simple_someip::static_channels::BufferPool; use simple_someip::transport::{ - ChannelFactory, OneshotSend, ReceivedDatagram, SocketOptions, Spawner, Timer, TransportError, - TransportFactory, TransportSocket, + ChannelFactory, OneshotSend, ReceivedDatagram, SocketOptions, Spawner, StaticBufferProvider, + Timer, TransportError, TransportFactory, TransportSocket, }; -use simple_someip::{Client, ClientDeps, RawPayload}; +use simple_someip::{Client, ClientDeps, RawPayload, UDP_BUFFER_SIZE}; // ── Counting global allocator ───────────────────────────────────────── @@ -299,6 +300,8 @@ async fn client_interface_read_after_construction_does_not_allocate() { Arc::new(std::sync::RwLock::new(Ipv4Addr::LOCALHOST)); let e2e_handle: Arc> = Arc::new(Mutex::new(E2ERegistry::new())); + static POOL: BufferPool<9, UDP_BUFFER_SIZE> = BufferPool::new(); + let (client, _updates, run_fut) = Client::< RawPayload, Arc>, @@ -311,6 +314,7 @@ async fn client_interface_read_after_construction_does_not_allocate() { timer: MockTimer, e2e_registry: e2e_handle, interface: interface_handle, + buffer_provider: StaticBufferProvider(&POOL), }, false, ); diff --git a/tools/size_probe/src/lib.rs b/tools/size_probe/src/lib.rs index 2ce0cf13..87a0ef97 100644 --- a/tools/size_probe/src/lib.rs +++ b/tools/size_probe/src/lib.rs @@ -247,10 +247,12 @@ mod client_future_probe { use simple_someip::client::{ClientUpdate, ControlMessage, ReceivedMessage, SendMessage}; use simple_someip::protocol::sd::RebootFlag; use simple_someip::protocol::{MessageId, sd}; + use simple_someip::static_channels::BufferPool; + use simple_someip::transport::StaticBufferProvider; use simple_someip::transport::probe::{ NullE2ERegistry, NullFactory, NullInterface, NullSpawner, NullTimer, }; - use simple_someip::{Client, ClientDeps, PayloadWireFormat, WireFormat}; + use simple_someip::{Client, ClientDeps, PayloadWireFormat, UDP_BUFFER_SIZE, WireFormat}; // `RawPayload` is std-gated (heap `Vec` SD storage), so the probe // carries its own minimal no_std `PayloadWireFormat` impl — @@ -397,12 +399,14 @@ mod client_future_probe { #[unsafe(no_mangle)] pub extern "C" fn probe_client_run_future_size() -> usize { + static POOL: BufferPool<9, UDP_BUFFER_SIZE> = BufferPool::new(); let deps = ClientDeps { factory: NullFactory, spawner: NullSpawner, timer: NullTimer, e2e_registry: NullE2ERegistry, interface: NullInterface(core::net::Ipv4Addr::LOCALHOST), + buffer_provider: StaticBufferProvider(&POOL), }; let (_client, _updates, run_fut) = Client::< ProbePayload, From d8c4b794a10750a4c11116993b1c15ff2dc877e2 Mon Sep 17 00:00:00 2001 From: Justin Kovacich Date: Wed, 17 Jun 2026 10:46:18 -0400 Subject: [PATCH 07/12] test(client): rename buffer-claim test to match its actual coverage (#125) Task 3+4 review (Minor): the test asserts claim+exhaustion, not release-on-close (RAII release is unit-covered in tests/buffer_pool.rs). Inert fn rename, no callers. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/bare_metal_e2e.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/bare_metal_e2e.rs b/tests/bare_metal_e2e.rs index 31776ec2..5b2c62c3 100644 --- a/tests/bare_metal_e2e.rs +++ b/tests/bare_metal_e2e.rs @@ -888,7 +888,7 @@ async fn inbound_datagram_larger_than_claimed_buffer_is_dropped_not_fatal() { /// close, so the integration test here focuses on the claim + exhaustion /// contract, which is the new bind-path behavior #125 introduces. #[tokio::test] -async fn each_bound_socket_claims_one_buffer_and_releases_on_close() { +async fn binding_sockets_claims_one_buffer_each_until_pool_exhausted() { // Exactly 2 slots so the 3rd concurrent unicast bind exhausts the pool. static POOL: BufferPool<2, UDP_BUFFER_SIZE> = BufferPool::new(); From a121539ba6cd3e932362f9b9dc707418ac13a3f2 Mon Sep 17 00:00:00 2001 From: Justin Kovacich Date: Wed, 17 Jun 2026 11:15:33 -0400 Subject: [PATCH 08/12] test+docs(client): tighten socket-loop future budget to 1024 B; rewrite footprint doc (#125) Socket-loop future is 776 B post-extraction (was ~2224); budget now ceil64(776*1.25)=1024, turning the #125 win into a regression tripwire. Footprint doc rewritten for pooled, caller-sized buffers. (Task 7 of the #125 plan.) Co-Authored-By: Claude Opus 4.8 (1M context) --- src/client/mod.rs | 37 ++++++++++++------------------------- tests/bare_metal_e2e.rs | 4 ++-- 2 files changed, 14 insertions(+), 27 deletions(-) diff --git a/src/client/mod.rs b/src/client/mod.rs index 601d0542..5fea706b 100644 --- a/src/client/mod.rs +++ b/src/client/mod.rs @@ -2,32 +2,19 @@ //! //! # Memory footprint //! -//! The client's internal `Inner` state is allocated inline rather than on -//! the heap. With the default capacity constants declared in `inner.rs` — -//! `REQUEST_QUEUE_CAP=32`, `PENDING_RESPONSES_CAP=64`, `UNICAST_SOCKETS_CAP=8`, -//! and `SESSION_CAP=64` — `Inner

` occupies on the order of **8–12 KiB**, -//! depending on `sizeof::

()` and `sizeof::>()`. +//! The client's `Inner` state is allocated inline. The per-socket +//! `UDP_BUFFER_SIZE` receive buffers are **not** part of the spawned +//! socket-loop futures: each loop claims a `&'static mut [u8]` from a +//! [`BufferProvider`](crate::transport::BufferProvider) at bind and releases +//! it when the socket closes. On the bare-metal path the consumer declares +//! the backing `BufferPool` as a `static`, choosing both the slot count and +//! the per-slot length (e.g. 2 × 512 B), so the buffer budget lives in +//! `.bss` and is sized by the caller rather than fixed at +//! `UNICAST_SOCKETS_CAP × UDP_BUFFER_SIZE`. On `std + tokio` the provider +//! is heap-backed and provisioned internally (`UDP_BUFFER_SIZE`-sized slots), +//! invisible to callers. //! -//! In addition, each `SocketManager`'s spawn loop holds a persistent -//! `[u8; UDP_BUFFER_SIZE]` receive/send buffer. When the send path needs -//! E2E protection (i.e. the destination key is registered in the -//! `E2ERegistry`), it transiently allocates a second -//! `[u8; UDP_BUFFER_SIZE]` on the stack for the protected output; sends -//! without E2E protection do not pay this cost. So an active -//! socket-loop future carries one always-live `UDP_BUFFER_SIZE` buffer -//! plus up to one additional `UDP_BUFFER_SIZE` buffer during E2E sends. -//! With `UNICAST_SOCKETS_CAP=8` sockets bound, the total per-client -//! buffer budget scales as `UNICAST_SOCKETS_CAP * UDP_BUFFER_SIZE` -//! always-live, up to `2 * UNICAST_SOCKETS_CAP * UDP_BUFFER_SIZE` at -//! peak during concurrent E2E-protected sends on every socket. At the -//! current default of `UDP_BUFFER_SIZE = 1500`, that is ~12 KiB -//! always-live / ~24 KiB peak per client. -//! -//! On `std + tokio`, all of this is allocated on the heap when each future -//! is spawned, so the overhead is invisible to callers. On the bare-metal -//! port (future), whoever drives the futures must arrange storage for them -//! (either a `static` or a heap allocator); the capacity constants plus -//! [`crate::UDP_BUFFER_SIZE`] are the knobs for trimming this footprint. +//! See `docs/simple_someip/plans/2026-06-09-phase22-125-memory-reduction-design.md`. mod bind_dispatch; mod error; mod inner; diff --git a/tests/bare_metal_e2e.rs b/tests/bare_metal_e2e.rs index 5b2c62c3..9b0976ca 100644 --- a/tests/bare_metal_e2e.rs +++ b/tests/bare_metal_e2e.rs @@ -601,8 +601,8 @@ async fn client_send_request_server_runloop_stable() { /// the TC4 build. Same semantics/update procedure as the constants in /// src/client/mod.rs; authoritative numbers come from /// `tools/capture_type_sizes.sh` (thumbv7em). -const BM_CLIENT_RUN_FUTURE_BUDGET: usize = 34048; // = ceil64(27208 × 1.25) -const BM_CLIENT_SOCKET_LOOP_BUDGET: usize = 2816; // = ceil64(2224 × 1.25) +const BM_CLIENT_RUN_FUTURE_BUDGET: usize = 34048; // = ceil64(27224 × 1.25) +const BM_CLIENT_SOCKET_LOOP_BUDGET: usize = 1024; // = ceil64(776 × 1.25); receive buffer moved to BufferProvider pool (Tasks 3+4) const BM_SERVER_RUN_FUTURE_BUDGET: usize = 9664; // = ceil64(7696 × 1.25) #[tokio::test] From c997ac01c8f719a850ccf08b9b49fad69aaed8a9 Mon Sep 17 00:00:00 2001 From: Justin Kovacich Date: Wed, 17 Jun 2026 12:12:47 -0400 Subject: [PATCH 09/12] docs: repair pre-existing unresolved intra-doc links + transport doctest (#125) Feature-subset cargo doc and cargo test --doc were red on the phase-21 base (ServerDeps/ClientDeps::tokio, server EventPublisher/announcement_loop dead links, an illegal with_announce(false) link, and a transport adapter-sketch doctest referencing the absent umbrella crate). Doc-comment-only: cross-feature links demoted to code spans; the doctest reshaped to compile against the real GAT traits. Folded into PR2 so the stack is green end-to-end. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/buffer_pool.rs | 2 +- src/client/mod.rs | 14 ++++++-------- src/server/mod.rs | 32 ++++++++++++++++---------------- src/static_channels/mod.rs | 4 ++-- src/transport.rs | 26 +++++++++++++++++--------- 5 files changed, 42 insertions(+), 36 deletions(-) diff --git a/src/buffer_pool.rs b/src/buffer_pool.rs index 7f1fd231..a0cca72e 100644 --- a/src/buffer_pool.rs +++ b/src/buffer_pool.rs @@ -1,7 +1,7 @@ //! Fixed-capacity pool of `&'static mut [u8]` buffers with claim/release //! semantics, mirroring the channel pools in this module. A `BufferPool` //! is declared as a `static` by the consumer; each `claim()` hands out one -//! slot as a [`BufferLease`] that returns the slot to the pool on drop. +//! slot as a `BufferLease` that returns the slot to the pool on drop. //! //! Synchronization uses per-slot `AtomicBool` compare-exchange so the same //! code is valid on the bare-metal target and on std without requiring a diff --git a/src/client/mod.rs b/src/client/mod.rs index 5fea706b..67064be3 100644 --- a/src/client/mod.rs +++ b/src/client/mod.rs @@ -60,7 +60,7 @@ use std::sync::{Arc, Mutex, RwLock}; /// # Required entries /// /// For a payload type `P: PayloadWireFormat + 'static`, the -/// [`define_static_channels!`] invocation must declare: +/// `define_static_channels!` invocation must declare: /// /// | Pool kind | Item type | Cardinality | /// |---|---|---| @@ -73,7 +73,7 @@ use std::sync::{Arc, Mutex, RwLock}; /// | `unbounded` | `ClientUpdate

` | per-pool default | /// /// where `C` is the channel-factory type generated by -/// [`define_static_channels!`]. `bare_metal` consumers will typically +/// `define_static_channels!`. `bare_metal` consumers will typically /// look at the `examples/bare_metal_client/` example for a copy-pasteable /// invocation matching this list. /// @@ -98,8 +98,6 @@ use std::sync::{Arc, Mutex, RwLock}; /// elaboration for these bounds, the per-impl repetition can /// collapse to a single `C: ClientChannelTypes

` supertrait without /// changing the outward contract. -/// -/// [`define_static_channels!`]: crate::define_static_channels pub trait ClientChannelTypes: ChannelFactory where Result<(), Error>: OneshotPooled, @@ -254,7 +252,7 @@ impl /// see one named field per dependency rather than positional args six /// deep). /// -/// Generic order mirrors [`crate::server::ServerDeps`] for the shared +/// Generic order mirrors `ServerDeps` for the shared /// infrastructure (`F`, `Tm`, `R`), then side-specific dependencies /// (`I` for the client's interface handle, `Sub` for the server's /// subscription handle), then any side-only extras (`Sp` for the @@ -342,7 +340,7 @@ impl /// Field-by-field fluent builder. Each `with_*` returns a new /// `ClientDeps` with that single field replaced (and its corresponding /// generic parameter updated). Lets callers start from -/// [`ClientDeps::tokio`] and override individual fields without +/// `ClientDeps::tokio` and override individual fields without /// spelling out the full struct literal. /// /// ```no_run @@ -499,9 +497,9 @@ where /// standard constructors `Self::new` / `Self::new_with_loopback` / /// `Self::new_with_spawner_and_loopback` (all under `client-tokio`). /// -/// # Note on generic-parameter alignment with [`crate::ServerDeps`] +/// # Note on generic-parameter alignment with `ServerDeps` /// -/// [`ClientDeps`] and [`crate::ServerDeps`] share their first three +/// [`ClientDeps`] and `ServerDeps` share their first three /// generic positions (`F`, `Tm`, `R`) to read symmetrically, but the /// `Client` struct itself carries only `` /// — `F`, `Tm`, and `Sp` (Spawner) live on the run-loop future diff --git a/src/server/mod.rs b/src/server/mod.rs index 5e781b63..6884f9bf 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -258,7 +258,7 @@ impl ServerConfig { } } -/// Bundle of pluggable infrastructure passed to [`Server::new_with_deps`]. +/// Bundle of pluggable infrastructure passed to `Server::new_with_deps`. /// Mirrors `crate::ClientDeps` (under `client`) but with the server's /// smaller surface /// — no `Spawner` (server has no internal task spawning), no @@ -344,7 +344,7 @@ impl /// Field-by-field fluent builder. Each `with_*` returns a new /// `ServerDeps` with that single field replaced (and its corresponding /// generic parameter updated). Lets callers start from -/// [`ServerDeps::tokio`] and override individual fields without +/// `ServerDeps::tokio` and override individual fields without /// spelling out the full struct literal. impl ServerDeps where @@ -425,10 +425,10 @@ where /// the other constructor variants) alongside the [`Server`] handle and /// the combined run-future. /// -/// Mirrors `crate::ClientUpdates`'s role on the [`Client`](crate::Client) +/// Mirrors `crate::ClientUpdates`'s role on the `Client` /// side: a place to hang things the caller will reach for once /// construction completes (today: just the -/// [`EventPublisher`](crate::server::EventPublisher) handle; future +/// [`EventPublisher`] handle; future /// fields are reserved for forward-compat). Existing /// `Server::publisher()` accessor is unchanged — the field on this /// struct is the more discoverable path now that `Server::new` returns @@ -538,7 +538,7 @@ where /// /// The generic order mirrors [`ServerDeps`] (and, for the shared /// infrastructure parameters `F`, `Tm`, `R`, the order is also shared -/// with [`crate::ClientDeps`]). +/// with `crate::ClientDeps`). /// /// The convenience constructors `Self::new` / `Self::new_with_loopback` /// / `Self::new_passive` (under the `server-tokio` feature) instantiate @@ -611,7 +611,7 @@ pub struct Server< /// On `server-tokio` builds this is a zero-sized `TokioTransport`. #[allow(dead_code)] factory: F, - /// Async sleep primitive used by [`Self::announcement_loop`]'s + /// Async sleep primitive used by `announcement_loop`'s /// 1-second tick. On `server-tokio` builds this is `TokioTimer` /// (wrapping `tokio::time::sleep`). timer: Tm, @@ -653,7 +653,7 @@ pub struct Server< /// the callback as a `(NonSdRequestCallback, usize)` pair and passed /// back verbatim on every invocation. It is deliberately `usize` /// rather than `*mut c_void`: a stored raw pointer would make -/// [`Server`] `!Send` and break [`Server::run`]'s declared `+ Send` +/// [`Server`] `!Send` and break `Server::run`'s declared `+ Send` /// bound, while `usize` is trivially `Send + Sync` and matches the /// `uintptr_t` an FFI caller holds anyway. No `unsafe` enters this /// crate — the cast back to a pointer (and its safety justification) @@ -804,7 +804,7 @@ impl /// incoming `SubscribeEventGroup` / `FindService` messages and routes /// them to the right `EventPublisher` via /// [`EventPublisher::register_subscriber`]). Do **not** call - /// [`Server::announcement_loop`] or spawn [`Server::run`] on a passive + /// `announcement_loop` or spawn [`Server::run`] on a passive /// server — the external dispatcher owns those responsibilities. /// /// # Errors @@ -950,7 +950,7 @@ where /// Passive servers bind a unicast socket as usual but bind their SD /// socket to an ephemeral port (port 0) instead of the SOME/IP SD /// port — see `Server::new_passive` under `server-tokio` for the - /// full explanation. Calling [`Self::announcement_loop`] or + /// full explanation. Calling `announcement_loop` or /// [`Self::run`] on the result is a programming error. /// /// # Errors @@ -1043,7 +1043,7 @@ where { /// Construct a `Server` from pre-built dependencies + storage /// handles. The bare-metal-no-alloc counterpart to - /// [`Self::new_with_deps`]. + /// `Self::new_with_deps`. /// /// Unlike `new_with_deps`, this constructor does NOT call /// `factory.bind(...)` and does NOT join any multicast group. @@ -1112,8 +1112,8 @@ where /// Passive-server counterpart to [`Self::new_with_handles`]. /// /// Same shape; the resulting server is marked - /// `is_passive = true` so [`Self::announcement_loop`] / - /// [`Self::announcement_loop_local`] / [`Self::run`] / + /// `is_passive = true` so `announcement_loop` / + /// `announcement_loop_local` / `Self::run` / /// [`Self::run_with_buffers`] return /// `Err(Error::InvalidUsage(...))` rather than driving the SD /// loop. The caller is expected to handle SD externally @@ -1230,7 +1230,7 @@ where /// 1-Hz `OfferService` announcement loop. The two are combined /// into a single future so callers cannot forget to spawn the /// announcement side; passing - /// [`ServerConfig::with_announce(false)`] suppresses the + /// [`ServerConfig::with_announce`]`(false)` suppresses the /// announcement arm for dispatcher topologies where a co-located /// `Client` drives SD on the server's behalf. /// @@ -1239,7 +1239,7 @@ where /// (~1500 bytes) and ideally up to the IP datagram limit /// (64 KiB - 1). On bare-metal targets, callers typically place /// these in `static` storage; on std (or any alloc-using - /// target), [`Self::run`] is the convenience shim that + /// target), `Self::run` is the convenience shim that /// heap-allocates 64 KiB buffers and delegates here. /// /// The returned future is independent of `&self` — the cheap @@ -1324,7 +1324,7 @@ where /// phase 21 removed — deliberately. An announce-only future never /// touches the receive path, so the invariant that motivated the /// phase-21 combined run-future (no two futures racing the same - /// sockets and SD session counter) is preserved: the [`Self::run`] + /// sockets and SD session counter) is preserved: the `Self::run` /// path is still guarded by the first-poll `started` latch, and /// supplementary announce loops only ever *send* on the shared SD /// socket. @@ -3522,7 +3522,7 @@ mod tests { }); } - /// Smoke test for [`Server::announcement_loop`]: a loopback server + /// Smoke test for `announcement_loop`: a loopback server /// with `multicast_loop` enabled should emit at least one /// `OfferService` on the SD multicast group within a couple of /// seconds. diff --git a/src/static_channels/mod.rs b/src/static_channels/mod.rs index b778acf1..e5938ca0 100644 --- a/src/static_channels/mod.rs +++ b/src/static_channels/mod.rs @@ -892,9 +892,9 @@ pub const UNBOUNDED_DEFAULT_CAP: usize = 128; /// /// # Required entries for `Client` /// -/// To use the generated factory with [`crate::Client`], the macro +/// To use the generated factory with `crate::Client`, the macro /// invocation must declare the seven channel types enumerated by -/// [`crate::client::ClientChannelTypes`]. See its rustdoc for the +/// `crate::client::ClientChannelTypes`. See its rustdoc for the /// exhaustive list and a worked example. #[macro_export] macro_rules! define_static_channels { diff --git a/src/transport.rs b/src/transport.rs index 1f137d79..cd2d1c61 100644 --- a/src/transport.rs +++ b/src/transport.rs @@ -103,13 +103,17 @@ //! # fn wrapper() { //! use core::future::Future; //! use core::net::{Ipv4Addr, SocketAddrV4}; +//! use core::pin::Pin; //! use core::time::Duration; -//! use futures::future::BoxFuture; //! use simple_someip::transport::{ //! IoErrorKind, ReceivedDatagram, SocketOptions, Timer, TransportError, //! TransportFactory, TransportSocket, //! }; //! +//! // A boxed future alias keeps this sketch short without pulling in the +//! // `futures` crate (the engine itself depends only on `futures-util`). +//! type BoxFuture<'a, T> = Pin + Send + 'a>>; +//! //! struct TokioTransport; //! //! struct TokioSocket { @@ -118,17 +122,18 @@ //! //! impl TransportFactory for TokioTransport { //! type Socket = TokioSocket; -//! fn bind( -//! &self, +//! type BindFuture<'a> = BoxFuture<'a, Result>; +//! fn bind<'a>( +//! &'a self, //! addr: SocketAddrV4, -//! _options: &SocketOptions, -//! ) -> impl Future> + Send { -//! async move { +//! _options: &'a SocketOptions, +//! ) -> Self::BindFuture<'a> { +//! Box::pin(async move { //! let inner = tokio::net::UdpSocket::bind(addr) //! .await //! .map_err(|_| TransportError::Io(IoErrorKind::Other))?; //! Ok(TokioSocket { inner }) -//! } +//! }) //! } //! } //! @@ -203,8 +208,11 @@ //! //! struct TokioTimer; //! impl Timer for TokioTimer { -//! fn sleep(&self, duration: Duration) -> impl Future + Send { -//! tokio::time::sleep(duration) +//! // `tokio::time::Sleep` is `!Send`; box it behind a non-`Send` +//! // future so this sketch stays backend-agnostic. +//! type SleepFuture<'a> = Pin + 'a>>; +//! fn sleep(&self, duration: Duration) -> Self::SleepFuture<'_> { +//! Box::pin(tokio::time::sleep(duration)) //! } //! } //! # } From 7367b2b8fe9a5813282ea925a24d8f7e6b21f728 Mon Sep 17 00:00:00 2001 From: Justin Kovacich Date: Wed, 17 Jun 2026 12:29:45 -0400 Subject: [PATCH 10/12] fix(client): E2E-protect send must bound-check against buf.len(), not UDP_BUFFER_SIZE (#125) Final-review Issue #1: on a caller-sized buffer smaller than UDP_BUFFER_SIZE, an E2E-protected frame that expands past buf.len() but stays under UDP_BUFFER_SIZE passed the constant guard and then indexed buf out of bounds, panicking the socket loop (the dft E2E path). Guard now uses buf.len(); regression test reproduces the OOB (RED) and confirms a typed Capacity error (GREEN). Also documents the coarse send() pre-filter. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/client/socket_manager.rs | 11 ++-- tests/bare_metal_e2e.rs | 99 ++++++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 3 deletions(-) diff --git a/src/client/socket_manager.rs b/src/client/socket_manager.rs index 4236c7d9..d408eb2d 100644 --- a/src/client/socket_manager.rs +++ b/src/client/socket_manager.rs @@ -466,6 +466,11 @@ where // message. Without this, an oversize encode would surface as a // protocol-level I/O error from inside the socket loop. let required = message.required_size(); + // Coarse fail-fast: `send()` has no leased buffer in scope, so + // UDP_BUFFER_SIZE is the only bound available here. The socket + // loop's `buf.len()` check is the authoritative guard; E2E + // protection can still expand a frame that passes this pre-filter + // beyond the leased buffer, and that case is caught there. if required > UDP_BUFFER_SIZE { warn!( "outgoing message size {required} exceeds UDP_BUFFER_SIZE ({UDP_BUFFER_SIZE}); rejecting with Capacity(\"udp_buffer\")" @@ -675,11 +680,11 @@ where ); match result { Some(Ok(protected_len)) => { - if 16 + protected_len > UDP_BUFFER_SIZE { + if 16 + protected_len > buf.len() { error!( - "E2E-protected payload ({} bytes) exceeds UDP_BUFFER_SIZE ({}); dropping send", + "E2E-protected payload ({} bytes) exceeds claimed buffer ({}); rejecting send", 16 + protected_len, - UDP_BUFFER_SIZE + buf.len() ); let _ = send_message .response diff --git a/tests/bare_metal_e2e.rs b/tests/bare_metal_e2e.rs index 9b0976ca..3efcfaab 100644 --- a/tests/bare_metal_e2e.rs +++ b/tests/bare_metal_e2e.rs @@ -966,6 +966,105 @@ async fn binding_sockets_claims_one_buffer_each_until_pool_exhausted() { run_handle.abort(); } +/// Task 5 (regression): E2E-protected send whose expanded payload exceeds the +/// leased buffer (but not `UDP_BUFFER_SIZE`) must return +/// `Err(Error::Capacity("udp_buffer"))`, not panic. +/// +/// # Why the window is deterministic +/// +/// Profile 4 protect prepends a 12-byte E2E header. With a 20-byte payload: +/// - Unprotected SOME/IP frame: 16 (header) + 20 (payload) = 36 bytes. +/// - Post-protect SOME/IP frame: 16 (header) + (12 + 20) (P4 output) = 48 bytes. +/// - Buffer slot: 40 bytes. +/// +/// Pre-guard (unprotected) : 36 ≤ 40 → passes. +/// Post-protect guard (before fix): 48 > UDP_BUFFER_SIZE (1400) → false → no guard fires. +/// `copy_from_slice` into buf[16..48] on a 40-byte buf → out-of-bounds panic (RED). +/// +/// Post-protect guard (after fix): 48 > 40 → true → `Capacity` error returned (GREEN). +#[tokio::test] +async fn e2e_protect_expanding_payload_beyond_leased_buffer_returns_capacity_error() { + // 40-byte slots: big enough for the 36-byte unprotected frame but not + // for the 48-byte post-P4-protect frame. + const BUF_LEN: usize = 40; + static POOL: BufferPool<4, BUF_LEN> = BufferPool::new(); + + let network = SharedNetwork::new(); + let client_factory = MockFactory { + tx_pipe: Arc::clone(&network.client_to_server), + rx_pipe: Arc::clone(&network.server_to_client), + next_port: Arc::new(Mutex::new(200)), + }; + + let client_e2e: Arc> = Arc::new(Mutex::new(E2ERegistry::new())); + let client_iface: Arc> = Arc::new(RwLock::new(Ipv4Addr::LOCALHOST)); + let deps = ClientDeps { + factory: client_factory, + spawner: TokioBackedSpawner, + timer: MockTimer, + e2e_registry: client_e2e, + interface: client_iface, + buffer_provider: StaticBufferProvider(&POOL), + }; + let (client, _updates, run_fut) = Client::< + RawPayload, + Arc>, + Arc>, + E2ETestChannels, + >::new_with_deps(deps, false); + let run_handle = tokio::spawn(run_fut); + + // Register E2E Profile 4 for service 0xABCD, method 0x0001. + // Profile 4 adds 12 bytes of header to every protected payload. + let service_id: u16 = 0xABCD; + let method_id: u16 = 0x0001; + let e2e_key = simple_someip::E2EKey::new(service_id, method_id); + client + .register_e2e( + e2e_key, + simple_someip::E2EProfile::Profile4(simple_someip::e2e::Profile4Config::new( + 0xDEAD_BEEF, + 15, + )), + ) + .expect("register E2E key"); + + let server_addr = SocketAddrV4::new(Ipv4Addr::LOCALHOST, 30800); + client + .add_endpoint(service_id, 1, server_addr, 42000) + .await + .expect("add_endpoint"); + + // 20-byte payload: unprotected frame = 36 B ≤ 40 B (fits buf), but + // post-protect frame = 48 B > 40 B (exceeds buf). + let payload_bytes = [0x55u8; 20]; + let msg_id = MessageId::new_from_service_and_method(service_id, method_id); + let payload = RawPayload::from_payload_bytes(msg_id, &payload_bytes).expect("create payload"); + let request = Message::::new( + Header::new( + msg_id, + 0x0001_0001, + 1, + 1, + MessageTypeField::new(MessageType::Request, false), + ReturnCode::Ok, + payload_bytes.len(), + ), + payload, + ); + + let result = client.send_to_service(service_id, 1, request).await; + + // Must return typed Capacity error — not panic. + assert!( + matches!(result, Err(ClientError::Capacity("udp_buffer"))), + "expected Err(Capacity(\"udp_buffer\")), got {result:?}" + ); + + client.shut_down(); + run_handle.abort(); +} + /// An empty `VecSdHeader` for building a minimal valid SD message. fn empty_vec_sd_header() -> simple_someip::VecSdHeader { use simple_someip::protocol::sd::{Flags, RebootFlag}; From 8e6e2459d5a188d94fa6956666225b94b54f0950 Mon Sep 17 00:00:00 2001 From: Justin Kovacich Date: Wed, 17 Jun 2026 13:52:01 -0400 Subject: [PATCH 11/12] fix(client): replace per-client Box::leak with Arc-backed buffer pool; capacity headroom + min-size floor (#125) Adversarial-review fixes (pool/unsafe cluster): - BufferLease redesigned to raw NonNull ptrs + an alloc-gated Arc keepalive (_owner). Static (bare-metal) path: _owner=None, no allocation. Tokio path: TokioBufferProvider now holds Arc> and claims via claim_arc(), so the pool is reference-counted, not leaked per client. - Pool sized 9->10 (UNICAST_SOCKETS_CAP + discovery + 1 release-lag slack) to avoid spurious Capacity errors when an evicted socket's lease frees async. - const assert LEN >= 16 (SOME/IP header floor); footprint doc states the practical floor and the +1 bare-metal sizing guidance. - Added a concurrent-claim test proving no double-claim / no aliasing under contention. Per-slot AtomicBool exclusivity invariant unchanged. nm-verified: client,bare_metal stays alloc-free (Arc path cfg'd out). Co-Authored-By: Claude Opus 4.8 (1M context) --- src/buffer_pool.rs | 166 ++++++++++++++++++++++++++++++++++------- src/client/mod.rs | 25 ++++++- src/tokio_transport.rs | 26 ++++--- tests/buffer_pool.rs | 98 ++++++++++++++++++++++-- 4 files changed, 267 insertions(+), 48 deletions(-) diff --git a/src/buffer_pool.rs b/src/buffer_pool.rs index a0cca72e..962c4a51 100644 --- a/src/buffer_pool.rs +++ b/src/buffer_pool.rs @@ -9,6 +9,7 @@ use core::cell::UnsafeCell; use core::ops::{Deref, DerefMut}; +use core::ptr::NonNull; use core::sync::atomic::{AtomicBool, Ordering}; /// Fixed-capacity pool of `LEN`-byte buffers. Declare as a `static` and call @@ -19,6 +20,15 @@ use core::sync::atomic::{AtomicBool, Ordering}; /// `BufferPool::new()` is `const fn`, so pools can be declared as `static` /// items initialized at link time with no runtime cost. /// +/// # Minimum slot length +/// +/// `LEN` must be at least 16 bytes — the size of a SOME/IP header — or the +/// client silently drops all inbound and rejects all sends. A compile-time +/// `const` assertion in [`Self::new`] enforces this floor. 16 is only the +/// absolute header minimum: in practice a slot must hold the largest expected +/// message (header + payload), realistically one full UDP datagram (see +/// [`crate::UDP_BUFFER_SIZE`]). +/// /// # Synchronization /// /// Each slot has an independent `AtomicBool` claimed flag. `claim()` scans @@ -43,8 +53,18 @@ unsafe impl Sync for BufferPool BufferPool { /// Create a new, empty pool. All slots are free. + /// + /// # Panics (compile-time) + /// + /// A `const` assertion rejects `LEN < 16` at compile time: a slot must be + /// large enough to hold a 16-byte SOME/IP header, otherwise the client + /// silently drops all inbound and rejects all sends. #[must_use] pub const fn new() -> Self { + // Compile-time floor: a slot must hold at least a SOME/IP header. + // Placed in `const {}` so it is evaluated during const-eval of every + // monomorphization that constructs a pool (e.g. the `static` init). + const { assert!(LEN >= 16, "BufferPool slot must hold at least a 16-byte SOME/IP header") }; Self { store: UnsafeCell::new([[0u8; LEN]; SLOTS]), claimed: [const { AtomicBool::new(false) }; SLOTS], @@ -57,6 +77,28 @@ impl BufferPool { /// The returned buffer is zeroed before hand-out so a reused slot never /// leaks the previous tenant's bytes. pub fn claim(&'static self) -> Option { + let (buf, flag) = self.try_claim_slot()?; + Some(BufferLease { + buf, + len: LEN, + flag, + // Truly-'static pool (bare-metal static-pool path): nothing to + // keep alive — the backing store outlives the lease via `'static`. + // No allocation on this path. The `_owner` field only exists when + // `alloc` is available; under `bare_metal` it is cfg'd out. + #[cfg(feature = "_alloc")] + _owner: None, + }) + } + + /// Scan for a free slot and atomically claim it. On success returns the + /// `NonNull` start-of-slot pointer and a `NonNull` to that slot's claimed + /// flag; on exhaustion returns `None`. + /// + /// Both pointers reference memory owned by `self`; the caller is + /// responsible for keeping `self` alive for as long as the pointers are + /// used (via `'static` or an `Arc` clone held in the `BufferLease`). + fn try_claim_slot(&self) -> Option<(NonNull, NonNull)> { for (idx, flag) in self.claimed.iter().enumerate() { // Attempt to atomically claim this slot. if flag @@ -64,28 +106,57 @@ impl BufferPool { .is_ok() { // SAFETY: we just won the compare_exchange on `claimed[idx]`, - // so no other `claim()` call holds a reference to slot `idx`. - // We derive the slot pointer by raw-pointer arithmetic to avoid - // forming a `&mut` to the whole array (which would alias already- - // claimed slots). The resulting `&'static mut [u8]` is valid for - // the lifetime of the `BufferPool` `static`. - let slot_ptr = - unsafe { self.store.get().cast::<[u8; LEN]>().add(idx) }; - // `'static` is sound because `self: &'static BufferPool`, so the - // backing store outlives the lease; the annotation, not a - // transmute, carries the lifetime. - let slot: &'static mut [u8] = unsafe { (*slot_ptr).as_mut_slice() }; - slot.fill(0); - return Some(BufferLease { - buf: slot, - claimed_flag: &self.claimed[idx], - }); + // so no other live lease references slot `idx`. We derive the + // slot pointer by raw-pointer arithmetic to avoid forming a + // `&mut` to the whole array (which would alias already-claimed + // slots). + let slot_ptr = unsafe { self.store.get().cast::().add(idx * LEN) }; + // Zero the freshly-claimed slot so a reused slot never leaks + // the previous tenant's bytes. + // + // SAFETY: `slot_ptr` is the start of slot `idx`, in bounds for + // `LEN` bytes, and we hold the exclusive claim on it; no other + // reference aliases these bytes. + unsafe { core::ptr::write_bytes(slot_ptr, 0, LEN) }; + // SAFETY: `slot_ptr` derives from `self.store.get()` (non-null) + // plus an in-bounds offset; `flag` is an element of the + // `claimed` array (non-null). + let buf = unsafe { NonNull::new_unchecked(slot_ptr) }; + let flag = NonNull::from(flag); + return Some((buf, flag)); } } None } } +#[cfg(feature = "_alloc")] +impl BufferPool { + /// Claim a free slot from an `Arc`-backed pool, returning a [`BufferLease`] + /// that holds an `Arc` clone to keep the pool alive for the lease's + /// lifetime, or `None` if all `SLOTS` are in use. + /// + /// This is the heap-backed counterpart to [`Self::claim`]: the static-pool + /// path uses `&'static self` and stores `_owner: None`; this path stores + /// `_owner: Some(arc.clone())` so the pool's backing store (and the slot's + /// claimed flag) stay valid until the last lease and provider drop. Only + /// compiled where `alloc` is available (the `_alloc` feature), so the + /// bare-metal `client,bare_metal` build stays allocation-free. + pub fn claim_arc(self: &alloc::sync::Arc) -> Option { + let (buf, flag) = self.try_claim_slot()?; + Some(BufferLease { + buf, + len: LEN, + flag, + // Keep the Arc'd pool alive for the lease's lifetime. The pool is + // `Send + Sync` (see the `unsafe impl Sync` above and the `Send` + // bounds on its contents), so `Arc` coerces to + // `Arc`. + _owner: Some(self.clone()), + }) + } +} + impl Default for BufferPool { fn default() -> Self { Self::new() @@ -105,34 +176,75 @@ impl core::fmt::Debug for BufferPool, + /// Length of the slot, in bytes. + len: usize, + /// This slot's claimed flag in the owning pool. + flag: NonNull, + /// Keeps an `Arc`-backed pool alive for the lease's lifetime; `None` for a + /// truly-`'static` (bare-metal) pool. Drop order: the flag is cleared + /// first (the raw pointer is still valid via `_owner` for the Arc case, or + /// `'static` for the static case), then `_owner` drops, releasing the + /// pool's last reference if this was the final holder. + #[cfg(feature = "_alloc")] + _owner: Option>, } -// SAFETY: `BufferLease` owns exclusive access to its slot (enforced by the -// pool's per-slot `AtomicBool`). Both `&'static AtomicBool` and -// `&'static mut [u8]` are Send. +// SAFETY: `BufferLease` is `Send` because: +// - The lease owns exclusive access to its slot, enforced by the pool's +// per-slot `AtomicBool` (won via compare_exchange in `try_claim_slot`); no +// other live lease can reference the same slot bytes. +// - The raw `NonNull` / `NonNull` pointers are themselves +// `Send` only by this `unsafe impl`; they reference memory kept alive +// either by `'static` (static path) or by the `Arc` in `_owner`, which is +// `Arc` and hence `Send`. Sending the lease to +// another thread moves all of these together, so the slot's memory and +// flag remain valid and exclusively owned. unsafe impl Send for BufferLease {} impl Deref for BufferLease { type Target = [u8]; fn deref(&self) -> &[u8] { - self.buf + // SAFETY: `buf` points at the start of an exclusively-claimed slot of + // `len` bytes, kept alive by `_owner`/`'static`. We hold `&self`, so + // an immutable slice is sound (no concurrent `&mut` exists — the + // claimed flag guarantees a single live lease per slot). + unsafe { core::slice::from_raw_parts(self.buf.as_ptr(), self.len) } } } impl DerefMut for BufferLease { fn deref_mut(&mut self) -> &mut [u8] { - self.buf + // SAFETY: as in `deref`, plus we hold `&mut self`, so a mutable slice + // is the unique reference to these bytes. + unsafe { core::slice::from_raw_parts_mut(self.buf.as_ptr(), self.len) } } } impl Drop for BufferLease { fn drop(&mut self) { - // Release the slot atomically. Any subsequent `claim()` that acquires - // this flag will see the updated store state. - self.claimed_flag.store(false, Ordering::Release); + // Release the slot atomically. The flag memory is still valid here: + // for the static path it is `'static`; for the Arc path `_owner` (which + // drops *after* this block) still holds a live reference to the pool. + // Any subsequent claim that acquires this flag will see the updated + // store state. + // + // SAFETY: `flag` references this slot's `AtomicBool` inside the pool, + // valid for the reasons above. + unsafe { self.flag.as_ref() }.store(false, Ordering::Release); + // `_owner` (if any) drops after this, releasing the pool's last + // reference when this is the final holder. } } diff --git a/src/client/mod.rs b/src/client/mod.rs index 67064be3..f3a2b5e3 100644 --- a/src/client/mod.rs +++ b/src/client/mod.rs @@ -11,8 +11,29 @@ //! the per-slot length (e.g. 2 × 512 B), so the buffer budget lives in //! `.bss` and is sized by the caller rather than fixed at //! `UNICAST_SOCKETS_CAP × UDP_BUFFER_SIZE`. On `std + tokio` the provider -//! is heap-backed and provisioned internally (`UDP_BUFFER_SIZE`-sized slots), -//! invisible to callers. +//! is heap-backed (a single reference-counted `BufferPool`, freed when the +//! last lease and provider drop — not leaked) and provisioned internally +//! (`UDP_BUFFER_SIZE`-sized slots), invisible to callers. +//! +//! ## Sizing the pool +//! +//! Bare-metal callers should size their pool at **`(max concurrent sockets) +//! + 1`** slots, not exactly the socket count. The unicast-eviction path +//! frees a buffer lease asynchronously (when the spawned loop future drops), +//! lagging the synchronous registry removal, so an evict-then-immediate-rebind +//! can transiently need one extra slot; without the `+ 1` slack that surfaces +//! as a spurious `Capacity("udp_buffer")`. The tokio provider already bakes +//! this in (it sizes its pool at 10 = `UNICAST_SOCKETS_CAP (8) + 1 discovery +//! + 1 release-lag`). +//! +//! ## Minimum slot length +//! +//! A pool slot must be at least 16 bytes — the SOME/IP header size — or the +//! client silently drops all inbound and rejects all sends; `BufferPool::new` +//! enforces this floor with a compile-time `const` assertion. 16 is only the +//! absolute header minimum: the practical floor is the largest expected +//! message (header + payload), realistically one full UDP datagram +//! ([`UDP_BUFFER_SIZE`](crate::UDP_BUFFER_SIZE)). //! //! See `docs/simple_someip/plans/2026-06-09-phase22-125-memory-reduction-design.md`. mod bind_dispatch; diff --git a/src/tokio_transport.rs b/src/tokio_transport.rs index 915ffa0d..778752cd 100644 --- a/src/tokio_transport.rs +++ b/src/tokio_transport.rs @@ -551,24 +551,28 @@ impl crate::transport::UnboundedPooled for T { // ── TokioBufferProvider ─────────────────────────────────────────────────── -// `Box` is not in scope by default: the crate is `#![no_std]`, so std-gated -// modules still import it explicitly (it is NOT a redundant import). -use std::boxed::Box; +use std::sync::Arc; use crate::buffer_pool::{BufferLease, BufferPool}; use crate::transport::BufferProvider; -/// Tokio-path buffer provider: a single leaked `BufferPool` sized at -/// `UNICAST_SOCKETS_CAP + 1` × `UDP_BUFFER_SIZE` (one per possible socket -/// plus discovery). Leaking is fine — a client process holds one for its -/// lifetime; the API hides it entirely from callers. -#[derive(Clone, Copy, Debug)] -pub struct TokioBufferProvider(&'static BufferPool<9, { crate::UDP_BUFFER_SIZE }>); +/// Tokio-path buffer provider: a single `Arc`-backed `BufferPool` sized at +/// 10 × `UDP_BUFFER_SIZE`. That is `UNICAST_SOCKETS_CAP (8) + 1 discovery + 1` +/// release-lag slot: the unicast-eviction path frees a buffer lease +/// asynchronously (when the spawned loop future drops), lagging the +/// synchronous registry removal, so an evict-then-immediate-rebind can +/// transiently need one extra slot. The pool is reference-counted, not leaked +/// — it is freed when the last [`BufferLease`] and the last provider clone +/// drop. Cloning a provider shares the same `Arc` (one provider per `Client`). +#[derive(Clone, Debug)] +pub struct TokioBufferProvider(Arc>); impl TokioBufferProvider { #[must_use] pub fn new() -> Self { - Self(Box::leak(Box::new(BufferPool::new()))) + // One heap allocation for the pool; no leak. Frees when the last + // lease + provider drop. + Self(Arc::new(BufferPool::new())) } } @@ -580,7 +584,7 @@ impl Default for TokioBufferProvider { impl BufferProvider for TokioBufferProvider { fn claim(&self) -> Option { - self.0.claim() + self.0.claim_arc() } } diff --git a/tests/buffer_pool.rs b/tests/buffer_pool.rs index dba27940..5fa46c43 100644 --- a/tests/buffer_pool.rs +++ b/tests/buffer_pool.rs @@ -3,28 +3,38 @@ use simple_someip::transport::{BufferProvider, StaticBufferProvider}; // One pool per test: a shared `static` would let libtest's parallel threads // race (one test claiming both slots makes the other's claim spuriously fail). -static POOL_EXHAUST: BufferPool<2, 4> = BufferPool::new(); -static POOL_RETURN: BufferPool<2, 4> = BufferPool::new(); +// +// Slot length is 16 — the SOME/IP-header floor enforced by +// `BufferPool::new`'s compile-time `const` assertion. A smaller `LEN` would +// fail to compile. +static POOL_EXHAUST: BufferPool<2, 16> = BufferPool::new(); +static POOL_RETURN: BufferPool<2, 16> = BufferPool::new(); #[test] fn claim_returns_distinct_zeroed_slices_until_exhausted() { let mut a = POOL_EXHAUST.claim().expect("slot 0"); let b = POOL_EXHAUST.claim().expect("slot 1"); - assert_eq!(a.len(), 4); - assert_eq!(&*b, &[0u8; 4]); // freshly handed-out slot is zeroed - a[0] = 0xAB; // writable + assert_eq!(a.len(), 16); + assert_eq!(&*b, &[0u8; 16]); // freshly handed-out slot is zeroed + a[0] = 0xAB; // writable assert_eq!(a[0], 0xAB); - assert!(POOL_EXHAUST.claim().is_none(), "pool of 2 must refuse a 3rd claim"); + assert!( + POOL_EXHAUST.claim().is_none(), + "pool of 2 must refuse a 3rd claim" + ); } #[test] fn dropping_a_lease_returns_its_slot() { let a = POOL_RETURN.claim().expect("slot"); drop(a); - assert!(POOL_RETURN.claim().is_some(), "slot must be reusable after the lease drops"); + assert!( + POOL_RETURN.claim().is_some(), + "slot must be reusable after the lease drops" + ); } -static PROV_POOL: BufferPool<2, 8> = BufferPool::new(); +static PROV_POOL: BufferPool<2, 16> = BufferPool::new(); #[test] fn static_provider_claims_through_a_shared_pool() { @@ -33,3 +43,75 @@ fn static_provider_claims_through_a_shared_pool() { let _b = prov.claim().expect("second"); assert!(prov.claim().is_none(), "provider exposes the pool's capacity"); } + +/// Concurrent-claim regression (mirrors the channel pools' +/// `*_concurrent_first_claim_does_not_panic`): N threads each call `claim()` +/// on a shared `static` pool of N slots. Asserts (a) all N succeed, (b) each +/// lease gets a distinct slot — verified by writing a unique byte per lease +/// and checking no two leases alias (the "one slot → one lease" invariant +/// under contention), and (c) a further claim returns `None` once all slots +/// are held. +#[test] +fn concurrent_claim_hands_out_distinct_non_aliasing_slots() { + use std::sync::Arc; + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::{Barrier, Mutex}; + + const N: usize = 8; + static POOL: BufferPool = BufferPool::new(); + + let success = Arc::new(AtomicUsize::new(0)); + let barrier = Arc::new(Barrier::new(N)); + // Collect each thread's claimed lease so the slots stay held until after + // we have checked for aliasing and exhaustion (dropping a lease would + // free its slot and let a later claim succeed). + let leases = Arc::new(Mutex::new(std::vec::Vec::new())); + + let mut handles = std::vec::Vec::new(); + for tag in 0..N { + let success = Arc::clone(&success); + let barrier = Arc::clone(&barrier); + let leases = Arc::clone(&leases); + handles.push(std::thread::spawn(move || { + // Maximize contention: every thread reaches `claim()` together. + barrier.wait(); + if let Some(mut lease) = POOL.claim() { + success.fetch_add(1, Ordering::SeqCst); + // Stamp a unique byte for this thread into its slot. If two + // leases aliased the same slot, the later write would clobber + // the earlier one and the per-lease check below would fail. + let stamp = u8::try_from(tag).unwrap(); + lease[0] = stamp; + leases.lock().unwrap().push((stamp, lease)); + } + })); + } + for h in handles { + h.join().unwrap(); + } + + assert_eq!( + success.load(Ordering::SeqCst), + N, + "all {N} concurrent claims should have succeeded against an {N}-slot pool", + ); + + let leases = leases.lock().unwrap(); + assert_eq!(leases.len(), N, "every claim should have produced a lease"); + + // No two leases alias: each lease still reads back its own stamp. Aliasing + // would have let one thread's write overwrite another's slot. + for (stamp, lease) in leases.iter() { + assert_eq!( + lease[0], *stamp, + "lease slot aliased — its stamp byte was clobbered by another lease", + ); + } + + // The pool is fully claimed (all N leases still held), so a further claim + // must fail. + assert!( + POOL.claim().is_none(), + "an {N}-slot pool with {N} leases outstanding must refuse another claim", + ); +} From 6b0aaa39da58c3e7caa09786d3e70773c68dbb95 Mon Sep 17 00:00:00 2001 From: Justin Kovacich Date: Wed, 17 Jun 2026 13:57:22 -0400 Subject: [PATCH 12/12] fix(client): make inbound-oversize drop+survive real on embassy-net; doc + test cleanup (#125) Adversarial-review finding #1: the oversize-drop guard was hollow on tokio (kernel-truncates, pre-existing #119) and FATAL on embassy-net (Truncated -> Io(Other), counted toward the kill cap -> 16 oversize datagrams killed the loop). - New IoErrorKind::Truncated variant, classified transient by is_transient_recv; embassy-net maps RecvError::Truncated to it, so oversize datagrams drop and the loop survives. Io(Other) stays fatal (no masking of real failures). - Honest doc at the guard re: per-backend behavior (tokio MSG_TRUNC is a tracked follow-up); honesty comment on the ScriptSocket guard-mechanism test. - Deleted a vacuous E2E-overflow unit test (1500-byte buffer made its check degenerate); authoritative coverage is bare_metal_e2e's small-buffer test. - Soundness-review nit: BufferLease module doc no longer claims &'static mut. Co-Authored-By: Claude Opus 4.8 (1M context) --- simple-someip-embassy-net/src/socket.rs | 28 ++++---- src/buffer_pool.rs | 15 +++-- src/client/socket_manager.rs | 85 +++++++++---------------- src/transport.rs | 18 +++++- tests/bare_metal_e2e.rs | 11 ++++ 5 files changed, 79 insertions(+), 78 deletions(-) diff --git a/simple-someip-embassy-net/src/socket.rs b/simple-someip-embassy-net/src/socket.rs index b0c19a4b..d82fa195 100644 --- a/simple-someip-embassy-net/src/socket.rs +++ b/simple-someip-embassy-net/src/socket.rs @@ -150,21 +150,19 @@ impl Future for EmbassyNetRecvFut<'_> { } }, Poll::Ready(Err(RecvError::Truncated)) => { - // CONTRACT NOTE: simple-someip's `TransportSocket:: - // recv_from` documents that "a datagram whose payload - // exceeds `buf` is **not** an error; it is returned - // with [`ReceivedDatagram::truncated`] set to `true`." - // // embassy-net 0.4's `poll_recv_from` returns - // `RecvError::Truncated` and (a) does not deliver any - // bytes when the datagram doesn't fit and (b) does - // not surface the original datagram length. We can't - // honor the trait's `truncated: true` semantics - // truthfully — there's no copied prefix to return and - // no original-length to record. This adapter - // therefore treats truncation as a fatal *operator* - // configuration error, mapped to `IoErrorKind::Other` - // so it shows up distinctly in logs. + // `RecvError::Truncated` when the datagram does not fit + // the receive buffer. It delivers NO bytes and does NOT + // surface the original datagram length, so we cannot + // fulfill the `TransportSocket::recv_from` contract + // (`truncated: true` with a partial prefix). + // + // The datagram is therefore dropped. We signal this via + // `IoErrorKind::Truncated`, which `is_transient_recv` + // classifies as a drop-and-continue condition: the + // socket loop survives and does NOT count this toward + // the consecutive-error kill cap. `IoErrorKind::Other` + // (genuine I/O errors) retains its fatal classification. // // The caller-side fix is to size `SocketPool`'s // `RX_BUF` ≥ link MTU (typically 1500). With @@ -172,7 +170,7 @@ impl Future for EmbassyNetRecvFut<'_> { // at 28 B, and `simple-someip::UDP_BUFFER_SIZE` // already at 1500, this branch should never fire // under correct configuration. - Poll::Ready(Err(TransportError::Io(IoErrorKind::Other))) + Poll::Ready(Err(TransportError::Io(IoErrorKind::Truncated))) } } } diff --git a/src/buffer_pool.rs b/src/buffer_pool.rs index 962c4a51..30d8db6a 100644 --- a/src/buffer_pool.rs +++ b/src/buffer_pool.rs @@ -1,7 +1,9 @@ -//! Fixed-capacity pool of `&'static mut [u8]` buffers with claim/release -//! semantics, mirroring the channel pools in this module. A `BufferPool` -//! is declared as a `static` by the consumer; each `claim()` hands out one -//! slot as a `BufferLease` that returns the slot to the pool on drop. +//! Fixed-capacity pool of byte buffers with claim/release semantics, +//! mirroring the channel pools in this module. A `BufferPool` is declared as +//! a `static` (bare-metal) or held behind an `Arc` (std/tokio) by the +//! consumer; each claim hands out one slot as a `BufferLease` (a raw +//! `NonNull` slice whose exclusivity is enforced by the per-slot +//! `AtomicBool`, not the borrow checker) that returns the slot on drop. //! //! Synchronization uses per-slot `AtomicBool` compare-exchange so the same //! code is valid on the bare-metal target and on std without requiring a @@ -37,8 +39,9 @@ use core::sync::atomic::{AtomicBool, Ordering}; /// `store(false, Release)`. No global lock is taken; claim and release are /// individually linearizable. pub struct BufferPool { - // `UnsafeCell` because `claim()` hands out `&'static mut` slices into - // this store. The `claimed` flags ensure at most one live `&mut` per slot. + // `UnsafeCell` because claims hand out raw `NonNull` slices into this + // store; the per-slot `claimed` AtomicBool (not the borrow checker) is + // what guarantees at most one live lease per slot. store: UnsafeCell<[[u8; LEN]; SLOTS]>, // One atomic flag per slot; `true` = slot is currently claimed. claimed: [AtomicBool; SLOTS], diff --git a/src/client/socket_manager.rs b/src/client/socket_manager.rs index d408eb2d..fec0028c 100644 --- a/src/client/socket_manager.rs +++ b/src/client/socket_manager.rs @@ -750,11 +750,26 @@ where })) => { consecutive_recv_errors = 0; if bytes_received > buf.len() { - // A backend reported a datagram larger than the - // claimed buffer. Parsing `&buf[..bytes_received]` - // would index out of bounds (and the bytes past - // `buf.len()` were never written), so drop it - // rather than parse a truncated buffer. + // A backend reported a received length larger than + // the buffer it was given. Parsing + // `&buf[..bytes_received]` would index out of + // bounds (bytes past `buf.len()` were never + // written), so drop this datagram rather than + // parse a truncated buffer. + // + // Backend notes: + // - **tokio**: the kernel silently clamps the copy + // to `buf.len()` (POSIX truncation). The true + // datagram length is never reported here, so + // oversize datagrams are silently truncated and + // parsed rather than dropped. A `MSG_TRUNC` fix + // is tracked as follow-up issue #119. + // - **embassy-net**: `RecvError::Truncated` is now + // mapped to `IoErrorKind::Truncated` (a transient + // recv error) so the socket loop drops the + // datagram and continues without this guard + // firing. The guard below therefore does NOT + // engage for embassy-net oversize datagrams. warn!( "inbound datagram ({bytes_received} B) exceeds claimed buffer ({} B); dropping", buf.len() @@ -1103,56 +1118,16 @@ mod tests { ); } - #[tokio::test] - async fn send_e2e_protected_payload_exceeding_udp_buffer_returns_capacity_error() { - use crate::RawPayload; - use crate::e2e::{E2EProfile, Profile4Config}; - use crate::protocol::{Header, MessageId, MessageType, MessageTypeField, ReturnCode}; - - // Craft a message whose raw-encoded size fits UDP_BUFFER_SIZE (16-byte - // SOME/IP header + payload <= cap) but whose E2E-protected size - // does not — Profile 4 adds `PROFILE4_HEADER_SIZE = 12` bytes, - // so a payload of `UDP_BUFFER_SIZE - 16 - 4` exactly fits raw and - // overflows by 8 once protected. Derive both fixture sizes from - // `UDP_BUFFER_SIZE` so this stays correct if the constant moves. - const SOMEIP_HEADER_SIZE: usize = 16; - const PAYLOAD_LEN: usize = UDP_BUFFER_SIZE - SOMEIP_HEADER_SIZE - 4; - - // Register an E2E profile so the protect branch runs. - let message_id = MessageId::new_from_service_and_method(0x1234, 0x5678); - let key = E2EKey::from_message_id(message_id); - let mut reg = E2ERegistry::new(); - reg.register(key, E2EProfile::Profile4(Profile4Config::new(0, 15))) - .expect("E2E registry has capacity for one entry"); - let e2e_registry = Arc::new(Mutex::new(reg)); - - let mut sm = SocketManager::::bind(0, e2e_registry) - .await - .unwrap(); - - let payload_bytes = [0u8; PAYLOAD_LEN]; - let payload = RawPayload::from_payload_bytes(message_id, &payload_bytes).unwrap(); - let header = Header::new( - message_id, - 0x0001_0001, - 0x01, - 0x01, - MessageTypeField::new(MessageType::Request, false), - ReturnCode::Ok, - payload_bytes.len(), - ); - let message = Message::new(header, payload); - - let target = SocketAddrV4::new(Ipv4Addr::LOCALHOST, 9999); - let err = sm - .send(target, message) - .await - .expect_err("E2E-protected oversize message must error"); - match err { - Error::Capacity(tag) => assert_eq!(tag, "udp_buffer"), - other => panic!("expected Error::Capacity(\"udp_buffer\"), got {other:?}"), - } - } + // `send_e2e_protected_payload_exceeding_udp_buffer_returns_capacity_error` + // was deleted (vacuous — it bound a full `UDP_BUFFER_SIZE` send buffer, so + // the E2E-overflow guard it claimed to test (`16 + protected_len > + // buf.len()`) coincided with `required > UDP_BUFFER_SIZE` and passed + // against both old and new code without exercising the smaller-buffer + // path that the bare-metal pool unlocks). The authoritative coverage lives + // in `tests/bare_metal_e2e.rs:: + // e2e_protect_expanding_payload_beyond_leased_buffer_returns_capacity_error`, + // which supplies a pool whose buffer is genuinely smaller than + // `UDP_BUFFER_SIZE` and verifies the `buf.len()`-keyed guard fires. /// Proves the public `bind_with_transport` entry point accepts an /// alternative `TransportFactory` implementation. The factory here is diff --git a/src/transport.rs b/src/transport.rs index cd2d1c61..8529046e 100644 --- a/src/transport.rs +++ b/src/transport.rs @@ -264,6 +264,15 @@ pub enum IoErrorKind { /// fatal. #[error("would block")] WouldBlock, + /// An inbound datagram was truncated because it exceeded the receive + /// buffer. The datagram is discarded; the socket loop survives. + /// + /// Backends that receive this signal MUST drop the datagram and continue + /// polling — it does NOT count toward the consecutive-error kill cap. + /// This variant is distinct from [`Self::Other`] so that genuine I/O + /// errors are still counted as potentially-fatal. + #[error("inbound datagram truncated (exceeded buffer)")] + Truncated, /// Any error that does not fit a more specific variant. #[error("i/o error")] Other, @@ -281,7 +290,11 @@ impl IoErrorKind { /// - [`Self::WouldBlock`] — by definition, retry-on-readiness; /// - [`Self::Interrupted`] — a signal interrupted the syscall; /// - [`Self::TimedOut`] — caller-driven timeout, not a socket - /// failure. + /// failure; + /// - [`Self::Truncated`] — an inbound datagram was truncated because + /// it exceeded the receive buffer; the datagram is dropped and the + /// loop continues (distinct from [`Self::Other`] so genuine I/O + /// errors are still counted as potentially-fatal). /// /// All other kinds (including [`Self::Other`]) are treated as /// potentially-fatal and DO count toward the cap. @@ -293,7 +306,8 @@ impl IoErrorKind { | Self::NetworkUnreachable | Self::WouldBlock | Self::Interrupted - | Self::TimedOut, + | Self::TimedOut + | Self::Truncated, ) } } diff --git a/tests/bare_metal_e2e.rs b/tests/bare_metal_e2e.rs index 3efcfaab..5ceeff3a 100644 --- a/tests/bare_metal_e2e.rs +++ b/tests/bare_metal_e2e.rs @@ -812,6 +812,17 @@ impl TransportSocket for ScriptSocket { /// must survive, and a subsequent in-budget datagram must still be /// delivered. Driven through the public `Client` discovery socket because /// `socket_loop_future` / `SocketManager` are private. +/// +/// **Scope note:** this test exercises the `bytes_received > buf.len()` +/// guard *mechanism* using a synthetic `ScriptSocket` that reports an +/// unclamped length. No shipped backend currently feeds the guard via +/// that exact shape: +/// - tokio: the kernel silently truncates to `buf.len()` — an oversize +/// datagram is silently truncated+parsed (pre-existing #119 behavior; +/// a `MSG_TRUNC` fix is a tracked follow-up). +/// - embassy-net: `RecvError::Truncated` is mapped to +/// `IoErrorKind::Truncated` (transient recv), so the loop drops and +/// continues without the `bytes_received > buf.len()` branch firing. #[tokio::test] async fn inbound_datagram_larger_than_claimed_buffer_is_dropped_not_fatal() { // Claim buffers of exactly 64 bytes: big enough for a small SD message