From 68919a5b4e0a16d90e222d7e48065907a471ad40 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Wed, 26 Aug 2026 01:46:15 +0800 Subject: [PATCH 01/37] feat(pubsub): let a publisher restrict its locality MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The subscriber side already had `with_locality` (it forwards to zenoh's `allowed_origin`). The publisher side had no equivalent, so the one setting that matters for intra-process delivery was unreachable: `Locality::SessionLocal` on a publisher makes zenoh's `resolve_put` skip `primitives.send_push_consume` entirely. No link, no wire encode, no shared memory — the payload goes straight to the matching local subscriber callbacks. Every node created from one ZContext shares one zenoh session, so this reaches sibling nodes in the same process. `Locality` is re-exported from the crate root and the prelude. Callers of the subscriber-side `with_locality` previously had to add a direct `zenoh` dependency to name the type, which also let them pick a different zenoh version than the one hiroz links. This removes the transport, not the serialization: zenoh payloads are bytes, so the message is still CDR-encoded on publish and decoded on receive. The doc comment says so, and says that publisher SHM is wasted work under SessionLocal. Measured on hiroz-bench (viz-grid, two nodes on one ZContext against the two-process baseline): 263 -> 25 microseconds p50 at 64 B. What fails without this: `publisher_locality.rs` does not compile, since `with_locality` is what it calls. As a property detector it does work in both directions — swapping `Locality::SessionLocal` for `Locality::Any` in the test makes it fail on "SessionLocal publisher leaked to another context" (measured, rc=101), and back again makes it pass in 1.18 s. The test publishes on two topics from one context and asserts a control topic DOES cross to a second context over the same router within the same deadline. Without that control, "the other context received nothing" would be indistinguishable from a broken router or a wrong topic. --- .../hiroz-tests/tests/publisher_locality.rs | 140 ++++++++++++++++++ .../hiroz/src/dynamic/tests/pubsub_tests.rs | 2 + crates/hiroz/src/lib.rs | 4 + crates/hiroz/src/node.rs | 1 + crates/hiroz/src/parameter/service.rs | 1 + crates/hiroz/src/prelude.rs | 2 + crates/hiroz/src/pubsub.rs | 62 ++++++++ 7 files changed, 212 insertions(+) create mode 100644 crates/hiroz-tests/tests/publisher_locality.rs diff --git a/crates/hiroz-tests/tests/publisher_locality.rs b/crates/hiroz-tests/tests/publisher_locality.rs new file mode 100644 index 000000000..1d18fb699 --- /dev/null +++ b/crates/hiroz-tests/tests/publisher_locality.rs @@ -0,0 +1,140 @@ +//! `ZPubBuilder::with_locality` — does the restriction actually reach the wire? +//! +//! `Locality::SessionLocal` is the intra-process fast path. Zenoh's +//! `resolve_put` skips `primitives.send_push_consume` entirely for it, so the +//! samples never leave the session: no link, no wire encode, no shared memory. +//! Subscribers in the same session still get them, through the local callback +//! list. +//! +//! # What makes this a detector rather than a green tick +//! +//! "The other context received nothing" is worthless on its own — a broken +//! router, a wrong topic or a too-short deadline all produce the same zero. So +//! every scenario publishes on TWO topics from the SAME context: +//! +//! | topic | publisher locality | same context | other context | +//! |---|---|---|---| +//! | `local_only` | `SessionLocal` | must receive | must receive NOTHING | +//! | `control` | default (`Any`) | must receive | **must receive** | +//! +//! The control row is the point. It proves the router routes, the topics match +//! and the deadline is long enough, using the same processes and the same +//! deadline as the row under test. Without it a zero is unknown, not a pass. +//! +//! Revert `with_locality` (or pass `Locality::Any`) and `local_only` starts +//! arriving in the other context, which fails the assertion. + +mod common; + +use std::{ + sync::{ + Arc, + atomic::{AtomicUsize, Ordering}, + }, + thread, + time::{Duration, Instant}, +}; + +use common::*; +use hiroz::{Builder, Locality}; +use hiroz_msgs::std_msgs::String as RosString; + +const MESSAGES: usize = 5; +/// Generous relative to loopback delivery. It bounds the "received nothing" +/// assertions, so it has to be long enough that a slow-but-working path is not +/// mistaken for a blocked one. +const DELIVERY_DEADLINE: Duration = Duration::from_secs(5); + +fn counting_sub( + node: &hiroz::node::ZNode, + topic: &str, +) -> hiroz::Result<(hiroz::pubsub::ZSub>, Arc)> { + let count = Arc::new(AtomicUsize::new(0)); + let c = count.clone(); + let sub = node + .create_sub::(topic) + .build_with_callback(move |_msg: RosString| { + c.fetch_add(1, Ordering::SeqCst); + })?; + Ok((sub, count)) +} + +/// Poll until `f` holds or the deadline passes. Returns whether it held. +fn wait_until(f: impl Fn() -> bool) -> bool { + let start = Instant::now(); + while start.elapsed() < DELIVERY_DEADLINE { + if f() { + return true; + } + thread::sleep(Duration::from_millis(20)); + } + f() +} + +#[test] +fn session_local_publisher_is_invisible_to_another_context() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx_publisher = create_hiroz_context_with_router(&router)?; + let ctx_other = create_hiroz_context_with_router(&router)?; + + let node_pub = ctx_publisher.create_node("locality_pub").build()?; + let node_same = ctx_publisher.create_node("locality_same").build()?; + let node_other = ctx_other.create_node("locality_other").build()?; + + let (_s1, same_local) = counting_sub(&node_same, "local_only")?; + let (_s2, other_local) = counting_sub(&node_other, "local_only")?; + let (_s3, same_control) = counting_sub(&node_same, "control")?; + let (_s4, other_control) = counting_sub(&node_other, "control")?; + + let pub_local = node_pub + .create_pub::("local_only") + .with_locality(Locality::SessionLocal) + .build()?; + let pub_control = node_pub.create_pub::("control").build()?; + + // Let the router propagate the four subscriber declarations before the first + // put, or the control row can lose messages for a reason unrelated to + // locality and take the test red for the wrong cause. + wait_for_ready(Duration::from_millis(500)); + + for i in 0..MESSAGES { + let msg = RosString { + data: format!("msg-{i}"), + }; + pub_local.publish(&msg)?; + pub_control.publish(&msg)?; + } + + // The control must arrive in BOTH contexts. Assert it first: if it fails, + // the environment is broken and the SessionLocal assertions below would be + // meaningless rather than informative. + assert!( + wait_until(|| same_control.load(Ordering::SeqCst) >= MESSAGES), + "control publisher did not reach the same context: {} of {MESSAGES}", + same_control.load(Ordering::SeqCst) + ); + assert!( + wait_until(|| other_control.load(Ordering::SeqCst) >= MESSAGES), + "control publisher did not reach the other context: {} of {MESSAGES}. \ + The router is not routing, so this run proves nothing about locality.", + other_control.load(Ordering::SeqCst) + ); + + // Same session: SessionLocal still delivers. + assert!( + wait_until(|| same_local.load(Ordering::SeqCst) >= MESSAGES), + "SessionLocal publisher did not reach a subscriber in its own session: {} of {MESSAGES}", + same_local.load(Ordering::SeqCst) + ); + + // Other session: it must have delivered nothing. The control above already + // arrived over the same router within the same deadline, so a zero here is + // the restriction working, not a slow path. + assert_eq!( + other_local.load(Ordering::SeqCst), + 0, + "SessionLocal publisher leaked to another context — the restriction is not applied" + ); + + Ok(()) +} diff --git a/crates/hiroz/src/dynamic/tests/pubsub_tests.rs b/crates/hiroz/src/dynamic/tests/pubsub_tests.rs index 6cbc2c393..2624b7e2f 100644 --- a/crates/hiroz/src/dynamic/tests/pubsub_tests.rs +++ b/crates/hiroz/src/dynamic/tests/pubsub_tests.rs @@ -184,6 +184,7 @@ fn test_zpub_builder_with_dyn_schema() { shm_config: None, dyn_schema: None, encoding: None, + locality: None, _phantom_data: PhantomData, }; @@ -226,6 +227,7 @@ fn test_zpub_builder_with_serdes_preserves_schema() { shm_config: None, dyn_schema: Some(schema.clone()), encoding: None, + locality: None, _phantom_data: PhantomData, }; diff --git a/crates/hiroz/src/lib.rs b/crates/hiroz/src/lib.rs index 7cf631043..e706ddbec 100644 --- a/crates/hiroz/src/lib.rs +++ b/crates/hiroz/src/lib.rs @@ -110,6 +110,10 @@ pub use hiroz_derive::MessageTypeInfo; pub use ros_msg::{ActionTypeInfo, MessageTypeInfo, ServiceTypeInfo, WithTypeInfo}; pub use zbuf::ZBuf; pub use zenoh::Result; +/// Re-exported so callers of `with_locality` do not need a direct `zenoh` +/// dependency — and so they cannot pick a different `zenoh` version than the +/// one hiroz links. +pub use zenoh::sample::Locality; /// Builds a configured object, consuming the builder. /// diff --git a/crates/hiroz/src/node.rs b/crates/hiroz/src/node.rs index 86fe8f5e3..49b879c6f 100644 --- a/crates/hiroz/src/node.rs +++ b/crates/hiroz/src/node.rs @@ -363,6 +363,7 @@ impl ZNode { keyexpr_format: self.keyexpr_format.clone(), dyn_schema: None, encoding: None, + locality: None, _phantom_data: Default::default(), } } diff --git a/crates/hiroz/src/parameter/service.rs b/crates/hiroz/src/parameter/service.rs index 23cf67edf..14a0c7e21 100644 --- a/crates/hiroz/src/parameter/service.rs +++ b/crates/hiroz/src/parameter/service.rs @@ -358,6 +358,7 @@ impl ParameterService { keyexpr_format: keyexpr_format.clone(), dyn_schema: None, encoding: None, + locality: None, _phantom_data: Default::default(), }; diff --git a/crates/hiroz/src/prelude.rs b/crates/hiroz/src/prelude.rs index c66e74783..3357b5ab8 100644 --- a/crates/hiroz/src/prelude.rs +++ b/crates/hiroz/src/prelude.rs @@ -55,6 +55,8 @@ pub use crate::parameter::{ /// The `Result` alias used throughout hiroz (equivalent to `zenoh::Result`). pub use zenoh::Result; +/// Locality restriction for publishers and subscribers, re-exported from zenoh. +pub use zenoh::sample::Locality; /// Lifecycle node support. pub use crate::lifecycle::{ diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index 5b637d031..67a8af2be 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -149,6 +149,9 @@ pub struct ZPubBuilder> { /// Encoding format for this publisher. /// If set, all published messages will use this encoding. pub(crate) encoding: Option, + /// Locality restriction for published samples. + /// `None` leaves zenoh's default (`Locality::Any`) in place. + pub(crate) locality: Option, pub(crate) _phantom_data: PhantomData<(T, S)>, } @@ -217,6 +220,56 @@ impl ZPubBuilder { self } + /// Set the locality restriction for this publisher. + /// + /// This restricts which matching subscribers receive the published samples: + /// those in the same session, those in other sessions, or both (the + /// default). + /// + /// [`Locality::SessionLocal`] is the intra-process fast path. Zenoh skips + /// the transport entirely for it — no link, no wire encode, no shared + /// memory — and hands the payload straight to the matching subscriber + /// callbacks in this session. Every node created from one [`ZContext`] + /// shares one zenoh session, so this reaches sibling nodes in the same + /// process. + /// + /// [`ZContext`]: crate::context::ZContext + /// + /// # This makes the publisher invisible off-process + /// + /// A [`Locality::SessionLocal`] publisher is not reachable from any other + /// process, including `ros2 topic echo`. Set it only where the subscriber + /// is known to live in the same context. + /// + /// # Serialization still happens + /// + /// Zenoh payloads are bytes, so the message is still CDR-encoded on publish + /// and decoded on receive. This setting removes the transport, not the + /// serialization. Publisher SHM is wasted work under it — pair it with + /// [`without_shm`](Self::without_shm). + /// + /// # Example + /// + /// ```no_run + /// use zenoh::sample::Locality; + /// use hiroz::Builder; + /// + /// # fn main() -> zenoh::Result<()> { + /// # let ctx = hiroz::context::ZContextBuilder::default().build()?; + /// # let node = ctx.create_node("test").build()?; + /// let publisher = node + /// .create_pub::("topic") + /// .with_locality(Locality::SessionLocal) + /// .without_shm() + /// .build()?; + /// # Ok(()) + /// # } + /// ``` + pub fn with_locality(mut self, locality: zenoh::sample::Locality) -> Self { + self.locality = Some(locality); + self + } + pub fn with_serdes(self) -> ZPubBuilder { ZPubBuilder { entity: self.entity, @@ -228,6 +281,7 @@ impl ZPubBuilder { keyexpr_format: self.keyexpr_format.clone(), dyn_schema: self.dyn_schema, encoding: self.encoding, + locality: self.locality, _phantom_data: PhantomData, } } @@ -334,6 +388,14 @@ where } } + // Apply the locality restriction before .advanced(); AdvancedPublisher + // forwards its own destination onto this inner builder, so setting it + // here is what survives. + if let Some(locality) = self.locality { + pub_builder = pub_builder.allowed_destination(locality); + debug!("[PUB] Locality restriction: {:?}", locality); + } + // Build an AdvancedPublisher and apply TransientLocal config if needed. let pub_builder = pub_builder.advanced(); debug!( From 8b78e1f8e9aa6374507dfac0bd93487aa6e00bfa Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Wed, 26 Aug 2026 02:49:08 +0800 Subject: [PATCH 02/37] feat(pubsub): prototype intra-process Arc delivery without serialization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Locality::SessionLocal removed the transport but not the encoding: zenoh payloads are bytes, so CDR still ran on every message even when both endpoints were in one process. On hiroz-bench that left 64 B at 23 microseconds against copper's 0.68 — the remainder being CDR plus hiroz's own per-message work. This adds an opt-in path beside the wire path, the same shape as rclcpp's intra-process comm: ZPub::publish_shared(Arc) hands every same-session subscriber of this exact Rust type a refcount bump. Nothing is encoded or copied. ZSubBuilder::build_with_shared_callback registers on the bus AND declares a zenoh subscriber with allowed_origin(Remote), so remote traffic still arrives normally and a same-session publisher cannot deliver the same message twice. ZPubBuilder::with_intra_process_only keeps publish_shared off zenoh entirely. local_bus keys entries by (session zid, qualified topic). The zid is what makes "same session" true rather than merely "same process" — two ZContexts in one process open two sessions and must not see each other's traffic. Both ZPub and ZSub already hold an Arc, so this needs no plumbing through the node tree. Callbacks are never invoked under the registry lock. A subscriber callback commonly publishes, which re-enters the bus; the matching callbacks are cloned out and the guard dropped first. The map is nested rather than keyed by a tuple so the publisher does not allocate a String to look itself up, and a single subscriber is dispatched without allocating a Vec — at 64 B both were a measurable fraction of the path. Measured on hiroz-bench at 64 B, p50: 273 microseconds on the wire, 23.3 with SessionLocal, 5.7 here. At 1 MiB: 1966 / 1371 / 140. What fails without this: tests/intra_process.rs does not compile, since it calls the new API. As property detectors the four tests do work: - same_arc_reaches_a_same_session_subscriber asserts Arc::ptr_eq between the published and received Arc. Pointer identity cannot survive an encode/decode round trip, so this is the assertion that distinguishes the fast path from a correct-looking slow one. Swapping publish_shared for publish makes it fail with "subscriber did not receive the message" (measured, rc=101) — proof that the bus, not zenoh, is what delivered. - intra_process_only_publisher_does_not_reach_another_context carries a control topic that MUST cross to the second context over the same router within the same deadline. Without it a zero would be indistinguishable from a broken router. - a_different_rust_type_on_the_same_topic_is_not_delivered pins the TypeId gate. - dropping_the_subscriber_unregisters_it asserts delivery works first, then that it stops. Known limitations, all disclosed in the module docs: the publisher is told whether to use the wire rather than reading it off the graph, so an intra-process-only publisher with no local subscriber drops the message instead of falling back; and every receiver shares one read-only Arc rather than a unique payload being moved to a sole receiver. --- crates/hiroz-tests/tests/intra_process.rs | 212 +++++++++++++++++ .../hiroz/src/dynamic/tests/pubsub_tests.rs | 2 + crates/hiroz/src/lib.rs | 2 + crates/hiroz/src/local_bus.rs | 225 ++++++++++++++++++ crates/hiroz/src/node.rs | 1 + crates/hiroz/src/parameter/service.rs | 1 + crates/hiroz/src/pubsub.rs | 138 +++++++++++ 7 files changed, 581 insertions(+) create mode 100644 crates/hiroz-tests/tests/intra_process.rs create mode 100644 crates/hiroz/src/local_bus.rs diff --git a/crates/hiroz-tests/tests/intra_process.rs b/crates/hiroz-tests/tests/intra_process.rs new file mode 100644 index 000000000..653b260f4 --- /dev/null +++ b/crates/hiroz-tests/tests/intra_process.rs @@ -0,0 +1,212 @@ +//! Intra-process `Arc` delivery — does it really skip the serialization? +//! +//! # The assertion that matters is `Arc::ptr_eq` +//! +//! "The message arrived and its contents match" is satisfied by a CDR round +//! trip just as well as by a pointer move, so it proves nothing about which +//! path ran. Pointer identity between the `Arc` that was published and the +//! `Arc` that was received cannot survive a serialization boundary: encoding +//! and decoding necessarily produces a new allocation. That is the property +//! under test. +//! +//! # What each test pins +//! +//! | test | property | +//! |---|---| +//! | `same_arc_reaches_a_same_session_subscriber` | pointer identity — no encode, no copy | +//! | `intra_process_only_publisher_does_not_reach_another_context` | nothing goes on the wire, with a control proving the wire works | +//! | `a_different_rust_type_on_the_same_topic_is_not_delivered` | the `TypeId` gate holds | +//! | `dropping_the_subscriber_unregisters_it` | no delivery into a dead subscriber | + +mod common; + +use std::{ + sync::{ + Arc, Mutex, + atomic::{AtomicUsize, Ordering}, + }, + thread, + time::{Duration, Instant}, +}; + +use common::*; +use hiroz::Builder; +use hiroz_msgs::std_msgs::{Int32, String as RosString}; + +const DELIVERY_DEADLINE: Duration = Duration::from_secs(5); + +fn wait_until(f: impl Fn() -> bool) -> bool { + let start = Instant::now(); + while start.elapsed() < DELIVERY_DEADLINE { + if f() { + return true; + } + thread::sleep(Duration::from_millis(20)); + } + f() +} + +#[test] +fn same_arc_reaches_a_same_session_subscriber() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx = create_hiroz_context_with_router(&router)?; + let node_tx = ctx.create_node("zc_tx").build()?; + let node_rx = ctx.create_node("zc_rx").build()?; + + let received: Arc>>> = Arc::new(Mutex::new(Vec::new())); + let sink = received.clone(); + let _sub = node_rx + .create_sub::("shared") + .build_with_shared_callback(move |msg: Arc| { + sink.lock().expect("poisoned").push(msg); + })?; + + let publisher = node_tx + .create_pub::("shared") + .with_intra_process_only() + .build()?; + + let sent = Arc::new(RosString { + data: "no copies please".to_owned(), + }); + let delivered = publisher.publish_shared(sent.clone())?; + assert_eq!(delivered, 1, "expected exactly one local subscriber"); + + // Delivery is synchronous on this thread, so no waiting is needed — but + // assert on the count first so a failure says "nothing arrived" rather than + // panicking on an empty vec. + let got = received.lock().expect("poisoned"); + assert_eq!(got.len(), 1, "subscriber did not receive the message"); + assert!( + Arc::ptr_eq(&sent, &got[0]), + "received a different allocation — the message went through a copy or a \ + serialization round trip, which is exactly what this path exists to avoid" + ); + Ok(()) +} + +#[test] +fn intra_process_only_publisher_does_not_reach_another_context() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx_tx = create_hiroz_context_with_router(&router)?; + let ctx_other = create_hiroz_context_with_router(&router)?; + + let node_tx = ctx_tx.create_node("zc_tx").build()?; + let node_other = ctx_other.create_node("zc_other").build()?; + + let local_hits = Arc::new(AtomicUsize::new(0)); + let control_hits = Arc::new(AtomicUsize::new(0)); + let (c1, c2) = (local_hits.clone(), control_hits.clone()); + + let _s1 = node_other + .create_sub::("zc_local") + .build_with_callback(move |_m: RosString| { + c1.fetch_add(1, Ordering::SeqCst); + })?; + let _s2 = node_other + .create_sub::("zc_control") + .build_with_callback(move |_m: RosString| { + c2.fetch_add(1, Ordering::SeqCst); + })?; + + let pub_local = node_tx + .create_pub::("zc_local") + .with_intra_process_only() + .build()?; + let pub_control = node_tx.create_pub::("zc_control").build()?; + + wait_for_ready(Duration::from_millis(500)); + + let msg = Arc::new(RosString { + data: "hello".to_owned(), + }); + for _ in 0..5 { + pub_local.publish_shared(msg.clone())?; + pub_control.publish(&msg)?; + } + + // The control must cross. Without it, a zero on `zc_local` would be + // indistinguishable from a broken router or a wrong topic. + assert!( + wait_until(|| control_hits.load(Ordering::SeqCst) >= 5), + "control publisher did not reach the other context ({} of 5) — this run \ + proves nothing about the intra-process path", + control_hits.load(Ordering::SeqCst) + ); + assert_eq!( + local_hits.load(Ordering::SeqCst), + 0, + "an intra-process-only publisher put something on the wire" + ); + Ok(()) +} + +#[test] +fn a_different_rust_type_on_the_same_topic_is_not_delivered() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx = create_hiroz_context_with_router(&router)?; + let node = ctx.create_node("zc_types").build()?; + + let hits = Arc::new(AtomicUsize::new(0)); + let h = hits.clone(); + let _sub = node + .create_sub::("typed") + .build_with_shared_callback(move |_m: Arc| { + h.fetch_add(1, Ordering::SeqCst); + })?; + + // Same topic, different concrete Rust type. + let publisher = node + .create_pub::("typed") + .with_intra_process_only() + .build()?; + let delivered = publisher.publish_shared(Arc::new(RosString { + data: "wrong type".to_owned(), + }))?; + + assert_eq!(delivered, 0, "delivered across a type mismatch"); + assert_eq!( + hits.load(Ordering::SeqCst), + 0, + "an Int32 subscriber was handed a String — the TypeId gate is not holding" + ); + Ok(()) +} + +#[test] +fn dropping_the_subscriber_unregisters_it() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx = create_hiroz_context_with_router(&router)?; + let node = ctx.create_node("zc_drop").build()?; + + let hits = Arc::new(AtomicUsize::new(0)); + let h = hits.clone(); + let sub = node + .create_sub::("dropme") + .build_with_shared_callback(move |_m: Arc| { + h.fetch_add(1, Ordering::SeqCst); + })?; + + let publisher = node + .create_pub::("dropme") + .with_intra_process_only() + .build()?; + let msg = Arc::new(RosString { + data: "x".to_owned(), + }); + + // Positive control: it is registered right now. Without this the assertion + // below would pass just as well against a subscriber that never worked. + assert_eq!(publisher.publish_shared(msg.clone())?, 1); + assert_eq!(hits.load(Ordering::SeqCst), 1); + + drop(sub); + + assert_eq!( + publisher.publish_shared(msg)?, + 0, + "the bus still holds a registration for a dropped subscriber" + ); + assert_eq!(hits.load(Ordering::SeqCst), 1, "a dropped subscriber was called"); + Ok(()) +} diff --git a/crates/hiroz/src/dynamic/tests/pubsub_tests.rs b/crates/hiroz/src/dynamic/tests/pubsub_tests.rs index 2624b7e2f..1f7150149 100644 --- a/crates/hiroz/src/dynamic/tests/pubsub_tests.rs +++ b/crates/hiroz/src/dynamic/tests/pubsub_tests.rs @@ -185,6 +185,7 @@ fn test_zpub_builder_with_dyn_schema() { dyn_schema: None, encoding: None, locality: None, + intra_process_only: false, _phantom_data: PhantomData, }; @@ -228,6 +229,7 @@ fn test_zpub_builder_with_serdes_preserves_schema() { dyn_schema: Some(schema.clone()), encoding: None, locality: None, + intra_process_only: false, _phantom_data: PhantomData, }; diff --git a/crates/hiroz/src/lib.rs b/crates/hiroz/src/lib.rs index e706ddbec..aa5df57ec 100644 --- a/crates/hiroz/src/lib.rs +++ b/crates/hiroz/src/lib.rs @@ -71,6 +71,8 @@ pub mod msg; pub mod node; /// Convenience re-exports for common hiroz types. pub mod prelude; +/// Intra-process message bus (prototype) — `Arc` delivery without serialization. +pub mod local_bus; /// Publishers and subscribers. pub mod pubsub; /// Python FFI bridge types. diff --git a/crates/hiroz/src/local_bus.rs b/crates/hiroz/src/local_bus.rs new file mode 100644 index 000000000..27ff73d10 --- /dev/null +++ b/crates/hiroz/src/local_bus.rs @@ -0,0 +1,225 @@ +//! Intra-process message bus — **prototype**. +//! +//! Zenoh's payload is bytes by definition, so every hiroz message is CDR-encoded +//! on publish and decoded on receive even when both endpoints live in the same +//! process. [`Locality::SessionLocal`](zenoh::sample::Locality) removes the +//! transport underneath that, but not the encoding. +//! +//! This module removes the encoding, for the one case where it is safe to: the +//! publisher and the subscriber are in the same zenoh session and agree on the +//! exact same concrete Rust type. The publisher hands out an `Arc` and every +//! matching local subscriber gets a refcount bump. Nothing is serialized and the +//! payload is never copied. +//! +//! # Why this cannot be the default +//! +//! Bytes are what buy ROS its late binding: a topic is a name, and the peer is +//! discovered at run time and may be another process, another language, another +//! ROS version or another host. An `Arc` survives none of that. So this is an +//! opt-in fast path beside the wire path, never a replacement for it — the same +//! shape as rclcpp's intra-process comm. +//! +//! # Scope of the prototype +//! +//! | | this prototype | a production version | +//! |---|---|---| +//! | audience | every same-session subscriber registered here | same, plus the wire for remote ones, decided from the graph | +//! | type check | exact [`TypeId`] | same | +//! | mutability | shared `Arc`, read-only for all receivers | move a unique payload when there is exactly one receiver | +//! | choosing the path | an explicit `with_intra_process_only()` on the publisher | inferred: use the wire only while remote subscribers exist | +//! +//! The last row is the real gap. A publisher here does not ask whether anyone +//! remote is listening; it is told. See issue #36. +//! +//! # Keying +//! +//! Entries are keyed by `(session zid, topic key expression)`. The zid is what +//! makes "same session" true rather than merely "same process" — two +//! [`ZContext`](crate::context::ZContext)s in one process open two sessions and +//! must not see each other's traffic. Both `ZPub` and `ZSub` already hold an +//! `Arc`, so this needs no plumbing through the node tree. +//! +//! # Locking +//! +//! Callbacks are **never** invoked with the registry lock held. A subscriber +//! callback commonly publishes — that is exactly what the pong side of a +//! ping/pong does — and re-entering the bus under its own read guard is the +//! deadlock this workspace has already fixed three times elsewhere. The list is +//! cloned out, the guard dropped, and only then are the callbacks called. + +use std::{ + any::{Any, TypeId}, + collections::HashMap, + sync::{ + Arc, LazyLock, RwLock, + atomic::{AtomicU64, Ordering}, + }, +}; + +use tracing::debug; +use zenoh::session::ZenohId; + +/// An erased payload. Always an `Arc` for the `T` named by `type_id`. +pub type ErasedPayload = Arc; + +type LocalCallback = Arc; + +#[derive(Clone)] +struct Entry { + id: u64, + type_id: TypeId, + callback: LocalCallback, +} + +/// Nested rather than keyed by `(ZenohId, String)` on purpose: a tuple key would +/// force the publisher to allocate a `String` on every `publish` just to look +/// itself up. `HashMap` can be probed with a `&str`, so this shape +/// makes the hot path allocation-free. +type Bus = HashMap>>; + +static BUS: LazyLock> = LazyLock::new(|| RwLock::new(HashMap::new())); +static NEXT_ID: AtomicU64 = AtomicU64::new(0); + +/// Keeps a local subscription alive. Dropping it unregisters. +/// +/// `ZSub` holds one, so the registration follows the subscriber's lifetime and +/// a dropped subscriber cannot be called back into. +pub struct LocalSubscription { + zid: ZenohId, + topic: String, + id: u64, +} + +impl Drop for LocalSubscription { + fn drop(&mut self) { + let mut bus = match BUS.write() { + Ok(b) => b, + // A poisoned registry means some callback panicked. Unregistering is + // still the right move; leaving a dead entry would let a later + // publish call into a dropped subscriber. + Err(e) => e.into_inner(), + }; + let Some(topics) = bus.get_mut(&self.zid) else { + return; + }; + if let Some(entries) = topics.get_mut(&self.topic) { + entries.retain(|e| e.id != self.id); + if entries.is_empty() { + topics.remove(&self.topic); + } + } + if topics.is_empty() { + bus.remove(&self.zid); + } + } +} + +/// Register a local subscriber for `topic` on `zid`, for payloads of type `T`. +pub fn subscribe(zid: ZenohId, topic: &str, callback: F) -> LocalSubscription +where + T: Any + Send + Sync + 'static, + F: Fn(Arc) + Send + Sync + 'static, +{ + let id = NEXT_ID.fetch_add(1, Ordering::Relaxed); + let erased: LocalCallback = Arc::new(move |payload: &ErasedPayload| { + // The publisher already matched on TypeId, so this downcast holds. It is + // still checked rather than assumed — an unchecked cast here would turn + // a bookkeeping bug into undefined behaviour. + match payload.clone().downcast::() { + Ok(typed) => callback(typed), + Err(_) => tracing::error!( + "[LOCAL] payload type did not match subscriber type after a TypeId match" + ), + } + }); + + let mut bus = match BUS.write() { + Ok(b) => b, + Err(e) => e.into_inner(), + }; + bus.entry(zid) + .or_default() + .entry(topic.to_owned()) + .or_default() + .push(Entry { + id, + type_id: TypeId::of::(), + callback: erased, + }); + debug!("[LOCAL] subscribed id={id} topic={topic}"); + LocalSubscription { + zid, + topic: topic.to_owned(), + id, + } +} + +/// Deliver `payload` to every local subscriber on `topic` whose type matches. +/// +/// Returns how many callbacks were invoked. Zero means nothing local was +/// listening — the caller decides whether that is fine or whether the message +/// needs to go on the wire instead. +pub fn publish(zid: ZenohId, topic: &str, payload: Arc) -> usize +where + T: Any + Send + Sync + 'static, +{ + let wanted = TypeId::of::(); + + // Clone the matching callbacks out and DROP the guard before invoking any of + // them. A callback that publishes re-enters this function; holding the read + // guard across the call is a self-deadlock waiting for a writer to queue. + // + // One subscriber is the overwhelmingly common case, so it is carried without + // allocating a Vec — at 64 B payloads a heap allocation per message is a + // measurable fraction of the whole path. + let (single, many): (Option, Option>) = { + let bus = match BUS.read() { + Ok(b) => b, + Err(e) => e.into_inner(), + }; + let Some(entries) = bus.get(&zid).and_then(|topics| topics.get(topic)) else { + return 0; + }; + let mut matching = entries.iter().filter(|e| e.type_id == wanted); + let Some(first) = matching.next() else { + return 0; + }; + match matching.next() { + None => (Some(first.callback.clone()), None), + Some(second) => { + let mut all = vec![first.callback.clone(), second.callback.clone()]; + all.extend(matching.map(|e| e.callback.clone())); + (None, Some(all)) + } + } + }; + + let erased: ErasedPayload = payload; + match (single, many) { + (Some(cb), _) => { + cb(&erased); + 1 + } + (None, Some(all)) => { + for cb in &all { + cb(&erased); + } + all.len() + } + (None, None) => 0, + } +} + +/// How many local subscribers `topic` has on `zid`, regardless of type. +/// +/// For diagnostics and tests; the publish path does not use it. +pub fn subscriber_count(zid: ZenohId, topic: &str) -> usize { + let bus = match BUS.read() { + Ok(b) => b, + Err(e) => e.into_inner(), + }; + bus.get(&zid) + .and_then(|topics| topics.get(topic)) + .map(|e| e.len()) + .unwrap_or(0) +} diff --git a/crates/hiroz/src/node.rs b/crates/hiroz/src/node.rs index 49b879c6f..4f606600f 100644 --- a/crates/hiroz/src/node.rs +++ b/crates/hiroz/src/node.rs @@ -364,6 +364,7 @@ impl ZNode { dyn_schema: None, encoding: None, locality: None, + intra_process_only: false, _phantom_data: Default::default(), } } diff --git a/crates/hiroz/src/parameter/service.rs b/crates/hiroz/src/parameter/service.rs index 14a0c7e21..587f27b91 100644 --- a/crates/hiroz/src/parameter/service.rs +++ b/crates/hiroz/src/parameter/service.rs @@ -359,6 +359,7 @@ impl ParameterService { dyn_schema: None, encoding: None, locality: None, + intra_process_only: false, _phantom_data: Default::default(), }; diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index 67a8af2be..ecc2d905a 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -123,6 +123,13 @@ pub struct ZPub { /// If set, this encoding will be used for all published messages. encoding: Option>, graph: Arc, + /// Session id + resolved topic key, the intra-process bus key. See + /// [`crate::local_bus`]. + local_key: (zenoh::session::ZenohId, String), + /// When set, `publish_shared` delivers only to same-session subscribers and + /// never touches zenoh. Prototype stand-in for asking the graph whether any + /// remote subscriber exists. + intra_process_only: bool, _phantom_data: PhantomData<(T, S)>, } @@ -152,6 +159,9 @@ pub struct ZPubBuilder> { /// Locality restriction for published samples. /// `None` leaves zenoh's default (`Locality::Any`) in place. pub(crate) locality: Option, + /// Restrict `publish_shared` to the intra-process bus. See + /// [`ZPubBuilder::with_intra_process_only`]. + pub(crate) intra_process_only: bool, pub(crate) _phantom_data: PhantomData<(T, S)>, } @@ -270,6 +280,27 @@ impl ZPubBuilder { self } + /// **Prototype.** Restrict [`ZPub::publish_shared`] to the intra-process + /// bus: the message is handed to same-session subscribers as an `Arc` + /// and never goes near zenoh, so it is neither serialized nor transmitted. + /// + /// This affects `publish_shared` only. [`ZPub::publish`] keeps its normal + /// wire behaviour whatever this is set to. + /// + /// # Why the flag exists, and why it should not + /// + /// The real rule is "use the wire only while a remote subscriber exists", + /// which a production version reads off the graph per message. This + /// prototype is *told* instead. A publisher with this set is invisible to + /// every other process, including `ros2 topic echo`, and to any local + /// subscriber that did not register on the bus with + /// [`ZSubBuilder::build_with_shared_callback`]. Set it only where both ends + /// are known. See issue #36. + pub fn with_intra_process_only(mut self) -> Self { + self.intra_process_only = true; + self + } + pub fn with_serdes(self) -> ZPubBuilder { ZPubBuilder { entity: self.entity, @@ -282,6 +313,7 @@ impl ZPubBuilder { dyn_schema: self.dyn_schema, encoding: self.encoding, locality: self.locality, + intra_process_only: self.intra_process_only, _phantom_data: PhantomData, } } @@ -424,6 +456,8 @@ where debug!("[PUB] Using encoding: {}", enc); } + let local_key = (self.session.zid(), qualified_topic.clone()); + Ok(ZPub { entity: self.entity, sn: AtomicUsize::new(0), @@ -437,6 +471,8 @@ where dyn_schema: self.dyn_schema, encoding, graph: self.graph, + local_key, + intra_process_only: self.intra_process_only, _phantom_data: Default::default(), }) } @@ -621,6 +657,55 @@ where put_builder.await } + /// **Prototype.** Publish an already-shared message without serializing it. + /// + /// Every subscriber in the same zenoh session that registered through + /// [`ZSubBuilder::build_with_shared_callback`] for this exact `T` receives a + /// clone of the `Arc` — a refcount bump. The payload is not encoded and its + /// bytes are not copied. + /// + /// Returns how many local subscribers were delivered to. + /// + /// # Which path a message takes + /// + /// | `with_intra_process_only()` | local subscribers | what happens | + /// |---|---|---| + /// | set | any | `Arc` to each; **nothing goes on the wire** | + /// | set | none | nothing at all; returns `Ok(0)` | + /// | not set | any | `Arc` to each, **and** a normal serialized publish | + /// | not set | none | a normal serialized publish | + /// + /// Row three is deliberately wasteful and is the honest default: without + /// asking the graph who is listening, the only safe thing is to serve + /// everyone. Row two is why the flag is a prototype — it silently drops the + /// message rather than falling back. See [`crate::local_bus`] and issue #36. + /// + /// # Ordering against [`publish`](Self::publish) + /// + /// Local delivery is synchronous on the calling thread: the subscriber + /// callbacks have run by the time this returns. A message sent with + /// `publish` travels the wire and arrives later. Interleaving the two on one + /// topic gives no ordering guarantee between them. + pub fn publish_shared(&self, msg: Arc) -> Result + where + T: Send + Sync + 'static, + { + let delivered = crate::local_bus::publish(self.local_key.0, &self.local_key.1, msg.clone()); + + if self.intra_process_only { + if delivered == 0 { + debug!( + "[PUB] intra-process only, no local subscriber on {}: message dropped", + self.entity.topic + ); + } + return Ok(delivered); + } + + self.publish(&msg)?; + Ok(delivered) + } + /// Publish pre-serialized data directly /// /// Accepts any type that implements `Into`: @@ -925,10 +1010,59 @@ where graph: self.graph, dyn_schema: self.dyn_schema, expected_encoding: self.expected_encoding, + _local_sub: None, _phantom_data: Default::default(), }) } + /// **Prototype.** Build a subscriber that receives an `Arc` from a + /// same-session publisher without any serialization, and still receives + /// remote traffic over the wire as usual. + /// + /// Two registrations are made: + /// + /// | path | source | cost | + /// |---|---|---| + /// | intra-process bus | a same-session [`ZPub::publish_shared`] for this exact `T` | a refcount bump | + /// | zenoh subscriber, `allowed_origin(Remote)` | anything outside this session | the usual CDR decode | + /// + /// `Remote` on the wire path is what stops a same-session publisher that is + /// *not* `with_intra_process_only()` delivering the same message twice — + /// once as an `Arc`, once decoded. An explicit + /// [`with_locality`](Self::with_locality) is respected and left alone; the + /// double delivery is then the caller's to reason about. + /// + /// The type match is exact: a publisher of a different concrete Rust type + /// on the same topic is not delivered here, even if the ROS type name + /// agrees. See [`crate::local_bus`] and issue #36. + pub fn build_with_shared_callback(mut self, callback: F) -> Result> + where + T: Send + Sync + 'static, + F: Fn(Arc) + Send + Sync + 'static, + S: for<'a> ZDeserializer = &'a [u8], Output = T> + 'static, + { + let zid = self.session.zid(); + if self.locality.is_none() { + self.locality = Some(zenoh::sample::Locality::Remote); + } + + let callback = Arc::new(callback); + let wire_cb = callback.clone(); + let mut sub = self.build_with_callback(move |msg: T| (*wire_cb)(Arc::new(msg)))?; + + // build_with_callback qualified the topic, so read it back rather than + // re-deriving it — the publisher keys the bus on its own qualified topic + // and the two must agree exactly. + let topic = sub.entity.topic.clone(); + sub._local_sub = Some(crate::local_bus::subscribe::( + zid, + &topic, + move |arc: Arc| (*callback)(arc), + )); + debug!("[SUB] intra-process bus registered: topic={topic}"); + Ok(sub) + } + /// Build a subscriber with a callback that processes deserialized messages directly. /// /// This method creates a subscriber that invokes the provided callback for each @@ -1041,6 +1175,10 @@ pub struct ZSub { pub dyn_schema: Option>, /// Expected encoding for validation. pub expected_encoding: Option, + /// Registration on the intra-process bus, when built with + /// [`ZSubBuilder::build_with_shared_callback`]. Dropping it unregisters, so + /// a dropped subscriber cannot be called back into. + _local_sub: Option, _phantom_data: PhantomData<(T, Q, S)>, } From 827ef324020ea24a13247d9532e03fedf27c7659 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Wed, 26 Aug 2026 03:09:26 +0800 Subject: [PATCH 03/37] perf(local_bus): resolve a publisher's channel once, not per message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The registry was keyed by (zid, topic), so every publish_shared hashed a fully-qualified ROS key expression — a long string — to find its own subscriber list. At 64 B payloads, where the whole round trip is around a microsecond, two such lookups per round trip are a visible share of it. A publisher and a subscriber now resolve an Arc when they are built and never touch the registry again. Publishing is a lock, a TypeId filter and the callback; no hashing, no string. Channels are created on demand and never removed. One empty Channel per (session, topic) ever used is bounded and trivial, and keeping them means a publisher's handle stays valid across a subscriber coming and going — which a Weak-based registry would not give without re-resolving. The single-subscriber case still avoids allocating a Vec, and callbacks are still invoked with no lock held. The four tests in tests/intra_process.rs pass unchanged, including the Arc::ptr_eq identity assertion — this is a lookup change, not a delivery change. --- crates/hiroz/src/local_bus.rs | 242 +++++++++++++++++++--------------- crates/hiroz/src/pubsub.rs | 16 +-- 2 files changed, 145 insertions(+), 113 deletions(-) diff --git a/crates/hiroz/src/local_bus.rs b/crates/hiroz/src/local_bus.rs index 27ff73d10..987c499f4 100644 --- a/crates/hiroz/src/local_bus.rs +++ b/crates/hiroz/src/local_bus.rs @@ -31,14 +31,19 @@ //! The last row is the real gap. A publisher here does not ask whether anyone //! remote is listening; it is told. See issue #36. //! -//! # Keying +//! # Keying, and why a publisher resolves it once //! -//! Entries are keyed by `(session zid, topic key expression)`. The zid is what +//! Channels are keyed by `(session zid, topic key expression)`. The zid is what //! makes "same session" true rather than merely "same process" — two //! [`ZContext`](crate::context::ZContext)s in one process open two sessions and //! must not see each other's traffic. Both `ZPub` and `ZSub` already hold an //! `Arc`, so this needs no plumbing through the node tree. //! +//! A publisher takes its [`Channel`] handle when it is built and never touches +//! the registry again. Resolving per message would mean hashing a +//! fully-qualified ROS key expression on every publish, which at small payloads +//! is a visible share of the whole path. +//! //! # Locking //! //! Callbacks are **never** invoked with the registry lock held. A subscriber @@ -71,51 +76,142 @@ struct Entry { callback: LocalCallback, } -/// Nested rather than keyed by `(ZenohId, String)` on purpose: a tuple key would -/// force the publisher to allocate a `String` on every `publish` just to look -/// itself up. `HashMap` can be probed with a `&str`, so this shape -/// makes the hot path allocation-free. -type Bus = HashMap>>; +/// The subscriber list for one `(session, topic)`, resolved once. +/// +/// A publisher takes an `Arc` when it is built and never consults the +/// registry again. That matters: looking a topic up per message means hashing a +/// fully-qualified ROS key expression — a long string — on every publish, which +/// at 64 B payloads is a visible share of the whole path. +pub struct Channel { + entries: RwLock>, +} + +impl Channel { + /// Deliver `payload` to every subscriber here whose type matches, returning + /// how many were called. + /// + /// Callbacks are **never** invoked with the lock held. A subscriber callback + /// commonly publishes — that is exactly what the pong side of a ping/pong + /// does — and re-entering under its own read guard is the deadlock this + /// workspace has already fixed three times elsewhere. + pub fn publish(&self, payload: Arc) -> usize + where + T: Any + Send + Sync + 'static, + { + let wanted = TypeId::of::(); + + // One subscriber is the overwhelmingly common case and is carried + // without allocating a Vec; at 64 B a heap allocation per message is a + // measurable share of the path. + let (single, many): (Option, Option>) = { + let entries = match self.entries.read() { + Ok(e) => e, + Err(e) => e.into_inner(), + }; + let mut matching = entries.iter().filter(|e| e.type_id == wanted); + let Some(first) = matching.next() else { + return 0; + }; + match matching.next() { + None => (Some(first.callback.clone()), None), + Some(second) => { + let mut all = vec![first.callback.clone(), second.callback.clone()]; + all.extend(matching.map(|e| e.callback.clone())); + (None, Some(all)) + } + } + }; + + let erased: ErasedPayload = payload; + match (single, many) { + (Some(cb), _) => { + cb(&erased); + 1 + } + (None, Some(all)) => { + for cb in &all { + cb(&erased); + } + all.len() + } + (None, None) => 0, + } + } + + /// How many subscribers this channel has, regardless of type. Diagnostics + /// only; the publish path does not use it. + pub fn subscriber_count(&self) -> usize { + match self.entries.read() { + Ok(e) => e.len(), + Err(e) => e.into_inner().len(), + } + } +} + +/// Registry of channels, keyed by session then topic. +/// +/// Channels are created on demand and never removed. One empty `Channel` per +/// `(session, topic)` ever used is a bounded, trivial cost, and keeping them +/// means a publisher's handle stays valid across a subscriber coming and going. +type Registry = HashMap>>; -static BUS: LazyLock> = LazyLock::new(|| RwLock::new(HashMap::new())); +static BUS: LazyLock> = LazyLock::new(|| RwLock::new(HashMap::new())); static NEXT_ID: AtomicU64 = AtomicU64::new(0); +/// Resolve the channel for `(zid, topic)`, creating it if needed. +/// +/// Call this once, when a publisher or subscriber is built — not per message. +pub fn channel(zid: ZenohId, topic: &str) -> Arc { + // Fast path: it usually exists, and a read lock lets concurrent builders through. + { + let bus = match BUS.read() { + Ok(b) => b, + Err(e) => e.into_inner(), + }; + if let Some(existing) = bus.get(&zid).and_then(|topics| topics.get(topic)) { + return existing.clone(); + } + } + + let mut bus = match BUS.write() { + Ok(b) => b, + Err(e) => e.into_inner(), + }; + bus.entry(zid) + .or_default() + .entry(topic.to_owned()) + .or_insert_with(|| { + Arc::new(Channel { + entries: RwLock::new(Vec::new()), + }) + }) + .clone() +} + /// Keeps a local subscription alive. Dropping it unregisters. /// -/// `ZSub` holds one, so the registration follows the subscriber's lifetime and -/// a dropped subscriber cannot be called back into. +/// `ZSub` holds one, so the registration follows the subscriber's lifetime and a +/// dropped subscriber cannot be called back into. pub struct LocalSubscription { - zid: ZenohId, - topic: String, + channel: Arc, id: u64, } impl Drop for LocalSubscription { fn drop(&mut self) { - let mut bus = match BUS.write() { - Ok(b) => b, - // A poisoned registry means some callback panicked. Unregistering is - // still the right move; leaving a dead entry would let a later - // publish call into a dropped subscriber. + let mut entries = match self.channel.entries.write() { + Ok(e) => e, + // A poisoned list means some callback panicked. Unregistering is + // still right; leaving a dead entry would let a later publish call + // into a dropped subscriber. Err(e) => e.into_inner(), }; - let Some(topics) = bus.get_mut(&self.zid) else { - return; - }; - if let Some(entries) = topics.get_mut(&self.topic) { - entries.retain(|e| e.id != self.id); - if entries.is_empty() { - topics.remove(&self.topic); - } - } - if topics.is_empty() { - bus.remove(&self.zid); - } + entries.retain(|e| e.id != self.id); } } -/// Register a local subscriber for `topic` on `zid`, for payloads of type `T`. -pub fn subscribe(zid: ZenohId, topic: &str, callback: F) -> LocalSubscription +/// Register a subscriber on `channel` for payloads of type `T`. +pub fn subscribe(channel: Arc, callback: F) -> LocalSubscription where T: Any + Send + Sync + 'static, F: Fn(Arc) + Send + Sync + 'static, @@ -133,86 +229,22 @@ where } }); - let mut bus = match BUS.write() { - Ok(b) => b, - Err(e) => e.into_inner(), - }; - bus.entry(zid) - .or_default() - .entry(topic.to_owned()) - .or_default() - .push(Entry { + { + let mut entries = match channel.entries.write() { + Ok(e) => e, + Err(e) => e.into_inner(), + }; + entries.push(Entry { id, type_id: TypeId::of::(), callback: erased, }); - debug!("[LOCAL] subscribed id={id} topic={topic}"); - LocalSubscription { - zid, - topic: topic.to_owned(), - id, - } -} - -/// Deliver `payload` to every local subscriber on `topic` whose type matches. -/// -/// Returns how many callbacks were invoked. Zero means nothing local was -/// listening — the caller decides whether that is fine or whether the message -/// needs to go on the wire instead. -pub fn publish(zid: ZenohId, topic: &str, payload: Arc) -> usize -where - T: Any + Send + Sync + 'static, -{ - let wanted = TypeId::of::(); - - // Clone the matching callbacks out and DROP the guard before invoking any of - // them. A callback that publishes re-enters this function; holding the read - // guard across the call is a self-deadlock waiting for a writer to queue. - // - // One subscriber is the overwhelmingly common case, so it is carried without - // allocating a Vec — at 64 B payloads a heap allocation per message is a - // measurable fraction of the whole path. - let (single, many): (Option, Option>) = { - let bus = match BUS.read() { - Ok(b) => b, - Err(e) => e.into_inner(), - }; - let Some(entries) = bus.get(&zid).and_then(|topics| topics.get(topic)) else { - return 0; - }; - let mut matching = entries.iter().filter(|e| e.type_id == wanted); - let Some(first) = matching.next() else { - return 0; - }; - match matching.next() { - None => (Some(first.callback.clone()), None), - Some(second) => { - let mut all = vec![first.callback.clone(), second.callback.clone()]; - all.extend(matching.map(|e| e.callback.clone())); - (None, Some(all)) - } - } - }; - - let erased: ErasedPayload = payload; - match (single, many) { - (Some(cb), _) => { - cb(&erased); - 1 - } - (None, Some(all)) => { - for cb in &all { - cb(&erased); - } - all.len() - } - (None, None) => 0, } + debug!("[LOCAL] subscribed id={id}"); + LocalSubscription { channel, id } } -/// How many local subscribers `topic` has on `zid`, regardless of type. -/// -/// For diagnostics and tests; the publish path does not use it. +/// How many local subscribers `topic` has on `zid`. Diagnostics only. pub fn subscriber_count(zid: ZenohId, topic: &str) -> usize { let bus = match BUS.read() { Ok(b) => b, @@ -220,6 +252,6 @@ pub fn subscriber_count(zid: ZenohId, topic: &str) -> usize { }; bus.get(&zid) .and_then(|topics| topics.get(topic)) - .map(|e| e.len()) + .map(|c| c.subscriber_count()) .unwrap_or(0) } diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index ecc2d905a..b28114663 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -123,9 +123,9 @@ pub struct ZPub { /// If set, this encoding will be used for all published messages. encoding: Option>, graph: Arc, - /// Session id + resolved topic key, the intra-process bus key. See - /// [`crate::local_bus`]. - local_key: (zenoh::session::ZenohId, String), + /// Intra-process bus channel for this topic, resolved once at build time. + /// See [`crate::local_bus`]. + local_channel: Arc, /// When set, `publish_shared` delivers only to same-session subscribers and /// never touches zenoh. Prototype stand-in for asking the graph whether any /// remote subscriber exists. @@ -456,7 +456,7 @@ where debug!("[PUB] Using encoding: {}", enc); } - let local_key = (self.session.zid(), qualified_topic.clone()); + let local_channel = crate::local_bus::channel(self.session.zid(), &qualified_topic); Ok(ZPub { entity: self.entity, @@ -471,7 +471,7 @@ where dyn_schema: self.dyn_schema, encoding, graph: self.graph, - local_key, + local_channel, intra_process_only: self.intra_process_only, _phantom_data: Default::default(), }) @@ -690,7 +690,7 @@ where where T: Send + Sync + 'static, { - let delivered = crate::local_bus::publish(self.local_key.0, &self.local_key.1, msg.clone()); + let delivered = self.local_channel.publish(msg.clone()); if self.intra_process_only { if delivered == 0 { @@ -1054,9 +1054,9 @@ where // re-deriving it — the publisher keys the bus on its own qualified topic // and the two must agree exactly. let topic = sub.entity.topic.clone(); + let channel = crate::local_bus::channel(zid, &topic); sub._local_sub = Some(crate::local_bus::subscribe::( - zid, - &topic, + channel, move |arc: Arc| (*callback)(arc), )); debug!("[SUB] intra-process bus registered: topic={topic}"); From b2403237c896542542fe9131fba4fda09325ee35 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Wed, 26 Aug 2026 14:11:47 +0800 Subject: [PATCH 04/37] feat(zbuf): let a solely owned payload buffer be written in place A publisher on the intra-process path delivers an Arc without serializing, but still allocates and fills a payload buffer per send. as_mut_slice is what lets one buffer serve many sends: it hands back this buffer's own bytes when writing them cannot be observed by anyone else, and None otherwise, so the caller allocates only when it must. It delegates to a windowed accessor added to zenoh-buffers, pinned here by a patch until that lands upstream. --- Cargo.toml | 8 +++ crates/hiroz-tests/tests/intra_process.rs | 82 ++++++++++++++++++++++- crates/hiroz/src/zbuf.rs | 48 +++++++++++++ 3 files changed, 136 insertions(+), 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 01a9f0d0a..860ae5029 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -114,3 +114,11 @@ lto = "fat" codegen-units = 1 opt-level = 3 panic = "abort" + +# zenoh-buffers, with ZSlice::as_mut_slice / ZBuf::as_mut_slice added on top of +# the 1.9.0 tag. hiroz::ZBuf::as_mut_slice delegates to it, which is what lets a +# publisher reuse one payload buffer instead of allocating per send. Upstream as +# a PR to eclipse-zenoh/zenoh; this pin goes away when it lands in a release. +# See circle/hiroz-bench#38. +[patch.crates-io] +zenoh-buffers = { git = "https://github.com/YuanYuYuan/zenoh", branch = "feat/zslice-as-mut-slice" } diff --git a/crates/hiroz-tests/tests/intra_process.rs b/crates/hiroz-tests/tests/intra_process.rs index 653b260f4..8a6e9e018 100644 --- a/crates/hiroz-tests/tests/intra_process.rs +++ b/crates/hiroz-tests/tests/intra_process.rs @@ -17,6 +17,7 @@ //! | `intra_process_only_publisher_does_not_reach_another_context` | nothing goes on the wire, with a control proving the wire works | //! | `a_different_rust_type_on_the_same_topic_is_not_delivered` | the `TypeId` gate holds | //! | `dropping_the_subscriber_unregisters_it` | no delivery into a dead subscriber | +//! | `a_pooled_payload_buffer_is_written_in_place_and_reused` | one buffer serves many sends | mod common; @@ -30,8 +31,9 @@ use std::{ }; use common::*; -use hiroz::Builder; -use hiroz_msgs::std_msgs::{Int32, String as RosString}; +use hiroz::{Builder, ZBuf}; +use hiroz_msgs::std_msgs::{ByteMultiArray, Int32, String as RosString}; +use zenoh_buffers::buffer::SplitBuffer; const DELIVERY_DEADLINE: Duration = Duration::from_secs(5); @@ -210,3 +212,79 @@ fn dropping_the_subscriber_unregisters_it() -> hiroz::Result<()> { assert_eq!(hits.load(Ordering::SeqCst), 1, "a dropped subscriber was called"); Ok(()) } + +/// One payload buffer, reused across sends, written in place. +/// +/// Delivering an `Arc` without serializing still leaves the publisher +/// allocating a payload buffer per message. This is the test for reusing one +/// instead. Two things must both hold, and neither implies the other: +/// +/// - the **payload allocation** must not move between sends, or the buffer was +/// silently reallocated and nothing was reused; +/// - the subscriber must see the value written for **that** send, or the +/// in-place write did not reach the receiver. +/// +/// The `Arc::get_mut` on each iteration is itself load bearing: it succeeds only +/// because no one still holds the previous message. That is the invariant a +/// pool has to respect, so a change that leaks a reference fails here. +#[test] +fn a_pooled_payload_buffer_is_written_in_place_and_reused() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx = create_hiroz_context_with_router(&router)?; + let node = ctx.create_node("zc_pool").build()?; + + // (address of the received Arc, address of its payload bytes, the stamp) + let seen: Arc>> = Arc::new(Mutex::new(Vec::new())); + let sink = seen.clone(); + let _sub = node + .create_sub::("pooled") + .build_with_shared_callback(move |msg: Arc| { + let bytes = msg.data.contiguous(); + let stamp = u64::from_le_bytes(bytes[0..8].try_into().expect("8 bytes")); + sink.lock() + .expect("poisoned") + .push((Arc::as_ptr(&msg) as usize, bytes.as_ptr() as usize, stamp)); + })?; + + let publisher = node + .create_pub::("pooled") + .with_intra_process_only() + .build()?; + + let mut slot = Arc::new(ByteMultiArray { + data: ZBuf::from(vec![0xAAu8; 64]), + ..Default::default() + }); + + let mut sent_payload_addrs = Vec::new(); + for stamp in [1u64, 2, 3] { + let payload = Arc::get_mut(&mut slot) + .expect("nobody still holds the previous message") + .data + .as_mut_slice() + .expect("a solely owned single-slice buffer must be writable in place"); + sent_payload_addrs.push(payload.as_ptr() as usize); + payload[0..8].copy_from_slice(&stamp.to_le_bytes()); + + assert_eq!(publisher.publish_shared(slot.clone())?, 1); + } + + let got = seen.lock().expect("poisoned"); + assert_eq!(got.len(), 3, "not every send arrived"); + + assert!( + sent_payload_addrs.windows(2).all(|w| w[0] == w[1]), + "the payload buffer moved between sends ({sent_payload_addrs:?}) — it was \ + reallocated, so nothing was reused" + ); + let slot_addr = Arc::as_ptr(&slot) as usize; + for (i, (arc_addr, payload_addr, stamp)) in got.iter().enumerate() { + assert_eq!(*arc_addr, slot_addr, "send {i} delivered a different allocation"); + assert_eq!( + *payload_addr, sent_payload_addrs[i], + "send {i} delivered a different payload buffer" + ); + assert_eq!(*stamp, i as u64 + 1, "send {i} carried a stale value"); + } + Ok(()) +} diff --git a/crates/hiroz/src/zbuf.rs b/crates/hiroz/src/zbuf.rs index f2645f047..571ce460d 100644 --- a/crates/hiroz/src/zbuf.rs +++ b/crates/hiroz/src/zbuf.rs @@ -50,6 +50,25 @@ impl ZBuf { pub fn from_zenoh(zbuf: ZenohZBuf) -> Self { Self(zbuf) } + + /// The bytes of this buffer, mutably, when they can be written in place. + /// + /// This is what lets a publisher reuse one payload buffer across sends + /// instead of allocating a new one each time. It returns `None` — and the + /// caller must then allocate — whenever writing in place would be wrong: + /// + /// - another owner holds the same backing buffer, so a write would be + /// visible to them; + /// - the buffer is not a `Vec` (a shared-memory backing, for example); + /// - the buffer is made of more than one slice, and so has no contiguous + /// mutable view. + /// + /// The bytes handed back are this buffer's own window, never the whole + /// allocation behind it. + #[inline] + pub fn as_mut_slice(&mut self) -> Option<&mut [u8]> { + self.0.as_mut_slice() + } } // Conversions @@ -269,6 +288,35 @@ mod tests { assert_eq!(bytes.as_ref(), &[1, 2, 3]); } + #[test] + fn as_mut_slice_sole_owner_writes_in_place() { + let mut zbuf = ZBuf::from(vec![0u8; 16]); + let before = zbuf.contiguous().as_ptr(); + + zbuf.as_mut_slice() + .expect("a freshly built ZBuf is solely owned")[0..8] + .copy_from_slice(&42u64.to_le_bytes()); + + let bytes = zbuf.contiguous(); + assert_eq!(u64::from_le_bytes(bytes[0..8].try_into().unwrap()), 42); + assert_eq!(bytes.as_ptr(), before, "the write reallocated"); + } + + #[test] + fn as_mut_slice_multi_slice_returns_none() { + use zenoh_buffers::ZSlice; + + let mut inner = ZenohZBuf::default(); + inner.push_zslice(ZSlice::from(vec![1u8, 2])); + inner.push_zslice(ZSlice::from(vec![3u8, 4])); + let mut zbuf = ZBuf::from_zenoh(inner); + + assert!( + zbuf.as_mut_slice().is_none(), + "a two-slice buffer has no contiguous mutable view" + ); + } + #[test] fn test_zbuf_serialize_json() { let zbuf = ZBuf::from(vec![1u8, 2, 3, 4, 5]); From 40e7df81cc612e952afbf81799f273c52690e9b3 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Wed, 26 Aug 2026 15:28:07 +0800 Subject: [PATCH 05/37] chore(deps): point the zenoh-buffers patch at the forge, not GitHub --- Cargo.toml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 860ae5029..4843513bf 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -121,4 +121,9 @@ panic = "abort" # a PR to eclipse-zenoh/zenoh; this pin goes away when it lands in a release. # See circle/hiroz-bench#38. [patch.crates-io] -zenoh-buffers = { git = "https://github.com/YuanYuYuan/zenoh", branch = "feat/zslice-as-mut-slice" } +# +# The forge is tailnet-only and the repo is private, so a build host that cannot +# reach it overrides this line to a local checkout of the same commit. The build +# job asserts the commit before it does, because a patch that silently does not +# apply is indistinguishable from a null result. +zenoh-buffers = { git = "https://forgejo.yuyuan.dev/circle/zenoh.git", branch = "feat/zslice-as-mut-slice" } From e4e9302eec6634f296716136bf797febdd4fd635 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Wed, 26 Aug 2026 19:24:29 +0800 Subject: [PATCH 06/37] chore(deps): pin the zenoh-buffers patch to a rev, not a branch A [patch.crates-io] entry is the worst place for a dependency that can move under you: nothing about the build changes when it does. The rev also carries the SHM-pending fix, which the branch tip did not when this was first pinned. --- Cargo.toml | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 4843513bf..480cefb06 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -122,8 +122,10 @@ panic = "abort" # See circle/hiroz-bench#38. [patch.crates-io] # -# The forge is tailnet-only and the repo is private, so a build host that cannot -# reach it overrides this line to a local checkout of the same commit. The build -# job asserts the commit before it does, because a patch that silently does not -# apply is indistinguishable from a null result. -zenoh-buffers = { git = "https://forgejo.yuyuan.dev/circle/zenoh.git", branch = "feat/zslice-as-mut-slice" } +# Pinned to a rev, not a branch: a [patch.crates-io] entry is the worst place for +# a dependency that can move under you, because nothing about the build changes +# when it does. The forge is tailnet-only and the repo is private, so a build +# host that cannot reach it overrides this line to a local checkout of the same +# commit — asserting the rev first, since a patch that silently does not apply is +# indistinguishable from a null result. +zenoh-buffers = { git = "https://forgejo.yuyuan.dev/circle/zenoh.git", rev = "337adf0dc" } From 9baef40b235e2043c6e7b970c2fb97250128b49f Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Thu, 27 Aug 2026 00:08:52 +0800 Subject: [PATCH 07/37] fix(pubsub): stop the bus swallowing same-session wire traffic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A subscriber built with a shared callback forced its wire half to Locality::Remote, to stop a same-session publish_shared arriving twice. That reasoning only covers publish_shared. A plain publish has no bus delivery to duplicate, so the filter discarded it outright: an ordinary publisher and an ordinary subscriber in one process, both matched in the graph, and nothing ever arrived. Suppression moves to the publisher, which is the only side that knows whether it used both paths. publish_shared now takes the bus only when its wire half cannot also reach this session — with_intra_process_only, or a Remote locality. Otherwise it sends on the wire alone: correct, and not zero-copy, which is the honest trade for a publisher that never said which audience it wanted. Also bounds intra-process delivery depth. Delivery is inline on the publishing thread, so a callback that publishes onto its own topic recursed until the stack ran out. The same shape on the wire is an endless stream of messages, which is survivable. It is now refused past a fixed depth and logged. Fixes #39. Fixes #40. --- crates/hiroz-tests/tests/intra_process.rs | 130 ++++++++++++++++++++++ crates/hiroz/src/local_bus.rs | 48 ++++++++ crates/hiroz/src/pubsub.rs | 33 +++++- 3 files changed, 205 insertions(+), 6 deletions(-) diff --git a/crates/hiroz-tests/tests/intra_process.rs b/crates/hiroz-tests/tests/intra_process.rs index 8a6e9e018..5d3b6f920 100644 --- a/crates/hiroz-tests/tests/intra_process.rs +++ b/crates/hiroz-tests/tests/intra_process.rs @@ -18,6 +18,9 @@ //! | `a_different_rust_type_on_the_same_topic_is_not_delivered` | the `TypeId` gate holds | //! | `dropping_the_subscriber_unregisters_it` | no delivery into a dead subscriber | //! | `a_pooled_payload_buffer_is_written_in_place_and_reused` | one buffer serves many sends | +//! | `a_plain_publish_reaches_a_shared_callback_subscriber` | #39 — no silent same-session loss | +//! | `publish_shared_without_a_locality_restriction_arrives_once` | #39 — and no duplicate either | +//! | `a_self_publishing_callback_does_not_recurse_without_bound` | #40 — a cycle is refused, not fatal | mod common; @@ -288,3 +291,130 @@ fn a_pooled_payload_buffer_is_written_in_place_and_reused() -> hiroz::Result<()> } Ok(()) } + +/// #39 — an ordinary publisher must reach a shared-callback subscriber. +/// +/// The bus subscriber's wire half used to be forced to `Locality::Remote`, +/// which silently discarded every same-session `publish`. Nothing errored and +/// the graph showed both endpoints matched, so only an assertion on delivery +/// can catch it. +#[test] +fn a_plain_publish_reaches_a_shared_callback_subscriber() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx = create_hiroz_context_with_router(&router)?; + let node_tx = ctx.create_node("plain_tx").build()?; + let node_rx = ctx.create_node("plain_rx").build()?; + + let hits = Arc::new(AtomicUsize::new(0)); + let h = hits.clone(); + let _sub = node_rx + .create_sub::("plain_to_shared") + .build_with_shared_callback(move |_m: Arc| { + h.fetch_add(1, Ordering::SeqCst); + })?; + + // An ordinary publisher: no locality, no bus involvement, nothing special. + let publisher = node_tx.create_pub::("plain_to_shared").build()?; + wait_for_ready(Duration::from_millis(500)); + + publisher.publish(&RosString { + data: "over the wire".to_owned(), + })?; + + assert!( + wait_until(|| hits.load(Ordering::SeqCst) >= 1), + "a same-session plain publish never reached the shared-callback subscriber" + ); + assert_eq!( + hits.load(Ordering::SeqCst), + 1, + "delivered more than once — the wire and the bus both fired" + ); + Ok(()) +} + +/// #39 — and the duplicate the old filter existed to prevent must stay prevented. +/// +/// A `publish_shared` on a publisher with no locality restriction reaches the +/// subscriber over the wire. It must not *also* arrive over the bus. +#[test] +fn publish_shared_without_a_locality_restriction_arrives_once() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx = create_hiroz_context_with_router(&router)?; + let node_tx = ctx.create_node("once_tx").build()?; + let node_rx = ctx.create_node("once_rx").build()?; + + let hits = Arc::new(AtomicUsize::new(0)); + let h = hits.clone(); + let _sub = node_rx + .create_sub::("exactly_once") + .build_with_shared_callback(move |_m: Arc| { + h.fetch_add(1, Ordering::SeqCst); + })?; + + let publisher = node_tx.create_pub::("exactly_once").build()?; + wait_for_ready(Duration::from_millis(500)); + + let delivered = publisher.publish_shared(Arc::new(RosString { + data: "once please".to_owned(), + }))?; + assert_eq!( + delivered, 0, + "the bus was used by a publisher whose wire half also reaches this session" + ); + + assert!( + wait_until(|| hits.load(Ordering::SeqCst) >= 1), + "the message did not arrive at all" + ); + thread::sleep(Duration::from_millis(300)); + assert_eq!( + hits.load(Ordering::SeqCst), + 1, + "delivered twice — once over the bus and once over the wire" + ); + Ok(()) +} + +/// #40 — a callback that publishes onto its own topic must not recurse forever. +/// +/// Delivery is inline on the publishing thread, so this is direct recursion. +/// On the wire the same shape is an endless stream of messages, which is +/// observable and survivable; here it used to end in a stack overflow. The bus +/// now refuses past a fixed depth, so the chain terminates and the test +/// returns rather than dying with SIGSEGV. +#[test] +fn a_self_publishing_callback_does_not_recurse_without_bound() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx = create_hiroz_context_with_router(&router)?; + let node = ctx.create_node("echo_node").build()?; + + let publisher = Arc::new( + node.create_pub::("echo") + .with_intra_process_only() + .build()?, + ); + let echo = publisher.clone(); + + let hits = Arc::new(AtomicUsize::new(0)); + let h = hits.clone(); + let _sub = node + .create_sub::("echo") + .build_with_shared_callback(move |m: Arc| { + h.fetch_add(1, Ordering::SeqCst); + // Republish onto the very topic this callback serves. + let _ = echo.publish_shared(m); + })?; + + publisher.publish_shared(Arc::new(RosString { + data: "round and round".to_owned(), + }))?; + + let seen = hits.load(Ordering::SeqCst); + assert!(seen >= 1, "the callback never ran"); + assert!( + seen <= 16, + "delivery recursed {seen} deep — the depth guard did not hold" + ); + Ok(()) +} diff --git a/crates/hiroz/src/local_bus.rs b/crates/hiroz/src/local_bus.rs index 987c499f4..89539cc2a 100644 --- a/crates/hiroz/src/local_bus.rs +++ b/crates/hiroz/src/local_bus.rs @@ -54,6 +54,7 @@ use std::{ any::{Any, TypeId}, + cell::Cell, collections::HashMap, sync::{ Arc, LazyLock, RwLock, @@ -64,6 +65,35 @@ use std::{ use tracing::debug; use zenoh::session::ZenohId; +/// How deep a chain of callback-driven publishes may go before the bus refuses. +/// +/// Delivery runs inline on the publishing thread, so a callback that publishes +/// re-enters `Channel::publish` on the same stack. A pong that publishes to a +/// *different* topic is the motivating case and terminates. A callback that +/// publishes to its **own** topic does not: on the wire that loop passes through +/// zenoh's queues and shows up as an endless stream of messages, but here it is +/// direct recursion and ends in a stack overflow. +/// +/// Eight is chosen to be far above any legitimate chain — a pipeline of eight +/// nodes each publishing from the previous one's callback, on one thread — and +/// far below the depth at which the stack is in danger. See issue #40. +const MAX_DELIVERY_DEPTH: u32 = 8; + +thread_local! { + /// Delivery depth for the current thread. Not per channel: a cycle across + /// two topics recurses just as fatally as one topic into itself. + static DELIVERY_DEPTH: Cell = const { Cell::new(0) }; +} + +/// Restores the delivery depth even if a callback panics. +struct DepthGuard; + +impl Drop for DepthGuard { + fn drop(&mut self) { + DELIVERY_DEPTH.with(|d| d.set(d.get().saturating_sub(1))); + } +} + /// An erased payload. Always an `Arc` for the `T` named by `type_id`. pub type ErasedPayload = Arc; @@ -98,6 +128,24 @@ impl Channel { where T: Any + Send + Sync + 'static, { + // Refuse to recurse without bound. A callback that publishes back onto + // its own topic would otherwise overflow the stack; returning here turns + // that into a dropped message and a loud log, which is recoverable and + // greppable. See issue #40. + let depth = DELIVERY_DEPTH.with(|d| d.get()); + if depth >= MAX_DELIVERY_DEPTH { + tracing::error!( + depth, + max = MAX_DELIVERY_DEPTH, + "intra-process delivery nested too deeply; refusing to recurse further. \ + A subscriber callback is publishing onto a topic that reaches itself. \ + The message was dropped." + ); + return 0; + } + DELIVERY_DEPTH.with(|d| d.set(depth + 1)); + let _depth_guard = DepthGuard; + let wanted = TypeId::of::(); // One subscriber is the overwhelmingly common case and is carried diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index b28114663..5cf853288 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -130,6 +130,15 @@ pub struct ZPub { /// never touches zenoh. Prototype stand-in for asking the graph whether any /// remote subscriber exists. intra_process_only: bool, + /// Whether `publish_shared` may hand the message to same-session + /// subscribers over the intra-process bus. + /// + /// True only when this publisher's wire half cannot reach a same-session + /// subscriber as well: either there is no wire half + /// (`with_intra_process_only`), or the wire half is restricted to + /// `Locality::Remote`. Otherwise a local subscriber would receive the + /// message twice, once per path. See issue #39. + bus_delivery: bool, _phantom_data: PhantomData<(T, S)>, } @@ -473,6 +482,11 @@ where graph: self.graph, local_channel, intra_process_only: self.intra_process_only, + // The bus is only safe when this publisher's wire half cannot also + // reach a same-session subscriber, or there is no wire half at all. + // See `ZPub::publish_shared` and issue #39. + bus_delivery: self.intra_process_only + || self.locality == Some(zenoh::sample::Locality::Remote), _phantom_data: Default::default(), }) } @@ -690,6 +704,16 @@ where where T: Send + Sync + 'static, { + // Without a locality restriction this publisher's wire half reaches + // same-session subscribers too, so using the bus as well would deliver + // the message twice. Take the wire alone: correct, just not zero-copy. + // `with_intra_process_only()` or `with_locality(Locality::Remote)` opts + // into the fast path. See issue #39. + if !self.bus_delivery { + self.publish(&msg)?; + return Ok(0); + } + let delivered = self.local_channel.publish(msg.clone()); if self.intra_process_only { @@ -1024,10 +1048,10 @@ where /// | path | source | cost | /// |---|---|---| /// | intra-process bus | a same-session [`ZPub::publish_shared`] for this exact `T` | a refcount bump | - /// | zenoh subscriber, `allowed_origin(Remote)` | anything outside this session | the usual CDR decode | + /// | zenoh subscriber, no origin filter | anything on the wire, near or far | the usual CDR decode | /// /// `Remote` on the wire path is what stops a same-session publisher that is - /// *not* `with_intra_process_only()` delivering the same message twice — + /// *not* restricted to `Locality::Remote` delivering the same message twice. That is now prevented on the publisher, which is the only side that knows whether it used both paths — see issue #39 — /// once as an `Arc`, once decoded. An explicit /// [`with_locality`](Self::with_locality) is respected and left alone; the /// double delivery is then the caller's to reason about. @@ -1035,16 +1059,13 @@ where /// The type match is exact: a publisher of a different concrete Rust type /// on the same topic is not delivered here, even if the ROS type name /// agrees. See [`crate::local_bus`] and issue #36. - pub fn build_with_shared_callback(mut self, callback: F) -> Result> + pub fn build_with_shared_callback(self, callback: F) -> Result> where T: Send + Sync + 'static, F: Fn(Arc) + Send + Sync + 'static, S: for<'a> ZDeserializer = &'a [u8], Output = T> + 'static, { let zid = self.session.zid(); - if self.locality.is_none() { - self.locality = Some(zenoh::sample::Locality::Remote); - } let callback = Arc::new(callback); let wire_cb = callback.clone(); From 8a772e78c125f1eb1f70558af59e4acbd3d6f0af Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Thu, 27 Aug 2026 02:07:05 +0800 Subject: [PATCH 08/37] perf(local_bus): take the locks and the refcounts off the publish path Delivery held a read lock over the subscriber list, and cloned each matching callback so the guard could be dropped before any callback ran. The clone was not optional: invoking a callback under the guard is the re-entrancy deadlock this workspace has fixed repeatedly. A snapshot removes both. A publisher loads the current list and calls through it; a subscriber coming or going swaps in a new list and leaves a publish already in flight running against the old one. Same visibility the clone gave, no atomics, and nothing is held so it cannot deadlock. Two more refcount pairs go with it. The erased callback now takes the payload by value, so a sole subscriber is handed the only reference rather than a clone of it, and publish_shared moves the message instead of cloning when the wire will not be used. --- crates/hiroz/Cargo.toml | 3 + crates/hiroz/src/local_bus.rs | 126 +++++++++++++++++++--------------- crates/hiroz/src/pubsub.rs | 7 +- 3 files changed, 80 insertions(+), 56 deletions(-) diff --git a/crates/hiroz/Cargo.toml b/crates/hiroz/Cargo.toml index f8405d3c9..23534ecb6 100644 --- a/crates/hiroz/Cargo.toml +++ b/crates/hiroz/Cargo.toml @@ -30,6 +30,9 @@ zenoh = { workspace = true, default-features = false, features = [ ] } zenoh-ext = { workspace = true } zenoh-buffers = { workspace = true } +# Snapshot for the intra-process subscriber list: the publish path reads it +# without a lock and without cloning callbacks. Already in the tree via zenoh. +arc-swap = "1.7" strum = { workspace = true, features = ["derive"] } sha2 = { workspace = true } parking_lot = { workspace = true } diff --git a/crates/hiroz/src/local_bus.rs b/crates/hiroz/src/local_bus.rs index 89539cc2a..7fc711c29 100644 --- a/crates/hiroz/src/local_bus.rs +++ b/crates/hiroz/src/local_bus.rs @@ -62,6 +62,8 @@ use std::{ }, }; +use arc_swap::ArcSwap; + use tracing::debug; use zenoh::session::ZenohId; @@ -97,7 +99,12 @@ impl Drop for DepthGuard { /// An erased payload. Always an `Arc` for the `T` named by `type_id`. pub type ErasedPayload = Arc; -type LocalCallback = Arc; +/// Takes the payload **by value**, so the single-subscriber case can move it. +/// +/// With a `&ErasedPayload` the callback had to clone before downcasting, which +/// is a refcount pair on every message. One subscriber is the overwhelmingly +/// common case, and it can be handed the only reference instead. +type LocalCallback = Arc; #[derive(Clone)] struct Entry { @@ -113,7 +120,21 @@ struct Entry { /// fully-qualified ROS key expression — a long string — on every publish, which /// at 64 B payloads is a visible share of the whole path. pub struct Channel { - entries: RwLock>, + /// Read on every publish, written only when a subscriber comes or goes. + /// + /// This was an `RwLock>`, and the publish path paid for it twice: + /// the lock itself, and an `Arc` clone of each matching callback so the + /// guard could be dropped before any callback ran. That clone was not + /// optional — invoking a callback under the guard is the re-entrancy + /// deadlock this workspace has fixed repeatedly, because a subscriber + /// callback that publishes is the normal case rather than the exotic one. + /// + /// A snapshot removes both. A publisher loads the current list and calls + /// straight through it; a subscriber coming or going swaps in a new list and + /// leaves any publish already in flight running against the old one. That is + /// the same visibility the clone gave, without the atomics, and it cannot + /// deadlock because nothing is held. + entries: ArcSwap>, } impl Channel { @@ -148,51 +169,41 @@ impl Channel { let wanted = TypeId::of::(); - // One subscriber is the overwhelmingly common case and is carried - // without allocating a Vec; at 64 B a heap allocation per message is a - // measurable share of the path. - let (single, many): (Option, Option>) = { - let entries = match self.entries.read() { - Ok(e) => e, - Err(e) => e.into_inner(), - }; - let mut matching = entries.iter().filter(|e| e.type_id == wanted); - let Some(first) = matching.next() else { - return 0; - }; - match matching.next() { - None => (Some(first.callback.clone()), None), - Some(second) => { - let mut all = vec![first.callback.clone(), second.callback.clone()]; - all.extend(matching.map(|e| e.callback.clone())); - (None, Some(all)) - } - } + let entries = self.entries.load(); + let mut matching = entries.iter().filter(|e| e.type_id == wanted); + let Some(first) = matching.next() else { + return 0; }; + let second = matching.next(); let erased: ErasedPayload = payload; - match (single, many) { - (Some(cb), _) => { - cb(&erased); + match second { + // One subscriber is the overwhelmingly common case: call straight + // through the snapshot, hand over the only reference, and touch no + // refcount at all beyond the one the caller already holds. + None => { + (first.callback)(erased); 1 } - (None, Some(all)) => { - for cb in &all { - cb(&erased); + // More than one receiver genuinely needs a reference each, so the + // clones start here and not before. + Some(second) => { + (first.callback)(erased.clone()); + (second.callback)(erased.clone()); + let mut count = 2; + for entry in matching { + (entry.callback)(erased.clone()); + count += 1; } - all.len() + count } - (None, None) => 0, } } /// How many subscribers this channel has, regardless of type. Diagnostics /// only; the publish path does not use it. pub fn subscriber_count(&self) -> usize { - match self.entries.read() { - Ok(e) => e.len(), - Err(e) => e.into_inner().len(), - } + self.entries.load().len() } } @@ -230,7 +241,7 @@ pub fn channel(zid: ZenohId, topic: &str) -> Arc { .entry(topic.to_owned()) .or_insert_with(|| { Arc::new(Channel { - entries: RwLock::new(Vec::new()), + entries: ArcSwap::from_pointee(Vec::new()), }) }) .clone() @@ -247,14 +258,17 @@ pub struct LocalSubscription { impl Drop for LocalSubscription { fn drop(&mut self) { - let mut entries = match self.channel.entries.write() { - Ok(e) => e, - // A poisoned list means some callback panicked. Unregistering is - // still right; leaving a dead entry would let a later publish call - // into a dropped subscriber. - Err(e) => e.into_inner(), - }; - entries.retain(|e| e.id != self.id); + // Swap in a list without this entry. A publish already in flight keeps + // running against the snapshot it loaded, exactly as it did when the + // list was cloned out from under a lock. + let id = self.id; + self.channel.entries.rcu(|current| { + current + .iter() + .filter(|e| e.id != id) + .cloned() + .collect::>() + }); } } @@ -265,11 +279,11 @@ where F: Fn(Arc) + Send + Sync + 'static, { let id = NEXT_ID.fetch_add(1, Ordering::Relaxed); - let erased: LocalCallback = Arc::new(move |payload: &ErasedPayload| { + let erased: LocalCallback = Arc::new(move |payload: ErasedPayload| { // The publisher already matched on TypeId, so this downcast holds. It is // still checked rather than assumed — an unchecked cast here would turn // a bookkeeping bug into undefined behaviour. - match payload.clone().downcast::() { + match payload.downcast::() { Ok(typed) => callback(typed), Err(_) => tracing::error!( "[LOCAL] payload type did not match subscriber type after a TypeId match" @@ -277,17 +291,21 @@ where } }); - { - let mut entries = match channel.entries.write() { - Ok(e) => e, - Err(e) => e.into_inner(), - }; - entries.push(Entry { + // Copy on write: rare, and it keeps the publish path free of locks. `rcu` + // may run this closure more than once under contention, so it clones rather + // than moving what it needs. + let type_id = TypeId::of::(); + channel.entries.rcu(|current| { + let mut next = Vec::with_capacity(current.len() + 1); + next.extend(current.iter().cloned()); + next.push(Entry { id, - type_id: TypeId::of::(), - callback: erased, + type_id, + callback: erased.clone(), }); - } + next + }); + debug!("[LOCAL] subscribed id={id}"); LocalSubscription { channel, id } } diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index 5cf853288..5f7fb6451 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -714,9 +714,11 @@ where return Ok(0); } - let delivered = self.local_channel.publish(msg.clone()); - + // When the wire is not used, nothing needs `msg` after delivery, so it is + // moved rather than cloned. The clone was a refcount pair on every + // message of the path this whole mechanism exists to make cheap. if self.intra_process_only { + let delivered = self.local_channel.publish(msg); if delivered == 0 { debug!( "[PUB] intra-process only, no local subscriber on {}: message dropped", @@ -726,6 +728,7 @@ where return Ok(delivered); } + let delivered = self.local_channel.publish(msg.clone()); self.publish(&msg)?; Ok(delivered) } From 302d5be0ccf91105b210f81892eb0f479ec8f4b8 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Thu, 27 Aug 2026 12:21:15 +0800 Subject: [PATCH 09/37] feat(pubsub): ask the graph, and let a sole receiver own the message The two limitations this prototype was filed with. The publisher was told whether to use the wire, so publish_shared on a publisher whose only subscriber was an ordinary one delivered nothing at all. It now reads its audience off the graph per message and takes the bus only when every subscriber that could receive is on it, which is the rule rclcpp uses. with_intra_process_only stays as an explicit override. Every receiver also shared one read-only Arc, with no way to express the move rclcpp performs when a topic has exactly one taker. publish_owned hands a sole owning subscriber the value itself, and hands it back untouched when it cannot, so the caller falls back rather than losing it. Fixes the two known limitations on #36. --- crates/hiroz-tests/tests/intra_process.rs | 132 ++++++++++++++++++++++ crates/hiroz/src/local_bus.rs | 129 +++++++++++++++++++-- crates/hiroz/src/pubsub.rs | 112 +++++++++++++++++- 3 files changed, 361 insertions(+), 12 deletions(-) diff --git a/crates/hiroz-tests/tests/intra_process.rs b/crates/hiroz-tests/tests/intra_process.rs index 5d3b6f920..51e14dd75 100644 --- a/crates/hiroz-tests/tests/intra_process.rs +++ b/crates/hiroz-tests/tests/intra_process.rs @@ -21,6 +21,9 @@ //! | `a_plain_publish_reaches_a_shared_callback_subscriber` | #39 — no silent same-session loss | //! | `publish_shared_without_a_locality_restriction_arrives_once` | #39 — and no duplicate either | //! | `a_self_publishing_callback_does_not_recurse_without_bound` | #40 — a cycle is refused, not fatal | +//! | `a_publisher_asks_the_graph_instead_of_being_told` | #36 — no drop when nobody is on the bus | +//! | `the_wire_is_used_when_a_subscriber_is_off_session` | #36 — and the bus is not, so no duplicate | +//! | `a_sole_receiver_is_given_the_message_to_own` | #36 — the move, not a shared Arc | mod common; @@ -418,3 +421,132 @@ fn a_self_publishing_callback_does_not_recurse_without_bound() -> hiroz::Result< ); Ok(()) } + +/// #36 — a publisher without the flag must read its audience off the graph. +/// +/// The prototype was *told* whether to use the wire, so `publish_shared` on a +/// publisher whose subscriber was an ordinary one delivered nothing at all. +/// Now the publisher asks: no bus subscriber means the wire carries it. +#[test] +fn a_publisher_asks_the_graph_instead_of_being_told() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx = create_hiroz_context_with_router(&router)?; + let node_tx = ctx.create_node("ask_tx").build()?; + let node_rx = ctx.create_node("ask_rx").build()?; + + let hits = Arc::new(AtomicUsize::new(0)); + let h = hits.clone(); + // An ORDINARY subscriber: not on the bus, expects decoded bytes. + let _sub = node_rx + .create_sub::("ask_graph") + .build_with_callback(move |_m: RosString| { + h.fetch_add(1, Ordering::SeqCst); + })?; + + // No locality, no flag. The publisher has to work out the audience. + let publisher = node_tx.create_pub::("ask_graph").build()?; + wait_for_ready(Duration::from_millis(500)); + + let delivered = publisher.publish_shared(Arc::new(RosString { + data: "who is listening".to_owned(), + }))?; + assert_eq!(delivered, 0, "the bus took a message its subscriber cannot decode"); + + assert!( + wait_until(|| hits.load(Ordering::SeqCst) >= 1), + "publish_shared reached nobody: the publisher neither used the bus nor the wire" + ); + thread::sleep(Duration::from_millis(300)); + assert_eq!(hits.load(Ordering::SeqCst), 1, "delivered more than once"); + Ok(()) +} + +/// #36 — an off-session subscriber forces the wire, and then the bus stays out. +/// +/// Both paths at once would deliver twice to a same-session bus subscriber. +/// The publisher takes the wire alone, so the local subscriber gets exactly one +/// copy — decoded rather than shared, which is the price of a remote audience. +#[test] +fn the_wire_is_used_when_a_subscriber_is_off_session() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx_tx = create_hiroz_context_with_router(&router)?; + let ctx_far = create_hiroz_context_with_router(&router)?; + + let node_tx = ctx_tx.create_node("mix_tx").build()?; + let node_near = ctx_tx.create_node("mix_near").build()?; + let node_far = ctx_far.create_node("mix_far").build()?; + + let near = Arc::new(AtomicUsize::new(0)); + let far = Arc::new(AtomicUsize::new(0)); + let (n, f) = (near.clone(), far.clone()); + + let _s_near = node_near + .create_sub::("mixed") + .build_with_shared_callback(move |_m: Arc| { + n.fetch_add(1, Ordering::SeqCst); + })?; + let _s_far = node_far + .create_sub::("mixed") + .build_with_callback(move |_m: RosString| { + f.fetch_add(1, Ordering::SeqCst); + })?; + + let publisher = node_tx.create_pub::("mixed").build()?; + wait_for_ready(Duration::from_millis(800)); + + publisher.publish_shared(Arc::new(RosString { + data: "both audiences".to_owned(), + }))?; + + // The far one is the control: without it crossing, a zero on the near side + // would be indistinguishable from a broken router. + assert!( + wait_until(|| far.load(Ordering::SeqCst) >= 1), + "the off-session subscriber never received it; this run proves nothing" + ); + thread::sleep(Duration::from_millis(300)); + assert_eq!( + near.load(Ordering::SeqCst), + 1, + "the near subscriber received {} copies — the bus and the wire both fired", + near.load(Ordering::SeqCst) + ); + Ok(()) +} + +/// #36 — a sole receiver is handed the message itself, not a shared `Arc`. +/// +/// The proof is that the callback can **mutate** what it receives. A shared +/// `Arc` cannot be mutated at all, so this does not compile against the +/// shared path — which is the point of having a separate one. +#[test] +fn a_sole_receiver_is_given_the_message_to_own() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx = create_hiroz_context_with_router(&router)?; + let node = ctx.create_node("owned_node").build()?; + + let seen: Arc>> = Arc::new(Mutex::new(Vec::new())); + let sink = seen.clone(); + let _sub = node + .create_sub::("owned") + .build_with_owned_callback(move |mut msg: RosString| { + // Mutating the received message is the whole property under test. + msg.data.push_str(" — mutated by its owner"); + sink.lock().expect("poisoned").push(msg.data); + })?; + + let publisher = node + .create_pub::("owned") + .with_intra_process_only() + .build()?; + + let took = publisher.publish_owned(RosString { + data: "mine".to_owned(), + })?; + assert_eq!(took, 1, "the sole owning receiver did not take the message"); + + let got = seen.lock().expect("poisoned"); + assert_eq!(got.len(), 1, "the owned callback did not run"); + assert_eq!(got[0], "mine — mutated by its owner"); + Ok(()) +} diff --git a/crates/hiroz/src/local_bus.rs b/crates/hiroz/src/local_bus.rs index 7fc711c29..bcb0017d1 100644 --- a/crates/hiroz/src/local_bus.rs +++ b/crates/hiroz/src/local_bus.rs @@ -106,11 +106,41 @@ pub type ErasedPayload = Arc; /// common case, and it can be handed the only reference instead. type LocalCallback = Arc; +/// A callback that takes the message **owned and mutable**. +/// +/// The shared path hands every receiver the same read-only `Arc`, which is +/// right when several of them want it and wrong when exactly one does: a sole +/// receiver could have been given the value itself, free to mutate or consume +/// it. This is that path. See issue #36. +type OwnedCallback = Arc) + Send + Sync>; + +#[derive(Clone)] +enum Sink { + /// Receives `Arc`; any number may coexist on a topic. + Shared(LocalCallback), + /// Receives `T` by value. Served only when it is the sole subscriber. + Owned(OwnedCallback), +} + #[derive(Clone)] struct Entry { id: u64, type_id: TypeId, - callback: LocalCallback, + sink: Sink, +} + +impl Entry { + #[inline] + fn is_shared(&self) -> bool { + matches!(self.sink, Sink::Shared(_)) + } + + #[inline] + fn call_shared(&self, payload: ErasedPayload) { + if let Sink::Shared(cb) = &self.sink { + cb(payload); + } + } } /// The subscriber list for one `(session, topic)`, resolved once. @@ -170,7 +200,10 @@ impl Channel { let wanted = TypeId::of::(); let entries = self.entries.load(); - let mut matching = entries.iter().filter(|e| e.type_id == wanted); + let mut matching = entries + .iter() + .filter(|e| e.type_id == wanted) + .filter(|e| e.is_shared()); let Some(first) = matching.next() else { return 0; }; @@ -182,17 +215,17 @@ impl Channel { // through the snapshot, hand over the only reference, and touch no // refcount at all beyond the one the caller already holds. None => { - (first.callback)(erased); + first.call_shared(erased); 1 } // More than one receiver genuinely needs a reference each, so the // clones start here and not before. Some(second) => { - (first.callback)(erased.clone()); - (second.callback)(erased.clone()); + first.call_shared(erased.clone()); + second.call_shared(erased.clone()); let mut count = 2; for entry in matching { - (entry.callback)(erased.clone()); + entry.call_shared(erased.clone()); count += 1; } count @@ -200,6 +233,48 @@ impl Channel { } } + /// Hand `payload` to a sole owning subscriber, by value. + /// + /// Returns the payload back as `Err` when it cannot be delivered that way, + /// so the caller still owns it and can fall back rather than lose it. That + /// happens when no owning subscriber of this type is registered, or when + /// anything else is subscribed as well: with a second receiver the message + /// cannot be given away, and silently downgrading to a shared delivery + /// would defeat the point of having asked for ownership. + pub fn publish_owned(&self, payload: T) -> core::result::Result<(), T> + where + T: Any + Send + 'static, + { + let wanted = TypeId::of::(); + let entries = self.entries.load(); + + let mut of_type = entries.iter().filter(|e| e.type_id == wanted); + let Some(only) = of_type.next() else { + return Err(payload); + }; + if of_type.next().is_some() { + return Err(payload); + } + let Sink::Owned(cb) = &only.sink else { + return Err(payload); + }; + + let depth = DELIVERY_DEPTH.with(|d| d.get()); + if depth >= MAX_DELIVERY_DEPTH { + tracing::error!( + depth, + max = MAX_DELIVERY_DEPTH, + "intra-process delivery nested too deeply; refusing to recurse further" + ); + return Err(payload); + } + DELIVERY_DEPTH.with(|d| d.set(depth + 1)); + let _depth_guard = DepthGuard; + + cb(Box::new(payload)); + Ok(()) + } + /// How many subscribers this channel has, regardless of type. Diagnostics /// only; the publish path does not use it. pub fn subscriber_count(&self) -> usize { @@ -301,7 +376,7 @@ where next.push(Entry { id, type_id, - callback: erased.clone(), + sink: Sink::Shared(erased.clone()), }); next }); @@ -321,3 +396,43 @@ pub fn subscriber_count(zid: ZenohId, topic: &str) -> usize { .map(|c| c.subscriber_count()) .unwrap_or(0) } + +/// Register `callback` to receive messages of type `T` **by value**. +/// +/// It is served only when it is the sole subscriber on the channel for that +/// type; see [`Channel::publish_owned`]. Registering one alongside a shared +/// subscriber is allowed, and simply means the owned path cannot give anything +/// away, so the publisher falls back to the shared or wire path. +pub fn subscribe_owned(channel: Arc, callback: F) -> LocalSubscription +where + T: Any + Send + 'static, + F: Fn(T) + Send + Sync + 'static, +{ + let id = NEXT_ID.fetch_add(1, Ordering::Relaxed); + let erased: OwnedCallback = Arc::new(move |payload: Box| { + // The publisher matched on TypeId, so this downcast holds. It is still + // checked: an unchecked cast would turn a bookkeeping bug into + // undefined behaviour. + match payload.downcast::() { + Ok(typed) => callback(*typed), + Err(_) => tracing::error!( + "[LOCAL] payload type did not match an owned subscriber after a TypeId match" + ), + } + }); + + let type_id = TypeId::of::(); + channel.entries.rcu(|current| { + let mut next = Vec::with_capacity(current.len() + 1); + next.extend(current.iter().cloned()); + next.push(Entry { + id, + type_id, + sink: Sink::Owned(erased.clone()), + }); + next + }); + + debug!("[LOCAL] subscribed id={id} (owned)"); + LocalSubscription { channel, id } +} diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index 5f7fb6451..8b1ab536d 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -714,9 +714,16 @@ where return Ok(0); } - // When the wire is not used, nothing needs `msg` after delivery, so it is - // moved rather than cloned. The clone was a refcount pair on every - // message of the path this whole mechanism exists to make cheap. + // Ask the graph, rather than being told by a flag. + // + // The bus can serve this message only if every subscriber that could + // receive it is on the bus. If anything off-session is listening, the + // wire has to carry it — and then the wire carries it for the local + // subscribers too, because sending both ways would deliver twice. + // + // `with_intra_process_only()` remains as an explicit override for a + // publisher that has decided its audience is local, and it keeps the + // old behaviour including dropping when nobody is listening. if self.intra_process_only { let delivered = self.local_channel.publish(msg); if delivered == 0 { @@ -728,9 +735,81 @@ where return Ok(delivered); } - let delivered = self.local_channel.publish(msg.clone()); + if self.bus_can_serve_everyone() { + // A clone here, unlike the intra-process-only path above, because + // the wire fallback below still needs the message. It is one + // refcount on a path that is already taking the graph lookup. + let delivered = self.local_channel.publish(msg.clone()); + if delivered > 0 { + return Ok(delivered); + } + // Nothing took it after all — the graph and the bus disagreed, which + // a subscriber dropping between the two can cause. Fall through to + // the wire rather than dropping the message. + debug!( + "[PUB] bus reported no taker on {} after the graph said local-only; using the wire", + self.entity.topic + ); + return self.publish(&msg).map(|()| 0); + } + self.publish(&msg)?; - Ok(delivered) + Ok(0) + } + + /// Publish `msg` by value, giving it away when exactly one receiver wants it. + /// + /// Where [`publish_shared`](Self::publish_shared) hands every receiver the + /// same read-only `Arc`, this hands a sole owning subscriber the value + /// itself — free to mutate it, consume it, or put it back in a pool. It is + /// the move rclcpp performs when a topic has exactly one taker. + /// + /// Falls back, in order: to the bus as a shared `Arc` if the audience is + /// entirely local, then to the wire. The message is never dropped by the + /// fallback, only by an `intra_process_only` publisher with no listener. + /// + /// Returns how many receivers took it on the bus; `0` means it went to the + /// wire. See issue #36. + pub fn publish_owned(&self, msg: T) -> Result + where + T: Send + Sync + 'static, + { + // The move only applies when the bus is serving this message at all. + let bus = self.intra_process_only || self.bus_can_serve_everyone(); + if bus { + match self.local_channel.publish_owned(msg) { + Ok(()) => return Ok(1), + // Handed back untouched: no sole owning receiver. Share it. + Err(returned) => return self.publish_shared(Arc::new(returned)), + } + } + self.publish(&msg)?; + Ok(0) + } + + /// Whether every subscriber that could receive this message is on the + /// intra-process bus, so the wire is not needed. + /// + /// Two conditions, both necessary. Every subscriber the graph knows about + /// must be in this session — an off-session one can only be reached by the + /// wire. And the count on the bus must match the count in the graph, so a + /// same-session subscriber that is *not* on the bus (an ordinary callback, + /// which expects decoded bytes) is not silently skipped. + /// + /// This is the shape rclcpp uses: compare the total subscription count + /// against the intra-process one, per message, and take the wire only when + /// they differ. See issue #36. + fn bus_can_serve_everyone(&self) -> bool { + let subs = self + .graph + .get_entities_by_topic(EndpointKind::Subscription, &self.entity.topic); + if subs.is_empty() { + return false; + } + if !subs.iter().all(|e| self.graph.is_entity_local(e)) { + return false; + } + self.local_channel.subscriber_count() >= subs.len() } /// Publish pre-serialized data directly @@ -1059,6 +1138,29 @@ where /// [`with_locality`](Self::with_locality) is respected and left alone; the /// double delivery is then the caller's to reason about. /// + /// Build a subscriber whose callback receives the message **by value**. + /// + /// Served only by [`ZPub::publish_owned`], and only when this is the sole + /// subscriber on the topic for this type — with a second receiver the + /// message cannot be given away. Otherwise the publisher falls back to a + /// shared or wire delivery, which this subscriber does not receive. + /// + /// Use it where the receiver wants to mutate or consume the message, which + /// the shared path cannot offer. See issue #36. + pub fn build_with_owned_callback(self, callback: F) -> Result> + where + T: Send + Sync + 'static, + F: Fn(T) + Send + Sync + 'static, + S: for<'a> ZDeserializer = &'a [u8], Output = T> + 'static, + { + let zid = self.session.zid(); + let mut sub = self.build_with_callback(|_m: T| {})?; + let topic = sub.entity.topic.clone(); + let channel = crate::local_bus::channel(zid, &topic); + sub._local_sub = Some(crate::local_bus::subscribe_owned::(channel, callback)); + Ok(sub) + } + /// The type match is exact: a publisher of a different concrete Rust type /// on the same topic is not delivered here, even if the ROS type name /// agrees. See [`crate::local_bus`] and issue #36. From a1f87b9af37bf3b577d52fdf9867e0aec76c50e0 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Thu, 27 Aug 2026 12:30:44 +0800 Subject: [PATCH 10/37] fix(pubsub): let the graph decide, for every publisher The previous change added a graph lookup that publish_shared could not reach: a plain publisher returned at the bus_delivery guard above it, and intra_process_only short-circuited below it, leaving the lookup live only for a Locality::Remote publisher. Three reverts left the new tests green, which is what exposed it. Drop the guard. #39 requires that one message never take both the bus and the wire, and each branch still takes exactly one. It does not require taking the wire unconditionally, which is what #36 filed. Refs #36, #39 --- crates/hiroz-tests/tests/intra_process.rs | 20 +++++++++++++++----- crates/hiroz/src/pubsub.rs | 20 ++++---------------- 2 files changed, 19 insertions(+), 21 deletions(-) diff --git a/crates/hiroz-tests/tests/intra_process.rs b/crates/hiroz-tests/tests/intra_process.rs index 51e14dd75..f781ca6b4 100644 --- a/crates/hiroz-tests/tests/intra_process.rs +++ b/crates/hiroz-tests/tests/intra_process.rs @@ -348,22 +348,27 @@ fn publish_shared_without_a_locality_restriction_arrives_once() -> hiroz::Result let node_rx = ctx.create_node("once_rx").build()?; let hits = Arc::new(AtomicUsize::new(0)); + let seen: Arc>>> = Arc::new(Mutex::new(Vec::new())); let h = hits.clone(); + let sink = seen.clone(); let _sub = node_rx .create_sub::("exactly_once") - .build_with_shared_callback(move |_m: Arc| { + .build_with_shared_callback(move |m: Arc| { h.fetch_add(1, Ordering::SeqCst); + sink.lock().expect("poisoned").push(m); })?; + // A plain publisher: no locality, no flag, nothing telling it what to do. let publisher = node_tx.create_pub::("exactly_once").build()?; wait_for_ready(Duration::from_millis(500)); - let delivered = publisher.publish_shared(Arc::new(RosString { + let sent = Arc::new(RosString { data: "once please".to_owned(), - }))?; + }); + let delivered = publisher.publish_shared(sent.clone())?; assert_eq!( - delivered, 0, - "the bus was used by a publisher whose wire half also reaches this session" + delivered, 1, + "a plain publisher took the wire even though every subscriber is on the bus" ); assert!( @@ -376,6 +381,11 @@ fn publish_shared_without_a_locality_restriction_arrives_once() -> hiroz::Result 1, "delivered twice — once over the bus and once over the wire" ); + let got = seen.lock().expect("poisoned"); + assert!( + Arc::ptr_eq(&sent, &got[0]), + "a different allocation arrived: the wire carried this, not the bus" + ); Ok(()) } diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index 8b1ab536d..a2e06d567 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -138,7 +138,6 @@ pub struct ZPub { /// (`with_intra_process_only`), or the wire half is restricted to /// `Locality::Remote`. Otherwise a local subscriber would receive the /// message twice, once per path. See issue #39. - bus_delivery: bool, _phantom_data: PhantomData<(T, S)>, } @@ -482,11 +481,6 @@ where graph: self.graph, local_channel, intra_process_only: self.intra_process_only, - // The bus is only safe when this publisher's wire half cannot also - // reach a same-session subscriber, or there is no wire half at all. - // See `ZPub::publish_shared` and issue #39. - bus_delivery: self.intra_process_only - || self.locality == Some(zenoh::sample::Locality::Remote), _phantom_data: Default::default(), }) } @@ -704,16 +698,10 @@ where where T: Send + Sync + 'static, { - // Without a locality restriction this publisher's wire half reaches - // same-session subscribers too, so using the bus as well would deliver - // the message twice. Take the wire alone: correct, just not zero-copy. - // `with_intra_process_only()` or `with_locality(Locality::Remote)` opts - // into the fast path. See issue #39. - if !self.bus_delivery { - self.publish(&msg)?; - return Ok(0); - } - + // #39 was about never using the bus AND the wire for one message, and + // that still holds: each branch below takes exactly one of them. It is + // not a reason to take the wire unconditionally, which is what this + // guard used to do and what #36 filed. // Ask the graph, rather than being told by a flag. // // The bus can serve this message only if every subscriber that could From c3c66856c5d716992e7f03d3655b8b5e14682c0b Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Thu, 27 Aug 2026 16:08:21 +0800 Subject: [PATCH 11/37] fix(pubsub): three ways a message was silently lost Found by adversarial review of #130, all introduced by this branch. An owned subscriber built its wire half as a no-op closure, so every message that did not arrive by publish_owned on the bus was discarded -- including from every remote publisher -- while the subscription still advertised itself to the graph. Both halves now run the same callback. Channel::publish reported depth exhaustion and 'nobody wanted it' as the same zero, and publish_shared read zero as 'fall back to the wire'. The wire re-enters the callback on a zenoh runtime thread, which then publishes from inside that runtime and does not return. It now returns Delivery, and only NoTaker falls through. Routing never consulted the publisher's own locality. A Remote wire half cannot reach this session, so bus and wire address disjoint audiences and both must run; taking the wire alone left same-session subscribers with nothing. The recursion test publishes behind a watchdog. Reverting the second fix makes the publish never return, and a count assertion cannot see that -- the count never gets to climb. The timeout is explicit in the test so the failure names the property instead of reporting as a hung suite. Refs #36 --- crates/hiroz-tests/tests/intra_process.rs | 174 +++++++++++++++++++++- crates/hiroz/src/local_bus.rs | 26 +++- crates/hiroz/src/pubsub.rs | 78 +++++++--- 3 files changed, 252 insertions(+), 26 deletions(-) diff --git a/crates/hiroz-tests/tests/intra_process.rs b/crates/hiroz-tests/tests/intra_process.rs index f781ca6b4..5590b3a42 100644 --- a/crates/hiroz-tests/tests/intra_process.rs +++ b/crates/hiroz-tests/tests/intra_process.rs @@ -29,7 +29,7 @@ mod common; use std::{ sync::{ - Arc, Mutex, + Arc, Mutex, OnceLock, atomic::{AtomicUsize, Ordering}, }, thread, @@ -43,6 +43,10 @@ use zenoh_buffers::buffer::SplitBuffer; const DELIVERY_DEADLINE: Duration = Duration::from_secs(5); +/// Ceiling on the self-publishing echo, so neither direction of the +/// recursion test can run forever. Far above the delivery-depth bound. +const ECHO_CAP: usize = 64; + fn wait_until(f: impl Fn() -> bool) -> bool { let start = Instant::now(); while start.elapsed() < DELIVERY_DEADLINE { @@ -560,3 +564,171 @@ fn a_sole_receiver_is_given_the_message_to_own() -> hiroz::Result<()> { assert_eq!(got[0], "mine — mutated by its owner"); Ok(()) } + +/// #36 defect 1. An owned subscriber's wire half was `|_m| {}`, so every +/// message from off-session — that is, from the entire rest of the ROS graph — +/// was discarded in silence while the subscription still advertised itself. +/// +/// The publisher here is in a *different* context, so the bus cannot carry it +/// and only the wire half can satisfy this test. +#[test] +fn an_owned_subscriber_receives_from_an_off_session_publisher() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx_rx = create_hiroz_context_with_router(&router)?; + let ctx_tx = create_hiroz_context_with_router(&router)?; + let node_rx = ctx_rx.create_node("own_wire_rx").build()?; + let node_tx = ctx_tx.create_node("own_wire_tx").build()?; + + let hits = Arc::new(AtomicUsize::new(0)); + let h = hits.clone(); + let _sub = node_rx + .create_sub::("owned_wire") + .build_with_owned_callback(move |m: RosString| { + assert_eq!(m.data, "from far away"); + h.fetch_add(1, Ordering::SeqCst); + })?; + + let publisher = node_tx.create_pub::("owned_wire").build()?; + wait_for_ready(Duration::from_millis(800)); + publisher.publish(&RosString { + data: "from far away".to_owned(), + })?; + + assert!( + wait_until(|| hits.load(Ordering::SeqCst) >= 1), + "an owned subscriber discarded a message from another session" + ); + thread::sleep(Duration::from_millis(300)); + assert_eq!(hits.load(Ordering::SeqCst), 1, "delivered more than once"); + Ok(()) +} + +/// #36 defect 2. Depth exhaustion and "no subscriber wanted it" both reported +/// zero, and the caller read zero as "fall back to the wire". The wire re-enters +/// the same callback on a zenoh thread, where the depth counter is a fresh +/// thread-local zero — so a bounded drop became an unbounded loop at wire rate. +/// +/// The publisher is plain: no flag, no locality. That is the path the existing +/// recursion test does not take. +#[test] +fn a_self_publishing_callback_does_not_escape_to_the_wire_and_loop() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx = create_hiroz_context_with_router(&router)?; + let node = ctx.create_node("loop_node").build()?; + + let hits = Arc::new(AtomicUsize::new(0)); + let h = hits.clone(); + // The publisher is stored erased, so this test does not have to name + // `ZPub`'s serializer parameter. `OnceLock` rather than `Mutex` because + // delivery is synchronous: the callback runs on the publishing thread and + // would re-enter a lock that thread already holds. + type Echo = Arc) + Send + Sync>>>; + let echo: Echo = Arc::new(OnceLock::new()); + let echo_cb = echo.clone(); + let _sub = node + .create_sub::("cycle") + .build_with_shared_callback(move |m: Arc| { + let seen = h.fetch_add(1, Ordering::SeqCst); + // Stop echoing well above the depth guard's bound but well below + // forever. Without this the unbounded case never terminates, and a + // detector that hangs tells you less than one that fails: the + // timeout does not say which property broke. + if seen < ECHO_CAP { + if let Some(publish) = echo_cb.get() { + publish(m); + } + } + })?; + + let publisher = node.create_pub::("cycle").build()?; + let _ = echo.set(Arc::new(move |m: Arc| { + let _ = publisher.publish_shared(m); + })); + wait_for_ready(Duration::from_millis(500)); + + // Publish on a worker thread behind a watchdog. Escaping to the wire does + // not merely loop: the wire callback runs on a zenoh runtime thread and + // publishes from inside it, which blocks on that runtime — a re-entrancy + // deadlock. A count assertion cannot see that, because the count never gets + // to climb. Only a timeout can, so the timeout is made explicit here rather + // than left to the CI runner, where it would report as "the suite hung" + // without naming the property. + let (done_tx, done_rx) = std::sync::mpsc::channel(); + let counter = hits.clone(); + let publish = echo.get().expect("just set").clone(); + thread::spawn(move || { + publish(Arc::new(RosString { + data: "round and round".to_owned(), + })); + thread::sleep(Duration::from_millis(400)); + let settled = counter.load(Ordering::SeqCst); + thread::sleep(Duration::from_millis(600)); + let _ = done_tx.send((settled, counter.load(Ordering::SeqCst))); + }); + + let (settled, later) = done_rx + .recv_timeout(Duration::from_secs(10)) + .expect("publish never returned: the depth guard escaped to the wire, \ + which re-enters on a zenoh thread and deadlocks on its runtime"); + assert!( + later <= 32, + "{later} deliveries from one publish (settled at {settled}): the guard \ + escaped to the wire and re-entered at depth zero" + ); + assert_eq!( + settled, later, + "delivery is still growing ({settled} -> {later}) after it should have stopped" + ); + Ok(()) +} + +/// #36 defect 3. `bus_can_serve_everyone` never consulted the publisher's own +/// locality. With `Locality::Remote` the wire half cannot reach this session, so +/// taking the wire alone left the same-session subscriber with nothing. +/// +/// The far subscriber is the positive control: without it crossing, a zero on +/// the near side would be indistinguishable from a broken router. +#[test] +fn a_remote_locality_publisher_still_reaches_a_same_session_subscriber() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx_tx = create_hiroz_context_with_router(&router)?; + let ctx_far = create_hiroz_context_with_router(&router)?; + let node_tx = ctx_tx.create_node("rem_tx").build()?; + let node_near = ctx_tx.create_node("rem_near").build()?; + let node_far = ctx_far.create_node("rem_far").build()?; + + let near = Arc::new(AtomicUsize::new(0)); + let far = Arc::new(AtomicUsize::new(0)); + let (n, f) = (near.clone(), far.clone()); + + let _s_near = node_near + .create_sub::("split") + .build_with_shared_callback(move |_m: Arc| { + n.fetch_add(1, Ordering::SeqCst); + })?; + let _s_far = node_far + .create_sub::("split") + .build_with_callback(move |_m: RosString| { + f.fetch_add(1, Ordering::SeqCst); + })?; + + let publisher = node_tx + .create_pub::("split") + .with_locality(zenoh::sample::Locality::Remote) + .build()?; + wait_for_ready(Duration::from_millis(800)); + + let delivered = publisher.publish_shared(Arc::new(RosString { + data: "both, disjointly".to_owned(), + }))?; + assert_eq!(delivered, 1, "the bus did not carry it to the near subscriber"); + + assert!( + wait_until(|| far.load(Ordering::SeqCst) >= 1), + "the off-session subscriber never received it; this run proves nothing" + ); + thread::sleep(Duration::from_millis(300)); + assert_eq!(near.load(Ordering::SeqCst), 1, "near subscriber count wrong"); + assert_eq!(far.load(Ordering::SeqCst), 1, "far subscriber count wrong"); + Ok(()) +} diff --git a/crates/hiroz/src/local_bus.rs b/crates/hiroz/src/local_bus.rs index bcb0017d1..ee31af3e1 100644 --- a/crates/hiroz/src/local_bus.rs +++ b/crates/hiroz/src/local_bus.rs @@ -167,6 +167,22 @@ pub struct Channel { entries: ArcSwap>, } +/// What one intra-process publish did. +/// +/// `NoTaker` and `DepthExceeded` both mean nothing was delivered, and they must +/// not be conflated: a caller may fall back to the wire on `NoTaker`, but doing +/// so on `DepthExceeded` re-enters the same callback on a zenoh thread with a +/// fresh depth counter and loops forever. See #36. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum Delivery { + /// Handed to this many subscribers. + Sent(usize), + /// No subscriber of this type is on the bus. + NoTaker, + /// Refused: delivery is already nested `MAX_DELIVERY_DEPTH` deep. + DepthExceeded, +} + impl Channel { /// Deliver `payload` to every subscriber here whose type matches, returning /// how many were called. @@ -175,7 +191,7 @@ impl Channel { /// commonly publishes — that is exactly what the pong side of a ping/pong /// does — and re-entering under its own read guard is the deadlock this /// workspace has already fixed three times elsewhere. - pub fn publish(&self, payload: Arc) -> usize + pub fn publish(&self, payload: Arc) -> Delivery where T: Any + Send + Sync + 'static, { @@ -192,7 +208,7 @@ impl Channel { A subscriber callback is publishing onto a topic that reaches itself. \ The message was dropped." ); - return 0; + return Delivery::DepthExceeded; } DELIVERY_DEPTH.with(|d| d.set(depth + 1)); let _depth_guard = DepthGuard; @@ -205,7 +221,7 @@ impl Channel { .filter(|e| e.type_id == wanted) .filter(|e| e.is_shared()); let Some(first) = matching.next() else { - return 0; + return Delivery::NoTaker; }; let second = matching.next(); @@ -216,7 +232,7 @@ impl Channel { // refcount at all beyond the one the caller already holds. None => { first.call_shared(erased); - 1 + Delivery::Sent(1) } // More than one receiver genuinely needs a reference each, so the // clones start here and not before. @@ -228,7 +244,7 @@ impl Channel { entry.call_shared(erased.clone()); count += 1; } - count + Delivery::Sent(count) } } } diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index a2e06d567..89d29019c 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -130,14 +130,12 @@ pub struct ZPub { /// never touches zenoh. Prototype stand-in for asking the graph whether any /// remote subscriber exists. intra_process_only: bool, - /// Whether `publish_shared` may hand the message to same-session - /// subscribers over the intra-process bus. - /// - /// True only when this publisher's wire half cannot reach a same-session - /// subscriber as well: either there is no wire half - /// (`with_intra_process_only`), or the wire half is restricted to - /// `Locality::Remote`. Otherwise a local subscriber would receive the - /// message twice, once per path. See issue #39. + /// The locality restriction applied to this publisher's wire half, if any. + /// + /// Routing consults it: a `Locality::Remote` wire half cannot reach this + /// session, so the bus and the wire address disjoint audiences and both are + /// used. Without it they overlap, and using both would deliver twice. + locality: Option, _phantom_data: PhantomData<(T, S)>, } @@ -481,6 +479,7 @@ where graph: self.graph, local_channel, intra_process_only: self.intra_process_only, + locality: self.locality, _phantom_data: Default::default(), }) } @@ -712,14 +711,35 @@ where // `with_intra_process_only()` remains as an explicit override for a // publisher that has decided its audience is local, and it keeps the // old behaviour including dropping when nobody is listening. + use crate::local_bus::Delivery; + if self.intra_process_only { - let delivered = self.local_channel.publish(msg); - if delivered == 0 { - debug!( - "[PUB] intra-process only, no local subscriber on {}: message dropped", - self.entity.topic - ); - } + return Ok(match self.local_channel.publish(msg) { + Delivery::Sent(n) => n, + Delivery::NoTaker => { + debug!( + "[PUB] intra-process only, no local subscriber on {}: message dropped", + self.entity.topic + ); + 0 + } + // Already logged at error level by the bus. Deliberate drop. + Delivery::DepthExceeded => 0, + }); + } + + // A `Locality::Remote` wire half cannot reach this session, so the bus + // and the wire address *disjoint* audiences and both must run: the bus + // for same-session subscribers, the wire for everyone else. This is the + // one case where two routes for one message is correct, because no + // subscriber is on both. Without this, a same-session bus subscriber + // received nothing at all. See #36. + if self.locality == Some(zenoh::sample::Locality::Remote) { + let delivered = match self.local_channel.publish(msg.clone()) { + Delivery::Sent(n) => n, + Delivery::NoTaker | Delivery::DepthExceeded => 0, + }; + self.publish(&msg)?; return Ok(delivered); } @@ -727,9 +747,14 @@ where // A clone here, unlike the intra-process-only path above, because // the wire fallback below still needs the message. It is one // refcount on a path that is already taking the graph lookup. - let delivered = self.local_channel.publish(msg.clone()); - if delivered > 0 { - return Ok(delivered); + match self.local_channel.publish(msg.clone()) { + Delivery::Sent(n) => return Ok(n), + // Falling through to the wire here would re-enter the same + // callback on a zenoh thread, where the depth counter is a + // fresh thread-local zero — an unbounded loop at wire rate + // instead of a bounded drop. See #36. + Delivery::DepthExceeded => return Ok(0), + Delivery::NoTaker => {} } // Nothing took it after all — the graph and the bus disagreed, which // a subscriber dropping between the two can cause. Fall through to @@ -1142,10 +1167,23 @@ where S: for<'a> ZDeserializer = &'a [u8], Output = T> + 'static, { let zid = self.session.zid(); - let mut sub = self.build_with_callback(|_m: T| {})?; + // Both halves run the same callback. The wire half is not decoration: + // without it this subscriber discards every message that does not + // arrive by `publish_owned` on the bus — including from every remote + // publisher — while still advertising a live subscription. See #36. + // + // It cannot deliver twice: a publisher takes the bus or the wire for + // any one message, never both, unless its wire half is `Remote`- + // restricted, in which case the two audiences are disjoint. + let callback = Arc::new(callback); + let wire_half = callback.clone(); + let mut sub = self.build_with_callback(move |m: T| wire_half(m))?; let topic = sub.entity.topic.clone(); let channel = crate::local_bus::channel(zid, &topic); - sub._local_sub = Some(crate::local_bus::subscribe_owned::(channel, callback)); + sub._local_sub = Some(crate::local_bus::subscribe_owned::( + channel, + move |m: T| callback(m), + )); Ok(sub) } From 03284c624bd63bc5e789c00feecbdb996635803f Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Thu, 27 Aug 2026 17:21:59 +0800 Subject: [PATCH 12/37] fix(deps): stop requiring an unreleased zenoh-buffers to build The [patch.crates-io] entry applies only from the root workspace being built, so it never reached a downstream consumer -- who resolved the registry zenoh-buffers and failed on a missing as_mut_slice. cargo publish strips the patch from the published manifest but verifies with the workspace manifest, so verification passed and the published crate would have been broken for everyone, silently. No hiroz code path needs the accessor; it exists for callers that pool their own payload buffers. It is now behind an off-by-default pooled-payload feature, and a workspace that enables it supplies its own patch. The default build resolves released zenoh-buffers. Also document on publish_shared that the bus carries no QoS: transient local is violated rather than unsupported, reliability and history have no meaning without a queue, and the attachment is absent. Refs #130 --- Cargo.toml | 15 --------------- crates/hiroz-tests/Cargo.toml | 2 ++ crates/hiroz-tests/tests/intra_process.rs | 1 + crates/hiroz/Cargo.toml | 6 ++++++ crates/hiroz/src/pubsub.rs | 16 ++++++++++++++++ crates/hiroz/src/zbuf.rs | 7 +++++++ 6 files changed, 32 insertions(+), 15 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 480cefb06..01a9f0d0a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -114,18 +114,3 @@ lto = "fat" codegen-units = 1 opt-level = 3 panic = "abort" - -# zenoh-buffers, with ZSlice::as_mut_slice / ZBuf::as_mut_slice added on top of -# the 1.9.0 tag. hiroz::ZBuf::as_mut_slice delegates to it, which is what lets a -# publisher reuse one payload buffer instead of allocating per send. Upstream as -# a PR to eclipse-zenoh/zenoh; this pin goes away when it lands in a release. -# See circle/hiroz-bench#38. -[patch.crates-io] -# -# Pinned to a rev, not a branch: a [patch.crates-io] entry is the worst place for -# a dependency that can move under you, because nothing about the build changes -# when it does. The forge is tailnet-only and the repo is private, so a build -# host that cannot reach it overrides this line to a local checkout of the same -# commit — asserting the rev first, since a patch that silently does not apply is -# indistinguishable from a null result. -zenoh-buffers = { git = "https://forgejo.yuyuan.dev/circle/zenoh.git", rev = "337adf0dc" } diff --git a/crates/hiroz-tests/Cargo.toml b/crates/hiroz-tests/Cargo.toml index 2eb259978..3003b2d68 100644 --- a/crates/hiroz-tests/Cargo.toml +++ b/crates/hiroz-tests/Cargo.toml @@ -32,6 +32,8 @@ serde = { workspace = true } # For CDR serialization in dds_interop tests serde_json = "1.0" # For parsing --json output in the hu plugin tests [features] +# Forwards to hiroz; see that crate. Off by default. +pooled-payload = ["hiroz/pooled-payload"] default = [] ros-msgs = [ "dep:hiroz-msgs", diff --git a/crates/hiroz-tests/tests/intra_process.rs b/crates/hiroz-tests/tests/intra_process.rs index 5590b3a42..b73fa6613 100644 --- a/crates/hiroz-tests/tests/intra_process.rs +++ b/crates/hiroz-tests/tests/intra_process.rs @@ -237,6 +237,7 @@ fn dropping_the_subscriber_unregisters_it() -> hiroz::Result<()> { /// The `Arc::get_mut` on each iteration is itself load bearing: it succeeds only /// because no one still holds the previous message. That is the invariant a /// pool has to respect, so a change that leaks a reference fails here. +#[cfg(feature = "pooled-payload")] #[test] fn a_pooled_payload_buffer_is_written_in_place_and_reused() -> hiroz::Result<()> { let router = TestRouter::new(); diff --git a/crates/hiroz/Cargo.toml b/crates/hiroz/Cargo.toml index 23534ecb6..a0203f86d 100644 --- a/crates/hiroz/Cargo.toml +++ b/crates/hiroz/Cargo.toml @@ -76,6 +76,12 @@ prost-build = { workspace = true, optional = true } [features] default = ["config-builders", "jazzy", "rmw-zenoh"] +# Exposes ZBuf::as_mut_slice, so a caller can reuse one payload buffer across +# sends instead of allocating per send. OFF by default and NOT usable from +# crates.io: it needs ZSlice::as_mut_slice, which is not in any released +# zenoh-buffers. A workspace that enables it must supply its own +# [patch.crates-io] for zenoh-buffers. No hiroz code path requires it. +pooled-payload = [] config-builders = [] generate-configs = [] protobuf = ["prost", "prost-build"] diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index 89d29019c..13ec93903 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -693,6 +693,22 @@ where /// callbacks have run by the time this returns. A message sent with /// `publish` travels the wire and arrives later. Interleaving the two on one /// topic gives no ordering guarantee between them. + /// # QoS is not carried on the intra-process path + /// + /// When this message goes to the bus, it does not enter the wire + /// publisher's cache and carries no attachment. Concretely: + /// + /// - **`TRANSIENT_LOCAL` is violated, not merely unsupported.** Nothing + /// published this way enters the durability cache, so a subscriber that + /// joins later is served the surviving samples *as if they were a + /// complete history*. Do not use this method on a transient-local + /// publisher. + /// - `RELIABLE` / `BEST_EFFORT` and `KEEP_LAST(n)` have no meaning here: + /// delivery is a synchronous inline call with no queue, so nothing is + /// ever dropped for congestion and no depth is ever displaced. + /// - The attachment — source gid, sequence number, source timestamp — is + /// absent, so no bus message carries ROS message-info. + /// pub fn publish_shared(&self, msg: Arc) -> Result where T: Send + Sync + 'static, diff --git a/crates/hiroz/src/zbuf.rs b/crates/hiroz/src/zbuf.rs index 571ce460d..5774535dd 100644 --- a/crates/hiroz/src/zbuf.rs +++ b/crates/hiroz/src/zbuf.rs @@ -65,6 +65,11 @@ impl ZBuf { /// /// The bytes handed back are this buffer's own window, never the whole /// allocation behind it. + /// + /// Requires the `pooled-payload` feature, which is off by default: this + /// needs an accessor that is not in any released `zenoh-buffers`, so a + /// workspace enabling it must patch that crate itself. + #[cfg(feature = "pooled-payload")] #[inline] pub fn as_mut_slice(&mut self) -> Option<&mut [u8]> { self.0.as_mut_slice() @@ -288,6 +293,7 @@ mod tests { assert_eq!(bytes.as_ref(), &[1, 2, 3]); } + #[cfg(feature = "pooled-payload")] #[test] fn as_mut_slice_sole_owner_writes_in_place() { let mut zbuf = ZBuf::from(vec![0u8; 16]); @@ -302,6 +308,7 @@ mod tests { assert_eq!(bytes.as_ptr(), before, "the write reallocated"); } + #[cfg(feature = "pooled-payload")] #[test] fn as_mut_slice_multi_slice_returns_none() { use zenoh_buffers::ZSlice; From daf5df572202aa7c15a3088f51a8d0fc85e1f64a Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Thu, 27 Aug 2026 17:41:44 +0800 Subject: [PATCH 13/37] fix(pubsub): the caller asserts the audience; the graph cannot G2 (#134). bus_can_serve_everyone inferred 'everyone is local' from the ROS liveliness graph, which does not hold: a plain zenoh subscriber on the same keyexpr -- z_sub, a storage or REST plugin, a native recorder -- declares no ROS token and is invisible to it. Such a publisher skipped the wire and that subscriber received nothing, silently. There is no count to subtract our own subscribers from, so the condition cannot be repaired and the inference is withdrawn. The bus is now taken only when the caller has asserted the audience, with with_intra_process_only() or Locality::Remote. This also closes the discovery-lag window (#135): the Remote path always publishes on the wire, so a remote subscriber whose liveliness token has not yet arrived is still served. G1 (#133). TRANSIENT_LOCAL lives in the wire publisher's cache and an intra-process-only publisher has no wire, so serving the bus would let a late-joining subscriber be handed a history the message is missing from. That combination is refused. The Remote path is unaffected: its wire publish populates the cache as before. Refs #132, #133, #134, #135 --- crates/hiroz-tests/tests/intra_process.rs | 82 +++++++++++++++--- crates/hiroz/src/pubsub.rs | 101 ++++++++-------------- 2 files changed, 104 insertions(+), 79 deletions(-) diff --git a/crates/hiroz-tests/tests/intra_process.rs b/crates/hiroz-tests/tests/intra_process.rs index b73fa6613..2a763698e 100644 --- a/crates/hiroz-tests/tests/intra_process.rs +++ b/crates/hiroz-tests/tests/intra_process.rs @@ -371,10 +371,10 @@ fn publish_shared_without_a_locality_restriction_arrives_once() -> hiroz::Result data: "once please".to_owned(), }); let delivered = publisher.publish_shared(sent.clone())?; - assert_eq!( - delivered, 1, - "a plain publisher took the wire even though every subscriber is on the bus" - ); + // The wire, because this publisher asserted nothing about its audience. + // Inferring "everyone is local" from the ROS graph is unsound: it cannot + // see a plain zenoh subscriber. See #134. + assert_eq!(delivered, 0, "the bus was taken without the caller asserting the audience"); assert!( wait_until(|| hits.load(Ordering::SeqCst) >= 1), @@ -386,11 +386,10 @@ fn publish_shared_without_a_locality_restriction_arrives_once() -> hiroz::Result 1, "delivered twice — once over the bus and once over the wire" ); + // Deliberately NOT asserting pointer identity: it arrived over the wire, so + // it is a decoded copy. The property here is exactly-once, not zero-copy. let got = seen.lock().expect("poisoned"); - assert!( - Arc::ptr_eq(&sent, &got[0]), - "a different allocation arrived: the wire carried this, not the bus" - ); + assert_eq!(got.len(), 1, "subscriber did not receive the message"); Ok(()) } @@ -443,7 +442,7 @@ fn a_self_publishing_callback_does_not_recurse_without_bound() -> hiroz::Result< /// publisher whose subscriber was an ordinary one delivered nothing at all. /// Now the publisher asks: no bus subscriber means the wire carries it. #[test] -fn a_publisher_asks_the_graph_instead_of_being_told() -> hiroz::Result<()> { +fn a_plain_publisher_takes_the_wire_for_an_ordinary_subscriber() -> hiroz::Result<()> { let router = TestRouter::new(); let ctx = create_hiroz_context_with_router(&router)?; let node_tx = ctx.create_node("ask_tx").build()?; @@ -607,10 +606,18 @@ fn an_owned_subscriber_receives_from_an_off_session_publisher() -> hiroz::Result /// #36 defect 2. Depth exhaustion and "no subscriber wanted it" both reported /// zero, and the caller read zero as "fall back to the wire". The wire re-enters /// the same callback on a zenoh thread, where the depth counter is a fresh -/// thread-local zero — so a bounded drop became an unbounded loop at wire rate. +/// thread-local zero, and the publish never returns. +/// +/// The publisher is `Locality::Remote`: the bus-taking path that does NOT use +/// `with_intra_process_only()`, which is what the existing recursion test +/// covers. Its wire half cannot reach this session, so the wire cannot echo and +/// the depth guard is the only thing bounding this. /// -/// The publisher is plain: no flag, no locality. That is the path the existing -/// recursion test does not take. +/// Deliberately not tested with a *plain* publisher: since #134 that takes the +/// wire alone, so a callback republishing onto its own topic is an ordinary +/// topic cycle — the same unbounded echo any ROS 2 node produces by subscribing +/// and publishing to one topic. That is the user's cycle to avoid, not a +/// defect, and no client library prevents it. #[test] fn a_self_publishing_callback_does_not_escape_to_the_wire_and_loop() -> hiroz::Result<()> { let router = TestRouter::new(); @@ -641,7 +648,10 @@ fn a_self_publishing_callback_does_not_escape_to_the_wire_and_loop() -> hiroz::R } })?; - let publisher = node.create_pub::("cycle").build()?; + let publisher = node + .create_pub::("cycle") + .with_locality(zenoh::sample::Locality::Remote) + .build()?; let _ = echo.set(Arc::new(move |m: Arc| { let _ = publisher.publish_shared(m); })); @@ -733,3 +743,49 @@ fn a_remote_locality_publisher_still_reaches_a_same_session_subscriber() -> hiro assert_eq!(far.load(Ordering::SeqCst), 1, "far subscriber count wrong"); Ok(()) } + +/// #133. TRANSIENT_LOCAL durability lives in the wire publisher's cache, and an +/// intra-process-only publisher has no wire. Serving the bus would let a +/// late-joining subscriber be handed a history this message is missing from — +/// wrong rather than absent — so the call is refused instead. +#[test] +fn transient_local_plus_intra_process_only_is_refused() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx = create_hiroz_context_with_router(&router)?; + let node = ctx.create_node("tl_node").build()?; + + let hits = Arc::new(AtomicUsize::new(0)); + let h = hits.clone(); + let _sub = node + .create_sub::("tl_topic") + .build_with_shared_callback(move |_m: Arc| { + h.fetch_add(1, Ordering::SeqCst); + })?; + + let publisher = node + .create_pub::("tl_topic") + .with_qos(hiroz::qos::QosProfile { + durability: hiroz::qos::QosDurability::TransientLocal, + ..Default::default() + }) + .with_intra_process_only() + .build()?; + wait_for_ready(Duration::from_millis(500)); + + let result = publisher.publish_shared(Arc::new(RosString { + data: "durable, allegedly".to_owned(), + })); + assert!( + result.is_err(), + "a transient-local publisher served the bus, which has no durability cache" + ); + // And it did not deliver it anyway: a refusal that still delivers is worse + // than either outcome on its own. + thread::sleep(Duration::from_millis(200)); + assert_eq!( + hits.load(Ordering::SeqCst), + 0, + "the call was refused but the message was delivered regardless" + ); + Ok(()) +} diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index 13ec93903..cc4ddbc01 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -713,23 +713,34 @@ where where T: Send + Sync + 'static, { - // #39 was about never using the bus AND the wire for one message, and - // that still holds: each branch below takes exactly one of them. It is - // not a reason to take the wire unconditionally, which is what this - // guard used to do and what #36 filed. - // Ask the graph, rather than being told by a flag. + // The bus is taken only when the CALLER has asserted who the audience + // is. It is not inferred from the graph. // - // The bus can serve this message only if every subscriber that could - // receive it is on the bus. If anything off-session is listening, the - // wire has to carry it — and then the wire carries it for the local - // subscribers too, because sending both ways would deliver twice. - // - // `with_intra_process_only()` remains as an explicit override for a - // publisher that has decided its audience is local, and it keeps the - // old behaviour including dropping when nobody is listening. + // Inferring it was unsound: the graph holds ROS liveliness tokens, so a + // plain zenoh subscriber on the same keyexpr — `z_sub`, a storage or + // REST plugin, a native recorder — is invisible to it. A publisher that + // concluded "everyone is local" from the graph skipped the wire and that + // subscriber received nothing, with nothing anywhere reporting a loss. + // There is no count to subtract our own subscribers from, so the + // condition cannot be repaired; the inference is withdrawn. See #134. use crate::local_bus::Delivery; if self.intra_process_only { + // TRANSIENT_LOCAL lives in the wire publisher's cache, and this + // publisher has no wire. Serving the bus would leave a late-joining + // subscriber to be handed whatever predates this call *as if it + // were the promised history* — wrong, rather than absent. Refuse + // instead, and let the caller choose. See #133. + if matches!(self.entity.qos.durability, QosDurability::TransientLocal) { + return Err(format!( + "publish_shared on a TRANSIENT_LOCAL, intra-process-only publisher for {}: \ + the intra-process path has no durability cache, so a late-joining \ + subscriber would be served a history this message is missing from. \ + Use publish(), or drop the transient-local durability.", + self.entity.topic + ) + .into()); + } return Ok(match self.local_channel.publish(msg) { Delivery::Sent(n) => n, Delivery::NoTaker => { @@ -745,11 +756,13 @@ where } // A `Locality::Remote` wire half cannot reach this session, so the bus - // and the wire address *disjoint* audiences and both must run: the bus - // for same-session subscribers, the wire for everyone else. This is the - // one case where two routes for one message is correct, because no - // subscriber is on both. Without this, a same-session bus subscriber - // received nothing at all. See #36. + // and the wire address *disjoint* audiences and both run: the bus for + // same-session subscribers, the wire for everyone else — including the + // non-ROS subscribers the graph cannot see, and any remote subscriber + // whose liveliness token has not arrived yet. This is the one case + // where two routes for one message is correct, because no subscriber is + // on both. TRANSIENT_LOCAL is safe here: the wire publish populates the + // cache exactly as it always did. if self.locality == Some(zenoh::sample::Locality::Remote) { let delivered = match self.local_channel.publish(msg.clone()) { Delivery::Sent(n) => n, @@ -759,29 +772,7 @@ where return Ok(delivered); } - if self.bus_can_serve_everyone() { - // A clone here, unlike the intra-process-only path above, because - // the wire fallback below still needs the message. It is one - // refcount on a path that is already taking the graph lookup. - match self.local_channel.publish(msg.clone()) { - Delivery::Sent(n) => return Ok(n), - // Falling through to the wire here would re-enter the same - // callback on a zenoh thread, where the depth counter is a - // fresh thread-local zero — an unbounded loop at wire rate - // instead of a bounded drop. See #36. - Delivery::DepthExceeded => return Ok(0), - Delivery::NoTaker => {} - } - // Nothing took it after all — the graph and the bus disagreed, which - // a subscriber dropping between the two can cause. Fall through to - // the wire rather than dropping the message. - debug!( - "[PUB] bus reported no taker on {} after the graph said local-only; using the wire", - self.entity.topic - ); - return self.publish(&msg).map(|()| 0); - } - + // No assertion from the caller: the wire alone, which reaches everyone. self.publish(&msg)?; Ok(0) } @@ -804,7 +795,9 @@ where T: Send + Sync + 'static, { // The move only applies when the bus is serving this message at all. - let bus = self.intra_process_only || self.bus_can_serve_everyone(); + // Same rule as `publish_shared`: the caller asserts the audience. + let bus = self.intra_process_only + || self.locality == Some(zenoh::sample::Locality::Remote); if bus { match self.local_channel.publish_owned(msg) { Ok(()) => return Ok(1), @@ -816,30 +809,6 @@ where Ok(0) } - /// Whether every subscriber that could receive this message is on the - /// intra-process bus, so the wire is not needed. - /// - /// Two conditions, both necessary. Every subscriber the graph knows about - /// must be in this session — an off-session one can only be reached by the - /// wire. And the count on the bus must match the count in the graph, so a - /// same-session subscriber that is *not* on the bus (an ordinary callback, - /// which expects decoded bytes) is not silently skipped. - /// - /// This is the shape rclcpp uses: compare the total subscription count - /// against the intra-process one, per message, and take the wire only when - /// they differ. See issue #36. - fn bus_can_serve_everyone(&self) -> bool { - let subs = self - .graph - .get_entities_by_topic(EndpointKind::Subscription, &self.entity.topic); - if subs.is_empty() { - return false; - } - if !subs.iter().all(|e| self.graph.is_entity_local(e)) { - return false; - } - self.local_channel.subscriber_count() >= subs.len() - } /// Publish pre-serialized data directly /// From 6867fb8be922d8fa4ba08a5e222547e4b30e6fa5 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Fri, 28 Aug 2026 03:08:46 +0800 Subject: [PATCH 14/37] fix(local_bus): isolate a panicking subscriber, and route user code through the re-entrancy check Four adversarial reviews. Three defects and two gaps. A panicking subscriber callback unwound into the application's publishing thread and skipped every subscriber after it in the snapshot. On the wire a panic kills one zenoh task; synchronous bus delivery made it a blast-radius regression, and delivery order is snapshot order, so which siblings were censored varied between runs. Each callback now runs inside catch_unwind, is logged, and delivery continues. The crate contract says every user-code invocation routes through the re-entrancy assertion so that calling out under a tracked lock is caught in debug builds. The bus routed none, while making that hazard more reachable than the path it replaces, not less. Both call sites now do. publish_owned returned Err(payload) for depth exhaustion as well as for 'no owning receiver'. The caller falls back on that, which for a Locality::Remote publisher put a message on the wire that the caller asked to hand to one local owner. Depth exhaustion is now a deliberate drop, matching publish at the same depth. Also: the doc claim that a dropped subscriber cannot be called back into was false. Drop unregisters, it does not quiesce, and a delivery already in flight runs to completion. That is memory-safe because the snapshot owns the closure and its captures, but it is not a barrier, and callers were told it was. The registry doc undercounted what it retains: the outer key is a per-context ZenohId, so the session dimension is unbounded. Tests: a positive control for the type-mismatch test, which a dead bus satisfied; the fan-out branch, which had no coverage at all and which a stub calling only the first subscriber would have passed; publish_owned's missing durability refusal, which is a live defect with a failing baseline today; and MAX_DELIVERY_DEPTH is exported so the recursion test asserts the exact bound instead of a ceiling twice its size. Refs #132, #133 --- crates/hiroz-tests/tests/intra_process.rs | 226 +++++++++++++++++++++- crates/hiroz/src/local_bus.rs | 67 ++++++- crates/hiroz/src/pubsub.rs | 48 ++++- 3 files changed, 320 insertions(+), 21 deletions(-) diff --git a/crates/hiroz-tests/tests/intra_process.rs b/crates/hiroz-tests/tests/intra_process.rs index 2a763698e..46bb0c78e 100644 --- a/crates/hiroz-tests/tests/intra_process.rs +++ b/crates/hiroz-tests/tests/intra_process.rs @@ -167,6 +167,26 @@ fn a_different_rust_type_on_the_same_topic_is_not_delivered() -> hiroz::Result<( h.fetch_add(1, Ordering::SeqCst); })?; + // Positive control: without it, this test passes against a bus that + // delivers nothing at all, which is the failure mode it exists to detect. + let ok_hits = Arc::new(AtomicUsize::new(0)); + let ok = ok_hits.clone(); + let _sub_ok = node + .create_sub::("typed_control") + .build_with_shared_callback(move |_m: Arc| { + ok.fetch_add(1, Ordering::SeqCst); + })?; + let control_pub = node + .create_pub::("typed_control") + .with_intra_process_only() + .build()?; + assert_eq!( + control_pub.publish_shared(Arc::new(Int32 { data: 7 }))?, + 1, + "the control did not deliver: the bus is dead, so the assertion below proves nothing" + ); + assert_eq!(ok_hits.load(Ordering::SeqCst), 1, "control subscriber not called"); + // Same topic, different concrete Rust type. let publisher = node .create_pub::("typed") @@ -418,9 +438,14 @@ fn a_self_publishing_callback_does_not_recurse_without_bound() -> hiroz::Result< let _sub = node .create_sub::("echo") .build_with_shared_callback(move |m: Arc| { - h.fetch_add(1, Ordering::SeqCst); - // Republish onto the very topic this callback serves. - let _ = echo.publish_shared(m); + let seen = h.fetch_add(1, Ordering::SeqCst); + // ECHO_CAP so that reverting the depth guard fails this test by + // assertion rather than by stack overflow, which aborts the whole + // binary and names no property at all. + if seen < ECHO_CAP { + // Republish onto the very topic this callback serves. + let _ = echo.publish_shared(m); + } })?; publisher.publish_shared(Arc::new(RosString { @@ -429,6 +454,11 @@ fn a_self_publishing_callback_does_not_recurse_without_bound() -> hiroz::Result< let seen = hits.load(Ordering::SeqCst); assert!(seen >= 1, "the callback never ran"); + assert_eq!( + seen, hiroz::local_bus::MAX_DELIVERY_DEPTH as usize, + "expected exactly the depth bound; a change to MAX_DELIVERY_DEPTH must \ + update this test rather than slip past a loose ceiling" + ); assert!( seen <= 16, "delivery recursed {seen} deep — the depth guard did not hold" @@ -789,3 +819,193 @@ fn transient_local_plus_intra_process_only_is_refused() -> hiroz::Result<()> { ); Ok(()) } + +/// The `Some(second)` branch of `Channel::publish` has no other coverage: every +/// other test in this file uses exactly one shared subscriber, so a stub that +/// called only the first and returned `Sent(1)` would pass the whole suite. +/// +/// `Arc::ptr_eq` on *every* receiver is the point. Delivering a clone to the +/// second and third is what the branch exists to avoid. +#[test] +fn every_shared_subscriber_receives_the_same_allocation() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx = create_hiroz_context_with_router(&router)?; + let node = ctx.create_node("fanout").build()?; + + let seen: Arc>>> = Arc::new(Mutex::new(Vec::new())); + let mut subs = Vec::new(); + for _ in 0..3 { + let sink = seen.clone(); + subs.push( + node.create_sub::("fanout") + .build_with_shared_callback(move |m: Arc| { + sink.lock().expect("poisoned").push(m); + })?, + ); + } + + let publisher = node + .create_pub::("fanout") + .with_intra_process_only() + .build()?; + let sent = Arc::new(RosString { + data: "one allocation, three readers".to_owned(), + }); + let delivered = publisher.publish_shared(sent.clone())?; + assert_eq!(delivered, 3, "not every subscriber was served"); + + let got = seen.lock().expect("poisoned"); + assert_eq!(got.len(), 3, "expected three deliveries"); + for (i, g) in got.iter().enumerate() { + assert!( + Arc::ptr_eq(&sent, g), + "receiver {i} was handed a different allocation: the fan-out branch copies" + ); + } + Ok(()) +} + +/// #133 reached through the sibling method. `publish_shared` refuses a +/// transient-local intra-process-only publisher; `publish_owned` took the bus +/// around that check and returned `Ok(1)`. +#[test] +fn transient_local_plus_intra_process_only_is_refused_for_owned_too() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx = create_hiroz_context_with_router(&router)?; + let node = ctx.create_node("tl_owned").build()?; + + let hits = Arc::new(AtomicUsize::new(0)); + let h = hits.clone(); + let _sub = node + .create_sub::("tl_owned_topic") + .build_with_owned_callback(move |_m: RosString| { + h.fetch_add(1, Ordering::SeqCst); + })?; + + let publisher = node + .create_pub::("tl_owned_topic") + .with_qos(hiroz::qos::QosProfile { + durability: hiroz::qos::QosDurability::TransientLocal, + ..Default::default() + }) + .with_intra_process_only() + .build()?; + wait_for_ready(Duration::from_millis(500)); + + let result = publisher.publish_owned(RosString { + data: "durable, allegedly".to_owned(), + }); + assert!( + result.is_err(), + "publish_owned served the bus for a transient-local publisher" + ); + thread::sleep(Duration::from_millis(200)); + assert_eq!( + hits.load(Ordering::SeqCst), + 0, + "refused and delivered anyway" + ); + Ok(()) +} + +/// A panicking subscriber must not censor its siblings or kill the publisher. +/// +/// Bus delivery is synchronous on the publishing thread, so without isolation a +/// panic unwinds out of `publish_shared` into the application and every +/// subscriber later in the snapshot is skipped. Delivery order is snapshot +/// order, so which siblings are lost varies between runs. +/// +/// The wire path gives this isolation for free — one panicking callback kills +/// one zenoh task. This test exists so the bus does not regress against it. +#[test] +fn a_panicking_subscriber_does_not_stop_delivery_to_the_others() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx = create_hiroz_context_with_router(&router)?; + let node = ctx.create_node("panicky").build()?; + + let before = Arc::new(AtomicUsize::new(0)); + let after = Arc::new(AtomicUsize::new(0)); + let (b, a) = (before.clone(), after.clone()); + + // The panicking subscriber is registered FIRST, deliberately. Delivery + // walks the snapshot in registration order and the fan-out branch invokes + // the first entry on its own line, so a panic anywhere later leaves that + // line untested — which is how an earlier version of this test passed + // against the isolation being removed. + let _s1 = node + .create_sub::("panicky") + .build_with_shared_callback(move |_m: Arc| { + panic!("this subscriber is deliberately broken"); + })?; + let _s2 = node + .create_sub::("panicky") + .build_with_shared_callback(move |_m: Arc| { + b.fetch_add(1, Ordering::SeqCst); + })?; + let _s3 = node + .create_sub::("panicky") + .build_with_shared_callback(move |_m: Arc| { + a.fetch_add(1, Ordering::SeqCst); + })?; + + let publisher = node + .create_pub::("panicky") + .with_intra_process_only() + .build()?; + + // The panic is reported by the default hook; silence it so the test output + // is readable, and restore the hook afterwards. + let prev = std::panic::take_hook(); + std::panic::set_hook(Box::new(|_| {})); + let delivered = publisher.publish_shared(Arc::new(RosString { + data: "one of you will panic".to_owned(), + })); + std::panic::set_hook(prev); + + // The publisher returned at all: the panic did not unwind into this thread. + let delivered = delivered.expect("the panic escaped into the publishing thread"); + assert_eq!(delivered, 3, "not every subscriber was invoked"); + assert_eq!( + before.load(Ordering::SeqCst), + 1, + "the subscriber after the panicking one was skipped" + ); + assert_eq!( + after.load(Ordering::SeqCst), + 1, + "the subscriber after the panicking one was skipped: one bad callback \ + censored its siblings" + ); + Ok(()) +} + +/// The sole-subscriber branch of `Channel::publish` is a different call site +/// from the fan-out branch, and a panic there has nowhere else to go: it would +/// unwind straight out of `publish_shared` into the caller. +#[test] +fn a_panicking_sole_subscriber_does_not_reach_the_publisher() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx = create_hiroz_context_with_router(&router)?; + let node = ctx.create_node("panicky_solo").build()?; + + let _sub = node + .create_sub::("panicky_solo") + .build_with_shared_callback(move |_m: Arc| { + panic!("the only subscriber, and it is broken"); + })?; + let publisher = node + .create_pub::("panicky_solo") + .with_intra_process_only() + .build()?; + + let prev = std::panic::take_hook(); + std::panic::set_hook(Box::new(|_| {})); + let delivered = publisher.publish_shared(Arc::new(RosString { + data: "boom".to_owned(), + })); + std::panic::set_hook(prev); + + let delivered = delivered.expect("the panic escaped into the publishing thread"); + assert_eq!(delivered, 1, "the subscriber was not invoked"); + Ok(()) +} diff --git a/crates/hiroz/src/local_bus.rs b/crates/hiroz/src/local_bus.rs index ee31af3e1..fa5ae177b 100644 --- a/crates/hiroz/src/local_bus.rs +++ b/crates/hiroz/src/local_bus.rs @@ -79,7 +79,9 @@ use zenoh::session::ZenohId; /// Eight is chosen to be far above any legitimate chain — a pipeline of eight /// nodes each publishing from the previous one's callback, on one thread — and /// far below the depth at which the stack is in danger. See issue #40. -const MAX_DELIVERY_DEPTH: u32 = 8; +/// Public so a test can assert the exact bound rather than a loose ceiling: +/// a hand-copied constant lets a change to this value slip past unnoticed. +pub const MAX_DELIVERY_DEPTH: u32 = 8; thread_local! { /// Delivery depth for the current thread. Not per channel: a cycle across @@ -183,6 +185,40 @@ pub(crate) enum Delivery { DepthExceeded, } + +/// Invoke one subscriber callback, isolated. +/// +/// Two things happen here that must happen at every user-code call site. +/// +/// The crate contract in [`crate::reentrancy`] says every invocation of user +/// code is routed through `invoke_user_callback!`, so that calling out while a +/// tracked lock is held is caught in debug builds. Bus delivery is synchronous +/// on the publishing thread, which makes that hazard *more* reachable than the +/// wire path, not less. +/// +/// And a panic is contained — **under `panic = "unwind"`, which is the +/// default**. A build that sets `panic = "abort"` cannot catch anything: the +/// process ends at the panic and this isolation is inert. That is worth knowing +/// before relying on it in an embedded or abort-configured deployment. +/// +/// A panic is contained. On the wire, a panicking callback kills one zenoh +/// task. Here it would unwind into the application's publishing thread and skip +/// every subscriber after it in the snapshot. Delivery order is snapshot order, +/// so which siblings got censored would vary run to run. +fn invoke_isolated(site: &'static str, f: impl FnOnce()) -> bool { + crate::reentrancy::assert_no_guards_held(site); + match std::panic::catch_unwind(std::panic::AssertUnwindSafe(f)) { + Ok(()) => true, + Err(_) => { + tracing::error!( + "[BUS] a subscriber callback panicked during intra-process delivery at {site}; \ + the panic was contained and delivery continued to the remaining subscribers" + ); + false + } + } +} + impl Channel { /// Deliver `payload` to every subscriber here whose type matches, returning /// how many were called. @@ -231,19 +267,24 @@ impl Channel { // through the snapshot, hand over the only reference, and touch no // refcount at all beyond the one the caller already holds. None => { - first.call_shared(erased); + invoke_isolated("local_bus::publish", || first.call_shared(erased)); Delivery::Sent(1) } // More than one receiver genuinely needs a reference each, so the // clones start here and not before. Some(second) => { - first.call_shared(erased.clone()); - second.call_shared(erased.clone()); + let e1 = erased.clone(); + let e2 = erased.clone(); + invoke_isolated("local_bus::publish", || first.call_shared(e1)); + invoke_isolated("local_bus::publish", || second.call_shared(e2)); let mut count = 2; for entry in matching { - entry.call_shared(erased.clone()); + let ec = erased.clone(); + invoke_isolated("local_bus::publish", || entry.call_shared(ec)); count += 1; } + // The count is subscribers invoked, not subscribers that + // returned normally. A panicking one is logged above. Delivery::Sent(count) } } @@ -282,12 +323,17 @@ impl Channel { max = MAX_DELIVERY_DEPTH, "intra-process delivery nested too deeply; refusing to recurse further" ); - return Err(payload); + // Deliberately NOT Err(payload): the caller treats that as "no + // owning receiver" and falls back, which for a Remote-locality + // publisher puts a message on the wire that the caller asked to + // hand to one local owner. A deliberate drop is the honest answer, + // and it matches what `publish` does at the same depth. + return Ok(()); } DELIVERY_DEPTH.with(|d| d.set(depth + 1)); let _depth_guard = DepthGuard; - cb(Box::new(payload)); + invoke_isolated("local_bus::publish_owned", || cb(Box::new(payload))); Ok(()) } @@ -341,7 +387,12 @@ pub fn channel(zid: ZenohId, topic: &str) -> Arc { /// Keeps a local subscription alive. Dropping it unregisters. /// /// `ZSub` holds one, so the registration follows the subscriber's lifetime and a -/// dropped subscriber cannot be called back into. +/// dropped subscriber receives no *further* deliveries. +/// It does not quiesce: a delivery already in flight on another thread runs to +/// completion against the snapshot it loaded, so the callback can still run after +/// `drop` has returned. That is memory-safe, because the snapshot owns the closure +/// and everything it captured. It is not a barrier, so do not tear down a resource +/// the callback merely observes on the strength of having dropped the subscriber. pub struct LocalSubscription { channel: Arc, id: u64, diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index cc4ddbc01..31a0c10c4 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -731,15 +731,8 @@ where // subscriber to be handed whatever predates this call *as if it // were the promised history* — wrong, rather than absent. Refuse // instead, and let the caller choose. See #133. - if matches!(self.entity.qos.durability, QosDurability::TransientLocal) { - return Err(format!( - "publish_shared on a TRANSIENT_LOCAL, intra-process-only publisher for {}: \ - the intra-process path has no durability cache, so a late-joining \ - subscriber would be served a history this message is missing from. \ - Use publish(), or drop the transient-local durability.", - self.entity.topic - ) - .into()); + if let Some(e) = self.refuse_durable_bus() { + return Err(e); } return Ok(match self.local_channel.publish(msg) { Delivery::Sent(n) => n, @@ -790,6 +783,33 @@ where /// /// Returns how many receivers took it on the bus; `0` means it went to the /// wire. See issue #36. + /// Refuse a durable publisher the bus, which has no durability cache. + /// + /// This lives in one place because it must guard **every** route onto the + /// bus. It was originally written inline in `publish_shared`, and + /// `publish_owned` reached the bus around it — the same violation, through + /// the sibling method. See #133. + fn refuse_durable_bus(&self) -> Option { + if !self.intra_process_only { + // A `Locality::Remote` publisher still puts every message on the + // wire, so its durability cache is populated as it always was. + return None; + } + if !matches!(self.entity.qos.durability, QosDurability::TransientLocal) { + return None; + } + Some( + format!( + "publish on a TRANSIENT_LOCAL, intra-process-only publisher for {}: \ + the intra-process path has no durability cache, so a late-joining \ + subscriber would be served a history this message is missing from. \ + Use publish(), or drop the transient-local durability.", + self.entity.topic + ) + .into(), + ) + } + pub fn publish_owned(&self, msg: T) -> Result where T: Send + Sync + 'static, @@ -799,6 +819,9 @@ where let bus = self.intra_process_only || self.locality == Some(zenoh::sample::Locality::Remote); if bus { + if let Some(e) = self.refuse_durable_bus() { + return Err(e); + } match self.local_channel.publish_owned(msg) { Ok(()) => return Ok(1), // Handed back untouched: no sole owning receiver. Share it. @@ -1314,7 +1337,12 @@ pub struct ZSub { pub expected_encoding: Option, /// Registration on the intra-process bus, when built with /// [`ZSubBuilder::build_with_shared_callback`]. Dropping it unregisters, so - /// a dropped subscriber cannot be called back into. + /// a dropped subscriber receives no *further* deliveries. + /// It does not quiesce: a delivery already in flight on another thread runs to + /// completion against the snapshot it loaded, so the callback can still run after + /// `drop` has returned. That is memory-safe, because the snapshot owns the closure + /// and everything it captured. It is not a barrier, so do not tear down a resource + /// the callback merely observes on the strength of having dropped the subscriber. _local_sub: Option, _phantom_data: PhantomData<(T, Q, S)>, } From 02a13a1c0d9e42d4abc8010525db0604221117a7 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 1 Sep 2026 20:43:36 +0800 Subject: [PATCH 15/37] fix(pubsub): publish_owned lost every remote subscriber, and Published could not say why Three defects on one call path, found by an adversarial audit. publish_owned on a Locality::Remote publisher took the bus and returned, never touching the wire. publish_shared gets this right one method away, and its comment says why: the two routes address disjoint audiences and BOTH must run. The move cannot run both - the wire half needs the value in order to serialize it - so a Remote publisher now shares instead of moving. That also closes the durability hole. refuse_durable_bus permits a TRANSIENT_LOCAL Remote publisher on the grounds that the wire still runs, which was false on exactly this path, so a late joiner was served a history the message was missing from. And both methods returned usize, collapsing NoTaker, DepthExceeded and 'there is no bus on this publisher' into 0. Delivery exists to keep the first two apart: a caller may fall back to the wire on NoTaker, and loops forever if it does so on DepthExceeded. Both now return Published { Wire, Bus(Delivery), BusAndWire(Delivery) }, and the bus's own publish_owned returns Delivery rather than reporting a depth-drop as a delivery. Every publish_owned test used with_intra_process_only(), which is the failure mode #132 warns about in as many words. --- crates/hiroz/src/local_bus.rs | 31 ++++++++++++---- crates/hiroz/src/prelude.rs | 1 + crates/hiroz/src/pubsub.rs | 69 +++++++++++++++++++---------------- 3 files changed, 62 insertions(+), 39 deletions(-) diff --git a/crates/hiroz/src/local_bus.rs b/crates/hiroz/src/local_bus.rs index fa5ae177b..59ea381ac 100644 --- a/crates/hiroz/src/local_bus.rs +++ b/crates/hiroz/src/local_bus.rs @@ -186,6 +186,23 @@ pub(crate) enum Delivery { } +/// Which routes one publish took, and what the bus did on its route. +/// +/// A caller cannot reconstruct this from a delivered count: zero means "no +/// taker", "refused at depth" and "there is no bus on this publisher", and the +/// first two must stay distinct — see [`Delivery`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[non_exhaustive] +pub enum Published { + /// zenoh only. The audience is not knowable from here. + Wire, + /// The intra-process bus only, with this outcome. + Bus(Delivery), + /// Both, to disjoint audiences: the bus for this session, the wire for + /// everyone else. Only a `Locality::Remote` publisher does this. + BusAndWire(Delivery), +} + /// Invoke one subscriber callback, isolated. /// /// Two things happen here that must happen at every user-code call site. @@ -298,7 +315,7 @@ impl Channel { /// anything else is subscribed as well: with a second receiver the message /// cannot be given away, and silently downgrading to a shared delivery /// would defeat the point of having asked for ownership. - pub fn publish_owned(&self, payload: T) -> core::result::Result<(), T> + pub fn publish_owned(&self, payload: T) -> core::result::Result where T: Any + Send + 'static, { @@ -324,17 +341,17 @@ impl Channel { "intra-process delivery nested too deeply; refusing to recurse further" ); // Deliberately NOT Err(payload): the caller treats that as "no - // owning receiver" and falls back, which for a Remote-locality - // publisher puts a message on the wire that the caller asked to - // hand to one local owner. A deliberate drop is the honest answer, - // and it matches what `publish` does at the same depth. - return Ok(()); + // owning receiver" and falls back, which re-enters this callback on + // a zenoh thread with a fresh depth counter and loops forever. The + // message is dropped, and `DepthExceeded` says so — reporting it as + // a delivery would hide a dropped message behind a success. + return Ok(Delivery::DepthExceeded); } DELIVERY_DEPTH.with(|d| d.set(depth + 1)); let _depth_guard = DepthGuard; invoke_isolated("local_bus::publish_owned", || cb(Box::new(payload))); - Ok(()) + Ok(Delivery::Sent(1)) } /// How many subscribers this channel has, regardless of type. Diagnostics diff --git a/crates/hiroz/src/prelude.rs b/crates/hiroz/src/prelude.rs index 3357b5ab8..3b251a1bc 100644 --- a/crates/hiroz/src/prelude.rs +++ b/crates/hiroz/src/prelude.rs @@ -23,6 +23,7 @@ pub use crate::Builder; /// Core runtime types. pub use crate::context::{ZContext, ZContextBuilder}; pub use crate::node::ZNode; +pub use crate::local_bus::{Delivery, Published}; pub use crate::pubsub::{ZPub, ZSub}; pub use crate::service::{RequestId, ServiceReply, ServiceRequest, ZClient, ZServer}; diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index 31a0c10c4..165432e00 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -12,6 +12,7 @@ use crate::common::DataHandler; use crate::entity::{EndpointEntity, EndpointKind}; use crate::event::EventsManager; use crate::graph::Graph; +use crate::local_bus::{Delivery, Published}; use crate::impl_with_type_info; use crate::queue::BoundedQueue; use crate::topic_name; @@ -709,7 +710,7 @@ where /// - The attachment — source gid, sequence number, source timestamp — is /// absent, so no bus message carries ROS message-info. /// - pub fn publish_shared(&self, msg: Arc) -> Result + pub fn publish_shared(&self, msg: Arc) -> Result where T: Send + Sync + 'static, { @@ -723,8 +724,6 @@ where // subscriber received nothing, with nothing anywhere reporting a loss. // There is no count to subtract our own subscribers from, so the // condition cannot be repaired; the inference is withdrawn. See #134. - use crate::local_bus::Delivery; - if self.intra_process_only { // TRANSIENT_LOCAL lives in the wire publisher's cache, and this // publisher has no wire. Serving the bus would leave a late-joining @@ -734,18 +733,17 @@ where if let Some(e) = self.refuse_durable_bus() { return Err(e); } - return Ok(match self.local_channel.publish(msg) { - Delivery::Sent(n) => n, - Delivery::NoTaker => { - debug!( - "[PUB] intra-process only, no local subscriber on {}: message dropped", - self.entity.topic - ); - 0 - } - // Already logged at error level by the bus. Deliberate drop. - Delivery::DepthExceeded => 0, - }); + let d = self.local_channel.publish(msg); + if d == Delivery::NoTaker { + debug!( + "[PUB] intra-process only, no local subscriber on {}: message dropped", + self.entity.topic + ); + } + // DepthExceeded is returned rather than folded into "nothing + // delivered": a caller may fall back to the wire on NoTaker and + // must not on DepthExceeded. See #36. + return Ok(Published::Bus(d)); } // A `Locality::Remote` wire half cannot reach this session, so the bus @@ -757,17 +755,14 @@ where // on both. TRANSIENT_LOCAL is safe here: the wire publish populates the // cache exactly as it always did. if self.locality == Some(zenoh::sample::Locality::Remote) { - let delivered = match self.local_channel.publish(msg.clone()) { - Delivery::Sent(n) => n, - Delivery::NoTaker | Delivery::DepthExceeded => 0, - }; + let d = self.local_channel.publish(msg.clone()); self.publish(&msg)?; - return Ok(delivered); + return Ok(Published::BusAndWire(d)); } // No assertion from the caller: the wire alone, which reaches everyone. self.publish(&msg)?; - Ok(0) + Ok(Published::Wire) } /// Publish `msg` by value, giving it away when exactly one receiver wants it. @@ -810,26 +805,36 @@ where ) } - pub fn publish_owned(&self, msg: T) -> Result + pub fn publish_owned(&self, msg: T) -> Result where T: Send + Sync + 'static, { - // The move only applies when the bus is serving this message at all. - // Same rule as `publish_shared`: the caller asserts the audience. - let bus = self.intra_process_only - || self.locality == Some(zenoh::sample::Locality::Remote); - if bus { + // A `Locality::Remote` publisher serves two disjoint audiences, and + // the wire half needs the value in order to serialize it. So the value + // cannot be given away: this shares instead of moving. + // + // Handing it to a sole local owner and returning would lose the message + // to every remote subscriber, and would leave a TRANSIENT_LOCAL cache + // unpopulated while `refuse_durable_bus` permits the publish on the + // grounds that the wire still runs. `publish_shared` gets this right one + // method away; this did not. + if self.locality == Some(zenoh::sample::Locality::Remote) { + return self.publish_shared(Arc::new(msg)); + } + + if self.intra_process_only { if let Some(e) = self.refuse_durable_bus() { return Err(e); } - match self.local_channel.publish_owned(msg) { - Ok(()) => return Ok(1), + return match self.local_channel.publish_owned(msg) { + Ok(d) => Ok(Published::Bus(d)), // Handed back untouched: no sole owning receiver. Share it. - Err(returned) => return self.publish_shared(Arc::new(returned)), - } + Err(returned) => self.publish_shared(Arc::new(returned)), + }; } + self.publish(&msg)?; - Ok(0) + Ok(Published::Wire) } From e336484ff0de64fa3a2d79475ae865aae6754082 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 1 Sep 2026 20:45:00 +0800 Subject: [PATCH 16/37] fix(local_bus): make Delivery public, as Published now carries it publish_shared returns Published, which contains Delivery, so a crate private Delivery is a private type in the public API. The same fix landed later in the stack when local_bus became a pub module; it belongs here now that the publisher's return type exposes it. --- crates/hiroz/src/local_bus.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/hiroz/src/local_bus.rs b/crates/hiroz/src/local_bus.rs index 59ea381ac..3605e3100 100644 --- a/crates/hiroz/src/local_bus.rs +++ b/crates/hiroz/src/local_bus.rs @@ -176,7 +176,7 @@ pub struct Channel { /// so on `DepthExceeded` re-enters the same callback on a zenoh thread with a /// fresh depth counter and loops forever. See #36. #[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub(crate) enum Delivery { +pub enum Delivery { /// Handed to this many subscribers. Sent(usize), /// No subscriber of this type is on the bus. From 0f153a3cf4acaadaca2ed7c06bea50ca2ef4fcd1 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 1 Sep 2026 20:46:14 +0800 Subject: [PATCH 17/37] test: per-path detectors for the three routing defects publish_owned_on_a_remote_publisher_still_reaches_the_wire is the detector that did not exist. Every other publish_owned test used with_intra_process_only(), so none could see the Remote arm take the bus and return. The far-subscriber assertion is the point; the near one passes against the defect. The other three pin what a count could not express: Wire against Bus(NoTaker) - a plain publisher never asks the bus, an asserted one asks and finds nobody - and a depth refusal reported as dropped rather than as one receiver taking it. --- crates/hiroz-tests/tests/intra_process.rs | 210 ++++++++++++++++++++++ 1 file changed, 210 insertions(+) diff --git a/crates/hiroz-tests/tests/intra_process.rs b/crates/hiroz-tests/tests/intra_process.rs index 46bb0c78e..bee1cb100 100644 --- a/crates/hiroz-tests/tests/intra_process.rs +++ b/crates/hiroz-tests/tests/intra_process.rs @@ -37,6 +37,7 @@ use std::{ }; use common::*; +use hiroz::local_bus::{Delivery, Published}; use hiroz::{Builder, ZBuf}; use hiroz_msgs::std_msgs::{ByteMultiArray, Int32, String as RosString}; use zenoh_buffers::buffer::SplitBuffer; @@ -1009,3 +1010,212 @@ fn a_panicking_sole_subscriber_does_not_reach_the_publisher() -> hiroz::Result<( assert_eq!(delivered, 1, "the subscriber was not invoked"); Ok(()) } + +/// B1. `publish_owned` on a `Locality::Remote` publisher must reach the wire. +/// +/// This is the detector that did not exist. Every other `publish_owned` test +/// uses `with_intra_process_only()`, so none of them could see that the Remote +/// arm took the bus and returned — losing the message to every off-session +/// subscriber, silently, while `publish_shared` in the identical configuration +/// served both. +/// +/// The far assertion is the point. The near one passes against the defect. +#[test] +fn publish_owned_on_a_remote_publisher_still_reaches_the_wire() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx_tx = create_hiroz_context_with_router(&router)?; + let ctx_far = create_hiroz_context_with_router(&router)?; + let node_tx = ctx_tx.create_node("owned_rem_tx").build()?; + let node_near = ctx_tx.create_node("owned_rem_near").build()?; + let node_far = ctx_far.create_node("owned_rem_far").build()?; + + let near = Arc::new(AtomicUsize::new(0)); + let far = Arc::new(AtomicUsize::new(0)); + let (n, f) = (near.clone(), far.clone()); + + let _s_near = node_near + .create_sub::("owned_split") + .build_with_owned_callback(move |_m: RosString| { + n.fetch_add(1, Ordering::SeqCst); + })?; + let _s_far = node_far + .create_sub::("owned_split") + .build_with_callback(move |_m: RosString| { + f.fetch_add(1, Ordering::SeqCst); + })?; + + let publisher = node_tx + .create_pub::("owned_split") + .with_locality(zenoh::sample::Locality::Remote) + .build()?; + wait_for_ready(Duration::from_millis(800)); + + let outcome = publisher.publish_owned(RosString { + data: "must reach both".to_owned(), + })?; + + // Shared, not moved: the wire half needs the value to serialize it, so a + // Remote publisher cannot give it away. + assert!( + matches!(outcome, Published::BusAndWire(_)), + "a Remote publisher must run both routes, got {outcome:?}" + ); + + assert!( + wait_until(|| far.load(Ordering::SeqCst) >= 1), + "the off-session subscriber never received it — this is B1" + ); + thread::sleep(Duration::from_millis(300)); + assert_eq!(near.load(Ordering::SeqCst), 1, "near subscriber count wrong"); + assert_eq!(far.load(Ordering::SeqCst), 1, "far subscriber count wrong"); + Ok(()) +} + +/// B2. The durability refusal permits a `Locality::Remote` publisher because +/// the wire still populates its cache. That justification has to hold on the +/// owned path too, or a late joiner is served a history this message is +/// missing from. +#[test] +fn transient_local_plus_remote_locality_is_allowed_on_the_owned_path() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx = create_hiroz_context_with_router(&router)?; + let node = ctx.create_node("tl_rem_node").build()?; + + let publisher = node + .create_pub::("tl_rem_topic") + .with_qos(hiroz::qos::QosProfile { + durability: hiroz::qos::QosDurability::TransientLocal, + ..Default::default() + }) + .with_locality(zenoh::sample::Locality::Remote) + .build()?; + wait_for_ready(Duration::from_millis(300)); + + // Permitted, because the wire runs and fills the cache. The companion test + // above is what proves the wire actually runs; without it this assertion + // would be satisfied by a publisher that silently dropped the message. + let outcome = publisher.publish_owned(RosString { + data: "durable".to_owned(), + })?; + assert!( + matches!(outcome, Published::BusAndWire(_) | Published::Wire), + "a durable Remote publisher must still reach the wire, got {outcome:?}" + ); + Ok(()) +} + +/// B3. `Published` must distinguish the three ways nothing was delivered +/// locally, because a caller may fall back to the wire on one of them and must +/// not on another. +/// +/// A count cannot carry this: `NoTaker`, `DepthExceeded` and "there is no bus +/// on this publisher" were all zero. +#[test] +fn published_says_which_routes_ran_and_why_nothing_was_taken() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx = create_hiroz_context_with_router(&router)?; + let node = ctx.create_node("outcome_node").build()?; + + // 1. No assertion from the caller: the wire alone, and the bus is not + // consulted at all. Not `Bus(NoTaker)` — nothing asked the bus. + let plain = node.create_pub::("outcome_plain").build()?; + let out = plain.publish_shared(Arc::new(RosString { + data: "wire".to_owned(), + }))?; + assert_eq!(out, Published::Wire, "a plain publisher must report Wire"); + + // 2. Bus asserted, nobody listening. Distinct from the case above: here the + // bus WAS asked and had no taker. + let local = node + .create_pub::("outcome_local") + .with_intra_process_only() + .build()?; + let out = local.publish_shared(Arc::new(RosString { + data: "nobody".to_owned(), + }))?; + assert_eq!( + out, + Published::Bus(Delivery::NoTaker), + "an empty bus must report NoTaker, not Wire" + ); + + // 3. Bus asserted, one subscriber. + let hits = Arc::new(AtomicUsize::new(0)); + let h = hits.clone(); + let _sub = node + .create_sub::("outcome_taken") + .build_with_shared_callback(move |_m: Arc| { + h.fetch_add(1, Ordering::SeqCst); + })?; + let taken = node + .create_pub::("outcome_taken") + .with_intra_process_only() + .build()?; + let out = taken.publish_shared(Arc::new(RosString { + data: "taken".to_owned(), + }))?; + assert_eq!(out, Published::Bus(Delivery::Sent(1))); + assert_eq!(hits.load(Ordering::SeqCst), 1); + Ok(()) +} + +/// B3, the half a count could never express: a message refused at +/// `MAX_DELIVERY_DEPTH` is dropped, and must not be reported as delivered. +/// +/// Before this, the bus returned `Ok(())` for both outcomes and `ZPub` mapped +/// it to `Ok(1)` — a dropped message counted as one receiver taking it. A +/// caller that retries on a low count would have retried forever on the one +/// outcome where retrying re-enters the same callback. +#[test] +fn a_depth_refusal_is_reported_as_dropped_not_delivered() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx = create_hiroz_context_with_router(&router)?; + let node = ctx.create_node("depth_outcome").build()?; + + let outcomes: Arc>> = Arc::new(Mutex::new(Vec::new())); + let publisher: Arc hiroz::Result + Send + Sync>>> = + Arc::new(OnceLock::new()); + + let p = publisher.clone(); + let o = outcomes.clone(); + let _sub = node + .create_sub::("depth_outcome") + .build_with_owned_callback(move |m: RosString| { + // Republish onto the same topic: each delivery nests one deeper. + if let Some(send) = p.get() { + if let Ok(out) = send(m) { + o.lock().expect("outcome lock").push(out); + } + } + })?; + + let pubr = node + .create_pub::("depth_outcome") + .with_intra_process_only() + .build()?; + let pubr = Arc::new(pubr); + let p2 = pubr.clone(); + publisher + .set(Box::new(move |m: RosString| p2.publish_owned(m))) + .map_err(|_| "publisher already set") + .expect("set once"); + + pubr.publish_owned(RosString { + data: "recurse".to_owned(), + })?; + + let seen = outcomes.lock().expect("outcome lock"); + assert!( + seen.iter() + .any(|o| matches!(o, Published::Bus(Delivery::DepthExceeded))), + "the depth refusal was never reported; outcomes were {seen:?}" + ); + assert!( + !seen + .iter() + .any(|o| matches!(o, Published::Bus(Delivery::Sent(_))) + && seen.len() > hiroz::local_bus::MAX_DELIVERY_DEPTH as usize + 2), + "more deliveries than the depth bound allows" + ); + Ok(()) +} From a3d847824b96d787a79b4b3aef85eea4d52b5e5e Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 1 Sep 2026 20:48:46 +0800 Subject: [PATCH 18/37] test: assert the outcome, not a count publish_shared now returns Published, so twelve assertions had to change. Each was converted by what its test is actually about rather than by a blanket rewrite: the three that asserted zero become exact, because zero was the value that hid the reason. Two of them are Published::Wire - the bus was never asked - and one is Bus(NoTaker), where it was asked and found nobody. A count could not tell those apart, which is the defect this change exists to fix. --- crates/hiroz-tests/tests/intra_process.rs | 63 ++++++++++++++++++----- 1 file changed, 51 insertions(+), 12 deletions(-) diff --git a/crates/hiroz-tests/tests/intra_process.rs b/crates/hiroz-tests/tests/intra_process.rs index bee1cb100..4ae620a64 100644 --- a/crates/hiroz-tests/tests/intra_process.rs +++ b/crates/hiroz-tests/tests/intra_process.rs @@ -83,7 +83,11 @@ fn same_arc_reaches_a_same_session_subscriber() -> hiroz::Result<()> { data: "no copies please".to_owned(), }); let delivered = publisher.publish_shared(sent.clone())?; - assert_eq!(delivered, 1, "expected exactly one local subscriber"); + assert_eq!( + delivered, + Published::Bus(Delivery::Sent(1)), + "expected exactly one local subscriber" + ); // Delivery is synchronous on this thread, so no waiting is needed — but // assert on the count first so a failure says "nothing arrived" rather than @@ -183,7 +187,7 @@ fn a_different_rust_type_on_the_same_topic_is_not_delivered() -> hiroz::Result<( .build()?; assert_eq!( control_pub.publish_shared(Arc::new(Int32 { data: 7 }))?, - 1, + Published::Bus(Delivery::Sent(1)), "the control did not deliver: the bus is dead, so the assertion below proves nothing" ); assert_eq!(ok_hits.load(Ordering::SeqCst), 1, "control subscriber not called"); @@ -197,7 +201,11 @@ fn a_different_rust_type_on_the_same_topic_is_not_delivered() -> hiroz::Result<( data: "wrong type".to_owned(), }))?; - assert_eq!(delivered, 0, "delivered across a type mismatch"); + assert_eq!( + delivered, + Published::Bus(Delivery::NoTaker), + "delivered across a type mismatch" + ); assert_eq!( hits.load(Ordering::SeqCst), 0, @@ -230,14 +238,17 @@ fn dropping_the_subscriber_unregisters_it() -> hiroz::Result<()> { // Positive control: it is registered right now. Without this the assertion // below would pass just as well against a subscriber that never worked. - assert_eq!(publisher.publish_shared(msg.clone())?, 1); + assert_eq!( + publisher.publish_shared(msg.clone())?, + Published::Bus(Delivery::Sent(1)) + ); assert_eq!(hits.load(Ordering::SeqCst), 1); drop(sub); assert_eq!( publisher.publish_shared(msg)?, - 0, + Published::Bus(Delivery::NoTaker), "the bus still holds a registration for a dropped subscriber" ); assert_eq!(hits.load(Ordering::SeqCst), 1, "a dropped subscriber was called"); @@ -395,7 +406,11 @@ fn publish_shared_without_a_locality_restriction_arrives_once() -> hiroz::Result // The wire, because this publisher asserted nothing about its audience. // Inferring "everyone is local" from the ROS graph is unsound: it cannot // see a plain zenoh subscriber. See #134. - assert_eq!(delivered, 0, "the bus was taken without the caller asserting the audience"); + assert_eq!( + delivered, + Published::Wire, + "the bus was taken without the caller asserting the audience" + ); assert!( wait_until(|| hits.load(Ordering::SeqCst) >= 1), @@ -495,7 +510,11 @@ fn a_plain_publisher_takes_the_wire_for_an_ordinary_subscriber() -> hiroz::Resul let delivered = publisher.publish_shared(Arc::new(RosString { data: "who is listening".to_owned(), }))?; - assert_eq!(delivered, 0, "the bus took a message its subscriber cannot decode"); + assert_eq!( + delivered, + Published::Wire, + "the bus took a message its subscriber cannot decode" + ); assert!( wait_until(|| hits.load(Ordering::SeqCst) >= 1), @@ -588,7 +607,11 @@ fn a_sole_receiver_is_given_the_message_to_own() -> hiroz::Result<()> { let took = publisher.publish_owned(RosString { data: "mine".to_owned(), })?; - assert_eq!(took, 1, "the sole owning receiver did not take the message"); + assert_eq!( + took, + Published::Bus(Delivery::Sent(1)), + "the sole owning receiver did not take the message" + ); let got = seen.lock().expect("poisoned"); assert_eq!(got.len(), 1, "the owned callback did not run"); @@ -763,7 +786,11 @@ fn a_remote_locality_publisher_still_reaches_a_same_session_subscriber() -> hiro let delivered = publisher.publish_shared(Arc::new(RosString { data: "both, disjointly".to_owned(), }))?; - assert_eq!(delivered, 1, "the bus did not carry it to the near subscriber"); + assert_eq!( + delivered, + Published::BusAndWire(Delivery::Sent(1)), + "the bus did not carry it to the near subscriber, or skipped the wire" + ); assert!( wait_until(|| far.load(Ordering::SeqCst) >= 1), @@ -853,7 +880,11 @@ fn every_shared_subscriber_receives_the_same_allocation() -> hiroz::Result<()> { data: "one allocation, three readers".to_owned(), }); let delivered = publisher.publish_shared(sent.clone())?; - assert_eq!(delivered, 3, "not every subscriber was served"); + assert_eq!( + delivered, + Published::Bus(Delivery::Sent(3)), + "not every subscriber was served" + ); let got = seen.lock().expect("poisoned"); assert_eq!(got.len(), 3, "expected three deliveries"); @@ -965,7 +996,11 @@ fn a_panicking_subscriber_does_not_stop_delivery_to_the_others() -> hiroz::Resul // The publisher returned at all: the panic did not unwind into this thread. let delivered = delivered.expect("the panic escaped into the publishing thread"); - assert_eq!(delivered, 3, "not every subscriber was invoked"); + assert_eq!( + delivered, + Published::Bus(Delivery::Sent(3)), + "not every subscriber was invoked" + ); assert_eq!( before.load(Ordering::SeqCst), 1, @@ -1007,7 +1042,11 @@ fn a_panicking_sole_subscriber_does_not_reach_the_publisher() -> hiroz::Result<( std::panic::set_hook(prev); let delivered = delivered.expect("the panic escaped into the publishing thread"); - assert_eq!(delivered, 1, "the subscriber was not invoked"); + assert_eq!( + delivered, + Published::Bus(Delivery::Sent(1)), + "the subscriber was not invoked" + ); Ok(()) } From 7294c530253b4e964481cd61651a6d84fb12617c Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 1 Sep 2026 20:52:37 +0800 Subject: [PATCH 19/37] fix(pubsub): serve owned subscribers on a Remote publisher The Remote arm of publish_moved shared the value instead of moving it, so the wire ran but the shared path filters on is_shared() and never reached an owned subscriber. Publish to the wire first from a reference, then give the value away. --- crates/hiroz/src/pubsub.rs | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index 165432e00..23b945f2f 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -809,17 +809,23 @@ where where T: Send + Sync + 'static, { - // A `Locality::Remote` publisher serves two disjoint audiences, and - // the wire half needs the value in order to serialize it. So the value - // cannot be given away: this shares instead of moving. + // A `Locality::Remote` publisher serves two disjoint audiences and both + // must run: the bus for this session, the wire for everyone else. // - // Handing it to a sole local owner and returning would lose the message - // to every remote subscriber, and would leave a TRANSIENT_LOCAL cache - // unpopulated while `refuse_durable_bus` permits the publish on the - // grounds that the wire still runs. `publish_shared` gets this right one - // method away; this did not. + // Order matters. The wire half needs the value in order to serialize it, + // and the bus half wants to own it, so the wire goes first from a + // reference and the value is given away afterwards. Sharing instead + // would reach the wire but silently stop serving owned subscribers, + // because the shared path filters on `is_shared()`. if self.locality == Some(zenoh::sample::Locality::Remote) { - return self.publish_shared(Arc::new(msg)); + self.publish(&msg)?; + return Ok(Published::BusAndWire( + match self.local_channel.publish_owned(msg) { + Ok(d) => d, + // No sole owning receiver; the shared half may still want it. + Err(returned) => self.local_channel.publish(Arc::new(returned)), + }, + )); } if self.intra_process_only { From 6ccb14c4b34982d0a5276c4713c7fa96d1ab2cc5 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 1 Sep 2026 20:57:30 +0800 Subject: [PATCH 20/37] refactor(pubsub): resolve the route once, and correct the stale docs The two publishing methods each decided the route themselves, in the opposite order, so a publisher carrying both assertions routed one way through publish_shared and the other through publish_owned. That is the drift that produced the Remote defect, in a configuration nothing covered. Route::{BusOnly,BusAndWire,WireOnly} is resolved in one place and both methods match on it; a test pins the precedence. The publish_shared table still promised a count and still described the graph inference that was withdrawn. Qualify thirteen bare issue refs that number hiroz-bench issues: hiroz has its own #36 and #40, so each one silently pointed at a stranger. --- crates/hiroz-tests/tests/intra_process.rs | 43 +++++ crates/hiroz/src/local_bus.rs | 30 +-- crates/hiroz/src/pubsub.rs | 222 ++++++++++++---------- 3 files changed, 184 insertions(+), 111 deletions(-) diff --git a/crates/hiroz-tests/tests/intra_process.rs b/crates/hiroz-tests/tests/intra_process.rs index 4ae620a64..567e57233 100644 --- a/crates/hiroz-tests/tests/intra_process.rs +++ b/crates/hiroz-tests/tests/intra_process.rs @@ -1258,3 +1258,46 @@ fn a_depth_refusal_is_reported_as_dropped_not_delivered() -> hiroz::Result<()> { ); Ok(()) } + +/// S9. `publish_shared` and `publish_owned` must agree about where a message +/// goes. They cannot, unless they read the same decision. +/// +/// They did not. Each tested the conditions itself, in the opposite order, so a +/// publisher carrying both assertions routed one way through one method and the +/// other way through the other — the same drift that produced B1, in a +/// configuration nothing exercised. `with_intra_process_only()` wins, because it +/// is the stronger claim: this publisher has no wire. +#[test] +fn both_publish_methods_agree_when_the_assertions_conflict() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx = create_hiroz_context_with_router(&router)?; + let node = ctx.create_node("conflict_node").build()?; + + let publisher = node + .create_pub::("conflict_topic") + .with_intra_process_only() + .with_locality(zenoh::sample::Locality::Remote) + .build()?; + wait_for_ready(Duration::from_millis(300)); + + let shared = publisher.publish_shared(Arc::new(RosString { + data: "shared".to_owned(), + }))?; + let moved = publisher.publish_owned(RosString { + data: "moved".to_owned(), + })?; + + // Nobody is subscribed, so both report the bus having no taker. The point is + // that neither reached the wire: a `BusAndWire` from either is the drift. + assert_eq!( + shared, + Published::Bus(Delivery::NoTaker), + "publish_shared took the wrong route" + ); + assert_eq!( + moved, + Published::Bus(Delivery::NoTaker), + "publish_owned disagreed with publish_shared" + ); + Ok(()) +} diff --git a/crates/hiroz/src/local_bus.rs b/crates/hiroz/src/local_bus.rs index 3605e3100..233675960 100644 --- a/crates/hiroz/src/local_bus.rs +++ b/crates/hiroz/src/local_bus.rs @@ -19,17 +19,23 @@ //! opt-in fast path beside the wire path, never a replacement for it — the same //! shape as rclcpp's intra-process comm. //! -//! # Scope of the prototype +//! # Scope //! -//! | | this prototype | a production version | +//! | | here | not here | //! |---|---|---| -//! | audience | every same-session subscriber registered here | same, plus the wire for remote ones, decided from the graph | -//! | type check | exact [`TypeId`] | same | -//! | mutability | shared `Arc`, read-only for all receivers | move a unique payload when there is exactly one receiver | -//! | choosing the path | an explicit `with_intra_process_only()` on the publisher | inferred: use the wire only while remote subscribers exist | +//! | audience | every same-session subscriber registered on this channel | subscribers reached only over the wire | +//! | type check | exact [`TypeId`] | any structural or version-tolerant match | +//! | mutability | shared `Arc`, or moved to a sole receiver | a receiver mutating a payload others still hold | +//! | choosing the path | the caller asserts the audience | inferring it | //! -//! The last row is the real gap. A publisher here does not ask whether anyone -//! remote is listening; it is told. See issue #36. +//! The last row is the one that has been tried and withdrawn. A publisher does +//! not ask whether anyone remote is listening, because the question has no +//! answer: a plain zenoh subscriber declares no ROS liveliness token, so the +//! graph cannot see it and there is no count to subtract our own from. Taking +//! the bus on a wrong inference loses that subscriber's messages silently. So +//! the caller says, with `with_intra_process_only()` or a `Locality`, and the +//! bus is taken only when that assertion makes the wire redundant. +//! See circle/hiroz-bench#36 for the bus, and #134 for the withdrawal. //! //! # Keying, and why a publisher resolves it once //! @@ -78,7 +84,7 @@ use zenoh::session::ZenohId; /// /// Eight is chosen to be far above any legitimate chain — a pipeline of eight /// nodes each publishing from the previous one's callback, on one thread — and -/// far below the depth at which the stack is in danger. See issue #40. +/// far below the depth at which the stack is in danger. See issue circle/hiroz-bench#40. /// Public so a test can assert the exact bound rather than a loose ceiling: /// a hand-copied constant lets a change to this value slip past unnoticed. pub const MAX_DELIVERY_DEPTH: u32 = 8; @@ -113,7 +119,7 @@ type LocalCallback = Arc; /// The shared path hands every receiver the same read-only `Arc`, which is /// right when several of them want it and wrong when exactly one does: a sole /// receiver could have been given the value itself, free to mutate or consume -/// it. This is that path. See issue #36. +/// it. This is that path. See issue circle/hiroz-bench#36. type OwnedCallback = Arc) + Send + Sync>; #[derive(Clone)] @@ -174,7 +180,7 @@ pub struct Channel { /// `NoTaker` and `DepthExceeded` both mean nothing was delivered, and they must /// not be conflated: a caller may fall back to the wire on `NoTaker`, but doing /// so on `DepthExceeded` re-enters the same callback on a zenoh thread with a -/// fresh depth counter and loops forever. See #36. +/// fresh depth counter and loops forever. See circle/hiroz-bench#36. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Delivery { /// Handed to this many subscribers. @@ -251,7 +257,7 @@ impl Channel { // Refuse to recurse without bound. A callback that publishes back onto // its own topic would otherwise overflow the stack; returning here turns // that into a dropped message and a loud log, which is recoverable and - // greppable. See issue #40. + // greppable. See issue circle/hiroz-bench#40. let depth = DELIVERY_DEPTH.with(|d| d.get()); if depth >= MAX_DELIVERY_DEPTH { tracing::error!( diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index 23b945f2f..d8c8bcd08 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -102,6 +102,18 @@ pub(crate) fn apply_transient_local_sub<'a, 'b, 'c, H>( builder } +/// Which routes a publisher's messages take, resolved from what the caller has +/// asserted about the audience. See [`ZPub::route`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Route { + /// The bus alone. The publisher has declared it has no wire. + BusOnly, + /// Both, to disjoint audiences: the bus for this session, the wire for the rest. + BusAndWire, + /// The wire alone. The bus is not consulted. + WireOnly, +} + /// A typed ROS 2-style publisher. Send messages with [`publish`](ZPub::publish) /// (synchronous) or [`async_publish`](ZPub::async_publish) (async). /// @@ -302,7 +314,7 @@ impl ZPubBuilder { /// every other process, including `ros2 topic echo`, and to any local /// subscriber that did not register on the bus with /// [`ZSubBuilder::build_with_shared_callback`]. Set it only where both ends - /// are known. See issue #36. + /// are known. See issue circle/hiroz-bench#36. pub fn with_intra_process_only(mut self) -> Self { self.intra_process_only = true; self @@ -672,38 +684,44 @@ where /// clone of the `Arc` — a refcount bump. The payload is not encoded and its /// bytes are not copied. /// - /// Returns how many local subscribers were delivered to. + /// Reports which routes ran, as a [`Published`]. /// /// # Which path a message takes /// - /// | `with_intra_process_only()` | local subscribers | what happens | + /// The route follows what the **caller has asserted about the audience**. + /// It is never inferred from the graph. + /// + /// | the publisher says | route | returns | /// |---|---|---| - /// | set | any | `Arc` to each; **nothing goes on the wire** | - /// | set | none | nothing at all; returns `Ok(0)` | - /// | not set | any | `Arc` to each, **and** a normal serialized publish | - /// | not set | none | a normal serialized publish | + /// | `with_intra_process_only()` | the bus alone; nothing goes on the wire | `Bus(Sent(n))`, or `Bus(NoTaker)` when nobody is listening | + /// | `with_locality(Remote)` | both, to disjoint audiences | `BusAndWire(..)` | + /// | neither | the wire alone; the bus is not consulted | `Wire` | + /// + /// Row three is the default and is deliberately conservative. A subscriber + /// that the ROS graph cannot see — a plain `z_sub`, a storage plugin, a + /// recorder — is reachable only over the wire, so a publisher that has not + /// been told the audience takes it. /// - /// Row three is deliberately wasteful and is the honest default: without - /// asking the graph who is listening, the only safe thing is to serve - /// everyone. Row two is why the flag is a prototype — it silently drops the - /// message rather than falling back. See [`crate::local_bus`] and issue #36. + /// `Bus(NoTaker)` is a distinct outcome rather than a silent drop: the + /// caller asserted there was a local audience and there was none, which is + /// worth acting on. See [`crate::local_bus`] and circle/hiroz-bench#36. /// /// # Ordering against [`publish`](Self::publish) /// - /// Local delivery is synchronous on the calling thread: the subscriber + /// Bus delivery is synchronous on the calling thread: the subscriber /// callbacks have run by the time this returns. A message sent with /// `publish` travels the wire and arrives later. Interleaving the two on one /// topic gives no ordering guarantee between them. + /// /// # QoS is not carried on the intra-process path /// - /// When this message goes to the bus, it does not enter the wire - /// publisher's cache and carries no attachment. Concretely: + /// A message that goes to the bus does not enter the wire publisher's + /// cache and carries no attachment. Concretely: /// - /// - **`TRANSIENT_LOCAL` is violated, not merely unsupported.** Nothing - /// published this way enters the durability cache, so a subscriber that - /// joins later is served the surviving samples *as if they were a - /// complete history*. Do not use this method on a transient-local - /// publisher. + /// - **`TRANSIENT_LOCAL` is refused, not silently violated.** Nothing on + /// the bus enters the durability cache, so a late joiner would be served + /// the surviving samples *as if they were a complete history*. A + /// transient-local publisher that has no wire is rejected instead. /// - `RELIABLE` / `BEST_EFFORT` and `KEEP_LAST(n)` have no meaning here: /// delivery is a synchronous inline call with no queue, so nothing is /// ever dropped for congestion and no depth is ever displaced. @@ -714,55 +732,40 @@ where where T: Send + Sync + 'static, { - // The bus is taken only when the CALLER has asserted who the audience - // is. It is not inferred from the graph. - // - // Inferring it was unsound: the graph holds ROS liveliness tokens, so a - // plain zenoh subscriber on the same keyexpr — `z_sub`, a storage or - // REST plugin, a native recorder — is invisible to it. A publisher that - // concluded "everyone is local" from the graph skipped the wire and that - // subscriber received nothing, with nothing anywhere reporting a loss. - // There is no count to subtract our own subscribers from, so the - // condition cannot be repaired; the inference is withdrawn. See #134. - if self.intra_process_only { - // TRANSIENT_LOCAL lives in the wire publisher's cache, and this - // publisher has no wire. Serving the bus would leave a late-joining - // subscriber to be handed whatever predates this call *as if it - // were the promised history* — wrong, rather than absent. Refuse - // instead, and let the caller choose. See #133. - if let Some(e) = self.refuse_durable_bus() { - return Err(e); + match self.route() { + Route::BusOnly => { + // TRANSIENT_LOCAL lives in the wire publisher's cache and this + // publisher has no wire. Serving the bus would hand a late + // joiner whatever predates this call *as if it were the + // promised history* — wrong, rather than absent. Refuse, and + // let the caller choose. See #133. + if let Some(e) = self.refuse_durable_bus() { + return Err(e); + } + let d = self.local_channel.publish(msg); + if d == Delivery::NoTaker { + debug!( + "[PUB] intra-process only, no local subscriber on {}: message dropped", + self.entity.topic + ); + } + // DepthExceeded is kept distinct from "nothing delivered": a + // caller may fall back to the wire on NoTaker and must not on + // DepthExceeded. See circle/hiroz-bench#36. + Ok(Published::Bus(d)) } - let d = self.local_channel.publish(msg); - if d == Delivery::NoTaker { - debug!( - "[PUB] intra-process only, no local subscriber on {}: message dropped", - self.entity.topic - ); + // Both audiences, and no subscriber is on both. TRANSIENT_LOCAL is + // safe here: the wire publish populates the cache as it always did. + Route::BusAndWire => { + let d = self.local_channel.publish(msg.clone()); + self.publish(&msg)?; + Ok(Published::BusAndWire(d)) + } + Route::WireOnly => { + self.publish(&msg)?; + Ok(Published::Wire) } - // DepthExceeded is returned rather than folded into "nothing - // delivered": a caller may fall back to the wire on NoTaker and - // must not on DepthExceeded. See #36. - return Ok(Published::Bus(d)); - } - - // A `Locality::Remote` wire half cannot reach this session, so the bus - // and the wire address *disjoint* audiences and both run: the bus for - // same-session subscribers, the wire for everyone else — including the - // non-ROS subscribers the graph cannot see, and any remote subscriber - // whose liveliness token has not arrived yet. This is the one case - // where two routes for one message is correct, because no subscriber is - // on both. TRANSIENT_LOCAL is safe here: the wire publish populates the - // cache exactly as it always did. - if self.locality == Some(zenoh::sample::Locality::Remote) { - let d = self.local_channel.publish(msg.clone()); - self.publish(&msg)?; - return Ok(Published::BusAndWire(d)); } - - // No assertion from the caller: the wire alone, which reaches everyone. - self.publish(&msg)?; - Ok(Published::Wire) } /// Publish `msg` by value, giving it away when exactly one receiver wants it. @@ -777,13 +780,35 @@ where /// fallback, only by an `intra_process_only` publisher with no listener. /// /// Returns how many receivers took it on the bus; `0` means it went to the - /// wire. See issue #36. + /// wire. See issue circle/hiroz-bench#36. /// Refuse a durable publisher the bus, which has no durability cache. /// /// This lives in one place because it must guard **every** route onto the /// bus. It was originally written inline in `publish_shared`, and /// `publish_owned` reached the bus around it — the same violation, through /// the sibling method. See #133. + /// Which routes this publisher's messages take. + /// + /// Resolved in one place because it is asserted by the caller and read by + /// every publishing method. When each method decided for itself they drifted: + /// `publish_owned` on a `Locality::Remote` publisher took the bus and + /// returned, losing the message for every off-session subscriber, while + /// `publish_shared` in the identical configuration served both. + /// + /// `with_intra_process_only()` wins over a locality, because it is the + /// stronger assertion: it says this publisher has no wire at all. + fn route(&self) -> Route { + if self.intra_process_only { + Route::BusOnly + } else if self.locality == Some(zenoh::sample::Locality::Remote) { + // The wire half cannot reach this session, so the two halves address + // disjoint audiences and both must run. + Route::BusAndWire + } else { + Route::WireOnly + } + } + fn refuse_durable_bus(&self) -> Option { if !self.intra_process_only { // A `Locality::Remote` publisher still puts every message on the @@ -809,38 +834,37 @@ where where T: Send + Sync + 'static, { - // A `Locality::Remote` publisher serves two disjoint audiences and both - // must run: the bus for this session, the wire for everyone else. - // - // Order matters. The wire half needs the value in order to serialize it, - // and the bus half wants to own it, so the wire goes first from a - // reference and the value is given away afterwards. Sharing instead - // would reach the wire but silently stop serving owned subscribers, - // because the shared path filters on `is_shared()`. - if self.locality == Some(zenoh::sample::Locality::Remote) { - self.publish(&msg)?; - return Ok(Published::BusAndWire( + match self.route() { + // The wire needs the value in order to serialize it and the bus + // wants to own it, so the wire goes first from a reference and the + // value is given away afterwards. Sharing instead would reach the + // wire but stop serving owned subscribers, because the shared path + // filters on `is_shared()`. + Route::BusAndWire => { + self.publish(&msg)?; + Ok(Published::BusAndWire( + match self.local_channel.publish_owned(msg) { + Ok(d) => d, + // No sole owning receiver; the shared half may want it. + Err(returned) => self.local_channel.publish(Arc::new(returned)), + }, + )) + } + Route::BusOnly => { + if let Some(e) = self.refuse_durable_bus() { + return Err(e); + } match self.local_channel.publish_owned(msg) { - Ok(d) => d, - // No sole owning receiver; the shared half may still want it. - Err(returned) => self.local_channel.publish(Arc::new(returned)), - }, - )); - } - - if self.intra_process_only { - if let Some(e) = self.refuse_durable_bus() { - return Err(e); + Ok(d) => Ok(Published::Bus(d)), + // Handed back untouched: no sole owning receiver. Share it. + Err(returned) => self.publish_shared(Arc::new(returned)), + } + } + Route::WireOnly => { + self.publish(&msg)?; + Ok(Published::Wire) } - return match self.local_channel.publish_owned(msg) { - Ok(d) => Ok(Published::Bus(d)), - // Handed back untouched: no sole owning receiver. Share it. - Err(returned) => self.publish_shared(Arc::new(returned)), - }; } - - self.publish(&msg)?; - Ok(Published::Wire) } @@ -1165,7 +1189,7 @@ where /// | zenoh subscriber, no origin filter | anything on the wire, near or far | the usual CDR decode | /// /// `Remote` on the wire path is what stops a same-session publisher that is - /// *not* restricted to `Locality::Remote` delivering the same message twice. That is now prevented on the publisher, which is the only side that knows whether it used both paths — see issue #39 — + /// *not* restricted to `Locality::Remote` delivering the same message twice. That is now prevented on the publisher, which is the only side that knows whether it used both paths — see issue circle/hiroz-bench#39 — /// once as an `Arc`, once decoded. An explicit /// [`with_locality`](Self::with_locality) is respected and left alone; the /// double delivery is then the caller's to reason about. @@ -1178,7 +1202,7 @@ where /// shared or wire delivery, which this subscriber does not receive. /// /// Use it where the receiver wants to mutate or consume the message, which - /// the shared path cannot offer. See issue #36. + /// the shared path cannot offer. See issue circle/hiroz-bench#36. pub fn build_with_owned_callback(self, callback: F) -> Result> where T: Send + Sync + 'static, @@ -1189,7 +1213,7 @@ where // Both halves run the same callback. The wire half is not decoration: // without it this subscriber discards every message that does not // arrive by `publish_owned` on the bus — including from every remote - // publisher — while still advertising a live subscription. See #36. + // publisher — while still advertising a live subscription. See circle/hiroz-bench#36. // // It cannot deliver twice: a publisher takes the bus or the wire for // any one message, never both, unless its wire half is `Remote`- @@ -1208,7 +1232,7 @@ where /// The type match is exact: a publisher of a different concrete Rust type /// on the same topic is not delivered here, even if the ROS type name - /// agrees. See [`crate::local_bus`] and issue #36. + /// agrees. See [`crate::local_bus`] and issue circle/hiroz-bench#36. pub fn build_with_shared_callback(self, callback: F) -> Result> where T: Send + Sync + 'static, From 4454fc52823c295ec85e49014698084fe4acf5d2 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 1 Sep 2026 20:58:46 +0800 Subject: [PATCH 21/37] docs(pubsub): reattach the shared-callback doc to its own method An edit left the head of build_with_shared_callback's documentation stranded above build_with_owned_callback, so rustdoc showed readers of the owned method a table of two registrations it does not make, and left the shared method with only the trailing paragraph. The sentence about the origin filter had also been spliced mid-clause and contradicted the table two lines above it; it now states what the code does. --- crates/hiroz/src/pubsub.rs | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index d8c8bcd08..f6c25ef8b 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -1177,23 +1177,6 @@ where }) } - /// **Prototype.** Build a subscriber that receives an `Arc` from a - /// same-session publisher without any serialization, and still receives - /// remote traffic over the wire as usual. - /// - /// Two registrations are made: - /// - /// | path | source | cost | - /// |---|---|---| - /// | intra-process bus | a same-session [`ZPub::publish_shared`] for this exact `T` | a refcount bump | - /// | zenoh subscriber, no origin filter | anything on the wire, near or far | the usual CDR decode | - /// - /// `Remote` on the wire path is what stops a same-session publisher that is - /// *not* restricted to `Locality::Remote` delivering the same message twice. That is now prevented on the publisher, which is the only side that knows whether it used both paths — see issue circle/hiroz-bench#39 — - /// once as an `Arc`, once decoded. An explicit - /// [`with_locality`](Self::with_locality) is respected and left alone; the - /// double delivery is then the caller's to reason about. - /// /// Build a subscriber whose callback receives the message **by value**. /// /// Served only by [`ZPub::publish_owned`], and only when this is the sole @@ -1230,6 +1213,24 @@ where Ok(sub) } + /// **Prototype.** Build a subscriber that receives an `Arc` from a + /// same-session publisher without any serialization, and still receives + /// remote traffic over the wire as usual. + /// + /// Two registrations are made: + /// + /// | path | source | cost | + /// |---|---|---| + /// | intra-process bus | a same-session [`ZPub::publish_shared`] for this exact `T` | a refcount bump | + /// | zenoh subscriber, no origin filter | anything on the wire, near or far | the usual CDR decode | + /// + /// The wire half applies no origin filter, and does not need one. A + /// publisher takes the bus or the wire for any one message, never both — + /// unless its wire half is `Locality::Remote`, in which case the two + /// audiences are disjoint and no subscriber sees it twice. Suppressing the + /// duplicate is the publisher's job, because it is the only side that knows + /// which routes it used. See circle/hiroz-bench#39. + /// /// The type match is exact: a publisher of a different concrete Rust type /// on the same topic is not delivered here, even if the ROS type name /// agrees. See [`crate::local_bus`] and issue circle/hiroz-bench#36. From 953a4a4c512cdc9567c2be66b39b27c0f5c2f4a6 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 1 Sep 2026 21:00:17 +0800 Subject: [PATCH 22/37] style: cargo fmt --all --- crates/hiroz-tests/tests/intra_process.rs | 54 +++++++++++++------ .../hiroz-tests/tests/publisher_locality.rs | 5 +- crates/hiroz/src/lib.rs | 4 +- 3 files changed, 45 insertions(+), 18 deletions(-) diff --git a/crates/hiroz-tests/tests/intra_process.rs b/crates/hiroz-tests/tests/intra_process.rs index 567e57233..382b6d370 100644 --- a/crates/hiroz-tests/tests/intra_process.rs +++ b/crates/hiroz-tests/tests/intra_process.rs @@ -37,8 +37,10 @@ use std::{ }; use common::*; -use hiroz::local_bus::{Delivery, Published}; -use hiroz::{Builder, ZBuf}; +use hiroz::{ + Builder, ZBuf, + local_bus::{Delivery, Published}, +}; use hiroz_msgs::std_msgs::{ByteMultiArray, Int32, String as RosString}; use zenoh_buffers::buffer::SplitBuffer; @@ -190,7 +192,11 @@ fn a_different_rust_type_on_the_same_topic_is_not_delivered() -> hiroz::Result<( Published::Bus(Delivery::Sent(1)), "the control did not deliver: the bus is dead, so the assertion below proves nothing" ); - assert_eq!(ok_hits.load(Ordering::SeqCst), 1, "control subscriber not called"); + assert_eq!( + ok_hits.load(Ordering::SeqCst), + 1, + "control subscriber not called" + ); // Same topic, different concrete Rust type. let publisher = node @@ -251,7 +257,11 @@ fn dropping_the_subscriber_unregisters_it() -> hiroz::Result<()> { Published::Bus(Delivery::NoTaker), "the bus still holds a registration for a dropped subscriber" ); - assert_eq!(hits.load(Ordering::SeqCst), 1, "a dropped subscriber was called"); + assert_eq!( + hits.load(Ordering::SeqCst), + 1, + "a dropped subscriber was called" + ); Ok(()) } @@ -284,9 +294,11 @@ fn a_pooled_payload_buffer_is_written_in_place_and_reused() -> hiroz::Result<()> .build_with_shared_callback(move |msg: Arc| { let bytes = msg.data.contiguous(); let stamp = u64::from_le_bytes(bytes[0..8].try_into().expect("8 bytes")); - sink.lock() - .expect("poisoned") - .push((Arc::as_ptr(&msg) as usize, bytes.as_ptr() as usize, stamp)); + sink.lock().expect("poisoned").push(( + Arc::as_ptr(&msg) as usize, + bytes.as_ptr() as usize, + stamp, + )); })?; let publisher = node @@ -322,7 +334,10 @@ fn a_pooled_payload_buffer_is_written_in_place_and_reused() -> hiroz::Result<()> ); let slot_addr = Arc::as_ptr(&slot) as usize; for (i, (arc_addr, payload_addr, stamp)) in got.iter().enumerate() { - assert_eq!(*arc_addr, slot_addr, "send {i} delivered a different allocation"); + assert_eq!( + *arc_addr, slot_addr, + "send {i} delivered a different allocation" + ); assert_eq!( *payload_addr, sent_payload_addrs[i], "send {i} delivered a different payload buffer" @@ -471,7 +486,8 @@ fn a_self_publishing_callback_does_not_recurse_without_bound() -> hiroz::Result< let seen = hits.load(Ordering::SeqCst); assert!(seen >= 1, "the callback never ran"); assert_eq!( - seen, hiroz::local_bus::MAX_DELIVERY_DEPTH as usize, + seen, + hiroz::local_bus::MAX_DELIVERY_DEPTH as usize, "expected exactly the depth bound; a change to MAX_DELIVERY_DEPTH must \ update this test rather than slip past a loose ceiling" ); @@ -731,10 +747,10 @@ fn a_self_publishing_callback_does_not_escape_to_the_wire_and_loop() -> hiroz::R let _ = done_tx.send((settled, counter.load(Ordering::SeqCst))); }); - let (settled, later) = done_rx - .recv_timeout(Duration::from_secs(10)) - .expect("publish never returned: the depth guard escaped to the wire, \ - which re-enters on a zenoh thread and deadlocks on its runtime"); + let (settled, later) = done_rx.recv_timeout(Duration::from_secs(10)).expect( + "publish never returned: the depth guard escaped to the wire, \ + which re-enters on a zenoh thread and deadlocks on its runtime", + ); assert!( later <= 32, "{later} deliveries from one publish (settled at {settled}): the guard \ @@ -797,7 +813,11 @@ fn a_remote_locality_publisher_still_reaches_a_same_session_subscriber() -> hiro "the off-session subscriber never received it; this run proves nothing" ); thread::sleep(Duration::from_millis(300)); - assert_eq!(near.load(Ordering::SeqCst), 1, "near subscriber count wrong"); + assert_eq!( + near.load(Ordering::SeqCst), + 1, + "near subscriber count wrong" + ); assert_eq!(far.load(Ordering::SeqCst), 1, "far subscriber count wrong"); Ok(()) } @@ -1105,7 +1125,11 @@ fn publish_owned_on_a_remote_publisher_still_reaches_the_wire() -> hiroz::Result "the off-session subscriber never received it — this is B1" ); thread::sleep(Duration::from_millis(300)); - assert_eq!(near.load(Ordering::SeqCst), 1, "near subscriber count wrong"); + assert_eq!( + near.load(Ordering::SeqCst), + 1, + "near subscriber count wrong" + ); assert_eq!(far.load(Ordering::SeqCst), 1, "far subscriber count wrong"); Ok(()) } diff --git a/crates/hiroz-tests/tests/publisher_locality.rs b/crates/hiroz-tests/tests/publisher_locality.rs index 1d18fb699..c5e1ff308 100644 --- a/crates/hiroz-tests/tests/publisher_locality.rs +++ b/crates/hiroz-tests/tests/publisher_locality.rs @@ -48,7 +48,10 @@ const DELIVERY_DEADLINE: Duration = Duration::from_secs(5); fn counting_sub( node: &hiroz::node::ZNode, topic: &str, -) -> hiroz::Result<(hiroz::pubsub::ZSub>, Arc)> { +) -> hiroz::Result<( + hiroz::pubsub::ZSub>, + Arc, +)> { let count = Arc::new(AtomicUsize::new(0)); let c = count.clone(); let sub = node diff --git a/crates/hiroz/src/lib.rs b/crates/hiroz/src/lib.rs index aa5df57ec..5237b12bd 100644 --- a/crates/hiroz/src/lib.rs +++ b/crates/hiroz/src/lib.rs @@ -65,14 +65,14 @@ pub mod ffi; pub mod graph; /// ROS 2 lifecycle node support (state machine, lifecycle publisher). pub mod lifecycle; +/// Intra-process message bus (prototype) — `Arc` delivery without serialization. +pub mod local_bus; /// Typed message wrappers and helpers. pub mod msg; /// ROS 2 node creation and management. pub mod node; /// Convenience re-exports for common hiroz types. pub mod prelude; -/// Intra-process message bus (prototype) — `Arc` delivery without serialization. -pub mod local_bus; /// Publishers and subscribers. pub mod pubsub; /// Python FFI bridge types. From bdfdf622a91de113022f5e7ae25832cd749b5bdd Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 1 Sep 2026 21:02:21 +0800 Subject: [PATCH 23/37] docs: fully qualify four intra-doc links cargo doc exits 0 over an unresolved link, so these passed every gate. A path in a //! doc resolves at the pub mod line, where the module's own types are not in scope, which is why TypeId and Channel failed there. --- crates/hiroz/src/local_bus.rs | 4 ++-- crates/hiroz/src/pubsub.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/hiroz/src/local_bus.rs b/crates/hiroz/src/local_bus.rs index 233675960..4d28d124b 100644 --- a/crates/hiroz/src/local_bus.rs +++ b/crates/hiroz/src/local_bus.rs @@ -24,7 +24,7 @@ //! | | here | not here | //! |---|---|---| //! | audience | every same-session subscriber registered on this channel | subscribers reached only over the wire | -//! | type check | exact [`TypeId`] | any structural or version-tolerant match | +//! | type check | exact [`core::any::TypeId`] | any structural or version-tolerant match | //! | mutability | shared `Arc`, or moved to a sole receiver | a receiver mutating a payload others still hold | //! | choosing the path | the caller asserts the audience | inferring it | //! @@ -45,7 +45,7 @@ //! must not see each other's traffic. Both `ZPub` and `ZSub` already hold an //! `Arc`, so this needs no plumbing through the node tree. //! -//! A publisher takes its [`Channel`] handle when it is built and never touches +//! A publisher takes its [`crate::local_bus::Channel`] handle when it is built and never touches //! the registry again. Resolving per message would mean hashing a //! fully-qualified ROS key expression on every publish, which at small payloads //! is a visible share of the whole path. diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index f6c25ef8b..d6f555038 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -255,7 +255,7 @@ impl ZPubBuilder { /// those in the same session, those in other sessions, or both (the /// default). /// - /// [`Locality::SessionLocal`] is the intra-process fast path. Zenoh skips + /// [`Locality::SessionLocal`](zenoh::sample::Locality::SessionLocal) is the intra-process fast path. Zenoh skips /// the transport entirely for it — no link, no wire encode, no shared /// memory — and hands the payload straight to the matching subscriber /// callbacks in this session. Every node created from one [`ZContext`] @@ -266,7 +266,7 @@ impl ZPubBuilder { /// /// # This makes the publisher invisible off-process /// - /// A [`Locality::SessionLocal`] publisher is not reachable from any other + /// A [`Locality::SessionLocal`](zenoh::sample::Locality::SessionLocal) publisher is not reachable from any other /// process, including `ros2 topic echo`. Set it only where the subscriber /// is known to live in the same context. /// From f164b2dd2e8f62dec0788bf61889ca0017c654c6 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 1 Sep 2026 22:05:00 +0800 Subject: [PATCH 24/37] refactor!: drop the pooled-payload feature and its ZBuf accessor hiroz never called it. It existed so an out-of-tree caller could rewrite a ZBuf in place, but ZBuf is a newtype with a public field and a DerefMut, so such a caller reaches zenoh's opt_mut_slice directly and needs nothing from hiroz. What it cost to keep: the feature's patched zenoh-buffers lives on a tailnet-only forge, so CI could not compile the test behind the gate. A cfg-gated suite that never builds is the documented way for a test to rot unnoticed. Removing it makes 'hiroz needs no patched zenoh' unconditional rather than feature-qualified. --- crates/hiroz-tests/Cargo.toml | 1 - crates/hiroz-tests/tests/intra_process.rs | 87 +---------------------- crates/hiroz/Cargo.toml | 1 - crates/hiroz/src/zbuf.rs | 54 -------------- 4 files changed, 2 insertions(+), 141 deletions(-) diff --git a/crates/hiroz-tests/Cargo.toml b/crates/hiroz-tests/Cargo.toml index 3003b2d68..2e59c01ef 100644 --- a/crates/hiroz-tests/Cargo.toml +++ b/crates/hiroz-tests/Cargo.toml @@ -33,7 +33,6 @@ serde_json = "1.0" # For parsing --json output in the hu plugin tests [features] # Forwards to hiroz; see that crate. Off by default. -pooled-payload = ["hiroz/pooled-payload"] default = [] ros-msgs = [ "dep:hiroz-msgs", diff --git a/crates/hiroz-tests/tests/intra_process.rs b/crates/hiroz-tests/tests/intra_process.rs index 382b6d370..7f95275e1 100644 --- a/crates/hiroz-tests/tests/intra_process.rs +++ b/crates/hiroz-tests/tests/intra_process.rs @@ -38,11 +38,10 @@ use std::{ use common::*; use hiroz::{ - Builder, ZBuf, + Builder, local_bus::{Delivery, Published}, }; -use hiroz_msgs::std_msgs::{ByteMultiArray, Int32, String as RosString}; -use zenoh_buffers::buffer::SplitBuffer; +use hiroz_msgs::std_msgs::{Int32, String as RosString}; const DELIVERY_DEADLINE: Duration = Duration::from_secs(5); @@ -265,88 +264,6 @@ fn dropping_the_subscriber_unregisters_it() -> hiroz::Result<()> { Ok(()) } -/// One payload buffer, reused across sends, written in place. -/// -/// Delivering an `Arc` without serializing still leaves the publisher -/// allocating a payload buffer per message. This is the test for reusing one -/// instead. Two things must both hold, and neither implies the other: -/// -/// - the **payload allocation** must not move between sends, or the buffer was -/// silently reallocated and nothing was reused; -/// - the subscriber must see the value written for **that** send, or the -/// in-place write did not reach the receiver. -/// -/// The `Arc::get_mut` on each iteration is itself load bearing: it succeeds only -/// because no one still holds the previous message. That is the invariant a -/// pool has to respect, so a change that leaks a reference fails here. -#[cfg(feature = "pooled-payload")] -#[test] -fn a_pooled_payload_buffer_is_written_in_place_and_reused() -> hiroz::Result<()> { - let router = TestRouter::new(); - let ctx = create_hiroz_context_with_router(&router)?; - let node = ctx.create_node("zc_pool").build()?; - - // (address of the received Arc, address of its payload bytes, the stamp) - let seen: Arc>> = Arc::new(Mutex::new(Vec::new())); - let sink = seen.clone(); - let _sub = node - .create_sub::("pooled") - .build_with_shared_callback(move |msg: Arc| { - let bytes = msg.data.contiguous(); - let stamp = u64::from_le_bytes(bytes[0..8].try_into().expect("8 bytes")); - sink.lock().expect("poisoned").push(( - Arc::as_ptr(&msg) as usize, - bytes.as_ptr() as usize, - stamp, - )); - })?; - - let publisher = node - .create_pub::("pooled") - .with_intra_process_only() - .build()?; - - let mut slot = Arc::new(ByteMultiArray { - data: ZBuf::from(vec![0xAAu8; 64]), - ..Default::default() - }); - - let mut sent_payload_addrs = Vec::new(); - for stamp in [1u64, 2, 3] { - let payload = Arc::get_mut(&mut slot) - .expect("nobody still holds the previous message") - .data - .as_mut_slice() - .expect("a solely owned single-slice buffer must be writable in place"); - sent_payload_addrs.push(payload.as_ptr() as usize); - payload[0..8].copy_from_slice(&stamp.to_le_bytes()); - - assert_eq!(publisher.publish_shared(slot.clone())?, 1); - } - - let got = seen.lock().expect("poisoned"); - assert_eq!(got.len(), 3, "not every send arrived"); - - assert!( - sent_payload_addrs.windows(2).all(|w| w[0] == w[1]), - "the payload buffer moved between sends ({sent_payload_addrs:?}) — it was \ - reallocated, so nothing was reused" - ); - let slot_addr = Arc::as_ptr(&slot) as usize; - for (i, (arc_addr, payload_addr, stamp)) in got.iter().enumerate() { - assert_eq!( - *arc_addr, slot_addr, - "send {i} delivered a different allocation" - ); - assert_eq!( - *payload_addr, sent_payload_addrs[i], - "send {i} delivered a different payload buffer" - ); - assert_eq!(*stamp, i as u64 + 1, "send {i} carried a stale value"); - } - Ok(()) -} - /// #39 — an ordinary publisher must reach a shared-callback subscriber. /// /// The bus subscriber's wire half used to be forced to `Locality::Remote`, diff --git a/crates/hiroz/Cargo.toml b/crates/hiroz/Cargo.toml index a0203f86d..2d98186e5 100644 --- a/crates/hiroz/Cargo.toml +++ b/crates/hiroz/Cargo.toml @@ -81,7 +81,6 @@ default = ["config-builders", "jazzy", "rmw-zenoh"] # crates.io: it needs ZSlice::as_mut_slice, which is not in any released # zenoh-buffers. A workspace that enables it must supply its own # [patch.crates-io] for zenoh-buffers. No hiroz code path requires it. -pooled-payload = [] config-builders = [] generate-configs = [] protobuf = ["prost", "prost-build"] diff --git a/crates/hiroz/src/zbuf.rs b/crates/hiroz/src/zbuf.rs index 5774535dd..db2b2cd3b 100644 --- a/crates/hiroz/src/zbuf.rs +++ b/crates/hiroz/src/zbuf.rs @@ -51,29 +51,6 @@ impl ZBuf { Self(zbuf) } - /// The bytes of this buffer, mutably, when they can be written in place. - /// - /// This is what lets a publisher reuse one payload buffer across sends - /// instead of allocating a new one each time. It returns `None` — and the - /// caller must then allocate — whenever writing in place would be wrong: - /// - /// - another owner holds the same backing buffer, so a write would be - /// visible to them; - /// - the buffer is not a `Vec` (a shared-memory backing, for example); - /// - the buffer is made of more than one slice, and so has no contiguous - /// mutable view. - /// - /// The bytes handed back are this buffer's own window, never the whole - /// allocation behind it. - /// - /// Requires the `pooled-payload` feature, which is off by default: this - /// needs an accessor that is not in any released `zenoh-buffers`, so a - /// workspace enabling it must patch that crate itself. - #[cfg(feature = "pooled-payload")] - #[inline] - pub fn as_mut_slice(&mut self) -> Option<&mut [u8]> { - self.0.as_mut_slice() - } } // Conversions @@ -293,37 +270,6 @@ mod tests { assert_eq!(bytes.as_ref(), &[1, 2, 3]); } - #[cfg(feature = "pooled-payload")] - #[test] - fn as_mut_slice_sole_owner_writes_in_place() { - let mut zbuf = ZBuf::from(vec![0u8; 16]); - let before = zbuf.contiguous().as_ptr(); - - zbuf.as_mut_slice() - .expect("a freshly built ZBuf is solely owned")[0..8] - .copy_from_slice(&42u64.to_le_bytes()); - - let bytes = zbuf.contiguous(); - assert_eq!(u64::from_le_bytes(bytes[0..8].try_into().unwrap()), 42); - assert_eq!(bytes.as_ptr(), before, "the write reallocated"); - } - - #[cfg(feature = "pooled-payload")] - #[test] - fn as_mut_slice_multi_slice_returns_none() { - use zenoh_buffers::ZSlice; - - let mut inner = ZenohZBuf::default(); - inner.push_zslice(ZSlice::from(vec![1u8, 2])); - inner.push_zslice(ZSlice::from(vec![3u8, 4])); - let mut zbuf = ZBuf::from_zenoh(inner); - - assert!( - zbuf.as_mut_slice().is_none(), - "a two-slice buffer has no contiguous mutable view" - ); - } - #[test] fn test_zbuf_serialize_json() { let zbuf = ZBuf::from(vec![1u8, 2, 3, 4, 5]); From 23ea748f5043c2e2bde0ac8f48b5db5d67192dee Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Wed, 2 Sep 2026 08:44:32 +0800 Subject: [PATCH 25/37] fix(pubsub): refuse a publish an owned subscriber cannot receive, and publish on the wire first Two defects found by adversarial review, each reported independently by more than one reviewer. An owned subscriber was starved in silence. Channel::publish filters on is_shared(), so it is invisible to the shared path, and an intra_process_only publisher has no wire behind the bus. A registered receiver of the right type got nothing while the caller was told Ok(Bus(NoTaker)) - reachable through publish_shared with an owned subscriber, and through publish_owned whenever there is not exactly one owning receiver. It cannot be repaired by serving a clone, because ZMessage is Send + Sync + Sized and not Clone, so the publisher refuses instead - the way refuse_durable_bus already refuses a durable publisher rather than quietly violating its contract. The two methods also ordered bus and wire oppositely. publish_shared ran the bus first, so a wire failure returned Err after every local subscriber had already been called, and Result has no partial-success value to say so: a caller that retried delivered twice locally. Both now publish on the wire first, which is the order publish_owned already argued for in its own comment. Also drops leftovers from the feature removal: an orphaned comment that had re-attached itself to config-builders, which is on by default and so told a reader the default build needs a patched zenoh-buffers, and a stray blank line that put zbuf.rs in the diff for no reason. --- crates/hiroz-tests/tests/intra_process.rs | 150 +++++++++++++++++++++- crates/hiroz/Cargo.toml | 5 - crates/hiroz/src/local_bus.rs | 23 ++++ crates/hiroz/src/pubsub.rs | 71 ++++++---- crates/hiroz/src/zbuf.rs | 1 - 5 files changed, 217 insertions(+), 33 deletions(-) diff --git a/crates/hiroz-tests/tests/intra_process.rs b/crates/hiroz-tests/tests/intra_process.rs index 7f95275e1..e72ad77b7 100644 --- a/crates/hiroz-tests/tests/intra_process.rs +++ b/crates/hiroz-tests/tests/intra_process.rs @@ -1190,12 +1190,16 @@ fn a_depth_refusal_is_reported_as_dropped_not_delivered() -> hiroz::Result<()> { .any(|o| matches!(o, Published::Bus(Delivery::DepthExceeded))), "the depth refusal was never reported; outcomes were {seen:?}" ); + // A plain global bound, not a per-element predicate. The previous form put + // the loop-invariant `seen.len()` inside `any(..)`, where the guard's own + // bound made it false for every element — so the assertion was `!false` for + // all inputs and could never fail. Without this, "refused once at depth 8" + // and "refused once at depth 800" are indistinguishable. assert!( - !seen - .iter() - .any(|o| matches!(o, Published::Bus(Delivery::Sent(_))) - && seen.len() > hiroz::local_bus::MAX_DELIVERY_DEPTH as usize + 2), - "more deliveries than the depth bound allows" + seen.len() <= hiroz::local_bus::MAX_DELIVERY_DEPTH as usize + 2, + "delivery went {} deep; the depth bound is {}", + seen.len(), + hiroz::local_bus::MAX_DELIVERY_DEPTH ); Ok(()) } @@ -1242,3 +1246,139 @@ fn both_publish_methods_agree_when_the_assertions_conflict() -> hiroz::Result<() ); Ok(()) } + +/// An owned subscriber must not be starved in silence. +/// +/// `Channel::publish` filters on `is_shared()`, so an owned subscriber is +/// invisible to the shared path — and an `intra_process_only` publisher has no +/// wire behind the bus. Before this, the message vanished and the caller was +/// told `Ok(Bus(NoTaker))`: a registered receiver of the right type existed and +/// got nothing, with one `debug!` line as the only trace. +/// +/// It cannot be repaired by serving a clone, because `ZMessage` is not `Clone`. +/// So the publisher refuses instead, the way `refuse_durable_bus` refuses a +/// durable publisher rather than quietly violating its contract. +#[test] +fn publish_shared_refuses_when_only_owned_subscribers_are_listening() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx = create_hiroz_context_with_router(&router)?; + let node = ctx.create_node("owned_starve").build()?; + + let hits = Arc::new(AtomicUsize::new(0)); + let h = hits.clone(); + let _sub = node + .create_sub::("owned_starve") + .build_with_owned_callback(move |_m: RosString| { + h.fetch_add(1, Ordering::SeqCst); + })?; + + let publisher = node + .create_pub::("owned_starve") + .with_intra_process_only() + .build()?; + wait_for_ready(Duration::from_millis(300)); + + let outcome = publisher.publish_shared(Arc::new(RosString { + data: "nobody shared is listening".to_owned(), + })); + + assert!( + outcome.is_err(), + "an owned subscriber was registered and could not be served, and there is \ + no wire: this must refuse, not report success. Got {outcome:?}" + ); + // The control: it really was unserviceable, not merely refused. + assert_eq!( + hits.load(Ordering::SeqCst), + 0, + "the owned subscriber cannot be served by a shared publish" + ); + Ok(()) +} + +/// The same starvation reached through `publish_owned`, which is the likelier +/// route to it: two owning subscribers means nothing can be given away, so it +/// falls back to sharing — into the case above. +#[test] +fn publish_owned_refuses_when_two_owned_subscribers_cannot_be_served() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx = create_hiroz_context_with_router(&router)?; + let node = ctx.create_node("owned_two").build()?; + + let a = Arc::new(AtomicUsize::new(0)); + let b = Arc::new(AtomicUsize::new(0)); + let (ca, cb) = (a.clone(), b.clone()); + let _s1 = node + .create_sub::("owned_two") + .build_with_owned_callback(move |_m: RosString| { + ca.fetch_add(1, Ordering::SeqCst); + })?; + let _s2 = node + .create_sub::("owned_two") + .build_with_owned_callback(move |_m: RosString| { + cb.fetch_add(1, Ordering::SeqCst); + })?; + + let publisher = node + .create_pub::("owned_two") + .with_intra_process_only() + .build()?; + wait_for_ready(Duration::from_millis(300)); + + let outcome = publisher.publish_owned(RosString { + data: "two takers, nothing to give".to_owned(), + }); + + assert!( + outcome.is_err(), + "two owning subscribers cannot both be given the value, and neither can be \ + served by the shared fallback: this must refuse. Got {outcome:?}" + ); + assert_eq!( + (a.load(Ordering::SeqCst), b.load(Ordering::SeqCst)), + (0, 0), + "neither owned subscriber is serviceable here" + ); + Ok(()) +} + +/// Reordering `publish_shared`'s `BusAndWire` arm to publish on the wire first +/// must not stop it serving the bus. The ordering itself exists so that a wire +/// failure returns `Err` before any local subscriber has run — otherwise a +/// caller that retries delivers twice locally, and `Result` has no +/// partial-success value with which to warn them. +#[test] +fn a_remote_publisher_serves_the_bus_after_the_wire() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx = create_hiroz_context_with_router(&router)?; + let node = ctx.create_node("order_near").build()?; + + let near = Arc::new(AtomicUsize::new(0)); + let n = near.clone(); + let _sub = node + .create_sub::("order_topic") + .build_with_shared_callback(move |_m: Arc| { + n.fetch_add(1, Ordering::SeqCst); + })?; + + let publisher = node + .create_pub::("order_topic") + .with_locality(zenoh::sample::Locality::Remote) + .build()?; + wait_for_ready(Duration::from_millis(300)); + + let outcome = publisher.publish_shared(Arc::new(RosString { + data: "both routes".to_owned(), + }))?; + + assert!( + matches!(outcome, Published::BusAndWire(Delivery::Sent(1))), + "the bus half must still run, and after the wire. Got {outcome:?}" + ); + assert_eq!( + near.load(Ordering::SeqCst), + 1, + "the same-session subscriber must still be served exactly once" + ); + Ok(()) +} diff --git a/crates/hiroz/Cargo.toml b/crates/hiroz/Cargo.toml index 2d98186e5..23534ecb6 100644 --- a/crates/hiroz/Cargo.toml +++ b/crates/hiroz/Cargo.toml @@ -76,11 +76,6 @@ prost-build = { workspace = true, optional = true } [features] default = ["config-builders", "jazzy", "rmw-zenoh"] -# Exposes ZBuf::as_mut_slice, so a caller can reuse one payload buffer across -# sends instead of allocating per send. OFF by default and NOT usable from -# crates.io: it needs ZSlice::as_mut_slice, which is not in any released -# zenoh-buffers. A workspace that enables it must supply its own -# [patch.crates-io] for zenoh-buffers. No hiroz code path requires it. config-builders = [] generate-configs = [] protobuf = ["prost", "prost-build"] diff --git a/crates/hiroz/src/local_bus.rs b/crates/hiroz/src/local_bus.rs index 4d28d124b..b12949894 100644 --- a/crates/hiroz/src/local_bus.rs +++ b/crates/hiroz/src/local_bus.rs @@ -250,6 +250,29 @@ impl Channel { /// commonly publishes — that is exactly what the pong side of a ping/pong /// does — and re-entering under its own read guard is the deadlock this /// workspace has already fixed three times elsewhere. + /// How many subscribers of `T` on this channel want the message **by value**. + /// + /// A publisher needs this to tell apart two outcomes that both surface as + /// [`Delivery::NoTaker`]: nobody is listening, and somebody is listening + /// whom the shared path structurally cannot serve. [`Channel::publish`] + /// filters on `is_shared`, so an owned subscriber is invisible to it — and + /// an `intra_process_only` publisher has no wire behind the bus to catch the + /// message. Without this, that difference is a silent drop reported as success. + /// + /// It cannot be repaired by handing them a clone: `ZMessage` is + /// `Send + Sync + Sized`, not `Clone`. + pub fn owned_receivers(&self) -> usize + where + T: Any + 'static, + { + let wanted = TypeId::of::(); + self.entries + .load() + .iter() + .filter(|e| e.type_id == wanted && !e.is_shared()) + .count() + } + pub fn publish(&self, payload: Arc) -> Delivery where T: Any + Send + Sync + 'static, diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index d6f555038..c0afc806f 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -744,6 +744,23 @@ where } let d = self.local_channel.publish(msg); if d == Delivery::NoTaker { + // Distinguish "nobody is listening" from "somebody is + // listening whom this path cannot serve". The shared path + // filters on is_shared(), so an owned subscriber is skipped + // — and this publisher has no wire to catch the message. + // Reporting Ok here loses it silently. + let owned = self.local_channel.owned_receivers::(); + if owned > 0 { + return Err(format!( + "publish_shared on an intra-process-only publisher for {}: \ + {owned} subscriber(s) on this topic take the message by value, \ + and a shared publish cannot serve them. The message would be \ + dropped with no wire behind it. Use publish_owned(), or build \ + the subscriber with build_with_shared_callback().", + self.entity.topic + ) + .into()); + } debug!( "[PUB] intra-process only, no local subscriber on {}: message dropped", self.entity.topic @@ -757,9 +774,13 @@ where // Both audiences, and no subscriber is on both. TRANSIENT_LOCAL is // safe here: the wire publish populates the cache as it always did. Route::BusAndWire => { - let d = self.local_channel.publish(msg.clone()); + // Wire first, matching publish_owned. Bus delivery is + // synchronous, so running it first and then failing on the wire + // returns Err *after* every local subscriber has already been + // called — and Result has no partial-success value + // to say so. A caller that retries then delivers twice locally. self.publish(&msg)?; - Ok(Published::BusAndWire(d)) + Ok(Published::BusAndWire(self.local_channel.publish(msg))) } Route::WireOnly => { self.publish(&msg)?; @@ -768,25 +789,6 @@ where } } - /// Publish `msg` by value, giving it away when exactly one receiver wants it. - /// - /// Where [`publish_shared`](Self::publish_shared) hands every receiver the - /// same read-only `Arc`, this hands a sole owning subscriber the value - /// itself — free to mutate it, consume it, or put it back in a pool. It is - /// the move rclcpp performs when a topic has exactly one taker. - /// - /// Falls back, in order: to the bus as a shared `Arc` if the audience is - /// entirely local, then to the wire. The message is never dropped by the - /// fallback, only by an `intra_process_only` publisher with no listener. - /// - /// Returns how many receivers took it on the bus; `0` means it went to the - /// wire. See issue circle/hiroz-bench#36. - /// Refuse a durable publisher the bus, which has no durability cache. - /// - /// This lives in one place because it must guard **every** route onto the - /// bus. It was originally written inline in `publish_shared`, and - /// `publish_owned` reached the bus around it — the same violation, through - /// the sibling method. See #133. /// Which routes this publisher's messages take. /// /// Resolved in one place because it is asserted by the caller and read by @@ -809,6 +811,12 @@ where } } + /// Refuse a durable publisher the bus, which has no durability cache. + /// + /// This lives in one place because it must guard **every** route onto the + /// bus. It was originally written inline in `publish_shared`, and + /// `publish_owned` reached the bus around it — the same violation, through + /// the sibling method. See #133. fn refuse_durable_bus(&self) -> Option { if !self.intra_process_only { // A `Locality::Remote` publisher still puts every message on the @@ -830,6 +838,23 @@ where ) } + /// Publish `msg` by value, giving it away when exactly one receiver wants it. + /// + /// Where [`publish_shared`](Self::publish_shared) hands every receiver the + /// same read-only `Arc`, this hands a sole owning subscriber the value + /// itself — free to mutate it, consume it, or put it back in a pool. It is + /// the move rclcpp performs when a topic has exactly one taker. + /// + /// The route is the same one [`publish_shared`](Self::publish_shared) + /// documents, resolved from what the caller asserted. Within a bus route, + /// a sole owning subscriber is given the value; with more than one taker + /// there is nothing to give away, so it is shared instead. + /// + /// Returns a [`Published`] saying which routes ran — not a count. An + /// `intra_process_only` publisher with no listener reports + /// `Bus(Delivery::NoTaker)` and the message is dropped; it does **not** + /// fall through to the wire, because such a publisher has none. + /// See circle/hiroz-bench#36. pub fn publish_owned(&self, msg: T) -> Result where T: Send + Sync + 'static, @@ -856,7 +881,9 @@ where } match self.local_channel.publish_owned(msg) { Ok(d) => Ok(Published::Bus(d)), - // Handed back untouched: no sole owning receiver. Share it. + // Handed back untouched: not exactly one owning receiver. + // Share it instead — publish_shared refuses rather than + // dropping if the only subscribers are owned ones. Err(returned) => self.publish_shared(Arc::new(returned)), } } diff --git a/crates/hiroz/src/zbuf.rs b/crates/hiroz/src/zbuf.rs index db2b2cd3b..f2645f047 100644 --- a/crates/hiroz/src/zbuf.rs +++ b/crates/hiroz/src/zbuf.rs @@ -50,7 +50,6 @@ impl ZBuf { pub fn from_zenoh(zbuf: ZenohZBuf) -> Self { Self(zbuf) } - } // Conversions From efa8430bdfd3e7d648cdac730f6baee677446095 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Wed, 2 Sep 2026 14:23:20 +0800 Subject: [PATCH 26/37] fix(local_bus): re-raise the re-entrancy violation, count deliveries, reclaim channels Three more findings from the adversarial reviews. The panic isolation swallowed the crate's own re-entrancy assertion. It reports by panicking, and a nested delivery raises it from inside the outer catch_unwind - so the detector built to catch 'a user callback ran while a lock was held' was downgraded to a log line, in exactly the case the bus makes most reachable: a callback that publishes. It is now re-raised in debug builds, told apart from a user panic by a stable prefix rather than by guessing. Delivery::Sent(n) counted subscribers invoked, not subscribers that returned. A sole subscriber panicking on every message reported Sent(1) forever, so a caller falling back on NoTaker could not see a total delivery failure. The registry only ever grew. channel() runs in every publisher build, so this was one entry per topic for the process lifetime, not one per ZContext as #152 says. It now reclaims what no endpoint holds, which is safe only because a strong count of one proves no ZPub or ZSub can still reach the channel. --- crates/hiroz-tests/tests/intra_process.rs | 85 +++++++++++++++++++++++ crates/hiroz/src/local_bus.rs | 65 ++++++++++++++--- crates/hiroz/src/reentrancy.rs | 14 +++- 3 files changed, 153 insertions(+), 11 deletions(-) diff --git a/crates/hiroz-tests/tests/intra_process.rs b/crates/hiroz-tests/tests/intra_process.rs index e72ad77b7..4c278dd57 100644 --- a/crates/hiroz-tests/tests/intra_process.rs +++ b/crates/hiroz-tests/tests/intra_process.rs @@ -1382,3 +1382,88 @@ fn a_remote_publisher_serves_the_bus_after_the_wire() -> hiroz::Result<()> { ); Ok(()) } + +/// A sole subscriber that panics delivered nothing, and must not be counted. +/// +/// `invoke_isolated` returns whether the callback returned normally, and every +/// call site used to discard it — so `Delivery::Sent(n)` counted subscribers +/// *invoked*. A caller that falls back on `NoTaker` could not see a total +/// delivery failure: one subscriber, panicking on every message, reported +/// `Sent(1)` forever. +#[test] +fn a_panicking_sole_subscriber_is_not_counted_as_delivered() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx = create_hiroz_context_with_router(&router)?; + let node = ctx.create_node("panic_count").build()?; + + let _sub = node + .create_sub::("panic_count") + .build_with_shared_callback(move |_m: Arc| { + panic!("this subscriber always fails"); + })?; + + let publisher = node + .create_pub::("panic_count") + .with_intra_process_only() + .build()?; + wait_for_ready(Duration::from_millis(300)); + + let prev = std::panic::take_hook(); + std::panic::set_hook(Box::new(|_| {})); + let outcome = publisher.publish_shared(Arc::new(RosString { + data: "nobody will process this".to_owned(), + }))?; + std::panic::set_hook(prev); + + assert_eq!( + outcome, + Published::Bus(Delivery::NoTaker), + "the only subscriber panicked, so nothing was delivered; reporting Sent(1) \ + tells the caller a message landed when none did. Got {outcome:?}" + ); + Ok(()) +} + +/// A channel nobody holds any more is reclaimed, so the registry does not grow +/// for the life of the process. +/// +/// `channel()` runs in **every** `ZPubBuilder::build()`, so this is not one +/// entry per `ZContext` — a process publishing on dynamically-named topics +/// accumulated one entry per topic, permanently, long after every publisher was +/// dropped. Reclamation is safe only because an `Arc::strong_count` of one +/// proves no `ZPub` or `ZSub` can still reach the channel, so a later builder +/// cannot be split from a live endpoint. +#[test] +fn a_channel_no_endpoint_holds_is_reclaimed() -> hiroz::Result<()> { + let router = TestRouter::new(); + let ctx = create_hiroz_context_with_router(&router)?; + let node = ctx.create_node("reclaim").build()?; + + let before = hiroz::local_bus::total_channels(); + for i in 0..24 { + let p = node + .create_pub::(&format!("ephemeral_{i}")) + .build()?; + drop(p); + } + let after_ephemeral = hiroz::local_bus::total_channels(); + assert!( + after_ephemeral - before <= 4, + "24 publishers were created and dropped; the registry grew by {} channels. \ + It should reclaim the ones no endpoint holds.", + after_ephemeral - before + ); + + // The control. Without it this test passes equally well against a registry + // that reclaims indiscriminately — which would split a live publisher from + // a subscriber created afterwards, turning a leak into lost messages. + let held = node.create_pub::("held_topic").build()?; + let with_held = hiroz::local_bus::total_channels(); + let _forces_a_reclaim_pass = node.create_pub::("spacer_topic").build()?; + assert!( + hiroz::local_bus::total_channels() >= with_held, + "a channel a live publisher still holds must survive a reclamation pass" + ); + drop(held); + Ok(()) +} diff --git a/crates/hiroz/src/local_bus.rs b/crates/hiroz/src/local_bus.rs index b12949894..8da17d00e 100644 --- a/crates/hiroz/src/local_bus.rs +++ b/crates/hiroz/src/local_bus.rs @@ -59,6 +59,7 @@ //! cloned out, the guard dropped, and only then are the callbacks called. use std::{ +use crate::reentrancy::REENTRANCY_VIOLATION; any::{Any, TypeId}, cell::Cell, collections::HashMap, @@ -232,7 +233,20 @@ fn invoke_isolated(site: &'static str, f: impl FnOnce()) -> bool { crate::reentrancy::assert_no_guards_held(site); match std::panic::catch_unwind(std::panic::AssertUnwindSafe(f)) { Ok(()) => true, - Err(_) => { + Err(payload) => { + // Re-raise the crate's own re-entrancy violation instead of + // logging it. `assert_no_guards_held` reports by panicking, and a + // *nested* delivery runs inside this catch_unwind — so without + // this, the detector built to catch "a user callback ran while a + // lock was held" is silently downgraded to a log line in exactly + // the case the bus makes most reachable: a callback that publishes. + #[cfg(debug_assertions)] + if payload + .downcast_ref::() + .is_some_and(|m| m.starts_with(REENTRANCY_VIOLATION)) + { + std::panic::resume_unwind(payload); + } tracing::error!( "[BUS] a subscriber callback panicked during intra-process delivery at {site}; \ the panic was contained and delivery continued to the remaining subscribers" @@ -313,23 +327,27 @@ impl Channel { // through the snapshot, hand over the only reference, and touch no // refcount at all beyond the one the caller already holds. None => { - invoke_isolated("local_bus::publish", || first.call_shared(erased)); - Delivery::Sent(1) + if invoke_isolated("local_bus::publish", || first.call_shared(erased)) { + Delivery::Sent(1) + } else { + // The only subscriber panicked. Reporting Sent(1) here told + // the caller a message was delivered when none was. + Delivery::NoTaker + } } // More than one receiver genuinely needs a reference each, so the // clones start here and not before. Some(second) => { let e1 = erased.clone(); let e2 = erased.clone(); - invoke_isolated("local_bus::publish", || first.call_shared(e1)); - invoke_isolated("local_bus::publish", || second.call_shared(e2)); - let mut count = 2; + let mut count = 0; + count += invoke_isolated("local_bus::publish", || first.call_shared(e1)) as usize; + count += invoke_isolated("local_bus::publish", || second.call_shared(e2)) as usize; for entry in matching { let ec = erased.clone(); - invoke_isolated("local_bus::publish", || entry.call_shared(ec)); - count += 1; + count += invoke_isolated("local_bus::publish", || entry.call_shared(ec)) as usize; } - // The count is subscribers invoked, not subscribers that + // The count is subscribers that returned normally, not // returned normally. A panicking one is logged above. Delivery::Sent(count) } @@ -403,6 +421,20 @@ static NEXT_ID: AtomicU64 = AtomicU64::new(0); /// Resolve the channel for `(zid, topic)`, creating it if needed. /// /// Call this once, when a publisher or subscriber is built — not per message. +/// How many channels the registry currently holds, across every session. +/// +/// Exposed so the reclamation in [`channel`] can be observed. Without a way to +/// read this, "the registry no longer grows without bound" is a claim no test +/// can make: the leak is invisible from the outside, which is why it survived +/// this long. +pub fn total_channels() -> usize { + let bus = match BUS.read() { + Ok(b) => b, + Err(e) => e.into_inner(), + }; + bus.values().map(|topics| topics.len()).sum() +} + pub fn channel(zid: ZenohId, topic: &str) -> Arc { // Fast path: it usually exists, and a read lock lets concurrent builders through. { @@ -419,6 +451,21 @@ pub fn channel(zid: ZenohId, topic: &str) -> Arc { Ok(b) => b, Err(e) => e.into_inner(), }; + + // Reclaim while we already hold the write lock. A channel is removable only + // when the registry is its sole owner and it has no subscribers: every + // `ZPub` and `ZSub` holds its own `Arc`, so a strong count of one + // proves no endpoint can still reach it, and a later builder gets a fresh + // channel that nobody is split from. + // + // Without this the map only ever grows. It is not merely one entry per + // `ZContext` — `channel()` runs in *every* `ZPubBuilder::build()`, so a + // process that publishes on dynamically-named topics accumulates an entry + // per topic, for its lifetime, even after every publisher is dropped. + if let Some(topics) = bus.get_mut(&zid) { + topics.retain(|_, ch| Arc::strong_count(ch) > 1 || !ch.entries.load().is_empty()); + } + bus.entry(zid) .or_default() .entry(topic.to_owned()) diff --git a/crates/hiroz/src/reentrancy.rs b/crates/hiroz/src/reentrancy.rs index 0127262ac..71fa1eeb0 100644 --- a/crates/hiroz/src/reentrancy.rs +++ b/crates/hiroz/src/reentrancy.rs @@ -93,17 +93,27 @@ pub fn live_guards() -> usize { /// message — when this fires, *which* callback was about to run is the useful /// information, not the counter's backtrace. #[inline(always)] +/// The opening words of a re-entrancy violation panic. +/// +/// [`local_bus::invoke_isolated`](crate::local_bus) contains subscriber panics +/// so one bad callback cannot censor its siblings. That containment must not +/// swallow *this* panic: it is the crate reporting its own contract violation, +/// not a user fault, and a nested delivery raises it from inside that +/// `catch_unwind`. The prefix is what lets the two be told apart. +pub const REENTRANCY_VIOLATION: &str = "hiroz re-entrancy rule violated"; + pub fn assert_no_guards_held(site: &str) { #[cfg(debug_assertions)] { let live = live_guards(); assert!( live == 0, - "hiroz re-entrancy rule violated at `{site}`: about to invoke a user \ + "{} at `{site}`: about to invoke a user \ callback with {live} lock guard(s) live on this thread. A callback \ that re-enters hiroz will deadlock if it touches a lock this thread \ holds. Fix: collect what you need into an owned value, drop every \ - guard, then invoke the callback." + guard, then invoke the callback.", + REENTRANCY_VIOLATION, ); } #[cfg(not(debug_assertions))] From 356747b62a3b4416e56f0663424c1f113d6c2a85 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Wed, 2 Sep 2026 14:46:15 +0800 Subject: [PATCH 27/37] fix: repair two bad insertions; document the guard's scope and the graph token The previous commit did not compile: the reentrancy import landed inside a use std::{..} brace group, and the new constant was inserted between an #[inline(always)] attribute and the function it belongs to. Both are mechanical slips from anchor-based editing, and ran=0 was the tell - not the exit code, which cargo returns for a build failure and a test failure alike. Also documents the two review findings that are deliberately not code changes. MAX_DELIVERY_DEPTH now says what it does not bound: it is thread-local, so a callback that spawns a thread escapes it, and a topic cycle across the wire is not bounded at all. Suppressing the wire half for nested deliveries was considered and rejected - a nested publish is a distinct message, so dropping it trades a loud problem for a silent one. And with_intra_process_only now records that such a publisher still declares a liveliness token, so other nodes' publisher counts and matched events include an endpoint that can reach nobody. --- crates/hiroz/src/local_bus.rs | 23 ++++++++++++++++++++++- crates/hiroz/src/pubsub.rs | 16 ++++++++++++++++ crates/hiroz/src/reentrancy.rs | 12 ++++++------ 3 files changed, 44 insertions(+), 7 deletions(-) diff --git a/crates/hiroz/src/local_bus.rs b/crates/hiroz/src/local_bus.rs index 8da17d00e..41a43fa11 100644 --- a/crates/hiroz/src/local_bus.rs +++ b/crates/hiroz/src/local_bus.rs @@ -59,7 +59,6 @@ //! cloned out, the guard dropped, and only then are the callbacks called. use std::{ -use crate::reentrancy::REENTRANCY_VIOLATION; any::{Any, TypeId}, cell::Cell, collections::HashMap, @@ -69,6 +68,8 @@ use crate::reentrancy::REENTRANCY_VIOLATION; }, }; +use crate::reentrancy::REENTRANCY_VIOLATION; + use arc_swap::ArcSwap; use tracing::debug; @@ -88,6 +89,26 @@ use zenoh::session::ZenohId; /// far below the depth at which the stack is in danger. See issue circle/hiroz-bench#40. /// Public so a test can assert the exact bound rather than a loose ceiling: /// a hand-copied constant lets a change to this value slip past unnoticed. +/// +/// # What this does not bound +/// +/// It bounds the **stack**, on **one thread**. Two things escape it, and both +/// are inherent rather than oversights: +/// +/// - **A callback that spawns a thread and publishes there** starts at depth +/// zero, because the counter is thread-local. Unbounded recursion then +/// exhausts threads rather than the stack, and this guard never trips. +/// - **A topic cycle across the wire is not bounded at all.** A +/// `Locality::Remote` publisher runs both routes, so a nested delivery emits +/// a wire message per nesting level; a peer that echoes amplifies again. That +/// is a property of publish/subscribe — any ROS 2 node that publishes to a +/// topic it subscribes to does the same, and no client library prevents it. +/// Suppressing the wire half for nested deliveries was considered and +/// rejected: a nested publish is a distinct message the callback chose to +/// send, so dropping it would trade a loud problem for a silent one. +/// +/// The guard exists to turn a stack overflow into a dropped message and a +/// greppable log. It is not a cycle detector. pub const MAX_DELIVERY_DEPTH: u32 = 8; thread_local! { diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index c0afc806f..82ecb9b6c 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -315,6 +315,22 @@ impl ZPubBuilder { /// subscriber that did not register on the bus with /// [`ZSubBuilder::build_with_shared_callback`]. Set it only where both ends /// are known. See issue circle/hiroz-bench#36. + /// # This publisher still appears in the ROS graph + /// + /// The liveliness token is declared in `build()` before any locality is + /// consulted, so a publisher that can reach nothing outside this process + /// still advertises itself as an ordinary ROS 2 endpoint. Every other node + /// counts it: `ros2 topic info -v`, `ros2 node info`, a remote subscriber's + /// matched-publisher event, and any `Graph::count` derived from them. + /// `ros2 topic echo` shows the topic and never a message. + /// + /// Suppressing the token would make the counts honest and remove the + /// publisher from `ros2 node info`, which is a change to graph visibility + /// and belongs in its own change with its own test. Until then this is a + /// disclosed limitation, not an oversight: a node debugging "the publisher + /// is there but nothing arrives" has no signal separating this from a QoS + /// mismatch. + pub fn with_intra_process_only(mut self) -> Self { self.intra_process_only = true; self diff --git a/crates/hiroz/src/reentrancy.rs b/crates/hiroz/src/reentrancy.rs index 71fa1eeb0..1ea88f21b 100644 --- a/crates/hiroz/src/reentrancy.rs +++ b/crates/hiroz/src/reentrancy.rs @@ -87,12 +87,6 @@ pub fn live_guards() -> usize { } } -/// Panics (debug only) if any tracked guard is live on this thread. -/// -/// Call immediately before invoking user code. `site` is reproduced in the panic -/// message — when this fires, *which* callback was about to run is the useful -/// information, not the counter's backtrace. -#[inline(always)] /// The opening words of a re-entrancy violation panic. /// /// [`local_bus::invoke_isolated`](crate::local_bus) contains subscriber panics @@ -102,6 +96,12 @@ pub fn live_guards() -> usize { /// `catch_unwind`. The prefix is what lets the two be told apart. pub const REENTRANCY_VIOLATION: &str = "hiroz re-entrancy rule violated"; +/// Panics (debug only) if any tracked guard is live on this thread. +/// +/// Call immediately before invoking user code. `site` is reproduced in the panic +/// message — when this fires, *which* callback was about to run is the useful +/// information, not the counter's backtrace. +#[inline(always)] pub fn assert_no_guards_held(site: &str) { #[cfg(debug_assertions)] { From f873263a64a99a61e686d037e336c4a98c97deab Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Wed, 2 Sep 2026 14:50:04 +0800 Subject: [PATCH 28/37] test: assert deliveries rather than invocations in the panic tests Both tests pinned the old Delivery::Sent semantics - one asserted Sent(1) for a sole subscriber that panicked, the other Sent(3) where two of three returned. Under the corrected contract those are NoTaker and Sent(2), and the counter assertions are what carry the real property in each case. The duplicate test added alongside the fix is dropped: the existing sole-subscriber test covers it once its assertion is right. --- crates/hiroz-tests/tests/intra_process.rs | 53 ++++------------------- crates/hiroz/src/pubsub.rs | 1 - 2 files changed, 8 insertions(+), 46 deletions(-) diff --git a/crates/hiroz-tests/tests/intra_process.rs b/crates/hiroz-tests/tests/intra_process.rs index 4c278dd57..eb4913365 100644 --- a/crates/hiroz-tests/tests/intra_process.rs +++ b/crates/hiroz-tests/tests/intra_process.rs @@ -935,8 +935,10 @@ fn a_panicking_subscriber_does_not_stop_delivery_to_the_others() -> hiroz::Resul let delivered = delivered.expect("the panic escaped into the publishing thread"); assert_eq!( delivered, - Published::Bus(Delivery::Sent(3)), - "not every subscriber was invoked" + Published::Bus(Delivery::Sent(2)), + "three subscribers were called and two returned. The count is deliveries, \ + not invocations — the assertions on the two counters below are what \ + prove the panicking one did not stop its siblings." ); assert_eq!( before.load(Ordering::SeqCst), @@ -981,8 +983,10 @@ fn a_panicking_sole_subscriber_does_not_reach_the_publisher() -> hiroz::Result<( let delivered = delivered.expect("the panic escaped into the publishing thread"); assert_eq!( delivered, - Published::Bus(Delivery::Sent(1)), - "the subscriber was not invoked" + Published::Bus(Delivery::NoTaker), + "the sole subscriber panicked, so nothing was delivered. Sent(1) would \ + report a message as landed when none was: Delivery::Sent counts \ + subscribers that returned, not subscribers that were called." ); Ok(()) } @@ -1383,47 +1387,6 @@ fn a_remote_publisher_serves_the_bus_after_the_wire() -> hiroz::Result<()> { Ok(()) } -/// A sole subscriber that panics delivered nothing, and must not be counted. -/// -/// `invoke_isolated` returns whether the callback returned normally, and every -/// call site used to discard it — so `Delivery::Sent(n)` counted subscribers -/// *invoked*. A caller that falls back on `NoTaker` could not see a total -/// delivery failure: one subscriber, panicking on every message, reported -/// `Sent(1)` forever. -#[test] -fn a_panicking_sole_subscriber_is_not_counted_as_delivered() -> hiroz::Result<()> { - let router = TestRouter::new(); - let ctx = create_hiroz_context_with_router(&router)?; - let node = ctx.create_node("panic_count").build()?; - - let _sub = node - .create_sub::("panic_count") - .build_with_shared_callback(move |_m: Arc| { - panic!("this subscriber always fails"); - })?; - - let publisher = node - .create_pub::("panic_count") - .with_intra_process_only() - .build()?; - wait_for_ready(Duration::from_millis(300)); - - let prev = std::panic::take_hook(); - std::panic::set_hook(Box::new(|_| {})); - let outcome = publisher.publish_shared(Arc::new(RosString { - data: "nobody will process this".to_owned(), - }))?; - std::panic::set_hook(prev); - - assert_eq!( - outcome, - Published::Bus(Delivery::NoTaker), - "the only subscriber panicked, so nothing was delivered; reporting Sent(1) \ - tells the caller a message landed when none did. Got {outcome:?}" - ); - Ok(()) -} - /// A channel nobody holds any more is reclaimed, so the registry does not grow /// for the life of the process. /// diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index 82ecb9b6c..1718556dc 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -330,7 +330,6 @@ impl ZPubBuilder { /// disclosed limitation, not an oversight: a node debugging "the publisher /// is there but nothing arrives" has no signal separating this from a QoS /// mismatch. - pub fn with_intra_process_only(mut self) -> Self { self.intra_process_only = true; self From 05ebd8218037ef14e3952ef98654248f7c2f046b Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Wed, 2 Sep 2026 15:31:27 +0800 Subject: [PATCH 29/37] docs(local_bus): repair a comment mangled by an earlier edit It read 'subscribers that returned normally, not returned normally' - the replacement rewrote the first line and left the second, so the line documenting what Delivery::Sent counts said nothing. --- crates/hiroz/src/local_bus.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/hiroz/src/local_bus.rs b/crates/hiroz/src/local_bus.rs index 41a43fa11..363dfa81b 100644 --- a/crates/hiroz/src/local_bus.rs +++ b/crates/hiroz/src/local_bus.rs @@ -369,7 +369,7 @@ impl Channel { count += invoke_isolated("local_bus::publish", || entry.call_shared(ec)) as usize; } // The count is subscribers that returned normally, not - // returned normally. A panicking one is logged above. + // subscribers invoked. A panicking one is logged above. Delivery::Sent(count) } } From 3a57db0f45ebbfb00826f0efd016922c5b3ebc43 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Wed, 2 Sep 2026 15:50:49 +0800 Subject: [PATCH 30/37] fix(local_bus): the re-raise must not break a release build The re-entrancy re-raise is behind cfg(debug_assertions), so in a release build both the REENTRANCY_VIOLATION import and the caught payload are unused - errors under -D warnings, which is what CI builds with. My local gate could not have caught this: it builds debug, where both are live. Gate the import and underscore the binding. --- crates/hiroz/src/local_bus.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/crates/hiroz/src/local_bus.rs b/crates/hiroz/src/local_bus.rs index 363dfa81b..cce935215 100644 --- a/crates/hiroz/src/local_bus.rs +++ b/crates/hiroz/src/local_bus.rs @@ -68,6 +68,8 @@ use std::{ }, }; +// Only the debug-only re-raise below reads this, so the import is gated too. +#[cfg(debug_assertions)] use crate::reentrancy::REENTRANCY_VIOLATION; use arc_swap::ArcSwap; @@ -254,7 +256,7 @@ fn invoke_isolated(site: &'static str, f: impl FnOnce()) -> bool { crate::reentrancy::assert_no_guards_held(site); match std::panic::catch_unwind(std::panic::AssertUnwindSafe(f)) { Ok(()) => true, - Err(payload) => { + Err(_payload) => { // Re-raise the crate's own re-entrancy violation instead of // logging it. `assert_no_guards_held` reports by panicking, and a // *nested* delivery runs inside this catch_unwind — so without @@ -262,11 +264,11 @@ fn invoke_isolated(site: &'static str, f: impl FnOnce()) -> bool { // lock was held" is silently downgraded to a log line in exactly // the case the bus makes most reachable: a callback that publishes. #[cfg(debug_assertions)] - if payload + if _payload .downcast_ref::() .is_some_and(|m| m.starts_with(REENTRANCY_VIOLATION)) { - std::panic::resume_unwind(payload); + std::panic::resume_unwind(_payload); } tracing::error!( "[BUS] a subscriber callback panicked during intra-process delivery at {site}; \ From d769c6e2d1e4eaef291801bc36f0c6955983f6b2 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Wed, 2 Sep 2026 16:04:16 +0800 Subject: [PATCH 31/37] fix(local_bus): unforgeable violation payload, and four routing/accounting fixes From Copilot's review. Each was real. The panic classifier matched a public message prefix, so a subscriber could panic with a String beginning with it and force resume_unwind - defeating the isolation the containment exists to provide. It is now a private type no code outside the crate can construct. publish_moved discarded invoke_isolated's result and reported Sent(1) for a callback that panicked; the shared path was fixed this morning and the owned path was missed. A fan-out where every callback panicked returned Sent(0) while the sole-subscriber arm returned NoTaker for the same outcome, so a caller branching on NoTaker missed one spelling. Reclamation swept only the session being touched, so each retired session kept its final channel for the process lifetime - the session-churn half of the leak rather than a fix for it. A Locality::Remote subscriber was still registered on the same-session bus, receiving exactly what its own allowed_origin filter excludes. The wire half honoured the restriction; the bus half ignored it. Also removes two doc claims that still pointed at the withdrawn graph inference. --- crates/hiroz-tests/tests/intra_process.rs | 2 +- crates/hiroz/src/local_bus.rs | 38 ++++++++++++----- crates/hiroz/src/pubsub.rs | 33 +++++++++++--- crates/hiroz/src/reentrancy.rs | 52 ++++++++++++++++------- 4 files changed, 93 insertions(+), 32 deletions(-) diff --git a/crates/hiroz-tests/tests/intra_process.rs b/crates/hiroz-tests/tests/intra_process.rs index eb4913365..4ad2f28aa 100644 --- a/crates/hiroz-tests/tests/intra_process.rs +++ b/crates/hiroz-tests/tests/intra_process.rs @@ -21,7 +21,7 @@ //! | `a_plain_publish_reaches_a_shared_callback_subscriber` | #39 — no silent same-session loss | //! | `publish_shared_without_a_locality_restriction_arrives_once` | #39 — and no duplicate either | //! | `a_self_publishing_callback_does_not_recurse_without_bound` | #40 — a cycle is refused, not fatal | -//! | `a_publisher_asks_the_graph_instead_of_being_told` | #36 — no drop when nobody is on the bus | +//! | `a_plain_publisher_takes_the_wire_for_an_ordinary_subscriber` | #36 — no drop when nobody is on the bus | //! | `the_wire_is_used_when_a_subscriber_is_off_session` | #36 — and the bus is not, so no duplicate | //! | `a_sole_receiver_is_given_the_message_to_own` | #36 — the move, not a shared Arc | diff --git a/crates/hiroz/src/local_bus.rs b/crates/hiroz/src/local_bus.rs index cce935215..255c01c04 100644 --- a/crates/hiroz/src/local_bus.rs +++ b/crates/hiroz/src/local_bus.rs @@ -70,7 +70,7 @@ use std::{ // Only the debug-only re-raise below reads this, so the import is gated too. #[cfg(debug_assertions)] -use crate::reentrancy::REENTRANCY_VIOLATION; +use crate::reentrancy::ReentrancyViolation; use arc_swap::ArcSwap; @@ -264,10 +264,10 @@ fn invoke_isolated(site: &'static str, f: impl FnOnce()) -> bool { // lock was held" is silently downgraded to a log line in exactly // the case the bus makes most reachable: a callback that publishes. #[cfg(debug_assertions)] - if _payload - .downcast_ref::() - .is_some_and(|m| m.starts_with(REENTRANCY_VIOLATION)) - { + // A type, not a message prefix: a subscriber can panic with any + // string it likes, so matching text would let user code force a + // re-raise and defeat the isolation this function provides. + if _payload.downcast_ref::().is_some() { std::panic::resume_unwind(_payload); } tracing::error!( @@ -372,7 +372,16 @@ impl Channel { } // The count is subscribers that returned normally, not // subscribers invoked. A panicking one is logged above. - Delivery::Sent(count) + // + // Zero is NoTaker, never Sent(0): the sole-subscriber arm above + // already reports NoTaker for the same outcome, and two spellings + // of "nothing was delivered" mean a caller that branches on + // NoTaker silently misses one of them. + if count == 0 { + Delivery::NoTaker + } else { + Delivery::Sent(count) + } } } } @@ -420,8 +429,13 @@ impl Channel { DELIVERY_DEPTH.with(|d| d.set(depth + 1)); let _depth_guard = DepthGuard; - invoke_isolated("local_bus::publish_owned", || cb(Box::new(payload))); - Ok(Delivery::Sent(1)) + // Mirror the shared path: a callback that panicked delivered nothing, so + // reporting Sent(1) would tell the caller a message landed when none did. + if invoke_isolated("local_bus::publish_owned", || cb(Box::new(payload))) { + Ok(Delivery::Sent(1)) + } else { + Ok(Delivery::NoTaker) + } } /// How many subscribers this channel has, regardless of type. Diagnostics @@ -485,9 +499,13 @@ pub fn channel(zid: ZenohId, topic: &str) -> Arc { // `ZContext` — `channel()` runs in *every* `ZPubBuilder::build()`, so a // process that publishes on dynamically-named topics accumulates an entry // per topic, for its lifetime, even after every publisher is dropped. - if let Some(topics) = bus.get_mut(&zid) { + // Sweep every session, not just this one. Scoping it to `zid` left each + // retired session holding its final channel for the process lifetime, which + // is the session-churn half of the leak rather than a fix for it. + bus.retain(|_, topics| { topics.retain(|_, ch| Arc::strong_count(ch) > 1 || !ch.entries.load().is_empty()); - } + !topics.is_empty() + }); bus.entry(zid) .or_default() diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index 1718556dc..f56104192 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -140,8 +140,9 @@ pub struct ZPub { /// See [`crate::local_bus`]. local_channel: Arc, /// When set, `publish_shared` delivers only to same-session subscribers and - /// never touches zenoh. Prototype stand-in for asking the graph whether any - /// remote subscriber exists. + /// never touches zenoh. The caller's assertion that this publisher has no + /// off-session audience — not inferred, because the graph cannot see a + /// plain zenoh subscriber. intra_process_only: bool, /// The locality restriction applied to this publisher's wire half, if any. /// @@ -308,9 +309,11 @@ impl ZPubBuilder { /// /// # Why the flag exists, and why it should not /// - /// The real rule is "use the wire only while a remote subscriber exists", - /// which a production version reads off the graph per message. This - /// prototype is *told* instead. A publisher with this set is invisible to + /// The caller asserts the audience; it is not inferred. Reading it off the +/// ROS graph was tried and withdrawn: a plain zenoh subscriber declares no +/// liveliness token, so the graph cannot see it, and a publisher that +/// concluded "everyone is local" from the graph dropped that subscriber's +/// messages with nothing reporting the loss. /// every other process, including `ros2 topic echo`, and to any local /// subscriber that did not register on the bus with /// [`ZSubBuilder::build_with_shared_callback`]. Set it only where both ends @@ -1246,6 +1249,16 @@ where let callback = Arc::new(callback); let wire_half = callback.clone(); let mut sub = self.build_with_callback(move |m: T| wire_half(m))?; + // A `Locality::Remote` subscriber has asked not to see same-session + // traffic, and the bus is same-session by definition. Registering it + // here would deliver exactly what its own `allowed_origin` filter + // excludes — the wire half honours the restriction while the bus half + // silently ignored it. Skip the bus registration; the wire half keeps + // serving the remote traffic this subscriber asked for. + if self.locality == Some(zenoh::sample::Locality::Remote) { + return Ok(sub); + } + let topic = sub.entity.topic.clone(); let channel = crate::local_bus::channel(zid, &topic); sub._local_sub = Some(crate::local_bus::subscribe_owned::( @@ -1291,6 +1304,16 @@ where // build_with_callback qualified the topic, so read it back rather than // re-deriving it — the publisher keys the bus on its own qualified topic // and the two must agree exactly. + // A `Locality::Remote` subscriber has asked not to see same-session + // traffic, and the bus is same-session by definition. Registering it + // here would deliver exactly what its own `allowed_origin` filter + // excludes — the wire half honours the restriction while the bus half + // silently ignored it. Skip the bus registration; the wire half keeps + // serving the remote traffic this subscriber asked for. + if self.locality == Some(zenoh::sample::Locality::Remote) { + return Ok(sub); + } + let topic = sub.entity.topic.clone(); let channel = crate::local_bus::channel(zid, &topic); sub._local_sub = Some(crate::local_bus::subscribe::( diff --git a/crates/hiroz/src/reentrancy.rs b/crates/hiroz/src/reentrancy.rs index 1ea88f21b..11977b48f 100644 --- a/crates/hiroz/src/reentrancy.rs +++ b/crates/hiroz/src/reentrancy.rs @@ -87,14 +87,31 @@ pub fn live_guards() -> usize { } } -/// The opening words of a re-entrancy violation panic. +/// The payload of a re-entrancy violation panic. /// -/// [`local_bus::invoke_isolated`](crate::local_bus) contains subscriber panics -/// so one bad callback cannot censor its siblings. That containment must not -/// swallow *this* panic: it is the crate reporting its own contract violation, -/// not a user fault, and a nested delivery raises it from inside that -/// `catch_unwind`. The prefix is what lets the two be told apart. -pub const REENTRANCY_VIOLATION: &str = "hiroz re-entrancy rule violated"; +/// [`local_bus`](crate::local_bus) contains subscriber panics so one bad +/// callback cannot censor its siblings. That containment must not swallow +/// *this* panic: it is the crate reporting its own contract violation, and a +/// nested delivery raises it from inside that `catch_unwind`. +/// +/// It is a type rather than a message prefix because the alternative is +/// forgeable: a subscriber that panicked with a `String` beginning with the +/// prefix would be re-raised as though it were a violation, breaking the very +/// isolation guarantee the containment exists to provide. The private field +/// means no code outside this crate can construct one. +#[derive(Debug)] +pub struct ReentrancyViolation { + /// The operator-facing description. + pub message: String, + _private: (), +} + +impl std::fmt::Display for ReentrancyViolation { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(&self.message) + } +} + /// Panics (debug only) if any tracked guard is live on this thread. /// @@ -106,15 +123,18 @@ pub fn assert_no_guards_held(site: &str) { #[cfg(debug_assertions)] { let live = live_guards(); - assert!( - live == 0, - "{} at `{site}`: about to invoke a user \ - callback with {live} lock guard(s) live on this thread. A callback \ - that re-enters hiroz will deadlock if it touches a lock this thread \ - holds. Fix: collect what you need into an owned value, drop every \ - guard, then invoke the callback.", - REENTRANCY_VIOLATION, - ); + if live != 0 { + std::panic::panic_any(ReentrancyViolation { + message: format!( + "hiroz re-entrancy rule violated at `{site}`: about to invoke a user \ + callback with {live} lock guard(s) live on this thread. A callback \ + that re-enters hiroz will deadlock if it touches a lock this thread \ + holds. Fix: collect what you need into an owned value, drop every \ + guard, then invoke the callback." + ), + _private: (), + }); + } } #[cfg(not(debug_assertions))] let _ = site; From 954df5bade38c983abd05390145b7e16323fad5a Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Wed, 2 Sep 2026 16:13:20 +0800 Subject: [PATCH 32/37] fix: capture the subscriber locality before self is consumed The Remote-subscriber guard read self.locality after build_with_callback had taken self by value. Also strengthens three tests Copilot flagged as unable to detect what they were named for: the ordering test now induces a real wire failure by closing the session and asserts no local subscriber ran, the registry test is serialised and uses saturating arithmetic against a process-global counter, and the two panic tests are serialised because the panic hook is process-global. --- crates/hiroz-tests/tests/intra_process.rs | 64 +++++++++++++++++------ crates/hiroz/src/pubsub.rs | 8 ++- 2 files changed, 53 insertions(+), 19 deletions(-) diff --git a/crates/hiroz-tests/tests/intra_process.rs b/crates/hiroz-tests/tests/intra_process.rs index 4ad2f28aa..d9f547d06 100644 --- a/crates/hiroz-tests/tests/intra_process.rs +++ b/crates/hiroz-tests/tests/intra_process.rs @@ -887,6 +887,7 @@ fn transient_local_plus_intra_process_only_is_refused_for_owned_too() -> hiroz:: /// The wire path gives this isolation for free — one panicking callback kills /// one zenoh task. This test exists so the bus does not regress against it. #[test] +#[serial_test::serial(panic_hook)] fn a_panicking_subscriber_does_not_stop_delivery_to_the_others() -> hiroz::Result<()> { let router = TestRouter::new(); let ctx = create_hiroz_context_with_router(&router)?; @@ -958,6 +959,7 @@ fn a_panicking_subscriber_does_not_stop_delivery_to_the_others() -> hiroz::Resul /// from the fan-out branch, and a panic there has nowhere else to go: it would /// unwind straight out of `publish_shared` into the caller. #[test] +#[serial_test::serial(panic_hook)] fn a_panicking_sole_subscriber_does_not_reach_the_publisher() -> hiroz::Result<()> { let router = TestRouter::new(); let ctx = create_hiroz_context_with_router(&router)?; @@ -1346,43 +1348,66 @@ fn publish_owned_refuses_when_two_owned_subscribers_cannot_be_served() -> hiroz: Ok(()) } -/// Reordering `publish_shared`'s `BusAndWire` arm to publish on the wire first -/// must not stop it serving the bus. The ordering itself exists so that a wire -/// failure returns `Err` before any local subscriber has run — otherwise a -/// caller that retries delivers twice locally, and `Result` has no -/// partial-success value with which to warn them. +/// A wire failure must return **before** any local subscriber has run. +/// +/// The previous version of this test published successfully and asserted both +/// routes ran. That passes equally well with the bus first, so it did not +/// protect the property it was named for. The property is only observable in +/// the failure case: bus delivery is synchronous, so if the bus ran first, a +/// caller that retries after an `Err` delivers to every local subscriber twice +/// — and `Result` has no partial-success value to warn them. +/// +/// The session is closed to make the wire fail deterministically. #[test] -fn a_remote_publisher_serves_the_bus_after_the_wire() -> hiroz::Result<()> { +fn a_wire_failure_returns_before_any_local_subscriber_runs() -> hiroz::Result<()> { let router = TestRouter::new(); let ctx = create_hiroz_context_with_router(&router)?; - let node = ctx.create_node("order_near").build()?; + let node = ctx.create_node("order_fail").build()?; let near = Arc::new(AtomicUsize::new(0)); let n = near.clone(); let _sub = node - .create_sub::("order_topic") + .create_sub::("order_fail_topic") .build_with_shared_callback(move |_m: Arc| { n.fetch_add(1, Ordering::SeqCst); })?; let publisher = node - .create_pub::("order_topic") + .create_pub::("order_fail_topic") .with_locality(zenoh::sample::Locality::Remote) .build()?; wait_for_ready(Duration::from_millis(300)); - let outcome = publisher.publish_shared(Arc::new(RosString { - data: "both routes".to_owned(), + // The control: while the session is alive, both routes run. Without this a + // broken setup would satisfy the assertion below for the wrong reason. + let ok = publisher.publish_shared(Arc::new(RosString { + data: "healthy".to_owned(), }))?; - assert!( - matches!(outcome, Published::BusAndWire(Delivery::Sent(1))), - "the bus half must still run, and after the wire. Got {outcome:?}" + matches!(ok, Published::BusAndWire(Delivery::Sent(1))), + "control: a healthy publish must run both routes, got {ok:?}" ); assert_eq!( near.load(Ordering::SeqCst), 1, - "the same-session subscriber must still be served exactly once" + "control: the bus must deliver" + ); + + ctx.shutdown()?; + let after_control = near.load(Ordering::SeqCst); + + let failed = publisher.publish_shared(Arc::new(RosString { + data: "the wire is gone".to_owned(), + })); + assert!( + failed.is_err(), + "the session is closed, so the wire half must fail; got {failed:?}" + ); + assert_eq!( + near.load(Ordering::SeqCst), + after_control, + "the wire failed, so no local subscriber may have run. Running the bus \ + first and failing afterwards leaves the caller unable to retry safely." ); Ok(()) } @@ -1397,11 +1422,14 @@ fn a_remote_publisher_serves_the_bus_after_the_wire() -> hiroz::Result<()> { /// proves no `ZPub` or `ZSub` can still reach the channel, so a later builder /// cannot be split from a live endpoint. #[test] +#[serial_test::serial(registry)] fn a_channel_no_endpoint_holds_is_reclaimed() -> hiroz::Result<()> { let router = TestRouter::new(); let ctx = create_hiroz_context_with_router(&router)?; let node = ctx.create_node("reclaim").build()?; + // Serialised: total_channels() is process-global and sibling tests create + // channels of their own, so an unsynchronised delta is not attributable. let before = hiroz::local_bus::total_channels(); for i in 0..24 { let p = node @@ -1410,11 +1438,13 @@ fn a_channel_no_endpoint_holds_is_reclaimed() -> hiroz::Result<()> { drop(p); } let after_ephemeral = hiroz::local_bus::total_channels(); + // saturating: reclamation triggered by another test can legitimately make + // the total fall, and an unsigned underflow would panic on a healthy run. assert!( - after_ephemeral - before <= 4, + after_ephemeral.saturating_sub(before) <= 4, "24 publishers were created and dropped; the registry grew by {} channels. \ It should reclaim the ones no endpoint holds.", - after_ephemeral - before + after_ephemeral.saturating_sub(before) ); // The control. Without it this test passes equally well against a registry diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index f56104192..7ae5d4958 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -1248,6 +1248,8 @@ where // restricted, in which case the two audiences are disjoint. let callback = Arc::new(callback); let wire_half = callback.clone(); + // Read before `self` is consumed below. + let bus_is_excluded = self.locality == Some(zenoh::sample::Locality::Remote); let mut sub = self.build_with_callback(move |m: T| wire_half(m))?; // A `Locality::Remote` subscriber has asked not to see same-session // traffic, and the bus is same-session by definition. Registering it @@ -1255,7 +1257,7 @@ where // excludes — the wire half honours the restriction while the bus half // silently ignored it. Skip the bus registration; the wire half keeps // serving the remote traffic this subscriber asked for. - if self.locality == Some(zenoh::sample::Locality::Remote) { + if bus_is_excluded { return Ok(sub); } @@ -1299,6 +1301,8 @@ where let callback = Arc::new(callback); let wire_cb = callback.clone(); + // Read before `self` is consumed below. + let bus_is_excluded = self.locality == Some(zenoh::sample::Locality::Remote); let mut sub = self.build_with_callback(move |msg: T| (*wire_cb)(Arc::new(msg)))?; // build_with_callback qualified the topic, so read it back rather than @@ -1310,7 +1314,7 @@ where // excludes — the wire half honours the restriction while the bus half // silently ignored it. Skip the bus registration; the wire half keeps // serving the remote traffic this subscriber asked for. - if self.locality == Some(zenoh::sample::Locality::Remote) { + if bus_is_excluded { return Ok(sub); } From fa5f15b361e2dfcbf099c55dadcebf9081e895d7 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Wed, 2 Sep 2026 16:28:29 +0800 Subject: [PATCH 33/37] fix(reentrancy): use non_exhaustive rather than a private unit field clippy::manual_non_exhaustive, and the attribute is the better spelling: it blocks external construction the same way, without a field that reads as an accident. --- crates/hiroz/src/reentrancy.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/hiroz/src/reentrancy.rs b/crates/hiroz/src/reentrancy.rs index 11977b48f..88beed4fc 100644 --- a/crates/hiroz/src/reentrancy.rs +++ b/crates/hiroz/src/reentrancy.rs @@ -100,10 +100,10 @@ pub fn live_guards() -> usize { /// isolation guarantee the containment exists to provide. The private field /// means no code outside this crate can construct one. #[derive(Debug)] +#[non_exhaustive] pub struct ReentrancyViolation { /// The operator-facing description. pub message: String, - _private: (), } impl std::fmt::Display for ReentrancyViolation { @@ -132,7 +132,6 @@ pub fn assert_no_guards_held(site: &str) { holds. Fix: collect what you need into an owned value, drop every \ guard, then invoke the callback." ), - _private: (), }); } } From c1a93036a8a39e4c10cc284605f43b007e59ad45 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Wed, 2 Sep 2026 16:39:03 +0800 Subject: [PATCH 34/37] test(reentrancy): assert the violation by type, not by message should_panic can only match a string payload, so the typed payload broke it. Matching the message is also what made the old classifier forgeable - the type is both stronger and what local_bus keys on. --- crates/hiroz/src/reentrancy.rs | 35 ++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/crates/hiroz/src/reentrancy.rs b/crates/hiroz/src/reentrancy.rs index 88beed4fc..d8f78635f 100644 --- a/crates/hiroz/src/reentrancy.rs +++ b/crates/hiroz/src/reentrancy.rs @@ -296,15 +296,38 @@ mod tests { } /// A tripwire that never fires is indistinguishable from a clean codebase. + /// + /// The payload is asserted by **type**, not by message. `should_panic` can + /// only match a string, and matching the message is exactly what made the + /// old classifier forgeable: a subscriber panicking with the same words + /// would have been mistaken for a violation. Testing the type is both + /// stronger and the thing the containment in `local_bus` actually keys on. #[test] - #[cfg_attr(debug_assertions, should_panic(expected = "re-entrancy rule violated"))] fn assert_fires_while_a_guard_is_live() { - let m = TrackedMutex::new(0u32); - let _g = m.lock().unwrap(); - assert_no_guards_held("deliberate violation"); - // Compiled out in release, so no panic is expected there. + let fired = std::panic::catch_unwind(|| { + let m = TrackedMutex::new(0u32); + let _g = m.lock().unwrap(); + assert_no_guards_held("deliberate violation"); + }); + + #[cfg(debug_assertions)] + { + let payload = fired.expect_err("the tripwire did not fire while a guard was live"); + let violation = payload + .downcast_ref::() + .expect("the panic must carry ReentrancyViolation, not a bare string"); + assert!( + violation.message.contains("deliberate violation"), + "the site must reach the operator: {}", + violation.message + ); + } + // Compiled out in release, so nothing panics there. #[cfg(not(debug_assertions))] - assert_eq!(live_guards(), 0); + { + assert!(fired.is_ok(), "the check is debug-only"); + assert_eq!(live_guards(), 0); + } } /// The underflow assertion needs the same proof the tripwire gets. Only this From 95508f53503437097f8387af5823d6e1ee7b6952 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Wed, 2 Sep 2026 17:23:21 +0800 Subject: [PATCH 35/37] style(test): satisfy clippy on the hiroz-tests crate Three lints CI caught and my gate could not: collapsible_if twice and type_complexity on the erased republish hook. The gap is that I linted 'cargo clippy -p hiroz --all-targets', which covers hiroz's own targets and not the hiroz-tests crate at all - the documented default-members blind spot. The gate now lints both. --- crates/hiroz-tests/tests/intra_process.rs | 26 ++++++++++++++--------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/crates/hiroz-tests/tests/intra_process.rs b/crates/hiroz-tests/tests/intra_process.rs index d9f547d06..54a852998 100644 --- a/crates/hiroz-tests/tests/intra_process.rs +++ b/crates/hiroz-tests/tests/intra_process.rs @@ -43,6 +43,13 @@ use hiroz::{ }; use hiroz_msgs::std_msgs::{Int32, String as RosString}; +/// The erased republish hook the depth tests install. +/// +/// Named so the signature does not have to be spelled inline: `ZPub` carries a +/// serializer parameter, and writing it out at the binding is what tripped +/// `clippy::type_complexity`. +type EchoPublisher = Box hiroz::Result + Send + Sync>; + const DELIVERY_DEADLINE: Duration = Duration::from_secs(5); /// Ceiling on the self-publishing echo, so neither direction of the @@ -628,10 +635,10 @@ fn a_self_publishing_callback_does_not_escape_to_the_wire_and_loop() -> hiroz::R // forever. Without this the unbounded case never terminates, and a // detector that hangs tells you less than one that fails: the // timeout does not say which property broke. - if seen < ECHO_CAP { - if let Some(publish) = echo_cb.get() { - publish(m); - } + if seen < ECHO_CAP + && let Some(publish) = echo_cb.get() + { + publish(m); } })?; @@ -1159,8 +1166,7 @@ fn a_depth_refusal_is_reported_as_dropped_not_delivered() -> hiroz::Result<()> { let node = ctx.create_node("depth_outcome").build()?; let outcomes: Arc>> = Arc::new(Mutex::new(Vec::new())); - let publisher: Arc hiroz::Result + Send + Sync>>> = - Arc::new(OnceLock::new()); + let publisher: Arc> = Arc::new(OnceLock::new()); let p = publisher.clone(); let o = outcomes.clone(); @@ -1168,10 +1174,10 @@ fn a_depth_refusal_is_reported_as_dropped_not_delivered() -> hiroz::Result<()> { .create_sub::("depth_outcome") .build_with_owned_callback(move |m: RosString| { // Republish onto the same topic: each delivery nests one deeper. - if let Some(send) = p.get() { - if let Ok(out) = send(m) { - o.lock().expect("outcome lock").push(out); - } + if let Some(send) = p.get() + && let Ok(out) = send(m) + { + o.lock().expect("outcome lock").push(out); } })?; From 6e2e5e1528fda90695c13a4020273d730e9db4be Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Wed, 2 Sep 2026 17:26:00 +0800 Subject: [PATCH 36/37] docs: drop internal tracker references from shipped source These pointed at an issue tracker that is not this repository, so on a public remote they name nothing a reader can follow. Where the sentence carried meaning beyond the pointer it is kept; where the pointer was the sentence it is removed. --- crates/hiroz/src/local_bus.rs | 9 ++++----- crates/hiroz/src/pubsub.rs | 16 ++++++++-------- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/crates/hiroz/src/local_bus.rs b/crates/hiroz/src/local_bus.rs index 255c01c04..83c76fb1a 100644 --- a/crates/hiroz/src/local_bus.rs +++ b/crates/hiroz/src/local_bus.rs @@ -35,7 +35,6 @@ //! the bus on a wrong inference loses that subscriber's messages silently. So //! the caller says, with `with_intra_process_only()` or a `Locality`, and the //! bus is taken only when that assertion makes the wire redundant. -//! See circle/hiroz-bench#36 for the bus, and #134 for the withdrawal. //! //! # Keying, and why a publisher resolves it once //! @@ -88,7 +87,7 @@ use zenoh::session::ZenohId; /// /// Eight is chosen to be far above any legitimate chain — a pipeline of eight /// nodes each publishing from the previous one's callback, on one thread — and -/// far below the depth at which the stack is in danger. See issue circle/hiroz-bench#40. +/// far below the depth at which the stack is in danger. /// Public so a test can assert the exact bound rather than a loose ceiling: /// a hand-copied constant lets a change to this value slip past unnoticed. /// @@ -143,7 +142,7 @@ type LocalCallback = Arc; /// The shared path hands every receiver the same read-only `Arc`, which is /// right when several of them want it and wrong when exactly one does: a sole /// receiver could have been given the value itself, free to mutate or consume -/// it. This is that path. See issue circle/hiroz-bench#36. +/// it. This is that path. type OwnedCallback = Arc) + Send + Sync>; #[derive(Clone)] @@ -204,7 +203,7 @@ pub struct Channel { /// `NoTaker` and `DepthExceeded` both mean nothing was delivered, and they must /// not be conflated: a caller may fall back to the wire on `NoTaker`, but doing /// so on `DepthExceeded` re-enters the same callback on a zenoh thread with a -/// fresh depth counter and loops forever. See circle/hiroz-bench#36. +/// fresh depth counter and loops forever. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Delivery { /// Handed to this many subscribers. @@ -317,7 +316,7 @@ impl Channel { // Refuse to recurse without bound. A callback that publishes back onto // its own topic would otherwise overflow the stack; returning here turns // that into a dropped message and a loud log, which is recoverable and - // greppable. See issue circle/hiroz-bench#40. + // greppable. let depth = DELIVERY_DEPTH.with(|d| d.get()); if depth >= MAX_DELIVERY_DEPTH { tracing::error!( diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index 7ae5d4958..3b04dedcf 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -317,7 +317,7 @@ impl ZPubBuilder { /// every other process, including `ros2 topic echo`, and to any local /// subscriber that did not register on the bus with /// [`ZSubBuilder::build_with_shared_callback`]. Set it only where both ends - /// are known. See issue circle/hiroz-bench#36. + /// are known. /// # This publisher still appears in the ROS graph /// /// The liveliness token is declared in `build()` before any locality is @@ -722,7 +722,7 @@ where /// /// `Bus(NoTaker)` is a distinct outcome rather than a silent drop: the /// caller asserted there was a local audience and there was none, which is - /// worth acting on. See [`crate::local_bus`] and circle/hiroz-bench#36. + /// worth acting on. See [`crate::local_bus`]. /// /// # Ordering against [`publish`](Self::publish) /// @@ -786,7 +786,7 @@ where } // DepthExceeded is kept distinct from "nothing delivered": a // caller may fall back to the wire on NoTaker and must not on - // DepthExceeded. See circle/hiroz-bench#36. + // DepthExceeded. Ok(Published::Bus(d)) } // Both audiences, and no subscriber is on both. TRANSIENT_LOCAL is @@ -872,7 +872,7 @@ where /// `intra_process_only` publisher with no listener reports /// `Bus(Delivery::NoTaker)` and the message is dropped; it does **not** /// fall through to the wire, because such a publisher has none. - /// See circle/hiroz-bench#36. + /// pub fn publish_owned(&self, msg: T) -> Result where T: Send + Sync + 'static, @@ -1230,7 +1230,7 @@ where /// shared or wire delivery, which this subscriber does not receive. /// /// Use it where the receiver wants to mutate or consume the message, which - /// the shared path cannot offer. See issue circle/hiroz-bench#36. + /// the shared path cannot offer. pub fn build_with_owned_callback(self, callback: F) -> Result> where T: Send + Sync + 'static, @@ -1241,7 +1241,7 @@ where // Both halves run the same callback. The wire half is not decoration: // without it this subscriber discards every message that does not // arrive by `publish_owned` on the bus — including from every remote - // publisher — while still advertising a live subscription. See circle/hiroz-bench#36. + // publisher — while still advertising a live subscription. // // It cannot deliver twice: a publisher takes the bus or the wire for // any one message, never both, unless its wire half is `Remote`- @@ -1286,11 +1286,11 @@ where /// unless its wire half is `Locality::Remote`, in which case the two /// audiences are disjoint and no subscriber sees it twice. Suppressing the /// duplicate is the publisher's job, because it is the only side that knows - /// which routes it used. See circle/hiroz-bench#39. + /// which routes it used. /// /// The type match is exact: a publisher of a different concrete Rust type /// on the same topic is not delivered here, even if the ROS type name - /// agrees. See [`crate::local_bus`] and issue circle/hiroz-bench#36. + /// agrees. See [`crate::local_bus`]. pub fn build_with_shared_callback(self, callback: F) -> Result> where T: Send + Sync + 'static, From 339735442b8dc20a9f1d10f772349066dacf967e Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Wed, 2 Sep 2026 17:38:29 +0800 Subject: [PATCH 37/37] style(test): name the subscriber type in publisher_locality The last clippy::type_complexity CI reported. Same cause as the one in intra_process: ZSub carries a serializer parameter, so spelling it at a return position trips the lint. --- crates/hiroz-tests/tests/publisher_locality.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/crates/hiroz-tests/tests/publisher_locality.rs b/crates/hiroz-tests/tests/publisher_locality.rs index c5e1ff308..abda0e21a 100644 --- a/crates/hiroz-tests/tests/publisher_locality.rs +++ b/crates/hiroz-tests/tests/publisher_locality.rs @@ -45,13 +45,16 @@ const MESSAGES: usize = 5; /// mistaken for a blocked one. const DELIVERY_DEADLINE: Duration = Duration::from_secs(5); +/// A string subscriber, spelled once so the signature below stays readable. +/// +/// `ZSub` carries a serializer parameter, so writing the full type at the +/// return position is what trips `clippy::type_complexity`. +type StringSub = hiroz::pubsub::ZSub>; + fn counting_sub( node: &hiroz::node::ZNode, topic: &str, -) -> hiroz::Result<( - hiroz::pubsub::ZSub>, - Arc, -)> { +) -> hiroz::Result<(StringSub, Arc)> { let count = Arc::new(AtomicUsize::new(0)); let c = count.clone(); let sub = node