From 9f17909b96d90e6c94e34f15dec42e1317b25ed5 Mon Sep 17 00:00:00 2001 From: Justin Kovacich Date: Wed, 29 Apr 2026 10:22:27 -0400 Subject: [PATCH] phase 20e: consolidate handle traits into SharedHandle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Collapses the three near-identical handle traits introduced in 19f / 20b / 20c into a single generic trait pair: pub trait SharedHandle: Clone + 'static { fn get(&self) -> &T; } pub trait WrappableSharedHandle: SharedHandle { fn wrap(value: T) -> Self; } Two blanket impls cover the alloc and no-alloc paths: impl SharedHandle for &'static T { ... } impl SharedHandle for Arc { ... } // alloc-gated impl WrappableSharedHandle for Arc { ... } // alloc-gated Deleted (each was a slot-specific copy of this same shape): - `SocketHandle` / `WrappableSocketHandle` (transport.rs). - `SdStateHandle` / `WrappableSdStateHandle` (server/sd_state.rs). - `EventPublisherHandle` / `WrappableEventPublisherHandle` (server/event_publisher.rs). - `StaticSocketHandle` (the `&'static T` blanket impl subsumes its only purpose: carrying the `'static` lifetime). Net trait count: 6 + 1 wrapper struct → 2 traits. ~60% less boilerplate across the three former trait modules. `Server`'s three handle bounds become uniform: - `H: SharedHandle` - `Hsd: SharedHandle` - `Hep: SharedHandle>` `EventPublisher` gains an explicit `T: TransportSocket` parameter — the price of carrying `T` as a generic on `SharedHandle` rather than as an associated type. The struct grows a `PhantomData` field so the type parameter is well-formed without affecting drop-check. Method calls collapse to one name everywhere: - `H::socket()` / `Hsd::sd_state()` / `Hep::publisher()` → `.get()` (consistent across all three slot types). Default type-param expressions adjust to spell the `T`: - `Hep = Arc::Socket>>`. Test type-aliases gain the new `T` slot: - `tests/client_server.rs::TestEventPublisher` - `event_publisher.rs::TestEventPublisher` (internal) - The `AlwaysFailSocket` regression-test type alias. Pure rename / shape-change patch — no behavior change. The runtime behavior of every public API call is identical to 20d's; this is type-system tidying motivated by the adversarial review. Adversarial-review observations addressed: - "Three near-identical handle traits is a code smell" — fixed. - "We didn't generalize this into a single Shared trait" — done. Trade-off accepted: `EventPublisher` signature widens from `` to ``. The cost is one extra type parameter at the EventPublisher layer; the win is removing six trait definitions and one wrapper struct. 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/event_publisher.rs | 159 ++++++++------------------ src/server/mod.rs | 70 ++++++------ src/server/sd_state.rs | 79 +------------ src/transport.rs | 203 +++++++++++++++------------------- tests/client_server.rs | 1 + 5 files changed, 175 insertions(+), 337 deletions(-) diff --git a/src/server/event_publisher.rs b/src/server/event_publisher.rs index 8a85f20..c4c45da 100644 --- a/src/server/event_publisher.rs +++ b/src/server/event_publisher.rs @@ -6,9 +6,10 @@ use crate::UDP_BUFFER_SIZE; use crate::e2e::E2EKey; use crate::protocol::{Header, Message}; use crate::traits::{PayloadWireFormat, WireFormat}; -use crate::transport::{E2ERegistryHandle, SocketHandle, TransportSocket}; -#[cfg(any(feature = "embassy_channels", feature = "server"))] +use crate::transport::{E2ERegistryHandle, SharedHandle, TransportSocket}; +#[cfg(test)] use alloc::sync::Arc; +use core::marker::PhantomData; use core::net::SocketAddrV4; use heapless::Vec as HeaplessVec; @@ -23,11 +24,11 @@ const _: () = assert!( /// Publishes events to subscribers. /// -/// Generic over `H: SocketHandle` (abstracting the storage of the -/// transport socket — `Arc` in the std/tokio path, -/// `StaticSocketHandle` on bare metal, etc.), -/// `R: E2ERegistryHandle`, and `S: SubscriptionHandle`. The -/// underlying socket type is reachable as `H::Socket`. +/// Generic over `H: SharedHandle` (abstracting how the +/// transport socket is shared — `Arc` in alloc-using builds, +/// `&'static T` on bare-metal-no-alloc), `T: TransportSocket` +/// (the concrete underlying socket type), `R: E2ERegistryHandle`, +/// and `S: SubscriptionHandle`. /// /// Pre-19f revision: this type held an `Arc` directly and required /// `T: Send + Sync + 'static`. The handle indirection drops the @@ -36,33 +37,47 @@ const _: () = assert!( /// construct an `EventPublisher`. Multi-threaded callers continue /// to use `Arc` (which is `Send + Sync` whenever `T` is) without /// any change. -pub struct EventPublisher +/// +/// The explicit `T` parameter is the price of consolidating all +/// three former handle traits (Phase 20e) into a single +/// [`SharedHandle`]: the trait carries `T` as a generic, not +/// as an associated type, so consumers that need to name the +/// socket type spell it out. +pub struct EventPublisher where R: E2ERegistryHandle, S: SubscriptionHandle, - H: SocketHandle, + T: TransportSocket + 'static, + H: SharedHandle, { subscriptions: S, socket: H, e2e_registry: R, + /// `T` appears only in the bound `H: SharedHandle`; the + /// struct doesn't directly hold a `T`. PhantomData carries the + /// type so the parameter is well-formed without affecting + /// drop-check or auto-trait propagation negatively. + _phantom: PhantomData, } -impl EventPublisher +impl EventPublisher where R: E2ERegistryHandle, S: SubscriptionHandle, - H: SocketHandle, + T: TransportSocket + 'static, + H: SharedHandle, { /// Create a new event publisher. /// - /// `socket` is whatever `SocketHandle` impl the caller chose for - /// storage — `Arc` on std, `StaticSocketHandle` on bare - /// metal. + /// `socket` is whatever [`SharedHandle`] impl the caller + /// chose for storage — `Arc` on std/alloc, `&'static T` on + /// bare-metal-no-alloc. pub fn new(subscriptions: S, socket: H, e2e_registry: R) -> Self { Self { subscriptions, socket, e2e_registry, + _phantom: PhantomData, } } @@ -197,7 +212,7 @@ where let mut sent_count = 0usize; let mut last_err: Option = None; for addr in &subscribers { - match self.socket.socket().send_to(datagram, *addr).await { + match self.socket.get().send_to(datagram, *addr).await { Ok(()) => { sent_count += 1; tracing::trace!( @@ -323,7 +338,7 @@ where let mut sent_count = 0usize; let mut last_err: Option = None; for addr in &subscribers { - match self.socket.socket().send_to(datagram, *addr).await { + match self.socket.get().send_to(datagram, *addr).await { Ok(()) => { sent_count += 1; } @@ -451,99 +466,13 @@ where } } -/// Shared handle to the [`EventPublisher`] backing a -/// [`Server`](super::Server). -/// -/// Abstracts how the event publisher is shared between the Server's -/// run loop and any external task that wants to publish events -/// (`server.publisher().publish_event(...)`). Two impls ship out -/// of the box, mirroring the pattern established by -/// [`crate::transport::SocketHandle`] and -/// [`super::SdStateHandle`]: -/// -/// - `Arc>` on alloc-using builds — the -/// default for `Server::new_with_deps` / `new_passive_with_deps`. -/// - `&'static EventPublisher` on bare-metal-no-alloc -/// — caller declares a `static` somewhere and supplies the -/// reference into a future `Server::new_with_handles` -/// constructor. -/// -/// `Clone + 'static` only — neither `Send` nor `Sync` at the trait -/// level. Method-level `where` clauses on Server add Send bounds -/// at use sites that need them (`announcement_loop`'s -/// `+ Send`-bounded return type, etc.). -pub trait EventPublisherHandle: Clone + 'static -where - R: E2ERegistryHandle, - S: SubscriptionHandle, - H: SocketHandle, -{ - /// Borrow the underlying [`EventPublisher`] for read-only - /// access. Used by Server's run loop and accessor. - fn publisher(&self) -> &EventPublisher; -} - -// `&'static EventPublisher<...>`: trivially `Copy + Clone + 'static` -// for any 'static publisher. Caller arranges the static storage. -impl EventPublisherHandle for &'static EventPublisher -where - R: E2ERegistryHandle, - S: SubscriptionHandle, - H: SocketHandle, -{ - fn publisher(&self) -> &EventPublisher { - self - } -} - -#[cfg(any(feature = "embassy_channels", feature = "server"))] -impl EventPublisherHandle for Arc> -where - R: E2ERegistryHandle, - S: SubscriptionHandle, - H: SocketHandle, -{ - fn publisher(&self) -> &EventPublisher { - self - } -} - -/// Extension of [`EventPublisherHandle`] for handles that can be -/// constructed inline from an owned [`EventPublisher`]. -/// -/// Required by `Server` constructors that build the publisher -/// internally (the alloc-using path). The future -/// `Server::new_with_handles` will accept a pre-built `Hep: -/// EventPublisherHandle` directly and won't need this trait — -/// callers using `&'static EventPublisher<...>` declare their -/// `static` storage themselves and pass the reference in. -/// -/// Mirrors the [`crate::transport::WrappableSocketHandle`] / -/// [`super::WrappableSdStateHandle`] split: the basic `Handle` -/// trait gives read access; the `Wrappable*` extension adds the -/// inline-construction path that requires an allocator. -pub trait WrappableEventPublisherHandle: EventPublisherHandle -where - R: E2ERegistryHandle, - S: SubscriptionHandle, - H: SocketHandle, -{ - /// Place an owned [`EventPublisher`] behind this handle's - /// shared storage. - fn wrap(publisher: EventPublisher) -> Self; -} - -#[cfg(any(feature = "embassy_channels", feature = "server"))] -impl WrappableEventPublisherHandle for Arc> -where - R: E2ERegistryHandle, - S: SubscriptionHandle, - H: SocketHandle, -{ - fn wrap(publisher: EventPublisher) -> Self { - Arc::new(publisher) - } -} +// Phase 20e collapsed `EventPublisherHandle` / +// `WrappableEventPublisherHandle` into the unified +// `crate::transport::SharedHandle>` / +// `WrappableSharedHandle>` traits. The +// blanket impls there cover both `&'static EventPublisher<...>` +// and `Arc>`; no dedicated trait survives +// here. #[cfg(all(test, feature = "server-tokio"))] mod tests { @@ -561,9 +490,13 @@ mod tests { /// Type alias bringing the tokio-flavor concrete type parameters back /// into scope so tests can spell `TestEventPublisher` without - /// chasing the three-type-parameter signature on every call site. - type TestEventPublisher = - EventPublisher>, Arc>, Arc>; + /// chasing the four-type-parameter signature on every call site. + type TestEventPublisher = EventPublisher< + Arc>, + Arc>, + Arc, + TokioSocket, + >; fn test_registry() -> Arc> { Arc::new(Mutex::new(E2ERegistry::new())) @@ -738,6 +671,7 @@ mod tests { Arc>, Arc>, Arc, + AlwaysFailSocket, > = EventPublisher::new(subscriptions, Arc::new(AlwaysFailSocket), test_registry()); let msg = make_test_message(); @@ -799,6 +733,7 @@ mod tests { Arc>, Arc>, Arc, + AlwaysFailSocket, > = EventPublisher::new(subscriptions, Arc::new(AlwaysFailSocket), test_registry()); let err = publisher diff --git a/src/server/mod.rs b/src/server/mod.rs index 4df9f61..c7a868c 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -13,7 +13,7 @@ mod service_info; mod subscription_manager; pub use error::Error; -pub use event_publisher::{EventPublisher, EventPublisherHandle, WrappableEventPublisherHandle}; +pub use event_publisher::EventPublisher; pub use service_info::Subscriber; #[cfg(feature = "std")] pub use service_info::{EventGroupInfo, ServiceInfo}; @@ -21,7 +21,7 @@ pub use service_info::{EventGroupInfo, ServiceInfo}; pub use subscription_manager::{StaticSubscriptionHandle, StaticSubscriptionStorage}; pub use subscription_manager::{SubscribeError, SubscriptionHandle, SubscriptionManager}; -pub use sd_state::{SdStateHandle, SdStateManager, WrappableSdStateHandle}; +pub use sd_state::SdStateManager; use core::sync::atomic::{AtomicBool, Ordering}; @@ -29,8 +29,8 @@ use crate::Timer; use crate::e2e::{E2EKey, E2EProfile}; use crate::protocol::sd::{self, Entry, Flags, OptionsCount, ServiceEntry, TransportProtocol}; use crate::transport::{ - E2ERegistryHandle, SocketHandle, SocketOptions, TransportFactory, TransportSocket, - WrappableSocketHandle, + E2ERegistryHandle, SharedHandle, SocketOptions, TransportFactory, TransportSocket, + WrappableSharedHandle, }; use alloc::sync::Arc; use core::net::{Ipv4Addr, SocketAddrV4}; @@ -151,9 +151,9 @@ where Tm: Timer, R: E2ERegistryHandle, S: SubscriptionHandle, - H: SocketHandle, - Hsd: SdStateHandle, - Hep: EventPublisherHandle, + H: SharedHandle, + Hsd: SharedHandle, + Hep: SharedHandle>, { /// Transport factory. Retained on the `Server` for any /// post-construction state the backend needs to keep alive @@ -207,16 +207,16 @@ pub struct Server< Tm, H = Arc<::Socket>, Hsd = Arc, - Hep = Arc>, + Hep = Arc::Socket>>, > where R: E2ERegistryHandle, S: SubscriptionHandle, F: TransportFactory + 'static, F::Socket: 'static, Tm: Timer + Clone + 'static, - H: SocketHandle, - Hsd: SdStateHandle, - Hep: EventPublisherHandle, + H: SharedHandle, + Hsd: SharedHandle, + Hep: SharedHandle>, { config: ServerConfig, /// Socket for receiving subscription requests, behind whatever @@ -356,9 +356,9 @@ where F: TransportFactory + 'static, F::Socket: 'static, Tm: Timer + Clone + 'static, - H: WrappableSocketHandle, - Hsd: WrappableSdStateHandle, - Hep: WrappableEventPublisherHandle, + H: WrappableSharedHandle, + Hsd: WrappableSharedHandle, + Hep: WrappableSharedHandle>, { /// Bare-metal-friendly constructor that takes every dependency /// explicitly via a [`ServerDeps`] bundle. The `server-tokio` @@ -525,9 +525,9 @@ where F: TransportFactory + 'static, F::Socket: 'static, Tm: Timer + Clone + 'static, - H: SocketHandle, - Hsd: SdStateHandle, - Hep: EventPublisherHandle, + H: SharedHandle, + Hsd: SharedHandle, + Hep: SharedHandle>, { /// Construct a `Server` from pre-built dependencies + storage /// handles. The bare-metal-no-alloc counterpart to @@ -555,7 +555,7 @@ where deps: ServerHandles, mut config: ServerConfig, ) -> Result { - let bound_port = deps.unicast_socket.socket().local_addr()?.port(); + let bound_port = deps.unicast_socket.get().local_addr()?.port(); config.local_port = bound_port; tracing::info!( "Server (handles) bound to {}:{} for service 0x{:04X}", @@ -603,7 +603,7 @@ where deps: ServerHandles, mut config: ServerConfig, ) -> Result { - let bound_port = deps.unicast_socket.socket().local_addr()?.port(); + let bound_port = deps.unicast_socket.get().local_addr()?.port(); config.local_port = bound_port; tracing::info!( "Passive server (handles) bound to {}:{} for service 0x{:04X}", @@ -703,8 +703,8 @@ where let mut announcement_count = 0u32; loop { match sd_state - .sd_state() - .send_offer_service(&config, sd_socket.socket()) + .get() + .send_offer_service(&config, sd_socket.get()) .await { Ok(()) => { @@ -783,8 +783,8 @@ where let mut announcement_count = 0u32; loop { match sd_state - .sd_state() - .send_offer_service(&config, sd_socket.socket()) + .get() + .send_offer_service(&config, sd_socket.get()) .await { Ok(()) => { @@ -838,7 +838,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.sd_state().next_session_id_with_reboot_flag(); + let (sid, reboot_flag) = self.sd_state.get().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]; @@ -849,7 +849,7 @@ where let target_v4 = socket_addr_v4(target)?; self.sd_socket - .socket() + .get() .send_to(&buffer[..total_len], target_v4) .await?; tracing::debug!( @@ -877,7 +877,7 @@ where /// /// Returns an error if the socket's local address cannot be retrieved. pub fn unicast_local_addr(&self) -> Result { - match self.unicast_socket.socket().local_addr() { + match self.unicast_socket.get().local_addr() { Ok(v4) => Ok(core::net::SocketAddr::V4(v4)), Err(e) => Err(Error::Transport(e)), } @@ -995,10 +995,10 @@ where // — direct `&mut foo` would produce `&mut &mut [u8]`. let unicast_fut = self .unicast_socket - .socket() + .get() .recv_from(&mut *unicast_buf) .fuse(); - let sd_fut = self.sd_socket.socket().recv_from(&mut *sd_buf).fuse(); + let sd_fut = self.sd_socket.get().recv_from(&mut *sd_buf).fuse(); pin_mut!(unicast_fut, sd_fut); select_biased! { result = unicast_fut => { @@ -1397,9 +1397,9 @@ where F: TransportFactory + 'static, F::Socket: 'static, Tm: Timer + Clone + 'static, - H: SocketHandle, - Hsd: SdStateHandle, - Hep: EventPublisherHandle, + H: SharedHandle, + Hsd: SharedHandle, + Hep: SharedHandle>, { /// Send `SubscribeAck` from an entry view async fn send_subscribe_ack_from_view( @@ -1425,7 +1425,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.sd_state().next_session_id_with_reboot_flag(); + let (sid, reboot_flag) = self.sd_state.get().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]; @@ -1436,7 +1436,7 @@ where let subscriber_v4 = socket_addr_v4(subscriber)?; self.sd_socket - .socket() + .get() .send_to(&buffer[..total_len], subscriber_v4) .await?; @@ -1475,7 +1475,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.sd_state().next_session_id_with_reboot_flag(); + let (sid, reboot_flag) = self.sd_state.get().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]; @@ -1486,7 +1486,7 @@ where let subscriber_v4 = socket_addr_v4(subscriber)?; self.sd_socket - .socket() + .get() .send_to(&buffer[..total_len], subscriber_v4) .await?; diff --git a/src/server/sd_state.rs b/src/server/sd_state.rs index bd50033..211b7cc 100644 --- a/src/server/sd_state.rs +++ b/src/server/sd_state.rs @@ -216,80 +216,11 @@ 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) - } -} +// Phase 20e collapsed `SdStateHandle` / `WrappableSdStateHandle` +// into the unified `crate::transport::SharedHandle` +// / `WrappableSharedHandle` traits. The blanket +// impls there cover both `&'static SdStateManager` and +// `Arc`; no dedicated trait survives here. #[cfg(all(test, feature = "server-tokio"))] mod tests { diff --git a/src/transport.rs b/src/transport.rs index 8817f96..b328b03 100644 --- a/src/transport.rs +++ b/src/transport.rs @@ -796,69 +796,94 @@ pub trait InterfaceHandle: Clone + Send + Sync + 'static { fn set(&self, addr: Ipv4Addr); } -/// Shared handle to a bound transport socket. +/// Shared handle to a single owned-or-borrowed `T`. /// -/// Abstracts over `Arc` on `std` (and any bare-metal target with an -/// allocator) and `StaticSocketHandle` on bare metal without an -/// allocator. The single method [`SocketHandle::socket`] borrows the -/// underlying socket so [`crate::server::Server`] / [`crate::server::EventPublisher`] -/// can forward `send_to` / `recv_from` / `local_addr` / -/// `join_multicast_v4` / `leave_multicast_v4` calls without caring how -/// the socket is stored. +/// One trait covering every "Server holds an `Arc` for sharing +/// between its run loop and consumer-side tasks" pattern in this +/// crate. Replaces the three separate handle traits this crate +/// shipped earlier (`SocketHandle`, `SdStateHandle`, +/// `EventPublisherHandle`), each of which had the same shape with +/// a different concrete `T`. /// -/// The trait is bounded on `Clone + 'static` only — neither `Send` nor -/// `Sync` — so a `Server` parameterized over a `SocketHandle` whose -/// underlying `Socket` is `!Sync` (e.g. an `embassy-net` -/// `UdpSocket<'static>` borrowing from a `RefCell>`-bearing -/// `Stack`) is still constructible. Methods that *return* a -/// `Send`-bounded future (notably [`crate::server::Server::announcement_loop`]) -/// add Send bounds at the method level so the impl block can stay -/// permissive. +/// Two impls ship out of the box, both via blanket impls so any +/// consumer-defined type wrapped in `Arc` or `&'static T` +/// satisfies the bound automatically: /// -/// `Socket` is an associated type rather than a generic parameter so -/// downstream stores (`EventPublisher`, `Server`) don't need to carry -/// it as a separate type parameter — the handle type uniquely -/// determines its target socket type, which matches the established -/// no-allocation pattern used by [`E2ERegistryHandle`] / -/// [`InterfaceHandle`]. +/// - `Arc: SharedHandle` on alloc-using builds (`std` or +/// `bare_metal`-with-alloc). `Arc::clone` increments the +/// refcount; `get` returns the inner reference. +/// - `&'static T: SharedHandle` on bare-metal-no-alloc. The +/// reference is `Copy + Clone + 'static`; the user declares the +/// underlying `static` storage at boot. /// -/// Matches the bound profile of -/// [`SubscriptionHandle`](crate::server::SubscriptionHandle): -/// `Clone + 'static`, no Send/Sync at the trait level. Two impls ship -/// out of the box: -/// - `Arc` on `std` (in `std_handle_impls`). -/// - `StaticSocketHandle` on bare metal (in `bare_metal_handle_impls`). -pub trait SocketHandle: Clone + 'static { - /// The underlying transport socket type this handle borrows. - type Socket: TransportSocket + 'static; - - /// Borrow the underlying socket. - fn socket(&self) -> &Self::Socket; +/// `Clone + 'static` only — neither `Send` nor `Sync` at the +/// trait level. Method-level `where` clauses on `Server` add +/// Send bounds at the use sites that need them +/// (`announcement_loop`'s `+ Send` return type, etc.). +/// +/// `T: 'static` because both blanket impls require it: an `Arc` +/// is `'static` only when `T: 'static`, and `&'static T` requires +/// `T: 'static` by definition. +/// +/// `?Sized` is intentionally NOT supported — the inline-construction +/// path ([`WrappableSharedHandle::wrap`]) needs an owned `T`, which +/// requires `Sized`. +pub trait SharedHandle: Clone + 'static { + /// Borrow the underlying `T`. Both blanket impls return a + /// reference into the underlying storage; consumers should + /// not assume more than a fresh borrow's worth of lifetime. + fn get(&self) -> &T; } -/// Extension of [`SocketHandle`] for handles that can be constructed -/// inline from an owned socket. -/// -/// Required by [`crate::server::Server`] constructors that bind -/// sockets internally via [`TransportFactory::bind`] (the std / -/// alloc path) — those constructors call `factory.bind(...).await?` -/// to get an owned `F::Socket`, then `H::wrap(socket)` to place it -/// behind whatever shared-storage the caller chose. +/// Extension of [`SharedHandle`] for handles that can be +/// constructed inline from an owned `T`. /// -/// `Arc` is the std-side impl: `Arc::new(socket)` is a no-op -/// wrapping. +/// Required by `Server` constructors that build the underlying +/// `T` internally (the alloc-using path — +/// e.g., `Server::new_with_deps` calls `factory.bind(...).await?` +/// to get an `F::Socket`, then `H::wrap(socket)` to place it +/// behind the caller's chosen shared-storage). The no-alloc +/// counterpart constructors (`Server::new_with_handles`) take +/// pre-built handles directly and don't need this trait. /// -/// `StaticSocketHandle` deliberately does **not** implement this -/// trait: materializing a `&'static T` requires either an -/// allocator (`Box::leak`) or a slot-based init pattern -/// (`StaticCell::init`) that the trait method's signature can't -/// express. Pure-no-alloc consumers need a future Server -/// constructor variant that takes pre-built handles directly -/// rather than binding internally; that variant is not in 19f's -/// scope. -pub trait WrappableSocketHandle: SocketHandle { - /// Place an owned socket behind this handle's shared storage. - fn wrap(socket: Self::Socket) -> Self; +/// `&'static T` deliberately does NOT implement this trait — +/// materializing a `&'static T` from an owned `T` inside a trait +/// method's body requires an allocator (`Box::leak`) or a +/// slot-based init pattern (`StaticCell::init`) that the trait +/// method's signature can't express. No-alloc consumers declare +/// their `static` storage themselves and pass `&STATIC` into the +/// no-wrap constructor. +pub trait WrappableSharedHandle: SharedHandle { + /// Place an owned `T` behind this handle's shared storage. + fn wrap(value: T) -> Self; +} + +// `&'static T` is the no-alloc handle. `&'static T: Copy + Clone + +// 'static` for any `T: 'static`, so the trait bounds are met +// without further work. +impl SharedHandle for &'static T { + fn get(&self) -> &T { + self + } +} + +// `Arc` is the alloc-using handle. `Arc::clone` is the +// reference-count increment; `wrap` is `Arc::new`. Gated to +// where `alloc` is available — `feature = "embassy_channels"` +// or `feature = "server"` per the crate-root `extern crate +// alloc` declaration. +#[cfg(any(feature = "embassy_channels", feature = "server"))] +impl SharedHandle for alloc::sync::Arc { + fn get(&self) -> &T { + self + } +} + +#[cfg(any(feature = "embassy_channels", feature = "server"))] +impl WrappableSharedHandle for alloc::sync::Arc { + fn wrap(value: T) -> Self { + alloc::sync::Arc::new(value) + } } /// Default `std`-flavoured impls of [`E2ERegistryHandle`] / @@ -868,26 +893,12 @@ pub trait WrappableSocketHandle: SocketHandle { /// module rather than the tokio backend. #[cfg(feature = "std")] mod std_handle_impls { - use super::{E2ERegistryHandle, InterfaceHandle, SocketHandle, TransportSocket}; + use super::{E2ERegistryHandle, InterfaceHandle}; use crate::e2e::Error as E2EError; use crate::e2e::{E2ECheckStatus, E2EKey, E2EProfile, E2ERegistry, E2ERegistryFull}; use core::net::Ipv4Addr; use std::sync::{Arc, Mutex, RwLock}; - impl SocketHandle for Arc { - type Socket = T; - - fn socket(&self) -> &T { - self - } - } - - impl super::WrappableSocketHandle for Arc { - fn wrap(socket: T) -> Self { - Arc::new(socket) - } - } - impl E2ERegistryHandle for Arc> { fn register(&self, key: E2EKey, profile: E2EProfile) -> Result<(), E2ERegistryFull> { self.lock() @@ -1036,52 +1047,12 @@ pub mod bare_metal_handle_impls { self.0.store(u32::from(addr), Ordering::Release); } } - - /// No-alloc [`SocketHandle`](super::SocketHandle) backed by - /// `&'static T`. - /// - /// Used by [`crate::server::Server`] / [`crate::server::EventPublisher`] - /// to share a transport socket without an allocator. Both clones - /// of the handle hold the same thin pointer, so the underlying - /// socket sees every operation through the same `&T` reference. - /// - /// ```ignore - /// // `Box::leak` is fine in system init; for fully-static targets, - /// // bind via a `OnceCell` / `static_cell::StaticCell::init` and - /// // wrap the resulting `&'static T` here. - /// let socket: T = factory.bind(...).await?; - /// let handle = StaticSocketHandle::new(Box::leak(Box::new(socket))); - /// ``` - pub struct StaticSocketHandle(&'static T); - - impl StaticSocketHandle { - /// Wraps a static reference to the backing socket. - #[must_use] - pub const fn new(socket: &'static T) -> Self { - Self(socket) - } - } - - // Manual `Clone` + `Copy` (rather than `#[derive]`) because the - // auto-derived bounds would require `T: Clone` / `T: Copy`; we - // only need cloning the reference, which is `Copy` regardless - // of `T`. `clone` delegates to `*self` to satisfy clippy's - // canonical-clone-on-Copy lint. - impl Clone for StaticSocketHandle { - fn clone(&self) -> Self { - *self - } - } - - impl Copy for StaticSocketHandle {} - - impl super::SocketHandle for StaticSocketHandle { - type Socket = T; - - fn socket(&self) -> &T { - self.0 - } - } + // Phase 20e collapsed `StaticSocketHandle(&'static T)` into a + // direct `impl SharedHandle for &'static T` blanket — the + // wrapper type's only role was carrying the `'static` lifetime, + // which the blanket impl achieves without a wrapper. Consumers + // that previously constructed `StaticSocketHandle::new(&SOCKET)` + // now pass `&SOCKET` directly into Server's no-wrap constructors. } /// `StaticE2EHandle` — no-alloc `E2ERegistryHandle` backed by a diff --git a/tests/client_server.rs b/tests/client_server.rs index 161ccbd..9b72d1f 100644 --- a/tests/client_server.rs +++ b/tests/client_server.rs @@ -75,6 +75,7 @@ type TestEventPublisher = simple_someip::server::EventPublisher< std::sync::Arc>, std::sync::Arc>, std::sync::Arc, + simple_someip::TokioSocket, >; /// Create a server on an ephemeral unicast port, returning (Server, actual_port).