From 9220cded9351d68abe17921924a55807b2f7bcd7 Mon Sep 17 00:00:00 2001 From: Justin Kovacich Date: Wed, 29 Apr 2026 09:51:02 -0400 Subject: [PATCH] server: SdStateHandle trait + drop Arc requirement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `SdStateHandle` + `WrappableSdStateHandle` traits in `src/server/sd_state.rs` and threads them through `Server` as a new `Hsd` type parameter (default `Arc`). Mirrors the pattern established by 19f's `SocketHandle` / `WrappableSocketHandle`. Same shape, same Send/Sync defaults (neither bound at trait level — caller adds at use sites). Two impls ship: - `Arc: SdStateHandle + WrappableSdStateHandle` (the existing default; preserves std-side behavior). - `&'static SdStateManager: SdStateHandle` (no-alloc; user declares `static SD_STATE: SdStateManager = SdStateManager::new();` and supplies `&SD_STATE` via a future `Server::new_with_handles` constructor). `SdStateManager` itself becomes `pub` and `SdStateManager::new()` becomes `pub const fn` so the static-storage pattern compiles. The internal methods (`next_session_id_with_reboot_flag`, `reboot_flag`, `send_offer_service`) stay `pub(super)` — consumers shouldn't call them directly; they go through Server. Server's existing `new_with_deps` / `new_passive_with_deps` constructors require `Hsd: WrappableSdStateHandle` because they build the manager internally via `SdStateManager::new()` then `Hsd::wrap(...)`. The future `Server::new_with_handles` will take `Hsd: SdStateHandle` directly (no `wrap` step), enabling the no-alloc path with `&'static SdStateManager`. `announcement_loop`'s method-level `where` clause picks up the new `Hsd: Send + Sync` bound, mirroring the existing `H: Send + Sync` and `F: Send + Sync` bounds. The `_local` variant has no such requirement and works for any `Hsd: SdStateHandle`. Type-signature width: Server now reads `Server, Hsd = Arc>`. Both defaults preserve every existing call site — `Server` and `Server>` both still resolve correctly. No churn in `tests/` or `examples/`. Clears 20-pre alloc audit's category-E.2 finding. Combined with the 4f9d36e recv-buffer split, two of the four "no-alloc Server" remediation items are done. What this leaves: - E.1: `Arc>` field on Server. Same shape via an `EventPublisherHandle` trait — next branch. - D: `Server::new_with_handles` constructor that takes pre-built `H` + `Hsd` (and the future `Hep` for E.1) directly, skipping the `wrap` step. Lands after E.1 so the constructor's parameter list is final. Gates green: - cargo fmt --check - cargo clippy --tests (2 pre-existing warnings, unrelated) - cargo build --workspace --all-targets - cargo build --no-default-features --features client,server,bare_metal - cargo build -p simple-someip-embassy-net --target thumbv7em-none-eabihf - cargo test --features client-tokio,server-tokio --test client_server -- --test-threads=1 (11/11) - cargo test --features client,server,bare_metal --test bare_metal_e2e (2/2) - cargo test -p simple-someip-embassy-net --test loopback (3/3) Co-Authored-By: Claude Opus 4.7 (1M context) --- src/server/mod.rs | 37 ++++++++++------- src/server/sd_state.rs | 91 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 112 insertions(+), 16 deletions(-) diff --git a/src/server/mod.rs b/src/server/mod.rs index bf75281a..f77d40e7 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -21,7 +21,7 @@ pub use service_info::{EventGroupInfo, ServiceInfo}; pub use subscription_manager::{StaticSubscriptionHandle, StaticSubscriptionStorage}; pub use subscription_manager::{SubscribeError, SubscriptionHandle, SubscriptionManager}; -use sd_state::SdStateManager; +pub use sd_state::{SdStateHandle, SdStateManager, WrappableSdStateHandle}; use core::sync::atomic::{AtomicBool, Ordering}; @@ -143,7 +143,7 @@ where /// these as `Arc>` / `Arc>` /// / `TokioTransport` / `TokioTimer`. Bare-metal callers use /// [`Self::new_with_deps`] (under `server`) and supply their own. -pub struct Server::Socket>> +pub struct Server::Socket>, Hsd = Arc> where R: E2ERegistryHandle, S: SubscriptionHandle, @@ -151,6 +151,7 @@ where F::Socket: 'static, Tm: Timer + Clone + 'static, H: SocketHandle, + Hsd: SdStateHandle, { config: ServerConfig, /// Socket for receiving subscription requests, behind whatever @@ -164,8 +165,10 @@ where subscriptions: S, /// Event publisher publisher: Arc>, - /// SD session-ID counter and announcement emitter - sd_state: Arc, + /// SD session-ID counter and announcement emitter, behind whatever + /// shared-storage `Hsd` chose (`Arc` on std, + /// `&'static SdStateManager` on bare-metal-no-alloc). + sd_state: Hsd, /// Shared E2E registry for runtime E2E configuration e2e_registry: R, /// Transport factory. Used at construction time to bind sockets; @@ -279,7 +282,7 @@ impl } } -impl Server +impl Server where R: E2ERegistryHandle, S: SubscriptionHandle, @@ -287,6 +290,7 @@ where F::Socket: 'static, Tm: Timer + Clone + 'static, H: WrappableSocketHandle, + Hsd: WrappableSdStateHandle, { /// Bare-metal-friendly constructor that takes every dependency /// explicitly via a [`ServerDeps`] bundle. The `server-tokio` @@ -366,7 +370,7 @@ where sd_socket, subscriptions, publisher, - sd_state: Arc::new(SdStateManager::new()), + sd_state: Hsd::wrap(SdStateManager::new()), e2e_registry, factory, timer, @@ -436,7 +440,7 @@ where sd_socket, subscriptions, publisher, - sd_state: Arc::new(SdStateManager::new()), + sd_state: Hsd::wrap(SdStateManager::new()), e2e_registry, factory, timer, @@ -446,7 +450,7 @@ where } } -impl Server +impl Server where R: E2ERegistryHandle, S: SubscriptionHandle, @@ -454,6 +458,7 @@ where F::Socket: 'static, Tm: Timer + Clone + 'static, H: SocketHandle, + Hsd: SdStateHandle, { /// Build the periodic-SD-announcement future. /// @@ -496,6 +501,7 @@ where F::Socket: Send + Sync, for<'a> ::SendFuture<'a>: Send, H: Send + Sync, + Hsd: Send + Sync, Tm: Send + Sync, for<'a> Tm::SleepFuture<'a>: Send, { @@ -523,13 +529,14 @@ where } let config = self.config.clone(); let sd_socket = self.sd_socket.clone(); - let sd_state = Arc::clone(&self.sd_state); + let sd_state = self.sd_state.clone(); let timer = self.timer.clone(); Ok(async move { let mut announcement_count = 0u32; loop { match sd_state + .sd_state() .send_offer_service(&config, sd_socket.socket()) .await { @@ -602,13 +609,14 @@ where } let config = self.config.clone(); let sd_socket = self.sd_socket.clone(); - let sd_state = Arc::clone(&self.sd_state); + let sd_state = self.sd_state.clone(); let timer = self.timer.clone(); Ok(async move { let mut announcement_count = 0u32; loop { match sd_state + .sd_state() .send_offer_service(&config, sd_socket.socket()) .await { @@ -663,7 +671,7 @@ where // 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 (sid, reboot_flag) = self.sd_state.sd_state().next_session_id_with_reboot_flag(); let sd_payload = sd::Header::new(Flags::new_sd(reboot_flag), &entries, &options); let mut buffer = [0u8; crate::UDP_BUFFER_SIZE]; @@ -1211,7 +1219,7 @@ fn extract_subscriber_endpoint( } } -impl Server +impl Server where R: E2ERegistryHandle, S: SubscriptionHandle, @@ -1219,6 +1227,7 @@ where F::Socket: 'static, Tm: Timer + Clone + 'static, H: SocketHandle, + Hsd: SdStateHandle, { /// Send `SubscribeAck` from an entry view async fn send_subscribe_ack_from_view( @@ -1244,7 +1253,7 @@ where let entries = [ack_entry]; // 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 (sid, reboot_flag) = self.sd_state.sd_state().next_session_id_with_reboot_flag(); let sd_payload = sd::Header::new(Flags::new_sd(reboot_flag), &entries, &[]); let mut buffer = [0u8; crate::UDP_BUFFER_SIZE]; @@ -1294,7 +1303,7 @@ where let entries = [nack_entry]; // 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 (sid, reboot_flag) = self.sd_state.sd_state().next_session_id_with_reboot_flag(); let sd_payload = sd::Header::new(Flags::new_sd(reboot_flag), &entries, &[]); let mut buffer = [0u8; crate::UDP_BUFFER_SIZE]; diff --git a/src/server/sd_state.rs b/src/server/sd_state.rs index 08837ff2..bd500331 100644 --- a/src/server/sd_state.rs +++ b/src/server/sd_state.rs @@ -30,7 +30,7 @@ use super::{Error, ServerConfig}; /// tracks that transition and exposes it via [`Self::reboot_flag`] so every /// server-side SD emission path reads from a single source of truth. #[derive(Debug)] -pub(super) struct SdStateManager { +pub struct SdStateManager { /// Packed `(has_wrapped, session_id)` state. /// /// - bits 0..16: current session id (1..=0xFFFF, never 0). @@ -50,7 +50,19 @@ const SID_MASK: u32 = 0xFFFF; const WRAPPED_BIT: u32 = 1 << 16; impl SdStateManager { - pub(super) const fn new() -> Self { + /// Construct an `SdStateManager` with a fresh session counter + /// (starts at `1`, reboot flag = `RecentlyRebooted`). + /// + /// `const fn` so consumers can declare a `static`-storage instance + /// without an allocator: + /// + /// ```ignore + /// static SD_STATE: SdStateManager = SdStateManager::new(); + /// // pass `&SD_STATE` (an `&'static SdStateManager`) into the + /// // appropriate `Server` constructor. + /// ``` + #[must_use] + pub const fn new() -> Self { Self::with_initial(1) } @@ -204,6 +216,81 @@ impl SdStateManager { } } +/// Shared handle to the [`SdStateManager`] backing a [`Server`]. +/// +/// Abstracts how the SD-session-state is shared between the Server's +/// run loop and its spawned `announcement_loop` future. Two impls +/// ship out of the box, mirroring the pattern established by +/// [`crate::transport::SocketHandle`]: +/// +/// - `Arc` on alloc-using builds — the existing +/// default for `Server::new_with_deps`. +/// - `&'static SdStateManager` on bare-metal-no-alloc — caller +/// declares a `static SdStateManager = SdStateManager::new();` +/// and passes the reference into a future +/// `Server::new_with_handles` constructor. +/// +/// Required to be `Clone + 'static` so the handle can be cheaply +/// cloned into the announcement-loop future without borrowing +/// `&self`. The bound is intentionally permissive — neither `Send` +/// nor `Sync` at the trait level — so a `!Send` storage backend +/// (e.g., `Rc` if a single-threaded alloc target +/// ever wants it) would also satisfy. +/// +/// [`Server`]: crate::server::Server +pub trait SdStateHandle: Clone + 'static { + /// Borrow the underlying `SdStateManager` for SD-session-state + /// reads / atomic increments. + fn sd_state(&self) -> &SdStateManager; +} + +// `&'static SdStateManager` is the no-alloc handle. `&'static T` is +// `Copy + Clone + 'static` for any `T: 'static` so the trait bounds +// are met without further work — the user only needs to declare +// the underlying `static` storage once at boot. +impl SdStateHandle for &'static SdStateManager { + fn sd_state(&self) -> &SdStateManager { + self + } +} + +#[cfg(any(feature = "embassy_channels", feature = "server"))] +impl SdStateHandle for alloc::sync::Arc { + fn sd_state(&self) -> &SdStateManager { + self + } +} + +/// Extension of [`SdStateHandle`] for handles that can be +/// constructed inline from an owned `SdStateManager`. +/// +/// Required by `Server` constructors that build an `SdStateManager` +/// internally (the alloc-using path — +/// `Server::new_with_deps` calls `SdStateManager::new()` then wraps). +/// The future `Server::new_with_handles` (post-alloc-audit follow-up) +/// will accept a pre-built `Hsd: SdStateHandle` directly and won't +/// need this trait. +/// +/// `&'static SdStateManager` deliberately does **not** implement this +/// trait — there is no allocator-free way to materialize a `&'static` +/// reference inside a trait method (the user has to declare a +/// `static` themselves and supply the reference via a different +/// constructor). This mirrors how +/// [`crate::transport::WrappableSocketHandle`] is split from +/// [`crate::transport::SocketHandle`]. +pub trait WrappableSdStateHandle: SdStateHandle { + /// Place an owned `SdStateManager` behind this handle's shared + /// storage. + fn wrap(state: SdStateManager) -> Self; +} + +#[cfg(any(feature = "embassy_channels", feature = "server"))] +impl WrappableSdStateHandle for alloc::sync::Arc { + fn wrap(state: SdStateManager) -> Self { + alloc::sync::Arc::new(state) + } +} + #[cfg(all(test, feature = "server-tokio"))] mod tests { use super::{SdStateManager, ServerConfig};