From e87c128787c38c13870a7e6f32bb9ac41a1f788b Mon Sep 17 00:00:00 2001 From: Justin Kovacich Date: Fri, 24 Apr 2026 21:06:21 -0400 Subject: [PATCH 1/2] phase 9: Spawner trait + executor-agnostic Client construction Final state of #83, squashed from 6 commits to keep the rebase onto the rewritten #82 tractable. Original commits, oldest first: - cfbd17a Added a Spawner trait in transport.rs. TokioSpawner is the default std impl. Client::new_with_spawner_and_loopback accepts a caller-supplied spawner so the bare-metal port can drive socket loops without tokio. New unit tests and bare_metal example now demonstrates usage with a MockSpawner. - 9122afd New bind tests; corrected documentation throughout to reflect the concrete next steps toward bare-metal support. - ee305e0 phase 9: fix intra-doc links that break default-feature rustdoc builds. Three sites that referenced feature-gated items via intra-doc links: tokio_transport's `[Spawner]` -> `[crate::transport::Spawner]`; transport.rs link to `[crate::tokio_transport::TokioSpawner]` -> code literal with feature-gate note; same pattern at transport.rs:478 for `[crate::client::Client]`. Also lib.rs module table: intra-doc links for feature-gated `[client]` / `[server]` modules -> plain code literals. - d618081 round-2: fix intra-doc link + correct channel-capacity docs. Replace the intra-doc link to Client::new_with_spawner_and_loopback in tokio_transport.rs with a plain code literal (breaks default-feature rustdoc in feature="server"-without-feature="client"). Correct socket_manager module doc: per-socket mpsc channels are 16/16 for discovery sockets, 4/4 for unicast (verified against mpsc::channel call sites). - d2c44e0 chore(clippy): address new warnings in phase9 PR (13 fixes). doc_markdown: backtick `no_alloc`, `no_std`, `bare_metal` identifiers in new docs. double_must_use on Client::new_with_spawner_and_loopback: replace bare #[must_use] with a message about the run-loop future. dead_code on SocketManager::{bind, bind_discovery_seeded}: gate under #[cfg(test)] since the Spawner refactor removes non-test callers. - 721223c round-3: address Copilot review on socket_manager doc links + test JoinHandle. socket_manager: render TokioTransport / TokioSpawner refs as code literals (same rustdoc-feature-gating pattern as elsewhere in the PR) so default-feature doc builds no longer see broken intra-doc links. client::tests: bind the JoinHandle from tokio::spawn(future) to `_` in the CountingSpawner test so the #[must_use] lint does not fire. Co-Authored-By: Claude Opus 4.7 (1M context) --- examples/bare_metal/src/main.rs | 86 ++++++++++++++---- src/client/inner.rs | 131 +++++++++++++++++++++++++-- src/client/mod.rs | 114 ++++++++++++++++++++++- src/client/socket_manager.rs | 155 ++++++++++++++++++++++++-------- src/lib.rs | 10 +-- src/tokio_transport.rs | 21 +++++ src/transport.rs | 70 +++++++++++++++ 7 files changed, 519 insertions(+), 68 deletions(-) diff --git a/examples/bare_metal/src/main.rs b/examples/bare_metal/src/main.rs index 33782ac..eeb7cdf 100644 --- a/examples/bare_metal/src/main.rs +++ b/examples/bare_metal/src/main.rs @@ -36,21 +36,52 @@ //! //! # Known gaps in the bare-metal story (independent of this example) //! -//! `SocketManager::bind*` today still pins `F::Socket = TokioSocket`, -//! so the trait impls below — while correct — cannot be plugged into -//! the crate's `Client` / `Server` event loops yet. Two upstream -//! blockers must land first: +//! The example exercises the **trait layer** (`TransportSocket`, +//! `TransportFactory`, `Timer`, `Spawner`) — and that is all. It does +//! NOT demonstrate a no_alloc integration with +//! `simple_someip::Client` / `simple_someip::Server`, because those +//! are not yet no_alloc-compatible. Phase 9 landed `Spawner`, which +//! abstracts ONE runtime primitive (task submission). Four others +//! remain before a no_alloc consumer can use `Client`: //! -//! 1. Relax the `F::Socket = TokioSocket` bound to -//! `F::Socket: TransportSocket` (requires stable Return-Type -//! Notation or a GAT-based parallel trait). -//! 2. Extract a `Spawner` trait so `SocketManager::bind*` can submit -//! per-socket loops to the user's executor instead of calling -//! `tokio::spawn` directly. See phase 9 in the refactor plan. +//! 1. **`tokio::sync::mpsc` channels** inside `SocketManager` +//! (capacities 4 and 16 per socket): heap-allocated AND +//! tokio-runtime-coupled (the `Waker` plumbing only works on a +//! tokio task). +//! 2. **`tokio::sync::oneshot`** used for send-ack round-trips: same +//! allocation + runtime-coupling issue. +//! 3. **`Arc>`** shared between the client's +//! control path and every per-socket loop: requires `alloc` + +//! `std::sync`. +//! 4. **`F::Socket = TokioSocket`** bound on `bind_*`: a phase-5 +//! compromise because stable Rust Return-Type Notation is still +//! nightly. //! -//! Until (1) and (2) land, bare-metal users CAN implement the traits -//! below, but they CANNOT route their implementations through -//! `Client` / `Server`. +//! Closing those four is additional phased work (roughly the same +//! scope again as phases 1–9 combined). Until then, `feature = "client"` +//! / `feature = "server"` pull in `std + tokio + socket2`. +//! +//! # Recommendation for no_alloc consumers today +//! +//! Do NOT route through `Client::new_with_spawner_and_loopback`. +//! Instead, depend on `simple-someip` with `default-features = false, +//! features = ["bare_metal"]` and consume the already-portable layers +//! directly: +//! +//! - `simple_someip::protocol` — wire format (headers, messages, SD +//! entries/options); zero-copy views for parsing. +//! - `simple_someip::e2e` — CRC-32 / CRC-16 protection profiles; owned +//! per-payload, no `Arc>` required. +//! - `simple_someip::transport` — the four traits exercised below. +//! +//! Then write a small SOME/IP orchestrator that owns its socket, a +//! stack-allocated request-map (e.g. +//! `heapless::FnvIndexMap`), and drives SD + r/r + +//! event subscription using `futures::select!` over +//! `TransportSocket::recv_from` / `Timer::sleep` directly. That is +//! the shape the trait layer was designed for; the `Client` / +//! `Server` types are a std+tokio convenience layer on top that +//! happens not to suit no_alloc targets yet. use core::future::Future; use core::net::{Ipv4Addr, SocketAddrV4}; @@ -197,6 +228,21 @@ impl Timer for MockTimer { } } +/// Phase 9 `Spawner` impl. A real bare-metal `Spawner` wraps the +/// executor's task-submission primitive — `embassy_executor::Spawner`, +/// smoltcp's task pool, or a hand-rolled single-core polling loop. +/// This mock drops every future it receives (equivalent to "never run +/// it"), which is fine for the demo because nothing in the trait-layer +/// round-trip below actually requires a spawned task. A production +/// impl must poll the future to completion. +struct MockSpawner; + +impl simple_someip::transport::Spawner for MockSpawner { + fn spawn(&self, _future: impl Future + Send + 'static) { + // DEMO-ONLY: real impls submit `_future` to their task pool. + } +} + /// Single-step `block_on` for the demo. /// /// **ANTI-PATTERN — DO NOT USE IN PRODUCTION.** `Waker::noop()` means @@ -275,6 +321,11 @@ fn main() { let timer = MockTimer; block_on(timer.sleep(Duration::from_millis(1))); + // Demonstrate the Spawner trait compiles against a MockSpawner. + // (The mock drops the future — a real spawner polls it.) + let spawner = MockSpawner; + simple_someip::transport::Spawner::spawn(&spawner, async {}); + println!( "bare-metal example: sent {} bytes from {} to {}, received cleanly.", datagram.bytes_received, @@ -282,7 +333,12 @@ fn main() { sock_b.local_addr().unwrap(), ); println!( - "note: this only exercises the trait layer — see source comments \ - for the Client/Server + Spawner gap (phase 9 work)." + "note: trait layer (TransportSocket + TransportFactory + Timer + \ + Spawner) exercised end-to-end. For a no_alloc SOME/IP client \ + today, build your own orchestrator on `protocol` + `e2e` + these \ + traits — do NOT route through `Client::new_with_spawner_and_loopback`: \ + the Client internals still depend on tokio::sync::mpsc/oneshot, \ + Arc>, and an F::Socket=TokioSocket bound (RTN). \ + See top-of-file docblock for the full blocker list." ); } diff --git a/src/client/inner.rs b/src/client/inner.rs index 586f29a..96832ca 100644 --- a/src/client/inner.rs +++ b/src/client/inner.rs @@ -23,8 +23,9 @@ use crate::{ }, e2e::E2ERegistry, protocol::{self, Message}, - tokio_transport::TokioTimer, + tokio_transport::{TokioSpawner, TokioTimer, TokioTransport}, traits::PayloadWireFormat, + transport::Spawner, }; use super::error::Error; @@ -289,7 +290,7 @@ impl ControlMessage

{ } } -pub(super) struct Inner { +pub(super) struct Inner { /// MPSC Receiver used to receive control messages from outer client control_receiver: Receiver>, /// Queue of pending control messages to process @@ -324,11 +325,15 @@ pub(super) struct Inner { e2e_registry: Arc>, /// Enable multicast loopback on SD sockets for same-host testing multicast_loopback: bool, + /// Task-spawner used by `bind_*` to drive per-socket I/O loops. + /// Default [`TokioSpawner`] wraps `tokio::spawn`; bare-metal + /// callers plug in their own. + spawner: S, /// Phantom data to represent the generic message definitions phantom: std::marker::PhantomData, } -impl std::fmt::Debug for Inner { +impl std::fmt::Debug for Inner { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("Inner") .field("interface", &self.interface) @@ -340,9 +345,10 @@ impl std::fmt::Debug for Inner Inner +impl Inner where PayloadDefinitions: PayloadWireFormat + Clone + std::fmt::Debug + 'static, + S: Spawner + Send + Sync + 'static, { /// Construct an `Inner` and return the control/update channels plus /// the run-loop future. The caller must drive the future on a Tokio @@ -358,6 +364,7 @@ where interface: Ipv4Addr, e2e_registry: Arc>, multicast_loopback: bool, + spawner: S, ) -> ( Sender>, mpsc::UnboundedReceiver>, @@ -383,6 +390,7 @@ where sd_session_has_wrapped: false, e2e_registry, multicast_loopback, + spawner, phantom: std::marker::PhantomData, }; (control_sender, update_receiver, inner.run_future()) @@ -392,7 +400,9 @@ where if self.discovery_socket.is_some() { Ok(()) } else { - let socket = SocketManager::bind_discovery_seeded( + let socket = SocketManager::bind_discovery_seeded_with_transport( + &TokioTransport, + &self.spawner, self.interface, Arc::clone(&self.e2e_registry), self.sd_session_id, @@ -435,7 +445,13 @@ where ); return Err(Error::Capacity("unicast_sockets")); } - let unicast_socket = SocketManager::bind(port, Arc::clone(&self.e2e_registry)).await?; + let unicast_socket = SocketManager::bind_with_transport( + &TokioTransport, + &self.spawner, + port, + Arc::clone(&self.e2e_registry), + ) + .await?; let bound_port = unicast_socket.port(); // Capacity was checked above, so insert cannot report "full" here. // A defensive check guards against a future refactor that changes @@ -947,8 +963,8 @@ where let sleep_fut = TokioTimer .sleep(std::time::Duration::from_millis(125)) .fuse(); - let discovery_fut = Inner::receive_discovery(discovery_socket).fuse(); - let unicast_fut = Inner::receive_any_unicast(unicast_sockets).fuse(); + let discovery_fut = Self::receive_discovery(discovery_socket).fuse(); + let unicast_fut = Self::receive_any_unicast(unicast_sockets).fuse(); pin_mut!(control_fut, sleep_fut, discovery_fut, unicast_fut); // `select!` (not `select_biased!`) randomizes the @@ -1253,6 +1269,7 @@ mod tests { sd_session_has_wrapped: false, e2e_registry: Arc::new(Mutex::new(E2ERegistry::new())), multicast_loopback: false, + spawner: TokioSpawner, phantom: std::marker::PhantomData, } } @@ -1425,12 +1442,89 @@ mod tests { ); } + /// Sibling to `client_new_with_spawner_routes_socket_spawns_through_it` + /// in `mod.rs`, which covers the `bind_discovery` path. This one + /// covers `bind_unicast`: each successful ephemeral unicast bind + /// must submit exactly one future through the injected `Spawner`. + /// Without this test, a future refactor could silently revert the + /// unicast bind path to direct `tokio::spawn` and only the + /// discovery path's test would fail to catch it. + #[tokio::test] + async fn bind_unicast_routes_through_injected_spawner() { + use core::sync::atomic::{AtomicUsize, Ordering}; + + #[derive(Clone)] + struct CountingSpawner { + count: Arc, + } + + impl Spawner for CountingSpawner { + fn spawn(&self, future: impl core::future::Future + Send + 'static) { + self.count.fetch_add(1, Ordering::SeqCst); + // Delegate so the socket loop actually runs — matters + // if the caller later issues a send that awaits the + // loop's oneshot ack. For the pure-spawn-count + // assertion below it would also work to drop the + // future; we delegate to keep the Inner in a healthy + // state in case assertion ordering changes. + drop(tokio::spawn(future)); + } + } + + let count = Arc::new(AtomicUsize::new(0)); + let spawner = CountingSpawner { + count: Arc::clone(&count), + }; + + // Build Inner directly with the counting spawner — same pattern + // as `make_inner_for_test`, but parameterized on S. + let (_control_sender, control_receiver) = mpsc::channel(4); + let (update_sender, _update_receiver) = mpsc::unbounded_channel(); + let mut inner: Inner = Inner { + control_receiver, + request_queue: Deque::new(), + pending_responses: FnvIndexMap::new(), + update_sender, + interface: Ipv4Addr::LOCALHOST, + discovery_socket: None, + unicast_sockets: FnvIndexMap::new(), + session_tracker: SessionTracker::default(), + service_registry: ServiceRegistry::default(), + run: true, + client_id: 0x1234, + session_counter: 1, + sd_session_id: 1, + sd_session_has_wrapped: false, + e2e_registry: Arc::new(Mutex::new(E2ERegistry::new())), + multicast_loopback: false, + spawner, + phantom: std::marker::PhantomData, + }; + + // Three ephemeral binds → three distinct socket loops spawned. + for i in 0..3 { + let bound = inner + .bind_unicast(0) + .await + .expect("ephemeral bind should succeed"); + assert_ne!(bound, 0, "iteration {i}: OS should assign a port"); + } + + assert_eq!( + count.load(Ordering::SeqCst), + 3, + "expected exactly three spawns (one per bind_unicast call), got {}", + count.load(Ordering::SeqCst) + ); + } + #[tokio::test] async fn test_inner_build_and_shutdown() { let (control_sender, mut update_receiver, run_fut) = Inner::::build( Ipv4Addr::LOCALHOST, Arc::new(Mutex::new(E2ERegistry::new())), false, + TokioSpawner, ); let _ = tokio::spawn(run_fut); // Drop control sender to trigger loop exit @@ -1466,6 +1560,7 @@ mod tests { Ipv4Addr::LOCALHOST, Arc::new(Mutex::new(E2ERegistry::new())), false, + TokioSpawner, ); let _ = tokio::spawn(run_fut); @@ -1483,6 +1578,7 @@ mod tests { Ipv4Addr::LOCALHOST, Arc::new(Mutex::new(E2ERegistry::new())), false, + TokioSpawner, ); let _ = tokio::spawn(run_fut); @@ -1500,6 +1596,7 @@ mod tests { Ipv4Addr::LOCALHOST, Arc::new(Mutex::new(E2ERegistry::new())), false, + TokioSpawner, ); let _ = tokio::spawn(run_fut); @@ -1519,6 +1616,7 @@ mod tests { Ipv4Addr::LOCALHOST, Arc::new(Mutex::new(E2ERegistry::new())), false, + TokioSpawner, ); let _ = tokio::spawn(run_fut); @@ -1549,6 +1647,7 @@ mod tests { Ipv4Addr::LOCALHOST, Arc::new(Mutex::new(E2ERegistry::new())), false, + TokioSpawner, ); let _ = tokio::spawn(run_fut); @@ -1620,6 +1719,7 @@ mod tests { Ipv4Addr::LOCALHOST, Arc::new(Mutex::new(E2ERegistry::new())), false, + TokioSpawner, ); let _ = tokio::spawn(run_fut); @@ -1638,6 +1738,7 @@ mod tests { Ipv4Addr::LOCALHOST, Arc::new(Mutex::new(E2ERegistry::new())), false, + TokioSpawner, ); let _ = tokio::spawn(run_fut); @@ -1655,6 +1756,7 @@ mod tests { Ipv4Addr::LOCALHOST, Arc::new(Mutex::new(E2ERegistry::new())), false, + TokioSpawner, ); let _ = tokio::spawn(run_fut); @@ -1682,6 +1784,7 @@ mod tests { Ipv4Addr::LOCALHOST, Arc::new(Mutex::new(E2ERegistry::new())), true, + TokioSpawner, ); let _ = tokio::spawn(run_fut); @@ -1697,6 +1800,7 @@ mod tests { Ipv4Addr::LOCALHOST, Arc::new(Mutex::new(E2ERegistry::new())), false, + TokioSpawner, ); let _ = tokio::spawn(run_fut); @@ -1717,6 +1821,7 @@ mod tests { Ipv4Addr::LOCALHOST, Arc::new(Mutex::new(E2ERegistry::new())), false, + TokioSpawner, ); let _ = tokio::spawn(run_fut); @@ -1738,6 +1843,7 @@ mod tests { Ipv4Addr::LOCALHOST, Arc::new(Mutex::new(E2ERegistry::new())), false, + TokioSpawner, ); let _ = tokio::spawn(run_fut); @@ -1763,6 +1869,7 @@ mod tests { Ipv4Addr::LOCALHOST, Arc::new(Mutex::new(E2ERegistry::new())), false, + TokioSpawner, ); let _ = tokio::spawn(run_fut); @@ -1794,6 +1901,7 @@ mod tests { Ipv4Addr::LOCALHOST, Arc::new(Mutex::new(E2ERegistry::new())), false, + TokioSpawner, ); let _ = tokio::spawn(run_fut); @@ -1819,6 +1927,7 @@ mod tests { Ipv4Addr::LOCALHOST, Arc::new(Mutex::new(E2ERegistry::new())), false, + TokioSpawner, ); let _ = tokio::spawn(run_fut); @@ -1838,6 +1947,7 @@ mod tests { Ipv4Addr::LOCALHOST, Arc::new(Mutex::new(E2ERegistry::new())), false, + TokioSpawner, ); let _ = tokio::spawn(run_fut); @@ -1873,6 +1983,7 @@ mod tests { Ipv4Addr::LOCALHOST, Arc::new(Mutex::new(E2ERegistry::new())), false, + TokioSpawner, ); let _ = tokio::spawn(run_fut); @@ -1891,6 +2002,7 @@ mod tests { Ipv4Addr::LOCALHOST, Arc::new(Mutex::new(E2ERegistry::new())), false, + TokioSpawner, ); let _ = tokio::spawn(run_fut); @@ -1916,6 +2028,7 @@ mod tests { Ipv4Addr::LOCALHOST, Arc::new(Mutex::new(E2ERegistry::new())), false, + TokioSpawner, ); let _ = tokio::spawn(run_fut); @@ -1947,6 +2060,7 @@ mod tests { Ipv4Addr::LOCALHOST, Arc::new(Mutex::new(E2ERegistry::new())), false, + TokioSpawner, ); let _ = tokio::spawn(run_fut); @@ -1994,6 +2108,7 @@ mod tests { Ipv4Addr::LOCALHOST, Arc::new(Mutex::new(E2ERegistry::new())), false, + TokioSpawner, ); let _ = tokio::spawn(run_fut); diff --git a/src/client/mod.rs b/src/client/mod.rs index fef0d03..46e5bc7 100644 --- a/src/client/mod.rs +++ b/src/client/mod.rs @@ -38,7 +38,8 @@ pub use error::Error; use crate::Timer; use crate::e2e::{E2ECheckStatus, E2EKey, E2EProfile, E2ERegistry}; -use crate::tokio_transport::TokioTimer; +use crate::tokio_transport::{TokioSpawner, TokioTimer}; +use crate::transport::Spawner; use crate::{protocol, protocol::Message, traits::PayloadWireFormat}; use inner::{ControlMessage, Inner}; use std::net::{Ipv4Addr, SocketAddr, SocketAddrV4}; @@ -254,9 +255,61 @@ where ClientUpdates, impl core::future::Future + Send + 'static, ) { + Self::new_with_spawner_and_loopback(interface, multicast_loopback, TokioSpawner) + } + + /// Like [`Self::new_with_loopback`], but with a caller-provided + /// [`Spawner`]. Per-socket I/O loops are submitted through this + /// spawner instead of the default [`TokioSpawner`] / `tokio::spawn`. + /// + /// ```no_run + /// # use simple_someip::{Client, RawPayload, Spawner}; + /// # use std::net::Ipv4Addr; + /// # async fn demo() { + /// struct MySpawner; // ...your executor's task-submission type. + /// # impl Spawner for MySpawner { + /// # fn spawn(&self, _: impl core::future::Future + Send + 'static) {} + /// # } + /// let (client, mut updates, run) = + /// Client::::new_with_spawner_and_loopback( + /// Ipv4Addr::LOCALHOST, + /// false, + /// MySpawner, + /// ); + /// tokio::spawn(run); + /// # let _ = (client, updates); + /// # } + /// ``` + /// + /// # Bounds + /// + /// `S: Spawner + Send + Sync + 'static` — the spawner is stored in + /// the run-loop future, which is `Send + 'static`, so the spawner + /// must match those bounds. `Sync` is required because `&self.spawner` + /// is held across `.await` points inside + /// `SocketManager::bind_with_transport` and + /// `bind_discovery_seeded_with_transport`, both of which execute on + /// the driven run-loop task (not on the user's call site). + #[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_spawner_and_loopback( + interface: Ipv4Addr, + multicast_loopback: bool, + spawner: S, + ) -> ( + Self, + ClientUpdates, + impl core::future::Future + Send + 'static, + ) + where + S: Spawner + Send + Sync + 'static, + { let e2e_registry = Arc::new(Mutex::new(E2ERegistry::new())); - let (control_sender, update_receiver, run_future) = - Inner::build(interface, Arc::clone(&e2e_registry), multicast_loopback); + let (control_sender, update_receiver, run_future) = Inner::build( + interface, + Arc::clone(&e2e_registry), + multicast_loopback, + spawner, + ); let client = Self { interface: Arc::new(RwLock::new(interface)), @@ -1500,4 +1553,59 @@ mod tests { let handle = std::thread::spawn(move || drop(run_fut)); handle.join().unwrap(); } + + /// Proves `Client::new_with_spawner_and_loopback` actually routes + /// per-socket spawns through the user-provided `Spawner`. The + /// `CountingSpawner` below increments a shared counter on every + /// `spawn` call AND delegates to `tokio::spawn` so the spawned + /// futures still run. Calling `bind_discovery` should cause + /// exactly one spawn (the SD socket's I/O loop); calling + /// `bind_discovery` again is a no-op (socket already bound) so + /// the count stays at 1. + #[tokio::test] + async fn client_new_with_spawner_routes_socket_spawns_through_it() { + use core::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::Arc; + + #[derive(Clone)] + struct CountingSpawner { + count: Arc, + } + + impl Spawner for CountingSpawner { + fn spawn(&self, future: impl core::future::Future + Send + 'static) { + self.count.fetch_add(1, Ordering::SeqCst); + let _ = tokio::spawn(future); + } + } + + let count = Arc::new(AtomicUsize::new(0)); + let spawner = CountingSpawner { + count: Arc::clone(&count), + }; + + let (client, _updates, run_fut) = + TestClient::new_with_spawner_and_loopback(Ipv4Addr::LOCALHOST, false, spawner); + tokio::spawn(run_fut); + + client + .bind_discovery() + .await + .expect("bind_discovery must succeed"); + // Idempotent second call; must NOT spawn again. + client + .bind_discovery() + .await + .expect("second bind_discovery is idempotent"); + + assert_eq!( + count.load(Ordering::SeqCst), + 1, + "expected exactly one spawn for the SD socket loop, \ + got {}", + count.load(Ordering::SeqCst) + ); + + client.shut_down(); + } } diff --git a/src/client/socket_manager.rs b/src/client/socket_manager.rs index 79d6499..567ccb2 100644 --- a/src/client/socket_manager.rs +++ b/src/client/socket_manager.rs @@ -1,13 +1,15 @@ //! Client-side UDP socket management. //! //! Each bound socket is backed by a `TokioSocket` (concrete, phase-5 -//! compromise) with its I/O loop running on a `tokio::spawn`'d task. -//! That spawn is the last `tokio::spawn` call inside the library -//! critical path — every other spawn was hoisted out to the caller in -//! phase 6 / 7. This one can't be hoisted until a `Spawner` trait is -//! introduced (planned for phase 9). +//! compromise — see the `bind_discovery_seeded_with_transport` +//! docstring for the RTN-gap analysis) with its I/O loop running on a +//! caller-supplied [`crate::transport::Spawner`]. Phase 9 introduced +//! the `Spawner` trait specifically to make this submission point +//! pluggable; on `std + tokio` consumers pass +//! [`crate::tokio_transport::TokioSpawner`] and the behavior matches +//! the previous `tokio::spawn` path exactly. //! -//! # Why the `tokio::spawn` in `bind_*` is still there +//! # Why `Inner` can't drive per-socket futures itself //! //! Briefly experimented with having `Inner` drive per-socket futures //! via `FuturesUnordered` (phase 8 attempt, reverted). That deadlocks: @@ -15,23 +17,44 @@ //! which internally awaits an mpsc→oneshot round-trip that requires //! the socket loop to make progress. But `Inner::run_future` is //! parked inside the handler, so nothing polls the socket loop. -//! Concurrency between the two is mandatory; on tokio we get it via -//! `tokio::spawn` giving each socket its own task. +//! Concurrency between the two is mandatory and cannot come from the +//! same task — hence the `Spawner` hook. //! -//! The fix is either (a) a `Spawner` trait that `Inner::bind_*` uses -//! instead of calling `tokio::spawn` directly (small, contradicts the -//! phase-4 "no `ExecutorAdapter`" decision but warranted given concrete -//! evidence), or (b) a non-await `SocketManager::send` that defers -//! completion to a later `select!` iteration (invasive). Phase 9 -//! picks (a). +//! # What phase 9's `Spawner` does NOT remove from the critical path +//! +//! `Spawner` abstracts task submission, not runtime primitives. The +//! socket loop still `.await`s on runtime-coupled types every +//! iteration. `no_alloc` bare-metal consumers are still blocked by: +//! +//! 1. **`tokio::sync::mpsc` channels** (per-socket: discovery uses +//! 16/16, unicast uses 4/4): heap-allocated + tokio-`Waker`- +//! specific. A `no_alloc` replacement needs a bounded inline-backed +//! channel with executor-agnostic waker registration (e.g. +//! `heapless::mpmc` + a hand-rolled `WakerRegistration`, or +//! `embassy-sync::Channel`). +//! 2. **`tokio::sync::oneshot` for send-acks** (see `SendMessage` +//! below): same problem at smaller scale; ownership restructure +//! is harder than the mpsc swap. +//! 3. **`Arc>`** shared between `Inner` and every +//! socket loop: requires `alloc` + `std::sync`. Collapses to +//! `&RefCell` on a single-task executor, but the +//! type change cascades through every call site. +//! 4. **`F::Socket = TokioSocket`** bound on `bind_*` (this module): +//! RTN-gap, see `bind_discovery_seeded_with_transport` docstring. +//! +//! Until all four are addressed, enabling `feature = "client"` pulls +//! in `std + tokio + socket2`. The `bare_metal` feature flag is a +//! marker today; it does not make this module `no_alloc`. For `no_alloc` +//! SOME/IP usage today, consume `protocol`, `e2e`, and the `transport` +//! trait layer directly — the `bare_metal` example workspace member +//! demonstrates that surface. use crate::{ UDP_BUFFER_SIZE, e2e::{E2ECheckStatus, E2EKey, E2ERegistry}, protocol::{Message, MessageView, sd}, - tokio_transport::TokioTransport, traits::{PayloadWireFormat, WireFormat}, - transport::{ReceivedDatagram, SocketOptions, TransportFactory, TransportSocket}, + transport::{ReceivedDatagram, SocketOptions, Spawner, TransportFactory, TransportSocket}, }; use super::error::Error; @@ -115,9 +138,19 @@ where /// reboot signal (`reboot_flag=1`) to peers after /// `unbind_discovery` + `bind_discovery`. /// - /// Uses the default [`TokioTransport`] backend. For tests or alternate - /// bind logic (e.g. an interceptor factory around `TokioTransport`), - /// use [`Self::bind_discovery_seeded_with_transport`]. + /// Uses the default `crate::tokio_transport::TokioTransport` and + /// `crate::tokio_transport::TokioSpawner` backends (rendered as + /// code literals because `tokio_transport` is only compiled with + /// the `client`/`server` features and an intra-doc link would + /// break default-feature rustdoc builds). + /// For tests or alternate bind logic (e.g. an interceptor factory + /// around `TokioTransport`), use + /// [`Self::bind_discovery_seeded_with_transport`]. + /// + /// Currently `#[cfg(test)]`-gated: production callers reach the + /// socket through the `_with_transport` variant so the `Spawner` + /// trait can be exercised end-to-end. + #[cfg(test)] pub async fn bind_discovery_seeded( interface: Ipv4Addr, e2e_registry: Arc>, @@ -125,8 +158,10 @@ where session_has_wrapped: bool, multicast_loopback: bool, ) -> Result { + use crate::tokio_transport::{TokioSpawner, TokioTransport}; Self::bind_discovery_seeded_with_transport( &TokioTransport, + &TokioSpawner, interface, e2e_registry, session_id, @@ -137,15 +172,37 @@ where } /// Variant of [`Self::bind_discovery_seeded`] that constructs the - /// underlying socket through a caller-supplied [`TransportFactory`]. + /// underlying socket through a caller-supplied [`TransportFactory`] + /// and submits the socket's I/O loop through a caller-supplied + /// [`Spawner`]. + /// + /// # Why `F::Socket` is still pinned to `TokioSocket` + /// /// The factory must still produce a - /// [`TokioSocket`](crate::tokio_transport::TokioSocket) because the - /// spawned I/O loop is currently tokio-specific; the bound will be - /// relaxed to any `TransportSocket` once the `tokio::spawn` that - /// drives `socket_loop_future` is hoisted out of `bind_discovery_*` - /// (tracked separately; phase 9+ spawner-trait work). - pub async fn bind_discovery_seeded_with_transport( + /// [`TokioSocket`](crate::tokio_transport::TokioSocket). Generalizing + /// to any `TransportSocket` requires stable-Rust Return-Type Notation + /// (RFC 3654) to express `Send` bounds on the trait's RPITIT methods + /// at this call site. RTN is nightly-only as of this writing; the + /// alternatives (GATs on `TransportSocket`, or boxed-future + /// type-erasure) each carry costs bigger than waiting — see the + /// module docstring for the full analysis. + /// + /// # Why relaxing this bound alone does NOT unblock `no_alloc` callers + /// + /// Even with a custom `F::Socket`, this function internally + /// allocates two `tokio::sync::mpsc` channels (capacities 16 and 16) + /// and constructs `tokio::sync::oneshot` instances per send. Both + /// are heap-backed AND tokio-runtime-coupled (their `Waker` + /// plumbing only works inside a tokio reactor task). A `no_alloc` + /// bare-metal consumer cannot use this entry point today regardless + /// of the `F::Socket` bound. The recommended path for `no_alloc` + /// consumers is to bypass `SocketManager` / `Client` entirely and + /// build a small orchestrator directly on top of `protocol`, `e2e`, + /// and the `transport` traits — the `bare_metal` example workspace + /// member demonstrates the trait layer in isolation. + pub async fn bind_discovery_seeded_with_transport( factory: &F, + spawner: &S, interface: Ipv4Addr, e2e_registry: Arc>, session_id: u16, @@ -154,6 +211,7 @@ where ) -> Result where F: TransportFactory, + S: Spawner, { let (rx_tx, rx_rx) = mpsc::channel(16); let (tx_tx, tx_rx) = mpsc::channel(16); @@ -180,7 +238,7 @@ where socket.join_multicast_v4(sd::MULTICAST_IP, interface)?; let fut = Self::socket_loop_future(socket, rx_tx, tx_rx, e2e_registry); - tokio::spawn(fut); + spawner.spawn(fut); Ok(Self { receiver: rx_rx, sender: tx_tx, @@ -190,21 +248,36 @@ where }) } + /// Bind a unicast SOME/IP socket on `port` using the default + /// `crate::tokio_transport::TokioTransport` and + /// `crate::tokio_transport::TokioSpawner` backends (rendered as + /// code literals for the same rustdoc-feature-gating reason + /// described on [`Self::bind_discovery_seeded`]). See + /// [`Self::bind_with_transport`] for the generic variant. + /// + /// Currently `#[cfg(test)]`-gated: production callers reach the + /// socket through the `_with_transport` variant so the `Spawner` + /// trait can be exercised end-to-end. + #[cfg(test)] pub async fn bind(port: u16, e2e_registry: Arc>) -> Result { - Self::bind_with_transport(&TokioTransport, port, e2e_registry).await + use crate::tokio_transport::{TokioSpawner, TokioTransport}; + Self::bind_with_transport(&TokioTransport, &TokioSpawner, port, e2e_registry).await } /// Variant of [`Self::bind`] that constructs the underlying socket - /// through a caller-supplied [`TransportFactory`]. See + /// through a caller-supplied [`TransportFactory`] and submits the + /// socket's I/O loop through a caller-supplied [`Spawner`]. See /// [`Self::bind_discovery_seeded_with_transport`] for the factory /// bound rationale. - pub async fn bind_with_transport( + pub async fn bind_with_transport( factory: &F, + spawner: &S, port: u16, e2e_registry: Arc>, ) -> Result where F: TransportFactory, + S: Spawner, { let (rx_tx, rx_rx) = mpsc::channel(4); let (tx_tx, tx_rx) = mpsc::channel(4); @@ -219,7 +292,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); - tokio::spawn(fut); + spawner.spawn(fut); Ok(Self { receiver: rx_rx, sender: tx_tx, @@ -491,6 +564,7 @@ where mod tests { use super::*; use crate::protocol::sd::test_support::{TestPayload, empty_sd_header}; + use crate::tokio_transport::TokioSpawner; use std::format; use std::vec; // Tests build ad-hoc UDP peers via tokio directly; this is not part of @@ -816,9 +890,10 @@ mod tests { calls: AtomicUsize::new(0), }; - let sm = TestSocketManager::bind_with_transport(&factory, 0, test_registry()) - .await - .expect("bind via custom factory"); + let sm = + TestSocketManager::bind_with_transport(&factory, &TokioSpawner, 0, test_registry()) + .await + .expect("bind via custom factory"); assert_eq!( factory.calls.load(Ordering::SeqCst), 1, @@ -858,6 +933,7 @@ mod tests { let mut sm = SocketManager::::bind_with_transport( &ForceReuseFactory, + &TokioSpawner, 0, test_registry(), ) @@ -916,9 +992,14 @@ mod tests { } } - let err = TestSocketManager::bind_with_transport(&AlwaysBusyFactory, 0, test_registry()) - .await - .expect_err("factory returned Err, bind must surface it"); + let err = TestSocketManager::bind_with_transport( + &AlwaysBusyFactory, + &TokioSpawner, + 0, + test_registry(), + ) + .await + .expect_err("factory returned Err, bind must surface it"); match err { Error::Transport(TransportError::AddressInUse) => {} other => { diff --git a/src/lib.rs b/src/lib.rs index 8a632c3..9a5eec7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -19,8 +19,8 @@ //! | [`protocol`] | Yes | Wire format: headers, messages, message types, return codes, and service discovery (SD) entries/options | //! | [`e2e`] | Yes | End-to-End protection — Profile 4 (CRC-32) and Profile 5 (CRC-16) | //! | [`WireFormat`] / [`PayloadWireFormat`] | Yes | Traits for serializing messages and defining custom payload types | -//! | [`client`] | No | Async tokio client — service discovery, subscriptions, and request/response (feature `client`) | -//! | [`server`] | No | Async tokio server — service offering, event publishing, and subscription management (feature `server`) | +//! | `client` | No | Async tokio client — service discovery, subscriptions, and request/response (feature `client`) | +//! | `server` | No | Async tokio server — service offering, event publishing, and subscription management (feature `server`) | //! //! ## Feature Flags //! @@ -29,7 +29,7 @@ //! | `std` | yes | Enables std-dependent helpers (`RawPayload`, `VecSdHeader`, `OfferedEndpoint`) | //! | `client` | no | Async tokio client; implies `std` + tokio + socket2 + futures | //! | `server` | no | Async tokio server; implies `std` + tokio + socket2 + futures | -//! | `bare_metal` | no | Pure marker feature — enables no crate code. Reserved for future phases to gate `no_std` helper types. To exercise the bare-metal trait surface today, use the `examples/bare_metal` workspace member (`cargo run -p bare_metal`). **Does not make the crate fully bare-metal-complete**: the `client`/`server` feature paths still rely on `tokio::spawn` to drive per-socket I/O loops. A fully tokio-free build additionally requires a user-provided `Spawner` impl, planned as a trait alongside `TransportSocket` and `Timer`. | +//! | `bare_metal` | no | Pure marker feature — enables no crate code. Reserved for future phases to gate no_std helper types. To exercise the bare-metal trait surface today, use the `examples/bare_metal` workspace member (`cargo run -p bare_metal`). **Does not make `client` / `server` bare-metal-usable.** The `Spawner` trait (phase 9) makes task submission pluggable, but the `client` / `server` feature paths still depend on: (1) `tokio::sync::mpsc` channels (heap + tokio-waker-coupled) for intra-module message passing, (2) `tokio::sync::oneshot` for send-acks, (3) `Arc>` for shared registry state (requires `alloc` + `std::sync`), and (4) an `F::Socket = TokioSocket` bound on `SocketManager::bind_*` that needs stable Rust Return-Type Notation to relax. Until all four are resolved, `feature = "client"` / `feature = "server"` remain `std`+tokio-only. `no_alloc` consumers today should build their own orchestrator on `protocol`, `e2e`, and the `transport` traits directly — those layers ARE fully `no_std` / `no_alloc`. | //! //! The default feature set is `["std"]`, which links `std` and enables //! the `RawPayload` / `VecSdHeader` helpers. For a minimal build with @@ -164,8 +164,8 @@ pub use e2e::{E2ECheckStatus, E2EKey, E2EProfile}; #[cfg(feature = "server")] pub use server::Server; #[cfg(any(feature = "client", feature = "server"))] -pub use tokio_transport::{TokioSocket, TokioTimer, TokioTransport}; +pub use tokio_transport::{TokioSocket, TokioSpawner, TokioTimer, TokioTransport}; pub use transport::{ - IoErrorKind, ReceivedDatagram, SocketOptions, Timer, TransportError, TransportFactory, + IoErrorKind, ReceivedDatagram, SocketOptions, Spawner, Timer, TransportError, TransportFactory, TransportSocket, }; diff --git a/src/tokio_transport.rs b/src/tokio_transport.rs index 58c7489..c19fb95 100644 --- a/src/tokio_transport.rs +++ b/src/tokio_transport.rs @@ -85,6 +85,16 @@ impl TokioSocket { #[derive(Debug, Default, Clone, Copy)] pub struct TokioTimer; +/// [`crate::transport::Spawner`] impl that routes submitted futures +/// to `tokio::spawn`. +/// +/// Zero-size unit struct; every `Inner` / `Client` pays nothing for the abstraction. Bare-metal +/// consumers substitute their own `Spawner` via the +/// `crate::Client::new_with_spawner_and_loopback` constructor. +#[derive(Debug, Default, Clone, Copy)] +pub struct TokioSpawner; + impl TransportFactory for TokioTransport { type Socket = TokioSocket; @@ -182,6 +192,17 @@ impl Timer for TokioTimer { } } +impl crate::transport::Spawner for TokioSpawner { + fn spawn(&self, future: impl Future + Send + 'static) { + // Drop the returned `JoinHandle` — per-socket loops run until + // their owning `SocketManager` drops its channel ends, at + // which point the future completes naturally. Callers that + // want cancel-on-abort semantics should spawn at their own + // call site; this trait is intentionally minimal. + drop(tokio::spawn(future)); + } +} + /// Synchronously create and configure a UDP socket via `socket2`, then /// hand it to tokio. Mirrors the existing bind paths in /// [`crate::client::socket_manager`] and [`crate::server`] so behavior is diff --git a/src/transport.rs b/src/transport.rs index 0f45ed0..85da95b 100644 --- a/src/transport.rs +++ b/src/transport.rs @@ -487,6 +487,76 @@ pub trait Timer { fn sleep(&self, duration: Duration) -> impl Future; } +/// Executor-agnostic task-spawning primitive. +/// +/// `simple-someip`'s per-socket I/O loops need to run concurrently with +/// the client's main event loop — otherwise `SocketManager::send`'s +/// internal oneshot wait deadlocks (the send future parks the main +/// loop, which is the only thing that would drive the socket loop to +/// produce its response). Phase 8 hit this and deferred the spawn to +/// a user-provided `Spawner` here, letting std+tokio callers pass a +/// one-line `TokioSpawner` and bare-metal callers wrap their own +/// executor's task-spawning primitive. +/// +/// # Why this reverses the phase-4 "no executor adapter" rule +/// +/// Phase 4 deliberately avoided wrapping spawn to prevent "reinventing +/// embassy" and trait-object dispatch in the hot path. Concrete +/// evidence from phase 8 showed that without a spawn abstraction, +/// `Inner::bind_*` has to call `tokio::spawn` directly — making the +/// whole crate tokio-only. The revised rule: spawn DOES need a trait, +/// but we avoid the phase-4 concerns by (1) keeping the trait generic +/// (monomorphized, no `dyn Spawner`) and (2) scoping it narrowly — +/// just spawn, not select/sleep which have other solutions. +/// +/// # Usage +/// +/// On `std + tokio`, use `crate::tokio_transport::TokioSpawner` +/// (available when the `client` or `server` feature is enabled) — +/// a zero-size unit struct whose `spawn` is a thin wrapper around +/// `tokio::spawn`. The path is rendered as a code literal rather +/// than an intra-doc link because the target module is feature-gated +/// and would break default-feature rustdoc builds. On embedded: +/// +/// ```ignore +/// struct EmbassySpawner(embassy_executor::Spawner); +/// impl simple_someip::Spawner for EmbassySpawner { +/// fn spawn(&self, fut: impl core::future::Future + Send + 'static) { +/// // embassy's Spawner has its own task-registration model; +/// // the adapter layer depends on how the user defined their tasks +/// todo!("call self.0.spawn(...)"); +/// } +/// } +/// ``` +pub trait Spawner { + /// Submit `future` to the executor. Must not block; must arrange + /// for the future to be polled to completion on some task. + /// + /// # Correctness requirement + /// + /// Implementations MUST poll the submitted future. Dropping it + /// without polling — or holding it in a queue that never drains — + /// will deadlock `crate::client::Client` (available when the + /// `client` feature is enabled): `SocketManager::send` + /// `await`s an internal mpsc→oneshot round-trip whose only driver + /// is the per-socket loop future submitted here. No poll, no + /// progress, no oneshot resolution; the caller's `send` hangs + /// forever. + /// + /// The `MockSpawner` in `examples/bare_metal/` deliberately + /// demonstrates the wrong pattern (drops the future) and annotates + /// it as DEMO-ONLY for exactly this reason. + /// + /// # Bound rationale + /// + /// The `Send + 'static` bound matches every mainstream multi-task + /// executor (tokio, async-std, smol, embassy with task arenas). + /// Bare-metal executors that use single-threaded task pools may + /// want to loosen this — a future release may add a + /// `spawn_local`-style variant gated on a cargo feature. + fn spawn(&self, future: impl Future + Send + 'static); +} + #[cfg(test)] mod tests { //! The traits are pure interfaces — these tests only verify that From 81d1deee355a21a06f79c4bddd0c79bfc8e07df9 Mon Sep 17 00:00:00 2001 From: Justin Kovacich Date: Mon, 27 Apr 2026 10:51:16 -0400 Subject: [PATCH 2/2] phase 9: round-N response to consolidated review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review (4 parallel reviewers + 87 Copilot comments triaged to 15 unresolved threads) surfaced 86 ranked items: 1 blocker, 13 high, 19 medium, 25 low, 28 nits. This round addresses everything fixable without an architectural redesign (4 explicitly deferred to phases 10+: ClientParts struct refactor, fully injectable Server::Timer, TTL reaping, and a tokio-feature opt-out path). Highlights, by phase: A. Blocker — bare_metal canary examples/bare_metal/src/main.rs's MockSocket signatures contradicted the phase-4 &self migration (4 x E0053). Fixed all four impl receivers, replaced MockSpawner-that-drops-the-future with a WorkingSpawner that actually polls (the trait contract requires polling, and the canary previously demonstrated the wrong pattern). cargo build -p bare_metal + cargo run -p bare_metal both pass; assert!ed that the spawned future was polled to completion. B. Semantic correctness - Client::reboot_flag now returns Result; QueryRebootFlag carries Result<_, Error>; force_sd_session_wrapped_for_test parallel migration. Matches every other public Client method's Shutdown semantics (regression test added). - SocketManager::send no longer .expect-panics on a dropped response oneshot. Phase-9 user-supplied Spawner made that path reachable. - Pre-encode UDP_BUFFER_SIZE check on send path mirrors the EventPublisher's existing guard; oversize messages now return Error::Capacity(\"udp_buffer\") uniformly. - request_queue overflow notifies senders with Capacity instead of dropping (callers see typed Capacity, not Shutdown via RecvError). - SocketManager recv-error hot loop bounded at 16 consecutive failures. - server::EventPublisher::publish_event returns Err(Error::E2e(_)) on protect failure instead of silently sending the UNPROTECTED payload. - SD Subscribe with mismatched major_version now NACKs. - SdStateManager exposes next_session_id_with_reboot_flag() atomic pair; eliminates a TOCTOU race around the wrap boundary. C. Docs / CHANGELOG / intra-doc links - CHANGELOG [Unreleased] now covers every phase 7-9 breaking change. - README pins simple-someip = \"0.7\"; transport module + bare_metal feature row added. - server/README.md SD diagram corrected (Unicast=true); register_subscriber Result documented. - Spawner trait: JoinHandle policy made explicit (fire-and-forget by design); embassy claim softened. - ReceivedDatagram::truncated and max_datagram_size MTU wording corrected. - All broken intra-doc links demoted to backtick code literals. cargo doc --all-features and --no-default-features both clean. - traits.rs: set_reboot_flag default no-op so std-but-not-client consumers don't have to implement an unused method. D. Spawn-handle hygiene 62 sites converted from bare or 'let _ = tokio::spawn(x)' to 'let _run_handle = tokio::spawn(x)'. Suppresses clippy::let_underscore_future under cargo 1.91. E-G. Drift cleanup, low, nits - Unique service IDs per integration test via static AtomicU16; parallel-execution caveat documented (SO_REUSEPORT routing flake pre-existing on main; --test-threads=1 required). - clippy --fix swept the bulk; manual cleanup for what remained. - publish_raw_event size guard moved BEFORE Header::new_event. - const _: () = assert!(...) for SUBSCRIBERS_PER_GROUP and EVENT_GROUPS_CAP compile-time invariants. - SessionTracker / sd_state docs migrated to typed-flag language. - announcement_loop_emits_first_offer test now aborts spawned handle. - select! cancel-safety contract documented in server::run. - bare_metal block_on doc points at cassette / embassy_executor::block_on. Verification: - cargo check across {all-features, no-default, client-only, server-only, client+server, bare_metal-feature}: all clean. - cargo doc {all-features, no-default-features}: zero warnings. - cargo clippy --workspace --all-features --all-targets -D warnings: zero warnings. - cargo test --lib --all-features: 452/452 pass. - cargo test --test client_server -- --test-threads=1: 11/11 pass. - cargo run -p bare_metal: end-to-end success. - cargo fmt --check: clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 35 ++- README.md | 23 +- examples/bare_metal/src/main.rs | 149 +++++---- examples/client_server/src/main.rs | 2 +- examples/discovery_client/src/main.rs | 2 +- src/client/error.rs | 5 +- src/client/inner.rs | 421 +++++++++++++------------- src/client/mod.rs | 195 ++++++++---- src/client/session.rs | 9 +- src/client/socket_manager.rs | 73 ++++- src/e2e/crc.rs | 14 +- src/e2e/e2e_checker.rs | 26 +- src/e2e/e2e_protector.rs | 14 +- src/e2e/mod.rs | 14 +- src/e2e/registry.rs | 2 +- src/lib.rs | 12 +- src/protocol/byte_order.rs | 4 + src/protocol/header.rs | 34 +-- src/protocol/message_id.rs | 22 +- src/protocol/sd/header.rs | 6 +- src/protocol/sd/options.rs | 4 +- src/server/README.md | 11 +- src/server/event_publisher.rs | 137 ++++++--- src/server/mod.rs | 168 +++++----- src/server/sd_state.rs | 70 +++-- src/server/subscription_manager.rs | 32 +- src/tokio_transport.rs | 40 +-- src/traits.rs | 11 +- src/transport.rs | 90 ++++-- tests/client_server.rs | 227 ++++++++++---- 30 files changed, 1181 insertions(+), 671 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 87dbc73..4349db2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,13 +4,46 @@ ### Added -- **`client::Error::Capacity(&'static str)`** — new variant returned when a fixed-capacity internal structure is full (e.g. `"unicast_sockets"`, `"udp_buffer"`). Because `client::Error` is not `#[non_exhaustive]`, this is a breaking change for downstream crates that match the enum exhaustively. +- **`client::Error::Capacity(&'static str)`** — new variant returned when a fixed-capacity internal structure is full. Current tags: `"unicast_sockets"`, `"udp_buffer"`, `"pending_responses"`, `"request_queue"`. Because `client::Error` is not `#[non_exhaustive]`, this is a breaking change for downstream crates that match the enum exhaustively. +- **`client::Error::Transport(crate::transport::TransportError)`** — new variant surfacing failures from the pluggable transport backend (`#[from]`-converted, displays transparently). Same exhaustive-match caveat as above. +- **`client::Error::Shutdown`** — new variant returned by every `Client` method when the control channel is closed (run-loop future was dropped, cancelled, or exited). Replaces the previous `.unwrap()`-on-closed-channel panic path. - **`server::SubscribeError`** — new public enum (`SubscribersPerGroupFull`, `EventGroupsFull`) returned by `SubscriptionManager::subscribe` and `EventPublisher::register_subscriber` when a bounded capacity rejects a subscription. Re-exported from `server::mod`. +- **`Client::new_with_loopback(interface, multicast_loopback)`** — constructor that exposes the previously-internal `multicast_loopback` knob for same-host integration tests. +- **`Client::new_with_spawner_and_loopback(interface, multicast_loopback, spawner)`** — phase-9 executor-agnostic constructor that accepts a caller-supplied `Spawner` impl. Bare-metal callers swap `TokioSpawner` for their own task pool. +- **`transport::Spawner` trait** (re-exported as `simple_someip::Spawner`) — executor-agnostic task-spawn abstraction. `tokio_transport::TokioSpawner` is the default `std + tokio` impl. +- **`transport::TransportSocket` / `TransportFactory` / `Timer` traits** — executor-agnostic UDP transport abstraction landed in phase 4 and finished out across phases 5–9. Default `tokio_transport::TokioTransport` / `TokioSocket` / `TokioTimer` impls available behind the `client` / `server` features. +- **`bare_metal` cargo feature** — pure marker, reserved for future no_std helpers. The real bare-metal canary is the `examples/bare_metal` workspace member, which depends on `simple-someip` with `default-features = false, features = ["bare_metal"]`. Validate with `cargo build -p bare_metal`, NOT `cargo build --workspace` (workspace builds may unify features and mask regressions). +- **`SubscriptionManager::subscribe` returning a `Result`** — see "Changed" below; the regression test list now exercises the major-version mismatch path explicitly. ### Changed +- **Breaking: `Client::new(interface)` return shape** — previously returned `(Client, ClientUpdates)`; now returns `(Client, ClientUpdates, impl Future + Send + 'static)`. The third element is the run-loop future, which the caller MUST drive (typically via `tokio::spawn`) for any `Client` method to make progress. Migration: change destructuring to a 3-tuple and spawn or otherwise actively poll the future. +- **Breaking: `Server::start_announcing` removed → `Server::announcement_loop`** — the new method returns `Result + Send + 'static, Error>` (annotated `#[must_use]`). Spawn the returned future to fire announcements; calling and dropping the future is a silent no-op. +- **Breaking: `Client::start_sd_announcements` renamed to `Client::sd_announcements_loop`** — same semantic shift as `announcement_loop`: returns an `impl Future` instead of spawning internally, so the caller drives execution. +- **Breaking: `Client::reboot_flag(&self)` now returns `Result`** — previously returned the bare flag and could panic if the run-loop had exited. All other public `Client` methods migrated to the same `Err(Error::Shutdown)` policy in this release; `reboot_flag` is now consistent. - **Breaking: `server::SubscriptionManager::subscribe` signature change** — now returns `Result<(), server::SubscribeError>` instead of `()`. Previously, capacity rejections were silently dropped with only a `warn!` log, which let the server emit a `SubscribeAck` for a subscription that had not been recorded. Callers must now handle the `Err` path (the server's own SD loop emits `SubscribeNack` on `Err`). - **Breaking: `server::EventPublisher::register_subscriber` signature change** — now returns `Result<(), server::SubscribeError>` instead of `()`, surfacing the same capacity-rejection signal to externally managed subscription dispatchers. +- **Breaking: default features changed `default = []` → `default = ["std"]`** — previously `embedded-io/std`, `thiserror/std`, and `tracing/std` were always-on; they are now gated behind the new `std` feature. Downstream consumers building with `default-features = false` who relied on the implicit `std` propagation must add `features = ["std"]` (or one of `client` / `server`, which both imply `std`). +- New optional dependency `dep:futures` (default-features-off) for `futures::select!` + `FusedFuture` plumbing — pulled in transitively by both `client` and `server` features. +- `client::Error::Transport` adopts `#[error(transparent)]` Display delegation (the previous wrapping with `{:?}` debug-formatted the inner `TransportError`); user-facing error strings are now stable. +- Subscribe-NACK reason strings normalized to `snake_case` for log consistency: `wrong_service_id`, `wrong_instance_id`, `wrong_major_version`, `no_endpoint_in_options`, `subscribers_per_group_full`, `event_groups_full`. Wire format is unchanged (NACK is signalled by `TTL=0`). + +### Fixed + +- **`server::EventPublisher::publish_event` no longer silently sends UNPROTECTED payloads on E2E protect failure** — counter exhaustion / key-lookup races etc. now surface as `Err(Error::E2e(_))` rather than logging and falling through (which had been emitting an unprotected message claiming an E2E-protected channel). +- **SD `Subscribe` with mismatched `major_version` is now NACKed** — previously an Ack would be returned and the subscription registered, leaving the application stack to silently mis-decode incompatible-version traffic. +- **`SocketManager::send` no longer panics on a dropped response oneshot** — phase-9 user-supplied `Spawner` made this path reachable; failures now return `Err(Error::SocketClosedUnexpectedly)`. +- **`client::Inner` request-queue overflow no longer drops control messages silently** — full queue now invokes `reject_with_capacity("request_queue")` on the rejected message, so callers see a typed `Err(Error::Capacity("request_queue"))` instead of a `RecvError` mapped to `Error::Shutdown`. +- **Per-socket recv-error hot loop bounded** — `SocketManager`'s socket loop now closes after `MAX_CONSECUTIVE_RECV_ERRORS = 16` consecutive `recv_from` failures rather than spinning indefinitely on a permanently broken fd. +- **`Client::send` fails fast on oversize messages** — pre-encode size check returns `Err(Error::Capacity("udp_buffer"))` for messages whose `required_size()` exceeds `UDP_BUFFER_SIZE`. Mirrors the existing `EventPublisher::publish_event` capacity guard. + +### Notes + +- **Crate version bumped to 0.7.0** — reflects the breaking changes above. Downstream `Cargo.toml` snippets in `README.md` were updated accordingly. + +### Known issues + +- `tests/client_server.rs` integration tests share the SD multicast port (30490) via `SO_REUSEPORT` and rely on Linux's reuseport hashing for traffic delivery. Under cargo's default parallel test runner this produces cross-test Subscribe deliveries that flake ~half the tests. Run with `cargo test --test client_server -- --test-threads=1` until each test can be given its own SD port. The `cargo test --lib` unit-test suite is unaffected. (Pre-existing, called out here so consumers do not assume `cargo test --workspace` is green.) ## [0.6.0](https://github.com/luminartech/simple_someip/compare/v0.5.3...v0.6.0) - 2026-04-20 diff --git a/README.md b/README.md index 00dded6..61979cd 100644 --- a/README.md +++ b/README.md @@ -10,9 +10,10 @@ The library supports both `std` and `no_std` environments, making it suitable fo ## Features -- **`no_std` compatible** — the `protocol`, `traits`, and `e2e` modules work without the standard library +- **`no_std` compatible** — `protocol`, `traits`, `transport`, and `e2e` modules work without the standard library - **Service Discovery** — SD entry/option encoding and decoding via fixed-capacity `heapless` collections (no heap allocation) - **End-to-End protection** — Profile 4 (CRC-32) and Profile 5 (CRC-16) with zero-allocation APIs +- **Executor-agnostic transport traits** — `TransportSocket`, `TransportFactory`, `Timer`, `Spawner` (default `tokio` impls behind feature gates) - **Async client and server** — tokio-based, gated behind optional feature flags - **`embedded-io`** traits for serialization — abstracts over `std::io::Read`/`Write` @@ -20,7 +21,9 @@ The library supports both `std` and `no_std` environments, making it suitable fo - `protocol` — Wire format layer: SOME/IP header, `MessageId`, `MessageType`, `ReturnCode`, SD entries/options - `traits` — `WireFormat` and `PayloadWireFormat` traits for custom message types +- `transport` — Executor-agnostic UDP socket / factory / timer / spawner traits (no_std-compatible) - `e2e` — End-to-End protection profiles (always available, no heap allocation) +- `tokio_transport` — Default `std + tokio` impls of the transport traits (requires `feature = "client"` or `feature = "server"`) - `client` — High-level async tokio client (requires `feature = "client"`) - `server` — Async tokio server with SD announcements and event publishing (requires `feature = "server"`) @@ -31,19 +34,19 @@ Add to your `Cargo.toml`: ```toml [dependencies] # Default — includes std, thiserror, and tracing -simple-someip = "0.5" +simple-someip = "0.7" -# no_std only (protocol/E2E/traits, no heap allocation) -simple-someip = { version = "0.5", default-features = false } +# no_std only (protocol/transport/E2E/traits, no heap allocation) +simple-someip = { version = "0.7", default-features = false } # Client only -simple-someip = { version = "0.5", features = ["client"] } +simple-someip = { version = "0.7", features = ["client"] } # Server only -simple-someip = { version = "0.5", features = ["server"] } +simple-someip = { version = "0.7", features = ["server"] } # Both client and server -simple-someip = { version = "0.5", features = ["client", "server"] } +simple-someip = { version = "0.7", features = ["client", "server"] } ``` ### Feature flags @@ -53,8 +56,9 @@ simple-someip = { version = "0.5", features = ["client", "server"] } | `std` | **yes** | Enables `thiserror`, `tracing`, and `embedded-io/std` | | `client` | no | Async tokio client; implies `std` + tokio + socket2 | | `server` | no | Async tokio server; implies `std` + tokio + socket2 | +| `bare_metal` | no | Pure marker — reserved for future no_std helpers. The real bare-metal canary is the `examples/bare_metal` workspace member; verify it with `cargo build -p bare_metal` (NOT `cargo build --workspace`, which can unify features). | -By default the crate enables `std`. To use in a `no_std` environment (e.g., embedded targets), disable default features with `default-features = false`. In that mode only the `protocol`, `traits`, and `e2e` modules are available, and the crate compiles in `no_std` mode. Most applications only need one of `client` or `server`. +By default the crate enables `std`. To use in a `no_std` environment (e.g., embedded targets), disable default features with `default-features = false`. In that mode the `protocol`, `traits`, `transport`, and `e2e` modules are available; `client` / `server` (and their `tokio_transport` backend) are not. Most applications only need one of `client` or `server`. ## Quick Start @@ -108,7 +112,8 @@ async fn main() -> Result<(), Box> { let publisher = server.publisher(); let run_handle = tokio::spawn(async move { server.run().await }); - // Publish events to subscribers... + // Publish events to subscribers, e.g.: + // publisher.publish_event(0x1234, 1, 0x01, &message).await?; tokio::select! { res = announce_handle => eprintln!("announcement loop exited unexpectedly: {res:?}"), diff --git a/examples/bare_metal/src/main.rs b/examples/bare_metal/src/main.rs index eeb7cdf..1ef8844 100644 --- a/examples/bare_metal/src/main.rs +++ b/examples/bare_metal/src/main.rs @@ -7,15 +7,22 @@ //! hand-rolled mock backend. The `Cargo.toml` in this directory //! depends on `simple-someip` with //! `default-features = false, features = ["bare_metal"]`, so building -//! or running this example proves **that the trait surface compiles -//! under exactly the feature set a firmware consumer would use** — -//! no `std`-feature paths from `simple-someip`, no tokio, no socket2. -//! `cargo build --workspace` catches any regression that breaks this -//! surface even without running the binary. +//! or running this package in isolation proves **that the trait +//! surface compiles under exactly the feature set a firmware consumer +//! would use** — no `std`-feature paths from `simple-someip`, no +//! tokio, no socket2. +//! +//! Use `cargo build -p bare_metal` (or `cargo run -p bare_metal`) as +//! the source of truth for that check; `cargo build --workspace` can +//! unify features across workspace members and may therefore mask +//! regressions in this minimal configuration. CI should run +//! `cargo build -p bare_metal` (and `cargo clippy -p bare_metal`) as a +//! dedicated step. //! //! # How to run //! //! ```text +//! cargo build -p bare_metal //! cargo run -p bare_metal //! ``` //! @@ -85,6 +92,7 @@ use core::future::Future; use core::net::{Ipv4Addr, SocketAddrV4}; +use core::pin::Pin; use core::task::{Context, Poll, Waker}; use core::time::Duration; @@ -135,7 +143,7 @@ impl TransportFactory for MockFactory { impl TransportSocket for MockSocket { fn send_to( - &mut self, + &self, buf: &[u8], target: SocketAddrV4, ) -> impl Future> { @@ -148,25 +156,21 @@ impl TransportSocket for MockSocket { } fn recv_from( - &mut self, + &self, buf: &mut [u8], ) -> impl Future> { - let pipe = Arc::clone(&self.pipe); - // Copy directly into `buf` by stealing its slice lifetime out - // of the async block via a raw-pointer round-trip would be - // unsafe; instead, poll the queue on first call and fill buf - // synchronously if a datagram is ready. If the queue is empty, - // this mock returns a ready - // `Err(TransportError::Io(IoErrorKind::TimedOut))` rather than - // a pending future. In this single-threaded example we always - // send first then recv, so the timeout branch is unreachable - // here. - // - // The mock borrow-dance is awkward compared to a real UDP - // socket's recv_from; a production bare-metal impl would copy - // bytes out of its driver's receive slab directly into `buf`. + // Read synchronously before the async block so we don't have to + // capture `buf` across the `.await` boundary. If the queue is + // empty, return a ready `Err(TimedOut)` rather than a pending + // future. A production bare-metal impl would instead register + // the `Context`'s `Waker` on the network driver's RX-ready + // signal and return `Poll::Pending` so the executor can park + // the task — see e.g. `embassy_net::UdpSocket` or smoltcp's + // socket polling model. In this single-threaded example we + // always send first then recv, so the timeout branch is + // unreachable here. let result = { - let mut q = pipe.recv_queue.lock().unwrap(); + let mut q = self.pipe.recv_queue.lock().unwrap(); q.pop_front() }; match result { @@ -187,21 +191,13 @@ impl TransportSocket for MockSocket { Ok(self.local_addr) } - fn join_multicast_v4( - &mut self, - _group: Ipv4Addr, - _iface: Ipv4Addr, - ) -> Result<(), TransportError> { + fn join_multicast_v4(&self, _group: Ipv4Addr, _iface: Ipv4Addr) -> Result<(), TransportError> { // Bare-metal stacks without multicast would return // Unsupported; our mock is happy to no-op. Ok(()) } - fn leave_multicast_v4( - &mut self, - _group: Ipv4Addr, - _iface: Ipv4Addr, - ) -> Result<(), TransportError> { + fn leave_multicast_v4(&self, _group: Ipv4Addr, _iface: Ipv4Addr) -> Result<(), TransportError> { Ok(()) } } @@ -228,18 +224,47 @@ impl Timer for MockTimer { } } -/// Phase 9 `Spawner` impl. A real bare-metal `Spawner` wraps the -/// executor's task-submission primitive — `embassy_executor::Spawner`, -/// smoltcp's task pool, or a hand-rolled single-core polling loop. -/// This mock drops every future it receives (equivalent to "never run -/// it"), which is fine for the demo because nothing in the trait-layer -/// round-trip below actually requires a spawned task. A production -/// impl must poll the future to completion. -struct MockSpawner; +/// Phase 9 `Spawner` impl that demonstrates the *correct* contract: +/// every submitted future is queued and later polled to completion. +/// +/// Why a working impl rather than a one-line "drop the future" mock: +/// the `Spawner` trait's docstring explicitly forbids dropping the +/// future without polling, because `Client::send`'s internal oneshot +/// round-trip needs the per-socket loop to make progress. A canary +/// that violates the contract isn't validating the contract. +/// +/// A real bare-metal `Spawner` wraps the executor's task-submission +/// primitive — `embassy_executor::Spawner`, smoltcp's task pool, or a +/// hand-rolled single-core polling loop. Here we keep submissions in +/// an in-memory queue and the demo's `main()` drains it at the end via +/// [`WorkingSpawner::drain`]. That mirrors the shape of a single-core +/// cooperative executor closely enough to prove the trait surface +/// works. +struct WorkingSpawner { + queue: Mutex + Send>>>>, +} + +impl WorkingSpawner { + fn new() -> Self { + Self { + queue: Mutex::new(Vec::new()), + } + } + + /// Block-on every queued future to completion, in submission order. + /// A real cooperative executor would interleave polls; the demo's + /// futures resolve on the first poll so order doesn't matter. + fn drain(&self) { + let queued = std::mem::take(&mut *self.queue.lock().unwrap()); + for fut in queued { + block_on(fut); + } + } +} -impl simple_someip::transport::Spawner for MockSpawner { - fn spawn(&self, _future: impl Future + Send + 'static) { - // DEMO-ONLY: real impls submit `_future` to their task pool. +impl simple_someip::transport::Spawner for WorkingSpawner { + fn spawn(&self, future: impl Future + Send + 'static) { + self.queue.lock().unwrap().push(Box::pin(future)); } } @@ -248,14 +273,19 @@ impl simple_someip::transport::Spawner for MockSpawner { /// **ANTI-PATTERN — DO NOT USE IN PRODUCTION.** `Waker::noop()` means /// no wake-up signal is ever registered; a future that yields /// `Pending` waiting on real I/O would never get polled again. The -/// loop-and-`spin_loop()` fallback here masks that by busy-spinning, -/// which is worse than useless on bare metal. Production executors -/// use proper `Waker` plumbing + a task queue driven by hardware -/// interrupts. This helper exists only to drive the demo's +/// loop-and-`spin_loop()` fallback masks that by busy-spinning, which +/// is worse than useless on bare metal. Production executors use +/// proper `Waker` plumbing + a task queue driven by hardware +/// interrupts; this helper exists only to drive the demo's /// synchronous mock futures (which resolve on the first poll). +/// +/// For a real no_alloc `block_on`, see e.g. `embassy_executor::block_on`, +/// the `cassette` crate, or roll your own around a hardware-timer-driven +/// `Waker`. The `Future::poll` loop body below is the part that stays +/// the same; only the `Waker` plumbing and yield strategy change. fn block_on(fut: F) -> F::Output { let waker = Waker::noop(); - let mut cx = Context::from_waker(&waker); + let mut cx = Context::from_waker(waker); let mut fut = Box::pin(fut); loop { match fut.as_mut().poll(&mut cx) { @@ -287,8 +317,8 @@ fn main() { }; let options = SocketOptions::new(); - let mut sock_a = block_on(factory_a.bind(factory_a.local_addr, &options)).expect("bind A"); - let mut sock_b = block_on(factory_b.bind(factory_b.local_addr, &options)).expect("bind B"); + let sock_a = block_on(factory_a.bind(factory_a.local_addr, &options)).expect("bind A"); + let sock_b = block_on(factory_b.bind(factory_b.local_addr, &options)).expect("bind B"); let payload = b"hello bare-metal"; block_on(sock_a.send_to(payload, sock_b.local_addr().unwrap())).expect("send_to"); @@ -321,10 +351,21 @@ fn main() { let timer = MockTimer; block_on(timer.sleep(Duration::from_millis(1))); - // Demonstrate the Spawner trait compiles against a MockSpawner. - // (The mock drops the future — a real spawner polls it.) - let spawner = MockSpawner; - simple_someip::transport::Spawner::spawn(&spawner, async {}); + // Demonstrate the Spawner trait by submitting a future and then + // draining the queue (proving the future was actually polled). A + // real bare-metal Spawner would dispatch into its executor's task + // pool and the executor would drain it on its own schedule. + let spawner = WorkingSpawner::new(); + let polled = Arc::new(Mutex::new(false)); + let polled_for_task = Arc::clone(&polled); + simple_someip::transport::Spawner::spawn(&spawner, async move { + *polled_for_task.lock().unwrap() = true; + }); + spawner.drain(); + assert!( + *polled.lock().unwrap(), + "WorkingSpawner must poll submitted futures to completion (Spawner trait contract)", + ); println!( "bare-metal example: sent {} bytes from {} to {}, received cleanly.", diff --git a/examples/client_server/src/main.rs b/examples/client_server/src/main.rs index f97c706..d715d3c 100644 --- a/examples/client_server/src/main.rs +++ b/examples/client_server/src/main.rs @@ -107,7 +107,7 @@ async fn main() -> Result<(), Box> { // ── Create the client (handles discovery, subscriptions, SD socket) ── let (client, mut updates, run_fut) = simple_someip::Client::::new(interface); - tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); client.bind_discovery().await?; info!("Client discovery bound"); diff --git a/examples/discovery_client/src/main.rs b/examples/discovery_client/src/main.rs index 0500536..3f17152 100644 --- a/examples/discovery_client/src/main.rs +++ b/examples/discovery_client/src/main.rs @@ -288,7 +288,7 @@ async fn main() -> Result<(), Error> { info!("Starting discovery client on interface {interface}"); let (client, mut updates, run_fut) = simple_someip::Client::::new(interface); - tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); client.bind_discovery().await.unwrap(); let mut state = DiscoveryState::new(); diff --git a/src/client/error.rs b/src/client/error.rs index 8746c4b..32d94f9 100644 --- a/src/client/error.rs +++ b/src/client/error.rs @@ -7,7 +7,7 @@ use thiserror::Error; /// This enum is **not** marked `#[non_exhaustive]`, so downstream crates /// may currently match it exhaustively. That convenience comes with a /// real cost: **any new variant added here is a breaking change** and -/// must be flagged in the changelog and reflected in the next SemVer +/// must be flagged in the changelog and reflected in the next `SemVer` /// bump (pre-1.0, a minor bump is sufficient, but it still requires a /// release-notes entry). The same is true of renaming or restructuring /// existing variants. @@ -46,6 +46,9 @@ pub enum Error { /// - `"unicast_sockets"` → `UNICAST_SOCKETS_CAP` /// - `"udp_buffer"` → `crate::UDP_BUFFER_SIZE` /// - `"pending_responses"` → `PENDING_RESPONSES_CAP` + /// - `"request_queue"` → `REQUEST_QUEUE_CAP` (returned when the + /// client's internal control-message queue is saturated, surfacing + /// on every public `Client` method that enqueues a control) #[error("internal capacity exceeded: {0}")] Capacity(&'static str), /// An error surfaced by the pluggable transport backend (see diff --git a/src/client/inner.rs b/src/client/inner.rs index 96832ca..4234abb 100644 --- a/src/client/inner.rs +++ b/src/client/inner.rs @@ -78,13 +78,13 @@ pub(super) enum ControlMessage { client_port: u16, response: oneshot::Sender>, }, - QueryRebootFlag(oneshot::Sender), + QueryRebootFlag(oneshot::Sender>), /// Test-only: force `sd_session_has_wrapped` to simulate the state a /// long-running client reaches after its SD session counter wraps past /// `0xFFFF`, without actually sending 65k SD messages. Fires the /// accompanying oneshot once the mutation is applied. #[cfg(test)] - ForceSdSessionWrappedForTest(bool, oneshot::Sender<()>), + ForceSdSessionWrappedForTest(bool, oneshot::Sender>), } impl std::fmt::Debug for ControlMessage

{ @@ -233,13 +233,18 @@ impl ControlMessage

{ ) } - pub fn query_reboot_flag() -> (oneshot::Receiver, Self) { + pub fn query_reboot_flag() -> ( + oneshot::Receiver>, + Self, + ) { let (sender, receiver) = oneshot::channel(); (receiver, Self::QueryRebootFlag(sender)) } #[cfg(test)] - pub fn force_sd_session_wrapped_for_test(wrapped: bool) -> (oneshot::Receiver<()>, Self) { + pub fn force_sd_session_wrapped_for_test( + wrapped: bool, + ) -> (oneshot::Receiver>, Self) { let (sender, receiver) = oneshot::channel(); ( receiver, @@ -274,17 +279,12 @@ impl ControlMessage

{ let _ = send_complete.send(Err(Error::Capacity(structure_name))); let _ = response.send(Err(Error::Capacity(structure_name))); } - // QueryRebootFlag and ForceSdSessionWrappedForTest carry - // non-Result oneshot payloads, so there is no Err variant to - // deliver — drop the sender, which surfaces as RecvError on - // the awaiting side. These are internal/test paths, not the - // public APIs whose unwrap-on-RecvError would panic callers. - Self::QueryRebootFlag(_) => { - let _ = structure_name; + Self::QueryRebootFlag(response) => { + let _ = response.send(Err(Error::Capacity(structure_name))); } #[cfg(test)] - Self::ForceSdSessionWrappedForTest(_, _) => { - let _ = structure_name; + Self::ForceSdSessionWrappedForTest(_, response) => { + let _ = response.send(Err(Error::Capacity(structure_name))); } } } @@ -494,7 +494,13 @@ where match self.pending_responses.insert(request_id, response) { Ok(None) => {} Ok(Some(displaced_response)) => { - warn!( + // `request_id` reuse is expected once `session_counter` + // wraps every ~65k requests on a long-lived client, and + // legitimate when the previous request is still pending. + // The displaced sender carries `Error::Capacity` to its + // awaiter; logging at `warn!` per wrap floods ops dashboards + // for a routine event, so demote to `debug!`. + debug!( "pending_responses already contained request_id \ 0x{:08X}; replacing existing pending response", request_id @@ -797,7 +803,7 @@ where #[cfg(test)] ControlMessage::ForceSdSessionWrappedForTest(wrapped, response) => { self.sd_session_has_wrapped = wrapped; - let _ = response.send(()); + let _ = response.send(Ok(())); } ControlMessage::QueryRebootFlag(response) => { // Prefer the live socket's tracked flag when bound. When @@ -810,11 +816,13 @@ where // next `reboot_flag()` call. let flag = if let Some(socket) = self.discovery_socket.as_ref() { socket.reboot_flag() + } else if self.sd_session_has_wrapped { + crate::protocol::sd::RebootFlag::Continuous } else { - crate::protocol::sd::RebootFlag::from(!self.sd_session_has_wrapped) + crate::protocol::sd::RebootFlag::RecentlyRebooted }; - if response.send(flag).is_err() { - warn!("QueryRebootFlag response receiver dropped (caller canceled)"); + if response.send(Ok(flag)).is_err() { + debug!("QueryRebootFlag: caller dropped the response receiver"); } } ControlMessage::Subscribe { @@ -924,186 +932,187 @@ where } #[allow(clippy::too_many_lines)] - fn run_future(mut self) -> impl core::future::Future + Send + 'static { - async move { - info!("SOME/IP Client processing loop started"); - loop { - // Scope the `&mut self` destructure + pinned per-iteration - // futures so all borrows of `self` drop before we call - // `self.handle_control_message().await` below. `pin_mut!` - // creates stack-pinned locals that outlive the select - // macro, so the inner block is required to release those - // borrows. - let should_break = { - let Self { - control_receiver, - pending_responses, - discovery_socket, - unicast_sockets, - update_sender, - request_queue, - session_tracker, - service_registry, - run, - .. - } = &mut self; - // Build fresh per-iteration futures and fuse them for - // `select!`'s `FusedFuture + Unpin` bound. - // `receive_discovery` / `receive_any_unicast` are - // async fns that are not `Unpin`; the `Timer::sleep` - // future likewise. Stack-pinning via `pin_mut!` - // satisfies both. - // - // The 125ms idle tick goes through the `Timer` trait - // rather than `tokio::time::sleep` directly so a - // bare-metal swap to `embassy_time` (or any other - // `Timer` impl) is a one-line change here. Today it - // resolves to `TokioTimer`. - let control_fut = control_receiver.recv().fuse(); - let sleep_fut = TokioTimer - .sleep(std::time::Duration::from_millis(125)) - .fuse(); - let discovery_fut = Self::receive_discovery(discovery_socket).fuse(); - let unicast_fut = Self::receive_any_unicast(unicast_sockets).fuse(); - pin_mut!(control_fut, sleep_fut, discovery_fut, unicast_fut); - - // `select!` (not `select_biased!`) randomizes the - // arm check order each poll so no single arm can - // starve the others under sustained load. Matches - // the original `tokio::select!` fairness behavior. - select! { - // Receive a control message - ctrl = control_fut => { - if let Some(ctrl) = ctrl { - debug!("Received control message: {:?}", ctrl); - if request_queue.push_back(ctrl).is_err() { - // Queue full: the rejected ControlMessage is - // dropped, so any oneshot senders inside it - // cancel — callers awaiting those receivers - // will observe `RecvError`. - warn!( - "request_queue at capacity ({}); dropping control message", - REQUEST_QUEUE_CAP - ); - } - } else { - // The sender has been dropped, so we should exit - *run = false; + async fn run_future(mut self) { + info!("SOME/IP Client processing loop started"); + loop { + // Scope the `&mut self` destructure + pinned per-iteration + // futures so all borrows of `self` drop before we call + // `self.handle_control_message().await` below. `pin_mut!` + // creates stack-pinned locals that outlive the select + // macro, so the inner block is required to release those + // borrows. + let should_break = { + let Self { + control_receiver, + pending_responses, + discovery_socket, + unicast_sockets, + update_sender, + request_queue, + session_tracker, + service_registry, + run, + .. + } = &mut self; + // Build fresh per-iteration futures and fuse them for + // `select!`'s `FusedFuture + Unpin` bound. + // `receive_discovery` / `receive_any_unicast` are + // async fns that are not `Unpin`; the `Timer::sleep` + // future likewise. Stack-pinning via `pin_mut!` + // satisfies both. + // + // The 125ms idle tick goes through the `Timer` trait + // rather than `tokio::time::sleep` directly so a + // bare-metal swap to `embassy_time` (or any other + // `Timer` impl) is a one-line change here. Today it + // resolves to `TokioTimer`. + let control_fut = control_receiver.recv().fuse(); + let sleep_fut = TokioTimer + .sleep(std::time::Duration::from_millis(125)) + .fuse(); + let discovery_fut = Self::receive_discovery(discovery_socket).fuse(); + let unicast_fut = Self::receive_any_unicast(unicast_sockets).fuse(); + pin_mut!(control_fut, sleep_fut, discovery_fut, unicast_fut); + + // `select!` (not `select_biased!`) randomizes the + // arm check order each poll so no single arm can + // starve the others under sustained load. Matches + // the original `tokio::select!` fairness behavior. + select! { + // Receive a control message + ctrl = control_fut => { + if let Some(ctrl) = ctrl { + debug!("Received control message: {:?}", ctrl); + if let Err(rejected) = request_queue.push_back(ctrl) { + // Queue full: notify the rejected message's + // oneshot senders with `Error::Capacity` so + // callers see a typed overload error rather + // than a `RecvError` (which `client::mod` + // maps to `Error::Shutdown`, conflating + // overload with lifecycle failure). + warn!( + "request_queue at capacity ({}); rejecting control message with Capacity error", + REQUEST_QUEUE_CAP + ); + rejected.reject_with_capacity("request_queue"); } + } else { + // The sender has been dropped, so we should exit + *run = false; } - () = sleep_fut => {} - // Receive a discovery message - discovery = discovery_fut => { - trace!("Received discovery message: {:?}", discovery); - match discovery { - Ok((source, someip_header, sd_header)) => { - // Extract session ID from SOME/IP request_id (lower 16 bits) - let session_id = (someip_header.request_id() & 0xFFFF) as u16; - let sd_payload = PayloadDefinitions::new_sd_payload(&sd_header); - // Extract reboot flag from the SD payload flags - let reboot_flag = sd_payload - .sd_flags() - .map_or(crate::protocol::sd::RebootFlag::Continuous, |f| { - f.reboot() - }); - - // Track sender session/reboot state for every SD entry - // that identifies a service instance, not only - // offer/stop-offer entries. This ensures reboot - // detection works for all SD traffic (FindService, - // Subscribe, SubscribeAck, etc.). - let mut rebooted = false; - for (svc_id, inst_id) in sd_payload.service_instances() { - let verdict = session_tracker.check( - source, - TransportKind::Multicast, - svc_id, - inst_id, - session_id, - reboot_flag, - ); - if verdict == SessionVerdict::Reboot { - rebooted = true; - } + } + () = sleep_fut => {} + // Receive a discovery message + discovery = discovery_fut => { + trace!("Received discovery message: {:?}", discovery); + match discovery { + Ok((source, someip_header, sd_header)) => { + // Extract session ID from SOME/IP request_id (lower 16 bits) + let session_id = (someip_header.request_id() & 0xFFFF) as u16; + let sd_payload = PayloadDefinitions::new_sd_payload(&sd_header); + // Extract reboot flag from the SD payload flags + let reboot_flag = sd_payload + .sd_flags() + .map_or(crate::protocol::sd::RebootFlag::Continuous, |f| { + f.reboot() + }); + + // Track sender session/reboot state for every SD entry + // that identifies a service instance, not only + // offer/stop-offer entries. This ensures reboot + // detection works for all SD traffic (FindService, + // Subscribe, SubscribeAck, etc.). + let mut rebooted = false; + for (svc_id, inst_id) in sd_payload.service_instances() { + let verdict = session_tracker.check( + source, + TransportKind::Multicast, + svc_id, + inst_id, + session_id, + reboot_flag, + ); + if verdict == SessionVerdict::Reboot { + rebooted = true; } + } - // Auto-populate service registry from offer/stop-offer - // SD entries. - for ep in sd_payload.offered_endpoints() { - let id = ServiceInstanceId { - service_id: ep.service_id, - instance_id: ep.instance_id, - }; - if ep.is_offer { - if let Some(addr) = ep.addr { - service_registry.insert( - id, - ServiceEndpointInfo { - addr, - local_port: 0, - major_version: ep.major_version, - minor_version: ep.minor_version, - }, - ); - trace!( - "Registry: added 0x{:04X}.0x{:04X} -> {}", - ep.service_id, ep.instance_id, addr, - ); - } - } else { - service_registry.remove(id); + // Auto-populate service registry from offer/stop-offer + // SD entries. + for ep in sd_payload.offered_endpoints() { + let id = ServiceInstanceId { + service_id: ep.service_id, + instance_id: ep.instance_id, + }; + if ep.is_offer { + if let Some(addr) = ep.addr { + service_registry.insert( + id, + ServiceEndpointInfo { + addr, + local_port: 0, + major_version: ep.major_version, + minor_version: ep.minor_version, + }, + ); trace!( - "Registry: removed 0x{:04X}.0x{:04X}", - ep.service_id, ep.instance_id, + "Registry: added 0x{:04X}.0x{:04X} -> {}", + ep.service_id, ep.instance_id, addr, ); } + } else { + service_registry.remove(id); + trace!( + "Registry: removed 0x{:04X}.0x{:04X}", + ep.service_id, ep.instance_id, + ); } - - if rebooted { - let _ = update_sender.send(ClientUpdate::SenderRebooted(source)); - } - - let discovery_msg = DiscoveryMessage { - source, - someip_header, - sd_header, - }; - let _ = update_sender.send(ClientUpdate::DiscoveryUpdated(discovery_msg)); } - Err(err) => { - error!("Error receiving discovery message: {:?}", err); - let _ = update_sender.send(ClientUpdate::Error(err)); + + if rebooted { + let _ = update_sender.send(ClientUpdate::SenderRebooted(source)); } + + let discovery_msg = DiscoveryMessage { + source, + someip_header, + sd_header, + }; + let _ = update_sender.send(ClientUpdate::DiscoveryUpdated(discovery_msg)); } - } - unicast = unicast_fut => { - trace!("Received unicast message: {:?}", unicast); - match unicast { - Ok(received) => { - let ReceivedMessage { message: received_message, e2e_status, .. } = received; - // Check if this matches a pending request-response by request_id - let request_id = received_message.header().request_id(); - if let Some(sender) = pending_responses.remove(&request_id) { - let _ = sender.send(Ok(received_message.payload().clone())); - continue; - } - // Not a response — forward as ClientUpdate::Unicast - let _ = update_sender.send(ClientUpdate::Unicast { message: received_message, e2e_status }); - } - Err(err) => { - let _ = update_sender.send(ClientUpdate::Error(err)); + Err(err) => { + error!("Error receiving discovery message: {:?}", err); + let _ = update_sender.send(ClientUpdate::Error(err)); + } + } + } + unicast = unicast_fut => { + trace!("Received unicast message: {:?}", unicast); + match unicast { + Ok(received) => { + let ReceivedMessage { message: received_message, e2e_status, .. } = received; + // Check if this matches a pending request-response by request_id + let request_id = received_message.header().request_id(); + if let Some(sender) = pending_responses.remove(&request_id) { + let _ = sender.send(Ok(received_message.payload().clone())); + continue; } + // Not a response — forward as ClientUpdate::Unicast + let _ = update_sender.send(ClientUpdate::Unicast { message: received_message, e2e_status }); + } + Err(err) => { + let _ = update_sender.send(ClientUpdate::Error(err)); } } - } - !*run - }; - if should_break { - info!("SOME/IP Client processing loop exiting"); - break; + } } - self.handle_control_message().await; + !*run + }; + if should_break { + info!("SOME/IP Client processing loop exiting"); + break; } + self.handle_control_message().await; } } } @@ -1526,7 +1535,7 @@ mod tests { false, TokioSpawner, ); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); // Drop control sender to trigger loop exit drop(control_sender); // The update receiver should eventually return None when the inner loop exits @@ -1562,7 +1571,7 @@ mod tests { false, TokioSpawner, ); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let (rx, msg) = TestControl::bind_discovery(); drop(rx); @@ -1580,7 +1589,7 @@ mod tests { false, TokioSpawner, ); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let (rx, msg) = TestControl::unbind_discovery(); drop(rx); @@ -1598,7 +1607,7 @@ mod tests { false, TokioSpawner, ); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); // SetInterface(LOCALHOST) on a fresh inner goes straight to // bind_discovery + send response (interface already matches). @@ -1618,7 +1627,7 @@ mod tests { false, TokioSpawner, ); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); // Bind discovery first so the SendSD path has a socket to use let (rx, msg) = TestControl::bind_discovery(); @@ -1649,7 +1658,7 @@ mod tests { false, TokioSpawner, ); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); // Bind discovery so SetInterface will take the multi-step path: // iteration 1: unbind discovery, re-queue SetInterface @@ -1690,14 +1699,14 @@ mod tests { #[test] fn test_send_to_service_constructor_returns_two_receivers() { let message = Message::::new_sd(1, &empty_sd_header()); - let (send_rx, resp_rx, _msg) = TestControl::send_to_service(0x1234, 0x0001, message); + let (send_rx, resp_rx, msg) = TestControl::send_to_service(0x1234, 0x0001, message); // Extract the senders from the control message if let ControlMessage::SendToService { send_complete, response, .. - } = _msg + } = msg { // Both channels are independent — sending on one doesn't affect the other send_complete.send(Ok(())).unwrap(); @@ -1721,7 +1730,7 @@ mod tests { false, TokioSpawner, ); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let addr = SocketAddrV4::new(Ipv4Addr::LOCALHOST, 5000); let (rx, msg) = TestControl::add_endpoint(0x1234, 0x0001, addr, 0); @@ -1740,7 +1749,7 @@ mod tests { false, TokioSpawner, ); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let (rx, msg) = TestControl::remove_endpoint(0x1234, 0x0001); drop(rx); @@ -1758,7 +1767,7 @@ mod tests { false, TokioSpawner, ); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); // Add an endpoint first so SendToService doesn't fail with ServiceNotFound let addr = SocketAddrV4::new(Ipv4Addr::LOCALHOST, 5000); @@ -1786,7 +1795,7 @@ mod tests { true, TokioSpawner, ); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let (rx, msg) = TestControl::bind_discovery(); control_sender.send(msg).await.unwrap(); @@ -1802,7 +1811,7 @@ mod tests { false, TokioSpawner, ); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let (rx, msg) = TestControl::bind_discovery(); control_sender.send(msg).await.unwrap(); @@ -1823,7 +1832,7 @@ mod tests { false, TokioSpawner, ); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let target = SocketAddrV4::new(Ipv4Addr::LOCALHOST, 30490); let sd_header = empty_sd_header(); @@ -1845,7 +1854,7 @@ mod tests { false, TokioSpawner, ); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let addr = SocketAddrV4::new(Ipv4Addr::LOCALHOST, 5000); let (rx, msg) = TestControl::add_endpoint(0x1234, 0x0001, addr, 0); @@ -1871,7 +1880,7 @@ mod tests { false, TokioSpawner, ); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); // Bind discovery first let (rx, msg) = TestControl::bind_discovery(); @@ -1903,7 +1912,7 @@ mod tests { false, TokioSpawner, ); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); // Add endpoint but do NOT bind discovery let addr = SocketAddrV4::new(Ipv4Addr::LOCALHOST, 5000); @@ -1929,7 +1938,7 @@ mod tests { false, TokioSpawner, ); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let (rx, msg) = TestControl::subscribe(0xFFFF, 0xFFFF, 1, 3, 0x01, 0); control_sender.send(msg).await.unwrap(); @@ -1949,7 +1958,7 @@ mod tests { false, TokioSpawner, ); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let addr = SocketAddrV4::new(Ipv4Addr::LOCALHOST, 5000); let (rx, msg) = TestControl::add_endpoint(0x1234, 0x0001, addr, 0); @@ -1985,7 +1994,7 @@ mod tests { false, TokioSpawner, ); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let (rx, msg) = TestControl::subscribe(0x1234, 0x0001, 1, 3, 0x01, 0); drop(rx); @@ -2004,7 +2013,7 @@ mod tests { false, TokioSpawner, ); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); // Change to a different loopback-range address (127.0.0.2). // Binding discovery on 127.0.0.2 should succeed on most systems. @@ -2030,7 +2039,7 @@ mod tests { false, TokioSpawner, ); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); // Bind discovery on LOCALHOST first let (rx, msg) = TestControl::bind_discovery(); @@ -2062,7 +2071,7 @@ mod tests { false, TokioSpawner, ); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); // Add endpoint and bind discovery let (rx, msg) = TestControl::bind_discovery(); @@ -2110,7 +2119,7 @@ mod tests { false, TokioSpawner, ); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let raw = UdpSocket::bind("127.0.0.1:0").await.unwrap(); let target = SocketAddrV4::new(Ipv4Addr::LOCALHOST, raw.local_addr().unwrap().port()); diff --git a/src/client/mod.rs b/src/client/mod.rs index 46e5bc7..15453fe 100644 --- a/src/client/mod.rs +++ b/src/client/mod.rs @@ -276,7 +276,7 @@ where /// false, /// MySpawner, /// ); - /// tokio::spawn(run); + /// let _run_task = tokio::spawn(run); /// # let _ = (client, updates); /// # } /// ``` @@ -427,25 +427,30 @@ where /// Like [`subscribe`](Self::subscribe) but does not wait for the /// subscription result. /// + /// Returns `()`: if the run-loop has exited the request is silently + /// lost — there is no error surface and no panic. Use + /// [`subscribe`](Self::subscribe) when you need to detect dispatch + /// failures. + /// /// This still awaits enqueueing the control message on the internal - /// channel, so it may block if that bounded channel is full. Useful for - /// periodic renewals where waiting for subscription processing is + /// channel, so it may block if that bounded channel is full. Useful + /// for periodic renewals where waiting for subscription processing is /// unnecessary. /// /// The response oneshot is simply dropped at the end of this call. /// The inner loop's send-to-dropped-receiver path is not logged at - /// `warn!`; at most it is logged at `debug!`, so fire-and-forget usage - /// remains low-noise. + /// `warn!`; at most it is logged at `debug!`, so fire-and-forget + /// usage remains low-noise. /// /// # Silent drop on a closed channel /// - /// Unlike the other `Client` methods (which `.unwrap()` the `send` - /// result and therefore panic if the run-loop has exited and closed - /// the receiver), `subscribe_no_wait` deliberately discards the - /// `send` result. If the client run-loop has exited, the request is - /// silently dropped — there is no error surface and no panic. This - /// matches the fire-and-forget contract: callers that need to know - /// whether the subscription was actually dispatched should use + /// Unlike the other `Client` methods (which return + /// `Err(Error::Shutdown)` if the run-loop has exited and closed the + /// receiver), `subscribe_no_wait` deliberately discards the `send` + /// result. If the run-loop has exited, the request is silently + /// dropped — no error surface, no panic. This matches the + /// fire-and-forget contract: callers that need to know whether the + /// subscription was actually dispatched should use /// [`subscribe`](Self::subscribe) instead. pub async fn subscribe_no_wait( &self, @@ -491,22 +496,39 @@ where /// Headers passed to [`sd_announcements_loop`](Self::sd_announcements_loop) /// are refreshed automatically per-tick and do not need this call. /// - /// # Panics + /// # Errors + /// + /// Returns [`Error::Shutdown`] if the client's run-loop future has + /// exited before this call (dropped, cancelled, or otherwise gone) + /// — the `Client` handle has outlived its driver and further + /// control-channel sends cannot make progress. /// - /// Panics if the internal control channel is closed. - pub async fn reboot_flag(&self) -> protocol::sd::RebootFlag { + /// Returns [`Error::Capacity`] (with tag `"request_queue"`) if the + /// run loop's bounded control queue is saturated under load. + pub async fn reboot_flag(&self) -> Result { let (response, message) = ControlMessage::query_reboot_flag(); - self.control_sender.send(message).await.unwrap(); - response.await.unwrap() + self.control_sender + .send(message) + .await + .map_err(|_| Error::Shutdown)?; + response.await.map_err(|_| Error::Shutdown)? } /// Test-only: force the inner loop's `sd_session_has_wrapped` so tests /// can observe post-wrap behavior without sending 65k SD messages. + /// Mirrors the public `Client` API: returns `Err(Error::Shutdown)` on + /// closed channels rather than panicking. #[cfg(test)] - pub(crate) async fn force_sd_session_wrapped_for_test(&self, wrapped: bool) { + pub(crate) async fn force_sd_session_wrapped_for_test( + &self, + wrapped: bool, + ) -> Result<(), Error> { let (response, message) = ControlMessage::force_sd_session_wrapped_for_test(wrapped); - self.control_sender.send(message).await.unwrap(); - response.await.unwrap(); + self.control_sender + .send(message) + .await + .map_err(|_| Error::Shutdown)?; + response.await.map_err(|_| Error::Shutdown)? } /// Sends an SD message to a specific target address. @@ -618,9 +640,20 @@ where // so long-running announcers transition from RecentlyRebooted // to Continuous once the session counter wraps. The weak // sender is upgraded, used to enqueue a single control - // message, then dropped before we await — keeping the strong - // sender alive across awaits would defeat the weak-sender - // shutdown path. + // message, then dropped before we await — keeping the + // strong sender alive across awaits would defeat the + // weak-sender shutdown path. + // + // Note: this iteration upgrades the weak sender twice (once + // for `query_reboot_flag`, once for `send_sd`). The user + // could call `shut_down` between them, in which case the + // first upgrade succeeds, the reboot flag arrives, then + // the second upgrade fails — emitting "Client shut down" + // partway through what was logically a single tick. The + // alternative (holding the strong sender across the + // `flag_rx.await`) would defeat the weak-sender shutdown + // path. The mid-tick log is harmless and not worth a + // refactor. let (flag_rx, flag_msg) = ControlMessage::query_reboot_flag(); let Some(sender) = weak_sender.upgrade() else { tracing::info!("Client shut down, stopping SD announcements"); @@ -632,9 +665,23 @@ where tracing::warn!("SD announcement channel closed, stopping"); break; } - let Ok(reboot) = flag_rx.await else { - tracing::warn!("SD announcement reboot-flag query dropped, stopping"); - break; + let reboot = match flag_rx.await { + Ok(Ok(flag)) => flag, + Ok(Err(e)) => { + // Run loop returned a typed error (e.g. + // `Error::Capacity("request_queue")`). Skip this + // tick and try again next interval — capacity + // pressure is transient. + tracing::warn!( + "SD announcement reboot-flag query returned error ({:?}), skipping tick", + e + ); + continue; + } + Err(_) => { + tracing::warn!("SD announcement reboot-flag query dropped, stopping"); + break; + } }; let mut header = sd_header.clone(); MessageDefinitions::set_reboot_flag(&mut header, reboot); @@ -853,7 +900,7 @@ mod tests { #[tokio::test] async fn test_client_new_and_interface() { let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); assert_eq!(client.interface(), Ipv4Addr::LOCALHOST); client.shut_down(); } @@ -861,7 +908,7 @@ mod tests { #[tokio::test] async fn test_client_debug() { let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let debug_str = format!("{client:?}"); assert!(debug_str.contains("Client")); assert!(debug_str.contains("127.0.0.1")); @@ -908,7 +955,7 @@ mod tests { #[tokio::test] async fn test_subscribe_unknown_service_returns_error() { let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let result = client.subscribe(0xFFFF, 0xFFFF, 1, 3, 0x01, 0).await; assert!( matches!(result, Err(Error::ServiceNotFound)), @@ -920,7 +967,7 @@ mod tests { #[tokio::test] async fn test_subscribe_no_wait_unknown_service_does_not_panic() { let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); // subscribe_no_wait is fire-and-forget — it should not panic even // when the service is unknown (the inner loop sends ServiceNotFound // on the dropped response channel, which is harmless). @@ -942,7 +989,7 @@ mod tests { #[tokio::test] async fn test_subscribe_no_wait_fire_and_forget_stress() { let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); // Unknown service so the inner loop's ServiceNotFound branch // fires on every iteration — that's the path where the @@ -973,7 +1020,7 @@ mod tests { #[tokio::test] async fn test_bind_discovery_and_unbind() { let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); client.bind_discovery().await.unwrap(); client.unbind_discovery().await.unwrap(); client.shut_down(); @@ -982,7 +1029,7 @@ mod tests { #[tokio::test] async fn test_set_interface() { let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let new_addr = Ipv4Addr::LOCALHOST; client.set_interface(new_addr).await.unwrap(); assert_eq!(client.interface(), new_addr); @@ -992,7 +1039,7 @@ mod tests { #[tokio::test] async fn test_add_endpoint_succeeds() { let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let addr = SocketAddrV4::new(Ipv4Addr::new(192, 168, 1, 1), 30000); client.add_endpoint(0x1234, 0x0001, addr, 0).await.unwrap(); client.shut_down(); @@ -1001,7 +1048,7 @@ mod tests { #[tokio::test] async fn test_send_to_service_unknown_returns_error() { let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let msg = crate::protocol::Message::new_sd(1, &empty_sd_header()); let result = client.send_to_service(0xFFFF, 0xFFFF, msg).await; assert!( @@ -1014,7 +1061,7 @@ mod tests { #[tokio::test] async fn test_remove_endpoint_succeeds() { let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let addr = SocketAddrV4::new(Ipv4Addr::new(192, 168, 1, 1), 30000); client.add_endpoint(0x1234, 0x0001, addr, 0).await.unwrap(); client.remove_endpoint(0x1234, 0x0001).await.unwrap(); @@ -1056,7 +1103,7 @@ mod tests { #[tokio::test] async fn test_send_sd_message() { let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); // Bind discovery first so the send path uses the existing socket client.bind_discovery().await.unwrap(); let target = SocketAddrV4::new(Ipv4Addr::LOCALHOST, 30490); @@ -1068,7 +1115,7 @@ mod tests { #[tokio::test] async fn test_send_to_service_success_returns_pending_response() { let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let addr = SocketAddrV4::new(Ipv4Addr::LOCALHOST, 30000); client.add_endpoint(0x1234, 0x0001, addr, 0).await.unwrap(); let msg = crate::protocol::Message::new_sd(1, &empty_sd_header()); @@ -1081,7 +1128,7 @@ mod tests { #[tokio::test] async fn test_recv_returns_none_after_shutdown() { let (client, mut updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); client.shut_down(); // Now the inner loop should exit; recv() should return None let result = tokio::time::timeout(std::time::Duration::from_secs(2), updates.recv()).await; @@ -1092,7 +1139,7 @@ mod tests { #[tokio::test] async fn test_register_and_unregister_e2e() { let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let key = E2EKey { service_id: 0x1234, method_or_event_id: 0x0001, @@ -1106,7 +1153,7 @@ mod tests { #[tokio::test] async fn test_client_is_clone() { let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let client2 = client.clone(); assert_eq!(client.interface(), client2.interface()); client.shut_down(); @@ -1115,7 +1162,7 @@ mod tests { #[tokio::test] async fn test_client_updates_debug() { let (_client, updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let debug_str = format!("{updates:?}"); assert!(debug_str.contains("ClientUpdates")); } @@ -1123,7 +1170,7 @@ mod tests { #[tokio::test] async fn test_request_unknown_service_returns_error() { let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let msg = crate::protocol::Message::new_sd(1, &empty_sd_header()); let result = client.request(0xFFFF, 0xFFFF, msg).await; assert!( @@ -1136,7 +1183,7 @@ mod tests { #[tokio::test] async fn test_sd_announcements_loop_does_not_panic() { let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); client.bind_discovery().await.unwrap(); let sd_header = empty_sd_header(); @@ -1161,7 +1208,7 @@ mod tests { #[tokio::test] async fn test_sd_announcements_loop_without_discovery_bound() { let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); // Don't bind discovery — the task should handle the error gracefully. let sd_header = empty_sd_header(); let handle = tokio::spawn( @@ -1184,7 +1231,7 @@ mod tests { #[tokio::test] async fn test_sd_announcements_loop_abort_stops_task() { let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); client.bind_discovery().await.unwrap(); let sd_header = empty_sd_header(); @@ -1213,7 +1260,7 @@ mod tests { // than using the stale caller-supplied value. let (client, mut updates, run_fut) = TestClient::new_with_loopback(Ipv4Addr::LOCALHOST, true); - tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); client.bind_discovery().await.unwrap(); // Caller bakes in Continuous — the announcer must override this. @@ -1226,8 +1273,10 @@ mod tests { ); // Loopback delivers our own SD announcements back as DiscoveryUpdated. - // Drain updates until we see one (tokio::time::interval skips the - // first immediate tick, so the first real send lands at ~100-200ms). + // Drain updates until we see one. `sd_announcements_loop` uses + // `Timer::sleep` repeatedly (not `tokio::time::interval`), so the + // first send lands ~one interval after the loop is polled, i.e. + // ~100ms here. let received = tokio::time::timeout(std::time::Duration::from_secs(2), async { loop { match updates.recv().await { @@ -1264,20 +1313,23 @@ mod tests { // `reboot_flag()` call after unbind — falsely advertising a reboot // to peers on the next manually-built SD header. let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); // No discovery bound. Fallback should reflect persisted state. // Default (unwrapped) → RecentlyRebooted. assert_eq!( - client.reboot_flag().await, + client.reboot_flag().await.expect("reboot_flag"), crate::protocol::sd::RebootFlag::RecentlyRebooted ); // Simulate post-wrap state (normally set by `unbind_discovery` // reading the departing socket's `reboot_flag`). - client.force_sd_session_wrapped_for_test(true).await; + client + .force_sd_session_wrapped_for_test(true) + .await + .expect("force_sd_session_wrapped_for_test"); assert_eq!( - client.reboot_flag().await, + client.reboot_flag().await.expect("reboot_flag"), crate::protocol::sd::RebootFlag::Continuous, "reboot_flag must report Continuous from persisted state while \ discovery is unbound" @@ -1287,7 +1339,7 @@ mod tests { // `bind_discovery_seeded`, so the live flag agrees. client.bind_discovery().await.unwrap(); assert_eq!( - client.reboot_flag().await, + client.reboot_flag().await.expect("reboot_flag"), crate::protocol::sd::RebootFlag::Continuous, "seeded socket must report Continuous after wrapped rebind" ); @@ -1298,25 +1350,44 @@ mod tests { #[tokio::test] async fn test_reboot_flag_defaults_to_recently_rebooted() { let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); // Discovery not bound — should fall back to RecentlyRebooted. assert_eq!( - client.reboot_flag().await, + client.reboot_flag().await.expect("reboot_flag"), crate::protocol::sd::RebootFlag::RecentlyRebooted ); client.bind_discovery().await.unwrap(); // Freshly bound socket also reports RecentlyRebooted (session has not wrapped). assert_eq!( - client.reboot_flag().await, + client.reboot_flag().await.expect("reboot_flag"), crate::protocol::sd::RebootFlag::RecentlyRebooted ); client.shut_down(); } + #[tokio::test] + async fn reboot_flag_returns_shutdown_error_when_run_loop_dropped() { + // Regression for the migration of `reboot_flag` from `.unwrap()` + // panics to `Result` (matches every other + // public Client method's Shutdown semantics). Dropping the run + // future closes the control channel; calling `reboot_flag` must + // surface `Err(Error::Shutdown)` rather than panicking. + let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); + drop(run_fut); + let err = client + .reboot_flag() + .await + .expect_err("reboot_flag must return an error after run loop is dropped"); + assert!( + matches!(err, Error::Shutdown), + "expected Shutdown, got {err:?}" + ); + } + #[tokio::test] async fn test_sd_announcements_loop_stops_on_shutdown() { let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); client.bind_discovery().await.unwrap(); let sd_header = empty_sd_header(); @@ -1441,7 +1512,7 @@ mod tests { }; let (client, _updates, run_fut) = TestClient::new_with_loopback(iface, true); - tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); client.bind_discovery().await.unwrap(); let interval = std::time::Duration::from_millis(100); @@ -1510,7 +1581,7 @@ mod tests { }; let (client, _updates, run_fut) = TestClient::new_with_loopback(iface, true); - tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); client.bind_discovery().await.unwrap(); let interval = std::time::Duration::from_millis(100); @@ -1575,7 +1646,7 @@ mod tests { impl Spawner for CountingSpawner { fn spawn(&self, future: impl core::future::Future + Send + 'static) { self.count.fetch_add(1, Ordering::SeqCst); - let _ = tokio::spawn(future); + let _run_handle = tokio::spawn(future); } } @@ -1586,7 +1657,7 @@ mod tests { let (client, _updates, run_fut) = TestClient::new_with_spawner_and_loopback(Ipv4Addr::LOCALHOST, false, spawner); - tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); client .bind_discovery() diff --git a/src/client/session.rs b/src/client/session.rs index 4639989..268b0b2 100644 --- a/src/client/session.rs +++ b/src/client/session.rs @@ -35,8 +35,9 @@ struct SessionState { pub enum SessionVerdict { /// Session is valid (normal increment or first message with matching state). Ok, - /// Sender has rebooted (reboot flag 0→1 transition, or session ID decreased - /// while reboot flag remains 1 within the same service instance stream). + /// Sender has rebooted (reboot flag transitioned `Continuous → RecentlyRebooted`, + /// or session ID decreased while the reboot flag remains `RecentlyRebooted` + /// within the same service instance stream). Reboot, /// First message ever seen from this service instance on this transport. Initial, @@ -46,8 +47,8 @@ pub enum SessionVerdict { /// /// A reboot is detected when, for a given `(sender, transport, service_id, /// instance_id)` tuple: -/// - The reboot flag transitions from 0 to 1, **or** -/// - The session ID decreases while the reboot flag remains 1 +/// - The reboot flag transitions from `Continuous` to `RecentlyRebooted`, **or** +/// - The session ID decreases while the reboot flag remains `RecentlyRebooted` /// /// Tracking per service instance (rather than per sender) avoids false /// positives when a sensor interleaves SD offers for multiple services diff --git a/src/client/socket_manager.rs b/src/client/socket_manager.rs index 567ccb2..6966a09 100644 --- a/src/client/socket_manager.rs +++ b/src/client/socket_manager.rs @@ -65,7 +65,7 @@ use std::{ task::{Context, Poll}, }; use tokio::sync::mpsc; -use tracing::{error, info, trace}; +use tracing::{debug, error, info, trace, warn}; /// A received message together with the source address it came from. /// @@ -307,14 +307,32 @@ where target_addr: SocketAddrV4, message: Message, ) -> Result<(), Error> { + // Pre-encode size check: fail fast with `Error::Capacity("udp_buffer")` + // for messages that exceed `UDP_BUFFER_SIZE`. Mirrors the analogous + // check in `server::EventPublisher` so callers see a uniform + // overload signal regardless of which path produced the oversize + // 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(); + if required > UDP_BUFFER_SIZE { + warn!( + "outgoing message size {required} exceeds UDP_BUFFER_SIZE ({UDP_BUFFER_SIZE}); rejecting with Capacity(\"udp_buffer\")" + ); + return Err(Error::Capacity("udp_buffer")); + } let (result_channel, message) = SendMessage::new(target_addr, message); self.sender.send(message).await.map_err(|e| { error!("Socket error: {e} when attempting to send message"); Error::SocketClosedUnexpectedly })?; - result_channel - .await - .expect("Socket manager must always return result of send before dropping channel")?; + // The socket loop's response sender can be dropped without sending + // (executor cancellation, bare-metal `Spawner` that drops futures, + // or a panic in the loop). Surface that as a typed error rather + // than `.expect`-panicking the caller. + result_channel.await.map_err(|_| { + debug!("send result channel dropped (socket loop gone)"); + Error::SocketClosedUnexpectedly + })??; if self.session_id == u16::MAX { self.session_id = 1; self.session_has_wrapped = true; @@ -382,6 +400,15 @@ where mut tx_rx: mpsc::Receiver>, e2e_registry: Arc>, ) { + // Maximum number of consecutive `recv_from` errors tolerated before + // the socket loop gives up. A single failure (transient I/O, peer + // RST, ICMP port-unreachable amplified into `ConnectionRefused`) + // is normal and should not tear down the socket. A persistent + // failure (e.g. `EBADF` after the kernel closed the fd, or a + // platform-level network-stack collapse) used to pin a CPU on a + // 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]; loop { @@ -506,6 +533,7 @@ where source, truncated, })) => { + consecutive_recv_errors = 0; if truncated { // A truncated datagram cannot be parsed reliably; // the length field in the SOME/IP header will not @@ -553,7 +581,23 @@ where } } Outcome::Recv(Err(recv_err)) => { - error!("Transport recv failed: {:?}", recv_err); + // `tokio_transport::map_io_error` already logs the + // underlying `std::io::Error` (debug for transient + // kinds, warn for unusual ones) — keep this + // call-site at debug to avoid duplicating the same + // failure on the operator's screen. + consecutive_recv_errors = consecutive_recv_errors.saturating_add(1); + debug!( + "socket recv_from error ({}/{}): {:?}", + consecutive_recv_errors, MAX_CONSECUTIVE_RECV_ERRORS, recv_err, + ); + if consecutive_recv_errors >= MAX_CONSECUTIVE_RECV_ERRORS { + error!( + "socket recv_from failed {} times consecutively; closing socket loop", + consecutive_recv_errors, + ); + break; + } } } } @@ -764,13 +808,13 @@ mod tests { #[tokio::test] async fn test_session_id_wraps_to_one_and_clears_reboot_flag() { + use crate::protocol::sd::RebootFlag; let mut sm = bind_ephemeral_spawned().await; let raw_socket = UdpSocket::bind("127.0.0.1:0").await.unwrap(); let target = SocketAddrV4::new(Ipv4Addr::LOCALHOST, raw_socket.local_addr().unwrap().port()); let msg = || Message::::new_sd(1, &empty_sd_header()); - use crate::protocol::sd::RebootFlag; // Set session_id to one before the wrap point sm.session_id = u16::MAX - 1; assert_eq!( @@ -813,6 +857,15 @@ mod tests { 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); @@ -824,11 +877,7 @@ mod tests { .await .unwrap(); - // Craft a message whose raw-encoded size fits UDP_BUFFER_SIZE (16-byte - // header + 1480-byte payload = 1496 bytes) but whose E2E-protected - // size does not (payload grows by PROFILE4_HEADER_SIZE = 12, pushing - // the total to 1508 bytes, 8 over MTU). - let payload_bytes = [0u8; 1480]; + let payload_bytes = [0u8; PAYLOAD_LEN]; let payload = RawPayload::from_payload_bytes(message_id, &payload_bytes).unwrap(); let header = Header::new( message_id, @@ -949,7 +998,7 @@ mod tests { .await .expect("send_to via custom-factory-built socket"); - let mut buf = [0u8; 1500]; + let mut buf = [0u8; UDP_BUFFER_SIZE]; let (len, from) = tokio::time::timeout(std::time::Duration::from_secs(2), recv.recv_from(&mut buf)) .await diff --git a/src/e2e/crc.rs b/src/e2e/crc.rs index 2eb08d7..91e60ca 100644 --- a/src/e2e/crc.rs +++ b/src/e2e/crc.rs @@ -115,10 +115,10 @@ mod tests { #[test] fn test_crc32_p4_basic() { // Basic smoke test - verify CRC changes with different inputs - let crc1 = compute_crc32_p4(10, 0, 0x12345678, b"test"); - let crc2 = compute_crc32_p4(10, 1, 0x12345678, b"test"); - let crc3 = compute_crc32_p4(10, 0, 0x12345679, b"test"); - let crc4 = compute_crc32_p4(10, 0, 0x12345678, b"Test"); + let crc1 = compute_crc32_p4(10, 0, 0x1234_5678, b"test"); + let crc2 = compute_crc32_p4(10, 1, 0x1234_5678, b"test"); + let crc3 = compute_crc32_p4(10, 0, 0x1234_5679, b"test"); + let crc4 = compute_crc32_p4(10, 0, 0x1234_5678, b"Test"); assert_ne!(crc1, crc2, "Different counter should produce different CRC"); assert_ne!(crc1, crc3, "Different data_id should produce different CRC"); @@ -141,8 +141,8 @@ mod tests { #[test] fn test_crc32_p4_deterministic() { // Same inputs should always produce same output - let crc1 = compute_crc32_p4(20, 5, 0xABCDEF01, b"payload data"); - let crc2 = compute_crc32_p4(20, 5, 0xABCDEF01, b"payload data"); + let crc1 = compute_crc32_p4(20, 5, 0xABCD_EF01, b"payload data"); + let crc2 = compute_crc32_p4(20, 5, 0xABCD_EF01, b"payload data"); assert_eq!(crc1, crc2); } @@ -157,7 +157,7 @@ mod tests { #[test] fn test_crc32_p4_empty_payload() { // Should work with empty payload - let crc = compute_crc32_p4(8, 0, 0x12345678, b""); + let crc = compute_crc32_p4(8, 0, 0x1234_5678, b""); assert_ne!(crc, 0); // CRC should be non-trivial even for empty payload } diff --git a/src/e2e/e2e_checker.rs b/src/e2e/e2e_checker.rs index 549f744..e8b7377 100644 --- a/src/e2e/e2e_checker.rs +++ b/src/e2e/e2e_checker.rs @@ -250,7 +250,7 @@ mod tests { #[test] fn test_check_profile4_valid() { - let config = Profile4Config::new(0x12345678, 15); + let config = Profile4Config::new(0x1234_5678, 15); let mut protect_state = Profile4State::new(); let mut check_state = Profile4State::new(); @@ -267,8 +267,8 @@ mod tests { #[test] fn test_check_profile4_wrong_data_id() { - let config1 = Profile4Config::new(0x12345678, 15); - let config2 = Profile4Config::new(0xDEADBEEF, 15); + let config1 = Profile4Config::new(0x1234_5678, 15); + let config2 = Profile4Config::new(0xDEAD_BEEF, 15); let mut protect_state = Profile4State::new(); let mut check_state = Profile4State::new(); @@ -283,7 +283,7 @@ mod tests { #[test] fn test_check_profile4_corrupted_crc() { - let config = Profile4Config::new(0x12345678, 15); + let config = Profile4Config::new(0x1234_5678, 15); let mut protect_state = Profile4State::new(); let mut check_state = Profile4State::new(); @@ -300,7 +300,7 @@ mod tests { #[test] fn test_check_profile4_corrupted_payload() { - let config = Profile4Config::new(0x12345678, 15); + let config = Profile4Config::new(0x1234_5678, 15); let mut protect_state = Profile4State::new(); let mut check_state = Profile4State::new(); @@ -317,7 +317,7 @@ mod tests { #[test] fn test_check_profile4_wrong_length() { - let config = Profile4Config::new(0x12345678, 15); + let config = Profile4Config::new(0x1234_5678, 15); let mut protect_state = Profile4State::new(); let mut check_state = Profile4State::new(); @@ -332,7 +332,7 @@ mod tests { #[test] fn test_check_profile4_too_short() { - let config = Profile4Config::new(0x12345678, 15); + let config = Profile4Config::new(0x1234_5678, 15); let mut check_state = Profile4State::new(); let short = [0u8; 11]; // Less than 12-byte header @@ -389,7 +389,7 @@ mod tests { #[test] fn test_sequence_repeated() { - let config = Profile4Config::new(0x12345678, 15); + let config = Profile4Config::new(0x1234_5678, 15); let mut protect_state = Profile4State::new(); let mut check_state = Profile4State::new(); @@ -409,7 +409,7 @@ mod tests { #[test] fn test_sequence_consecutive() { - let config = Profile4Config::new(0x12345678, 15); + let config = Profile4Config::new(0x1234_5678, 15); let mut protect_state = Profile4State::new(); let mut check_state = Profile4State::new(); @@ -425,7 +425,7 @@ mod tests { #[test] fn test_sequence_some_lost() { - let config = Profile4Config::new(0x12345678, 10); + let config = Profile4Config::new(0x1234_5678, 10); let mut protect_state = Profile4State::new(); let mut check_state = Profile4State::new(); @@ -450,7 +450,7 @@ mod tests { #[test] fn test_sequence_wrong_sequence() { - let config = Profile4Config::new(0x12345678, 3); + let config = Profile4Config::new(0x1234_5678, 3); let mut protect_state = Profile4State::new(); let mut check_state = Profile4State::new(); @@ -475,7 +475,7 @@ mod tests { #[test] fn test_sequence_wraparound() { - let config = Profile4Config::new(0x12345678, 5); + let config = Profile4Config::new(0x1234_5678, 5); let mut protect_state = Profile4State::with_initial_counter(u16::MAX - 2); let mut check_state = Profile4State::new(); @@ -533,7 +533,7 @@ mod tests { assert_eq!(result.status, E2ECheckStatus::Ok); assert_eq!(result.counter, Some(0)); - assert_eq!(result.payload.as_deref(), Some(payload.as_slice())); + assert_eq!(result.payload, Some(payload.as_slice())); } #[test] diff --git a/src/e2e/e2e_protector.rs b/src/e2e/e2e_protector.rs index 90122f8..9a3d48d 100644 --- a/src/e2e/e2e_protector.rs +++ b/src/e2e/e2e_protector.rs @@ -196,7 +196,7 @@ mod tests { #[test] fn test_protect_profile4_header_format() { - let config = Profile4Config::new(0x12345678, 15); + let config = Profile4Config::new(0x1234_5678, 15); let mut state = Profile4State::new(); let payload = b"test"; @@ -217,7 +217,7 @@ mod tests { // Check data_id field (bytes 4-7) let data_id = u32::from_be_bytes([protected[4], protected[5], protected[6], protected[7]]); - assert_eq!(data_id, 0x12345678); + assert_eq!(data_id, 0x1234_5678); // Check payload at end assert_eq!(&protected[12..], b"test"); @@ -225,7 +225,7 @@ mod tests { #[test] fn test_protect_profile4_counter_increment() { - let config = Profile4Config::new(0x12345678, 15); + let config = Profile4Config::new(0x1234_5678, 15); let mut state = Profile4State::new(); let payload = b"test"; @@ -241,7 +241,7 @@ mod tests { #[test] fn test_protect_profile4_counter_wraps() { - let config = Profile4Config::new(0x12345678, 15); + let config = Profile4Config::new(0x1234_5678, 15); let mut state = Profile4State::with_initial_counter(u16::MAX); let payload = b"test"; @@ -400,7 +400,7 @@ mod tests { #[test] fn test_protect_profile4_buffer_too_small() { - let config = Profile4Config::new(0x12345678, 15); + let config = Profile4Config::new(0x1234_5678, 15); let mut state = Profile4State::new(); let payload = b"test"; @@ -458,7 +458,7 @@ mod tests { #[test] #[cfg(feature = "std")] fn test_protect_profile4_length_overflow() { - let config = Profile4Config::new(0x12345678, 15); + let config = Profile4Config::new(0x1234_5678, 15); let mut state = Profile4State::new(); // payload of 65536 bytes => total = 12 + 65536 = 65548 > u16::MAX @@ -470,7 +470,7 @@ mod tests { #[test] fn test_protect_profile4_empty_payload() { - let config = Profile4Config::new(0x12345678, 15); + let config = Profile4Config::new(0x1234_5678, 15); let mut state = Profile4State::new(); let mut buf = [0u8; 256]; diff --git a/src/e2e/mod.rs b/src/e2e/mod.rs index 233db20..02a52b6 100644 --- a/src/e2e/mod.rs +++ b/src/e2e/mod.rs @@ -12,7 +12,7 @@ //! E2ECheckStatus, //! }; //! -//! let config = Profile4Config::new(0x12345678, 15); +//! let config = Profile4Config::new(0x1234_5678, 15); //! let mut protect_state = Profile4State::new(); //! let mut check_state = Profile4State::new(); //! @@ -251,7 +251,7 @@ mod tests { #[test] fn test_profile4_roundtrip() { - let config = Profile4Config::new(0x12345678, 15); + let config = Profile4Config::new(0x1234_5678, 15); let mut protect_state = Profile4State::new(); let mut check_state = Profile4State::new(); @@ -291,7 +291,7 @@ mod tests { #[test] fn test_profile4_sequence_detection() { - let config = Profile4Config::new(0x12345678, 5); + let config = Profile4Config::new(0x1234_5678, 5); let mut protect_state = Profile4State::new(); let mut check_state = Profile4State::new(); @@ -319,7 +319,7 @@ mod tests { #[test] fn test_profile4_some_lost_detection() { - let config = Profile4Config::new(0x12345678, 5); + let config = Profile4Config::new(0x1234_5678, 5); let mut protect_state = Profile4State::new(); let mut check_state = Profile4State::new(); @@ -343,7 +343,7 @@ mod tests { #[test] fn test_profile4_wrong_sequence_detection() { - let config = Profile4Config::new(0x12345678, 2); + let config = Profile4Config::new(0x1234_5678, 2); let mut protect_state = Profile4State::new(); let mut check_state = Profile4State::new(); @@ -368,7 +368,7 @@ mod tests { #[test] fn test_profile4_crc_error() { - let config = Profile4Config::new(0x12345678, 15); + let config = Profile4Config::new(0x1234_5678, 15); let mut protect_state = Profile4State::new(); let mut check_state = Profile4State::new(); @@ -403,7 +403,7 @@ mod tests { #[test] fn test_profile4_bad_argument_short_message() { - let config = Profile4Config::new(0x12345678, 15); + let config = Profile4Config::new(0x1234_5678, 15); let mut check_state = Profile4State::new(); // Message too short (less than 12-byte header) diff --git a/src/e2e/registry.rs b/src/e2e/registry.rs index 0dcd8c8..7a7c39b 100644 --- a/src/e2e/registry.rs +++ b/src/e2e/registry.rs @@ -85,7 +85,7 @@ mod tests { fn register_and_check_profile4() { let mut reg = E2ERegistry::new(); let key = make_key(); - let config = Profile4Config::new(0x12345678, 15); + let config = Profile4Config::new(0x1234_5678, 15); reg.register(key, E2EProfile::Profile4(config.clone())); assert!(reg.contains_key(&key)); diff --git a/src/lib.rs b/src/lib.rs index 9a5eec7..e0d67b4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -29,7 +29,7 @@ //! | `std` | yes | Enables std-dependent helpers (`RawPayload`, `VecSdHeader`, `OfferedEndpoint`) | //! | `client` | no | Async tokio client; implies `std` + tokio + socket2 + futures | //! | `server` | no | Async tokio server; implies `std` + tokio + socket2 + futures | -//! | `bare_metal` | no | Pure marker feature — enables no crate code. Reserved for future phases to gate no_std helper types. To exercise the bare-metal trait surface today, use the `examples/bare_metal` workspace member (`cargo run -p bare_metal`). **Does not make `client` / `server` bare-metal-usable.** The `Spawner` trait (phase 9) makes task submission pluggable, but the `client` / `server` feature paths still depend on: (1) `tokio::sync::mpsc` channels (heap + tokio-waker-coupled) for intra-module message passing, (2) `tokio::sync::oneshot` for send-acks, (3) `Arc>` for shared registry state (requires `alloc` + `std::sync`), and (4) an `F::Socket = TokioSocket` bound on `SocketManager::bind_*` that needs stable Rust Return-Type Notation to relax. Until all four are resolved, `feature = "client"` / `feature = "server"` remain `std`+tokio-only. `no_alloc` consumers today should build their own orchestrator on `protocol`, `e2e`, and the `transport` traits directly — those layers ARE fully `no_std` / `no_alloc`. | +//! | `bare_metal` | no | Pure marker — does not enable any crate code. See `examples/bare_metal/` (the trait-surface canary) for the full bare-metal-readiness story. | //! //! The default feature set is `["std"]`, which links `std` and enables //! the `RawPayload` / `VecSdHeader` helpers. For a minimal build with @@ -37,7 +37,10 @@ //! `e2e` modules only — pass `--no-default-features`. The //! trait-surface canary at `examples/bare_metal/` depends on the crate //! with `default-features = false, features = ["bare_metal"]` and -//! proves the no-default-features build compiles. +//! validates that configuration when the `bare_metal` workspace member +//! is built in isolation (`cargo build -p bare_metal` or +//! `cargo run -p bare_metal`), rather than as part of a workspace-wide +//! build where features may be unified across members. //! //! ## Examples //! @@ -150,7 +153,10 @@ pub mod tokio_transport; mod traits; /// Executor-agnostic UDP transport abstraction used by the client and /// server modules. `no_std`-compatible; a default `std + tokio` backend -/// ships in [`tokio_transport`] under the `client` / `server` features. +/// ships in `tokio_transport` (available under the `client` / `server` +/// features) — the link is rendered as a code literal because the target +/// module is feature-gated and would break default-feature rustdoc +/// builds. pub mod transport; #[cfg(feature = "std")] pub use raw_payload::{RawPayload, VecSdHeader}; diff --git a/src/protocol/byte_order.rs b/src/protocol/byte_order.rs index 6d1061f..9c41106 100644 --- a/src/protocol/byte_order.rs +++ b/src/protocol/byte_order.rs @@ -273,6 +273,10 @@ impl WriteBytesExt for T { } #[cfg(test)] +// Strict float equality is correct here: these tests verify byte-level +// round-tripping of `to_be_bytes` / `read_f*_be`, where the result must +// be bitwise-identical to the input. +#[allow(clippy::float_cmp)] mod tests { use super::*; diff --git a/src/protocol/header.rs b/src/protocol/header.rs index 1a3ca2c..921ee22 100644 --- a/src/protocol/header.rs +++ b/src/protocol/header.rs @@ -321,6 +321,23 @@ impl<'a> HeaderView<'a> { } } +impl WireFormat for Header { + fn required_size(&self) -> usize { + 16 + } + + fn encode(&self, writer: &mut T) -> Result { + writer.write_u32_be(self.message_id.message_id())?; + writer.write_u32_be(self.length)?; + writer.write_u32_be(self.request_id)?; + writer.write_u8(self.protocol_version)?; + writer.write_u8(self.interface_version)?; + writer.write_u8(u8::from(self.message_type))?; + writer.write_u8(u8::from(self.return_code))?; + Ok(16) + } +} + #[cfg(test)] mod tests { use super::*; @@ -591,20 +608,3 @@ mod tests { assert_eq!(view.to_owned(), h); } } - -impl WireFormat for Header { - fn required_size(&self) -> usize { - 16 - } - - fn encode(&self, writer: &mut T) -> Result { - writer.write_u32_be(self.message_id.message_id())?; - writer.write_u32_be(self.length)?; - writer.write_u32_be(self.request_id)?; - writer.write_u8(self.protocol_version)?; - writer.write_u8(self.interface_version)?; - writer.write_u8(u8::from(self.message_type))?; - writer.write_u8(u8::from(self.return_code))?; - Ok(16) - } -} diff --git a/src/protocol/message_id.rs b/src/protocol/message_id.rs index d2815cb..533550b 100644 --- a/src/protocol/message_id.rs +++ b/src/protocol/message_id.rs @@ -83,6 +83,17 @@ impl MessageId { } } +impl core::fmt::Debug for MessageId { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + write!( + f, + "Message Id: {{ service_id: {:#02X}, method_id: {:#02X} }}", + self.service_id(), + self.method_id(), + ) + } +} + #[cfg(test)] mod tests { use super::*; @@ -180,14 +191,3 @@ mod tests { assert!(buf.contains("method_id")); } } - -impl core::fmt::Debug for MessageId { - fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - write!( - f, - "Message Id: {{ service_id: {:#02X}, method_id: {:#02X} }}", - self.service_id(), - self.method_id(), - ) - } -} diff --git a/src/protocol/sd/header.rs b/src/protocol/sd/header.rs index b513152..2321a31 100644 --- a/src/protocol/sd/header.rs +++ b/src/protocol/sd/header.rs @@ -234,7 +234,7 @@ mod tests { service_id: 0x1234, instance_id: 0x0001, major_version: 1, - ttl: 0xFFFFFF, + ttl: 0xFF_FFFF, index_first_options_run: 0, index_second_options_run: 0, options_count: OptionsCount::new(1, 0), @@ -264,7 +264,7 @@ mod tests { #[test] fn subscribe_ack_round_trips() { let entry = Entry::SubscribeAckEventGroup(EventGroupEntry::new( - 0xAAAA, 0x0001, 1, 0xFFFFFF, 0x0010, + 0xAAAA, 0x0001, 1, 0xFF_FFFF, 0x0010, )); let entries = [entry]; let h = Header::new(Flags::new_sd(RebootFlag::RecentlyRebooted), &entries, &[]); @@ -281,7 +281,7 @@ mod tests { service_id: 0x1234, instance_id: 0x0001, major_version: 1, - ttl: 0xFFFFFF, + ttl: 0xFF_FFFF, index_first_options_run: 0, index_second_options_run: 0, options_count: OptionsCount::new(1, 0), diff --git a/src/protocol/sd/options.rs b/src/protocol/sd/options.rs index 8286649..96fd9a6 100644 --- a/src/protocol/sd/options.rs +++ b/src/protocol/sd/options.rs @@ -241,7 +241,7 @@ impl Options { /// /// # Panics /// - /// Panics if the option size minus [`OPTION_LENGTH_SIZE_DELTA`] exceeds `u16::MAX` + /// Panics if the option size minus `OPTION_LENGTH_SIZE_DELTA` exceeds `u16::MAX` /// (unreachable in practice). pub fn write( &self, @@ -353,7 +353,7 @@ impl<'a> OptionView<'a> { OptionType::try_from(self.0[OPTION_TYPE_OFFSET]) } - /// Total wire size of this option (length field value + [`OPTION_LENGTH_SIZE_DELTA`]). + /// Total wire size of this option (length field value + `OPTION_LENGTH_SIZE_DELTA`). #[must_use] pub fn wire_size(&self) -> usize { let length = u16::from_be_bytes([self.0[0], self.0[1]]); diff --git a/src/server/README.md b/src/server/README.md index 845905a..effa13b 100644 --- a/src/server/README.md +++ b/src/server/README.md @@ -91,7 +91,7 @@ The server periodically sends **OfferService** messages to the multicast group ` ``` SD Message Structure: -├─ Flags: Reboot=true, Unicast=false +├─ Flags: Reboot=Recently/Continuous (per session-counter wrap), Unicast=true ├─ Entry: OfferService │ ├─ Service ID │ ├─ Instance ID @@ -171,6 +171,11 @@ Publishes events to subscribers: - `publish_raw_event(service_id, instance_id, event_group_id, event_id, session_id, protocol_version, interface_version, payload) -> Result` - Low-level event publishing using raw bytes - Returns number of subscribers that received the event +- `register_subscriber(service_id, instance_id, event_group_id, subscriber_addr) -> Result<(), SubscribeError>` + - Manually register a subscriber (advanced use; the built-in SD loop calls this for you) + - Capacity-rejects with `SubscribeError::*` so external dispatchers can emit a `SubscribeNack` +- `remove_subscriber(service_id, instance_id, event_group_id, subscriber_addr)` + - Manually remove a subscriber - `has_subscribers(service_id, instance_id, event_group_id) -> bool` - Check if any subscribers exist for an event group - `subscriber_count(service_id, instance_id, event_group_id) -> usize` @@ -180,10 +185,12 @@ Publishes events to subscribers: Manages event group subscriptions: -- `subscribe(service_id, instance_id, event_group_id, subscriber_addr)` - Add subscriber (deduplicates automatically) +- `subscribe(service_id, instance_id, event_group_id, subscriber_addr) -> Result<(), SubscribeError>` - Add subscriber (deduplicates automatically); returns `Err` when a fixed-capacity bound (`SUBSCRIBERS_PER_GROUP` or `EVENT_GROUPS_CAP`) is exhausted - `unsubscribe(service_id, instance_id, event_group_id, subscriber_addr)` - Remove subscriber - `get_subscribers(service_id, instance_id, event_group_id) -> Vec` - Get all subscribers +External dispatchers (those calling `EventPublisher::register_subscriber` directly) must NACK on `Err(SubscribeError::*)`; the server's built-in SD loop already does this automatically. + ## Troubleshooting ### No subscribers diff --git a/src/server/event_publisher.rs b/src/server/event_publisher.rs index 37dc09c..683d47b 100644 --- a/src/server/event_publisher.rs +++ b/src/server/event_publisher.rs @@ -127,7 +127,15 @@ impl EventPublisher { message_length = 16 + protected_len; } Some(Err(e)) => { - tracing::error!("E2E protect error: {:?}", e); + // Surface protect failures as `Err(Error::E2e(_))` + // rather than logging-and-falling-through, which + // would silently send the UNPROTECTED payload + // claiming an E2E-protected channel and break the + // receiver's CRC/counter checks. Counter + // exhaustion, key-lookup races, and similar + // backend errors all funnel here. + tracing::error!("E2E protect error: {:?}; dropping publish", e); + return Err(Error::E2e(e)); } None => unreachable!("contains_key was true"), } @@ -197,6 +205,22 @@ impl EventPublisher { return Ok(0); } + // Pre-build size check. Fail fast with `Error::Capacity` BEFORE + // calling `Header::new_event`, which `assert!`s on payloads + // larger than `u32::MAX as usize - 8`. The earlier + // `checked_add(header_len, payload.len())` guard below was dead + // for that reason; keeping it for defence-in-depth on platforms + // where `Header::SIZE + payload` could overflow `usize`. The + // `16` here is the SOME/IP header size in bytes. + if payload.len() > UDP_BUFFER_SIZE.saturating_sub(16) { + tracing::error!( + "raw event payload ({} bytes) + 16-byte header exceeds UDP_BUFFER_SIZE ({}); dropping publish", + payload.len(), + UDP_BUFFER_SIZE + ); + return Err(Error::Capacity("udp_buffer")); + } + // Build SOME/IP header let header = Header::new_event( service_id, @@ -219,6 +243,10 @@ impl EventPublisher { ); return Err(Error::Capacity("udp_buffer")); }; + // Defence-in-depth: the pre-build guard above already rejects + // oversize payloads, but a future caller adding optional + // post-encode tail bytes (e.g. another protect profile) would + // need this branch. Cheap to keep. if total_len > UDP_BUFFER_SIZE { tracing::error!( "raw event ({} bytes) exceeds UDP_BUFFER_SIZE ({}); dropping publish", @@ -408,9 +436,8 @@ mod tests { // Create a receiver socket to act as subscriber let receiver = UdpSocket::bind("127.0.0.1:0").await.unwrap(); - let recv_addr = match receiver.local_addr().unwrap() { - std::net::SocketAddr::V4(a) => a, - _ => panic!("expected v4"), + let std::net::SocketAddr::V4(recv_addr) = receiver.local_addr().unwrap() else { + panic!("expected v4 source address"); }; // Add subscriber @@ -496,9 +523,8 @@ mod tests { // test stays correct if the constant is retuned. Mirrors the // client-side oversize fixture in // `send_raw_message_exceeding_udp_buffer_returns_capacity_error`. - const SOMEIP_HEADER_SIZE: usize = 16; let message_id = MessageId::new_from_service_and_method(0x1234, 0x5678); - let payload_len = UDP_BUFFER_SIZE - SOMEIP_HEADER_SIZE + 1; + let payload_len = UDP_BUFFER_SIZE - 16 + 1 /* SOME/IP header is 16 bytes */; let payload_bytes = vec![0u8; payload_len]; let payload = RawPayload::from_payload_bytes(message_id, &payload_bytes).unwrap(); let header = Header::new( @@ -548,7 +574,8 @@ mod tests { let subscriptions = Arc::new(RwLock::new(SubscriptionManager::new())); { let mut mgr = subscriptions.write().await; - mgr.subscribe(0x5B, 1, 0x01, SocketAddrV4::new(Ipv4Addr::LOCALHOST, 9999)).unwrap(); + mgr.subscribe(0x5B, 1, 0x01, SocketAddrV4::new(Ipv4Addr::LOCALHOST, 9999)) + .unwrap(); } let socket = Arc::new(UdpSocket::bind("127.0.0.1:0").await.unwrap()); @@ -559,8 +586,7 @@ mod tests { // protection to push the encoded message over the limit and // exercise the post-protect guard — regardless of how // `UDP_BUFFER_SIZE` is retuned. - const SOMEIP_HEADER_SIZE: usize = 16; - let payload_len = UDP_BUFFER_SIZE - SOMEIP_HEADER_SIZE; // raw total == UDP_BUFFER_SIZE + let payload_len = UDP_BUFFER_SIZE - 16; // raw total == UDP_BUFFER_SIZE; SOME/IP header = 16 let payload_bytes = vec![0u8; payload_len]; let payload = RawPayload::from_payload_bytes(message_id, &payload_bytes).unwrap(); let header = Header::new_event( @@ -593,9 +619,8 @@ mod tests { let subscriptions = Arc::new(RwLock::new(SubscriptionManager::new())); let receiver = UdpSocket::bind("127.0.0.1:0").await.unwrap(); - let recv_addr = match receiver.local_addr().unwrap() { - std::net::SocketAddr::V4(a) => a, - _ => panic!("expected v4"), + let std::net::SocketAddr::V4(recv_addr) = receiver.local_addr().unwrap() else { + panic!("expected v4 source address"); }; { @@ -629,8 +654,8 @@ mod tests { #[tokio::test] async fn test_subscriber_count() { let subscriptions = Arc::new(RwLock::new(SubscriptionManager::new())); - let addr1 = SocketAddrV4::new(Ipv4Addr::new(127, 0, 0, 1), 9001); - let addr2 = SocketAddrV4::new(Ipv4Addr::new(127, 0, 0, 1), 9002); + let addr1 = SocketAddrV4::new(Ipv4Addr::LOCALHOST, 9001); + let addr2 = SocketAddrV4::new(Ipv4Addr::LOCALHOST, 9002); { let mut mgr = subscriptions.write().await; @@ -651,13 +676,8 @@ mod tests { { let mut mgr = subscriptions.write().await; - mgr.subscribe( - 0x5B, - 1, - 0x01, - SocketAddrV4::new(Ipv4Addr::new(127, 0, 0, 1), 9001), - ) - .unwrap(); + mgr.subscribe(0x5B, 1, 0x01, SocketAddrV4::new(Ipv4Addr::LOCALHOST, 9001)) + .unwrap(); } assert!(publisher.has_subscribers(0x5B, 1, 0x01).await); @@ -679,7 +699,10 @@ mod tests { let (publisher, _) = make_publisher(Arc::clone(&subscriptions)).await; assert!(!publisher.has_subscribers(0x5B, 1, 0x01).await); - publisher.register_subscriber(0x5B, 1, 0x01, ADDR_A).await.unwrap(); + publisher + .register_subscriber(0x5B, 1, 0x01, ADDR_A) + .await + .unwrap(); assert!(publisher.has_subscribers(0x5B, 1, 0x01).await); assert_eq!(publisher.subscriber_count(0x5B, 1, 0x01).await, 1); } @@ -691,9 +714,18 @@ mod tests { // Simulate TTL refreshes — the same (tuple, addr) called repeatedly // must not grow the subscriber list. - publisher.register_subscriber(0x5B, 1, 0x01, ADDR_A).await.unwrap(); - publisher.register_subscriber(0x5B, 1, 0x01, ADDR_A).await.unwrap(); - publisher.register_subscriber(0x5B, 1, 0x01, ADDR_A).await.unwrap(); + publisher + .register_subscriber(0x5B, 1, 0x01, ADDR_A) + .await + .unwrap(); + publisher + .register_subscriber(0x5B, 1, 0x01, ADDR_A) + .await + .unwrap(); + publisher + .register_subscriber(0x5B, 1, 0x01, ADDR_A) + .await + .unwrap(); assert_eq!(publisher.subscriber_count(0x5B, 1, 0x01).await, 1); } @@ -703,8 +735,14 @@ mod tests { let subscriptions = Arc::new(RwLock::new(SubscriptionManager::new())); let (publisher, _) = make_publisher(Arc::clone(&subscriptions)).await; - publisher.register_subscriber(0x5B, 1, 0x01, ADDR_A).await.unwrap(); - publisher.register_subscriber(0x5B, 1, 0x02, ADDR_A).await.unwrap(); + publisher + .register_subscriber(0x5B, 1, 0x01, ADDR_A) + .await + .unwrap(); + publisher + .register_subscriber(0x5B, 1, 0x02, ADDR_A) + .await + .unwrap(); assert_eq!(publisher.subscriber_count(0x5B, 1, 0x01).await, 1); assert_eq!(publisher.subscriber_count(0x5B, 1, 0x02).await, 1); @@ -717,7 +755,10 @@ mod tests { let subscriptions = Arc::new(RwLock::new(SubscriptionManager::new())); let (publisher, _) = make_publisher(Arc::clone(&subscriptions)).await; - publisher.register_subscriber(0x5B, 1, 0x01, ADDR_A).await.unwrap(); + publisher + .register_subscriber(0x5B, 1, 0x01, ADDR_A) + .await + .unwrap(); assert!(publisher.has_subscribers(0x5B, 1, 0x01).await); publisher.remove_subscriber(0x5B, 1, 0x01, ADDR_A).await; @@ -730,9 +771,18 @@ mod tests { let subscriptions = Arc::new(RwLock::new(SubscriptionManager::new())); let (publisher, _) = make_publisher(Arc::clone(&subscriptions)).await; - publisher.register_subscriber(0x5B, 1, 0x01, ADDR_A).await.unwrap(); - publisher.register_subscriber(0x5B, 1, 0x01, ADDR_B).await.unwrap(); - publisher.register_subscriber(0x5B, 1, 0x01, ADDR_C).await.unwrap(); + publisher + .register_subscriber(0x5B, 1, 0x01, ADDR_A) + .await + .unwrap(); + publisher + .register_subscriber(0x5B, 1, 0x01, ADDR_B) + .await + .unwrap(); + publisher + .register_subscriber(0x5B, 1, 0x01, ADDR_C) + .await + .unwrap(); assert_eq!(publisher.subscriber_count(0x5B, 1, 0x01).await, 3); publisher.remove_subscriber(0x5B, 1, 0x01, ADDR_B).await; @@ -757,7 +807,10 @@ mod tests { assert_eq!(publisher.subscriber_count(0x5B, 1, 0x01).await, 0); // Register one subscriber, then remove a different address. - publisher.register_subscriber(0x5B, 1, 0x01, ADDR_A).await.unwrap(); + publisher + .register_subscriber(0x5B, 1, 0x01, ADDR_A) + .await + .unwrap(); publisher.remove_subscriber(0x5B, 1, 0x01, ADDR_B).await; assert_eq!(publisher.subscriber_count(0x5B, 1, 0x01).await, 1); @@ -771,8 +824,14 @@ mod tests { let subscriptions = Arc::new(RwLock::new(SubscriptionManager::new())); let (publisher, _) = make_publisher(Arc::clone(&subscriptions)).await; - publisher.register_subscriber(0x5B, 1, 0x01, ADDR_A).await.unwrap(); - publisher.register_subscriber(0x5B, 1, 0x01, ADDR_B).await.unwrap(); + publisher + .register_subscriber(0x5B, 1, 0x01, ADDR_A) + .await + .unwrap(); + publisher + .register_subscriber(0x5B, 1, 0x01, ADDR_B) + .await + .unwrap(); assert!(publisher.has_subscribers(0x5B, 1, 0x01).await); publisher.remove_subscriber(0x5B, 1, 0x01, ADDR_A).await; @@ -789,9 +848,15 @@ mod tests { let subscriptions = Arc::new(RwLock::new(SubscriptionManager::new())); let (publisher, _) = make_publisher(Arc::clone(&subscriptions)).await; - publisher.register_subscriber(0x5B, 1, 0x01, ADDR_A).await.unwrap(); + publisher + .register_subscriber(0x5B, 1, 0x01, ADDR_A) + .await + .unwrap(); publisher.remove_subscriber(0x5B, 1, 0x01, ADDR_A).await; - publisher.register_subscriber(0x5B, 1, 0x01, ADDR_A).await.unwrap(); + publisher + .register_subscriber(0x5B, 1, 0x01, ADDR_A) + .await + .unwrap(); assert_eq!(publisher.subscriber_count(0x5B, 1, 0x01).await, 1); } diff --git a/src/server/mod.rs b/src/server/mod.rs index a5c33cc..9b1ac18 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -364,16 +364,11 @@ impl Server { let entries = [entry]; let options = [option]; - // See the ordering note on `SdStateManager::send_offer_service`: - // advance the session counter first so `has_wrapped` latches, - // then read the reboot flag so the wrap message itself carries - // `Continuous`. - let sid = self.sd_state.next_session_id(); - let sd_payload = sd::Header::new( - Flags::new_sd(self.sd_state.reboot_flag()), - &entries, - &options, - ); + // Atomic (sid, reboot_flag) pair so concurrent emissions cannot + // race around the wrap boundary — see + // `SdStateManager::next_session_id_with_reboot_flag` docs. + let (sid, reboot_flag) = self.sd_state.next_session_id_with_reboot_flag(); + let sd_payload = sd::Header::new(Flags::new_sd(reboot_flag), &entries, &options); let mut sd_data = Vec::new(); sd_payload.encode(&mut sd_data)?; @@ -472,6 +467,15 @@ impl Server { ))); } + // Incoming-peer buffers sized to the IP datagram limit (64 KiB - 1). + // Do NOT shrink to `UDP_BUFFER_SIZE` (1500): peer SD messages are + // bounded by the link MTU but `recv_from` here is a server-side + // sink for any peer datagram landing on the SD/unicast port, and + // larger-than-MTU peer messages must surface (or be cleanly + // truncated by the kernel) rather than being silently capped at + // 1500 by an undersized buffer. Out-going `EventPublisher` paths + // do use the smaller `UDP_BUFFER_SIZE` because we control the + // wire size of what we emit; that asymmetry is intentional. let mut unicast_buf = vec![0u8; 65535]; let mut sd_buf = vec![0u8; 65535]; @@ -481,6 +485,16 @@ impl Server { // `tokio::select!` behavior and avoids starving either the // unicast or SD-multicast arm under sustained one-sided load. // + // SAFETY: both arms are `tokio::net::UdpSocket::recv_from`, + // which is cancel-safe per tokio docs — a non-selected arm + // can be dropped without losing in-flight kernel state. A + // future contributor adding a non-cancel-safe `FusedFuture` + // arm here (e.g. a custom state machine that holds + // partially-read bytes) would silently lose that state when + // the arm is dropped on a select win. Both futures must + // therefore stay `Send + FusedFuture + Unpin` *and* + // cancel-safe. + // // Fresh futures are constructed each iteration so the borrows // of `unicast_buf` / `sd_buf` / the sockets end when the // select macro returns, freeing the buffer we index into @@ -558,6 +572,7 @@ impl Server { } /// Handle a Service Discovery message + #[allow(clippy::too_many_lines)] async fn handle_sd_message( &mut self, sd_view: &sd::SdHeaderView<'_>, @@ -584,7 +599,7 @@ impl Server { self.config.service_id, entry_view.service_id() ); - self.send_subscribe_nack_from_view(&entry_view, sender, "Wrong service ID") + self.send_subscribe_nack_from_view(&entry_view, sender, "wrong_service_id") .await?; } else if entry_view.instance_id() != self.config.instance_id { tracing::warn!( @@ -595,7 +610,26 @@ impl Server { self.send_subscribe_nack_from_view( &entry_view, sender, - "Wrong instance ID", + "wrong_instance_id", + ) + .await?; + } else if entry_view.major_version() != self.config.major_version { + // Per AUTOSAR SOME/IP-SD: a Subscribe whose + // major_version disagrees with the server's + // configured major must be NACKed (TTL=0). Without + // this arm a client probing for a v2 service + // against a v1 server would get an Ack and start + // sending traffic that the application stack + // would silently mis-decode. + tracing::warn!( + "Subscribe for wrong major_version: expected {}, got {}", + self.config.major_version, + entry_view.major_version() + ); + self.send_subscribe_nack_from_view( + &entry_view, + sender, + "wrong_major_version", ) .await?; } else { @@ -651,12 +685,8 @@ impl Server { SubscribeError::EventGroupsFull => "event_groups_full", }; tracing::debug!("Subscription rejected: {reason}"); - self.send_subscribe_nack_from_view( - &entry_view, - sender, - reason, - ) - .await?; + self.send_subscribe_nack_from_view(&entry_view, sender, reason) + .await?; } } } else { @@ -664,7 +694,7 @@ impl Server { self.send_subscribe_nack_from_view( &entry_view, sender, - "No endpoint in options", + "no_endpoint_in_options", ) .await?; } @@ -806,11 +836,10 @@ impl Server { }); let entries = [ack_entry]; - // Ordering: advance the session id first so `has_wrapped` latches - // on the wrap boundary, then read `reboot_flag()` for this - // message — see `SdStateManager::send_offer_service`. - let sid = self.sd_state.next_session_id(); - let sd_payload = sd::Header::new(Flags::new_sd(self.sd_state.reboot_flag()), &entries, &[]); + // Atomic (sid, reboot_flag) pair — see + // `SdStateManager::next_session_id_with_reboot_flag`. + let (sid, reboot_flag) = self.sd_state.next_session_id_with_reboot_flag(); + let sd_payload = sd::Header::new(Flags::new_sd(reboot_flag), &entries, &[]); let mut sd_data = Vec::new(); sd_payload.encode(&mut sd_data)?; @@ -855,10 +884,10 @@ impl Server { }); let entries = [nack_entry]; - // Ordering: advance first so `has_wrapped` latches, then read - // reboot flag — see `SdStateManager::send_offer_service`. - let sid = self.sd_state.next_session_id(); - let sd_payload = sd::Header::new(Flags::new_sd(self.sd_state.reboot_flag()), &entries, &[]); + // Atomic (sid, reboot_flag) pair — see + // `SdStateManager::next_session_id_with_reboot_flag`. + let (sid, reboot_flag) = self.sd_state.next_session_id_with_reboot_flag(); + let sd_payload = sd::Header::new(Flags::new_sd(reboot_flag), &entries, &[]); let mut sd_data = Vec::new(); sd_payload.encode(&mut sd_data)?; @@ -893,7 +922,7 @@ mod tests { #[tokio::test] async fn test_server_creation() { - let config = ServerConfig::new(Ipv4Addr::new(127, 0, 0, 1), 30682, 0x5B, 1); + let config = ServerConfig::new(Ipv4Addr::LOCALHOST, 30682, 0x5B, 1); let server: Result = Server::new(config).await; assert!(server.is_ok()); @@ -905,7 +934,7 @@ mod tests { // when the test binary runs tests in parallel. The SD socket binds // the SD multicast port (30490) and relies on SO_REUSEPORT, the same // as `test_server_creation`. - let config = ServerConfig::new(Ipv4Addr::new(127, 0, 0, 1), 30683, 0x5C, 1); + let config = ServerConfig::new(Ipv4Addr::LOCALHOST, 30683, 0x5C, 1); let server = Server::new_with_loopback(config, true) .await @@ -953,17 +982,18 @@ mod tests { /// Helper: create a server on an ephemeral port and return (Server, port) async fn create_test_server(service_id: u16, instance_id: u16) -> (Server, u16) { // Use port 0 to get an ephemeral port - let config = ServerConfig::new(Ipv4Addr::new(127, 0, 0, 1), 0, service_id, instance_id); + let config = ServerConfig::new(Ipv4Addr::LOCALHOST, 0, service_id, instance_id); let mut server = Server::new(config).await.expect("Failed to create server"); let port = match server.unicast_local_addr().unwrap() { std::net::SocketAddr::V4(addr) => addr.port(), - _ => panic!("Expected IPv4 address"), + std::net::SocketAddr::V6(_) => panic!("expected IPv4 address"), }; // Update config to reflect actual bound port server.set_local_port(port); (server, port) } + #[allow(clippy::too_many_arguments)] fn make_subscription_header( service_id: u16, instance_id: u16, @@ -1009,14 +1039,14 @@ mod tests { 1, 3, 0x01, - Ipv4Addr::new(127, 0, 0, 1), + Ipv4Addr::LOCALHOST, sd::TransportProtocol::Udp, server_port, ); // Send to the server client_socket - .send_to(&message, format!("127.0.0.1:{}", server_port)) + .send_to(&message, format!("127.0.0.1:{server_port}")) .await .unwrap(); @@ -1047,7 +1077,7 @@ mod tests { .unwrap(); let ttl = parse_subscribe_ack_ttl(&resp_buf[..resp_len]); - assert!(ttl > 0, "Expected ACK (TTL > 0), got TTL={}", ttl); + assert!(ttl > 0, "Expected ACK (TTL > 0), got TTL={ttl}"); server_handle.await.unwrap(); } @@ -1063,12 +1093,12 @@ mod tests { 1, 3, 0x01, - Ipv4Addr::new(127, 0, 0, 1), + Ipv4Addr::LOCALHOST, sd::TransportProtocol::Udp, server_port, ); client_socket - .send_to(&message, format!("127.0.0.1:{}", server_port)) + .send_to(&message, format!("127.0.0.1:{server_port}")) .await .unwrap(); @@ -1097,7 +1127,7 @@ mod tests { .unwrap(); let ttl = parse_subscribe_ack_ttl(&resp_buf[..resp_len]); - assert_eq!(ttl, 0, "Expected NACK (TTL=0), got TTL={}", ttl); + assert_eq!(ttl, 0, "Expected NACK (TTL=0), got TTL={ttl}"); server_handle.await.unwrap(); } @@ -1113,12 +1143,12 @@ mod tests { 1, 3, 0x01, - Ipv4Addr::new(127, 0, 0, 1), + Ipv4Addr::LOCALHOST, sd::TransportProtocol::Udp, server_port, ); client_socket - .send_to(&message, format!("127.0.0.1:{}", server_port)) + .send_to(&message, format!("127.0.0.1:{server_port}")) .await .unwrap(); @@ -1144,7 +1174,7 @@ mod tests { .unwrap(); let ttl = parse_subscribe_ack_ttl(&resp_buf[..resp_len]); - assert_eq!(ttl, 0, "Expected NACK (TTL=0), got TTL={}", ttl); + assert_eq!(ttl, 0, "Expected NACK (TTL=0), got TTL={ttl}"); server_handle.await.unwrap(); } @@ -1164,7 +1194,7 @@ mod tests { ); let message = build_sd_message(&sd_header); client_socket - .send_to(&message, format!("127.0.0.1:{}", server_port)) + .send_to(&message, format!("127.0.0.1:{server_port}")) .await .unwrap(); @@ -1215,7 +1245,7 @@ mod tests { ); let message = build_sd_message(&sd_header); client_socket - .send_to(&message, format!("127.0.0.1:{}", server_port)) + .send_to(&message, format!("127.0.0.1:{server_port}")) .await .unwrap(); @@ -1262,7 +1292,7 @@ mod tests { ); let message = build_sd_message(&sd_header); client_socket - .send_to(&message, format!("127.0.0.1:{}", server_port)) + .send_to(&message, format!("127.0.0.1:{server_port}")) .await .unwrap(); @@ -1302,7 +1332,7 @@ mod tests { let message = build_sd_message(&sd_header); client_socket - .send_to(&message, format!("127.0.0.1:{}", server_port)) + .send_to(&message, format!("127.0.0.1:{server_port}")) .await .unwrap(); @@ -1330,7 +1360,7 @@ mod tests { .unwrap(); let ttl = parse_subscribe_ack_ttl(&resp_buf[..resp_len]); - assert_eq!(ttl, 0, "Expected NACK (TTL=0), got TTL={}", ttl); + assert_eq!(ttl, 0, "Expected NACK (TTL=0), got TTL={ttl}"); server_handle.await.unwrap(); } @@ -1388,7 +1418,7 @@ mod tests { let client_socket = UdpSocket::bind("127.0.0.1:0").await.unwrap(); let client_port = match client_socket.local_addr().unwrap() { std::net::SocketAddr::V4(a) => a.port(), - _ => panic!("expected v4"), + std::net::SocketAddr::V6(_) => panic!("expected v4 source address"), }; let subscriptions = Arc::clone(&server.subscriptions); @@ -1410,7 +1440,7 @@ mod tests { let mut non_sd_buf = Vec::new(); non_sd_header.encode(&mut non_sd_buf).unwrap(); client_socket - .send_to(&non_sd_buf, format!("127.0.0.1:{}", server_port)) + .send_to(&non_sd_buf, format!("127.0.0.1:{server_port}")) .await .unwrap(); @@ -1422,12 +1452,12 @@ mod tests { 1, 3, 0x01, - Ipv4Addr::new(127, 0, 0, 1), + Ipv4Addr::LOCALHOST, sd::TransportProtocol::Udp, client_port, ); client_socket - .send_to(&message, format!("127.0.0.1:{}", server_port)) + .send_to(&message, format!("127.0.0.1:{server_port}")) .await .unwrap(); @@ -1442,7 +1472,7 @@ mod tests { .unwrap(); let ttl = parse_subscribe_ack_ttl(&resp_buf[..resp_len]); - assert!(ttl > 0, "Expected ACK (TTL > 0), got TTL={}", ttl); + assert!(ttl > 0, "Expected ACK (TTL > 0), got TTL={ttl}"); // Verify subscription was added (non-SD message was ignored) let subs = subscriptions.read().await; @@ -1457,7 +1487,7 @@ mod tests { let client_socket = UdpSocket::bind("127.0.0.1:0").await.unwrap(); let client_port = match client_socket.local_addr().unwrap() { std::net::SocketAddr::V4(a) => a.port(), - _ => panic!("expected v4"), + std::net::SocketAddr::V6(_) => panic!("expected v4 source address"), }; let subscriptions = Arc::clone(&server.subscriptions); @@ -1468,7 +1498,7 @@ mod tests { // Send garbage bytes client_socket - .send_to(&[0xFF, 0xFE, 0xFD], format!("127.0.0.1:{}", server_port)) + .send_to(&[0xFF, 0xFE, 0xFD], format!("127.0.0.1:{server_port}")) .await .unwrap(); @@ -1480,12 +1510,12 @@ mod tests { 1, 3, 0x01, - Ipv4Addr::new(127, 0, 0, 1), + Ipv4Addr::LOCALHOST, sd::TransportProtocol::Udp, client_port, ); client_socket - .send_to(&message, format!("127.0.0.1:{}", server_port)) + .send_to(&message, format!("127.0.0.1:{server_port}")) .await .unwrap(); @@ -1500,7 +1530,7 @@ mod tests { .unwrap(); let ttl = parse_subscribe_ack_ttl(&resp_buf[..resp_len]); - assert!(ttl > 0, "Expected ACK (TTL > 0), got TTL={}", ttl); + assert!(ttl > 0, "Expected ACK (TTL > 0), got TTL={ttl}"); let subs = subscriptions.read().await; assert_eq!(subs.subscription_count(), 1); @@ -1549,12 +1579,12 @@ mod tests { 1, 3, 0x01, - Ipv4Addr::new(127, 0, 0, 1), + Ipv4Addr::LOCALHOST, sd::TransportProtocol::Udp, server_port.wrapping_add(1), // Subscriber's port, different from server ); client_socket - .send_to(&message, format!("127.0.0.1:{}", server_port)) + .send_to(&message, format!("127.0.0.1:{server_port}")) .await .unwrap(); @@ -1581,7 +1611,7 @@ mod tests { .unwrap(); let ttl = parse_subscribe_ack_ttl(&resp_buf[..resp_len]); - assert!(ttl > 0, "Expected ACK (TTL > 0), got TTL={}", ttl); + assert!(ttl > 0, "Expected ACK (TTL > 0), got TTL={ttl}"); server_handle.await.unwrap(); } @@ -2262,18 +2292,18 @@ mod tests { }); } - /// Smoke test for [`Server::start_announcing`]: a loopback server with - /// `multicast_loop` enabled should emit at least one `OfferService` on - /// the SD multicast group within a couple of seconds. + /// Smoke test for [`Server::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. /// /// `#[ignore]`d for the same reason as the `sd_state` tests — hosts /// without the MULTICAST flag on `lo` drop the packet silently. The - /// spawned announcer task keeps running until runtime teardown; that - /// is intentional (there is no stop API on `Server`) and harmless in - /// a `#[tokio::test]`. + /// announcer task is captured and aborted at the end of the test so + /// it does not leak multicast traffic into other parallel tests. #[ignore = "requires loopback multicast support (MULTICAST on lo)"] #[tokio::test] - async fn start_announcing_emits_first_offer_within_timeout() { + async fn announcement_loop_emits_first_offer_within_timeout() { use crate::protocol::MessageView; use crate::protocol::sd::EntryType; @@ -2307,7 +2337,7 @@ mod tests { let announce_fut = server .announcement_loop() .expect("announcement_loop should build on a non-passive server"); - tokio::spawn(announce_fut); + let announce_handle = tokio::spawn(announce_fut); // Scan the multicast group for our OfferService. The first tick // happens immediately; 2s is ample headroom for scheduler jitter. @@ -2337,6 +2367,8 @@ mod tests { }; tokio::time::timeout(std::time::Duration::from_secs(2), recv_loop) .await - .expect("start_announcing should emit at least one OfferService within 2s"); + .expect("announcement_loop should emit at least one OfferService within 2s"); + announce_handle.abort(); + let _ = announce_handle.await; } } diff --git a/src/server/sd_state.rs b/src/server/sd_state.rs index 11e7ea5..803f7bf 100644 --- a/src/server/sd_state.rs +++ b/src/server/sd_state.rs @@ -53,11 +53,23 @@ impl SdStateManager { } /// Advance the counter and return the next SOME/IP-SD session ID - /// (`client_id = 0`, session ID in the low 16 bits). Skips 0 on wrap, + /// (`client_id = 0`, session ID in the low 16 bits) together with the + /// reboot flag that *belongs to this same emission*. Skips 0 on wrap, /// and latches [`Self::has_wrapped`] the first time the counter crosses /// the `0xFFFF → 0x0001` boundary so the reboot flag flips to /// [`RebootFlag::Continuous`] permanently. - pub(super) fn next_session_id(&self) -> u32 { + /// + /// Returns `(session_id, reboot_flag)` as a tuple to avoid a TOCTOU + /// race around the wrap boundary: a separate `next_session_id() + + /// reboot_flag()` call pair could see thread A's pre-wrap session + /// ID and thread B's post-wrap latched flag (or the inverse), and + /// thus advertise `Continuous` with `session_id=0xFFFF` (or + /// `RecentlyRebooted` with `session_id=0x0001`) — both violations + /// of AUTOSAR SOME/IP-SD's stated semantics that the wrap message + /// itself carries `Continuous`. By computing both inside the same + /// `fetch_update` closure, the pair is consistent for every + /// individual emission. + pub(super) fn next_session_id_with_reboot_flag(&self) -> (u32, RebootFlag) { let prev = self .session_id .fetch_update(Ordering::Relaxed, Ordering::Relaxed, |v| { @@ -66,25 +78,48 @@ impl SdStateManager { }) .unwrap(); // The only value whose successor wraps through 0 is 0xFFFF; latch - // the flag exactly on that transition. + // the flag exactly on that transition. We then read the flag for + // this emission AFTER the latch, so the wrap message itself + // advertises `Continuous`. if prev == u16::MAX { self.has_wrapped.store(true, Ordering::Relaxed); } let next = prev.wrapping_add(1); - u32::from(if next == 0 { 1 } else { next }) + let session_id = u32::from(if next == 0 { 1 } else { next }); + let reboot_flag = if self.has_wrapped.load(Ordering::Relaxed) { + RebootFlag::Continuous + } else { + RebootFlag::RecentlyRebooted + }; + (session_id, reboot_flag) + } + + /// Convenience: advance the counter and return only the session id. + /// Use [`Self::next_session_id_with_reboot_flag`] when the same + /// emission also needs the reboot flag — calling these two methods + /// separately races around the wrap boundary. Only used by unit + /// tests; production paths take the atomic pair. + #[cfg(test)] + pub(super) fn next_session_id(&self) -> u32 { + self.next_session_id_with_reboot_flag().0 } /// Current SD reboot flag for this server. /// /// Returns [`RebootFlag::RecentlyRebooted`] until the session counter /// has wrapped past `0xFFFF` at least once, then - /// [`RebootFlag::Continuous`] permanently. Every server-side SD - /// emission path ([`Self::send_offer_service`], plus the unicast - /// offer / `SubscribeAck` / `SubscribeNack` paths in - /// [`crate::server::Server`]) calls this so the flag on the wire - /// reflects a single tracked state. + /// [`RebootFlag::Continuous`] permanently. Production emission paths + /// must use [`Self::next_session_id_with_reboot_flag`] instead to + /// avoid a TOCTOU race around the wrap boundary; this accessor is + /// `#[cfg(test)]`-only so future code cannot accidentally reach for + /// the racy pair. + #[cfg(test)] pub(super) fn reboot_flag(&self) -> RebootFlag { - RebootFlag::from(!self.has_wrapped.load(Ordering::Relaxed)) + if self.has_wrapped.load(Ordering::Relaxed) { + RebootFlag::Continuous + } else { + RebootFlag::RecentlyRebooted + } } /// Send a multicast `OfferService` announcement for the given config. @@ -115,15 +150,12 @@ impl SdStateManager { let entries = [entry]; let options = [option]; - // Advance the session counter FIRST so `has_wrapped` latches on - // the wrap transition, then derive the reboot flag for this - // same message. Without this ordering the message carrying - // session_id=0x0001 after a wrap would still advertise - // `RebootFlag::RecentlyRebooted`, and the flip would only land - // on the NEXT emission — violating AUTOSAR SOME/IP-SD semantics - // (the wrap message itself should carry `Continuous`). - let sid = self.next_session_id(); - let sd_payload = sd::Header::new(Flags::new_sd(self.reboot_flag()), &entries, &options); + // Atomic (sid, reboot_flag) pair so that concurrent emissions + // around the wrap boundary cannot disagree about whether this + // very message advertises `RecentlyRebooted` or `Continuous`. + // See `next_session_id_with_reboot_flag` docs for the race. + let (sid, reboot_flag) = self.next_session_id_with_reboot_flag(); + let sd_payload = sd::Header::new(Flags::new_sd(reboot_flag), &entries, &options); let mut sd_data = Vec::new(); sd_payload.encode(&mut sd_data)?; diff --git a/src/server/subscription_manager.rs b/src/server/subscription_manager.rs index ca181c5..bdf548c 100644 --- a/src/server/subscription_manager.rs +++ b/src/server/subscription_manager.rs @@ -12,16 +12,29 @@ const EVENT_GROUPS_CAP: usize = 32; /// with a `warn!` log rather than silently. const SUBSCRIBERS_PER_GROUP: usize = 16; +// Compile-time invariants. Trip these at `cargo build` so that retuning +// the constants above can't quietly produce a `subscribe` impl that +// panics on first push (zero `SUBSCRIBERS_PER_GROUP`) or that fails the +// `heapless::FnvIndexMap` build (non-power-of-two `EVENT_GROUPS_CAP`). +const _: () = assert!( + SUBSCRIBERS_PER_GROUP >= 1, + "SUBSCRIBERS_PER_GROUP must be >= 1: a value of 0 would crash subscribe() on first push" +); +const _: () = assert!( + EVENT_GROUPS_CAP.is_power_of_two(), + "EVENT_GROUPS_CAP must be a power of two for heapless::FnvIndexMap" +); + /// Why a call to [`SubscriptionManager::subscribe`] failed to record a new /// subscriber. Callers (typically the server's `Subscribe` handler) should /// use this to emit a `SubscribeNack` instead of a misleading `SubscribeAck`. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum SubscribeError { /// The per-event-group subscriber list is already full - /// ([`SUBSCRIBERS_PER_GROUP`] entries). The caller's request was not + /// (`SUBSCRIBERS_PER_GROUP` entries). The caller's request was not /// recorded. SubscribersPerGroupFull, - /// The outer event-group map is already full ([`EVENT_GROUPS_CAP`] + /// The outer event-group map is already full (`EVENT_GROUPS_CAP` /// distinct `(service_id, instance_id, event_group_id)` keys). The /// caller's request was not recorded. EventGroupsFull, @@ -45,8 +58,8 @@ type SubscribersList = HeaplessVec; /// Manages subscriptions to event groups. /// -/// Capacity is bounded at compile time: up to [`EVENT_GROUPS_CAP`] distinct -/// event groups, each with up to [`SUBSCRIBERS_PER_GROUP`] subscribers. +/// Capacity is bounded at compile time: up to `EVENT_GROUPS_CAP` distinct +/// event groups, each with up to `SUBSCRIBERS_PER_GROUP` subscribers. #[derive(Debug)] pub struct SubscriptionManager { /// Map of (`service_id`, `instance_id`, `event_group_id`) -> list of subscribers @@ -77,6 +90,17 @@ impl SubscriptionManager { /// (typically the server's `Subscribe` handler) should send a /// `SubscribeNack` on `Err`, not a `SubscribeAck`. /// + /// # Errors + /// + /// Returns: + /// - `SubscribeError::SubscribersPerGroupFull` when an existing event + /// group already has `SUBSCRIBERS_PER_GROUP` subscribers and this + /// call would push a new one. + /// - `SubscribeError::EventGroupsFull` when this is the first + /// subscriber for a previously-unseen `(service_id, instance_id, + /// event_group_id)` triple but the outer event-group map is full + /// (`EVENT_GROUPS_CAP` distinct groups). + /// /// # Panics /// /// Panics if `SUBSCRIBERS_PER_GROUP == 0`, a compile-time constant that diff --git a/src/tokio_transport.rs b/src/tokio_transport.rs index c19fb95..f53ca6b 100644 --- a/src/tokio_transport.rs +++ b/src/tokio_transport.rs @@ -67,9 +67,7 @@ impl TokioSocket { /// Returns [`TransportError`] if the backend cannot read the flag. #[allow(dead_code)] // used in tests; kept available for field debugging. pub(crate) fn multicast_loop_v4(&self) -> Result { - self.inner - .multicast_loop_v4() - .map_err(|e| map_io_error(&e)) + self.inner.multicast_loop_v4().map_err(|e| map_io_error(&e)) } } @@ -88,9 +86,10 @@ pub struct TokioTimer; /// [`crate::transport::Spawner`] impl that routes submitted futures /// to `tokio::spawn`. /// -/// Zero-size unit struct; every `Inner` / `Client` pays nothing for the abstraction. Bare-metal -/// consumers substitute their own `Spawner` via the +/// Zero-size unit struct; every `Inner` / `Client

` +/// pays nothing for the abstraction (the `Inner` carries the spawner +/// generic; `Client

` is a thin handle that forwards to it). +/// Bare-metal consumers substitute their own `Spawner` via the /// `crate::Client::new_with_spawner_and_loopback` constructor. #[derive(Debug, Default, Clone, Copy)] pub struct TokioSpawner; @@ -111,11 +110,7 @@ impl TransportFactory for TokioTransport { } impl TransportSocket for TokioSocket { - async fn send_to( - &self, - buf: &[u8], - target: SocketAddrV4, - ) -> Result<(), TransportError> { + async fn send_to(&self, buf: &[u8], target: SocketAddrV4) -> Result<(), TransportError> { self.inner .send_to(buf, target) .await @@ -123,10 +118,7 @@ impl TransportSocket for TokioSocket { .map_err(|e| map_io_error(&e)) } - async fn recv_from( - &self, - buf: &mut [u8], - ) -> Result { + async fn recv_from(&self, buf: &mut [u8]) -> Result { let (n, src) = self .inner .recv_from(buf) @@ -165,21 +157,13 @@ impl TransportSocket for TokioSocket { } } - fn join_multicast_v4( - &self, - group: Ipv4Addr, - iface: Ipv4Addr, - ) -> Result<(), TransportError> { + fn join_multicast_v4(&self, group: Ipv4Addr, iface: Ipv4Addr) -> Result<(), TransportError> { self.inner .join_multicast_v4(group, iface) .map_err(|e| map_io_error(&e)) } - fn leave_multicast_v4( - &self, - group: Ipv4Addr, - iface: Ipv4Addr, - ) -> Result<(), TransportError> { + fn leave_multicast_v4(&self, group: Ipv4Addr, iface: Ipv4Addr) -> Result<(), TransportError> { self.inner .leave_multicast_v4(group, iface) .map_err(|e| map_io_error(&e)) @@ -205,8 +189,10 @@ impl crate::transport::Spawner for TokioSpawner { /// Synchronously create and configure a UDP socket via `socket2`, then /// hand it to tokio. Mirrors the existing bind paths in -/// [`crate::client::socket_manager`] and [`crate::server`] so behavior is -/// identical. +/// `crate::client::socket_manager` and `crate::server` (rendered as +/// code literals because both are feature-gated and would break +/// default-feature rustdoc builds via broken intra-doc links) so +/// behavior is identical. fn bind_with_options(addr: SocketAddrV4, options: SocketOptions) -> std::io::Result { let raw = socket2::Socket::new( socket2::Domain::IPV4, diff --git a/src/traits.rs b/src/traits.rs index abd3134..6cd8c2f 100644 --- a/src/traits.rs +++ b/src/traits.rs @@ -103,11 +103,14 @@ pub trait PayloadWireFormat: core::fmt::Debug + Send + Sized + Sync { /// Override the reboot flag on an SD header in-place. /// - /// Used by `Client::start_sd_announcements` (when the `client` feature is - /// enabled) to refresh the reboot flag per-tick from the client's tracked - /// state. + /// Used by `Client::sd_announcements_loop` (when the `client` feature is + /// enabled) to refresh the reboot flag per-tick from the client's + /// tracked state. Defaults to a no-op so that `std`-but-not-`client` + /// consumers (e.g. host-side tooling that builds SD headers manually + /// without ever driving an announcement loop) don't have to provide + /// an impl that will never be called. #[cfg(feature = "std")] - fn set_reboot_flag(header: &mut Self::SdHeader, reboot: sd::RebootFlag); + fn set_reboot_flag(_header: &mut Self::SdHeader, _reboot: sd::RebootFlag) {} /// Extract offered/stopped service endpoints from this SD payload. /// diff --git a/src/transport.rs b/src/transport.rs index 85da95b..acbeedf 100644 --- a/src/transport.rs +++ b/src/transport.rs @@ -17,11 +17,19 @@ //! //! Three explicit design choices: //! -//! 1. **Executor-agnostic.** Methods return `impl Future`, not `async fn`, -//! and the traits make no statement about `Send` or `'static` bounds on -//! the returned futures. Callers that need those bounds (e.g. to -//! `tokio::spawn`) require them at the consumer site. Bare-metal callers -//! driving the future on a single executor task pay no `Send` tax. +//! 1. **Executor-agnostic for socket / timer I/O.** [`TransportSocket`] +//! and [`Timer`] methods return `impl Future`, not `async fn`, and +//! those traits make no statement about `Send` or `'static` bounds on +//! their returned futures. Callers that need those bounds (e.g. to +//! `tokio::spawn`) require them at the consumer site. Bare-metal +//! callers driving the future on a single executor task pay no `Send` +//! tax for socket I/O. **[`Spawner::spawn`] is the deliberate +//! exception:** it is a multi-task abstraction by definition, so it +//! requires `Send + 'static` on its argument. Single-core executors +//! that need a `!Send` variant (embassy with `task_arena_size = 0`, +//! `LocalSet`-style models) need either a future `spawn_local` shim +//! or a hand-rolled adapter; the `Send + 'static` bound is documented +//! on the trait method itself. //! 2. **IPv4-only address type.** This transport abstraction currently //! uses [`core::net::SocketAddrV4`] directly rather than `SocketAddr`, //! matching the crate's present transport-layer reach for unicast and @@ -309,11 +317,19 @@ impl Default for SocketOptions { /// The result of a successful [`TransportSocket::recv_from`]. /// /// `truncated` is set if the backend delivered only a prefix of the -/// incoming datagram because it did not fit in the caller's buffer. -/// On backends that size `buf` at least as large as the link MTU (the -/// expected configuration — see [`crate::UDP_BUFFER_SIZE`]), truncation -/// should not occur in practice; the field exists so backends that cannot -/// guarantee this can surface it explicitly instead of silently dropping. +/// incoming datagram because it did not fit in the caller's buffer. If +/// callers use a buffer sized to [`crate::UDP_BUFFER_SIZE`], truncation is +/// generally not expected on backends whose delivered datagrams are +/// bounded by that configured application-level cap. Backends that may +/// deliver larger datagrams should surface this explicitly instead of +/// silently dropping the fact that data was discarded. +/// +/// Note: the default Tokio backend currently always reports +/// `truncated: false` because `tokio::net::UdpSocket::recv_from` does not +/// expose `MSG_TRUNC` (or equivalent). Reliable truncation detection +/// requires a backend that does — e.g. a `recvmsg`-based backend, or a +/// `no_std` stack like smoltcp / embassy-net that surfaces the original +/// datagram length. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct ReceivedDatagram { /// Number of bytes written to the caller's buffer. @@ -321,15 +337,20 @@ pub struct ReceivedDatagram { /// Source address of the datagram. pub source: SocketAddrV4, /// `true` if the incoming datagram was larger than the caller's - /// buffer and the tail was discarded. + /// buffer and the tail was discarded. See the type-level docs for + /// the default Tokio backend's caveat. pub truncated: bool, } /// A bound, configured UDP socket usable for SOME/IP message exchange. /// -/// Implementations are obtained via [`TransportFactory::bind`]. All I/O -/// methods return `impl Future` so the trait is executor-agnostic; the -/// caller awaits them on whatever runtime it owns. +/// Implementations are obtained via [`TransportFactory::bind`]. The +/// send/receive methods return `impl Future` so the trait is +/// executor-agnostic; the caller awaits them on whatever runtime it +/// owns. The smaller socket-level queries ([`Self::local_addr`], +/// [`Self::join_multicast_v4`], [`Self::leave_multicast_v4`]) are +/// synchronous because they are typically O(1) lookups on a backend's +/// internal handle and do not benefit from yielding to the executor. /// /// Multicast group membership is joined *after* bind via /// [`TransportSocket::join_multicast_v4`]; the bind-time @@ -413,8 +434,7 @@ pub trait TransportSocket { /// Returns [`TransportError::Unsupported`] if the backend has no /// multicast support; otherwise [`TransportError::Io`] with an /// appropriate kind. - fn join_multicast_v4(&self, group: Ipv4Addr, iface: Ipv4Addr) - -> Result<(), TransportError>; + fn join_multicast_v4(&self, group: Ipv4Addr, iface: Ipv4Addr) -> Result<(), TransportError>; /// Leave IPv4 multicast group `group` on interface `iface`. Symmetric /// to [`Self::join_multicast_v4`]. Most backends implicitly leave on @@ -426,15 +446,14 @@ pub trait TransportSocket { /// Returns [`TransportError::Unsupported`] if the backend has no /// multicast support; otherwise [`TransportError::Io`] with an /// appropriate kind. - fn leave_multicast_v4( - &self, - group: Ipv4Addr, - iface: Ipv4Addr, - ) -> Result<(), TransportError>; + fn leave_multicast_v4(&self, group: Ipv4Addr, iface: Ipv4Addr) -> Result<(), TransportError>; /// Upper bound, in bytes, on datagrams this socket will successfully /// accept in `send_to` or return via `recv_from`. The default returns - /// [`crate::UDP_BUFFER_SIZE`] (1500), matching standard Ethernet MTU. + /// [`crate::UDP_BUFFER_SIZE`], the crate's default application-level + /// UDP payload cap (currently 1500 bytes — note that this is *not* + /// MTU-safe; see [`crate::UDP_BUFFER_SIZE`]'s own docs for the + /// IPv4/IPv6 header overhead). /// /// Backends with a smaller effective MTU (for example, some /// resource-constrained embedded stacks) should override this to @@ -547,13 +566,30 @@ pub trait Spawner { /// demonstrates the wrong pattern (drops the future) and annotates /// it as DEMO-ONLY for exactly this reason. /// + /// # Fire-and-forget by design + /// + /// `spawn` returns `()`, not a join-handle. The rest of the crate + /// observes `tokio::JoinHandle`s wherever it spawns work directly + /// (commit `d92c5a3`); this trait is the deliberate exception. The + /// per-socket loops have no observable result — they run forever and + /// only exit when their owning `SocketManager` drops its channel + /// ends — so a join-handle would just be storage with no callers. + /// A future revision MAY add an associated `Handle` type if a + /// concrete shutdown / cancellation use case appears; today there is + /// none. + /// /// # Bound rationale /// - /// The `Send + 'static` bound matches every mainstream multi-task - /// executor (tokio, async-std, smol, embassy with task arenas). - /// Bare-metal executors that use single-threaded task pools may - /// want to loosen this — a future release may add a - /// `spawn_local`-style variant gated on a cargo feature. + /// The `Send + 'static` bound matches multi-threaded executors like + /// tokio, async-std, and smol — the captured per-socket loop is + /// already `Send + 'static` because its underlying `TokioSocket` is. + /// Embassy and other `no_alloc` / single-core executors typically need + /// additional adapter scaffolding (a typed `SpawnToken`, a static + /// task arena, hardware-specific waker plumbing) to satisfy + /// `Send + 'static`; the example at the top of this docstring has a + /// `todo!()` precisely because the adapter is not one-line. A future + /// release MAY add a `spawn_local`-style variant gated on a cargo + /// feature for those targets. fn spawn(&self, future: impl Future + Send + 'static); } diff --git a/tests/client_server.rs b/tests/client_server.rs index 9e0388c..15f6a12 100644 --- a/tests/client_server.rs +++ b/tests/client_server.rs @@ -1,10 +1,46 @@ //! Integration tests exercising the Client and Server together on localhost. +//! +//! # Parallel execution caveat +//! +//! These tests share `sd::MULTICAST_PORT` (30490) and bind it via +//! `SO_REUSEPORT`. Linux's reuseport hashing then load-balances incoming +//! Subscribe / SD multicast traffic across whichever sockets are +//! currently bound, which means one test's Subscribe message can be +//! delivered to a *different* test's server. Each test verifies its own +//! `EventPublisher::has_subscribers` (per-server `SubscriptionManager` +//! state, not a shared one), so the cross-routing produces flaky +//! failures when the suite runs with cargo's default parallelism. +//! +//! Until we can give each test its own SD port (which would require +//! widening the protocol layer's `MULTICAST_PORT` constant to a runtime +//! config) or its own network namespace, **run this binary with +//! `--test-threads=1`** to serialise the SD-port contention: +//! +//! ```text +//! cargo test --test client_server -- --test-threads=1 +//! ``` +//! +//! `cargo test --workspace` (parallel default) is expected to flake on +//! ~half of the tests in this file. The unit-test suite under +//! `cargo test --lib` does not have this issue and runs reliably in +//! parallel. The fix is tracked alongside the phase 10+ bare-metal +//! refactor (which will need to abstract the port anyway). use simple_someip::e2e::{E2ECheckStatus, E2EKey, E2EProfile, Profile4Config}; use simple_someip::protocol::{Header, Message, MessageId, sd}; use simple_someip::server::ServerConfig; use simple_someip::{Client, ClientUpdate, PayloadWireFormat, RawPayload, Server, VecSdHeader}; use std::net::{Ipv4Addr, SocketAddrV4}; +use std::sync::atomic::{AtomicU16, Ordering}; + +/// Allocate a unique service ID per test invocation. Multiple +/// integration tests in this file run in parallel (cargo's default) and +/// would otherwise collide on the SD multicast group + a shared service +/// ID, causing cross-test SubscribeAck bleed-through. +fn next_service_id() -> u16 { + static NEXT: AtomicU16 = AtomicU16::new(0x5B); + NEXT.fetch_add(1, Ordering::Relaxed) +} fn empty_sd_header() -> VecSdHeader { VecSdHeader { @@ -51,19 +87,26 @@ async fn wait_for_subscribers( #[tokio::test] async fn test_client_server_subscribe_and_receive_event() { // Start server on ephemeral port - let (mut server, server_port) = create_server(0x5B, 1).await; + let service_id = next_service_id(); + let (mut server, server_port) = create_server(service_id, 1).await; let publisher = server.publisher(); let server_handle = tokio::spawn(async move { server.run().await }); // Create client and subscribe to the server's event group let (client, mut updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let server_addr = SocketAddrV4::new(Ipv4Addr::LOCALHOST, server_port); - client.add_endpoint(0x5B, 1, server_addr, 0).await.unwrap(); - client.subscribe(0x5B, 1, 1, 3, 0x01, 0).await.unwrap(); + client + .add_endpoint(service_id, 1, server_addr, 0) + .await + .unwrap(); + client + .subscribe(service_id, 1, 1, 3, 0x01, 0) + .await + .unwrap(); assert!( - wait_for_subscribers(&publisher, 0x5B, 1, 0x01).await, + wait_for_subscribers(&publisher, service_id, 1, 0x01).await, "server should have registered the subscriber" ); @@ -73,7 +116,7 @@ async fn test_client_server_subscribe_and_receive_event() { // Publish an event from the server to the client's unicast port let event_msg = Message::::new_sd(0x0001, &empty_sd_header()); let sent = publisher - .publish_event(0x5B, 1, 0x01, &event_msg) + .publish_event(service_id, 1, 0x01, &event_msg) .await .expect("publish_event failed"); assert_eq!(sent, 1); @@ -96,18 +139,19 @@ async fn test_client_server_subscribe_and_receive_event() { #[tokio::test] async fn test_client_send_sd_auto_binds_discovery() { // Create server so there is something to send to - let (mut server, server_port) = create_server(0x5B, 1).await; + let service_id = next_service_id(); + let (mut server, server_port) = create_server(service_id, 1).await; let server_handle = tokio::spawn(async move { server.run().await }); // Create client — NO bind_discovery let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); // send_sd_message should auto-bind discovery and succeed let sd_header = VecSdHeader { flags: sd::Flags::new_sd(sd::RebootFlag::RecentlyRebooted), entries: vec![sd::Entry::SubscribeEventGroup(sd::EventGroupEntry::new( - 0x5B, 1, 1, 3, 0x01, + service_id, 1, 1, 3, 0x01, ))], options: vec![sd::Options::IpV4Endpoint { ip: Ipv4Addr::LOCALHOST, @@ -129,17 +173,24 @@ async fn test_client_send_sd_auto_binds_discovery() { /// while an SD message round-trip is in flight. #[tokio::test] async fn test_client_bind_unbind_lifecycle_with_server() { - let (mut server, server_port) = create_server(0x5B, 1).await; + let service_id = next_service_id(); + let (mut server, server_port) = create_server(service_id, 1).await; let server_handle = tokio::spawn(async move { server.run().await }); let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); // Bind discovery, subscribe, then unbind and rebind client.bind_discovery().await.unwrap(); let server_addr = SocketAddrV4::new(Ipv4Addr::LOCALHOST, server_port); - client.add_endpoint(0x5B, 1, server_addr, 0).await.unwrap(); - client.subscribe(0x5B, 1, 1, 3, 0x01, 0).await.unwrap(); + client + .add_endpoint(service_id, 1, server_addr, 0) + .await + .unwrap(); + client + .subscribe(service_id, 1, 1, 3, 0x01, 0) + .await + .unwrap(); // Unbind and rebind discovery — covers unbind_discovery + re-bind path client.unbind_discovery().await.unwrap(); @@ -157,24 +208,31 @@ async fn test_client_bind_unbind_lifecycle_with_server() { /// registry, auto-binds unicast, sends the request, and receives a response. #[tokio::test] async fn test_add_endpoint_and_send_to_service() { - let (mut server, server_port) = create_server(0x5B, 1).await; + let service_id = next_service_id(); + let (mut server, server_port) = create_server(service_id, 1).await; let publisher = server.publisher(); let server_handle = tokio::spawn(async move { server.run().await }); let (client, mut updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); client.bind_discovery().await.unwrap(); // Register the server's endpoint manually (simulating non-broadcasting service) let server_addr = SocketAddrV4::new(Ipv4Addr::LOCALHOST, server_port); - client.add_endpoint(0x5B, 1, server_addr, 0).await.unwrap(); + client + .add_endpoint(service_id, 1, server_addr, 0) + .await + .unwrap(); // Subscribe to server's event group (auto-binds unicast internally) - client.subscribe(0x5B, 1, 1, 3, 0x01, 0).await.unwrap(); + client + .subscribe(service_id, 1, 1, 3, 0x01, 0) + .await + .unwrap(); // Wait for the server to process the subscription assert!( - wait_for_subscribers(&publisher, 0x5B, 1, 0x01).await, + wait_for_subscribers(&publisher, service_id, 1, 0x01).await, "server should have registered the subscriber" ); @@ -184,7 +242,7 @@ async fn test_add_endpoint_and_send_to_service() { // Publish an event from the server let event_msg = Message::::new_sd(0x0001, &empty_sd_header()); let sent = publisher - .publish_event(0x5B, 1, 0x01, &event_msg) + .publish_event(service_id, 1, 0x01, &event_msg) .await .expect("publish_event failed"); assert_eq!(sent, 1); @@ -199,9 +257,9 @@ async fn test_add_endpoint_and_send_to_service() { ); // Remove the endpoint and verify send_to_service returns ServiceNotFound - client.remove_endpoint(0x5B, 1).await.unwrap(); + client.remove_endpoint(service_id, 1).await.unwrap(); let msg = Message::::new_sd(0x0001, &empty_sd_header()); - let result = client.send_to_service(0x5B, 1, msg).await; + let result = client.send_to_service(service_id, 1, msg).await; assert!( matches!(result, Err(simple_someip::client::Error::ServiceNotFound)), "expected ServiceNotFound after remove, got {result:?}" @@ -217,20 +275,27 @@ async fn test_add_endpoint_and_send_to_service() { /// Exercises the Subscribe auto-bind discovery path in inner.rs. #[tokio::test] async fn test_subscribe_auto_binds_discovery() { - let (mut server, server_port) = create_server(0x5B, 1).await; + let service_id = next_service_id(); + let (mut server, server_port) = create_server(service_id, 1).await; let publisher = server.publisher(); let server_handle = tokio::spawn(async move { server.run().await }); // Create client — do NOT bind discovery manually let (client, mut updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let server_addr = SocketAddrV4::new(Ipv4Addr::LOCALHOST, server_port); - client.add_endpoint(0x5B, 1, server_addr, 0).await.unwrap(); + client + .add_endpoint(service_id, 1, server_addr, 0) + .await + .unwrap(); // Subscribe should auto-bind discovery internally - client.subscribe(0x5B, 1, 1, 3, 0x01, 0).await.unwrap(); + client + .subscribe(service_id, 1, 1, 3, 0x01, 0) + .await + .unwrap(); assert!( - wait_for_subscribers(&publisher, 0x5B, 1, 0x01).await, + wait_for_subscribers(&publisher, service_id, 1, 0x01).await, "server should have registered the subscriber" ); @@ -240,7 +305,7 @@ async fn test_subscribe_auto_binds_discovery() { // Publish an event and verify the client can receive it let event_msg = Message::::new_sd(0x0001, &empty_sd_header()); let sent = publisher - .publish_event(0x5B, 1, 0x01, &event_msg) + .publish_event(service_id, 1, 0x01, &event_msg) .await .expect("publish_event failed"); assert_eq!(sent, 1); @@ -261,18 +326,25 @@ async fn test_subscribe_auto_binds_discovery() { /// Exercises the pending_responses HashMap matching path in inner.rs. #[tokio::test] async fn test_client_request_resolves_via_unicast_reply() { - let (mut server, server_port) = create_server(0x5B, 1).await; + let service_id = next_service_id(); + let (mut server, server_port) = create_server(service_id, 1).await; let publisher = server.publisher(); let server_handle = tokio::spawn(async move { server.run().await }); let (client, mut updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let server_addr = SocketAddrV4::new(Ipv4Addr::LOCALHOST, server_port); - client.add_endpoint(0x5B, 1, server_addr, 0).await.unwrap(); - client.subscribe(0x5B, 1, 1, 3, 0x01, 0).await.unwrap(); + client + .add_endpoint(service_id, 1, server_addr, 0) + .await + .unwrap(); + client + .subscribe(service_id, 1, 1, 3, 0x01, 0) + .await + .unwrap(); assert!( - wait_for_subscribers(&publisher, 0x5B, 1, 0x01).await, + wait_for_subscribers(&publisher, service_id, 1, 0x01).await, "server should have registered the subscriber" ); @@ -283,14 +355,14 @@ async fn test_client_request_resolves_via_unicast_reply() { // which has a matching request_id, resolving it. let msg = Message::::new_sd(0x0001, &empty_sd_header()); let pending = client - .send_to_service(0x5B, 1, msg) + .send_to_service(service_id, 1, msg) .await .expect("send_to_service failed"); // Publish an event that the client unicast socket will receive let event_msg = Message::::new_sd(0x0001, &empty_sd_header()); publisher - .publish_event(0x5B, 1, 0x01, &event_msg) + .publish_event(service_id, 1, 0x01, &event_msg) .await .expect("publish_event failed"); @@ -314,12 +386,13 @@ async fn test_client_request_resolves_via_unicast_reply() { /// Exercises E2E protect in event_publisher.rs and E2E check in socket_manager.rs. #[tokio::test] async fn test_e2e_protect_on_publish_and_check_on_receive() { - let (mut server, server_port) = create_server(0x5B, 1).await; + let service_id = next_service_id(); + let (mut server, server_port) = create_server(service_id, 1).await; let publisher = server.publisher(); // Register E2E profile on server for the event message ID let key = E2EKey { - service_id: 0x5B, + service_id, method_or_event_id: 0x0001, }; let profile = E2EProfile::Profile4(Profile4Config::new(0x12345678, 15)); @@ -328,17 +401,23 @@ async fn test_e2e_protect_on_publish_and_check_on_receive() { let server_handle = tokio::spawn(async move { server.run().await }); let (client, mut updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); // Register matching E2E profile on client client.register_e2e(key, profile); let server_addr = SocketAddrV4::new(Ipv4Addr::LOCALHOST, server_port); - client.add_endpoint(0x5B, 1, server_addr, 0).await.unwrap(); - client.subscribe(0x5B, 1, 1, 3, 0x01, 0).await.unwrap(); + client + .add_endpoint(service_id, 1, server_addr, 0) + .await + .unwrap(); + client + .subscribe(service_id, 1, 1, 3, 0x01, 0) + .await + .unwrap(); assert!( - wait_for_subscribers(&publisher, 0x5B, 1, 0x01).await, + wait_for_subscribers(&publisher, service_id, 1, 0x01).await, "server should have registered the subscriber" ); @@ -346,14 +425,14 @@ async fn test_e2e_protect_on_publish_and_check_on_receive() { let _ = tokio::time::timeout(std::time::Duration::from_millis(250), updates.recv()).await; // Publish an event — server will E2E-protect it - // Construct a non-SD message with service_id=0x5B, method/event_id=0x0001 + // Construct a non-SD message with service_id=service_id, method/event_id=0x0001 let payload_bytes = [0xAA, 0xBB]; - let msg_id = MessageId::new_from_service_and_method(0x5B, 0x0001); + let msg_id = MessageId::new_from_service_and_method(service_id, 0x0001); let raw_payload = RawPayload::from_payload_bytes(msg_id, &payload_bytes).unwrap(); - let header = Header::new_event(0x5B, 0x0001, 0, 0x01, 0x01, payload_bytes.len()); + let header = Header::new_event(service_id, 0x0001, 0, 0x01, 0x01, payload_bytes.len()); let event_msg = Message::new(header, raw_payload); let sent = publisher - .publish_event(0x5B, 1, 0x01, &event_msg) + .publish_event(service_id, 1, 0x01, &event_msg) .await .expect("publish_event failed"); assert_eq!(sent, 1); @@ -385,7 +464,8 @@ async fn test_e2e_protect_on_publish_and_check_on_receive() { /// Exercises multi-subscriber path in event_publisher.rs. #[tokio::test] async fn test_multiple_subscribers_receive_events() { - let (mut server, server_port) = create_server(0x5B, 1).await; + let service_id = next_service_id(); + let (mut server, server_port) = create_server(service_id, 1).await; let publisher = server.publisher(); let server_handle = tokio::spawn(async move { server.run().await }); @@ -394,24 +474,36 @@ async fn test_multiple_subscribers_receive_events() { // Client 1 let (client1, mut updates1, run_fut1) = TestClient::new(Ipv4Addr::LOCALHOST); tokio::spawn(run_fut1); - client1.add_endpoint(0x5B, 1, server_addr, 0).await.unwrap(); - client1.subscribe(0x5B, 1, 1, 3, 0x01, 0).await.unwrap(); + client1 + .add_endpoint(service_id, 1, server_addr, 0) + .await + .unwrap(); + client1 + .subscribe(service_id, 1, 1, 3, 0x01, 0) + .await + .unwrap(); // Client 2 let (client2, mut updates2, run_fut2) = TestClient::new(Ipv4Addr::LOCALHOST); tokio::spawn(run_fut2); - client2.add_endpoint(0x5B, 1, server_addr, 0).await.unwrap(); - client2.subscribe(0x5B, 1, 1, 3, 0x01, 0).await.unwrap(); + client2 + .add_endpoint(service_id, 1, server_addr, 0) + .await + .unwrap(); + client2 + .subscribe(service_id, 1, 1, 3, 0x01, 0) + .await + .unwrap(); // Wait for both subscribers for _ in 0..40 { - if publisher.subscriber_count(0x5B, 1, 0x01).await >= 2 { + if publisher.subscriber_count(service_id, 1, 0x01).await >= 2 { break; } tokio::time::sleep(std::time::Duration::from_millis(50)).await; } assert!( - publisher.subscriber_count(0x5B, 1, 0x01).await >= 2, + publisher.subscriber_count(service_id, 1, 0x01).await >= 2, "expected at least 2 subscribers" ); @@ -422,7 +514,7 @@ async fn test_multiple_subscribers_receive_events() { // Publish event let event_msg = Message::::new_sd(0x0001, &empty_sd_header()); let sent = publisher - .publish_event(0x5B, 1, 0x01, &event_msg) + .publish_event(service_id, 1, 0x01, &event_msg) .await .expect("publish_event failed"); assert!(sent >= 2, "expected sent >= 2, got {sent}"); @@ -453,7 +545,7 @@ async fn test_multiple_subscribers_receive_events() { #[tokio::test] async fn test_updates_drain_after_shutdown() { let (client, mut updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); client.shut_down(); let result = tokio::time::timeout(std::time::Duration::from_secs(2), updates.recv()) @@ -465,17 +557,24 @@ async fn test_updates_drain_after_shutdown() { /// Verify that cloned client handles work independently. #[tokio::test] async fn test_cloned_client_works() { - let (mut server, server_port) = create_server(0x5B, 1).await; + let service_id = next_service_id(); + let (mut server, server_port) = create_server(service_id, 1).await; let server_handle = tokio::spawn(async move { server.run().await }); let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let client2 = client.clone(); // Both clones can send commands let server_addr = SocketAddrV4::new(Ipv4Addr::LOCALHOST, server_port); - client.add_endpoint(0x5B, 1, server_addr, 0).await.unwrap(); - client2.subscribe(0x5B, 1, 1, 3, 0x01, 0).await.unwrap(); + client + .add_endpoint(service_id, 1, server_addr, 0) + .await + .unwrap(); + client2 + .subscribe(service_id, 1, 1, 3, 0x01, 0) + .await + .unwrap(); client.shut_down(); // client2 is also dropped @@ -486,23 +585,27 @@ async fn test_cloned_client_works() { /// Exercises the port-reuse path in Subscribe handling. #[tokio::test] async fn test_subscribe_specific_port_reuse() { - let (mut server, server_port) = create_server(0x5B, 1).await; + let service_id = next_service_id(); + let (mut server, server_port) = create_server(service_id, 1).await; let server_handle = tokio::spawn(async move { server.run().await }); let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); - let _ = tokio::spawn(run_fut); + let _run_handle = tokio::spawn(run_fut); let server_addr = SocketAddrV4::new(Ipv4Addr::LOCALHOST, server_port); - client.add_endpoint(0x5B, 1, server_addr, 0).await.unwrap(); + client + .add_endpoint(service_id, 1, server_addr, 0) + .await + .unwrap(); // Use specific port let specific_port = 44444; client - .subscribe(0x5B, 1, 1, 3, 0x01, specific_port) + .subscribe(service_id, 1, 1, 3, 0x01, specific_port) .await .unwrap(); // Second subscribe reuses the port client - .subscribe(0x5B, 1, 1, 3, 0x02, specific_port) + .subscribe(service_id, 1, 1, 3, 0x02, specific_port) .await .unwrap();