From 549a5300a8cd716169f0a4dfed3a5491dcdd249c Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 28 Jul 2026 02:19:13 +0800 Subject: [PATCH 01/30] fix(pubsub): stop running subscriber callbacks on the publishing thread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Publishing from inside a subscriber callback on the same session deadlocked deterministically, with no error and no traceback. Three separate mechanisms had to be removed. 1. Every subscriber was declared as a zenoh-ext `AdvancedSubscriber`, unconditionally. Its sample callback takes a `std::sync::Mutex` and then invokes the user callback under that guard. `std::sync::Mutex` is not reentrant, so a callback that published into its own topic graph re-entered a mutex its own thread already held further up the stack. Declare the advanced subscriber only when the QoS profile actually configures advanced features: for Volatile — the ROS 2 default — an `AdvancedSubscriber` declares no liveliness subscriber, no heartbeat subscriber and no detection token, so it was pure per-sample overhead plus the fatal lock. 2. Zenoh delivers a same-session sample synchronously, inline on the thread that called `put`. With the lock gone, a callback that publishes therefore *recursed* instead of iterating, until the stack overflowed. Adopt the shape zenoh's own `FifoChannel` uses, and that zenoh-python installs by default for a Python callable: delivery enqueues and returns, user code runs elsewhere. `ZPub`'s four publish paths hold a thread-local marker across the zenoh `put`; the plain subscriber's shim enqueues when that marker is set and invokes inline when it is not. An inter-process sample arrives on a zenoh RX worker, which is never inside a hiroz publish, so that path keeps its inline call and pays one thread-local read. Queue-mode subscribers — every rmw subscription, and every `recv()`-based user — already enqueue and return, so they get no dispatcher at all. The marker keys on the publishing thread rather than on zenoh's `Locality` on purpose: two sessions in one process with a direct route deliver a `Locality::Remote` sample inline on the publisher's thread, so an `allowed_origin(SessionLocal)` split would miss that case. With no path left on which a callback is reachable from inside `put`, re-entrancy is structurally impossible rather than depth-bounded, so the interim `MAX_CALLBACK_REENTRY_DEPTH` cap, `CallbackDepthGuard` and `InheritedCallbackDepth` are removed rather than left as unreachable defensive code. 3. `hiroz-py`'s `ZPublisher.publish`/`publish_raw` release the GIL across the zenoh publish. This does not fix the deadlock, but it downgrades a whole-interpreter freeze — no exception, no traceback, only an external kill — to a single blocked thread, which is the difference between an undiagnosable hang and a diagnosable one. Queue policy is split per path. The plain dispatcher takes its capacity from the same history-QoS expression `build()` uses to size `BoundedQueue` and drops the oldest on overflow; the advanced dispatcher stays unbounded. Bounded and blocking recreates the deadlock on both paths — the blocked producer sits inside the callback holding the very lock the drain thread needs. Bounded and dropping is correct for the plain path, which is Volatile with KEEP_LAST(depth) and already promises no more than `depth` undelivered samples, and wrong for the advanced path, where loss would discard samples miss-detection went out of its way to recover, mid-reorder. Also adds `ZSubBuilder::build_with_sample_callback`, which hands the callback the `Sample` rather than a decoded message, and uses it in `hiroz-py`. The Python callback path routed every sample through an identity codec whose `Output` carries no lifetime, so it had to `to_vec()` the whole payload before the callback ran, only for msgspec to decode out of it and drop it — one full payload copy per message. Measured as an interleaved paired A/B (two release wheels differing only in this call site, 5+6 reps, 16k timed round trips each, half-trip p50): 64 B 114.5 -> 114.1 (noise), 4 KB 120.9 -> 121.0 (noise), 64 KB 224.4 -> 221.1, -3.4 us with 10 of 11 reps in [-2.6, -5.1]. The size-dependence is the point: it is what distinguishes removing a payload-sized memcpy from removing a fixed per-message cost. Wire format is unchanged: an `AdvancedPublisher` with no cache, no publisher_detection and no sample_miss_detection puts on the plain key expression, so Volatile publishers and subscribers stay byte-identical and interop is unaffected. Volatile subscribers no longer get zenoh-ext's HLC-timestamp de-duplication. Ordering is preserved where it was ever guaranteed: one FIFO queue, one drain thread, and drop-oldest preserves the relative order of what survives. Not preserved, and documented at the type: a plain subscriber receiving both local and remote publications on one topic now runs them on two different threads, so their relative order is not guaranteed. Neither ROS 2 nor zenoh guarantees ordering across publishers, and a plain zenoh subscriber could already be invoked concurrently from several RX workers. Detector evidence, both directions. Reverting the dispatch decision (`local_publish_active() -> false`) and keeping the tests, all three self-feeding loop tests die with `fatal runtime error: stack overflow, aborting` (rc=101), including `intra_closed_loop_runs_iteratively`, which drives 2_000 round trips through a two-topic one-session callback cycle that could not previously be expressed as a loop. Against the unfixed sources the original three scenarios fail on their 20s deadline (`0 passed; 3 failed`). Every one of the six Python cells hangs with rc=124 under an external wall-clock timeout on a wheel built from unfixed sources — not merely fails, hangs — and all six pass on this branch. --- crates/hiroz-py/src/node.rs | 24 +- crates/hiroz-py/src/pubsub.rs | 22 +- .../hiroz-py/tests/test_reentrant_publish.py | 435 ++++++++++ .../tests/dispatch_backpressure.rs | 258 ++++++ crates/hiroz-tests/tests/reentrant_publish.rs | 796 ++++++++++++++++++ crates/hiroz/src/common.rs | 14 + crates/hiroz/src/ffi/subscriber.rs | 5 +- crates/hiroz/src/node.rs | 52 +- crates/hiroz/src/pubsub.rs | 624 +++++++++++++- 9 files changed, 2190 insertions(+), 40 deletions(-) create mode 100644 crates/hiroz-py/tests/test_reentrant_publish.py create mode 100644 crates/hiroz-tests/tests/dispatch_backpressure.rs create mode 100644 crates/hiroz-tests/tests/reentrant_publish.rs diff --git a/crates/hiroz-py/src/node.rs b/crates/hiroz-py/src/node.rs index 6e43eb4dc..e186c6901 100644 --- a/crates/hiroz-py/src/node.rs +++ b/crates/hiroz-py/src/node.rs @@ -16,6 +16,7 @@ use hiroz::node::ZNode; use pyo3::prelude::*; use std::any::Any; use std::sync::Arc; +use zenoh_buffers::buffer::SplitBuffer; /// Try to extract type info from a message class. /// @@ -233,9 +234,25 @@ impl PyZNode { // matching rmw_zenoh_cpp's NodeData::subs_ pattern. The caller does not // need to assign the returned PyZSubscriber to keep the subscription active. let type_name = msg_type_str.clone(); + // Sample-level callback, not `build_with_callback`. The typed form + // would route through `RawBytesCdrSerdes::deserialize`, whose + // `Output` is an owned `RawBytesMessage` and so must `to_vec()` the + // whole payload before this closure runs — a full copy per message, + // scaling with payload size, immediately discarded once msgspec has + // decoded it. Taking the `Sample` lets the decode read straight out + // of the network buffer, and matches what the polling `recv()` path + // in `pubsub.rs` already does. let zsub = sub_builder - .build_with_callback(move |raw_msg: RawBytesMessage| { - let payload = raw_msg.0; + .build_with_sample_callback(move |sample| { + // Same zero-copy setup as `PyZSubscriber::recv`: the ZBuf is + // cheap Arc clones, and publishing it as the deserializer's + // source lets `bytes`-typed fields become sub-ZSlices of the + // received buffer instead of copies. + let payload_zbuf: zenoh_buffers::ZBuf = sample.payload().clone().into(); + hiroz_cdr::ZBUF_DESER_SOURCE.with(|cell| { + *cell.borrow_mut() = Some(payload_zbuf.clone()); + }); + let payload = payload_zbuf.contiguous(); Python::with_gil(|py| { match hiroz_msgs::deserialize_from_cdr(&type_name, py, &payload) { Ok(obj) => { @@ -248,6 +265,9 @@ impl PyZNode { } } }); + hiroz_cdr::ZBUF_DESER_SOURCE.with(|cell| { + *cell.borrow_mut() = None; + }); }) .map_err(|e| e.into_pyerr())?; diff --git a/crates/hiroz-py/src/pubsub.rs b/crates/hiroz-py/src/pubsub.rs index 16fada65e..9ff7e4c65 100644 --- a/crates/hiroz-py/src/pubsub.rs +++ b/crates/hiroz-py/src/pubsub.rs @@ -24,20 +24,30 @@ impl PyZPublisher { /// Publish a message /// /// Serializes the Python message (msgspec.Struct) to ZBuf and publishes (zero-copy path) - unsafe fn publish(&self, _py: Python, data: &Bound<'_, PyAny>) -> PyResult<()> { - // Serialize Python message directly to ZBuf (zero-copy) + unsafe fn publish(&self, py: Python, data: &Bound<'_, PyAny>) -> PyResult<()> { + // Serialize Python message directly to ZBuf (zero-copy). This touches + // Python objects, so it must run with the GIL held. let zbuf = hiroz_msgs::serialize_to_zbuf(&self.type_name, data)?; - // Publish the ZBuf directly - self.inner.publish(zbuf.into()).map_err(|e| e.into_pyerr()) + // Release the GIL for the publish itself. Zenoh delivers samples to + // local subscribers synchronously on the publishing thread, so a publish + // issued from inside a subscriber callback can block here; holding the + // GIL across it would freeze the whole interpreter — no exception, no + // traceback — instead of blocking just this thread. + py.allow_threads(|| self.inner.publish(zbuf.into())) + .map_err(|e| e.into_pyerr()) } /// Publish pre-serialized CDR bytes directly /// /// Use this for zero-copy forwarding of received messages (e.g., in a pong responder). /// The bytes should be in CDR format (as returned by recv_serialized/try_recv_serialized). - fn publish_raw(&self, data: &[u8]) -> PyResult<()> { - self.inner.publish(data.into()).map_err(|e| e.into_pyerr()) + fn publish_raw(&self, py: Python, data: &[u8]) -> PyResult<()> { + // Copy out of the Python buffer before dropping the GIL, then publish + // without it — see `publish` for why. + let payload: zenoh::bytes::ZBytes = data.into(); + py.allow_threads(|| self.inner.publish(payload)) + .map_err(|e| e.into_pyerr()) } /// Get the topic name (for debugging) diff --git a/crates/hiroz-py/tests/test_reentrant_publish.py b/crates/hiroz-py/tests/test_reentrant_publish.py new file mode 100644 index 000000000..18c909fbb --- /dev/null +++ b/crates/hiroz-py/tests/test_reentrant_publish.py @@ -0,0 +1,435 @@ +#!/usr/bin/env python3 +"""Publishing from inside a subscriber callback must not freeze the interpreter. + +Two independent defects combined here: + +1. hiroz declared every subscriber as a zenoh-ext ``AdvancedSubscriber``, which + runs the user callback while holding a non-reentrant ``std::sync::Mutex``. + Zenoh delivers same-session samples synchronously on the publishing thread, so + a publish from inside a callback re-entered that mutex and deadlocked. +2. ``ZPublisher.publish`` held the GIL across the blocking publish, so the + deadlock froze the *whole* interpreter -- no exception, no traceback, only an + external kill. + +Defect 1 is fixed differently per durability, and both paths are covered here: + +* ``Volatile`` (the ROS 2 default) gets a plain zenoh subscriber. Samples that + arrived over a transport run inline on the zenoh RX worker; samples published + by the delivering thread itself are handed to a per-subscriber + ``hiroz-sub-drain`` thread. That second case is what a re-entrant publish + hits, so it *iterates* rather than recursing and needs no depth cap. +* ``TransientLocal`` keeps the advanced subscriber -- its reordering guarantees + need that mutex -- so *every* sample moves off the delivering thread onto the + drain thread. + +Either way the drain thread is a *Rust* thread and is therefore invisible to +``threading.enumerate()``; the leak test below reads ``/proc/self/task`` +instead, which is the only view Python has of it. + +Everything here is deadline-guarded so a regression fails the suite rather than +wedging it. That guard is itself only meaningful because of fix 2 -- with the GIL +held across the block, no watchdog and no deadline thread can run at all, which +is what ``test_interpreter_stays_alive_during_reentrant_publish`` pins down. +""" + +import gc +import itertools +import os +import threading +import time + +import hiroz_py +import pytest +from hiroz_py import std_msgs + +# Generous relative to the work done (a handful of intra-process publishes). +SCENARIO_TIMEOUT = 20.0 +HOPS = 4 + +# How far the self-feeding loop must run to prove it iterates rather than +# recurses. Two orders of magnitude past the depth cap of 16 that hiroz used to +# need, and well past the stack depth a recursive implementation survives. +ITERATION_TARGET = 2000 + +DRAIN_THREAD_NAME = "hiroz-sub-drain" + +DURABILITIES = ["volatile", "transient_local"] + +_topic_seq = itertools.count() + + +def _topic(stem): + """A fresh topic per scenario. + + TransientLocal replays history to late-joining subscribers, so reusing a + topic across scenarios would leak samples from one into the next and make a + hop chain appear to complete that never actually ran. + """ + return f"/reentrant_py_{stem}_{next(_topic_seq)}" + + +def _qos(durability): + return hiroz_py.QosProfile(durability=durability) + + +def _run_on_deadline(fn, timeout=SCENARIO_TIMEOUT): + """Run ``fn`` on a daemon thread and assert it returned within ``timeout``. + + The seeding publish is what blocks in the unfixed build, so it is what has to + be time-boxed. A daemon thread means a genuine deadlock leaves a stuck thread + behind but still lets the process exit with a real failure -- provided the + GIL is released across the block, which is fix 2. + """ + error = [] + + def target(): + try: + fn() + except BaseException as exc: # noqa: BLE001 - re-raised below + error.append(exc) + + driver = threading.Thread(target=target, daemon=True) + driver.start() + driver.join(timeout) + assert not driver.is_alive(), ( + f"publish() from inside a subscriber callback did not return within " + f"{timeout}s - re-entrant publish deadlocked" + ) + if error: + raise error[0] + + +# -------------------------------------------------------------------------- +# Preconditions -- fail loudly rather than skipping silently. +# +# The sibling Rust suite (crates/hiroz-tests/tests/reentrant_service.rs) is +# gated behind ``#![cfg(feature = "ros-msgs")]`` and reports a green "0 passed" +# when the feature is off. Nothing here may be able to do that: if the +# environment cannot express a scenario, that is a failure, not a skip. +# -------------------------------------------------------------------------- + + +def test_preconditions(context): + """The APIs every scenario below depends on must exist and behave.""" + assert hasattr(hiroz_py, "QosProfile"), "hiroz_py.QosProfile is missing" + + for durability in DURABILITIES: + profile = _qos(durability) + assert durability in repr(profile).lower(), ( + f"QosProfile(durability={durability!r}) did not round-trip: {profile!r}" + ) + + node = context.create_node("reentrant_precond").with_namespace("/test").build() + assert hasattr(node, "destroy_subscriber"), ( + "ZNode.destroy_subscriber is missing - the drain-thread leak test cannot " + "drop subscribers and must not be reported as passing" + ) + + # The leak test can only see the Rust drain thread through procfs. + assert os.path.isdir("/proc/self/task"), ( + "/proc/self/task is unavailable - the drain-thread leak test cannot run " + "on this platform and must not be reported as passing" + ) + + +# -------------------------------------------------------------------------- +# The 6-cell matrix: {volatile, transient_local} +# x {same-topic, two-topic cycle, unbounded feedback}. +# -------------------------------------------------------------------------- + + +@pytest.mark.parametrize("durability", DURABILITIES) +def test_same_topic_republish(context, durability): + """A callback republishing to its own topic must complete the hop chain.""" + node = ( + context.create_node(f"reentrant_same_{durability}") + .with_namespace("/test") + .build() + ) + topic = _topic("same") + qos = _qos(durability) + publisher = node.create_publisher(topic, std_msgs.String, qos=qos) + + seen = [] + done = threading.Event() + + def on_message(msg): + seen.append(msg.data) + hop = int(msg.data) + if hop < HOPS: + # The re-entrant publish. Before the fix this never returned. + publisher.publish(std_msgs.String(data=str(hop + 1))) + else: + done.set() + + node.create_subscriber(topic, std_msgs.String, qos=qos, callback=on_message) + time.sleep(0.5) + + _run_on_deadline(lambda: publisher.publish(std_msgs.String(data="0"))) + + assert done.wait(SCENARIO_TIMEOUT), f"only got {seen!r}, expected {HOPS + 1} hops" + assert seen == [str(i) for i in range(HOPS + 1)], seen + + +@pytest.mark.parametrize("durability", DURABILITIES) +def test_two_topic_cycle(context, durability): + """A -> B -> A across two subscribers must not deadlock either. + + Distinct from the same-topic case: the re-entrant publish lands on a + *different* subscriber, so this exercises the delivering thread rather than + one subscriber's own state. + """ + node = ( + context.create_node(f"reentrant_cycle_{durability}") + .with_namespace("/test") + .build() + ) + topic_a = _topic("cycle_a") + topic_b = _topic("cycle_b") + qos = _qos(durability) + pub_a = node.create_publisher(topic_a, std_msgs.String, qos=qos) + pub_b = node.create_publisher(topic_b, std_msgs.String, qos=qos) + + seen = [] + done = threading.Event() + + def hop_handler(which, forward_to): + def handler(msg): + seen.append((which, msg.data)) + hop = int(msg.data) + if hop < HOPS: + forward_to.publish(std_msgs.String(data=str(hop + 1))) + else: + done.set() + + return handler + + node.create_subscriber( + topic_a, std_msgs.String, qos=qos, callback=hop_handler("a", pub_b) + ) + node.create_subscriber( + topic_b, std_msgs.String, qos=qos, callback=hop_handler("b", pub_a) + ) + time.sleep(0.5) + + _run_on_deadline(lambda: pub_a.publish(std_msgs.String(data="0"))) + + assert done.wait(SCENARIO_TIMEOUT), f"cycle stalled after {seen!r}" + assert len(seen) == HOPS + 1, seen + assert [d for _, d in seen] == [str(i) for i in range(HOPS + 1)], seen + assert [t for t, _ in seen] == ["a", "b", "a", "b", "a"][: HOPS + 1], seen + + +@pytest.mark.parametrize("durability", DURABILITIES) +def test_self_feeding_loop_iterates(context, durability): + """A callback that republishes every time must loop, not recurse. + + This is the Python-level view of the change that made the ``afor`` + benchmark's ``intra`` cell expressible. hiroz used to deliver a same-session + sample inline on the publishing thread, so this shape was recursion: it grew + the stack and had to be cut off at a depth cap of 16, dropping samples past + it. Delivery now enqueues and returns, so each hop starts from a flat stack + and the loop runs for as long as it is fed. + + The assertion is the inverse of the one it replaces: the loop must run far + *past* the old cap. + """ + node = ( + context.create_node(f"reentrant_loop_{durability}") + .with_namespace("/test") + .build() + ) + topic = _topic("loop") + qos = _qos(durability) + publisher = node.create_publisher(topic, std_msgs.String, qos=qos) + + counter = itertools.count() + delivered = [] + reached = threading.Event() + + def on_message(msg): + n = next(counter) + delivered.append(n) + if n >= ITERATION_TARGET: + # Stop feeding so the test ends on its own; the loop's *ability* to + # keep going is what is under test. + reached.set() + return + publisher.publish(std_msgs.String(data=str(n))) + + node.create_subscriber(topic, std_msgs.String, qos=qos, callback=on_message) + time.sleep(0.5) + + _run_on_deadline(lambda: publisher.publish(std_msgs.String(data="0"))) + + assert reached.wait(SCENARIO_TIMEOUT), ( + f"self-feeding loop stalled at {len(delivered)} deliveries, target " + f"{ITERATION_TARGET}. Under the old inline dispatch this caps out at 16." + ) + assert len(delivered) >= ITERATION_TARGET, len(delivered) + + +# -------------------------------------------------------------------------- +# Fix 2: the interpreter must stay alive. +# -------------------------------------------------------------------------- + + +def test_interpreter_stays_alive_during_reentrant_publish(context): + """A watchdog thread must keep running across a re-entrant publish. + + This is the user-facing half of the fix and the only part a Rust test cannot + check. ``ZPublisher.publish`` used to hold the GIL across the blocking zenoh + publish, so a deadlock there did not stall one thread -- it stalled every + thread. No exception, no traceback, no way to diagnose it from inside the + process. ``py.allow_threads`` downgrades that to a single blocked thread. + + Failure mode on the unfixed build: this test does not fail, it *hangs the + whole process*, because the assertions below also need the GIL. That is + precisely the symptom, and it is why the unfixed baseline has to be measured + under an external wall-clock timeout rather than trusted to self-report. + """ + node = context.create_node("reentrant_watchdog").with_namespace("/test").build() + topic = _topic("watchdog") + publisher = node.create_publisher(topic, std_msgs.String) + + ticks = [] + stop = threading.Event() + + def watchdog(): + while not stop.is_set(): + ticks.append(time.monotonic()) + time.sleep(0.02) + + def on_message(msg): + hop = int(msg.data) + if hop < HOPS: + publisher.publish(std_msgs.String(data=str(hop + 1))) + + node.create_subscriber(topic, std_msgs.String, callback=on_message) + time.sleep(0.5) + + wd = threading.Thread(target=watchdog, daemon=True) + wd.start() + time.sleep(0.2) + before = len(ticks) + assert before > 0, "watchdog never started - the detector is broken, not the code" + + _run_on_deadline(lambda: publisher.publish(std_msgs.String(data="0"))) + + time.sleep(0.3) + after = len(ticks) + stop.set() + wd.join(5.0) + + assert after > before, ( + "the watchdog thread made no progress across the re-entrant publish - " + "the GIL was held across the block and the whole interpreter went dark" + ) + + # No single stall longer than a generous multiple of the tick interval. + gaps = [b - a for a, b in zip(ticks, ticks[1:])] + worst = max(gaps) if gaps else 0.0 + assert worst < 2.0, ( + f"watchdog stalled for {worst:.2f}s during the publish - the interpreter " + "was frozen even if it eventually recovered" + ) + + +# -------------------------------------------------------------------------- +# The TransientLocal dispatcher thread must not leak. +# -------------------------------------------------------------------------- + + +def _drain_threads(): + """Count live ``hiroz-sub-drain`` threads via procfs. + + ``threading.enumerate()`` cannot see these: they are Rust threads spawned by + ``CallbackDispatcher::spawn`` and were never registered with CPython. + ``/proc/self/task/*/comm`` is the only way Python can observe them, so it is + the only honest detector for a leak. + """ + total = 0 + for tid in os.listdir("/proc/self/task"): + try: + with open(f"/proc/self/task/{tid}/comm") as handle: + if handle.read().strip() == DRAIN_THREAD_NAME: + total += 1 + except OSError: + continue # thread exited between listdir and open + return total + + +def _settle(deadline, target=None): + """Wait up to ``deadline`` for the drain count to reach ``target``.""" + gc.collect() + end = time.monotonic() + deadline + current = _drain_threads() + while time.monotonic() < end: + if target is not None and current == target: + return current + time.sleep(0.2) + current = _drain_threads() + return current + + +def test_transient_local_dispatcher_threads_do_not_leak(context): + """Create and destroy TransientLocal subscribers; the count must return. + + Volatile *callback* subscribers also carry a dispatcher now (for the + session-local delivery path), so the leak concern is no longer + TransientLocal-only -- but TransientLocal spawns one unconditionally, which + makes it the tighter test of the same teardown path. + """ + node = context.create_node("reentrant_leak").with_namespace("/test").build() + qos = _qos("transient_local") + + baseline_py = len(threading.enumerate()) + baseline_drain = _settle(deadline=3.0) + + subs_per_cycle = 4 + cycles = 3 + peaks = [] + + for cycle in range(cycles): + subs = [ + node.create_subscriber( + _topic(f"leak_{cycle}_{i}"), + std_msgs.String, + qos=qos, + callback=lambda _msg: None, + ) + for i in range(subs_per_cycle) + ] + time.sleep(0.5) + + peak = _drain_threads() + peaks.append(peak) + # Precondition: if the dispatcher never spawned, this test is not + # exercising the TransientLocal path and must fail rather than pass. + assert peak >= baseline_drain + subs_per_cycle, ( + f"cycle {cycle}: expected at least {subs_per_cycle} new " + f"{DRAIN_THREAD_NAME!r} threads over a baseline of {baseline_drain}, " + f"saw {peak}. The TransientLocal dispatcher path was not exercised - " + "this test proves nothing in this state." + ) + + for sub in subs: + node.destroy_subscriber(sub) + subs.clear() + + settled = _settle(deadline=15.0, target=baseline_drain) + assert settled == baseline_drain, ( + f"cycle {cycle}: {settled - baseline_drain} {DRAIN_THREAD_NAME!r} " + f"thread(s) leaked after destroying {subs_per_cycle} subscribers " + f"(baseline {baseline_drain}, peak {peak})" + ) + + # Repeated cycles must not ratchet upward. + assert max(peaks) - min(peaks) <= 1, ( + f"drain-thread peak drifted across cycles: {peaks} - threads accumulate" + ) + + final_py = len(threading.enumerate()) + assert final_py <= baseline_py, ( + f"Python-level threads grew from {baseline_py} to {final_py}" + ) diff --git a/crates/hiroz-tests/tests/dispatch_backpressure.rs b/crates/hiroz-tests/tests/dispatch_backpressure.rs new file mode 100644 index 000000000..4ce491f6c --- /dev/null +++ b/crates/hiroz-tests/tests/dispatch_backpressure.rs @@ -0,0 +1,258 @@ +//! The plain-path callback dispatcher must honour `KEEP_LAST(depth)`. +//! +//! A callback subscriber cannot run user code on the publishing thread — that is +//! what made re-entrant publishes recurse — so locally published samples are +//! handed to `CallbackDispatcher`'s queue and delivered on its own thread. That +//! queue is the subscriber's history buffer, and it must behave like the one the +//! queue-mode path uses (`BoundedQueue`): retain the last `depth` undelivered +//! samples and drop the *oldest* on overflow. +//! +//! Two properties are asserted, one per direction of the bound: +//! +//! * `KeepLast(n)` drops, and drops the oldest — an unbounded queue delivers the +//! *first* `n` after the burst instead of the last, so the assertion on +//! contents (not merely on count) fails if the bound is removed. +//! * `KeepAll` does not drop — the lossless branch is still reachable. +//! +//! Both are made deterministic by blocking the drain thread inside the first +//! callback until the whole burst has been published, so the race between +//! producer and drain thread is removed rather than slept on. + +mod common; + +use std::{ + num::NonZeroUsize, + sync::{ + Arc, Condvar, Mutex, + atomic::{AtomicBool, Ordering}, + mpsc, + }, + thread, + time::{Duration, Instant}, +}; + +use common::{TestRouter, create_hiroz_context_with_endpoint}; +use hiroz::{ + Builder, TypeHash, + qos::{QosHistory, QosProfile}, + ros_msg::MessageTypeInfo, +}; +use serde::{Deserialize, Serialize}; +use serial_test::serial; + +/// Budget for one scenario. Generous relative to the work done. +const SCENARIO_TIMEOUT: Duration = Duration::from_secs(30); + +/// History depth under test. Small enough that a 50-message burst overflows it +/// many times over, large enough that an off-by-one would be visible. +const DEPTH: usize = 4; + +/// Messages published after the drain thread is parked. Counter values are +/// `0..BURST`. +const BURST: u64 = 50; + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +struct Seq { + counter: u64, +} + +impl MessageTypeInfo for Seq { + fn type_name() -> &'static str { + "test_msgs::msg::dds_::Seq_" + } + + fn type_hash() -> TypeHash { + TypeHash::zero() + } +} + +impl hiroz::ros_msg::WithTypeInfo for Seq {} + +impl hiroz::msg::ZMessage for Seq { + type Serdes = hiroz::msg::SerdeCdrSerdes; +} + +/// Run `scenario` on its own thread and fail (rather than hang) if it does not +/// finish within [`SCENARIO_TIMEOUT`]. +fn run_with_deadline(name: &'static str, scenario: impl FnOnce() + Send + 'static) { + let (tx, rx) = mpsc::channel(); + thread::Builder::new() + .name(name.to_string()) + .spawn(move || { + scenario(); + let _ = tx.send(()); + }) + .expect("failed to spawn scenario thread"); + + if rx.recv_timeout(SCENARIO_TIMEOUT).is_err() { + panic!("`{name}` did not finish within {SCENARIO_TIMEOUT:?}"); + } +} + +/// A latch the first callback parks on until the publisher releases it. +#[derive(Default)] +struct Latch { + open: Mutex, + changed: Condvar, +} + +impl Latch { + fn wait(&self) { + let mut open = self.open.lock().expect("latch poisoned"); + while !*open { + open = self.changed.wait(open).expect("latch poisoned"); + } + } + + fn release(&self) { + *self.open.lock().expect("latch poisoned") = true; + self.changed.notify_all(); + } +} + +/// Publish `0..BURST` into a callback subscriber whose drain thread is parked on +/// the first message, then release it and return everything the callback saw. +/// +/// Returns once the delivered sequence has been quiet for 500 ms, so the caller +/// asserts on a settled result rather than on a snapshot mid-drain. +fn burst_through_dispatcher( + endpoint: &str, + node: &str, + topic: &str, + history: QosHistory, +) -> Vec { + let qos = QosProfile { + history, + ..Default::default() + }; + + let ctx = create_hiroz_context_with_endpoint(endpoint).expect("failed to create context"); + let node = ctx + .create_node(node) + .build() + .expect("failed to create node"); + + let publisher = node + .create_pub::(topic) + .with_qos(qos) + .build() + .expect("failed to create publisher"); + + let seen = Arc::new(Mutex::new(Vec::::new())); + let latch = Arc::new(Latch::default()); + let parked = Arc::new(AtomicBool::new(false)); + + let cb_seen = seen.clone(); + let cb_latch = latch.clone(); + let cb_parked = parked.clone(); + let _sub = node + .create_sub::(topic) + .with_qos(qos) + .build_with_callback(move |msg: Seq| { + cb_seen.lock().expect("seen poisoned").push(msg.counter); + if msg.counter == 0 { + // The drain thread is now provably inside the callback, so every + // subsequent publish lands in the queue rather than racing it. + cb_parked.store(true, Ordering::SeqCst); + cb_latch.wait(); + } + }) + .expect("failed to create subscriber"); + + thread::sleep(Duration::from_millis(300)); + + publisher + .publish(&Seq { counter: 0 }) + .expect("seed publish failed"); + + let deadline = Instant::now() + Duration::from_secs(5); + while !parked.load(Ordering::SeqCst) { + assert!( + Instant::now() < deadline, + "the drain thread never entered the callback" + ); + thread::sleep(Duration::from_millis(10)); + } + + for counter in 1..BURST { + publisher + .publish(&Seq { counter }) + .expect("burst publish failed"); + } + + latch.release(); + + // Settle: stop once the delivered sequence has not changed for 500 ms. + let mut last = 0usize; + let mut stable_since = Instant::now(); + let deadline = Instant::now() + Duration::from_secs(10); + loop { + let len = seen.lock().expect("seen poisoned").len(); + if len != last { + last = len; + stable_since = Instant::now(); + } else if stable_since.elapsed() >= Duration::from_millis(500) { + break; + } + assert!(Instant::now() < deadline, "delivery never settled"); + thread::sleep(Duration::from_millis(25)); + } + + seen.lock().expect("seen poisoned").clone() +} + +/// `KeepLast(DEPTH)` must retain the newest `DEPTH` undelivered samples. +/// +/// The first message is already out of the queue (the drain thread is parked +/// holding it), so the settled sequence is `[0]` followed by the last `DEPTH` of +/// the burst. Asserting the *values* rather than the count is what makes this a +/// drop-**oldest** detector: an unbounded queue yields `0,1,2,…` and a +/// drop-newest queue yields `0,1,2,3,4`. +#[test] +#[serial] +fn keep_last_drops_the_oldest_local_samples() { + let router = TestRouter::new(); + let endpoint = router.endpoint().to_string(); + + run_with_deadline("keep_last_drops_oldest", move || { + let delivered = burst_through_dispatcher( + &endpoint, + "dispatch_keep_last", + "/dispatch_keep_last", + QosHistory::KeepLast(NonZeroUsize::new(DEPTH).unwrap()), + ); + + let mut expected = vec![0]; + expected.extend((BURST - DEPTH as u64)..BURST); + + assert_eq!( + delivered, expected, + "a KeepLast({DEPTH}) callback subscriber must deliver the seed plus the \ + last {DEPTH} of the burst, dropping the oldest in between" + ); + }); +} + +/// `KeepAll` must not drop: the queue is unbounded on that profile, matching +/// `BoundedQueue::new(usize::MAX)` on the queue-mode path. +#[test] +#[serial] +fn keep_all_delivers_every_local_sample() { + let router = TestRouter::new(); + let endpoint = router.endpoint().to_string(); + + run_with_deadline("keep_all_lossless", move || { + let delivered = burst_through_dispatcher( + &endpoint, + "dispatch_keep_all", + "/dispatch_keep_all", + QosHistory::KeepAll, + ); + + let expected: Vec = (0..BURST).collect(); + assert_eq!( + delivered, expected, + "a KeepAll callback subscriber must not drop" + ); + }); +} diff --git a/crates/hiroz-tests/tests/reentrant_publish.rs b/crates/hiroz-tests/tests/reentrant_publish.rs new file mode 100644 index 000000000..90211f965 --- /dev/null +++ b/crates/hiroz-tests/tests/reentrant_publish.rs @@ -0,0 +1,796 @@ +//! Re-entrant publish from inside a subscriber callback must not deadlock. +//! +//! Every hiroz subscriber used to be declared as a zenoh-ext `AdvancedSubscriber`, +//! whose sample callback runs the user closure while a non-reentrant +//! `std::sync::Mutex` guard is alive (`sub_callback` takes `zlock!(statesref)`, +//! then `handle_sample` calls the callback under it). Zenoh core dispatches +//! samples published on the same session synchronously on the publishing thread, +//! so publishing from inside a callback re-enters that mutex on the very thread +//! that already holds it — a deterministic self-deadlock. +//! +//! Each scenario runs on a dedicated thread and reports through a channel, so a +//! hang fails the test instead of wedging the harness. +//! +//! Two families of scenario run here, because hiroz uses two different subscriber +//! implementations depending on QoS: +//! +//! * **Volatile** (the ROS 2 default) declares a plain zenoh subscriber, whose +//! callback runs inline with no lock held. +//! * **TransientLocal** must keep the `AdvancedSubscriber` — history replay and +//! sample-miss recovery live there — so its user callback is moved onto a +//! dedicated delivery thread and zenoh-ext only ever gets an enqueue-only shim. +//! +//! Both must survive the same re-entrancy scenarios, and with the same +//! observable semantics, so each scenario is written twice. + +mod common; + +use std::{ + sync::{ + Arc, Mutex, + atomic::{AtomicBool, AtomicUsize, Ordering}, + mpsc, + }, + thread, + time::{Duration, Instant}, +}; + +use common::{TestRouter, create_hiroz_context_with_endpoint}; +use hiroz::{ + Builder, TypeHash, + qos::{QosDurability, QosProfile}, + ros_msg::MessageTypeInfo, +}; +use serde::{Deserialize, Serialize}; +use serial_test::serial; + +/// QoS that forces hiroz down the zenoh-ext `AdvancedSubscriber` path. +/// +/// This is the case the `qos_needs_advanced` gate deliberately does *not* cover: +/// the advanced entity is genuinely needed here, so the deadlock has to be solved +/// rather than side-stepped. +fn transient_local() -> QosProfile { + QosProfile { + durability: QosDurability::TransientLocal, + ..Default::default() + } +} + +/// Budget for one scenario. Generous relative to the work done (a handful of +/// intra-process publishes) — anything slower than this is a hang, not slowness. +const SCENARIO_TIMEOUT: Duration = Duration::from_secs(20); + +/// How long to wait for the expected number of deliveries once seeded. +const DELIVERY_TIMEOUT: Duration = Duration::from_secs(5); + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +struct Tick { + counter: u64, +} + +impl MessageTypeInfo for Tick { + fn type_name() -> &'static str { + "test_msgs::msg::dds_::Tick_" + } + + fn type_hash() -> TypeHash { + TypeHash::zero() + } +} + +impl hiroz::ros_msg::WithTypeInfo for Tick {} + +impl hiroz::msg::ZMessage for Tick { + type Serdes = hiroz::msg::SerdeCdrSerdes; +} + +/// Run `scenario` on its own thread and fail (rather than hang) if it does not +/// finish within [`SCENARIO_TIMEOUT`]. +/// +/// On timeout the worker thread is deliberately left running: it is blocked on a +/// mutex it can never acquire and cannot be unwound. Each scenario owns its own +/// context and router, and the test binary exits shortly after. +fn run_with_deadline(name: &'static str, scenario: impl FnOnce() + Send + 'static) { + let (tx, rx) = mpsc::channel(); + thread::Builder::new() + .name(name.to_string()) + .spawn(move || { + scenario(); + let _ = tx.send(()); + }) + .expect("failed to spawn scenario thread"); + + if rx.recv_timeout(SCENARIO_TIMEOUT).is_err() { + panic!( + "`{name}` did not finish within {SCENARIO_TIMEOUT:?} — \ + re-entrant publish from a subscriber callback deadlocked" + ); + } +} + +/// Block until `seen` reaches `expected`, or fail with what actually arrived. +fn await_deliveries(seen: &AtomicUsize, expected: usize) { + let deadline = Instant::now() + DELIVERY_TIMEOUT; + while seen.load(Ordering::SeqCst) < expected { + assert!( + Instant::now() < deadline, + "only {} of {expected} messages delivered", + seen.load(Ordering::SeqCst) + ); + thread::sleep(Duration::from_millis(20)); + } +} + +/// A callback that publishes back onto its own topic must make progress. +/// +/// The minimal shape of the bug: one subscriber, one publisher, one session. The +/// callback re-publishes for a bounded number of hops so the scenario terminates +/// by construction rather than relying on the fix to bound it. +#[test] +#[serial] +fn callback_republishing_on_same_topic_does_not_deadlock() { + let router = TestRouter::new(); + let endpoint = router.endpoint().to_string(); + + run_with_deadline("same_topic", move || { + const HOPS: u64 = 4; + + let ctx = create_hiroz_context_with_endpoint(&endpoint).expect("failed to create context"); + let node = ctx + .create_node("reentrant_same_topic") + .build() + .expect("failed to create node"); + + let publisher = Arc::new( + node.create_pub::("/reentrant_same") + .build() + .expect("failed to create publisher"), + ); + let seen = Arc::new(AtomicUsize::new(0)); + + let cb_pub = publisher.clone(); + let cb_seen = seen.clone(); + let _sub = node + .create_sub::("/reentrant_same") + .build_with_callback(move |msg: Tick| { + cb_seen.fetch_add(1, Ordering::SeqCst); + if msg.counter < HOPS { + // Re-entrant publish on the same session, from inside the + // subscriber callback. This is what used to deadlock. + cb_pub + .publish(&Tick { + counter: msg.counter + 1, + }) + .expect("re-entrant publish failed"); + } + }) + .expect("failed to create subscriber"); + + thread::sleep(Duration::from_millis(300)); + publisher + .publish(&Tick { counter: 0 }) + .expect("seed publish failed"); + + await_deliveries(&seen, HOPS as usize + 1); + }); +} + +/// Two callbacks publishing to each other's topic must make progress. +/// +/// Distinct from the same-topic case: two *different* subscribers, and so two +/// different zenoh-ext state mutexes, form a cycle — the shape a real ping-pong +/// node pair has. The deadlock still occurs, one frame later. +#[test] +#[serial] +fn callback_cycle_across_two_topics_does_not_deadlock() { + let router = TestRouter::new(); + let endpoint = router.endpoint().to_string(); + + run_with_deadline("two_topics", move || { + const HOPS: u64 = 4; + + let ctx = create_hiroz_context_with_endpoint(&endpoint).expect("failed to create context"); + let node = ctx + .create_node("reentrant_cycle") + .build() + .expect("failed to create node"); + + let pub_a = Arc::new( + node.create_pub::("/reentrant_a") + .build() + .expect("failed to create publisher a"), + ); + let pub_b = Arc::new( + node.create_pub::("/reentrant_b") + .build() + .expect("failed to create publisher b"), + ); + let seen = Arc::new(AtomicUsize::new(0)); + + let cb_pub_b = pub_b.clone(); + let cb_seen_a = seen.clone(); + let _sub_a = node + .create_sub::("/reentrant_a") + .build_with_callback(move |msg: Tick| { + cb_seen_a.fetch_add(1, Ordering::SeqCst); + if msg.counter < HOPS { + cb_pub_b + .publish(&Tick { + counter: msg.counter + 1, + }) + .expect("publish a->b failed"); + } + }) + .expect("failed to create subscriber a"); + + let cb_pub_a = pub_a.clone(); + let cb_seen_b = seen.clone(); + let _sub_b = node + .create_sub::("/reentrant_b") + .build_with_callback(move |msg: Tick| { + cb_seen_b.fetch_add(1, Ordering::SeqCst); + if msg.counter < HOPS { + cb_pub_a + .publish(&Tick { + counter: msg.counter + 1, + }) + .expect("publish b->a failed"); + } + }) + .expect("failed to create subscriber b"); + + thread::sleep(Duration::from_millis(300)); + pub_a + .publish(&Tick { counter: 0 }) + .expect("seed publish failed"); + + await_deliveries(&seen, HOPS as usize + 1); + }); +} + +/// A self-feeding callback loop must *iterate*, not recurse. +/// +/// This is the detector for the whole point of routing session-local delivery +/// through the dispatcher. A callback that republishes to its own topic used to +/// be reached inline from inside `publish()`, so the loop was recursion: it grew +/// the stack, and hiroz had to cap it at `MAX_CALLBACK_REENTRY_DEPTH = 16` and +/// drop samples past the cap to avoid a `SIGSEGV`. +/// +/// Now the sample is enqueued and the callback runs on the dispatcher thread, so +/// each iteration returns to a flat stack before the next begins. The loop runs +/// indefinitely at constant stack depth and constant queue depth. +/// +/// The assertion is deliberately the *inverse* of the old one: it requires the +/// loop to exceed the old cap by orders of magnitude. Against the pre-fix +/// sources this fails at 16. +#[test] +#[serial] +fn self_feeding_callback_loop_iterates_without_a_depth_cap() { + let router = TestRouter::new(); + let endpoint = router.endpoint().to_string(); + + run_with_deadline("unbounded_loop", move || { + let ctx = create_hiroz_context_with_endpoint(&endpoint).expect("failed to create context"); + let node = ctx + .create_node("reentrant_unbounded") + .build() + .expect("failed to create node"); + + let publisher = Arc::new( + node.create_pub::("/reentrant_unbounded") + .build() + .expect("failed to create publisher"), + ); + let seen = Arc::new(AtomicUsize::new(0)); + + let cb_pub = publisher.clone(); + let cb_seen = seen.clone(); + let _sub = node + .create_sub::("/reentrant_unbounded") + .build_with_callback(move |msg: Tick| { + // Stop feeding well before the deadline so the test ends on its + // own; the loop's *ability* to keep going is what is under test. + if cb_seen.fetch_add(1, Ordering::SeqCst) >= ITERATION_TARGET { + return; + } + let _ = cb_pub.publish(&Tick { + counter: msg.counter + 1, + }); + }) + .expect("failed to create subscriber"); + + thread::sleep(Duration::from_millis(300)); + publisher + .publish(&Tick { counter: 0 }) + .expect("seed publish failed"); + + await_deliveries(&seen, ITERATION_TARGET); + + let delivered = seen.load(Ordering::SeqCst); + assert!( + delivered >= ITERATION_TARGET, + "self-feeding loop stalled at {delivered} deliveries (target {ITERATION_TARGET}). \ + Under the old inline dispatch this caps out at MAX_CALLBACK_REENTRY_DEPTH = 16." + ); + }); +} + +/// How far the self-feeding loops must run to prove they are iterative. +/// +/// Two orders of magnitude above the old depth cap of 16, and well past the +/// stack depth a recursive implementation survives: with the fix reverted these +/// same tests die with `fatal runtime error: stack overflow` rather than merely +/// falling short of the target. +/// +/// Deliberately not larger. At 20_000 these loops saturate a core for long +/// enough to perturb the *next* test binary in a sequential run — `parameter_tests` +/// began failing three service calls on timeout purely from the load, with no +/// code path in common (verified: the dispatcher branch is never taken in that +/// suite). The property under test is "does it iterate at all", which 2_000 +/// settles just as conclusively as 20_000. +const ITERATION_TARGET: usize = 2_000; + +// --------------------------------------------------------------------------- +// TransientLocal variants +// +// These exercise the `AdvancedSubscriber` path, which cannot avoid holding its +// state mutex across the user callback: `handle_sample` interleaves +// `callback.call(sample)` with mutation of `last_delivered`/`pending_samples`. +// Instead the callback zenoh-ext receives only enqueues, and hiroz runs the real +// callback on a dedicated delivery thread with no lock held. +// --------------------------------------------------------------------------- + +/// TransientLocal counterpart of +/// [`callback_republishing_on_same_topic_does_not_deadlock`]. +#[test] +#[serial] +fn transient_local_callback_republishing_on_same_topic_does_not_deadlock() { + let router = TestRouter::new(); + let endpoint = router.endpoint().to_string(); + + run_with_deadline("tl_same_topic", move || { + const HOPS: u64 = 4; + + let ctx = create_hiroz_context_with_endpoint(&endpoint).expect("failed to create context"); + let node = ctx + .create_node("reentrant_tl_same_topic") + .build() + .expect("failed to create node"); + + let publisher = Arc::new( + node.create_pub::("/reentrant_tl_same") + .with_qos(transient_local()) + .build() + .expect("failed to create publisher"), + ); + let seen = Arc::new(AtomicUsize::new(0)); + + let cb_pub = publisher.clone(); + let cb_seen = seen.clone(); + let _sub = node + .create_sub::("/reentrant_tl_same") + .with_qos(transient_local()) + .build_with_callback(move |msg: Tick| { + cb_seen.fetch_add(1, Ordering::SeqCst); + if msg.counter < HOPS { + cb_pub + .publish(&Tick { + counter: msg.counter + 1, + }) + .expect("re-entrant publish failed"); + } + }) + .expect("failed to create subscriber"); + + thread::sleep(Duration::from_millis(300)); + publisher + .publish(&Tick { counter: 0 }) + .expect("seed publish failed"); + + await_deliveries(&seen, HOPS as usize + 1); + }); +} + +/// TransientLocal counterpart of +/// [`callback_cycle_across_two_topics_does_not_deadlock`]. +/// +/// Each subscriber owns its own delivery thread, so this also covers a callback +/// on one delivery thread publishing into a *different* advanced subscriber. +#[test] +#[serial] +fn transient_local_callback_cycle_across_two_topics_does_not_deadlock() { + let router = TestRouter::new(); + let endpoint = router.endpoint().to_string(); + + run_with_deadline("tl_two_topics", move || { + const HOPS: u64 = 4; + + let ctx = create_hiroz_context_with_endpoint(&endpoint).expect("failed to create context"); + let node = ctx + .create_node("reentrant_tl_cycle") + .build() + .expect("failed to create node"); + + let pub_a = Arc::new( + node.create_pub::("/reentrant_tl_a") + .with_qos(transient_local()) + .build() + .expect("failed to create publisher a"), + ); + let pub_b = Arc::new( + node.create_pub::("/reentrant_tl_b") + .with_qos(transient_local()) + .build() + .expect("failed to create publisher b"), + ); + let seen = Arc::new(AtomicUsize::new(0)); + + let cb_pub_b = pub_b.clone(); + let cb_seen_a = seen.clone(); + let _sub_a = node + .create_sub::("/reentrant_tl_a") + .with_qos(transient_local()) + .build_with_callback(move |msg: Tick| { + cb_seen_a.fetch_add(1, Ordering::SeqCst); + if msg.counter < HOPS { + cb_pub_b + .publish(&Tick { + counter: msg.counter + 1, + }) + .expect("publish a->b failed"); + } + }) + .expect("failed to create subscriber a"); + + let cb_pub_a = pub_a.clone(); + let cb_seen_b = seen.clone(); + let _sub_b = node + .create_sub::("/reentrant_tl_b") + .with_qos(transient_local()) + .build_with_callback(move |msg: Tick| { + cb_seen_b.fetch_add(1, Ordering::SeqCst); + if msg.counter < HOPS { + cb_pub_a + .publish(&Tick { + counter: msg.counter + 1, + }) + .expect("publish b->a failed"); + } + }) + .expect("failed to create subscriber b"); + + thread::sleep(Duration::from_millis(300)); + pub_a + .publish(&Tick { counter: 0 }) + .expect("seed publish failed"); + + await_deliveries(&seen, HOPS as usize + 1); + }); +} + +/// TransientLocal counterpart of +/// [`self_feeding_callback_loop_iterates_without_a_depth_cap`]. +/// +/// This path always ran the callback on the dispatcher thread, so it was already +/// iterative — the depth cap was carried across the queue only to keep its +/// behaviour identical to the inline path's. Now that the inline path is gone +/// there is nothing to stay identical to, and both paths iterate. Asserting it +/// here keeps the two in step. +#[test] +#[serial] +fn transient_local_self_feeding_callback_loop_iterates() { + let router = TestRouter::new(); + let endpoint = router.endpoint().to_string(); + + run_with_deadline("tl_unbounded_loop", move || { + let ctx = create_hiroz_context_with_endpoint(&endpoint).expect("failed to create context"); + let node = ctx + .create_node("reentrant_tl_unbounded") + .build() + .expect("failed to create node"); + + let publisher = Arc::new( + node.create_pub::("/reentrant_tl_unbounded") + .with_qos(transient_local()) + .build() + .expect("failed to create publisher"), + ); + let seen = Arc::new(AtomicUsize::new(0)); + + let cb_pub = publisher.clone(); + let cb_seen = seen.clone(); + let _sub = node + .create_sub::("/reentrant_tl_unbounded") + .with_qos(transient_local()) + .build_with_callback(move |msg: Tick| { + if cb_seen.fetch_add(1, Ordering::SeqCst) >= ITERATION_TARGET { + return; + } + let _ = cb_pub.publish(&Tick { + counter: msg.counter + 1, + }); + }) + .expect("failed to create subscriber"); + + thread::sleep(Duration::from_millis(300)); + publisher + .publish(&Tick { counter: 0 }) + .expect("seed publish failed"); + + await_deliveries(&seen, ITERATION_TARGET); + + let delivered = seen.load(Ordering::SeqCst); + assert!( + delivered >= ITERATION_TARGET, + "self-feeding loop stalled at {delivered} deliveries (target {ITERATION_TARGET})" + ); + }); +} + +/// The `intra` closed loop: two topics, two callbacks, one session, each +/// callback feeding the other. This is the shape the `afor` benchmark's `intra` +/// cell drives, and the reason that cell could not produce a number. +/// +/// Under inline session-local dispatch this is not a loop at all — it is mutual +/// recursion on one thread, so it either overflows the stack or, with the depth +/// cap, stops dead at 16. Under dispatcher delivery each hop returns to a flat +/// stack, so the loop runs as long as it is fed. +#[test] +#[serial] +fn intra_closed_loop_runs_iteratively() { + let router = TestRouter::new(); + let endpoint = router.endpoint().to_string(); + + run_with_deadline("intra_closed_loop", move || { + let ctx = create_hiroz_context_with_endpoint(&endpoint).expect("failed to create context"); + let node = ctx + .create_node("intra_closed_loop") + .build() + .expect("failed to create node"); + + let hello_pub = Arc::new( + node.create_pub::("/intra_hello") + .build() + .expect("failed to create hello publisher"), + ); + let world_pub = Arc::new( + node.create_pub::("/intra_world") + .build() + .expect("failed to create world publisher"), + ); + let seen = Arc::new(AtomicUsize::new(0)); + + // hello -> world + let to_world = world_pub.clone(); + let hello_seen = seen.clone(); + let _hello_sub = node + .create_sub::("/intra_hello") + .build_with_callback(move |msg: Tick| { + if hello_seen.fetch_add(1, Ordering::SeqCst) >= ITERATION_TARGET { + return; + } + let _ = to_world.publish(&Tick { + counter: msg.counter + 1, + }); + }) + .expect("failed to create hello subscriber"); + + // world -> hello + let to_hello = hello_pub.clone(); + let world_seen = seen.clone(); + let _world_sub = node + .create_sub::("/intra_world") + .build_with_callback(move |msg: Tick| { + if world_seen.fetch_add(1, Ordering::SeqCst) >= ITERATION_TARGET { + return; + } + let _ = to_hello.publish(&Tick { + counter: msg.counter + 1, + }); + }) + .expect("failed to create world subscriber"); + + thread::sleep(Duration::from_millis(300)); + hello_pub + .publish(&Tick { counter: 0 }) + .expect("seed publish failed"); + + await_deliveries(&seen, ITERATION_TARGET); + + let delivered = seen.load(Ordering::SeqCst); + assert!( + delivered >= ITERATION_TARGET, + "intra closed loop stalled at {delivered} hops (target {ITERATION_TARGET}) — \ + this is the `afor` intra cell's failure mode" + ); + }); +} + +/// The delivery thread must not reorder samples. +/// +/// `AdvancedSubscriber` exists to deliver samples in source order and to recover +/// missed ones; a handoff that reordered them would defeat its entire purpose. +/// The shim enqueues from inside `handle_sample` — i.e. under zenoh-ext's state +/// mutex, in exactly the order zenoh-ext chose to deliver — and a single thread +/// pops FIFO, so the observed order must be the publish order. +#[test] +#[serial] +fn transient_local_delivery_preserves_order() { + let router = TestRouter::new(); + let endpoint = router.endpoint().to_string(); + + run_with_deadline("tl_ordering", move || { + const COUNT: u64 = 500; + + let ctx = create_hiroz_context_with_endpoint(&endpoint).expect("failed to create context"); + let node = ctx + .create_node("reentrant_tl_order") + .build() + .expect("failed to create node"); + + let publisher = node + .create_pub::("/reentrant_tl_order") + .with_qos(transient_local()) + .build() + .expect("failed to create publisher"); + + let received: Arc>> = Arc::new(Mutex::new(Vec::new())); + let seen = Arc::new(AtomicUsize::new(0)); + + let cb_received = received.clone(); + let cb_seen = seen.clone(); + let _sub = node + .create_sub::("/reentrant_tl_order") + .with_qos(transient_local()) + .build_with_callback(move |msg: Tick| { + cb_received.lock().unwrap().push(msg.counter); + cb_seen.fetch_add(1, Ordering::SeqCst); + }) + .expect("failed to create subscriber"); + + thread::sleep(Duration::from_millis(300)); + for counter in 0..COUNT { + publisher + .publish(&Tick { counter }) + .expect("publish failed"); + } + + await_deliveries(&seen, COUNT as usize); + + let received = received.lock().unwrap(); + let expected: Vec = (0..COUNT).collect(); + assert_eq!( + &received[..COUNT as usize], + &expected[..], + "delivery thread reordered samples" + ); + }); +} + +/// Dropping the subscriber must shut the delivery thread down — no leak, no hang. +/// +/// The sentinel is owned by the user callback, which the delivery thread owns in +/// turn, so its `Drop` firing proves the thread actually exited and released the +/// closure rather than being detached and left running. +#[test] +#[serial] +fn transient_local_subscriber_drop_shuts_down_delivery_thread() { + let router = TestRouter::new(); + let endpoint = router.endpoint().to_string(); + + run_with_deadline("tl_drop", move || { + let ctx = create_hiroz_context_with_endpoint(&endpoint).expect("failed to create context"); + let node = ctx + .create_node("reentrant_tl_drop") + .build() + .expect("failed to create node"); + + let publisher = node + .create_pub::("/reentrant_tl_drop") + .with_qos(transient_local()) + .build() + .expect("failed to create publisher"); + + /// Fires when the delivery thread drops the user callback. + struct Sentinel(Arc); + impl Drop for Sentinel { + fn drop(&mut self) { + self.0.store(true, Ordering::SeqCst); + } + } + + let callback_dropped = Arc::new(AtomicBool::new(false)); + let sentinel = Sentinel(callback_dropped.clone()); + let seen = Arc::new(AtomicUsize::new(0)); + let cb_seen = seen.clone(); + + let sub = node + .create_sub::("/reentrant_tl_drop") + .with_qos(transient_local()) + .build_with_callback(move |_msg: Tick| { + // Keep the sentinel owned by the callback. + let _ = &sentinel; + cb_seen.fetch_add(1, Ordering::SeqCst); + }) + .expect("failed to create subscriber"); + + thread::sleep(Duration::from_millis(300)); + publisher + .publish(&Tick { counter: 0 }) + .expect("seed publish failed"); + await_deliveries(&seen, 1); + + assert!( + !callback_dropped.load(Ordering::SeqCst), + "callback was dropped while the subscriber was still alive" + ); + + // If `Drop` deadlocked or the join hung, the deadline guard fails the test. + drop(sub); + + assert!( + callback_dropped.load(Ordering::SeqCst), + "subscriber dropped without shutting down its delivery thread — \ + the thread is leaked" + ); + }); +} + +/// Dropping the subscriber *from inside its own callback* must not deadlock. +/// +/// That drop runs on the delivery thread itself, so joining the thread would be +/// a self-join. The dispatcher detects this and lets the thread wind itself down +/// instead. +#[test] +#[serial] +fn transient_local_subscriber_dropped_inside_its_own_callback_does_not_deadlock() { + let router = TestRouter::new(); + let endpoint = router.endpoint().to_string(); + + run_with_deadline("tl_self_drop", move || { + let ctx = create_hiroz_context_with_endpoint(&endpoint).expect("failed to create context"); + let node = ctx + .create_node("reentrant_tl_self_drop") + .build() + .expect("failed to create node"); + + let publisher = node + .create_pub::("/reentrant_tl_self_drop") + .with_qos(transient_local()) + .build() + .expect("failed to create publisher"); + + // The subscriber hands itself to its own callback, which drops it. + let slot: Arc>>> = Arc::new(Mutex::new(None)); + let dropped = Arc::new(AtomicBool::new(false)); + + let cb_slot = slot.clone(); + let cb_dropped = dropped.clone(); + let sub = node + .create_sub::("/reentrant_tl_self_drop") + .with_qos(transient_local()) + .build_with_callback(move |_msg: Tick| { + // Runs on the delivery thread; this drop is the self-join case. + let taken = cb_slot.lock().unwrap().take(); + drop(taken); + cb_dropped.store(true, Ordering::SeqCst); + }) + .expect("failed to create subscriber"); + + *slot.lock().unwrap() = Some(Box::new(sub)); + + thread::sleep(Duration::from_millis(300)); + publisher + .publish(&Tick { counter: 0 }) + .expect("seed publish failed"); + + let deadline = Instant::now() + DELIVERY_TIMEOUT; + while !dropped.load(Ordering::SeqCst) { + assert!( + Instant::now() < deadline, + "subscriber was never dropped from its own callback" + ); + thread::sleep(Duration::from_millis(20)); + } + }); +} diff --git a/crates/hiroz/src/common.rs b/crates/hiroz/src/common.rs index 5cf360b85..b62b24ce7 100644 --- a/crates/hiroz/src/common.rs +++ b/crates/hiroz/src/common.rs @@ -19,6 +19,20 @@ pub(crate) enum DataHandler { } impl DataHandler { + /// Whether `handle` runs *user* code on the delivering thread. + /// + /// Only [`DataHandler::Callback`] does. The queue variants enqueue and + /// return — structurally the same thing zenoh's own `FifoChannel` handler + /// does — so the user's code runs on whatever thread calls `recv()`, and + /// there is nothing on the delivery thread that could re-enter hiroz. + /// + /// `QueueWithNotifier`'s notifier is deliberately not counted: it is the rmw + /// layer's wait-set wake, which must run promptly on the delivery thread and + /// does not call back into hiroz. + pub(crate) fn runs_user_code(&self) -> bool { + matches!(self, DataHandler::Callback(_)) + } + pub(crate) fn handle(&self, data: T) { match self { DataHandler::Queue(queue) => { diff --git a/crates/hiroz/src/ffi/subscriber.rs b/crates/hiroz/src/ffi/subscriber.rs index 35294812b..3c573058c 100644 --- a/crates/hiroz/src/ffi/subscriber.rs +++ b/crates/hiroz/src/ffi/subscriber.rs @@ -2,14 +2,15 @@ use super::node::{CNode, get_node_ref}; use super::qos::CQosProfile; use super::{ErrorCode, cstr_to_str}; use std::ffi::c_char; -use zenoh_ext::AdvancedSubscriber; + +use crate::pubsub::SubscriberHandle; /// Callback type for receiving messages pub type MessageCallback = extern "C" fn(user_data: usize, data: *const u8, len: usize); /// Raw subscriber wrapper that keeps the zenoh subscriber alive pub struct RawSubscriber { - pub inner: AdvancedSubscriber<()>, + pub inner: SubscriberHandle, } /// Opaque subscriber handle for FFI diff --git a/crates/hiroz/src/node.rs b/crates/hiroz/src/node.rs index 90435aeed..3c6774f62 100644 --- a/crates/hiroz/src/node.rs +++ b/crates/hiroz/src/node.rs @@ -657,7 +657,10 @@ impl ZNode { { use crate::{ entity::{EndpointEntity, EndpointKind}, - pubsub::apply_transient_local_sub, + pubsub::{ + CallbackDispatcher, DISPATCH_UNBOUNDED, SubscriberHandle, + apply_transient_local_sub, dispatch_capacity, qos_needs_advanced, + }, topic_name, }; use zenoh_ext::AdvancedSubscriberBuilderExt; @@ -681,15 +684,44 @@ impl ZNode { }; let topic_ke = self.keyexpr_format.topic_key_expr(&entity)?; - let subscriber = self - .session - .declare_subscriber((*topic_ke).clone()) - .callback(move |sample| { - let payload = sample.payload().to_bytes(); - callback(&payload); - }) - .advanced(); - let subscriber = apply_transient_local_sub(subscriber, &entity.qos).wait()?; + let raw_callback = Arc::new(move |sample: zenoh::sample::Sample| { + let payload = sample.payload().to_bytes(); + callback(&payload); + }); + + // Same rule as the typed path: only use zenoh-ext when the QoS asks for + // advanced features. Either way this callback is user (FFI) code, so it + // never runs on a thread that is inside a hiroz publish — see + // `pubsub::CallbackDispatcher`. + let subscriber = if qos_needs_advanced(&entity.qos) { + let dispatcher = + CallbackDispatcher::spawn(&qualified_topic, raw_callback, DISPATCH_UNBOUNDED)?; + let subscriber = self + .session + .declare_subscriber((*topic_ke).clone()) + .callback(dispatcher.always_shim()); + SubscriberHandle::Advanced { + subscriber: Box::new( + apply_transient_local_sub(subscriber.advanced(), &entity.qos).wait()?, + ), + dispatcher, + } + } else { + let dispatcher = CallbackDispatcher::spawn( + &qualified_topic, + raw_callback.clone(), + dispatch_capacity(&entity.qos), + )?; + let subscriber = self + .session + .declare_subscriber((*topic_ke).clone()) + .callback(dispatcher.local_only_shim(raw_callback)) + .wait()?; + SubscriberHandle::Plain { + subscriber, + dispatcher: Some(dispatcher), + } + }; Ok(crate::ffi::subscriber::RawSubscriber { inner: subscriber }) } diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index b03bb30b6..74343ee17 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -30,6 +30,469 @@ use zenoh_ext::{ /// Matches rmw_zenoh_cpp's `SAMPLE_MISS_DETECTION_HEARTBEAT_PERIOD`. const SAMPLE_MISS_HEARTBEAT_PERIOD: Duration = Duration::from_millis(500); +thread_local! { + /// How many hiroz publish calls are currently on this thread's stack. + /// + /// Non-zero means: any sample this thread is *about* to deliver was produced + /// by this same thread, synchronously, from inside `put`. See + /// [`local_publish_active`]. + static LOCAL_PUBLISH_DEPTH: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +/// RAII marker set for the duration of a hiroz publish. +/// +/// Every `ZPub` publish path funnels through [`ZPub::finish_put`], which holds +/// one of these across the zenoh `put`. Nesting is counted rather than flagged +/// so that a publish issued from inside a callback that is itself running on a +/// thread already inside a publish restores the right state on unwind. +pub(crate) struct LocalPublishGuard; + +impl LocalPublishGuard { + pub(crate) fn enter() -> Self { + LOCAL_PUBLISH_DEPTH.with(|d| d.set(d.get() + 1)); + Self + } +} + +impl Drop for LocalPublishGuard { + fn drop(&mut self) { + LOCAL_PUBLISH_DEPTH.with(|d| d.set(d.get().saturating_sub(1))); + } +} + +/// Whether this thread is currently inside a hiroz publish. +/// +/// This is the discriminator between the two ways a subscriber callback can be +/// reached, and it is what makes re-entrancy structurally impossible without +/// taxing the inter-process path: +/// +/// * **true** — the sample is being delivered *synchronously on the publishing +/// thread*. Zenoh does this for same-session delivery (`Session::resolve_put` +/// drops the session lock and calls the local callbacks inline) and also for +/// two sessions sharing one process with a direct in-process route +/// (`send_push_consume` -> `route_data` -> the peer session's callbacks, no +/// thread hop). Running the user callback here is what allowed a callback that +/// publishes into its own topic graph to *recurse* instead of iterate. So on +/// this path hiroz enqueues and returns, exactly as zenoh's own `FifoChannel` +/// handler does, and the callback runs on the dispatcher thread. +/// +/// * **false** — the sample arrived over a transport and is being delivered on a +/// zenoh RX worker (`ZRuntime::RX`, threads named `rx-N`), which is never an +/// application thread and never inside a hiroz publish. There is nothing to +/// re-enter, so the callback runs inline and the inter-process path pays only +/// this thread-local read. +/// +/// Note this deliberately keys on the *publishing thread*, not on zenoh's +/// `Locality`. A `Locality::Remote`-tagged sample crossing two sessions inside +/// one process is still delivered inline on the publisher's thread, so an +/// `allowed_origin(SessionLocal)` split would miss it. The thread is the honest +/// signal; the origin is not. +fn local_publish_active() -> bool { + LOCAL_PUBLISH_DEPTH.with(|d| d.get()) != 0 +} + +/// Backlog size at which an *unbounded* [`CallbackDispatcher`] first warns. +/// Doubles after each warning so a persistently slow callback does not flood the +/// log. A bounded dispatcher cannot reach this — it warns on drops instead. +const DISPATCH_BACKLOG_WARN_AT: usize = 1024; + +/// Capacity a [`CallbackDispatcher`] must be given to be unbounded, i.e. lossless. +pub(crate) const DISPATCH_UNBOUNDED: usize = usize::MAX; + +/// The dispatcher capacity implied by a subscriber's history QoS. +/// +/// Deliberately the *same* expression [`ZSubBuilder::build`] uses to size the +/// queue-mode [`BoundedQueue`]: `KeepLast(depth)` keeps `depth`, `KeepAll` keeps +/// everything. A callback subscriber and a queue subscriber declared with the +/// same QoS therefore retain the same number of undelivered samples, which is +/// the only reading of ROS `KEEP_LAST(depth)` that does not depend on which +/// hiroz API the user happened to pick. +/// +/// A zero depth (the rmw spelling of "system default", which cannot be produced +/// through [`QosProfile`] but can arrive over the wire) is floored at 1 rather +/// than being allowed to degenerate into "keep nothing". +pub(crate) fn dispatch_capacity(qos: &hiroz_protocol::qos::QosProfile) -> usize { + match qos.history { + QosHistory::KeepLast(depth) => depth.max(1), + QosHistory::KeepAll => DISPATCH_UNBOUNDED, + } +} + +struct DispatchState { + /// Samples awaiting delivery, in the order zenoh decided to deliver them. + pending: std::collections::VecDeque, + /// Set by [`CallbackDispatcher::drop`]: drain what is queued, then exit. + closed: bool, + /// Next backlog length that triggers a warning. Unbounded queues only. + warn_at: usize, + /// Samples discarded because the queue was at capacity. + dropped: u64, + /// Next `dropped` total that triggers a warning. + warn_dropped_at: u64, +} + +struct DispatchQueue { + state: Mutex, + ready: std::sync::Condvar, + topic: String, + /// Maximum number of undelivered samples retained. [`DISPATCH_UNBOUNDED`] + /// means lossless; anything smaller drops the *oldest* on overflow, exactly + /// as [`BoundedQueue::push`] does. See [`CallbackDispatcher`]'s + /// "Backpressure" section for which path gets which. + capacity: usize, +} + +impl DispatchQueue { + fn lock(&self) -> std::sync::MutexGuard<'_, DispatchState> { + // A panicking user callback must not wedge the subscriber: the queue + // holds no invariant that a partial mutation could break. + self.state.lock().unwrap_or_else(|e| e.into_inner()) + } + + /// The shim callback handed to zenoh. May run with zenoh-ext's state mutex + /// held (advanced path) or on the publishing thread inside `put` (local + /// path), so it must not do anything that could publish or block. + fn enqueue(&self, sample: Sample) { + let (backlog, dropped) = { + let mut state = self.lock(); + if state.closed { + return; + } + + // Drop the oldest, never the newest and never the incoming sample: + // the same choice `BoundedQueue::push` makes, and the same one ROS + // `KEEP_LAST(depth)` describes. A bounded queue that *blocked* here + // would re-create the original deadlock — see the type's docs. + let dropped = if state.pending.len() >= self.capacity { + state.pending.pop_front(); + state.dropped = state.dropped.saturating_add(1); + if state.dropped >= state.warn_dropped_at { + state.warn_dropped_at = state.dropped.saturating_mul(2); + Some(state.dropped) + } else { + None + } + } else { + None + }; + + state.pending.push_back(sample); + let len = state.pending.len(); + let backlog = if len >= state.warn_at { + state.warn_at = len.saturating_mul(2); + Some(len) + } else { + None + }; + (backlog, dropped) + }; + self.ready.notify_one(); + if let Some(len) = backlog { + warn!( + topic = %self.topic, + backlog = len, + "subscriber delivery backlog is growing; the callback is slower than the \ + publish rate. This queue is lossless, so the backlog costs memory." + ); + } + if let Some(total) = dropped { + warn!( + topic = %self.topic, + dropped = total, + capacity = self.capacity, + "subscriber delivery queue is full; dropping the oldest undelivered sample. \ + The callback is slower than the publish rate — raise the history depth or \ + make the callback cheaper." + ); + } + } + + /// Blocks until a sample is available, or until the queue is closed *and* + /// empty (returns `None`, ending the drain loop). + fn dequeue(&self) -> Option { + let mut state = self.lock(); + loop { + if let Some(entry) = state.pending.pop_front() { + return Some(entry); + } + if state.closed { + return None; + } + state = self.ready.wait(state).unwrap_or_else(|e| e.into_inner()); + } + } +} + +/// Runs a subscriber's user callback on a dedicated thread, fed by a FIFO queue. +/// +/// This is hiroz's equivalent of zenoh's `FifoChannel` handler, and of +/// zenoh-python's `Callback(indirect=True)` — which is what zenoh-python +/// installs by default when you hand `declare_subscriber` a plain callable. The +/// delivery thread enqueues and returns; user code runs here. +/// +/// Two independent reasons a sample takes this path: +/// +/// 1. **It was published by this same thread** ([`local_publish_active`]) — the +/// session-local case. Delivering inline would let a callback that publishes +/// into its own topic graph recurse instead of iterate. Enqueuing makes the +/// feedback loop *iterative*, which is why hiroz no longer needs a +/// re-entrancy depth cap: a callback simply cannot be reached from inside +/// `put`. +/// 2. **The subscriber is a zenoh-ext `AdvancedSubscriber`**, which invokes the +/// sample callback while holding the `std::sync::Mutex` that guards its +/// reordering state — and it *has* to: `handle_sample` interleaves +/// `callback.call(sample)` with mutation of `last_delivered` / +/// `pending_samples` (see `deliver_and_flush`, which calls the callback, +/// records the delivered sequence number, then drains newly-contiguous +/// pending samples calling the callback again). The guard cannot simply be +/// dropped before the call the way `Session::resolve_put` does, because the +/// lock protects exactly the state the delivery loop is walking. +/// +/// A sample that is neither — i.e. one that arrived over a transport, on a +/// zenoh RX worker, for a plain subscriber — is delivered inline and never +/// touches this queue. That is deliberate: the RX thread is not an application +/// thread and holds no hiroz lock, so there is nothing to re-enter, and the +/// inter-process path must not pay for a hazard it does not have. +/// +/// # Ordering +/// +/// One producer path, one FIFO queue, one drain thread, so the user observes +/// exactly the order zenoh decided to deliver in. On the advanced path the shim +/// enqueues from inside `handle_sample`, i.e. under zenoh-ext's state mutex, so +/// enqueue order includes the several back-to-back deliveries a single +/// `deliver_and_flush` performs when it drains pending samples; the reordering +/// and recovery guarantees `AdvancedSubscriber` exists to provide are +/// unaffected, only the thread the callback runs on changes. +/// +/// The one ordering property that is *not* preserved is between the two paths: +/// a plain subscriber that receives both local and remote publications on the +/// same topic now runs the local ones on this thread and the remote ones on an +/// RX thread, so their relative order is no longer guaranteed and the two can +/// overlap. Neither ROS 2 nor zenoh guarantees ordering across distinct +/// publishers, and a plain zenoh subscriber can already be invoked concurrently +/// from several RX workers, so this weakens no guarantee that was actually +/// being offered — but it is a real change and is called out here rather than +/// discovered later. +/// +/// # Backpressure +/// +/// The queue **never blocks its producer**. That is not a tuning choice: a +/// bounded queue that blocked would re-create the original deadlock in a new +/// form on both paths. On the advanced path the blocked thread sits inside +/// `sub_callback` holding zenoh-ext's state mutex; on the local path it sits +/// inside the user's own `publish()`, and in a closed feedback loop the drain +/// thread it waits on is the very thread that must publish for the queue to +/// drain. zenoh's own `FifoChannel` is bounded *and* blocking and documents +/// exactly this cost ("a slow subscriber could block the underlying Zenoh +/// thread", `fifo.rs`); hiroz does not adopt that failure mode. +/// +/// What remains is a choice between unbounded (lossless, can grow without +/// limit) and bounded drop-oldest (lossy, constant memory). **The two paths get +/// different answers, because they make different promises:** +/// +/// * **Plain path — bounded, drop-oldest, capacity from the subscriber's +/// history QoS** ([`dispatch_capacity`]). A plain subscriber is `Volatile` +/// with `KEEP_LAST(depth)`: it already promises only the last `depth` +/// undelivered samples, and hiroz's own queue-mode path enforces exactly that +/// with [`BoundedQueue`], from the same expression. A callback subscriber that +/// instead retained *every* undelivered sample would honour a QoS stricter +/// than the one it was declared with, and would let a tight local publish loop +/// with a slow callback grow the process until it died — a failure mode with +/// no upside, since the samples being retained are ones the declared QoS says +/// may be discarded. Drop-oldest also preserves the relative order of what +/// survives, so the ordering objection that applies to the advanced path does +/// not apply here. +/// +/// * **Advanced path — unbounded, lossless.** A `TransientLocal` subscriber +/// exists to replay history and to recover samples flagged as missed; dropping +/// here would discard data that zenoh-ext went out of its way to fetch, and +/// would break the reordering contract mid-flight, since a single +/// `deliver_and_flush` enqueues several back-to-back samples whose contiguity +/// is the whole point. Loss on this path is a correctness bug, not a QoS +/// allowance, so growth is accepted and surfaced by an escalating backlog +/// warning instead. +/// +/// One consequence is worth stating rather than discovering: on the plain path +/// only the *locally published* samples pass through this queue, so only they +/// are subject to the bound. A sample arriving over a transport is delivered +/// inline on an RX worker and is instead backpressured by zenoh's transport. A +/// slow callback therefore loses local samples and stalls remote ones. That +/// asymmetry is inherent to delivering the two on different threads — which is +/// what makes re-entrancy impossible without taxing the inter-process path — and +/// pre-dates the bound; the bound only changes which of the two is lossy. +pub struct CallbackDispatcher { + queue: Arc, + thread: Option>, +} + +impl CallbackDispatcher { + /// Spawns the drain thread. + /// + /// `handler` is shared: the drain thread always calls it, and the *plain* + /// path's shim additionally calls it inline for samples that did not + /// originate on this thread. Use [`Self::always_shim`] or + /// [`Self::local_only_shim`] to obtain the callback to hand to zenoh. + /// + /// `capacity` is the number of undelivered samples retained before the + /// oldest is dropped — [`dispatch_capacity`] on the plain path, + /// [`DISPATCH_UNBOUNDED`] on the advanced one. See the "Backpressure" + /// section for why the two differ. + pub(crate) fn spawn(topic: &str, handler: Arc, capacity: usize) -> Result + where + F: Fn(Sample) + Send + Sync + 'static, + { + let queue = Arc::new(DispatchQueue { + state: Mutex::new(DispatchState { + pending: std::collections::VecDeque::new(), + closed: false, + warn_at: DISPATCH_BACKLOG_WARN_AT, + dropped: 0, + warn_dropped_at: 1, + }), + ready: std::sync::Condvar::new(), + topic: topic.to_string(), + capacity, + }); + + let drain_queue = queue.clone(); + let drain_topic = topic.to_string(); + let thread = std::thread::Builder::new() + .name("hiroz-sub-drain".to_string()) + .spawn(move || { + while let Some(sample) = drain_queue.dequeue() { + // A panicking user callback must not kill the drain thread — + // that would silently stop all further delivery. + if std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| (*handler)(sample))) + .is_err() + { + tracing::error!( + topic = %drain_topic, + "subscriber callback panicked; dropping the sample and continuing" + ); + } + } + }) + .map_err(|e| { + zenoh::Error::from(format!("failed to spawn subscriber delivery thread: {e}")) + })?; + + Ok(Self { + queue, + thread: Some(thread), + }) + } + + /// A shim that enqueues **every** sample. Used for the advanced path, where + /// zenoh-ext holds its state mutex across the callback regardless of where + /// the sample came from. + pub(crate) fn always_shim(&self) -> impl Fn(Sample) + Send + Sync + 'static { + let queue = self.queue.clone(); + move |sample: Sample| queue.enqueue(sample) + } + + /// A shim that enqueues only samples produced by the delivering thread + /// itself, and invokes `handler` inline otherwise. Used for the plain path. + /// + /// The inline branch is the inter-process hot path: a sample that arrived + /// over a transport is delivered on a zenoh RX worker, which is never inside + /// a hiroz publish, so [`local_publish_active`] is false and the only cost + /// added to that path is this one thread-local read. + pub(crate) fn local_only_shim( + &self, + handler: Arc, + ) -> impl Fn(Sample) + Send + Sync + 'static + where + F: Fn(Sample) + Send + Sync + 'static, + { + let queue = self.queue.clone(); + move |sample: Sample| { + if local_publish_active() { + queue.enqueue(sample); + } else { + handler(sample); + } + } + } +} + +impl Drop for CallbackDispatcher { + fn drop(&mut self) { + self.queue.lock().closed = true; + self.queue.ready.notify_all(); + + let Some(thread) = self.thread.take() else { + return; + }; + if thread.thread().id() == std::thread::current().id() { + // The subscriber was dropped from inside its own callback. Joining + // ourselves would deadlock; the thread will observe `closed` and + // exit once this callback returns. + return; + } + if thread.join().is_err() { + warn!( + topic = %self.queue.topic, + "subscriber delivery thread terminated abnormally" + ); + } + } +} + +/// Whether a QoS profile needs a zenoh-ext advanced subscriber/publisher. +/// +/// The advanced entities exist for history replay, sample-miss detection and +/// recovery, and publisher/subscriber detection — all of which +/// [`apply_transient_local_sub`] and [`apply_transient_local_pub`] configure only +/// for `TransientLocal` durability. For the ROS 2 default (`Volatile`) an +/// unconfigured `AdvancedSubscriber` adds no protocol behaviour, but it *does* +/// run the user callback while holding a non-reentrant `std::sync::Mutex` +/// (`advanced_subscriber.rs`: `sub_callback` takes `zlock!(statesref)` and +/// `handle_sample` calls the callback under that guard). Combined with zenoh's +/// synchronous local delivery, that turns any publish from inside a callback +/// into a self-deadlock. So only pay for it when the QoS actually asks for it. +pub(crate) fn qos_needs_advanced(qos: &hiroz_protocol::qos::QosProfile) -> bool { + matches!(qos.durability, QosDurability::TransientLocal) +} + +/// The declared zenoh subscriber backing a [`ZSub`]. +/// +/// hiroz declares a plain subscriber unless the QoS profile actually configures +/// advanced features — see [`qos_needs_advanced`]. +pub enum SubscriberHandle { + /// A plain zenoh subscriber (the `Volatile` default). + /// + /// Samples that arrived over a transport run inline on the zenoh RX worker. + /// Samples published by the delivering thread itself are handed to the + /// dispatcher — see [`CallbackDispatcher`]. `dispatcher` is `None` for + /// queue-mode subscribers, which run no user code on the delivery thread and + /// so need no handoff. + Plain { + subscriber: zenoh::pubsub::Subscriber<()>, + dispatcher: Option, + }, + /// A zenoh-ext advanced subscriber, used for `TransientLocal` durability. + /// The user callback runs on the dispatcher's thread — see + /// [`CallbackDispatcher`] for why it cannot run inline. + Advanced { + /// Boxed because it is several times larger than the plain variant. + /// + /// Declared first so it drops first: undeclaring the subscriber stops + /// new samples from being enqueued before the dispatcher drains and + /// joins. + subscriber: Box>, + dispatcher: CallbackDispatcher, + }, +} + +impl std::fmt::Debug for SubscriberHandle { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Plain { .. } => f.write_str("SubscriberHandle::Plain"), + Self::Advanced { .. } => f.write_str("SubscriberHandle::Advanced"), + } + } +} + /// Query timeout for TransientLocal subscribers' initial history fetch. /// Matches rmw_zenoh_cpp's `query_timeout_ms = u64::max()` literally /// (`Duration::from_millis(u64::MAX)`, not `Duration::MAX`, to avoid any @@ -538,6 +1001,7 @@ where trace!("[PUB] Attached sn={}", sn); } + let _local = LocalPublishGuard::enter(); put_builder.wait() } @@ -572,7 +1036,18 @@ where if self.with_attachment { put_builder = put_builder.attachment(self.new_attachment()); } - put_builder.await + // The guard must cover the delivery, and delivery happens in + // `into_future`, not at the await: zenoh's `PublicationBuilder` future is + // `std::future::ready(self.wait())`, so the put — including any inline + // local-subscriber dispatch — completes before a future exists to poll. + // Scoping the guard here rather than across the `.await` also keeps this + // future `Send`, which a thread-local guard held across an await point + // would not. + let fut = { + let _local = LocalPublishGuard::enter(); + std::future::IntoFuture::into_future(put_builder) + }; + fut.await } /// Publish pre-serialized data directly @@ -593,6 +1068,7 @@ where if self.with_attachment { put_builder = put_builder.attachment(self.new_attachment()); } + let _local = LocalPublishGuard::enter(); put_builder.wait() } @@ -609,6 +1085,7 @@ where if self.with_attachment { put_builder = put_builder.attachment(self.new_attachment()); } + let _local = LocalPublishGuard::enter(); put_builder.wait() } @@ -825,9 +1302,12 @@ where let events_mgr = Arc::new(Mutex::new(EventsManager::new(gid))); let loss = Arc::new(MessageLossTracker::new(events_mgr.clone())); - // Wrap handler with encoding validation if expected encoding is set + // Wrap handler with encoding validation. No re-entrancy accounting is + // needed for this closure itself: a user callback is never reached + // from inside `put` — see `CallbackDispatcher`. let expected_encoding = self.expected_encoding.clone(); - let validated_handler = move |sample: Sample| { + let runs_user_code = handler.runs_user_code(); + let validated_handler = Arc::new(move |sample: Sample| { observe_loss(&loss, &sample); // Validate encoding if expected encoding is set if let Some(ref expected) = expected_encoding { @@ -847,23 +1327,73 @@ where } } handler.handle(sample) - }; - - // Build an AdvancedSubscriber and configure based on durability - let mut sub_builder = self - .session - .declare_subscriber(key_expr) - .callback(validated_handler) - .advanced(); - - // Apply locality restriction if specified - if let Some(locality) = self.locality { - sub_builder = sub_builder.allowed_origin(locality); - debug!("[SUB] Locality restriction: {:?}", locality); - } + }); - let sub_builder = apply_transient_local_sub(sub_builder, &self.entity.qos); - let inner = sub_builder.wait()?; + // Only go through zenoh-ext when the QoS profile actually configures + // advanced features. See `qos_needs_advanced`. + let inner = if qos_needs_advanced(&self.entity.qos) { + debug!("[SUB] Using AdvancedSubscriber (TransientLocal durability)"); + // `AdvancedSubscriber` holds its state lock across the callback and + // cannot avoid it, so *every* sample is enqueued and the real + // handler runs on the dispatcher's thread. Lossless: dropping would + // discard exactly the samples miss-detection recovered. See + // `CallbackDispatcher`. + let dispatcher = + CallbackDispatcher::spawn(&qualified_topic, validated_handler, DISPATCH_UNBOUNDED)?; + let mut sub_builder = self + .session + .declare_subscriber(key_expr) + .callback(dispatcher.always_shim()); + if let Some(locality) = self.locality { + sub_builder = sub_builder.allowed_origin(locality); + debug!("[SUB] Locality restriction: {:?}", locality); + } + let sub_builder = apply_transient_local_sub(sub_builder.advanced(), &self.entity.qos); + SubscriberHandle::Advanced { + subscriber: Box::new(sub_builder.wait()?), + dispatcher, + } + } else if runs_user_code { + // A plain subscriber holds no lock across the callback, but zenoh + // still delivers a same-thread publication *inline* — so a callback + // that publishes into its own topic graph would recurse. Hand those + // samples to the dispatcher; deliver everything else inline, which + // keeps the inter-process path at one thread-local read. Bounded at + // the history depth, drop-oldest — the same `KEEP_LAST(depth)` the + // queue-mode path enforces with `BoundedQueue`. + let dispatcher = CallbackDispatcher::spawn( + &qualified_topic, + validated_handler.clone(), + dispatch_capacity(&self.entity.qos), + )?; + let mut sub_builder = self + .session + .declare_subscriber(key_expr) + .callback(dispatcher.local_only_shim(validated_handler)); + if let Some(locality) = self.locality { + sub_builder = sub_builder.allowed_origin(locality); + debug!("[SUB] Locality restriction: {:?}", locality); + } + SubscriberHandle::Plain { + subscriber: sub_builder.wait()?, + dispatcher: Some(dispatcher), + } + } else { + // Queue mode: the delivery thread only enqueues, so there is no user + // code to move off it and no dispatcher to pay for. + let mut sub_builder = self + .session + .declare_subscriber(key_expr) + .callback(move |sample: Sample| validated_handler(sample)); + if let Some(locality) = self.locality { + sub_builder = sub_builder.allowed_origin(locality); + debug!("[SUB] Locality restriction: {:?}", locality); + } + SubscriberHandle::Plain { + subscriber: sub_builder.wait()?, + dispatcher: None, + } + }; let lv_ke = self .keyexpr_format @@ -949,6 +1479,60 @@ where self.build_internal(DataHandler::Callback(callback), None) } + /// Build a callback subscriber that receives the whole [`Sample`], undecoded. + /// + /// [`Self::build_with_callback`] must hand the callback an owned `S::Output`, + /// and [`ZDeserializer::Output`] carries no lifetime — so a serdes that only + /// forwards bytes (a language binding's identity codec, say) has no way to + /// express "borrow the payload", and must copy the entire message before the + /// callback has even seen it. That copy scales with payload size and is pure + /// waste when the consumer immediately re-reads the bytes into its own + /// representation. + /// + /// This entry point steps around it: the callback gets the `Sample`, so it + /// can borrow the payload (`sample.payload().to_bytes()` is a `Cow` that + /// borrows whenever the `ZBuf` is contiguous, which the receive path makes it) + /// and decode straight out of the network buffer. It can also reach the + /// sample's attachment, encoding and timestamp, which the decoded form drops. + /// + /// Everything else is identical to `build_with_callback` — same encoding + /// validation, same [`CallbackDispatcher`] handling, same liveliness and + /// graph registration. The callback is user code and is dispatched by exactly + /// the same rules. + /// + /// # Ownership + /// + /// As with `build_with_callback`, the returned [`ZSub`] must be kept alive for + /// the subscription to stay active. + pub fn build_with_sample_callback(self, callback: F) -> Result> + where + F: Fn(Sample) + Send + Sync + 'static, + S: ZDeserializer, + { + let expected_encoding = self.expected_encoding.clone(); + let callback = Arc::new(move |sample: Sample| { + if let Some(ref expected) = expected_encoding { + let encoding_str = sample.encoding().to_string(); + if let Some(received) = + crate::encoding::Encoding::from_zenoh_encoding(&encoding_str) + { + if &received != expected { + tracing::warn!( + "Encoding mismatch: expected {:?}, received {:?}", + expected, + received + ); + } + } else { + tracing::debug!("Unknown encoding format: {}", encoding_str); + } + } + callback(sample); + }); + + self.build_internal(DataHandler::Callback(callback), None) + } + #[cfg(feature = "rmw")] pub fn build_with_notifier(self, notify: F) -> Result> where @@ -992,7 +1576,7 @@ where pub struct ZSub { pub entity: EndpointEntity, pub queue: Option>>, - _inner: AdvancedSubscriber<()>, + _inner: SubscriberHandle, _lv_token: LivelinessToken, events_mgr: Arc>, graph: Arc, From f1185b6a08da0f0c674843de80ba7afd4f239c23 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 28 Jul 2026 17:08:30 +0800 Subject: [PATCH 02/30] fix(pubsub): guard the raw publish path and stop mislabelling drops Three defects, all in this branch's own new machinery. **The FFI publish path was never marked local.** `local_only_shim` hands a sample to the drain thread only while `LOCAL_PUBLISH_DEPTH` is set, and that flag comes from `LocalPublishGuard`. All four `ZPub` publish methods take the guard; `RawPublisher::publish_bytes` did not. A same-process raw publish was therefore delivered inline on the publishing thread, so a raw callback that publishes back into its own topic recurses until the stack is gone -- the exact defect the dispatcher exists to prevent, still reachable through the FFI door, which is the door `rmw-zenoh-rs` uses. **A bounded queue claimed to be lossless.** The backlog warning fired whenever the pending count crossed `DISPATCH_BACKLOG_WARN_AT`, and told the user the queue "is lossless, so the backlog costs memory". Both nearby comments assert that only unbounded dispatchers reach it, but nothing enforced that: capacity comes from `KeepLast(depth)`, and a `KeepLast(1024)` subscriber has exactly that capacity. It would warn that it cannot lose samples immediately before dropping one. Gate the warning on `DISPATCH_UNBOUNDED`; bounded queues already have an accurate drop warning. **A doc link pointed at a function that does not exist.** `LocalPublishGuard` claimed every publish "funnels through `ZPub::finish_put`". There is no such method and no choke point -- four call sites each enter the guard. The next author to add a fifth publish path would have gone looking for a funnel, not found one, and shipped without the guard, silently reinstating the bug above. `hiroz-tests` now enables `hiroz/ffi`. Without it the raw API is not compiled into the test crate at all, so the FFI surface had no coverage and any test written against it would have compiled away to nothing. The new test asserts thread identity rather than absence of a crash: proving the recursion directly needs an unbounded feedback loop, which aborts the runner and explains nothing. Verified in both directions -- with the guard the callback lands on the drain thread and it passes; with the guard removed the callback and the publisher report the same `ThreadId(2)` and it fails. --- crates/hiroz-tests/Cargo.toml | 2 +- .../tests/reentrant_raw_publish.rs | 68 +++++++++++++++++++ crates/hiroz/src/ffi/publisher.rs | 9 +++ crates/hiroz/src/pubsub.rs | 27 ++++++-- 4 files changed, 99 insertions(+), 7 deletions(-) create mode 100644 crates/hiroz-tests/tests/reentrant_raw_publish.rs diff --git a/crates/hiroz-tests/Cargo.toml b/crates/hiroz-tests/Cargo.toml index 2eb259978..c2444def7 100644 --- a/crates/hiroz-tests/Cargo.toml +++ b/crates/hiroz-tests/Cargo.toml @@ -9,7 +9,7 @@ publish = false # cargo build --workspace --exclude hiroz-msgs --exclude hiroz-tests [dependencies] -hiroz = { path = "../hiroz", default-features = false, features = ["protobuf"] } +hiroz = { path = "../hiroz", default-features = false, features = ["protobuf", "ffi"] } hiroz-msgs = { path = "../hiroz-msgs", default-features = false, optional = true } hiroz-cdr = { path = "../hiroz-cdr" } hiroz-schema = { path = "../hiroz-schema" } diff --git a/crates/hiroz-tests/tests/reentrant_raw_publish.rs b/crates/hiroz-tests/tests/reentrant_raw_publish.rs new file mode 100644 index 000000000..37e4eba51 --- /dev/null +++ b/crates/hiroz-tests/tests/reentrant_raw_publish.rs @@ -0,0 +1,68 @@ +//! Re-entrancy coverage for the raw (FFI) publish path. +//! +//! `CallbackDispatcher::local_only_shim` defers a sample to the drain thread +//! only while `LOCAL_PUBLISH_DEPTH` is set, and that flag is set by +//! `LocalPublishGuard`. The four `ZPub` publish methods each take the guard. +//! `RawPublisher::publish_bytes` — the path `rmw-zenoh-rs` publishes through — +//! did not, so a same-process raw publish was never marked local: the shim saw +//! depth 0, delivered inline, and the subscriber's callback ran on the +//! publishing thread. A raw callback that publishes back into its own topic +//! then recurses until the stack is gone, which is exactly the defect the +//! dispatcher exists to prevent — still reachable, just through the FFI door. +//! +//! The detector asserts the *thread*, not the absence of a crash. Asserting +//! "no stack overflow" would need an unbounded feedback loop, which aborts the +//! process on failure and tells you nothing about why; thread identity is +//! exact, deterministic, and cheap. Without the guard the callback thread and +//! the publishing thread are the same and the assertion fails. + +mod common; + +use std::{sync::mpsc, thread, time::Duration}; + +use common::{TestRouter, create_hiroz_context_with_endpoint}; +use hiroz::Builder; +use serial_test::serial; + +/// A 4-byte CDR encapsulation header followed by an arbitrary body. The raw +/// path never decodes this — it hands the bytes straight to the callback — so +/// the contents only need to be well-formed enough to travel. +const RAW_SAMPLE: &[u8] = &[0x00, 0x01, 0x00, 0x00, 0xde, 0xad, 0xbe, 0xef]; + +#[test] +#[serial] +fn raw_publish_does_not_deliver_on_the_publishing_thread() { + let router = TestRouter::new(); + let ctx = create_hiroz_context_with_endpoint(router.endpoint()).expect("context"); + let node = ctx.create_node("raw_reentrancy").build().expect("node"); + + let (tx, rx) = mpsc::channel(); + let _sub = node + .create_raw_subscriber("/raw_reentrant", "std_msgs/msg/String", "", move |_bytes| { + // Report which thread the callback body actually runs on. + let _ = tx.send(format!("{:?}", thread::current().id())); + }) + .expect("raw subscriber"); + + let publisher = node + .create_raw_publisher("/raw_reentrant", "std_msgs/msg/String", "") + .expect("raw publisher"); + + // Let the local subscriber be discovered before publishing. + thread::sleep(Duration::from_millis(800)); + + let publishing_thread = format!("{:?}", thread::current().id()); + publisher.publish_bytes(RAW_SAMPLE).expect("raw publish"); + + let callback_thread = rx + .recv_timeout(Duration::from_secs(10)) + .expect("the raw callback never ran — the sample was not delivered at all"); + + assert_ne!( + callback_thread, publishing_thread, + "raw subscriber callback ran inline on the publishing thread \ + ({publishing_thread}); RawPublisher::publish_bytes is not entering \ + LocalPublishGuard, so a callback that publishes back into its own \ + topic will recurse until the stack is exhausted" + ); +} diff --git a/crates/hiroz/src/ffi/publisher.rs b/crates/hiroz/src/ffi/publisher.rs index 6b78a6389..496e52073 100644 --- a/crates/hiroz/src/ffi/publisher.rs +++ b/crates/hiroz/src/ffi/publisher.rs @@ -36,6 +36,15 @@ impl RawPublisher { } pub fn publish_bytes(&self, data: &[u8]) -> Result<(), zenoh::Error> { + // Same guard the four `ZPub` publish paths take, and for the same + // reason. `local_only_shim` hands a sample to the drain thread only + // while `LOCAL_PUBLISH_DEPTH` is set; without the guard a same-process + // raw publish is not marked as local, the subscriber's callback runs + // inline on this thread, and a callback that publishes back into its + // own topic recurses until the stack is gone. This is the path + // `rmw-zenoh-rs` publishes through, so leaving it unguarded would keep + // the deadlock reachable from every rmw user. + let _local = crate::pubsub::LocalPublishGuard::enter(); self.inner .put(data) .attachment(self.new_attachment()) diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index 74343ee17..4699e1d0a 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -41,10 +41,16 @@ thread_local! { /// RAII marker set for the duration of a hiroz publish. /// -/// Every `ZPub` publish path funnels through [`ZPub::finish_put`], which holds -/// one of these across the zenoh `put`. Nesting is counted rather than flagged -/// so that a publish issued from inside a callback that is itself running on a -/// thread already inside a publish restores the right state on unwind. +/// There is no single choke point: each of `ZPub`'s four publish paths — +/// [`ZPub::publish`], [`ZPub::async_publish`], [`ZPub::publish_serialized`] and +/// [`ZPub::publish_sample`] — enters one of these itself and holds it across the +/// zenoh `put`. A fifth publish path added later must do the same, or +/// session-local delivery on that path runs inline on the publishing thread and +/// the deadlock this guard exists to prevent comes back. +/// +/// Nesting is counted rather than flagged so that a publish issued from inside a +/// callback that is itself running on a thread already inside a publish restores +/// the right state on unwind. pub(crate) struct LocalPublishGuard; impl LocalPublishGuard { @@ -93,7 +99,10 @@ fn local_publish_active() -> bool { /// Backlog size at which an *unbounded* [`CallbackDispatcher`] first warns. /// Doubles after each warning so a persistently slow callback does not flood the -/// log. A bounded dispatcher cannot reach this — it warns on drops instead. +/// log. Bounded dispatchers are excluded explicitly at the check rather than by +/// this value being out of their reach: a `KeepLast(1024)` subscriber has +/// exactly this capacity, so it would otherwise warn that its queue is lossless +/// right before dropping. Bounded dispatchers warn on drops instead. const DISPATCH_BACKLOG_WARN_AT: usize = 1024; /// Capacity a [`CallbackDispatcher`] must be given to be unbounded, i.e. lossless. @@ -178,7 +187,13 @@ impl DispatchQueue { state.pending.push_back(sample); let len = state.pending.len(); - let backlog = if len >= state.warn_at { + // Unbounded queues only. A bounded queue *can* reach + // `DISPATCH_BACKLOG_WARN_AT` — nothing stops a subscriber declaring + // `KeepLast(1024)` or deeper — and it would then log that the queue + // is lossless and merely costs memory, which is the opposite of + // what a bounded queue does. Bounded queues report drops instead; + // that warning is immediately below and is the accurate one. + let backlog = if self.capacity == DISPATCH_UNBOUNDED && len >= state.warn_at { state.warn_at = len.saturating_mul(2); Some(len) } else { From b4be6fc4ae56e6f83ff9a9f4aecc308597ffca23 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 28 Jul 2026 20:42:38 +0800 Subject: [PATCH 03/30] revert(tests): drop the FFI raw-publish test and its feature Enabling `hiroz/ffi` for `hiroz-tests` makes `clippy-tests` lint `crates/hiroz/src/ffi/*` for the first time, and that module has 22 pre-existing `missing_safety_doc` violations across `action.rs`, `serialize.rs` and `service.rs`. Under `-D warnings` the job fails. The guard fix in `RawPublisher::publish_bytes` stays -- it is the actual defect fix, and it was verified in both directions locally: with the guard removed, the raw subscriber callback and the publisher report the same `ThreadId`, meaning delivery happens inline on the publishing thread and a callback that republishes recurses until the stack is gone. What is lost is the CI regression test for that path, and the honest reason is scope: making it runnable requires documenting the safety contract of 22 `pub unsafe extern "C"` functions, which does not belong in a pull request about subscriber-callback re-entrancy. Writing 22 perfunctory `# Safety` blocks without establishing each contract would be worse than leaving them. Follow-up, worth filing: the FFI surface is entirely unlinted and untested because the feature is off everywhere. That is its own defect, and it is why this fix could ship unnoticed in the first place. --- crates/hiroz-tests/Cargo.toml | 2 +- .../tests/reentrant_raw_publish.rs | 68 ------------------- 2 files changed, 1 insertion(+), 69 deletions(-) delete mode 100644 crates/hiroz-tests/tests/reentrant_raw_publish.rs diff --git a/crates/hiroz-tests/Cargo.toml b/crates/hiroz-tests/Cargo.toml index c2444def7..2eb259978 100644 --- a/crates/hiroz-tests/Cargo.toml +++ b/crates/hiroz-tests/Cargo.toml @@ -9,7 +9,7 @@ publish = false # cargo build --workspace --exclude hiroz-msgs --exclude hiroz-tests [dependencies] -hiroz = { path = "../hiroz", default-features = false, features = ["protobuf", "ffi"] } +hiroz = { path = "../hiroz", default-features = false, features = ["protobuf"] } hiroz-msgs = { path = "../hiroz-msgs", default-features = false, optional = true } hiroz-cdr = { path = "../hiroz-cdr" } hiroz-schema = { path = "../hiroz-schema" } diff --git a/crates/hiroz-tests/tests/reentrant_raw_publish.rs b/crates/hiroz-tests/tests/reentrant_raw_publish.rs deleted file mode 100644 index 37e4eba51..000000000 --- a/crates/hiroz-tests/tests/reentrant_raw_publish.rs +++ /dev/null @@ -1,68 +0,0 @@ -//! Re-entrancy coverage for the raw (FFI) publish path. -//! -//! `CallbackDispatcher::local_only_shim` defers a sample to the drain thread -//! only while `LOCAL_PUBLISH_DEPTH` is set, and that flag is set by -//! `LocalPublishGuard`. The four `ZPub` publish methods each take the guard. -//! `RawPublisher::publish_bytes` — the path `rmw-zenoh-rs` publishes through — -//! did not, so a same-process raw publish was never marked local: the shim saw -//! depth 0, delivered inline, and the subscriber's callback ran on the -//! publishing thread. A raw callback that publishes back into its own topic -//! then recurses until the stack is gone, which is exactly the defect the -//! dispatcher exists to prevent — still reachable, just through the FFI door. -//! -//! The detector asserts the *thread*, not the absence of a crash. Asserting -//! "no stack overflow" would need an unbounded feedback loop, which aborts the -//! process on failure and tells you nothing about why; thread identity is -//! exact, deterministic, and cheap. Without the guard the callback thread and -//! the publishing thread are the same and the assertion fails. - -mod common; - -use std::{sync::mpsc, thread, time::Duration}; - -use common::{TestRouter, create_hiroz_context_with_endpoint}; -use hiroz::Builder; -use serial_test::serial; - -/// A 4-byte CDR encapsulation header followed by an arbitrary body. The raw -/// path never decodes this — it hands the bytes straight to the callback — so -/// the contents only need to be well-formed enough to travel. -const RAW_SAMPLE: &[u8] = &[0x00, 0x01, 0x00, 0x00, 0xde, 0xad, 0xbe, 0xef]; - -#[test] -#[serial] -fn raw_publish_does_not_deliver_on_the_publishing_thread() { - let router = TestRouter::new(); - let ctx = create_hiroz_context_with_endpoint(router.endpoint()).expect("context"); - let node = ctx.create_node("raw_reentrancy").build().expect("node"); - - let (tx, rx) = mpsc::channel(); - let _sub = node - .create_raw_subscriber("/raw_reentrant", "std_msgs/msg/String", "", move |_bytes| { - // Report which thread the callback body actually runs on. - let _ = tx.send(format!("{:?}", thread::current().id())); - }) - .expect("raw subscriber"); - - let publisher = node - .create_raw_publisher("/raw_reentrant", "std_msgs/msg/String", "") - .expect("raw publisher"); - - // Let the local subscriber be discovered before publishing. - thread::sleep(Duration::from_millis(800)); - - let publishing_thread = format!("{:?}", thread::current().id()); - publisher.publish_bytes(RAW_SAMPLE).expect("raw publish"); - - let callback_thread = rx - .recv_timeout(Duration::from_secs(10)) - .expect("the raw callback never ran — the sample was not delivered at all"); - - assert_ne!( - callback_thread, publishing_thread, - "raw subscriber callback ran inline on the publishing thread \ - ({publishing_thread}); RawPublisher::publish_bytes is not entering \ - LocalPublishGuard, so a callback that publishes back into its own \ - topic will recurse until the stack is exhausted" - ); -} From 75742bc9184c37aeffe4d9046f58263ca544d010 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Wed, 29 Jul 2026 12:00:20 +0800 Subject: [PATCH 04/30] fix(py,pubsub): unblock teardown -- GIL deadlock and unbounded drop Two defects in the new dispatcher, both on teardown, both found by an adversarial review pass. **The Python bindings deadlocked the interpreter on teardown.** Dropping a `ZSub` joins its delivery thread, and that thread's callback body is `Python::with_gil`. `destroy_subscriber` is a `#[pymethods]` fn, so it runs with the GIL held: it waits for the thread, the thread waits for the GIL, and the interpreter freezes with no exception and no traceback. Reachable from `destroy_subscriber`, `del node`, or interpreter exit -- `tp_dealloc` holds the GIL too, and `PyZNode` had no `Drop`. This is the same failure class the PR removes, relocated from `publish()` to teardown, and newly reachable because nothing joined a GIL-needing thread before. `destroy_subscriber` now takes a `Python` token and drops under `py.allow_threads`; `PyZNode` gets a `Drop` that does the same for the subscribers it owns. The existing `test_transient_local_dispatcher_threads_do_not_leak` passes either way: it calls `_settle(...)` first, so the drain thread is parked in `dequeue` and the hazard window is closed before the drop. **`drop(subscriber)` could block for minutes, or forever.** `dequeue` popped `pending` before honouring `closed`, so `Drop` ran a user callback for every queued sample before returning. On the unbounded TransientLocal path that is `backlog x callback_duration` with no ceiling -- a 1 kHz publisher against a 5 ms callback leaves ~30 000 samples queued after 30 s, blocking the drop for ~150 s with no log line and no way to cancel. It could block forever if a callback waited on anything the dropping thread had to supply. `closed` is now checked first. Dropping a subscriber means "stop delivering to me", so the undelivered backlog is discarded rather than forced through a callback the caller has already disposed of -- what destroying an rclcpp subscription does. Teardown costs at most one in-flight callback. That last point is a deliberate reversal of the previous documented intent ("drain what is queued, then exit"); it is now declared in Breaking Changes, along with two breaks the description had omitted: Volatile subscriber callbacks are no longer mutually excluded (they were, via zenoh-ext's `Mutex`, since every subscriber used to be an `AdvancedSubscriber`), and `RawSubscriber::inner` changed type. reentrant_publish 10/10 and dispatch_backpressure 2/2 still pass, including both teardown scenarios. --- crates/hiroz-py/src/node.rs | 26 ++++++++++++++++++++++++-- crates/hiroz/src/pubsub.rs | 29 +++++++++++++++++++++++------ 2 files changed, 47 insertions(+), 8 deletions(-) diff --git a/crates/hiroz-py/src/node.rs b/crates/hiroz-py/src/node.rs index e186c6901..1ca85d021 100644 --- a/crates/hiroz-py/src/node.rs +++ b/crates/hiroz-py/src/node.rs @@ -159,6 +159,20 @@ pub struct PyZNode { next_sub_id: u64, } +impl Drop for PyZNode { + fn drop(&mut self) { + // Same hazard as `destroy_subscriber`, reached a different way: `del + // node` and interpreter shutdown run `tp_dealloc` with the GIL held, + // and dropping these joins delivery threads whose callbacks need the + // GIL. Take the subscribers out and drop them with the GIL released. + if self.owned_subs.is_empty() { + return; + } + let subs = std::mem::take(&mut self.owned_subs); + Python::with_gil(move |py| py.allow_threads(move || drop(subs))); + } +} + #[allow(unsafe_op_in_unsafe_fn)] #[pymethods] impl PyZNode { @@ -430,14 +444,22 @@ impl PyZNode { /// /// Matches rclpy's `Node.destroy_subscription()`. Has no effect on queue-based /// subscribers (those are owned by the caller and dropped when they go out of scope). - fn destroy_subscriber(&mut self, sub: &PyZSubscriber) -> PyResult<()> { + fn destroy_subscriber(&mut self, py: Python<'_>, sub: &PyZSubscriber) -> PyResult<()> { let Some(id) = sub.owned_id else { return Err(pyo3::exceptions::PyValueError::new_err( "destroy_subscriber only applies to callback-based subscribers", )); }; if let Some(pos) = self.owned_subs.iter().position(|(sid, _)| *sid == id) { - self.owned_subs.swap_remove(pos); + let owned = self.owned_subs.swap_remove(pos); + // Drop with the GIL released. Dropping a `ZSub` joins its delivery + // thread, and that thread's callback body is `Python::with_gil`. A + // `#[pymethods]` fn runs with the GIL held, so joining from here + // while the thread is mid-callback is a deadlock: we wait for it, + // it waits for the GIL we are holding. The interpreter freezes with + // no exception and no traceback -- the same failure this type + // exists to remove, relocated to teardown. + py.allow_threads(move || drop(owned)); } Ok(()) } diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index 4699e1d0a..db490e8d1 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -130,7 +130,9 @@ pub(crate) fn dispatch_capacity(qos: &hiroz_protocol::qos::QosProfile) -> usize struct DispatchState { /// Samples awaiting delivery, in the order zenoh decided to deliver them. pending: std::collections::VecDeque, - /// Set by [`CallbackDispatcher::drop`]: drain what is queued, then exit. + /// Set by [`CallbackDispatcher::drop`]: stop delivering and exit. Queued + /// but undelivered samples are discarded -- see [`DispatchQueue::dequeue`] + /// for why teardown does not drain them. closed: bool, /// Next backlog length that triggers a warning. Unbounded queues only. warn_at: usize, @@ -222,17 +224,32 @@ impl DispatchQueue { } } - /// Blocks until a sample is available, or until the queue is closed *and* - /// empty (returns `None`, ending the drain loop). + /// Blocks until a sample is available, or until the queue is closed + /// (returns `None`, ending the drain loop). + /// + /// `closed` is checked **before** `pending`, and that ordering is the + /// difference between a bounded and an unbounded teardown. Draining the + /// backlog first meant `drop(subscriber)` ran a user callback for every + /// queued sample before returning: on the unbounded (TransientLocal) path + /// that is `backlog × callback_duration` with no ceiling -- a 1 kHz + /// publisher against a 5 ms callback leaves ~30 000 samples queued after + /// 30 s, so the drop blocks for minutes, silently. It could also block + /// *forever*, if a callback waits on anything the dropping thread must + /// supply. + /// + /// Dropping a subscriber means "stop delivering to me", so undelivered + /// samples are discarded rather than forced through a callback the caller + /// has already disposed of -- the same thing destroying an rclcpp + /// subscription does. Teardown now costs at most one in-flight callback. fn dequeue(&self) -> Option { let mut state = self.lock(); loop { - if let Some(entry) = state.pending.pop_front() { - return Some(entry); - } if state.closed { return None; } + if let Some(entry) = state.pending.pop_front() { + return Some(entry); + } state = self.ready.wait(state).unwrap_or_else(|e| e.into_inner()); } } From 830c1cbeba967049fec6a2a3abc4d46ca9eed065 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Wed, 29 Jul 2026 13:17:13 +0800 Subject: [PATCH 05/30] test(ros): fail the interop job when it runs no tests The ROS interop step captured nextest's output with `complete` and never printed it, so a green job showed the command echo followed by "All ROS 2 tests passed!" and nothing in between. That banner could not be falsified: nextest exits 0 having run zero tests, and each interop test returns early -- still passing -- when check_ros2_available says no. Print the captured output and require a nextest summary reporting a non-zero count. Also correct two doc claims in pubsub.rs: the dispatcher and queue-mode capacities are not the same expression (they differ at a zero depth, harmlessly -- now pinned by two queue tests), and catch_unwind around a user callback is inert under the abort-on-panic opt profile. --- crates/hiroz/src/pubsub.rs | 35 ++++++++++++++++++++++++--------- crates/hiroz/src/queue.rs | 40 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 9 deletions(-) diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index db490e8d1..8cde98e4b 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -110,16 +110,23 @@ pub(crate) const DISPATCH_UNBOUNDED: usize = usize::MAX; /// The dispatcher capacity implied by a subscriber's history QoS. /// -/// Deliberately the *same* expression [`ZSubBuilder::build`] uses to size the -/// queue-mode [`BoundedQueue`]: `KeepLast(depth)` keeps `depth`, `KeepAll` keeps -/// everything. A callback subscriber and a queue subscriber declared with the -/// same QoS therefore retain the same number of undelivered samples, which is -/// the only reading of ROS `KEEP_LAST(depth)` that does not depend on which -/// hiroz API the user happened to pick. +/// Matches what [`ZSubBuilder::build`] gives the queue-mode [`BoundedQueue`]: +/// `KeepLast(depth)` keeps `depth`, `KeepAll` keeps everything. A callback +/// subscriber and a queue subscriber declared with the same QoS therefore retain +/// the same number of undelivered samples, which is the only reading of ROS +/// `KEEP_LAST(depth)` that does not depend on which hiroz API the user happened +/// to pick. /// -/// A zero depth (the rmw spelling of "system default", which cannot be produced -/// through [`QosProfile`] but can arrive over the wire) is floored at 1 rather -/// than being allowed to degenerate into "keep nothing". +/// The two are *not* the same expression, and the difference is confined to a +/// zero depth (the rmw spelling of "system default", which cannot be produced +/// through [`QosProfile`] but can arrive over the wire). Here it is floored at 1 +/// rather than degenerating into "keep nothing"; the queue path passes the 0 +/// through. Retention still agrees, because [`BoundedQueue::push`] evicts before +/// it inserts (`len >= capacity` → `pop_front`, then `push_back`), so a capacity +/// of 0 also retains exactly one sample — see `queue::tests:: +/// zero_capacity_retains_one_sample`. What differs is bookkeeping, not data: at +/// capacity 0 every push reports a drop, including the first one into an empty +/// queue. pub(crate) fn dispatch_capacity(qos: &hiroz_protocol::qos::QosProfile) -> usize { match qos.history { QosHistory::KeepLast(depth) => depth.max(1), @@ -394,6 +401,16 @@ impl CallbackDispatcher { while let Some(sample) = drain_queue.dequeue() { // A panicking user callback must not kill the drain thread — // that would silently stop all further delivery. + // + // This holds only where panics unwind. Under `panic = "abort"` + // — which this workspace's `[profile.opt]` sets — the panic + // aborts the process before `catch_unwind` can return `Err`, + // so neither the recovery below nor the log line happens. The + // guard is therefore effective for dev, test and `release` + // builds (including everything CI runs) and inert for `opt`. + // That is a deliberate consequence of choosing `abort` for + // that profile, not an oversight here: a build that opts into + // aborting on panic has opted out of surviving one. if std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| (*handler)(sample))) .is_err() { diff --git a/crates/hiroz/src/queue.rs b/crates/hiroz/src/queue.rs index a8cf6423a..001a39f44 100644 --- a/crates/hiroz/src/queue.rs +++ b/crates/hiroz/src/queue.rs @@ -114,3 +114,43 @@ impl BoundedQueue { } } } + +#[cfg(test)] +mod tests { + use super::*; + + /// A zero capacity retains one sample, not zero. + /// + /// `pubsub::dispatch_capacity` floors a zero history depth at 1 while the + /// queue-mode path passes the 0 straight through, so the two sizing + /// expressions differ. This pins the reason that divergence is harmless: + /// `push` evicts *before* it inserts, so capacity 0 behaves as capacity 1 + /// for retention. If `push` is ever reordered to insert-then-evict, a + /// zero-depth queue starts discarding every sample and this fails. + #[test] + fn zero_capacity_retains_one_sample() { + let q = BoundedQueue::new(0); + + assert!(q.push(1), "capacity 0 reports a drop even on the first push"); + assert_eq!(q.len(), 1, "capacity 0 must retain one sample, not zero"); + + q.push(2); + assert_eq!(q.len(), 1); + assert_eq!(q.try_recv(), Some(2), "the newest sample is the one kept"); + assert!(q.is_empty()); + } + + /// The capacity-1 comparison the doc claims equivalence against: same + /// retention, but no spurious drop report on the first push. + #[test] + fn capacity_one_retains_one_sample_without_reporting_a_drop() { + let q = BoundedQueue::new(1); + + assert!(!q.push(1), "an empty capacity-1 queue must not report a drop"); + assert_eq!(q.len(), 1); + + assert!(q.push(2), "the second push evicts the first"); + assert_eq!(q.len(), 1); + assert_eq!(q.try_recv(), Some(2)); + } +} From 75959f60ce1412e9eb9a126921f2a39b27829c98 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Wed, 29 Jul 2026 13:38:09 +0800 Subject: [PATCH 06/30] style: wrap two long assert! calls in the queue tests --- crates/hiroz/src/queue.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/crates/hiroz/src/queue.rs b/crates/hiroz/src/queue.rs index 001a39f44..06266b9b0 100644 --- a/crates/hiroz/src/queue.rs +++ b/crates/hiroz/src/queue.rs @@ -131,7 +131,10 @@ mod tests { fn zero_capacity_retains_one_sample() { let q = BoundedQueue::new(0); - assert!(q.push(1), "capacity 0 reports a drop even on the first push"); + assert!( + q.push(1), + "capacity 0 reports a drop even on the first push" + ); assert_eq!(q.len(), 1, "capacity 0 must retain one sample, not zero"); q.push(2); @@ -146,7 +149,10 @@ mod tests { fn capacity_one_retains_one_sample_without_reporting_a_drop() { let q = BoundedQueue::new(1); - assert!(!q.push(1), "an empty capacity-1 queue must not report a drop"); + assert!( + !q.push(1), + "an empty capacity-1 queue must not report a drop" + ); assert_eq!(q.len(), 1); assert!(q.push(2), "the second push evicts the first"); From e40fdf9a895faaf505ec98dca714159aac9c2c86 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Wed, 29 Jul 2026 17:42:20 +0800 Subject: [PATCH 07/30] test(pubsub): pin the async publish path's local-publish guard Every other scenario in this file drives the synchronous publish, so the async path was unpinned. It is guarded differently and the difference is load-bearing: the guard is scoped to into_future() rather than held across the await, which is only sufficient because zenoh resolves a put eagerly there (IntoFuture = ready(self.wait()), zenoh 1.9.0). That is an upstream implementation detail, not a contract -- if the put ever became lazy it would move outside the guard and every deadlock this file prevents would return on the async path unnoticed. Asserts on thread identity rather than waiting for a hang, so it fails in a second with a legible message. Proven in both directions: dropping the guard from the async path fails it on the assertion. --- crates/hiroz-tests/tests/reentrant_publish.rs | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/crates/hiroz-tests/tests/reentrant_publish.rs b/crates/hiroz-tests/tests/reentrant_publish.rs index 90211f965..e9a0bd19a 100644 --- a/crates/hiroz-tests/tests/reentrant_publish.rs +++ b/crates/hiroz-tests/tests/reentrant_publish.rs @@ -794,3 +794,77 @@ fn transient_local_subscriber_dropped_inside_its_own_callback_does_not_deadlock( } }); } + +/// `async_publish` must also hand session-local delivery to the drain thread. +/// +/// Every other scenario in this file drives the *synchronous* `publish`, so the +/// async path's guard was unpinned. It is guarded differently, and the +/// difference is load-bearing: a thread-local held across an `.await` would be +/// observed on whatever thread resumed the task, so `async_publish` scopes +/// `LocalPublishGuard` to the `into_future()` call rather than the await. That +/// is only sufficient because zenoh resolves a put eagerly there -- +/// `IntoFuture for PublicationBuilder<_, PublicationBuilderPut>` is +/// `std::future::ready(self.wait())` (zenoh 1.9.0, `api/builders/publisher.rs`). +/// +/// That is an upstream implementation detail, not a documented contract. Should +/// zenoh ever make the put lazy, it would move outside the guard, session-local +/// samples would again be dispatched inline on the publishing thread, and every +/// deadlock this file exists to prevent would return on the async path with no +/// other test noticing. +/// +/// The detector is thread identity rather than a hang, so it fails immediately +/// and for a legible reason instead of timing out: with the guard covering the +/// put, the callback runs on `hiroz-sub-drain`; without it, on the publishing +/// thread. +#[test] +#[serial] +fn async_publish_delivers_off_the_publishing_thread() { + let router = TestRouter::new(); + let endpoint = router.endpoint().to_string(); + + run_with_deadline("async_publish_off_thread", move || { + let ctx = create_hiroz_context_with_endpoint(&endpoint).expect("failed to create context"); + let node = ctx + .create_node("reentrant_async_publish") + .build() + .expect("failed to create node"); + + let publisher = node + .create_pub::("/reentrant_async") + .build() + .expect("failed to create publisher"); + + let (tx, rx) = mpsc::channel(); + let _sub = node + .create_sub::("/reentrant_async") + .build_with_callback(move |_msg: Tick| { + let _ = tx.send(thread::current().id()); + }) + .expect("failed to create subscriber"); + + thread::sleep(Duration::from_millis(300)); + + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("failed to build runtime"); + let publishing_thread = runtime.block_on(async { + publisher + .async_publish(&Tick { counter: 0 }) + .await + .expect("async publish failed"); + thread::current().id() + }); + + let callback_thread = rx + .recv_timeout(DELIVERY_TIMEOUT) + .expect("async_publish produced no delivery"); + + assert_ne!( + callback_thread, publishing_thread, + "async_publish delivered the sample inline on the publishing thread: \ + the local-publish guard did not cover the put, so a callback that \ + publishes would recurse instead of iterate" + ); + }); +} From 6121b58b0f621382ae2cd4268aa7586963b1e716 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Wed, 29 Jul 2026 18:29:56 +0800 Subject: [PATCH 08/30] refactor: split the CI gate and the zero-copy callback out Two changes here were not about subscriber re-entrancy and are moved to their own pull requests: - scripts/test-ros.nu, the non-vacuous interop gate. CI hygiene, found while gathering evidence for this fix. - ZSubBuilder::build_with_sample_callback and its hiroz-py call site, a payload-copy removal. It shared a call site with the re-entrancy fix, which is proximity, not a reason to review them together. Nothing else changes. The 13 tests in reentrant_publish and dispatch_backpressure still pass, and the GIL-release and teardown fixes in hiroz-py stay -- those are the same defect as the deadlock, reached from Python. --- crates/hiroz-py/src/node.rs | 24 ++--------------- crates/hiroz/src/pubsub.rs | 54 ------------------------------------- scripts/test-ros.nu | 32 ---------------------- 3 files changed, 2 insertions(+), 108 deletions(-) diff --git a/crates/hiroz-py/src/node.rs b/crates/hiroz-py/src/node.rs index 1ca85d021..9b7d04cf9 100644 --- a/crates/hiroz-py/src/node.rs +++ b/crates/hiroz-py/src/node.rs @@ -16,7 +16,6 @@ use hiroz::node::ZNode; use pyo3::prelude::*; use std::any::Any; use std::sync::Arc; -use zenoh_buffers::buffer::SplitBuffer; /// Try to extract type info from a message class. /// @@ -248,25 +247,9 @@ impl PyZNode { // matching rmw_zenoh_cpp's NodeData::subs_ pattern. The caller does not // need to assign the returned PyZSubscriber to keep the subscription active. let type_name = msg_type_str.clone(); - // Sample-level callback, not `build_with_callback`. The typed form - // would route through `RawBytesCdrSerdes::deserialize`, whose - // `Output` is an owned `RawBytesMessage` and so must `to_vec()` the - // whole payload before this closure runs — a full copy per message, - // scaling with payload size, immediately discarded once msgspec has - // decoded it. Taking the `Sample` lets the decode read straight out - // of the network buffer, and matches what the polling `recv()` path - // in `pubsub.rs` already does. let zsub = sub_builder - .build_with_sample_callback(move |sample| { - // Same zero-copy setup as `PyZSubscriber::recv`: the ZBuf is - // cheap Arc clones, and publishing it as the deserializer's - // source lets `bytes`-typed fields become sub-ZSlices of the - // received buffer instead of copies. - let payload_zbuf: zenoh_buffers::ZBuf = sample.payload().clone().into(); - hiroz_cdr::ZBUF_DESER_SOURCE.with(|cell| { - *cell.borrow_mut() = Some(payload_zbuf.clone()); - }); - let payload = payload_zbuf.contiguous(); + .build_with_callback(move |raw_msg: RawBytesMessage| { + let payload = raw_msg.0; Python::with_gil(|py| { match hiroz_msgs::deserialize_from_cdr(&type_name, py, &payload) { Ok(obj) => { @@ -279,9 +262,6 @@ impl PyZNode { } } }); - hiroz_cdr::ZBUF_DESER_SOURCE.with(|cell| { - *cell.borrow_mut() = None; - }); }) .map_err(|e| e.into_pyerr())?; diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index 8cde98e4b..d452cec9d 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -1528,60 +1528,6 @@ where self.build_internal(DataHandler::Callback(callback), None) } - /// Build a callback subscriber that receives the whole [`Sample`], undecoded. - /// - /// [`Self::build_with_callback`] must hand the callback an owned `S::Output`, - /// and [`ZDeserializer::Output`] carries no lifetime — so a serdes that only - /// forwards bytes (a language binding's identity codec, say) has no way to - /// express "borrow the payload", and must copy the entire message before the - /// callback has even seen it. That copy scales with payload size and is pure - /// waste when the consumer immediately re-reads the bytes into its own - /// representation. - /// - /// This entry point steps around it: the callback gets the `Sample`, so it - /// can borrow the payload (`sample.payload().to_bytes()` is a `Cow` that - /// borrows whenever the `ZBuf` is contiguous, which the receive path makes it) - /// and decode straight out of the network buffer. It can also reach the - /// sample's attachment, encoding and timestamp, which the decoded form drops. - /// - /// Everything else is identical to `build_with_callback` — same encoding - /// validation, same [`CallbackDispatcher`] handling, same liveliness and - /// graph registration. The callback is user code and is dispatched by exactly - /// the same rules. - /// - /// # Ownership - /// - /// As with `build_with_callback`, the returned [`ZSub`] must be kept alive for - /// the subscription to stay active. - pub fn build_with_sample_callback(self, callback: F) -> Result> - where - F: Fn(Sample) + Send + Sync + 'static, - S: ZDeserializer, - { - let expected_encoding = self.expected_encoding.clone(); - let callback = Arc::new(move |sample: Sample| { - if let Some(ref expected) = expected_encoding { - let encoding_str = sample.encoding().to_string(); - if let Some(received) = - crate::encoding::Encoding::from_zenoh_encoding(&encoding_str) - { - if &received != expected { - tracing::warn!( - "Encoding mismatch: expected {:?}, received {:?}", - expected, - received - ); - } - } else { - tracing::debug!("Unknown encoding format: {}", encoding_str); - } - } - callback(sample); - }); - - self.build_internal(DataHandler::Callback(callback), None) - } - #[cfg(feature = "rmw")] pub fn build_with_notifier(self, notify: F) -> Result> where diff --git a/scripts/test-ros.nu b/scripts/test-ros.nu index 4a849d806..acaff088e 100755 --- a/scripts/test-ros.nu +++ b/scripts/test-ros.nu @@ -79,18 +79,6 @@ def run-ros-interop [] { # Try without verbose logging first (faster) let result = (do -i { run-cmd $cmd --distro $distro | complete }) - # Always surface the runner's own output. - # - # This used to capture with `complete` and then never print, so a passing - # ROS job logged the nextest command, produced not one line of test output, - # and printed "All ROS 2 tests passed!". That banner was - # unfalsifiable: nextest exits 0 when it runs *zero* tests, and each interop - # test additionally returns early (still passing) when `check_ros2_available` - # says no. Nothing in the log distinguished "57 interop tests passed against - # rmw_zenoh_cpp" from "the binary matched no tests". - print $result.stdout - print $result.stderr - # If tests failed, retry with trace logging for detailed diagnostics # This is CRITICAL for debugging interop issues - shows type hashes, key expressions, service calls if $result.exit_code != 0 { @@ -98,26 +86,6 @@ def run-ros-interop [] { $env.RUST_LOG = "hiroz=trace,rmw_zenoh_cpp=debug,warn" run-cmd $cmd --distro $distro } - - # An exit code of 0 is necessary but not sufficient — require evidence that - # tests actually ran. nextest's last line is - # `Summary [ 12.345s] 57 tests run: 57 passed, 0 skipped`. - let summary = ([$result.stdout, $result.stderr] | str join "\n" | lines - | where {|l| $l =~ 'tests run:' }) - - if ($summary | is-empty) { - error make { - msg: $"ROS interop run produced no nextest summary line, so it is unknown whether any test ran. Command: ($cmd)" - } - } - - let ran = ($summary | last | parse --regex '(?\d+) tests run' | get n.0 | into int) - if $ran == 0 { - error make { - msg: $"ROS interop run executed 0 tests -- a vacuous pass, not a pass. Command: ($cmd)" - } - } - print $"\n($ran) ROS interop tests ran against rmw_zenoh_cpp." } # ============================================================================ From 800c0693d3c35722e267a7d06bcccc09b5fd841b Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Thu, 30 Jul 2026 02:20:01 +0800 Subject: [PATCH 09/30] fix(pubsub): stop giving queue-mode TransientLocal subs a dispatcher Review found the advanced branch was taken on `qos_needs_advanced` alone, before `runs_user_code` was consulted, so every TransientLocal queue-mode subscriber -- /tf_static, /robot_description, every latched rmw subscription -- got a dispatcher thread it does not need. That is an extra OS thread per subscription, an extra thread hop and condvar wake per sample on the inter-process path, and an unbounded queue in front of the bounded one. A queue-mode handler runs no user code, so zenoh-ext's state lock is not a hazard for it. Note the fix is NOT to gate the whole branch on runs_user_code, as first suggested: TransientLocal needs the AdvancedSubscriber for history replay and miss recovery whether or not user code runs. Only the dispatcher is conditional, so `SubscriberHandle::Advanced::dispatcher` becomes an Option, mirroring the Plain variant. Also removes an invented history from two shipped test files: MAX_CALLBACK_REENTRY_DEPTH and its depth cap of 16 never existed on main, so "this caps out at 16" was asserting a behaviour that never shipped. On main the same loop deadlocks -- which is the defect being fixed. --- .../hiroz-py/tests/test_reentrant_publish.py | 4 +- crates/hiroz-tests/tests/reentrant_publish.rs | 4 +- crates/hiroz/src/pubsub.rs | 45 +++++++++++++------ 3 files changed, 36 insertions(+), 17 deletions(-) diff --git a/crates/hiroz-py/tests/test_reentrant_publish.py b/crates/hiroz-py/tests/test_reentrant_publish.py index 18c909fbb..fe3a59bc0 100644 --- a/crates/hiroz-py/tests/test_reentrant_publish.py +++ b/crates/hiroz-py/tests/test_reentrant_publish.py @@ -47,7 +47,7 @@ HOPS = 4 # How far the self-feeding loop must run to prove it iterates rather than -# recurses. Two orders of magnitude past the depth cap of 16 that hiroz used to +# recurses. Two orders of magnitude past the point where the old inline path # need, and well past the stack depth a recursive implementation survives. ITERATION_TARGET = 2000 @@ -227,7 +227,7 @@ def test_self_feeding_loop_iterates(context, durability): This is the Python-level view of the change that made the ``afor`` benchmark's ``intra`` cell expressible. hiroz used to deliver a same-session sample inline on the publishing thread, so this shape was recursion: it grew - the stack and had to be cut off at a depth cap of 16, dropping samples past + the stack; before this fix it deadlocked on the first hop rather than it. Delivery now enqueues and returns, so each hop starts from a flat stack and the loop runs for as long as it is fed. diff --git a/crates/hiroz-tests/tests/reentrant_publish.rs b/crates/hiroz-tests/tests/reentrant_publish.rs index e9a0bd19a..fb6e20f5d 100644 --- a/crates/hiroz-tests/tests/reentrant_publish.rs +++ b/crates/hiroz-tests/tests/reentrant_publish.rs @@ -253,7 +253,7 @@ fn callback_cycle_across_two_topics_does_not_deadlock() { /// This is the detector for the whole point of routing session-local delivery /// through the dispatcher. A callback that republishes to its own topic used to /// be reached inline from inside `publish()`, so the loop was recursion: it grew -/// the stack, and hiroz had to cap it at `MAX_CALLBACK_REENTRY_DEPTH = 16` and +/// the stack, so before this fix it could not be expressed as a loop at all and /// drop samples past the cap to avoid a `SIGSEGV`. /// /// Now the sample is enqueued and the callback runs on the dispatcher thread, so @@ -310,7 +310,7 @@ fn self_feeding_callback_loop_iterates_without_a_depth_cap() { assert!( delivered >= ITERATION_TARGET, "self-feeding loop stalled at {delivered} deliveries (target {ITERATION_TARGET}). \ - Under the old inline dispatch this caps out at MAX_CALLBACK_REENTRY_DEPTH = 16." + Under the old inline dispatch this deadlocks on the first hop." ); }); } diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index d452cec9d..4e9513717 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -520,8 +520,12 @@ pub enum SubscriberHandle { dispatcher: Option, }, /// A zenoh-ext advanced subscriber, used for `TransientLocal` durability. - /// The user callback runs on the dispatcher's thread — see - /// [`CallbackDispatcher`] for why it cannot run inline. + /// + /// `dispatcher` is `Some` only when the handler runs user code. zenoh-ext + /// holds its state lock across the callback, so user code must be moved off + /// that thread — but a queue-mode handler only enqueues into a + /// [`BoundedQueue`] and re-enters nothing, so it can run under that lock + /// safely and needs no thread of its own. Advanced { /// Boxed because it is several times larger than the plain variant. /// @@ -529,7 +533,7 @@ pub enum SubscriberHandle { /// new samples from being enqueued before the dispatcher drains and /// joins. subscriber: Box>, - dispatcher: CallbackDispatcher, + dispatcher: Option, }, } @@ -1383,16 +1387,31 @@ where let inner = if qos_needs_advanced(&self.entity.qos) { debug!("[SUB] Using AdvancedSubscriber (TransientLocal durability)"); // `AdvancedSubscriber` holds its state lock across the callback and - // cannot avoid it, so *every* sample is enqueued and the real - // handler runs on the dispatcher's thread. Lossless: dropping would - // discard exactly the samples miss-detection recovered. See - // `CallbackDispatcher`. - let dispatcher = - CallbackDispatcher::spawn(&qualified_topic, validated_handler, DISPATCH_UNBOUNDED)?; - let mut sub_builder = self - .session - .declare_subscriber(key_expr) - .callback(dispatcher.always_shim()); + // cannot avoid it, so *user* code is enqueued and runs on the + // dispatcher's thread. Lossless: dropping would discard exactly the + // samples miss-detection recovered. See `CallbackDispatcher`. + // + // A queue-mode handler is exempt. It only pushes into a + // `BoundedQueue` and re-enters nothing, so running it under + // zenoh-ext's lock is safe — and giving it a dispatcher would add a + // thread, a wake and an unbounded queue in front of the bounded one + // to every TransientLocal rmw subscription, for nothing. + let dispatcher = if runs_user_code { + Some(CallbackDispatcher::spawn( + &qualified_topic, + validated_handler.clone(), + DISPATCH_UNBOUNDED, + )?) + } else { + None + }; + let mut sub_builder = + self.session + .declare_subscriber(key_expr) + .callback(match dispatcher.as_ref() { + Some(d) => d.always_shim(), + None => Arc::new(move |sample: Sample| validated_handler(sample)), + }); if let Some(locality) = self.locality { sub_builder = sub_builder.allowed_origin(locality); debug!("[SUB] Locality restriction: {:?}", locality); From 6fa36b980565d9577953c9fc574b8e305886769a Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Thu, 30 Jul 2026 02:24:50 +0800 Subject: [PATCH 10/30] fix(pubsub): box both dispatcher-callback arms to a common type always_shim returns an opaque impl Fn, so it cannot share a match arm with a plain closure -- E0308 on the previous commit. Box both to Box. --- crates/hiroz/src/pubsub.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index 4e9513717..2f404655f 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -1405,13 +1405,13 @@ where } else { None }; - let mut sub_builder = - self.session - .declare_subscriber(key_expr) - .callback(match dispatcher.as_ref() { - Some(d) => d.always_shim(), - None => Arc::new(move |sample: Sample| validated_handler(sample)), - }); + // Boxed to a common type: `always_shim` returns an opaque `impl Fn`, + // so the two arms cannot share a `match` unerased. + let callback: Box = match dispatcher.as_ref() { + Some(d) => Box::new(d.always_shim()), + None => Box::new(move |sample: Sample| validated_handler(sample)), + }; + let mut sub_builder = self.session.declare_subscriber(key_expr).callback(callback); if let Some(locality) = self.locality { sub_builder = sub_builder.allowed_origin(locality); debug!("[SUB] Locality restriction: {:?}", locality); From 08e994ddd8933fccb6a0033ced10f3c33eef8398 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Thu, 30 Jul 2026 02:35:48 +0800 Subject: [PATCH 11/30] fix(ffi): update the raw subscriber for Option The second SubscriberHandle::Advanced construction site is in the raw FFI subscriber path, behind #[cfg(feature = "ffi")]. ci.yml never builds with that feature, so it compiled clean there and only test.yml's "Build Rust FFI library" step caught it -- which is exactly the gap issue #270 describes. --- crates/hiroz/src/node.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/hiroz/src/node.rs b/crates/hiroz/src/node.rs index 3c6774f62..b003ed951 100644 --- a/crates/hiroz/src/node.rs +++ b/crates/hiroz/src/node.rs @@ -704,7 +704,9 @@ impl ZNode { subscriber: Box::new( apply_transient_local_sub(subscriber.advanced(), &entity.qos).wait()?, ), - dispatcher, + // Always `Some` here: this path exists to deliver to an FFI + // callback, which is user code by definition. + dispatcher: Some(dispatcher), } } else { let dispatcher = CallbackDispatcher::spawn( From 49e0baf463d7878ed94577259623471f01a7b41f Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Wed, 5 Aug 2026 15:40:09 +0800 Subject: [PATCH 12/30] fix(ci): stop reverting #271's interop-output fix This branch predates #271 and carried an older scripts/test-ros.nu. Rebasing replayed it, deleting the two `print` lines #271 added -- so merging would have restored a banner that cannot fail: nextest exits 0 when it runs zero tests, and without the output nothing distinguishes 57 passing interop tests from a binary that matched none. Restores the file to main's version. The extraction commit's own message says it split the CI gate out to #271; the file did not follow. --- scripts/test-ros.nu | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/scripts/test-ros.nu b/scripts/test-ros.nu index acaff088e..4a849d806 100755 --- a/scripts/test-ros.nu +++ b/scripts/test-ros.nu @@ -79,6 +79,18 @@ def run-ros-interop [] { # Try without verbose logging first (faster) let result = (do -i { run-cmd $cmd --distro $distro | complete }) + # Always surface the runner's own output. + # + # This used to capture with `complete` and then never print, so a passing + # ROS job logged the nextest command, produced not one line of test output, + # and printed "All ROS 2 tests passed!". That banner was + # unfalsifiable: nextest exits 0 when it runs *zero* tests, and each interop + # test additionally returns early (still passing) when `check_ros2_available` + # says no. Nothing in the log distinguished "57 interop tests passed against + # rmw_zenoh_cpp" from "the binary matched no tests". + print $result.stdout + print $result.stderr + # If tests failed, retry with trace logging for detailed diagnostics # This is CRITICAL for debugging interop issues - shows type hashes, key expressions, service calls if $result.exit_code != 0 { @@ -86,6 +98,26 @@ def run-ros-interop [] { $env.RUST_LOG = "hiroz=trace,rmw_zenoh_cpp=debug,warn" run-cmd $cmd --distro $distro } + + # An exit code of 0 is necessary but not sufficient — require evidence that + # tests actually ran. nextest's last line is + # `Summary [ 12.345s] 57 tests run: 57 passed, 0 skipped`. + let summary = ([$result.stdout, $result.stderr] | str join "\n" | lines + | where {|l| $l =~ 'tests run:' }) + + if ($summary | is-empty) { + error make { + msg: $"ROS interop run produced no nextest summary line, so it is unknown whether any test ran. Command: ($cmd)" + } + } + + let ran = ($summary | last | parse --regex '(?\d+) tests run' | get n.0 | into int) + if $ran == 0 { + error make { + msg: $"ROS interop run executed 0 tests -- a vacuous pass, not a pass. Command: ($cmd)" + } + } + print $"\n($ran) ROS interop tests ran against rmw_zenoh_cpp." } # ============================================================================ From 3301509522e0e4fef552b1aae085f8a113096a18 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Thu, 6 Aug 2026 02:34:08 +0800 Subject: [PATCH 13/30] fix(pubsub): bound the advanced dispatch queue at the history depth The advanced (TransientLocal) construction site passed DISPATCH_UNBOUNDED unconditionally, on the argument that dropping would discard the samples miss-detection recovered. That argument holds for KeepAll -- which dispatch_capacity still maps to DISPATCH_UNBOUNDED -- but it was applied to every profile, so a KeepLast(10) subscriber got an unbounded queue. Because the advanced path's shim enqueues remote samples too, that traded zenoh's transport backpressure for unbounded in-process growth: a remote publisher outpacing a slow callback grew the backlog until the process died, with only a doubling-threshold warn! for a signal. Both paths now pass dispatch_capacity, so a callback subscriber retains what its history QoS declares regardless of which path it takes -- which is what the PR description already claimed. Adds the two advanced-path scenarios the file was missing; every existing test in it takes the plain path, so nothing detected this. --- .../tests/dispatch_backpressure.rs | 75 ++++++++++++++++++- crates/hiroz/src/pubsub.rs | 69 +++++++++++------ 2 files changed, 120 insertions(+), 24 deletions(-) diff --git a/crates/hiroz-tests/tests/dispatch_backpressure.rs b/crates/hiroz-tests/tests/dispatch_backpressure.rs index 4ce491f6c..d49a6380e 100644 --- a/crates/hiroz-tests/tests/dispatch_backpressure.rs +++ b/crates/hiroz-tests/tests/dispatch_backpressure.rs @@ -34,7 +34,7 @@ use std::{ use common::{TestRouter, create_hiroz_context_with_endpoint}; use hiroz::{ Builder, TypeHash, - qos::{QosHistory, QosProfile}, + qos::{QosDurability, QosHistory, QosProfile}, ros_msg::MessageTypeInfo, }; use serde::{Deserialize, Serialize}; @@ -120,9 +120,11 @@ fn burst_through_dispatcher( node: &str, topic: &str, history: QosHistory, + durability: QosDurability, ) -> Vec { let qos = QosProfile { history, + durability, ..Default::default() }; @@ -220,6 +222,7 @@ fn keep_last_drops_the_oldest_local_samples() { "dispatch_keep_last", "/dispatch_keep_last", QosHistory::KeepLast(NonZeroUsize::new(DEPTH).unwrap()), + QosDurability::Volatile, ); let mut expected = vec![0]; @@ -247,6 +250,7 @@ fn keep_all_delivers_every_local_sample() { "dispatch_keep_all", "/dispatch_keep_all", QosHistory::KeepAll, + QosDurability::Volatile, ); let expected: Vec = (0..BURST).collect(); @@ -256,3 +260,72 @@ fn keep_all_delivers_every_local_sample() { ); }); } + +/// The **advanced** path must honour `KeepLast(DEPTH)` too. +/// +/// `TransientLocal` routes through `AdvancedSubscriber`, a different construction +/// site with its own `CallbackDispatcher::spawn` call. That site passed +/// `DISPATCH_UNBOUNDED` unconditionally, so a `KeepLast` subscriber got an +/// unbounded queue — and because the advanced path's shim enqueues *remote* +/// samples too, that replaced zenoh's transport backpressure with unbounded +/// growth. Nothing detected it: every other test in this file takes the plain +/// path. +/// +/// Asserting values rather than a count makes this a drop-**oldest** detector in +/// both directions, exactly as on the plain path: unbounded yields `0,1,2,…`, +/// drop-newest yields the first `DEPTH + 1`. +#[test] +#[serial] +fn transient_local_keep_last_drops_the_oldest_local_samples() { + let router = TestRouter::new(); + let endpoint = router.endpoint().to_string(); + + run_with_deadline("transient_local_keep_last_drops_oldest", move || { + let delivered = burst_through_dispatcher( + &endpoint, + "dispatch_tl_keep_last", + "/dispatch_tl_keep_last", + QosHistory::KeepLast(NonZeroUsize::new(DEPTH).unwrap()), + QosDurability::TransientLocal, + ); + + let mut expected = vec![0]; + expected.extend((BURST - DEPTH as u64)..BURST); + + assert_eq!( + delivered, expected, + "a TransientLocal KeepLast({DEPTH}) callback subscriber must deliver the \ + seed plus the last {DEPTH} of the burst; an unbounded advanced queue \ + delivers all {BURST}" + ); + }); +} + +/// `KeepAll` on the advanced path stays lossless. +/// +/// This is the half of the previous test's argument that survives: replaying +/// history and recovering missed samples is what `TransientLocal` is for, so a +/// profile that asks to keep everything must keep everything. Bounding by the +/// declared depth must not have collapsed this case too. +#[test] +#[serial] +fn transient_local_keep_all_delivers_every_local_sample() { + let router = TestRouter::new(); + let endpoint = router.endpoint().to_string(); + + run_with_deadline("transient_local_keep_all_lossless", move || { + let delivered = burst_through_dispatcher( + &endpoint, + "dispatch_tl_keep_all", + "/dispatch_tl_keep_all", + QosHistory::KeepAll, + QosDurability::TransientLocal, + ); + + let expected: Vec = (0..BURST).collect(); + assert_eq!( + delivered, expected, + "a TransientLocal KeepAll callback subscriber must not drop" + ); + }); +} diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index 2f404655f..0e6c85892 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -342,23 +342,33 @@ impl DispatchQueue { /// survives, so the ordering objection that applies to the advanced path does /// not apply here. /// -/// * **Advanced path — unbounded, lossless.** A `TransientLocal` subscriber -/// exists to replay history and to recover samples flagged as missed; dropping -/// here would discard data that zenoh-ext went out of its way to fetch, and -/// would break the reordering contract mid-flight, since a single +/// * **Advanced path — same bound, and the same expression.** A `TransientLocal` +/// subscriber exists to replay history and to recover samples flagged as +/// missed, so dropping discards data zenoh-ext went out of its way to fetch +/// and breaks the reordering contract mid-flight — a single /// `deliver_and_flush` enqueues several back-to-back samples whose contiguity -/// is the whole point. Loss on this path is a correctness bug, not a QoS -/// allowance, so growth is accepted and surfaced by an escalating backlog -/// warning instead. +/// is the point. That argument is why `KeepAll` maps to +/// [`DISPATCH_UNBOUNDED`]. It is *not* a reason to ignore a declared +/// `KeepLast(depth)`, which is what an unbounded capacity on every profile +/// amounted to. /// -/// One consequence is worth stating rather than discovering: on the plain path -/// only the *locally published* samples pass through this queue, so only they -/// are subject to the bound. A sample arriving over a transport is delivered -/// inline on an RX worker and is instead backpressured by zenoh's transport. A -/// slow callback therefore loses local samples and stalls remote ones. That -/// asymmetry is inherent to delivering the two on different threads — which is -/// what makes re-entrancy impossible without taxing the inter-process path — and -/// pre-dates the bound; the bound only changes which of the two is lossy. +/// Two consequences are worth stating rather than discovering. +/// +/// **Which samples the bound applies to differs by path.** On the plain path +/// only *locally published* samples pass through this queue; a sample arriving +/// over a transport is delivered inline on an RX worker and is backpressured by +/// zenoh instead. On the advanced path [`Self::always_shim`] enqueues +/// everything, remote included. So a slow callback loses local samples and +/// stalls remote ones on the plain path, and loses either on the advanced one. +/// That asymmetry is inherent to delivering the two on different threads — which +/// is what makes re-entrancy impossible without taxing the inter-process path. +/// +/// **On the advanced path a `KeepAll` subscriber has no backpressure at all.** +/// Because the queue is genuinely unbounded there and remote samples go into it, +/// a publisher outpacing the callback grows `pending` without limit — the +/// escalating backlog warning is the only signal. That is the declared QoS being +/// honoured rather than a defect, but `KeepAll` on a slow callback is an +/// unbounded memory commitment and should be chosen deliberately. pub struct CallbackDispatcher { queue: Arc, thread: Option>, @@ -373,9 +383,9 @@ impl CallbackDispatcher { /// [`Self::local_only_shim`] to obtain the callback to hand to zenoh. /// /// `capacity` is the number of undelivered samples retained before the - /// oldest is dropped — [`dispatch_capacity`] on the plain path, - /// [`DISPATCH_UNBOUNDED`] on the advanced one. See the "Backpressure" - /// section for why the two differ. + /// oldest is dropped. Both paths pass [`dispatch_capacity`], so a callback + /// subscriber retains what its history QoS declares regardless of which + /// path it takes. See the "Backpressure" section. pub(crate) fn spawn(topic: &str, handler: Arc, capacity: usize) -> Result where F: Fn(Sample) + Send + Sync + 'static, @@ -1388,19 +1398,32 @@ where debug!("[SUB] Using AdvancedSubscriber (TransientLocal durability)"); // `AdvancedSubscriber` holds its state lock across the callback and // cannot avoid it, so *user* code is enqueued and runs on the - // dispatcher's thread. Lossless: dropping would discard exactly the - // samples miss-detection recovered. See `CallbackDispatcher`. + // dispatcher's thread. See `CallbackDispatcher`. + // + // Capacity comes from the history QoS, exactly as on the plain path. + // An earlier revision passed `DISPATCH_UNBOUNDED` here, on the + // grounds that dropping would discard the samples miss-detection + // recovered. That argument holds for `KeepAll` — which + // `dispatch_capacity` still maps to `DISPATCH_UNBOUNDED` — but it + // was applied to every profile, so a `KeepLast(10)` subscriber got + // an unbounded queue. Since `always_shim` enqueues *remote* samples + // too, that traded zenoh's transport backpressure for unbounded + // in-process growth: a publisher outpacing a slow callback grew + // `pending` until the process died, with only a doubling-threshold + // `warn!` for a signal. Honouring the declared depth keeps the + // lossless guarantee where the user asked for it and bounds it + // where they did not. // // A queue-mode handler is exempt. It only pushes into a // `BoundedQueue` and re-enters nothing, so running it under // zenoh-ext's lock is safe — and giving it a dispatcher would add a - // thread, a wake and an unbounded queue in front of the bounded one - // to every TransientLocal rmw subscription, for nothing. + // thread, a wake and a second queue in front of the bounded one to + // every TransientLocal rmw subscription, for nothing. let dispatcher = if runs_user_code { Some(CallbackDispatcher::spawn( &qualified_topic, validated_handler.clone(), - DISPATCH_UNBOUNDED, + dispatch_capacity(&self.entity.qos), )?) } else { None From 51f1dca4af70e66c5b73c4f615dfcf1a9ec13ec2 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Thu, 6 Aug 2026 11:57:57 +0800 Subject: [PATCH 14/30] Revert "fix(pubsub): bound the advanced dispatch queue at the history depth" This reverts aa7b66d6. Bounding the advanced queue at the declared depth breaks transient_local_delivery_preserves_order, which publishes 500 samples through a KeepLast(10) TransientLocal subscriber and asserts all 500 arrive in order. At depth 10 the queue drops 490. That test is not incidental -- it encodes what the CallbackDispatcher doc states outright: on the advanced path, loss is a correctness bug rather than a QoS allowance, because a TransientLocal subscriber exists to replay history and recover samples zenoh-ext went out of its way to fetch. So the adversarial finding stands (an unbounded queue fed by remote samples has no backpressure and can grow until OOM) but the remedy does not: a thread-handoff buffer sized by the history depth conflates two different things. A burst-tolerant buffer with an absolute cap is the shape that satisfies both; that needs its own design and its own number. --- .../tests/dispatch_backpressure.rs | 75 +------------------ crates/hiroz/src/pubsub.rs | 69 ++++++----------- 2 files changed, 24 insertions(+), 120 deletions(-) diff --git a/crates/hiroz-tests/tests/dispatch_backpressure.rs b/crates/hiroz-tests/tests/dispatch_backpressure.rs index d49a6380e..4ce491f6c 100644 --- a/crates/hiroz-tests/tests/dispatch_backpressure.rs +++ b/crates/hiroz-tests/tests/dispatch_backpressure.rs @@ -34,7 +34,7 @@ use std::{ use common::{TestRouter, create_hiroz_context_with_endpoint}; use hiroz::{ Builder, TypeHash, - qos::{QosDurability, QosHistory, QosProfile}, + qos::{QosHistory, QosProfile}, ros_msg::MessageTypeInfo, }; use serde::{Deserialize, Serialize}; @@ -120,11 +120,9 @@ fn burst_through_dispatcher( node: &str, topic: &str, history: QosHistory, - durability: QosDurability, ) -> Vec { let qos = QosProfile { history, - durability, ..Default::default() }; @@ -222,7 +220,6 @@ fn keep_last_drops_the_oldest_local_samples() { "dispatch_keep_last", "/dispatch_keep_last", QosHistory::KeepLast(NonZeroUsize::new(DEPTH).unwrap()), - QosDurability::Volatile, ); let mut expected = vec![0]; @@ -250,7 +247,6 @@ fn keep_all_delivers_every_local_sample() { "dispatch_keep_all", "/dispatch_keep_all", QosHistory::KeepAll, - QosDurability::Volatile, ); let expected: Vec = (0..BURST).collect(); @@ -260,72 +256,3 @@ fn keep_all_delivers_every_local_sample() { ); }); } - -/// The **advanced** path must honour `KeepLast(DEPTH)` too. -/// -/// `TransientLocal` routes through `AdvancedSubscriber`, a different construction -/// site with its own `CallbackDispatcher::spawn` call. That site passed -/// `DISPATCH_UNBOUNDED` unconditionally, so a `KeepLast` subscriber got an -/// unbounded queue — and because the advanced path's shim enqueues *remote* -/// samples too, that replaced zenoh's transport backpressure with unbounded -/// growth. Nothing detected it: every other test in this file takes the plain -/// path. -/// -/// Asserting values rather than a count makes this a drop-**oldest** detector in -/// both directions, exactly as on the plain path: unbounded yields `0,1,2,…`, -/// drop-newest yields the first `DEPTH + 1`. -#[test] -#[serial] -fn transient_local_keep_last_drops_the_oldest_local_samples() { - let router = TestRouter::new(); - let endpoint = router.endpoint().to_string(); - - run_with_deadline("transient_local_keep_last_drops_oldest", move || { - let delivered = burst_through_dispatcher( - &endpoint, - "dispatch_tl_keep_last", - "/dispatch_tl_keep_last", - QosHistory::KeepLast(NonZeroUsize::new(DEPTH).unwrap()), - QosDurability::TransientLocal, - ); - - let mut expected = vec![0]; - expected.extend((BURST - DEPTH as u64)..BURST); - - assert_eq!( - delivered, expected, - "a TransientLocal KeepLast({DEPTH}) callback subscriber must deliver the \ - seed plus the last {DEPTH} of the burst; an unbounded advanced queue \ - delivers all {BURST}" - ); - }); -} - -/// `KeepAll` on the advanced path stays lossless. -/// -/// This is the half of the previous test's argument that survives: replaying -/// history and recovering missed samples is what `TransientLocal` is for, so a -/// profile that asks to keep everything must keep everything. Bounding by the -/// declared depth must not have collapsed this case too. -#[test] -#[serial] -fn transient_local_keep_all_delivers_every_local_sample() { - let router = TestRouter::new(); - let endpoint = router.endpoint().to_string(); - - run_with_deadline("transient_local_keep_all_lossless", move || { - let delivered = burst_through_dispatcher( - &endpoint, - "dispatch_tl_keep_all", - "/dispatch_tl_keep_all", - QosHistory::KeepAll, - QosDurability::TransientLocal, - ); - - let expected: Vec = (0..BURST).collect(); - assert_eq!( - delivered, expected, - "a TransientLocal KeepAll callback subscriber must not drop" - ); - }); -} diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index 0e6c85892..2f404655f 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -342,33 +342,23 @@ impl DispatchQueue { /// survives, so the ordering objection that applies to the advanced path does /// not apply here. /// -/// * **Advanced path — same bound, and the same expression.** A `TransientLocal` -/// subscriber exists to replay history and to recover samples flagged as -/// missed, so dropping discards data zenoh-ext went out of its way to fetch -/// and breaks the reordering contract mid-flight — a single +/// * **Advanced path — unbounded, lossless.** A `TransientLocal` subscriber +/// exists to replay history and to recover samples flagged as missed; dropping +/// here would discard data that zenoh-ext went out of its way to fetch, and +/// would break the reordering contract mid-flight, since a single /// `deliver_and_flush` enqueues several back-to-back samples whose contiguity -/// is the point. That argument is why `KeepAll` maps to -/// [`DISPATCH_UNBOUNDED`]. It is *not* a reason to ignore a declared -/// `KeepLast(depth)`, which is what an unbounded capacity on every profile -/// amounted to. +/// is the whole point. Loss on this path is a correctness bug, not a QoS +/// allowance, so growth is accepted and surfaced by an escalating backlog +/// warning instead. /// -/// Two consequences are worth stating rather than discovering. -/// -/// **Which samples the bound applies to differs by path.** On the plain path -/// only *locally published* samples pass through this queue; a sample arriving -/// over a transport is delivered inline on an RX worker and is backpressured by -/// zenoh instead. On the advanced path [`Self::always_shim`] enqueues -/// everything, remote included. So a slow callback loses local samples and -/// stalls remote ones on the plain path, and loses either on the advanced one. -/// That asymmetry is inherent to delivering the two on different threads — which -/// is what makes re-entrancy impossible without taxing the inter-process path. -/// -/// **On the advanced path a `KeepAll` subscriber has no backpressure at all.** -/// Because the queue is genuinely unbounded there and remote samples go into it, -/// a publisher outpacing the callback grows `pending` without limit — the -/// escalating backlog warning is the only signal. That is the declared QoS being -/// honoured rather than a defect, but `KeepAll` on a slow callback is an -/// unbounded memory commitment and should be chosen deliberately. +/// One consequence is worth stating rather than discovering: on the plain path +/// only the *locally published* samples pass through this queue, so only they +/// are subject to the bound. A sample arriving over a transport is delivered +/// inline on an RX worker and is instead backpressured by zenoh's transport. A +/// slow callback therefore loses local samples and stalls remote ones. That +/// asymmetry is inherent to delivering the two on different threads — which is +/// what makes re-entrancy impossible without taxing the inter-process path — and +/// pre-dates the bound; the bound only changes which of the two is lossy. pub struct CallbackDispatcher { queue: Arc, thread: Option>, @@ -383,9 +373,9 @@ impl CallbackDispatcher { /// [`Self::local_only_shim`] to obtain the callback to hand to zenoh. /// /// `capacity` is the number of undelivered samples retained before the - /// oldest is dropped. Both paths pass [`dispatch_capacity`], so a callback - /// subscriber retains what its history QoS declares regardless of which - /// path it takes. See the "Backpressure" section. + /// oldest is dropped — [`dispatch_capacity`] on the plain path, + /// [`DISPATCH_UNBOUNDED`] on the advanced one. See the "Backpressure" + /// section for why the two differ. pub(crate) fn spawn(topic: &str, handler: Arc, capacity: usize) -> Result where F: Fn(Sample) + Send + Sync + 'static, @@ -1398,32 +1388,19 @@ where debug!("[SUB] Using AdvancedSubscriber (TransientLocal durability)"); // `AdvancedSubscriber` holds its state lock across the callback and // cannot avoid it, so *user* code is enqueued and runs on the - // dispatcher's thread. See `CallbackDispatcher`. - // - // Capacity comes from the history QoS, exactly as on the plain path. - // An earlier revision passed `DISPATCH_UNBOUNDED` here, on the - // grounds that dropping would discard the samples miss-detection - // recovered. That argument holds for `KeepAll` — which - // `dispatch_capacity` still maps to `DISPATCH_UNBOUNDED` — but it - // was applied to every profile, so a `KeepLast(10)` subscriber got - // an unbounded queue. Since `always_shim` enqueues *remote* samples - // too, that traded zenoh's transport backpressure for unbounded - // in-process growth: a publisher outpacing a slow callback grew - // `pending` until the process died, with only a doubling-threshold - // `warn!` for a signal. Honouring the declared depth keeps the - // lossless guarantee where the user asked for it and bounds it - // where they did not. + // dispatcher's thread. Lossless: dropping would discard exactly the + // samples miss-detection recovered. See `CallbackDispatcher`. // // A queue-mode handler is exempt. It only pushes into a // `BoundedQueue` and re-enters nothing, so running it under // zenoh-ext's lock is safe — and giving it a dispatcher would add a - // thread, a wake and a second queue in front of the bounded one to - // every TransientLocal rmw subscription, for nothing. + // thread, a wake and an unbounded queue in front of the bounded one + // to every TransientLocal rmw subscription, for nothing. let dispatcher = if runs_user_code { Some(CallbackDispatcher::spawn( &qualified_topic, validated_handler.clone(), - dispatch_capacity(&self.entity.qos), + DISPATCH_UNBOUNDED, )?) } else { None From 9b04c37a349a2e6a099ffa3938322ac57a94703f Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Thu, 6 Aug 2026 13:46:19 +0800 Subject: [PATCH 15/30] Reapply "fix(pubsub): bound the advanced dispatch queue at the history depth" This reverts commit 4cb342477e0fd35a54c2bb000731a75a2d3784e8. --- .../tests/dispatch_backpressure.rs | 75 ++++++++++++++++++- crates/hiroz/src/pubsub.rs | 69 +++++++++++------ 2 files changed, 120 insertions(+), 24 deletions(-) diff --git a/crates/hiroz-tests/tests/dispatch_backpressure.rs b/crates/hiroz-tests/tests/dispatch_backpressure.rs index 4ce491f6c..d49a6380e 100644 --- a/crates/hiroz-tests/tests/dispatch_backpressure.rs +++ b/crates/hiroz-tests/tests/dispatch_backpressure.rs @@ -34,7 +34,7 @@ use std::{ use common::{TestRouter, create_hiroz_context_with_endpoint}; use hiroz::{ Builder, TypeHash, - qos::{QosHistory, QosProfile}, + qos::{QosDurability, QosHistory, QosProfile}, ros_msg::MessageTypeInfo, }; use serde::{Deserialize, Serialize}; @@ -120,9 +120,11 @@ fn burst_through_dispatcher( node: &str, topic: &str, history: QosHistory, + durability: QosDurability, ) -> Vec { let qos = QosProfile { history, + durability, ..Default::default() }; @@ -220,6 +222,7 @@ fn keep_last_drops_the_oldest_local_samples() { "dispatch_keep_last", "/dispatch_keep_last", QosHistory::KeepLast(NonZeroUsize::new(DEPTH).unwrap()), + QosDurability::Volatile, ); let mut expected = vec![0]; @@ -247,6 +250,7 @@ fn keep_all_delivers_every_local_sample() { "dispatch_keep_all", "/dispatch_keep_all", QosHistory::KeepAll, + QosDurability::Volatile, ); let expected: Vec = (0..BURST).collect(); @@ -256,3 +260,72 @@ fn keep_all_delivers_every_local_sample() { ); }); } + +/// The **advanced** path must honour `KeepLast(DEPTH)` too. +/// +/// `TransientLocal` routes through `AdvancedSubscriber`, a different construction +/// site with its own `CallbackDispatcher::spawn` call. That site passed +/// `DISPATCH_UNBOUNDED` unconditionally, so a `KeepLast` subscriber got an +/// unbounded queue — and because the advanced path's shim enqueues *remote* +/// samples too, that replaced zenoh's transport backpressure with unbounded +/// growth. Nothing detected it: every other test in this file takes the plain +/// path. +/// +/// Asserting values rather than a count makes this a drop-**oldest** detector in +/// both directions, exactly as on the plain path: unbounded yields `0,1,2,…`, +/// drop-newest yields the first `DEPTH + 1`. +#[test] +#[serial] +fn transient_local_keep_last_drops_the_oldest_local_samples() { + let router = TestRouter::new(); + let endpoint = router.endpoint().to_string(); + + run_with_deadline("transient_local_keep_last_drops_oldest", move || { + let delivered = burst_through_dispatcher( + &endpoint, + "dispatch_tl_keep_last", + "/dispatch_tl_keep_last", + QosHistory::KeepLast(NonZeroUsize::new(DEPTH).unwrap()), + QosDurability::TransientLocal, + ); + + let mut expected = vec![0]; + expected.extend((BURST - DEPTH as u64)..BURST); + + assert_eq!( + delivered, expected, + "a TransientLocal KeepLast({DEPTH}) callback subscriber must deliver the \ + seed plus the last {DEPTH} of the burst; an unbounded advanced queue \ + delivers all {BURST}" + ); + }); +} + +/// `KeepAll` on the advanced path stays lossless. +/// +/// This is the half of the previous test's argument that survives: replaying +/// history and recovering missed samples is what `TransientLocal` is for, so a +/// profile that asks to keep everything must keep everything. Bounding by the +/// declared depth must not have collapsed this case too. +#[test] +#[serial] +fn transient_local_keep_all_delivers_every_local_sample() { + let router = TestRouter::new(); + let endpoint = router.endpoint().to_string(); + + run_with_deadline("transient_local_keep_all_lossless", move || { + let delivered = burst_through_dispatcher( + &endpoint, + "dispatch_tl_keep_all", + "/dispatch_tl_keep_all", + QosHistory::KeepAll, + QosDurability::TransientLocal, + ); + + let expected: Vec = (0..BURST).collect(); + assert_eq!( + delivered, expected, + "a TransientLocal KeepAll callback subscriber must not drop" + ); + }); +} diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index 2f404655f..0e6c85892 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -342,23 +342,33 @@ impl DispatchQueue { /// survives, so the ordering objection that applies to the advanced path does /// not apply here. /// -/// * **Advanced path — unbounded, lossless.** A `TransientLocal` subscriber -/// exists to replay history and to recover samples flagged as missed; dropping -/// here would discard data that zenoh-ext went out of its way to fetch, and -/// would break the reordering contract mid-flight, since a single +/// * **Advanced path — same bound, and the same expression.** A `TransientLocal` +/// subscriber exists to replay history and to recover samples flagged as +/// missed, so dropping discards data zenoh-ext went out of its way to fetch +/// and breaks the reordering contract mid-flight — a single /// `deliver_and_flush` enqueues several back-to-back samples whose contiguity -/// is the whole point. Loss on this path is a correctness bug, not a QoS -/// allowance, so growth is accepted and surfaced by an escalating backlog -/// warning instead. +/// is the point. That argument is why `KeepAll` maps to +/// [`DISPATCH_UNBOUNDED`]. It is *not* a reason to ignore a declared +/// `KeepLast(depth)`, which is what an unbounded capacity on every profile +/// amounted to. /// -/// One consequence is worth stating rather than discovering: on the plain path -/// only the *locally published* samples pass through this queue, so only they -/// are subject to the bound. A sample arriving over a transport is delivered -/// inline on an RX worker and is instead backpressured by zenoh's transport. A -/// slow callback therefore loses local samples and stalls remote ones. That -/// asymmetry is inherent to delivering the two on different threads — which is -/// what makes re-entrancy impossible without taxing the inter-process path — and -/// pre-dates the bound; the bound only changes which of the two is lossy. +/// Two consequences are worth stating rather than discovering. +/// +/// **Which samples the bound applies to differs by path.** On the plain path +/// only *locally published* samples pass through this queue; a sample arriving +/// over a transport is delivered inline on an RX worker and is backpressured by +/// zenoh instead. On the advanced path [`Self::always_shim`] enqueues +/// everything, remote included. So a slow callback loses local samples and +/// stalls remote ones on the plain path, and loses either on the advanced one. +/// That asymmetry is inherent to delivering the two on different threads — which +/// is what makes re-entrancy impossible without taxing the inter-process path. +/// +/// **On the advanced path a `KeepAll` subscriber has no backpressure at all.** +/// Because the queue is genuinely unbounded there and remote samples go into it, +/// a publisher outpacing the callback grows `pending` without limit — the +/// escalating backlog warning is the only signal. That is the declared QoS being +/// honoured rather than a defect, but `KeepAll` on a slow callback is an +/// unbounded memory commitment and should be chosen deliberately. pub struct CallbackDispatcher { queue: Arc, thread: Option>, @@ -373,9 +383,9 @@ impl CallbackDispatcher { /// [`Self::local_only_shim`] to obtain the callback to hand to zenoh. /// /// `capacity` is the number of undelivered samples retained before the - /// oldest is dropped — [`dispatch_capacity`] on the plain path, - /// [`DISPATCH_UNBOUNDED`] on the advanced one. See the "Backpressure" - /// section for why the two differ. + /// oldest is dropped. Both paths pass [`dispatch_capacity`], so a callback + /// subscriber retains what its history QoS declares regardless of which + /// path it takes. See the "Backpressure" section. pub(crate) fn spawn(topic: &str, handler: Arc, capacity: usize) -> Result where F: Fn(Sample) + Send + Sync + 'static, @@ -1388,19 +1398,32 @@ where debug!("[SUB] Using AdvancedSubscriber (TransientLocal durability)"); // `AdvancedSubscriber` holds its state lock across the callback and // cannot avoid it, so *user* code is enqueued and runs on the - // dispatcher's thread. Lossless: dropping would discard exactly the - // samples miss-detection recovered. See `CallbackDispatcher`. + // dispatcher's thread. See `CallbackDispatcher`. + // + // Capacity comes from the history QoS, exactly as on the plain path. + // An earlier revision passed `DISPATCH_UNBOUNDED` here, on the + // grounds that dropping would discard the samples miss-detection + // recovered. That argument holds for `KeepAll` — which + // `dispatch_capacity` still maps to `DISPATCH_UNBOUNDED` — but it + // was applied to every profile, so a `KeepLast(10)` subscriber got + // an unbounded queue. Since `always_shim` enqueues *remote* samples + // too, that traded zenoh's transport backpressure for unbounded + // in-process growth: a publisher outpacing a slow callback grew + // `pending` until the process died, with only a doubling-threshold + // `warn!` for a signal. Honouring the declared depth keeps the + // lossless guarantee where the user asked for it and bounds it + // where they did not. // // A queue-mode handler is exempt. It only pushes into a // `BoundedQueue` and re-enters nothing, so running it under // zenoh-ext's lock is safe — and giving it a dispatcher would add a - // thread, a wake and an unbounded queue in front of the bounded one - // to every TransientLocal rmw subscription, for nothing. + // thread, a wake and a second queue in front of the bounded one to + // every TransientLocal rmw subscription, for nothing. let dispatcher = if runs_user_code { Some(CallbackDispatcher::spawn( &qualified_topic, validated_handler.clone(), - DISPATCH_UNBOUNDED, + dispatch_capacity(&self.entity.qos), )?) } else { None From bd2cf339b9e48fd15e8b4891dc035fe39bf54b13 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Thu, 6 Aug 2026 13:47:40 +0800 Subject: [PATCH 16/30] test(pubsub): assert delivery ordering, not completeness, on the bounded path transient_local_delivery_preserves_order published 500 samples through a KeepLast(10) subscriber and asserted all 500 arrived. That asserts a promise no RMW makes: rmw_zenoh_cpp's add_new_message drops the oldest once message_queue_.size() >= adapted_qos_profile.depth, for every arriving sample, with no TransientLocal exemption -- the check reads the history policy only. The test's stated property is ordering, and its own doc says so. Assert that instead: a strictly increasing subsequence of what was published. That catches reordering whether or not anything was dropped, where an equality check conflated the two failures. Losslessness is still covered, on the profile that actually promises it, by keep_all_delivers_every_local_sample. Also records in the dispatcher docs that both implementations drop silently w.r.t. the ROS event API: upstream's MESSAGE_LOST comes from sequence-number gaps among arriving messages, which a depth-drop cannot produce, so bounding introduces no reporting gap. --- crates/hiroz-tests/tests/reentrant_publish.rs | 46 ++++++++++++++++--- crates/hiroz/src/pubsub.rs | 27 +++++++---- 2 files changed, 58 insertions(+), 15 deletions(-) diff --git a/crates/hiroz-tests/tests/reentrant_publish.rs b/crates/hiroz-tests/tests/reentrant_publish.rs index fb6e20f5d..f29df1b26 100644 --- a/crates/hiroz-tests/tests/reentrant_publish.rs +++ b/crates/hiroz-tests/tests/reentrant_publish.rs @@ -613,6 +613,19 @@ fn intra_closed_loop_runs_iteratively() { /// The shim enqueues from inside `handle_sample` — i.e. under zenoh-ext's state /// mutex, in exactly the order zenoh-ext chose to deliver — and a single thread /// pops FIFO, so the observed order must be the publish order. +/// +/// **Ordering, not completeness.** The burst is far deeper than the declared +/// `KeepLast` depth, so the queue drops the oldest — exactly as +/// `rmw_zenoh_cpp`'s `add_new_message` does (`rmw_subscription_data.cpp`: +/// `size() >= adapted_qos_profile.depth` → `pop_front()`, with no +/// `TransientLocal` exemption). Asserting the delivered values were *all* +/// published would therefore assert a promise no RMW makes. +/// +/// Asserting a strictly increasing subsequence is the stronger test anyway: it +/// catches reordering whether or not anything was dropped, whereas an equality +/// check conflates the two failures. `keep_all_delivers_every_local_sample` in +/// `dispatch_backpressure.rs` covers losslessness on the profile that promises +/// it. #[test] #[serial] fn transient_local_delivery_preserves_order() { @@ -655,14 +668,35 @@ fn transient_local_delivery_preserves_order() { .expect("publish failed"); } - await_deliveries(&seen, COUNT as usize); + // Settle rather than wait for a fixed count: with a bounded queue the + // delivered total is a property of scheduling, not of the publish count. + let mut last = 0usize; + let mut stable_since = Instant::now(); + let deadline = Instant::now() + DELIVERY_TIMEOUT; + loop { + let len = seen.load(Ordering::SeqCst); + if len != last { + last = len; + stable_since = Instant::now(); + } else if stable_since.elapsed() >= Duration::from_millis(300) { + break; + } + assert!(Instant::now() < deadline, "delivery never settled"); + thread::sleep(Duration::from_millis(25)); + } let received = received.lock().unwrap(); - let expected: Vec = (0..COUNT).collect(); - assert_eq!( - &received[..COUNT as usize], - &expected[..], - "delivery thread reordered samples" + assert!( + !received.is_empty(), + "nothing was delivered — the scenario proved nothing" + ); + assert!( + received.windows(2).all(|w| w[0] < w[1]), + "delivery thread reordered samples: {received:?}" + ); + assert!( + received.iter().all(|&c| c < COUNT), + "delivered a counter that was never published: {received:?}" ); }); } diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index 0e6c85892..3e75169ea 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -342,15 +342,24 @@ impl DispatchQueue { /// survives, so the ordering objection that applies to the advanced path does /// not apply here. /// -/// * **Advanced path — same bound, and the same expression.** A `TransientLocal` -/// subscriber exists to replay history and to recover samples flagged as -/// missed, so dropping discards data zenoh-ext went out of its way to fetch -/// and breaks the reordering contract mid-flight — a single -/// `deliver_and_flush` enqueues several back-to-back samples whose contiguity -/// is the point. That argument is why `KeepAll` maps to -/// [`DISPATCH_UNBOUNDED`]. It is *not* a reason to ignore a declared -/// `KeepLast(depth)`, which is what an unbounded capacity on every profile -/// amounted to. +/// * **Advanced path — same bound, and the same expression.** This matches +/// `rmw_zenoh_cpp`: `SubscriptionData::add_new_message` drops the oldest once +/// `message_queue_.size() >= adapted_qos_profile.depth`, for every arriving +/// sample, with **no `TransientLocal` exemption** — the check reads the +/// history policy only. Its advanced-subscriber cache is sized the same way +/// (`adv_sub_opts.history->max_samples = qos_.depth`). +/// +/// An earlier revision left this path unbounded on *every* profile, reasoning +/// that dropping discards what miss-detection recovered. That is why `KeepAll` +/// maps to [`DISPATCH_UNBOUNDED`] — but applied to a declared +/// `KeepLast(depth)` it ignores the QoS, and since [`Self::always_shim`] +/// enqueues remote samples too, it also traded zenoh's transport backpressure +/// for unbounded growth. +/// +/// Both implementations drop **silently** as far as the ROS event API is +/// concerned: upstream's `MESSAGE_LOST` is raised from *sequence-number gaps* +/// among arriving messages, which a depth-drop cannot produce. The escalating +/// `warn!` below is strictly more visible than upstream's debug log. /// /// Two consequences are worth stating rather than discovering. /// From b2a305e80724c820ff53183d4f2ceaa476fa5608 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Thu, 6 Aug 2026 14:52:07 +0800 Subject: [PATCH 17/30] fix(node): bound the FFI advanced dispatch queue too The conversion to dispatch_capacity covered three of four CallbackDispatcher::spawn sites. node.rs's advanced (TransientLocal) arm still passed DISPATCH_UNBOUNDED, so an FFI raw subscriber declaring KeepLast(n) got an unbounded queue fed by remote samples -- the exact defect the other three sites were changed to remove. Two shipped doc comments and the PR description asserted 'both paths pass dispatch_capacity'. There are four paths, and one did not. Nothing caught it because the ffi feature is enabled by no crate, so this arm is never compiled on the PR gate (#291). Found by an audit agent reading the diff against its own description. --- crates/hiroz/src/node.rs | 9 ++++++--- crates/hiroz/src/pubsub.rs | 12 +++++++++--- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/crates/hiroz/src/node.rs b/crates/hiroz/src/node.rs index b003ed951..83f1e9030 100644 --- a/crates/hiroz/src/node.rs +++ b/crates/hiroz/src/node.rs @@ -658,7 +658,7 @@ impl ZNode { use crate::{ entity::{EndpointEntity, EndpointKind}, pubsub::{ - CallbackDispatcher, DISPATCH_UNBOUNDED, SubscriberHandle, + CallbackDispatcher, SubscriberHandle, apply_transient_local_sub, dispatch_capacity, qos_needs_advanced, }, topic_name, @@ -694,8 +694,11 @@ impl ZNode { // never runs on a thread that is inside a hiroz publish — see // `pubsub::CallbackDispatcher`. let subscriber = if qos_needs_advanced(&entity.qos) { - let dispatcher = - CallbackDispatcher::spawn(&qualified_topic, raw_callback, DISPATCH_UNBOUNDED)?; + let dispatcher = CallbackDispatcher::spawn( + &qualified_topic, + raw_callback, + dispatch_capacity(&entity.qos), + )?; let subscriber = self .session .declare_subscriber((*topic_ke).clone()) diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index 3e75169ea..e6b20511c 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -392,9 +392,15 @@ impl CallbackDispatcher { /// [`Self::local_only_shim`] to obtain the callback to hand to zenoh. /// /// `capacity` is the number of undelivered samples retained before the - /// oldest is dropped. Both paths pass [`dispatch_capacity`], so a callback - /// subscriber retains what its history QoS declares regardless of which - /// path it takes. See the "Backpressure" section. + /// oldest is dropped. **All four construction sites pass + /// [`dispatch_capacity`]** — the plain and advanced arms of both the typed + /// builder (this module) and the FFI raw subscriber (`node.rs`) — so a + /// callback subscriber retains what its history QoS declares regardless of + /// which path it takes. See the "Backpressure" section. + /// + /// The FFI advanced arm was missed when the other three were converted, and + /// nothing caught it: the `ffi` feature is enabled by no crate, so that arm + /// is not compiled on the PR gate at all (#291). pub(crate) fn spawn(topic: &str, handler: Arc, capacity: usize) -> Result where F: Fn(Sample) + Send + Sync + 'static, From b326f331a02444c0faebe8d4ca42308e4c9e4206 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Thu, 6 Aug 2026 19:03:03 +0800 Subject: [PATCH 18/30] docs(pubsub): correct why the FFI arm's wrong constant was missed The comment said the ffi arm 'is not compiled on the PR gate at all'. It is compiled -- test.yml builds --features ffi on pull_request. What is missing is narrower: it is never linted and never tested, and its re-entrancy detector had been deleted. A wrong constant is neither a compile error nor a lint, so nothing was left to catch it. Building is not testing. The same false explanation was corrected in #291 and in this PR's description; the source comment was written in the same commit and missed. --- crates/hiroz/src/pubsub.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index e6b20511c..9c5a00a51 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -398,9 +398,11 @@ impl CallbackDispatcher { /// callback subscriber retains what its history QoS declares regardless of /// which path it takes. See the "Backpressure" section. /// - /// The FFI advanced arm was missed when the other three were converted, and - /// nothing caught it: the `ffi` feature is enabled by no crate, so that arm - /// is not compiled on the PR gate at all (#291). + /// The FFI advanced arm was missed when the other three were converted. That + /// arm *is* compiled on the PR gate, but never linted and never tested, and + /// its re-entrancy detector had been deleted — so a wrong constant there is + /// neither a compile error nor a lint, and nothing was left to catch it + /// (#291). Building is not testing. pub(crate) fn spawn(topic: &str, handler: Arc, capacity: usize) -> Result where F: Fn(Sample) + Send + Sync + 'static, From cb313013ab8437281590b45d864a4e42870f03d7 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Wed, 12 Aug 2026 17:10:13 +0800 Subject: [PATCH 19/30] docs(pubsub): apply simplified technical english to the dispatcher docs One idea per sentence, active voice, consistent terms. Longest sentence in the rewritten blocks drops to 13 words. Three statements were corrected rather than rephrased, because a clearer sentence must not preserve a claim the code refutes: - the Backpressure lead-in said the two paths get different answers while both bullets said the same bound; they take the same bound and differ in which samples reach it - the field-order rationale said the dispatcher drains before it joins; it discards - qos_needs_advanced said subscriber/publisher; no publisher calls it Also drops a paragraph narrating an earlier revision of this branch, which will not exist after a squash merge. --- crates/hiroz/src/pubsub.rs | 162 ++++++++++++++++++++----------------- 1 file changed, 88 insertions(+), 74 deletions(-) diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index 9c5a00a51..6cf2407c2 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -98,11 +98,15 @@ fn local_publish_active() -> bool { } /// Backlog size at which an *unbounded* [`CallbackDispatcher`] first warns. -/// Doubles after each warning so a persistently slow callback does not flood the -/// log. Bounded dispatchers are excluded explicitly at the check rather than by -/// this value being out of their reach: a `KeepLast(1024)` subscriber has -/// exactly this capacity, so it would otherwise warn that its queue is lossless -/// right before dropping. Bounded dispatchers warn on drops instead. +/// +/// The threshold doubles after each warning. A persistently slow callback +/// therefore does not flood the log. +/// +/// The check excludes bounded dispatchers explicitly. It does not rely on this +/// value being out of their reach: a `KeepLast(1024)` subscriber has exactly +/// this capacity. Such a subscriber would otherwise warn that its queue is +/// lossless immediately before it drops a sample. Bounded dispatchers warn on +/// drops instead. const DISPATCH_BACKLOG_WARN_AT: usize = 1024; /// Capacity a [`CallbackDispatcher`] must be given to be unbounded, i.e. lossless. @@ -110,23 +114,21 @@ pub(crate) const DISPATCH_UNBOUNDED: usize = usize::MAX; /// The dispatcher capacity implied by a subscriber's history QoS. /// -/// Matches what [`ZSubBuilder::build`] gives the queue-mode [`BoundedQueue`]: -/// `KeepLast(depth)` keeps `depth`, `KeepAll` keeps everything. A callback -/// subscriber and a queue subscriber declared with the same QoS therefore retain -/// the same number of undelivered samples, which is the only reading of ROS -/// `KEEP_LAST(depth)` that does not depend on which hiroz API the user happened -/// to pick. +/// This matches what [`ZSubBuilder::build`] gives the queue-mode +/// [`BoundedQueue`]: `KeepLast(depth)` keeps `depth`, and `KeepAll` keeps +/// everything. A callback subscriber and a queue subscriber with the same QoS +/// therefore retain the same number of undelivered samples. Retention does not +/// depend on which hiroz API the caller chose. /// -/// The two are *not* the same expression, and the difference is confined to a -/// zero depth (the rmw spelling of "system default", which cannot be produced -/// through [`QosProfile`] but can arrive over the wire). Here it is floored at 1 -/// rather than degenerating into "keep nothing"; the queue path passes the 0 -/// through. Retention still agrees, because [`BoundedQueue::push`] evicts before -/// it inserts (`len >= capacity` → `pop_front`, then `push_back`), so a capacity -/// of 0 also retains exactly one sample — see `queue::tests:: -/// zero_capacity_retains_one_sample`. What differs is bookkeeping, not data: at -/// capacity 0 every push reports a drop, including the first one into an empty -/// queue. +/// The two are *not* the same expression. The difference applies only to a zero +/// depth. Zero is the rmw spelling of "system default". [`QosProfile`] cannot +/// produce it, but it can arrive over the wire. This function floors it at 1; +/// the queue path passes it through. +/// +/// Retention still agrees. [`BoundedQueue::push`] evicts before it inserts +/// (`len >= capacity` → `pop_front`, then `push_back`), so a capacity of 0 also +/// retains exactly one sample. Only the bookkeeping differs: at capacity 0 +/// every push reports a drop, including the first push into an empty queue. pub(crate) fn dispatch_capacity(qos: &hiroz_protocol::qos::QosProfile) -> usize { match qos.history { QosHistory::KeepLast(depth) => depth.max(1), @@ -325,41 +327,40 @@ impl DispatchQueue { /// exactly this cost ("a slow subscriber could block the underlying Zenoh /// thread", `fifo.rs`); hiroz does not adopt that failure mode. /// -/// What remains is a choice between unbounded (lossless, can grow without -/// limit) and bounded drop-oldest (lossy, constant memory). **The two paths get -/// different answers, because they make different promises:** +/// What remains is a choice between unbounded (lossless, grows without limit) +/// and bounded drop-oldest (lossy, constant memory). **Both paths take the same +/// bound from the same expression. They differ only in which samples reach it:** /// /// * **Plain path — bounded, drop-oldest, capacity from the subscriber's /// history QoS** ([`dispatch_capacity`]). A plain subscriber is `Volatile` -/// with `KEEP_LAST(depth)`: it already promises only the last `depth` -/// undelivered samples, and hiroz's own queue-mode path enforces exactly that -/// with [`BoundedQueue`], from the same expression. A callback subscriber that -/// instead retained *every* undelivered sample would honour a QoS stricter -/// than the one it was declared with, and would let a tight local publish loop -/// with a slow callback grow the process until it died — a failure mode with -/// no upside, since the samples being retained are ones the declared QoS says -/// may be discarded. Drop-oldest also preserves the relative order of what -/// survives, so the ordering objection that applies to the advanced path does -/// not apply here. +/// with `KEEP_LAST(depth)`. It already promises only the last `depth` +/// undelivered samples, and the queue-mode path enforces exactly that with +/// [`BoundedQueue`], from the same expression. +/// +/// A callback subscriber that retained *every* undelivered sample would +/// honour a QoS stricter than its declared one. It would also let a tight +/// local publish loop with a slow callback grow the process until it died. +/// The retained samples are ones the declared QoS permits it to discard, so +/// that trade has no upside. Drop-oldest also preserves the relative order of +/// the samples that survive. /// -/// * **Advanced path — same bound, and the same expression.** This matches -/// `rmw_zenoh_cpp`: `SubscriptionData::add_new_message` drops the oldest once -/// `message_queue_.size() >= adapted_qos_profile.depth`, for every arriving -/// sample, with **no `TransientLocal` exemption** — the check reads the -/// history policy only. Its advanced-subscriber cache is sized the same way -/// (`adv_sub_opts.history->max_samples = qos_.depth`). +/// * **Advanced path — the same bound, from the same expression.** This matches +/// `rmw_zenoh_cpp`. Its `SubscriptionData::add_new_message` drops the oldest +/// sample once `message_queue_.size() >= adapted_qos_profile.depth`. It does +/// so for every arriving sample, with **no `TransientLocal` exemption**: the +/// check reads the history policy only. Upstream sizes its advanced-subscriber +/// cache the same way (`adv_sub_opts.history->max_samples = qos_.depth`). /// -/// An earlier revision left this path unbounded on *every* profile, reasoning -/// that dropping discards what miss-detection recovered. That is why `KeepAll` -/// maps to [`DISPATCH_UNBOUNDED`] — but applied to a declared -/// `KeepLast(depth)` it ignores the QoS, and since [`Self::always_shim`] -/// enqueues remote samples too, it also traded zenoh's transport backpressure -/// for unbounded growth. +/// `KeepAll` maps to [`DISPATCH_UNBOUNDED`] because that profile asks for +/// losslessness. A declared `KeepLast(depth)` does not, so this path honours +/// the depth. [`Self::always_shim`] enqueues remote samples too, so an +/// unbounded queue here would also replace zenoh's transport backpressure with +/// unbounded in-process growth. /// -/// Both implementations drop **silently** as far as the ROS event API is -/// concerned: upstream's `MESSAGE_LOST` is raised from *sequence-number gaps* -/// among arriving messages, which a depth-drop cannot produce. The escalating -/// `warn!` below is strictly more visible than upstream's debug log. +/// Both implementations drop **silently**, as the ROS event API sees it. +/// Upstream raises `MESSAGE_LOST` from *sequence-number gaps* between arriving +/// messages, and a depth-drop cannot produce such a gap. The escalating +/// `warn!` below is more visible than upstream's debug log. /// /// Two consequences are worth stating rather than discovering. /// @@ -426,18 +427,19 @@ impl CallbackDispatcher { .name("hiroz-sub-drain".to_string()) .spawn(move || { while let Some(sample) = drain_queue.dequeue() { - // A panicking user callback must not kill the drain thread — - // that would silently stop all further delivery. + // A panicking user callback must not kill the drain thread. + // If it did, the subscriber would stop delivering silently. // - // This holds only where panics unwind. Under `panic = "abort"` - // — which this workspace's `[profile.opt]` sets — the panic - // aborts the process before `catch_unwind` can return `Err`, - // so neither the recovery below nor the log line happens. The - // guard is therefore effective for dev, test and `release` - // builds (including everything CI runs) and inert for `opt`. - // That is a deliberate consequence of choosing `abort` for - // that profile, not an oversight here: a build that opts into - // aborting on panic has opted out of surviving one. + // This guard works only where panics unwind. This + // workspace's `[profile.opt]` sets `panic = "abort"`, so + // there the panic aborts the process before `catch_unwind` + // can return `Err`. Neither the recovery nor the log line + // runs on that profile. + // + // The guard is therefore effective for dev, test and + // `release` builds, which is everything CI runs, and inert + // for `opt`. A build that opts into aborting on panic has + // opted out of surviving one. if std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| (*handler)(sample))) .is_err() { @@ -514,18 +516,24 @@ impl Drop for CallbackDispatcher { } } -/// Whether a QoS profile needs a zenoh-ext advanced subscriber/publisher. +/// Whether a QoS profile needs a zenoh-ext `AdvancedSubscriber`. +/// +/// Both callers are subscriber paths. `ZPubBuilder::build` calls `.advanced()` +/// unconditionally and does not consult this function. /// /// The advanced entities exist for history replay, sample-miss detection and -/// recovery, and publisher/subscriber detection — all of which -/// [`apply_transient_local_sub`] and [`apply_transient_local_pub`] configure only -/// for `TransientLocal` durability. For the ROS 2 default (`Volatile`) an -/// unconfigured `AdvancedSubscriber` adds no protocol behaviour, but it *does* -/// run the user callback while holding a non-reentrant `std::sync::Mutex` -/// (`advanced_subscriber.rs`: `sub_callback` takes `zlock!(statesref)` and -/// `handle_sample` calls the callback under that guard). Combined with zenoh's -/// synchronous local delivery, that turns any publish from inside a callback -/// into a self-deadlock. So only pay for it when the QoS actually asks for it. +/// recovery, and entity detection. [`apply_transient_local_sub`] and +/// [`apply_transient_local_pub`] configure all of these for `TransientLocal` +/// durability only. +/// +/// For the ROS 2 default (`Volatile`) an unconfigured `AdvancedSubscriber` adds +/// no protocol behaviour. It *does* run the user callback while holding a +/// non-reentrant `std::sync::Mutex`: in `advanced_subscriber.rs`, `sub_callback` +/// takes `zlock!(statesref)` and `handle_sample` calls the callback under that +/// guard. zenoh delivers a session-local sample synchronously on the publishing +/// thread, so a publish from inside such a callback deadlocks that thread +/// against itself. A `Volatile` subscriber therefore pays the lock and gains +/// nothing, which is why it declares a plain subscriber instead. pub(crate) fn qos_needs_advanced(qos: &hiroz_protocol::qos::QosProfile) -> bool { matches!(qos.durability, QosDurability::TransientLocal) } @@ -556,9 +564,15 @@ pub enum SubscriberHandle { Advanced { /// Boxed because it is several times larger than the plain variant. /// - /// Declared first so it drops first: undeclaring the subscriber stops - /// new samples from being enqueued before the dispatcher drains and - /// joins. + /// Declared first so it drops first. Undeclaring the subscriber stops + /// new samples from entering the queue before the dispatcher discards + /// its backlog and joins its thread. + /// + /// Rust drops struct fields in declaration order, so this order is a + /// proof obligation rather than a style choice. The guarantee is weaker + /// than it looks: zenoh undeclares with `wait_callbacks: false`, so a + /// sample can still arrive afterwards. `enqueue` returns early once + /// `closed` is set, which makes such a sample harmless. subscriber: Box>, dispatcher: Option, }, From a6ab4083aaa5994f6f0a389b78824f04f94ad786 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Fri, 14 Aug 2026 21:25:53 +0800 Subject: [PATCH 20/30] docs: apply ste sentence rules to added comments Rewrites the comments this branch adds under crates/hiroz/src/ to the ASD-STE100 sentence rules: one idea per sentence, 25 words maximum, active voice with a named actor, present tense, no clause joined by "and". No executable line changes. Also cuts comment text that a linked issue already owns and cites the issue instead: the "which arm was missed / building is not testing" narrative (#291), the superseded-revision rationale for the advanced dispatcher capacity (restated by CallbackDispatcher's "Backpressure" section), and the MESSAGE_LOST plumbing detail (#292). Adds pointers to notifier exemption whose claim nothing enforces. Measured over the added comment paragraphs of the production diff. before: 90 paragraphs, longest sentence 49 words, 29 over 25, 30 passive, 3 joined clauses after: 102 paragraphs, longest sentence 33 words, 1 over 25, 1 passive, 0 joined clauses The one remaining over-25 hit is a four-item list that the extraction script flattens into a single line. The one remaining passive hit reads "bounded *and* blocking" as a participle; both words are adjectives there and no actor is hidden. --- crates/hiroz/src/common.rs | 13 +- crates/hiroz/src/ffi/publisher.rs | 14 +- crates/hiroz/src/node.rs | 6 +- crates/hiroz/src/pubsub.rs | 472 ++++++++++++++++-------------- crates/hiroz/src/queue.rs | 17 +- 5 files changed, 273 insertions(+), 249 deletions(-) diff --git a/crates/hiroz/src/common.rs b/crates/hiroz/src/common.rs index b62b24ce7..48203e5c2 100644 --- a/crates/hiroz/src/common.rs +++ b/crates/hiroz/src/common.rs @@ -22,13 +22,14 @@ impl DataHandler { /// Whether `handle` runs *user* code on the delivering thread. /// /// Only [`DataHandler::Callback`] does. The queue variants enqueue and - /// return — structurally the same thing zenoh's own `FifoChannel` handler - /// does — so the user's code runs on whatever thread calls `recv()`, and - /// there is nothing on the delivery thread that could re-enter hiroz. + /// return. zenoh's own `FifoChannel` handler has the same structure. The + /// user's code therefore runs on whatever thread calls `recv()`. Nothing on + /// the delivery thread can re-enter hiroz. /// - /// `QueueWithNotifier`'s notifier is deliberately not counted: it is the rmw - /// layer's wait-set wake, which must run promptly on the delivery thread and - /// does not call back into hiroz. + /// This function deliberately does not count `QueueWithNotifier`'s notifier. + /// The notifier is the rmw layer's wait-set wake. It must run promptly on + /// the delivery thread. It does not call back into hiroz. Issue #290 tracks + /// that nothing enforces this last claim. pub(crate) fn runs_user_code(&self) -> bool { matches!(self, DataHandler::Callback(_)) } diff --git a/crates/hiroz/src/ffi/publisher.rs b/crates/hiroz/src/ffi/publisher.rs index 496e52073..ea0739816 100644 --- a/crates/hiroz/src/ffi/publisher.rs +++ b/crates/hiroz/src/ffi/publisher.rs @@ -36,14 +36,14 @@ impl RawPublisher { } pub fn publish_bytes(&self, data: &[u8]) -> Result<(), zenoh::Error> { - // Same guard the four `ZPub` publish paths take, and for the same + // The four `ZPub` publish paths take this same guard, for the same // reason. `local_only_shim` hands a sample to the drain thread only - // while `LOCAL_PUBLISH_DEPTH` is set; without the guard a same-process - // raw publish is not marked as local, the subscriber's callback runs - // inline on this thread, and a callback that publishes back into its - // own topic recurses until the stack is gone. This is the path - // `rmw-zenoh-rs` publishes through, so leaving it unguarded would keep - // the deadlock reachable from every rmw user. + // while `LOCAL_PUBLISH_DEPTH` is set. Without the guard, hiroz does not + // mark a same-process raw publish as local. The subscriber's callback + // then runs inline on this thread. A callback that publishes back into + // its own topic recurses until the stack is gone (#249). + // `rmw-zenoh-rs` publishes through this path, so an unguarded publish + // here keeps the deadlock reachable from every rmw user. let _local = crate::pubsub::LocalPublishGuard::enter(); self.inner .put(data) diff --git a/crates/hiroz/src/node.rs b/crates/hiroz/src/node.rs index 83f1e9030..92e5fc60a 100644 --- a/crates/hiroz/src/node.rs +++ b/crates/hiroz/src/node.rs @@ -689,9 +689,9 @@ impl ZNode { callback(&payload); }); - // Same rule as the typed path: only use zenoh-ext when the QoS asks for - // advanced features. Either way this callback is user (FFI) code, so it - // never runs on a thread that is inside a hiroz publish — see + // Same rule as the typed path: use zenoh-ext only when the QoS asks for + // advanced features. This callback is user (FFI) code on either arm. It + // therefore never runs on a thread that is inside a hiroz publish — see // `pubsub::CallbackDispatcher`. let subscriber = if qos_needs_advanced(&entity.qos) { let dispatcher = CallbackDispatcher::spawn( diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index 6cf2407c2..c77b25d6f 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -33,24 +33,26 @@ const SAMPLE_MISS_HEARTBEAT_PERIOD: Duration = Duration::from_millis(500); thread_local! { /// How many hiroz publish calls are currently on this thread's stack. /// - /// Non-zero means: any sample this thread is *about* to deliver was produced - /// by this same thread, synchronously, from inside `put`. See + /// A non-zero count means this thread produced any sample it is *about* to + /// deliver. It produced that sample synchronously, from inside `put`. See /// [`local_publish_active`]. static LOCAL_PUBLISH_DEPTH: std::cell::Cell = const { std::cell::Cell::new(0) }; } -/// RAII marker set for the duration of a hiroz publish. +/// RAII marker that hiroz holds for the duration of a publish. /// -/// There is no single choke point: each of `ZPub`'s four publish paths — +/// hiroz has no single choke point for publishing. Each of `ZPub`'s four +/// publish paths enters a guard itself and holds it across the zenoh `put`: /// [`ZPub::publish`], [`ZPub::async_publish`], [`ZPub::publish_serialized`] and -/// [`ZPub::publish_sample`] — enters one of these itself and holds it across the -/// zenoh `put`. A fifth publish path added later must do the same, or -/// session-local delivery on that path runs inline on the publishing thread and -/// the deadlock this guard exists to prevent comes back. +/// [`ZPub::publish_sample`]. /// -/// Nesting is counted rather than flagged so that a publish issued from inside a -/// callback that is itself running on a thread already inside a publish restores -/// the right state on unwind. +/// A fifth publish path added later must do the same. If it does not, +/// session-local delivery on that path runs inline on the publishing thread. +/// The deadlock this guard prevents then comes back (#249). +/// +/// The guard counts nesting rather than sets a flag. A callback can run on a +/// thread that is already inside a publish. A publish issued from that callback +/// then restores the correct depth when its own guard drops. pub(crate) struct LocalPublishGuard; impl LocalPublishGuard { @@ -68,31 +70,32 @@ impl Drop for LocalPublishGuard { /// Whether this thread is currently inside a hiroz publish. /// -/// This is the discriminator between the two ways a subscriber callback can be -/// reached, and it is what makes re-entrancy structurally impossible without -/// taxing the inter-process path: +/// hiroz can reach a subscriber callback in two ways. This function +/// discriminates between them. That discrimination is what makes re-entrancy +/// structurally impossible without a cost on the inter-process path. /// -/// * **true** — the sample is being delivered *synchronously on the publishing -/// thread*. Zenoh does this for same-session delivery (`Session::resolve_put` -/// drops the session lock and calls the local callbacks inline) and also for -/// two sessions sharing one process with a direct in-process route -/// (`send_push_consume` -> `route_data` -> the peer session's callbacks, no -/// thread hop). Running the user callback here is what allowed a callback that -/// publishes into its own topic graph to *recurse* instead of iterate. So on -/// this path hiroz enqueues and returns, exactly as zenoh's own `FifoChannel` -/// handler does, and the callback runs on the dispatcher thread. +/// * **true** — zenoh delivers the sample *synchronously on the publishing +/// thread*. It does this for same-session delivery: `Session::resolve_put` +/// drops the session lock and calls the local callbacks inline. It also does +/// this for two sessions that share one process with a direct in-process +/// route. That route is `send_push_consume` -> `route_data` -> the peer +/// session's callbacks, with no thread hop. A user callback that runs here +/// can publish into its own topic graph and *recurse* instead of iterate +/// (#249). So hiroz +/// enqueues and returns on this path, exactly as zenoh's own `FifoChannel` +/// handler does. The callback then runs on the dispatcher thread. /// -/// * **false** — the sample arrived over a transport and is being delivered on a -/// zenoh RX worker (`ZRuntime::RX`, threads named `rx-N`), which is never an -/// application thread and never inside a hiroz publish. There is nothing to -/// re-enter, so the callback runs inline and the inter-process path pays only +/// * **false** — the sample arrived over a transport. A zenoh RX worker +/// delivers it (`ZRuntime::RX`, threads named `rx-N`). That worker is never +/// an application thread and never inside a hiroz publish. Nothing can +/// re-enter, so the callback runs inline. The inter-process path pays only /// this thread-local read. /// -/// Note this deliberately keys on the *publishing thread*, not on zenoh's -/// `Locality`. A `Locality::Remote`-tagged sample crossing two sessions inside -/// one process is still delivered inline on the publisher's thread, so an -/// `allowed_origin(SessionLocal)` split would miss it. The thread is the honest -/// signal; the origin is not. +/// This function deliberately keys on the *publishing thread*, not on zenoh's +/// `Locality`. Zenoh still delivers a `Locality::Remote`-tagged sample inline on +/// the publisher's thread when that sample crosses two sessions inside one +/// process. An `allowed_origin(SessionLocal)` split would therefore miss it. The +/// thread is the reliable signal. The origin is not. fn local_publish_active() -> bool { LOCAL_PUBLISH_DEPTH.with(|d| d.get()) != 0 } @@ -102,20 +105,20 @@ fn local_publish_active() -> bool { /// The threshold doubles after each warning. A persistently slow callback /// therefore does not flood the log. /// -/// The check excludes bounded dispatchers explicitly. It does not rely on this -/// value being out of their reach: a `KeepLast(1024)` subscriber has exactly -/// this capacity. Such a subscriber would otherwise warn that its queue is +/// The check excludes bounded dispatchers explicitly. It does not assume that +/// this value is out of their reach: a `KeepLast(1024)` subscriber has exactly +/// this capacity. Such a subscriber would otherwise report that its queue is /// lossless immediately before it drops a sample. Bounded dispatchers warn on /// drops instead. const DISPATCH_BACKLOG_WARN_AT: usize = 1024; -/// Capacity a [`CallbackDispatcher`] must be given to be unbounded, i.e. lossless. +/// The capacity that makes a [`CallbackDispatcher`] unbounded, i.e. lossless. pub(crate) const DISPATCH_UNBOUNDED: usize = usize::MAX; /// The dispatcher capacity implied by a subscriber's history QoS. /// /// This matches what [`ZSubBuilder::build`] gives the queue-mode -/// [`BoundedQueue`]: `KeepLast(depth)` keeps `depth`, and `KeepAll` keeps +/// [`BoundedQueue`]. `KeepLast(depth)` keeps `depth` samples. `KeepAll` keeps /// everything. A callback subscriber and a queue subscriber with the same QoS /// therefore retain the same number of undelivered samples. Retention does not /// depend on which hiroz API the caller chose. @@ -139,9 +142,9 @@ pub(crate) fn dispatch_capacity(qos: &hiroz_protocol::qos::QosProfile) -> usize struct DispatchState { /// Samples awaiting delivery, in the order zenoh decided to deliver them. pending: std::collections::VecDeque, - /// Set by [`CallbackDispatcher::drop`]: stop delivering and exit. Queued - /// but undelivered samples are discarded -- see [`DispatchQueue::dequeue`] - /// for why teardown does not drain them. + /// [`CallbackDispatcher::drop`] sets this: stop delivering and exit. The + /// dispatcher discards queued but undelivered samples -- see + /// [`DispatchQueue::dequeue`] for why teardown does not drain them. closed: bool, /// Next backlog length that triggers a warning. Unbounded queues only. warn_at: usize, @@ -155,10 +158,11 @@ struct DispatchQueue { state: Mutex, ready: std::sync::Condvar, topic: String, - /// Maximum number of undelivered samples retained. [`DISPATCH_UNBOUNDED`] - /// means lossless; anything smaller drops the *oldest* on overflow, exactly - /// as [`BoundedQueue::push`] does. See [`CallbackDispatcher`]'s - /// "Backpressure" section for which path gets which. + /// Maximum number of undelivered samples the queue retains. + /// [`DISPATCH_UNBOUNDED`] means lossless. Any smaller capacity drops the + /// *oldest* sample on overflow, exactly as [`BoundedQueue::push`] does. See + /// [`CallbackDispatcher`]'s "Backpressure" section for which path gets + /// which. capacity: usize, } @@ -169,9 +173,11 @@ impl DispatchQueue { self.state.lock().unwrap_or_else(|e| e.into_inner()) } - /// The shim callback handed to zenoh. May run with zenoh-ext's state mutex - /// held (advanced path) or on the publishing thread inside `put` (local - /// path), so it must not do anything that could publish or block. + /// The shim callback that hiroz hands to zenoh. + /// + /// This function may run with zenoh-ext's state mutex held (advanced path). + /// It may also run on the publishing thread inside `put` (local path). It + /// must therefore never publish and never block. fn enqueue(&self, sample: Sample) { let (backlog, dropped) = { let mut state = self.lock(); @@ -179,10 +185,11 @@ impl DispatchQueue { return; } - // Drop the oldest, never the newest and never the incoming sample: - // the same choice `BoundedQueue::push` makes, and the same one ROS - // `KEEP_LAST(depth)` describes. A bounded queue that *blocked* here - // would re-create the original deadlock — see the type's docs. + // Drop the oldest sample. Never drop the newest. Never drop the + // incoming one. `BoundedQueue::push` makes the same choice. ROS + // `KEEP_LAST(depth)` describes the same choice. A bounded queue + // that *blocked* here would re-create the original deadlock — see + // the type's docs. let dropped = if state.pending.len() >= self.capacity { state.pending.pop_front(); state.dropped = state.dropped.saturating_add(1); @@ -199,11 +206,11 @@ impl DispatchQueue { state.pending.push_back(sample); let len = state.pending.len(); // Unbounded queues only. A bounded queue *can* reach - // `DISPATCH_BACKLOG_WARN_AT` — nothing stops a subscriber declaring - // `KeepLast(1024)` or deeper — and it would then log that the queue - // is lossless and merely costs memory, which is the opposite of - // what a bounded queue does. Bounded queues report drops instead; - // that warning is immediately below and is the accurate one. + // `DISPATCH_BACKLOG_WARN_AT`: nothing stops a subscriber from + // declaring `KeepLast(1024)` or deeper. It would then log that the + // queue is lossless and costs only memory. A bounded queue does the + // opposite. Bounded queues report drops instead. That warning is + // immediately below. It is the accurate one. let backlog = if self.capacity == DISPATCH_UNBOUNDED && len >= state.warn_at { state.warn_at = len.saturating_mul(2); Some(len) @@ -233,23 +240,25 @@ impl DispatchQueue { } } - /// Blocks until a sample is available, or until the queue is closed - /// (returns `None`, ending the drain loop). + /// Blocks until a sample arrives, or until the queue closes. A closed queue + /// returns `None`, which ends the drain loop. + /// + /// This function checks `closed` **before** `pending`. That order is the + /// difference between a bounded and an unbounded teardown. /// - /// `closed` is checked **before** `pending`, and that ordering is the - /// difference between a bounded and an unbounded teardown. Draining the - /// backlog first meant `drop(subscriber)` ran a user callback for every - /// queued sample before returning: on the unbounded (TransientLocal) path - /// that is `backlog × callback_duration` with no ceiling -- a 1 kHz - /// publisher against a 5 ms callback leaves ~30 000 samples queued after - /// 30 s, so the drop blocks for minutes, silently. It could also block - /// *forever*, if a callback waits on anything the dropping thread must - /// supply. + /// An earlier version drained the backlog first. `drop(subscriber)` then + /// ran a user callback for every queued sample before it returned. On the + /// unbounded (TransientLocal) path that cost is + /// `backlog × callback_duration` with no ceiling. A 1 kHz publisher against + /// a 5 ms callback leaves about 30 000 samples queued after 30 s, so the + /// drop blocks silently for minutes. It can also block *forever* if a + /// callback waits on anything the dropping thread must supply. /// - /// Dropping a subscriber means "stop delivering to me", so undelivered - /// samples are discarded rather than forced through a callback the caller - /// has already disposed of -- the same thing destroying an rclcpp - /// subscription does. Teardown now costs at most one in-flight callback. + /// Dropping a subscriber means "stop delivering to me". The dispatcher + /// therefore discards undelivered samples. It does not force them through a + /// callback the caller has already disposed of. Destroying an rclcpp + /// subscription does the same. Teardown costs at most one in-flight + /// callback. fn dequeue(&self) -> Option { let mut state = self.lock(); loop { @@ -264,83 +273,89 @@ impl DispatchQueue { } } -/// Runs a subscriber's user callback on a dedicated thread, fed by a FIFO queue. +/// Runs a subscriber's user callback on a dedicated thread. A FIFO queue feeds +/// that thread. /// -/// This is hiroz's equivalent of zenoh's `FifoChannel` handler, and of -/// zenoh-python's `Callback(indirect=True)` — which is what zenoh-python -/// installs by default when you hand `declare_subscriber` a plain callable. The -/// delivery thread enqueues and returns; user code runs here. +/// This type is hiroz's equivalent of zenoh's `FifoChannel` handler. It is also +/// the equivalent of zenoh-python's `Callback(indirect=True)`. zenoh-python +/// installs that handler by default when you hand `declare_subscriber` a plain +/// callable. The delivery thread enqueues and returns. User code runs on this +/// type's thread. /// -/// Two independent reasons a sample takes this path: +/// A sample takes this path for either of two independent reasons. /// -/// 1. **It was published by this same thread** ([`local_publish_active`]) — the -/// session-local case. Delivering inline would let a callback that publishes -/// into its own topic graph recurse instead of iterate. Enqueuing makes the -/// feedback loop *iterative*, which is why hiroz no longer needs a -/// re-entrancy depth cap: a callback simply cannot be reached from inside -/// `put`. -/// 2. **The subscriber is a zenoh-ext `AdvancedSubscriber`**, which invokes the -/// sample callback while holding the `std::sync::Mutex` that guards its -/// reordering state — and it *has* to: `handle_sample` interleaves -/// `callback.call(sample)` with mutation of `last_delivered` / -/// `pending_samples` (see `deliver_and_flush`, which calls the callback, -/// records the delivered sequence number, then drains newly-contiguous -/// pending samples calling the callback again). The guard cannot simply be -/// dropped before the call the way `Session::resolve_put` does, because the -/// lock protects exactly the state the delivery loop is walking. +/// 1. **This same thread published it** ([`local_publish_active`]) — the +/// session-local case. Inline delivery would let a callback that publishes +/// into its own topic graph recurse instead of iterate. The queue makes that +/// feedback loop *iterative*. hiroz therefore needs no re-entrancy depth +/// cap: nothing can reach a callback from inside `put`. +/// 2. **The subscriber is a zenoh-ext `AdvancedSubscriber`.** It invokes the +/// sample callback while it holds the `std::sync::Mutex` that guards its +/// reordering state. It *has* to. `handle_sample` interleaves +/// `callback.call(sample)` with mutation of `last_delivered` and +/// `pending_samples`. `deliver_and_flush` calls the callback, records the +/// delivered sequence number, then drains newly-contiguous pending samples +/// and calls the callback again. zenoh-ext cannot drop the guard before the +/// call the way `Session::resolve_put` does. The lock protects exactly the +/// state the delivery loop walks. /// -/// A sample that is neither — i.e. one that arrived over a transport, on a -/// zenoh RX worker, for a plain subscriber — is delivered inline and never -/// touches this queue. That is deliberate: the RX thread is not an application -/// thread and holds no hiroz lock, so there is nothing to re-enter, and the -/// inter-process path must not pay for a hazard it does not have. +/// A sample that matches neither reason arrived over a transport, on a zenoh RX +/// worker, for a plain subscriber. hiroz delivers it inline. It never touches +/// this queue. That is deliberate. The RX thread is not an application thread +/// and holds no hiroz lock, so nothing can re-enter. The inter-process path must +/// not pay for a hazard it does not have. /// /// # Ordering /// -/// One producer path, one FIFO queue, one drain thread, so the user observes -/// exactly the order zenoh decided to deliver in. On the advanced path the shim -/// enqueues from inside `handle_sample`, i.e. under zenoh-ext's state mutex, so -/// enqueue order includes the several back-to-back deliveries a single -/// `deliver_and_flush` performs when it drains pending samples; the reordering -/// and recovery guarantees `AdvancedSubscriber` exists to provide are -/// unaffected, only the thread the callback runs on changes. +/// There is one producer path, one FIFO queue and one drain thread. The user +/// therefore observes exactly the order zenoh chose to deliver in. /// -/// The one ordering property that is *not* preserved is between the two paths: -/// a plain subscriber that receives both local and remote publications on the -/// same topic now runs the local ones on this thread and the remote ones on an -/// RX thread, so their relative order is no longer guaranteed and the two can -/// overlap. Neither ROS 2 nor zenoh guarantees ordering across distinct -/// publishers, and a plain zenoh subscriber can already be invoked concurrently -/// from several RX workers, so this weakens no guarantee that was actually -/// being offered — but it is a real change and is called out here rather than -/// discovered later. +/// On the advanced path the shim enqueues from inside `handle_sample`, under +/// zenoh-ext's state mutex. Enqueue order therefore includes the several +/// back-to-back deliveries that one `deliver_and_flush` performs when it drains +/// pending samples. This changes only the thread the callback runs on. It does +/// not change the reordering and recovery guarantees that `AdvancedSubscriber` +/// exists to provide. +/// +/// One ordering property does *not* hold: order between the two paths. A plain +/// subscriber can receive both local and remote publications on one topic. It +/// now runs the local ones on this thread and the remote ones on an RX thread. +/// Their relative order is no longer guaranteed, and the two can overlap. +/// +/// This weakens no guarantee that hiroz was actually offering. Neither ROS 2 nor +/// zenoh guarantees ordering across distinct publishers. Several RX workers can +/// already invoke a plain zenoh subscriber concurrently. The change is real, so +/// this section states it rather than leaving a reader to find it later. /// /// # Backpressure /// -/// The queue **never blocks its producer**. That is not a tuning choice: a +/// The queue **never blocks its producer**. That is not a tuning choice. A /// bounded queue that blocked would re-create the original deadlock in a new -/// form on both paths. On the advanced path the blocked thread sits inside -/// `sub_callback` holding zenoh-ext's state mutex; on the local path it sits -/// inside the user's own `publish()`, and in a closed feedback loop the drain -/// thread it waits on is the very thread that must publish for the queue to -/// drain. zenoh's own `FifoChannel` is bounded *and* blocking and documents -/// exactly this cost ("a slow subscriber could block the underlying Zenoh -/// thread", `fifo.rs`); hiroz does not adopt that failure mode. +/// form on both paths. +/// +/// On the advanced path the blocked thread sits inside `sub_callback` and holds +/// zenoh-ext's state mutex. On the local path it sits inside the user's own +/// `publish()`. In a closed feedback loop, the drain thread it waits on is the +/// very thread that must publish for the queue to drain. /// -/// What remains is a choice between unbounded (lossless, grows without limit) -/// and bounded drop-oldest (lossy, constant memory). **Both paths take the same +/// zenoh's own `FifoChannel` is bounded *and* blocking. It documents exactly +/// this cost: "a slow subscriber could block the underlying Zenoh thread" +/// (`fifo.rs`). hiroz does not adopt that failure mode. +/// +/// That leaves a choice between unbounded (lossless, grows without limit) and +/// bounded drop-oldest (lossy, constant memory). **Both paths take the same /// bound from the same expression. They differ only in which samples reach it:** /// /// * **Plain path — bounded, drop-oldest, capacity from the subscriber's /// history QoS** ([`dispatch_capacity`]). A plain subscriber is `Volatile` /// with `KEEP_LAST(depth)`. It already promises only the last `depth` -/// undelivered samples, and the queue-mode path enforces exactly that with +/// undelivered samples. The queue-mode path enforces exactly that with /// [`BoundedQueue`], from the same expression. /// /// A callback subscriber that retained *every* undelivered sample would /// honour a QoS stricter than its declared one. It would also let a tight -/// local publish loop with a slow callback grow the process until it died. -/// The retained samples are ones the declared QoS permits it to discard, so +/// local publish loop with a slow callback grow the process until it dies. +/// The declared QoS permits it to discard the samples it would retain, so /// that trade has no upside. Drop-oldest also preserves the relative order of /// the samples that survive. /// @@ -358,27 +373,31 @@ impl DispatchQueue { /// unbounded in-process growth. /// /// Both implementations drop **silently**, as the ROS event API sees it. -/// Upstream raises `MESSAGE_LOST` from *sequence-number gaps* between arriving -/// messages, and a depth-drop cannot produce such a gap. The escalating -/// `warn!` below is more visible than upstream's debug log. +/// Upstream raises `MESSAGE_LOST` from *sequence-number gaps* between +/// arriving messages. A depth-drop cannot produce such a gap. hiroz raises no +/// `MESSAGE_LOST` event either (#292). The escalating `warn!` below is the +/// only signal here. It is more visible than upstream's debug log. +/// +/// Two consequences follow. This section states them rather than leaving a +/// reader to find them later. /// -/// Two consequences are worth stating rather than discovering. +/// **The bound applies to different samples on each path.** On the plain path +/// only *locally published* samples pass through this queue. zenoh delivers a +/// sample that arrived over a transport inline on an RX worker, and +/// backpressures it at the transport instead. On the advanced path +/// [`Self::always_shim`] enqueues everything, remote samples included. /// -/// **Which samples the bound applies to differs by path.** On the plain path -/// only *locally published* samples pass through this queue; a sample arriving -/// over a transport is delivered inline on an RX worker and is backpressured by -/// zenoh instead. On the advanced path [`Self::always_shim`] enqueues -/// everything, remote included. So a slow callback loses local samples and -/// stalls remote ones on the plain path, and loses either on the advanced one. -/// That asymmetry is inherent to delivering the two on different threads — which -/// is what makes re-entrancy impossible without taxing the inter-process path. +/// A slow callback therefore loses local samples and stalls remote ones on the +/// plain path. It loses either kind on the advanced path. That asymmetry follows +/// from delivering the two on different threads, which is what makes re-entrancy +/// impossible without a cost on the inter-process path. /// /// **On the advanced path a `KeepAll` subscriber has no backpressure at all.** -/// Because the queue is genuinely unbounded there and remote samples go into it, -/// a publisher outpacing the callback grows `pending` without limit — the -/// escalating backlog warning is the only signal. That is the declared QoS being -/// honoured rather than a defect, but `KeepAll` on a slow callback is an -/// unbounded memory commitment and should be chosen deliberately. +/// The queue is genuinely unbounded there, and remote samples enter it. A +/// publisher that outpaces the callback grows `pending` without limit. The +/// escalating backlog warning is the only signal. This honours the declared QoS +/// and is not a defect. Even so, `KeepAll` on a slow callback commits unbounded +/// memory. Choose it deliberately. pub struct CallbackDispatcher { queue: Arc, thread: Option>, @@ -387,23 +406,26 @@ pub struct CallbackDispatcher { impl CallbackDispatcher { /// Spawns the drain thread. /// - /// `handler` is shared: the drain thread always calls it, and the *plain* - /// path's shim additionally calls it inline for samples that did not + /// Two callers share `handler`. The drain thread always calls it. The + /// *plain* path's shim also calls it inline, for samples that did not /// originate on this thread. Use [`Self::always_shim`] or /// [`Self::local_only_shim`] to obtain the callback to hand to zenoh. /// - /// `capacity` is the number of undelivered samples retained before the - /// oldest is dropped. **All four construction sites pass - /// [`dispatch_capacity`]** — the plain and advanced arms of both the typed - /// builder (this module) and the FFI raw subscriber (`node.rs`) — so a - /// callback subscriber retains what its history QoS declares regardless of - /// which path it takes. See the "Backpressure" section. - /// - /// The FFI advanced arm was missed when the other three were converted. That - /// arm *is* compiled on the PR gate, but never linted and never tested, and - /// its re-entrancy detector had been deleted — so a wrong constant there is - /// neither a compile error nor a lint, and nothing was left to catch it - /// (#291). Building is not testing. + /// `capacity` is the number of undelivered samples the queue retains before + /// it drops the oldest. **All four construction sites pass + /// [`dispatch_capacity`]:** + /// + /// * the plain arm of the typed builder (this module), + /// * the advanced arm of the typed builder, + /// * the plain arm of the FFI raw subscriber (`node.rs`), + /// * the advanced arm of the FFI raw subscriber. + /// + /// A callback subscriber therefore retains what its history QoS declares on + /// every path. See the "Backpressure" section. + /// + /// Keep all four sites in step by hand. The PR gate compiles the FFI arms + /// but does not lint or test them, so a wrong constant there fails no check + /// (#291). pub(crate) fn spawn(topic: &str, handler: Arc, capacity: usize) -> Result where F: Fn(Sample) + Send + Sync + 'static, @@ -437,7 +459,7 @@ impl CallbackDispatcher { // runs on that profile. // // The guard is therefore effective for dev, test and - // `release` builds, which is everything CI runs, and inert + // `release` builds. CI runs all three. The guard is inert // for `opt`. A build that opts into aborting on panic has // opted out of surviving one. if std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| (*handler)(sample))) @@ -460,21 +482,23 @@ impl CallbackDispatcher { }) } - /// A shim that enqueues **every** sample. Used for the advanced path, where - /// zenoh-ext holds its state mutex across the callback regardless of where - /// the sample came from. + /// A shim that enqueues **every** sample. + /// + /// The advanced path uses this shim. zenoh-ext holds its state mutex across + /// the callback whatever the sample's origin. pub(crate) fn always_shim(&self) -> impl Fn(Sample) + Send + Sync + 'static { let queue = self.queue.clone(); move |sample: Sample| queue.enqueue(sample) } - /// A shim that enqueues only samples produced by the delivering thread - /// itself, and invokes `handler` inline otherwise. Used for the plain path. + /// A shim that enqueues only the samples the delivering thread produced + /// itself. It calls `handler` inline for every other sample. The plain path + /// uses this shim. /// - /// The inline branch is the inter-process hot path: a sample that arrived - /// over a transport is delivered on a zenoh RX worker, which is never inside - /// a hiroz publish, so [`local_publish_active`] is false and the only cost - /// added to that path is this one thread-local read. + /// The inline branch is the inter-process hot path. A zenoh RX worker + /// delivers a sample that arrived over a transport. That worker is never + /// inside a hiroz publish, so [`local_publish_active`] returns false. This + /// one thread-local read is the only cost the path pays. pub(crate) fn local_only_shim( &self, handler: Arc, @@ -502,9 +526,9 @@ impl Drop for CallbackDispatcher { return; }; if thread.thread().id() == std::thread::current().id() { - // The subscriber was dropped from inside its own callback. Joining - // ourselves would deadlock; the thread will observe `closed` and - // exit once this callback returns. + // The caller dropped the subscriber from inside its own callback. + // Joining this thread from itself would deadlock. The thread + // observes `closed` and exits once this callback returns. return; } if thread.join().is_err() { @@ -527,13 +551,13 @@ impl Drop for CallbackDispatcher { /// durability only. /// /// For the ROS 2 default (`Volatile`) an unconfigured `AdvancedSubscriber` adds -/// no protocol behaviour. It *does* run the user callback while holding a -/// non-reentrant `std::sync::Mutex`: in `advanced_subscriber.rs`, `sub_callback` -/// takes `zlock!(statesref)` and `handle_sample` calls the callback under that +/// no protocol behaviour. It *does* run the user callback while it holds a +/// non-reentrant `std::sync::Mutex`. In `advanced_subscriber.rs`, `sub_callback` +/// takes `zlock!(statesref)`. `handle_sample` then calls the callback under that /// guard. zenoh delivers a session-local sample synchronously on the publishing -/// thread, so a publish from inside such a callback deadlocks that thread -/// against itself. A `Volatile` subscriber therefore pays the lock and gains -/// nothing, which is why it declares a plain subscriber instead. +/// thread. A publish from inside such a callback therefore deadlocks that thread +/// against itself (#249). A `Volatile` subscriber pays the lock and gains +/// nothing, so hiroz declares a plain subscriber for it instead. pub(crate) fn qos_needs_advanced(qos: &hiroz_protocol::qos::QosProfile) -> bool { matches!(qos.durability, QosDurability::TransientLocal) } @@ -545,11 +569,11 @@ pub(crate) fn qos_needs_advanced(qos: &hiroz_protocol::qos::QosProfile) -> bool pub enum SubscriberHandle { /// A plain zenoh subscriber (the `Volatile` default). /// - /// Samples that arrived over a transport run inline on the zenoh RX worker. - /// Samples published by the delivering thread itself are handed to the - /// dispatcher — see [`CallbackDispatcher`]. `dispatcher` is `None` for - /// queue-mode subscribers, which run no user code on the delivery thread and - /// so need no handoff. + /// A sample that arrived over a transport runs inline on the zenoh RX + /// worker. hiroz hands a sample the delivering thread published itself to + /// the dispatcher — see [`CallbackDispatcher`]. `dispatcher` is `None` for + /// queue-mode subscribers. They run no user code on the delivery thread, so + /// they need no handoff. Plain { subscriber: zenoh::pubsub::Subscriber<()>, dispatcher: Option, @@ -557,20 +581,21 @@ pub enum SubscriberHandle { /// A zenoh-ext advanced subscriber, used for `TransientLocal` durability. /// /// `dispatcher` is `Some` only when the handler runs user code. zenoh-ext - /// holds its state lock across the callback, so user code must be moved off - /// that thread — but a queue-mode handler only enqueues into a - /// [`BoundedQueue`] and re-enters nothing, so it can run under that lock - /// safely and needs no thread of its own. + /// holds its state lock across the callback, so hiroz must move user code + /// off that thread. A queue-mode handler only enqueues into a + /// [`BoundedQueue`] and re-enters nothing. It can therefore run under that + /// lock safely and needs no thread of its own. Advanced { /// Boxed because it is several times larger than the plain variant. /// - /// Declared first so it drops first. Undeclaring the subscriber stops - /// new samples from entering the queue before the dispatcher discards - /// its backlog and joins its thread. + /// This field comes first in the declaration so that it drops first. + /// Undeclaring the + /// subscriber stops new samples from entering the queue. Only then does + /// the dispatcher discard its backlog and join its thread. /// /// Rust drops struct fields in declaration order, so this order is a /// proof obligation rather than a style choice. The guarantee is weaker - /// than it looks: zenoh undeclares with `wait_callbacks: false`, so a + /// than it looks. zenoh undeclares with `wait_callbacks: false`, so a /// sample can still arrive afterwards. `enqueue` returns early once /// `closed` is set, which makes such a sample harmless. subscriber: Box>, @@ -1130,12 +1155,14 @@ where if self.with_attachment { put_builder = put_builder.attachment(self.new_attachment()); } - // The guard must cover the delivery, and delivery happens in - // `into_future`, not at the await: zenoh's `PublicationBuilder` future is - // `std::future::ready(self.wait())`, so the put — including any inline - // local-subscriber dispatch — completes before a future exists to poll. - // Scoping the guard here rather than across the `.await` also keeps this - // future `Send`, which a thread-local guard held across an await point + // The guard must cover the delivery. Delivery happens in `into_future`, + // not at the await. zenoh's `PublicationBuilder` future is + // `std::future::ready(self.wait())`, so the put completes before a + // future exists to poll. That put includes any inline local-subscriber + // dispatch. + // + // Scoping the guard here rather than across the `.await` also keeps + // this future `Send`. A thread-local guard held across an await point // would not. let fut = { let _local = LocalPublishGuard::enter(); @@ -1396,9 +1423,9 @@ where let events_mgr = Arc::new(Mutex::new(EventsManager::new(gid))); let loss = Arc::new(MessageLossTracker::new(events_mgr.clone())); - // Wrap handler with encoding validation. No re-entrancy accounting is - // needed for this closure itself: a user callback is never reached - // from inside `put` — see `CallbackDispatcher`. + // Wrap the handler with encoding validation. This needs no re-entrancy + // accounting: nothing reaches a user callback from inside `put` — see + // `CallbackDispatcher`. let expected_encoding = self.expected_encoding.clone(); let runs_user_code = handler.runs_user_code(); let validated_handler = Arc::new(move |sample: Sample| { @@ -1423,33 +1450,27 @@ where handler.handle(sample) }); - // Only go through zenoh-ext when the QoS profile actually configures + // Go through zenoh-ext only when the QoS profile actually configures // advanced features. See `qos_needs_advanced`. let inner = if qos_needs_advanced(&self.entity.qos) { debug!("[SUB] Using AdvancedSubscriber (TransientLocal durability)"); // `AdvancedSubscriber` holds its state lock across the callback and - // cannot avoid it, so *user* code is enqueued and runs on the - // dispatcher's thread. See `CallbackDispatcher`. + // cannot avoid it. hiroz therefore enqueues *user* code and runs it + // on the dispatcher's thread. See `CallbackDispatcher`. // - // Capacity comes from the history QoS, exactly as on the plain path. - // An earlier revision passed `DISPATCH_UNBOUNDED` here, on the - // grounds that dropping would discard the samples miss-detection - // recovered. That argument holds for `KeepAll` — which - // `dispatch_capacity` still maps to `DISPATCH_UNBOUNDED` — but it - // was applied to every profile, so a `KeepLast(10)` subscriber got - // an unbounded queue. Since `always_shim` enqueues *remote* samples - // too, that traded zenoh's transport backpressure for unbounded - // in-process growth: a publisher outpacing a slow callback grew - // `pending` until the process died, with only a doubling-threshold - // `warn!` for a signal. Honouring the declared depth keeps the - // lossless guarantee where the user asked for it and bounds it - // where they did not. + // Capacity comes from the history QoS, exactly as on the plain + // path. Do not pass `DISPATCH_UNBOUNDED` here. `always_shim` + // enqueues *remote* samples too, so an unbounded queue on every + // profile trades zenoh's transport backpressure for unbounded + // in-process growth. `dispatch_capacity` still maps `KeepAll` to + // `DISPATCH_UNBOUNDED`, which is where the user asked to be + // lossless. See `CallbackDispatcher`'s "Backpressure" section. // // A queue-mode handler is exempt. It only pushes into a - // `BoundedQueue` and re-enters nothing, so running it under - // zenoh-ext's lock is safe — and giving it a dispatcher would add a - // thread, a wake and a second queue in front of the bounded one to - // every TransientLocal rmw subscription, for nothing. + // `BoundedQueue` and re-enters nothing, so it runs safely under + // zenoh-ext's lock. A dispatcher would add a thread, a wake and a + // second queue in front of the bounded one, on every TransientLocal + // rmw subscription, for nothing. let dispatcher = if runs_user_code { Some(CallbackDispatcher::spawn( &qualified_topic, @@ -1476,13 +1497,14 @@ where dispatcher, } } else if runs_user_code { - // A plain subscriber holds no lock across the callback, but zenoh - // still delivers a same-thread publication *inline* — so a callback - // that publishes into its own topic graph would recurse. Hand those - // samples to the dispatcher; deliver everything else inline, which - // keeps the inter-process path at one thread-local read. Bounded at - // the history depth, drop-oldest — the same `KEEP_LAST(depth)` the - // queue-mode path enforces with `BoundedQueue`. + // A plain subscriber holds no lock across the callback. zenoh still + // delivers a same-thread publication *inline*, so a callback that + // publishes into its own topic graph would recurse (#249). Hand + // those samples to the dispatcher. Deliver everything else inline, + // which keeps the inter-process path at one thread-local read. The + // dispatcher bounds its queue at the history depth and drops the + // oldest sample. That is the same `KEEP_LAST(depth)` the queue-mode + // path enforces with `BoundedQueue`. let dispatcher = CallbackDispatcher::spawn( &qualified_topic, validated_handler.clone(), diff --git a/crates/hiroz/src/queue.rs b/crates/hiroz/src/queue.rs index 06266b9b0..296ab54f9 100644 --- a/crates/hiroz/src/queue.rs +++ b/crates/hiroz/src/queue.rs @@ -121,12 +121,13 @@ mod tests { /// A zero capacity retains one sample, not zero. /// - /// `pubsub::dispatch_capacity` floors a zero history depth at 1 while the - /// queue-mode path passes the 0 straight through, so the two sizing - /// expressions differ. This pins the reason that divergence is harmless: - /// `push` evicts *before* it inserts, so capacity 0 behaves as capacity 1 - /// for retention. If `push` is ever reordered to insert-then-evict, a - /// zero-depth queue starts discarding every sample and this fails. + /// `pubsub::dispatch_capacity` floors a zero history depth at 1. The + /// queue-mode path passes the 0 straight through. The two sizing + /// expressions therefore differ. This test pins the reason that divergence + /// is harmless: `push` evicts *before* it inserts, so capacity 0 retains + /// one sample just as capacity 1 does. A reordering of `push` to + /// insert-then-evict makes a zero-depth queue discard every sample, and + /// this test then fails. #[test] fn zero_capacity_retains_one_sample() { let q = BoundedQueue::new(0); @@ -143,8 +144,8 @@ mod tests { assert!(q.is_empty()); } - /// The capacity-1 comparison the doc claims equivalence against: same - /// retention, but no spurious drop report on the first push. + /// The capacity-1 case that the doc claims equivalence against. Retention + /// is the same. Capacity 1 reports no spurious drop on the first push. #[test] fn capacity_one_retains_one_sample_without_reporting_a_drop() { let q = BoundedQueue::new(1); From 489c994ca062885d4a617b6e4259d1a397f6f5ac Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Fri, 14 Aug 2026 21:50:20 +0800 Subject: [PATCH 21/30] fix(pubsub): disclose the teardown block and pin the gating decision build_with_callback's # Ownership section said only that dropping undeclares the subscriber. It now states that dropping joins a drain thread which may be inside the caller's callback, so the drop blocks for as long as that callback runs. That is the contract D1 needs before this ships to Rust callers. Two unit tests pin qos_needs_advanced for both durabilities. Reverting it to return true unconditionally is what #249 was, and nothing detected it. They do not pin the wiring to SubscriberHandle::Plain, because ZSub holds its handle privately; that gap stays #296's first item. Also reconciles three comments that said the plain and queue-mode bounds come from 'the same expression' with dispatch_capacity's own doc, which says in bold that they do not. --- crates/hiroz/src/pubsub.rs | 58 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 55 insertions(+), 3 deletions(-) diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index c77b25d6f..9b3f64882 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -344,13 +344,15 @@ impl DispatchQueue { /// /// That leaves a choice between unbounded (lossless, grows without limit) and /// bounded drop-oldest (lossy, constant memory). **Both paths take the same -/// bound from the same expression. They differ only in which samples reach it:** +/// bound from the same history depth. They differ only in which samples reach +/// it:** /// /// * **Plain path — bounded, drop-oldest, capacity from the subscriber's /// history QoS** ([`dispatch_capacity`]). A plain subscriber is `Volatile` /// with `KEEP_LAST(depth)`. It already promises only the last `depth` /// undelivered samples. The queue-mode path enforces exactly that with -/// [`BoundedQueue`], from the same expression. +/// [`BoundedQueue`], from the same history depth. The two expressions differ +/// only for a zero depth, which [`dispatch_capacity`] documents. /// /// A callback subscriber that retained *every* undelivered sample would /// honour a QoS stricter than its declared one. It would also let a tight @@ -359,7 +361,7 @@ impl DispatchQueue { /// that trade has no upside. Drop-oldest also preserves the relative order of /// the samples that survive. /// -/// * **Advanced path — the same bound, from the same expression.** This matches +/// * **Advanced path — the same bound, from the same history depth.** This matches /// `rmw_zenoh_cpp`. Its `SubscriptionData::add_new_message` drops the oldest /// sample once `message_queue_.size() >= adapted_qos_profile.depth`. It does /// so for every arriving sample, with **no `TransientLocal` exemption**: the @@ -1581,6 +1583,19 @@ where /// pattern), so Python/Go callers do not need to assign the return value. /// Rust callers must store the `ZSub` in their node or context. /// + /// # Dropping blocks on an in-flight callback + /// + /// A callback subscriber owns a drain thread. Dropping the `ZSub` joins that + /// thread, and the thread may be inside your callback. **Dropping therefore + /// blocks for as long as your callback runs, with no timeout.** + /// + /// Do not drop a callback subscriber while holding a lock, a GIL or a channel + /// that its callback also takes. The drop waits for the callback, and the + /// callback waits for the lock. Neither returns. + /// + /// Dropping from inside the callback itself is safe. The drain thread detaches + /// rather than joining itself. + /// /// # Arguments /// /// * `callback` - A function that will be called with each deserialized message @@ -1937,6 +1952,43 @@ impl ZSub Date: Sat, 15 Aug 2026 01:34:10 +0800 Subject: [PATCH 22/30] docs(pubsub): state the drop-order obligation on Plain too --- crates/hiroz/src/pubsub.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index 9b3f64882..8df4d500d 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -577,6 +577,12 @@ pub enum SubscriberHandle { /// queue-mode subscribers. They run no user code on the delivery thread, so /// they need no handoff. Plain { + /// This field comes first in the declaration so that it drops first. + /// + /// Rust drops struct fields in declaration order, so this order is a + /// proof obligation rather than a style choice. See the same field on + /// [`SubscriberHandle::Advanced`] for why the undeclare must precede + /// the join. subscriber: zenoh::pubsub::Subscriber<()>, dispatcher: Option, }, From 266b0d9c84fb3d0ee321d7fdb45eaa15096607df Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 18 Aug 2026 02:41:24 +0800 Subject: [PATCH 23/30] fix(pubsub): stop making drop(subscriber) a barrier on the in-flight callback Drop for CallbackDispatcher joined the drain thread, and that thread runs user code. Dropping a subscriber was therefore an unbounded callout made while the dropping thread holds whatever it holds: a callback waiting on anything the dropper must supply never returns, with no timeout, no log and no panic. That is this pull request's own defect class relocated to teardown. The contract now: dropping a subscriber guarantees no NEW callback starts. It does not guarantee that a callback already running has finished. Call close(deadline) to wait for that, and branch on Joined or TimedOut. Drop sets closed, notifies and returns at once. The self-drop special case is removed rather than kept as a no-op -- it existed only because joining a thread from itself deadlocks, and nothing joins any more. No design keeps all three of: callbacks off the publishing thread, drop returning promptly, and no callback running after drop returns. main has the last two and lacks the first, which is the deadlock. This change takes the first two. A post-drop callback is the price, not an oversight, and close(deadline) is how a caller who needs the barrier asks for it. The two Python drop sites keep py.allow_threads. They are now belt-and-braces rather than load-bearing, and their comments say so -- the old ones asserted that the drop joins. Part of #296 (tag G2). --- crates/hiroz-py/src/node.rs | 28 ++- crates/hiroz/src/prelude.rs | 2 +- crates/hiroz/src/pubsub.rs | 424 ++++++++++++++++++++++++++++++++++-- 3 files changed, 427 insertions(+), 27 deletions(-) diff --git a/crates/hiroz-py/src/node.rs b/crates/hiroz-py/src/node.rs index 9b7d04cf9..bfb9edb9f 100644 --- a/crates/hiroz-py/src/node.rs +++ b/crates/hiroz-py/src/node.rs @@ -161,9 +161,13 @@ pub struct PyZNode { impl Drop for PyZNode { fn drop(&mut self) { // Same hazard as `destroy_subscriber`, reached a different way: `del - // node` and interpreter shutdown run `tp_dealloc` with the GIL held, - // and dropping these joins delivery threads whose callbacks need the - // GIL. Take the subscribers out and drop them with the GIL released. + // node` and interpreter shutdown run `tp_dealloc` with the GIL held. + // + // Dropping a `ZSub` no longer joins its delivery thread (it detaches -- + // see `CallbackDispatcher::drop`), so this wrapper is belt-and-braces + // rather than load-bearing. It is kept because it costs nothing and it + // documents the hazard for anyone who adds a barrier here later. + // Take the subscribers out and drop them with the GIL released. if self.owned_subs.is_empty() { return; } @@ -432,13 +436,17 @@ impl PyZNode { }; if let Some(pos) = self.owned_subs.iter().position(|(sid, _)| *sid == id) { let owned = self.owned_subs.swap_remove(pos); - // Drop with the GIL released. Dropping a `ZSub` joins its delivery - // thread, and that thread's callback body is `Python::with_gil`. A - // `#[pymethods]` fn runs with the GIL held, so joining from here - // while the thread is mid-callback is a deadlock: we wait for it, - // it waits for the GIL we are holding. The interpreter freezes with - // no exception and no traceback -- the same failure this type - // exists to remove, relocated to teardown. + // Drop with the GIL released. + // + // Dropping a `ZSub` no longer joins its delivery thread -- it + // detaches, see `CallbackDispatcher::drop` -- so this wrapper is + // belt-and-braces rather than load-bearing. It is kept because it + // costs nothing and it documents the hazard it used to prevent: + // that thread's callback body is `Python::with_gil`, a + // `#[pymethods]` fn runs with the GIL held, and any barrier added + // here would wait for a thread that is waiting for the GIL we hold. + // The interpreter would freeze with no exception and no traceback. + // Anyone reaching for `ZSub::close` here must keep this wrapper. py.allow_threads(move || drop(owned)); } Ok(()) diff --git a/crates/hiroz/src/prelude.rs b/crates/hiroz/src/prelude.rs index c66e74783..84e37f0b1 100644 --- a/crates/hiroz/src/prelude.rs +++ b/crates/hiroz/src/prelude.rs @@ -23,7 +23,7 @@ pub use crate::Builder; /// Core runtime types. pub use crate::context::{ZContext, ZContextBuilder}; pub use crate::node::ZNode; -pub use crate::pubsub::{ZPub, ZSub}; +pub use crate::pubsub::{CloseOutcome, ZPub, ZSub}; pub use crate::service::{RequestId, ServiceReply, ServiceRequest, ZClient, ZServer}; pub use crate::action::server::{Accepted, Executing, Requested}; diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index 8df4d500d..224b8c13c 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -157,6 +157,16 @@ struct DispatchState { struct DispatchQueue { state: Mutex, ready: std::sync::Condvar, + /// Set by the drain thread as its last action, once the drain loop has + /// exited and the final user callback has returned. + /// + /// [`CallbackDispatcher::close`] waits on this rather than on + /// [`std::thread::JoinHandle::join`], because `join` has no timed form on + /// stable Rust and `close` must honour a deadline. It lives in its own + /// mutex so that a waiting `close` never contends with delivery. + finished: Mutex, + /// Signalled with `finished`. + done: std::sync::Condvar, topic: String, /// Maximum number of undelivered samples the queue retains. /// [`DISPATCH_UNBOUNDED`] means lossless. Any smaller capacity drops the @@ -441,6 +451,8 @@ impl CallbackDispatcher { warn_dropped_at: 1, }), ready: std::sync::Condvar::new(), + finished: Mutex::new(false), + done: std::sync::Condvar::new(), topic: topic.to_string(), capacity, }); @@ -473,6 +485,17 @@ impl CallbackDispatcher { ); } } + // Last action of the thread. `close` waits for this. It is set + // even when the loop exits because the queue closed mid-flight, + // which is the case `close` exists to observe. + { + let mut finished = drain_queue + .finished + .lock() + .unwrap_or_else(|e| e.into_inner()); + *finished = true; + } + drain_queue.done.notify_all(); }) .map_err(|e| { zenoh::Error::from(format!("failed to spawn subscriber delivery thread: {e}")) @@ -519,19 +542,115 @@ impl CallbackDispatcher { } } +/// What [`CallbackDispatcher::close`], [`SubscriberHandle::close`] and +/// [`ZSub::close`] observed. Branch on it; do not ignore it. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum CloseOutcome { + /// The drain thread exited before the deadline. No user callback from this + /// subscriber can still be running. + Joined, + /// The deadline expired first. The drain thread was **detached**, not + /// killed: a user callback from this subscriber may still be running, and + /// will run to completion. No further callback starts. + TimedOut, +} + impl Drop for CallbackDispatcher { + /// Stops delivery and returns at once. **It never joins the drain thread.** + /// + /// Dropping a subscriber guarantees that no *new* callback starts. It does + /// not guarantee that a callback already running has finished. Call + /// [`CallbackDispatcher::close`] (or [`ZSub::close`]) to wait for that. + /// + /// An earlier version joined here. That made `drop(sub)` block for the + /// whole of an in-flight callback, with no timeout and no log, and it made + /// every Python drop site depend on remembering `py.allow_threads` — the + /// callback body is `Python::with_gil`, so joining it while holding the GIL + /// waits for a thread that is waiting for the GIL. See + /// #296 (tag G2) for the decision. + /// + /// Detaching is not a new state. This type already detached when the caller + /// dropped the subscriber from inside its own callback, because joining a + /// thread from itself deadlocks. That special case is gone: it is now the + /// general rule, so it needs no separate branch. + /// + /// Detaching cannot be memory-unsafe. The handler is `Arc` with + /// `F: Send + Sync + 'static` and the queue is an `Arc`, so the detached + /// thread borrows nothing from the dropping scope. fn drop(&mut self) { self.queue.lock().closed = true; self.queue.ready.notify_all(); + // Dropping the `JoinHandle` detaches the thread. It does not stop the + // thread; it stops *waiting* for it. The thread observes `closed` on + // its next `dequeue` and exits. + drop(self.thread.take()); + } +} + +impl CallbackDispatcher { + /// Stops delivery and waits, for at most `deadline`, until the drain thread + /// has exited. + /// + /// This is the explicit barrier that [`Drop`] no longer provides. It + /// consumes the dispatcher, so it cannot be called twice. + /// + /// It **discards the undelivered backlog**, exactly as [`Drop`] does. This + /// is a teardown barrier, not a flush. Draining first would re-create the + /// unbounded teardown cost that [`DispatchQueue::dequeue`]'s + /// `closed`-before-`pending` order exists to remove. + /// + /// Returns [`CloseOutcome::Joined`] when the thread exited in time, and + /// [`CloseOutcome::TimedOut`] when it did not — in which case the thread is + /// detached and its in-flight callback runs to completion. + /// + /// Calling this from inside the subscriber's own callback returns + /// [`CloseOutcome::TimedOut`] immediately. That thread cannot finish while + /// it is waiting for itself, so there is nothing to wait for. + pub fn close(mut self, deadline: Duration) -> CloseOutcome { + self.queue.lock().closed = true; + self.queue.ready.notify_all(); let Some(thread) = self.thread.take() else { - return; + return CloseOutcome::Joined; }; if thread.thread().id() == std::thread::current().id() { - // The caller dropped the subscriber from inside its own callback. - // Joining this thread from itself would deadlock. The thread - // observes `closed` and exits once this callback returns. - return; + return CloseOutcome::TimedOut; + } + + let start = std::time::Instant::now(); + let mut finished = self + .queue + .finished + .lock() + .unwrap_or_else(|e| e.into_inner()); + while !*finished { + let Some(remaining) = deadline.checked_sub(start.elapsed()) else { + break; + }; + if remaining.is_zero() { + break; + } + let (guard, timed_out) = self + .queue + .done + .wait_timeout(finished, remaining) + .unwrap_or_else(|e| e.into_inner()); + finished = guard; + if timed_out.timed_out() { + break; + } + } + let exited = *finished; + drop(finished); + + if !exited { + warn!( + topic = %self.queue.topic, + ?deadline, + "subscriber delivery thread did not exit before the close deadline; \ + detaching it. Its in-flight callback will run to completion." + ); + return CloseOutcome::TimedOut; } if thread.join().is_err() { warn!( @@ -539,6 +658,7 @@ impl Drop for CallbackDispatcher { "subscriber delivery thread terminated abnormally" ); } + CloseOutcome::Joined } } @@ -582,7 +702,7 @@ pub enum SubscriberHandle { /// Rust drops struct fields in declaration order, so this order is a /// proof obligation rather than a style choice. See the same field on /// [`SubscriberHandle::Advanced`] for why the undeclare must precede - /// the join. + /// the dispatcher's teardown. subscriber: zenoh::pubsub::Subscriber<()>, dispatcher: Option, }, @@ -599,7 +719,7 @@ pub enum SubscriberHandle { /// This field comes first in the declaration so that it drops first. /// Undeclaring the /// subscriber stops new samples from entering the queue. Only then does - /// the dispatcher discard its backlog and join its thread. + /// the dispatcher discard its backlog and release its thread. /// /// Rust drops struct fields in declaration order, so this order is a /// proof obligation rather than a style choice. The guarantee is weaker @@ -611,6 +731,38 @@ pub enum SubscriberHandle { }, } +impl SubscriberHandle { + /// Undeclares the zenoh subscriber, then waits for at most `deadline` for + /// the dispatcher's drain thread to exit. See [`CallbackDispatcher::close`]. + /// + /// A queue-mode subscriber has no dispatcher and runs no user code on the + /// delivery thread, so it reports [`CloseOutcome::Joined`] at once. + pub fn close(self, deadline: Duration) -> CloseOutcome { + // Undeclare first, for the same reason the field order does it: no new + // sample should enter a queue that is about to be discarded. + let dispatcher = match self { + Self::Plain { + subscriber, + dispatcher, + } => { + drop(subscriber); + dispatcher + } + Self::Advanced { + subscriber, + dispatcher, + } => { + drop(subscriber); + dispatcher + } + }; + match dispatcher { + Some(dispatcher) => dispatcher.close(deadline), + None => CloseOutcome::Joined, + } + } +} + impl std::fmt::Debug for SubscriberHandle { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { @@ -1589,18 +1741,22 @@ where /// pattern), so Python/Go callers do not need to assign the return value. /// Rust callers must store the `ZSub` in their node or context. /// - /// # Dropping blocks on an in-flight callback + /// # Dropping is not a barrier /// - /// A callback subscriber owns a drain thread. Dropping the `ZSub` joins that - /// thread, and the thread may be inside your callback. **Dropping therefore - /// blocks for as long as your callback runs, with no timeout.** + /// **Dropping a subscriber guarantees that no *new* callback starts. It does + /// not guarantee that a callback already running has finished.** The drop + /// returns at once: it closes the queue, discards the undelivered backlog + /// and detaches the drain thread. /// - /// Do not drop a callback subscriber while holding a lock, a GIL or a channel - /// that its callback also takes. The drop waits for the callback, and the - /// callback waits for the lock. Neither returns. + /// A callback that was already running therefore runs to completion, and may + /// still be running after `drop(sub)` returns. Do not assume that dropping + /// the subscriber releases a resource your callback captured. /// - /// Dropping from inside the callback itself is safe. The drain thread detaches - /// rather than joining itself. + /// Call [`ZSub::close`] when you need the barrier. It waits, with a deadline + /// you choose, and tells you whether the thread exited or was detached. + /// + /// Dropping from inside the callback itself is safe, and needs no special + /// case: the drop never waits for anything. /// /// # Arguments /// @@ -1699,6 +1855,35 @@ pub struct ZSub { _phantom_data: PhantomData<(T, Q, S)>, } +impl ZSub { + /// Tears the subscriber down and waits, for at most `deadline`, until no + /// callback of this subscriber is still running. + /// + /// Use this when you need a barrier. Plain `drop` gives you none: it + /// guarantees that no *new* callback starts, and returns without waiting + /// for one that is already running. + /// + /// This **discards the undelivered backlog**, exactly as `drop` does. It is + /// a teardown barrier, not a flush. + /// + /// ```text + /// match sub.close(Duration::from_secs(5)) { + /// CloseOutcome::Joined => // nothing of mine is still running + /// CloseOutcome::TimedOut => // a callback is still running; it was detached + /// } + /// ``` + pub fn close(self, deadline: Duration) -> CloseOutcome { + let ZSub { + _inner, _lv_token, .. + } = self; + let outcome = _inner.close(deadline); + // Same order the field declarations impose on `drop`: the subscriber + // goes away, then the liveliness token leaves the ROS graph. + drop(_lv_token); + outcome + } +} + impl std::fmt::Debug for ZSub { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("ZSub") @@ -1970,6 +2155,213 @@ mod tests { // That gap is #296's first item. // ----------------------------------------------------------------------- + // ----------------------------------------------------------------------- + // D1 -- the teardown contract (#296, tag G2) + // + // The contract: `Drop` sets `closed`, notifies, and NEVER joins. It returns + // at once. `close(deadline)` gives the caller the barrier when they want it, + // and reports which of the two things happened. + // + // `ci/revert-d1.patch` restores the join in `Drop`. Under that revert + // `drop_returns_while_a_callback_is_still_running` must fail. Every wait + // below has its own deadline, so the reverted run fails rather than hangs. + // ----------------------------------------------------------------------- + + /// A one-shot latch the test controls. `wait_for` returns whether the latch + /// was set before the deadline, so a failing run reports rather than hangs. + struct Latch { + set: Mutex, + cv: std::sync::Condvar, + } + + impl Latch { + fn new() -> Arc { + Arc::new(Self { + set: Mutex::new(false), + cv: std::sync::Condvar::new(), + }) + } + + fn set(&self) { + *self.set.lock().unwrap() = true; + self.cv.notify_all(); + } + + fn wait_for(&self, deadline: Duration) -> bool { + let start = std::time::Instant::now(); + let mut set = self.set.lock().unwrap(); + while !*set { + let Some(remaining) = deadline.checked_sub(start.elapsed()) else { + break; + }; + if remaining.is_zero() { + break; + } + let (guard, res) = self.cv.wait_timeout(set, remaining).unwrap(); + set = guard; + if res.timed_out() { + break; + } + } + *set + } + } + + /// Long enough that a genuinely blocking teardown cannot pass by luck, + /// short enough that a red run reports quickly. + const D1_SHORT: Duration = Duration::from_secs(5); + /// The upper bound on a parked callback. Only a red run ever waits this + /// long, and only to clean up after the assertion has already been made. + const D1_LONG: Duration = Duration::from_secs(60); + + fn d1_sample() -> Sample { + zenoh::sample::SampleBuilder::put( + zenoh::key_expr::KeyExpr::try_from("hiroz/test/d1").unwrap(), + vec![0u8], + ) + .into() + } + + /// Spawns a dispatcher whose callback parks on `release` until the test + /// lets it go, and signals `entered` as it starts. Returns the dispatcher + /// with one sample already enqueued and its callback confirmed running. + fn d1_parked_dispatcher( + entered: &Arc, + release: &Arc, + calls: &Arc, + ) -> CallbackDispatcher { + let (e, r, c) = (entered.clone(), release.clone(), calls.clone()); + let handler = Arc::new(move |_sample: Sample| { + c.fetch_add(1, Ordering::SeqCst); + e.set(); + assert!( + r.wait_for(D1_LONG), + "the parked callback was never released; the test leaked a thread" + ); + }); + let dispatcher = CallbackDispatcher::spawn("/d1", handler, 8).unwrap(); + let shim = dispatcher.always_shim(); + shim(d1_sample()); + assert!( + entered.wait_for(D1_SHORT), + "the callback never started, so the teardown assertion would be vacuous" + ); + dispatcher + } + + #[test] + fn drop_returns_while_a_callback_is_still_running() { + let entered = Latch::new(); + let release = Latch::new(); + let dropped = Latch::new(); + let calls = Arc::new(AtomicUsize::new(0)); + + let dispatcher = d1_parked_dispatcher(&entered, &release, &calls); + + // Drop from another thread, so this thread can time the drop while the + // callback is still parked. + let dropped_signal = dropped.clone(); + let dropper = std::thread::spawn(move || { + drop(dispatcher); + dropped_signal.set(); + }); + + let returned = dropped.wait_for(D1_SHORT); + // Release before asserting: a failing run must still terminate. + release.set(); + dropper.join().unwrap(); + + assert!( + returned, + "drop(dispatcher) did not return within {D1_SHORT:?} while a callback was \ + still running. Dropping a subscriber must stop new callbacks, not wait \ + for the in-flight one -- see #296 (tag G2)." + ); + } + + #[test] + fn close_waits_for_an_in_flight_callback() { + let entered = Latch::new(); + let release = Latch::new(); + let calls = Arc::new(AtomicUsize::new(0)); + + let dispatcher = d1_parked_dispatcher(&entered, &release, &calls); + + let releaser = { + let release = release.clone(); + std::thread::spawn(move || { + std::thread::sleep(Duration::from_millis(200)); + release.set(); + }) + }; + + let outcome = dispatcher.close(D1_SHORT); + releaser.join().unwrap(); + + assert_eq!( + outcome, + CloseOutcome::Joined, + "close() must report that it joined when the callback finishes in time" + ); + } + + #[test] + fn close_reports_a_timeout_rather_than_blocking_forever() { + let entered = Latch::new(); + let release = Latch::new(); + let calls = Arc::new(AtomicUsize::new(0)); + + let dispatcher = d1_parked_dispatcher(&entered, &release, &calls); + + let start = std::time::Instant::now(); + let outcome = dispatcher.close(Duration::from_millis(300)); + let waited = start.elapsed(); + release.set(); + + assert_eq!( + outcome, + CloseOutcome::TimedOut, + "a callback that outlives the deadline must be reported, not waited on" + ); + assert!( + waited < D1_SHORT, + "close() waited {waited:?}, far past its 300ms deadline" + ); + } + + #[test] + fn close_discards_the_backlog_rather_than_flushing_it() { + let entered = Latch::new(); + let release = Latch::new(); + let calls = Arc::new(AtomicUsize::new(0)); + + let dispatcher = d1_parked_dispatcher(&entered, &release, &calls); + + // Three more samples queue up behind the parked callback. + let shim = dispatcher.always_shim(); + for _ in 0..3 { + shim(d1_sample()); + } + + let releaser = { + let release = release.clone(); + std::thread::spawn(move || { + std::thread::sleep(Duration::from_millis(200)); + release.set(); + }) + }; + let outcome = dispatcher.close(D1_SHORT); + releaser.join().unwrap(); + + assert_eq!(outcome, CloseOutcome::Joined); + assert_eq!( + calls.load(Ordering::SeqCst), + 1, + "close() is a teardown barrier, not a flush: the three queued samples \ + must be discarded, exactly as drop discards them" + ); + } + #[test] fn volatile_does_not_need_an_advanced_subscriber() { let qos = hiroz_protocol::qos::QosProfile { From 58f59e656d09be266f83a5027f1d3cd07181c3fe Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 18 Aug 2026 02:41:45 +0800 Subject: [PATCH 24/30] fix(pubsub): guard the plain path's inline callback too The drain loop wraps the user callback in catch_unwind. The plain path does not always use that loop: local_only_shim enqueues only the samples the delivering thread published itself, and calls the handler inline for every other one -- which is every sample that arrived over a transport. That inline branch is the DEFAULT profile. qos_needs_advanced is true only for TransientLocal durability, and the ROS 2 default is Volatile, so a default subscriber takes the plain path and its inter-process traffic ran the callback with no guard at all. The guard protected the non-default profile and the local case, and left the common one open. zenoh does not catch it either: there is no catch_unwind on its subscriber-delivery path at 1.9.0, so the panic unwound out of hiroz and into a receive worker. Part of #296 (tag G7). --- crates/hiroz/src/pubsub.rs | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/crates/hiroz/src/pubsub.rs b/crates/hiroz/src/pubsub.rs index 224b8c13c..600864713 100644 --- a/crates/hiroz/src/pubsub.rs +++ b/crates/hiroz/src/pubsub.rs @@ -532,11 +532,33 @@ impl CallbackDispatcher { F: Fn(Sample) + Send + Sync + 'static, { let queue = self.queue.clone(); + let topic = self.queue.topic.clone(); move |sample: Sample| { if local_publish_active() { queue.enqueue(sample); } else { - handler(sample); + // The inline branch runs on a zenoh RX worker, so it needs the + // same guard the drain loop has. Without one, a panicking + // callback unwinds out of hiroz and into zenoh's receive path. + // + // This is the DEFAULT profile, which is what makes it matter: + // `qos_needs_advanced` is true only for `TransientLocal`, and + // the ROS 2 default is `Volatile`. Guarding only the drain + // thread therefore left every remote sample on the common path + // unguarded -- the half that carries inter-process traffic. + // + // The `panic = "abort"` caveat on the drain loop's guard + // applies here identically: a build that opts into aborting on + // a panic has opted out of surviving one. + if std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| handler(sample))) + .is_err() + { + tracing::error!( + topic = %topic, + "subscriber callback panicked on the inline path; dropping the \ + sample and continuing" + ); + } } } } From e343d510ec93e83877bd4dc6f3a444863a59b84a Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 18 Aug 2026 02:46:57 +0800 Subject: [PATCH 25/30] style(node): rustfmt the ported import block --- crates/hiroz/src/node.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/hiroz/src/node.rs b/crates/hiroz/src/node.rs index 92e5fc60a..950edfdd8 100644 --- a/crates/hiroz/src/node.rs +++ b/crates/hiroz/src/node.rs @@ -658,8 +658,8 @@ impl ZNode { use crate::{ entity::{EndpointEntity, EndpointKind}, pubsub::{ - CallbackDispatcher, SubscriberHandle, - apply_transient_local_sub, dispatch_capacity, qos_needs_advanced, + CallbackDispatcher, SubscriberHandle, apply_transient_local_sub, dispatch_capacity, + qos_needs_advanced, }, topic_name, }; From f89f30938b401e4e083292d1cb021a24aed03f37 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 18 Aug 2026 12:31:30 +0800 Subject: [PATCH 26/30] test(pubsub): pin the inline panic guard, both directions The guard added to local_only_shim's else arm shipped with no detector. A behavioural change with no demonstrated failing baseline is unjustified, and this closes that. Two tests, and the second is what makes the first mean anything: - a_panicking_callback_on_a_remote_sample_does_not_stop_delivery publishes from a separate context through a router, panics in the callback on one sample, and asserts later samples still arrive. - delivery_continues_without_a_panic runs the identical shape with the panic removed. Without it, 'nothing arrived after the panic' cannot be told apart from 'this configuration never delivered'. Three vacuity guards. The sample must be remote -- the shim's selector is a thread-local depth counter, not session or process locality, so a same-thread publish would take the enqueue arm and test the path the drain loop already guards; the test asserts the delivering thread was not hiroz-sub-drain. The file is gated on panic = 'unwind', so it skips visibly under the aborting profile instead of passing. And the panicking sample itself must have arrived, or the panic never happened. ci/verify-48.sh runs both directions and reports RESULT48. --- ci/revert-48.patch | 62 +++++ ci/verify-48.sh | 87 ++++++ .../hiroz-tests/tests/panic_guard_inline.rs | 256 ++++++++++++++++++ 3 files changed, 405 insertions(+) create mode 100644 ci/revert-48.patch create mode 100755 ci/verify-48.sh create mode 100644 crates/hiroz-tests/tests/panic_guard_inline.rs diff --git a/ci/revert-48.patch b/ci/revert-48.patch new file mode 100644 index 000000000..8e1c0d800 --- /dev/null +++ b/ci/revert-48.patch @@ -0,0 +1,62 @@ +commit 04d8cf65682be393abe391f747431f3a4d7f957a +Author: yuanyuyuan +Date: Tue Aug 18 02:41:45 2026 +0800 + + fix(pubsub): guard the plain path's inline callback too + + The drain loop wraps the user callback in catch_unwind. The plain path + does not always use that loop: local_only_shim enqueues only the samples + the delivering thread published itself, and calls the handler inline for + every other one -- which is every sample that arrived over a transport. + + That inline branch is the DEFAULT profile. qos_needs_advanced is true + only for TransientLocal durability, and the ROS 2 default is Volatile, so + a default subscriber takes the plain path and its inter-process traffic + ran the callback with no guard at all. The guard protected the + non-default profile and the local case, and left the common one open. + + zenoh does not catch it either: there is no catch_unwind on its + subscriber-delivery path at 1.9.0, so the panic unwound out of hiroz and + into a receive worker. + + Part of #296 (tag G7). + +diff --git b/crates/hiroz/src/pubsub.rs a/crates/hiroz/src/pubsub.rs +index 9b35e384..406db22d 100644 +--- b/crates/hiroz/src/pubsub.rs ++++ a/crates/hiroz/src/pubsub.rs +@@ -532,33 +532,11 @@ impl CallbackDispatcher { + F: Fn(Sample) + Send + Sync + 'static, + { + let queue = self.queue.clone(); +- let topic = self.queue.topic.clone(); + move |sample: Sample| { + if local_publish_active() { + queue.enqueue(sample); + } else { +- // The inline branch runs on a zenoh RX worker, so it needs the +- // same guard the drain loop has. Without one, a panicking +- // callback unwinds out of hiroz and into zenoh's receive path. +- // +- // This is the DEFAULT profile, which is what makes it matter: +- // `qos_needs_advanced` is true only for `TransientLocal`, and +- // the ROS 2 default is `Volatile`. Guarding only the drain +- // thread therefore left every remote sample on the common path +- // unguarded -- the half that carries inter-process traffic. +- // +- // The `panic = "abort"` caveat on the drain loop's guard +- // applies here identically: a build that opts into aborting on +- // a panic has opted out of surviving one. +- if std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| handler(sample))) +- .is_err() +- { +- tracing::error!( +- topic = %topic, +- "subscriber callback panicked on the inline path; dropping the \ +- sample and continuing" +- ); +- } ++ handler(sample); + } + } + } diff --git a/ci/verify-48.sh b/ci/verify-48.sh new file mode 100755 index 000000000..50bc6963f --- /dev/null +++ b/ci/verify-48.sh @@ -0,0 +1,87 @@ +#!/usr/bin/env bash +# Both-directions proof for the inline panic guard. +# +# Direction 1 (GREEN): the two tests pass as shipped. +# Direction 2 (RED): ci/revert-48.patch removes the catch_unwind from +# local_only_shim's else arm, and +# a_panicking_callback_on_a_remote_sample_does_not_stop_delivery +# must FAIL while the control still passes. +# +# The control passing in direction 2 is what separates "the guard was removed" +# from "this configuration cannot deliver at all". +# +# Read the RESULT line. The runner is `set -e` and this script ends in `echo`, +# so the job reports success either way. + +set +e +mkdir -p _tmp + +echo "HEAD=$(git rev-parse HEAD)" +FEATURES="ros-msgs,jazzy" +T=panic_guard_inline +PANIC_TEST=a_panicking_callback_on_a_remote_sample_does_not_stop_delivery +CONTROL_TEST=delivery_continues_without_a_panic + +# The file is gated `#![cfg(panic = "unwind")]`. Under an aborting profile it +# compiles to nothing and both directions would report 0 tests -- which must +# read as SKIPPED, never as passed. +run_both() { + cargo test -p hiroz-tests --features "$FEATURES" --test "$T" -- --test-threads=1 \ + > "$1" 2>&1 + echo $? +} + +echo "=== direction 1: as shipped ===" +GREEN=$(run_both _tmp/g48.log) +tail -20 _tmp/g48.log +G_PANIC=$(grep -c "^test $PANIC_TEST ... ok" _tmp/g48.log) +G_CTRL=$(grep -c "^test $CONTROL_TEST ... ok" _tmp/g48.log) +G_RAN=$(grep -cE "^test (a_panicking|delivery_continues)" _tmp/g48.log) +echo "GREEN=$GREEN G_RAN=$G_RAN G_PANIC_OK=$G_PANIC G_CTRL_OK=$G_CTRL" + +if [ "$G_RAN" -eq 0 ]; then + echo "RESULT48 VERDICT=SKIPPED reason=no_tests_ran_check_panic_strategy" + echo SCRIPT_DONE + exit 0 +fi + +git apply ci/revert-48.patch && APPLIED=yes || APPLIED=no +echo "REVERT_APPLIED=$APPLIED" +if [ "$APPLIED" != "yes" ]; then + echo "RESULT48 VERDICT=ABORT reason=revert_would_not_apply" + echo SCRIPT_DONE + exit 0 +fi + +echo "=== direction 2: guard removed from the inline branch ===" +RED=$(run_both _tmp/r48.log) +tail -30 _tmp/r48.log +R_PANIC_OK=$(grep -c "^test $PANIC_TEST ... ok" _tmp/r48.log) +R_CTRL_OK=$(grep -c "^test $CONTROL_TEST ... ok" _tmp/r48.log) +R_RAN=$(grep -cE "^test (a_panicking|delivery_continues)" _tmp/r48.log) +echo "RED=$RED R_RAN=$R_RAN R_PANIC_OK=$R_PANIC_OK R_CTRL_OK=$R_CTRL_OK" + +git apply -R ci/revert-48.patch +echo "REVERT_REVERSED_DIRTY=$(git status --porcelain crates/ | wc -l)" + +# A compile failure in direction 2 gives RED!=0 with R_RAN=0. Reading that as +# "the detector fired" is the trap rules/verification.md records: cargo prints +# `error: test failed` when tests fail, so a non-zero status proves nothing. +if [ "$R_RAN" -eq 0 ]; then + echo "RESULT48 VERDICT=INCONCLUSIVE reason=reverted_build_ran_no_tests green=$GREEN red=$RED" + echo SCRIPT_DONE + exit 0 +fi + +if [ "$GREEN" -eq 0 ] && [ "$R_PANIC_OK" -eq 0 ] && [ "$R_CTRL_OK" -eq 1 ]; then + V=PROVEN +elif [ "$GREEN" -eq 0 ] && [ "$R_PANIC_OK" -eq 1 ]; then + V=DETECTOR_DEAD +else + V=INCONCLUSIVE +fi + +echo "RESULT48 VERDICT=$V GREEN=$GREEN RED=$RED \ +G_PANIC_OK=$G_PANIC G_CTRL_OK=$G_CTRL R_PANIC_OK=$R_PANIC_OK R_CTRL_OK=$R_CTRL_OK \ +HEAD=$(git rev-parse HEAD)" +echo SCRIPT_DONE diff --git a/crates/hiroz-tests/tests/panic_guard_inline.rs b/crates/hiroz-tests/tests/panic_guard_inline.rs new file mode 100644 index 000000000..197ae165b --- /dev/null +++ b/crates/hiroz-tests/tests/panic_guard_inline.rs @@ -0,0 +1,256 @@ +//! The plain path's INLINE branch must survive a panicking callback. +//! +//! `CallbackDispatcher`'s drain loop wraps the user callback in `catch_unwind`. +//! The plain path does not always use that loop: `local_only_shim` enqueues only +//! the samples the delivering thread published itself, and calls the handler +//! inline for every other one — which is every sample that arrived over a +//! transport. +//! +//! That inline branch is the ROS 2 default. `qos_needs_advanced` is true only +//! for `TransientLocal` durability and the default is `Volatile`, so a default +//! subscriber takes the plain path and its inter-process traffic is exactly the +//! inline case. Before the guard was added there, a panicking callback unwound +//! out of hiroz and into a zenoh receive worker. +//! +//! # How this test avoids passing for the wrong reason +//! +//! Three properties, each of which would otherwise let it pass vacuously. +//! +//! 1. **The sample must be remote.** The shim's selector is +//! [`local_publish_active`], a THREAD-LOCAL depth counter — not session and +//! not process locality. A publish on the same thread takes the `if` arm and +//! exercises the already-guarded queue path, proving nothing about the +//! branch under test. Publisher and subscriber therefore use separate +//! contexts through a router, and the test asserts the delivering thread was +//! a zenoh receive worker rather than `hiroz-sub-drain`. +//! +//! 2. **Panics must unwind.** `catch_unwind` can only return `Err` where they +//! do, and this workspace's `[profile.opt]` sets `panic = "abort"`. The file +//! is gated so it SKIPS visibly on such a profile instead of passing. +//! +//! 3. **A positive control.** `delivery_continues_without_a_panic` runs the +//! identical shape with the panic removed. Without it, "the later samples +//! never arrived" would be indistinguishable from "delivery never worked +//! here at all". +//! +//! # Revert direction +//! +//! Remove the `catch_unwind` from `local_only_shim`'s `else` arm in +//! `crates/hiroz/src/pubsub.rs` — leaving a bare `handler(sample)` — and +//! `a_panicking_callback_on_a_remote_sample_does_not_stop_delivery` must fail. +//! +//! Part of #296 (tag G7). + +#![cfg(panic = "unwind")] + +mod common; + +use std::{ + collections::BTreeSet, + sync::{ + Arc, Mutex, + atomic::{AtomicU64, Ordering}, + mpsc, + }, + thread, + time::Duration, +}; + +use common::{TestRouter, create_hiroz_context_with_endpoint}; +use hiroz::{Builder, TypeHash, ros_msg::MessageTypeInfo}; +use serde::{Deserialize, Serialize}; +use serial_test::serial; + +/// Budget for one scenario. Generous relative to the work done. +const SCENARIO_TIMEOUT: Duration = Duration::from_secs(30); + +/// Samples published. The callback panics on exactly one of them. +const COUNT: u64 = 12; + +/// Which sample panics. Chosen so that several arrive before it and several +/// after, making "delivery stopped" distinguishable from "delivery never +/// started". +const PANIC_AT: u64 = 4; + +/// How long to wait for the delivered set to settle before asserting. +const SETTLE: Duration = Duration::from_secs(3); + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +struct Seq { + counter: u64, +} + +impl MessageTypeInfo for Seq { + fn type_name() -> &'static str { + "test_msgs::msg::dds_::Seq_" + } + + fn type_hash() -> TypeHash { + TypeHash::zero() + } +} + +impl hiroz::ros_msg::WithTypeInfo for Seq {} + +impl hiroz::msg::ZMessage for Seq { + type Serdes = hiroz::msg::SerdeCdrSerdes; +} + +/// Run `scenario` on its own thread and fail rather than hang. +fn run_with_deadline(name: &'static str, scenario: impl FnOnce() + Send + 'static) { + let (tx, rx) = mpsc::channel(); + thread::Builder::new() + .name(name.to_string()) + .spawn(move || { + scenario(); + let _ = tx.send(()); + }) + .expect("failed to spawn scenario thread"); + + if rx.recv_timeout(SCENARIO_TIMEOUT).is_err() { + panic!("`{name}` did not finish within {SCENARIO_TIMEOUT:?}"); + } +} + +/// What one run observed. +struct Observed { + /// Counter values the callback received. + seen: BTreeSet, + /// Names of the threads the callback ran on. + threads: BTreeSet, +} + +/// Publish `0..COUNT` from a SEPARATE context through `router`, into a +/// default-QoS callback subscriber. `panic_at` makes the callback panic on that +/// counter value; `None` is the positive control. +fn deliver_remote(endpoint: &str, topic: &str, panic_at: Option) -> Observed { + let sub_ctx = create_hiroz_context_with_endpoint(endpoint).expect("subscriber context"); + let pub_ctx = create_hiroz_context_with_endpoint(endpoint).expect("publisher context"); + + let sub_node = sub_ctx.create_node("panic_guard_sub").build().expect("sub node"); + let pub_node = pub_ctx.create_node("panic_guard_pub").build().expect("pub node"); + + let seen: Arc>> = Arc::new(Mutex::new(BTreeSet::new())); + let threads: Arc>> = Arc::new(Mutex::new(BTreeSet::new())); + let received = Arc::new(AtomicU64::new(0)); + + let c_seen = seen.clone(); + let c_threads = threads.clone(); + let c_recv = received.clone(); + + // Default QoS: Volatile, so this is the PLAIN path. + let _sub = sub_node + .create_sub::(topic) + .build_with_callback(move |msg: Seq| { + c_threads.lock().expect("threads poisoned").insert( + thread::current() + .name() + .unwrap_or("") + .to_string(), + ); + c_seen.lock().expect("seen poisoned").insert(msg.counter); + c_recv.fetch_add(1, Ordering::Relaxed); + if panic_at == Some(msg.counter) { + panic!("deliberate panic in a subscriber callback, counter={}", msg.counter); + } + }) + .expect("subscriber"); + + let zpub = pub_node.create_pub::(topic).build().expect("publisher"); + + // Let discovery settle so the first samples are not lost to a race. A lost + // early sample would weaken the "delivery started" half of the assertion. + thread::sleep(Duration::from_millis(500)); + + for counter in 0..COUNT { + zpub.publish(&Seq { counter }).expect("publish"); + thread::sleep(Duration::from_millis(50)); + } + + // Wait for the delivered set to go quiet rather than sleeping a fixed time. + let mut last = u64::MAX; + for _ in 0..(SETTLE.as_millis() / 100) { + let now = received.load(Ordering::Relaxed); + if now == last { + break; + } + last = now; + thread::sleep(Duration::from_millis(100)); + } + + let seen = seen.lock().expect("seen poisoned").clone(); + let threads = threads.lock().expect("threads poisoned").clone(); + Observed { seen, threads } +} + +/// GUARD 1: the samples must have arrived over a transport, on a zenoh receive +/// worker. If they came in on `hiroz-sub-drain` the enqueue arm was taken and +/// the already-guarded path was tested instead. +fn assert_took_the_inline_branch(o: &Observed, what: &str) { + assert!( + !o.threads.is_empty(), + "{what}: the callback never ran, so nothing was tested" + ); + assert!( + !o.threads.contains("hiroz-sub-drain"), + "{what}: delivery ran on `hiroz-sub-drain`, so the sample took \ + local_only_shim's enqueue arm. That is the queue path, which the drain \ + loop already guards — this test measured the wrong branch. \ + Threads seen: {:?}", + o.threads + ); +} + +#[test] +#[serial] +fn a_panicking_callback_on_a_remote_sample_does_not_stop_delivery() { + let router = TestRouter::new(); + let endpoint = router.endpoint().to_string(); + + run_with_deadline("panic_guard_inline", move || { + let o = deliver_remote(&endpoint, "/panic_guard_inline", Some(PANIC_AT)); + + assert_took_the_inline_branch(&o, "panic run"); + + assert!( + o.seen.contains(&PANIC_AT), + "the panicking sample itself never arrived, so the panic never \ + happened and nothing was tested. Seen: {:?}", + o.seen + ); + + let after: Vec = o.seen.iter().copied().filter(|c| *c > PANIC_AT).collect(); + assert!( + !after.is_empty(), + "no sample after the panic was delivered: the panicking callback \ + stopped the subscriber. The inline branch of `local_only_shim` \ + needs the same `catch_unwind` the drain loop has. Seen: {:?}", + o.seen + ); + }); +} + +/// GUARD 3: the positive control. Without it, a failure above could mean +/// "delivery never worked in this configuration" rather than "the panic +/// stopped it". +#[test] +#[serial] +fn delivery_continues_without_a_panic() { + let router = TestRouter::new(); + let endpoint = router.endpoint().to_string(); + + run_with_deadline("panic_guard_control", move || { + let o = deliver_remote(&endpoint, "/panic_guard_control", None); + + assert_took_the_inline_branch(&o, "control run"); + + let after: Vec = o.seen.iter().copied().filter(|c| *c > PANIC_AT).collect(); + assert!( + !after.is_empty(), + "the control run delivered nothing after counter {PANIC_AT}, so this \ + configuration cannot deliver at all and the panic test above proves \ + nothing either way. Seen: {:?}", + o.seen + ); + }); +} From c631096cebcf71b5a19e723e289a9b328120c2c1 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 18 Aug 2026 12:34:18 +0800 Subject: [PATCH 27/30] test(ci): count detector outcomes from the failures block Job 3943 reported G_PANIC_OK=0 on a run where the test passed. The tracing ERROR the guard emits interleaves into stdout and splits 'test ... ok' across two lines, so the per-test grep matched nothing. The verdict was unaffected -- it gates on the cargo exit status, not on that count -- but a confusing number inside an evidence line is worth fixing rather than explaining. Count names appearing under 'failures:' instead, which no interleaving can break, and require the shipped direction to show zero failures rather than inferring it. --- ci/verify-48.sh | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/ci/verify-48.sh b/ci/verify-48.sh index 50bc6963f..a69036e12 100755 --- a/ci/verify-48.sh +++ b/ci/verify-48.sh @@ -34,10 +34,13 @@ run_both() { echo "=== direction 1: as shipped ===" GREEN=$(run_both _tmp/g48.log) tail -20 _tmp/g48.log -G_PANIC=$(grep -c "^test $PANIC_TEST ... ok" _tmp/g48.log) -G_CTRL=$(grep -c "^test $CONTROL_TEST ... ok" _tmp/g48.log) +# Count from the FAILURES block, not the per-test line: tracing output interleaves +# into stdout and splits `test ... ok` across two lines, so matching that +# pattern silently reports 0 for a test that passed. +G_PANIC_FAIL=$(grep -cE "^ $PANIC_TEST$" _tmp/g48.log) +G_CTRL_FAIL=$(grep -cE "^ $CONTROL_TEST$" _tmp/g48.log) G_RAN=$(grep -cE "^test (a_panicking|delivery_continues)" _tmp/g48.log) -echo "GREEN=$GREEN G_RAN=$G_RAN G_PANIC_OK=$G_PANIC G_CTRL_OK=$G_CTRL" +echo "GREEN=$GREEN G_RAN=$G_RAN G_PANIC_FAIL=$G_PANIC_FAIL G_CTRL_FAIL=$G_CTRL_FAIL" if [ "$G_RAN" -eq 0 ]; then echo "RESULT48 VERDICT=SKIPPED reason=no_tests_ran_check_panic_strategy" @@ -56,10 +59,10 @@ fi echo "=== direction 2: guard removed from the inline branch ===" RED=$(run_both _tmp/r48.log) tail -30 _tmp/r48.log -R_PANIC_OK=$(grep -c "^test $PANIC_TEST ... ok" _tmp/r48.log) -R_CTRL_OK=$(grep -c "^test $CONTROL_TEST ... ok" _tmp/r48.log) +R_PANIC_FAIL=$(grep -cE "^ $PANIC_TEST$" _tmp/r48.log) +R_CTRL_FAIL=$(grep -cE "^ $CONTROL_TEST$" _tmp/r48.log) R_RAN=$(grep -cE "^test (a_panicking|delivery_continues)" _tmp/r48.log) -echo "RED=$RED R_RAN=$R_RAN R_PANIC_OK=$R_PANIC_OK R_CTRL_OK=$R_CTRL_OK" +echo "RED=$RED R_RAN=$R_RAN R_PANIC_FAIL=$R_PANIC_FAIL R_CTRL_FAIL=$R_CTRL_FAIL" git apply -R ci/revert-48.patch echo "REVERT_REVERSED_DIRTY=$(git status --porcelain crates/ | wc -l)" @@ -73,15 +76,15 @@ if [ "$R_RAN" -eq 0 ]; then exit 0 fi -if [ "$GREEN" -eq 0 ] && [ "$R_PANIC_OK" -eq 0 ] && [ "$R_CTRL_OK" -eq 1 ]; then +if [ "$GREEN" -eq 0 ] && [ "$G_PANIC_FAIL" -eq 0 ] && [ "$R_PANIC_FAIL" -eq 1 ] && [ "$R_CTRL_FAIL" -eq 0 ]; then V=PROVEN -elif [ "$GREEN" -eq 0 ] && [ "$R_PANIC_OK" -eq 1 ]; then +elif [ "$GREEN" -eq 0 ] && [ "$R_PANIC_FAIL" -eq 0 ]; then V=DETECTOR_DEAD else V=INCONCLUSIVE fi echo "RESULT48 VERDICT=$V GREEN=$GREEN RED=$RED \ -G_PANIC_OK=$G_PANIC G_CTRL_OK=$G_CTRL R_PANIC_OK=$R_PANIC_OK R_CTRL_OK=$R_CTRL_OK \ +G_PANIC_FAIL=$G_PANIC_FAIL G_CTRL_FAIL=$G_CTRL_FAIL R_PANIC_FAIL=$R_PANIC_FAIL R_CTRL_FAIL=$R_CTRL_FAIL \ HEAD=$(git rev-parse HEAD)" echo SCRIPT_DONE From 7eeab06b5ffd75536e48885bce3d9bf4a09bb64c Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 18 Aug 2026 12:55:48 +0800 Subject: [PATCH 28/30] chore: drop the CI verification artifacts, rustfmt the new test Two problems with the previous commit. The ci/ directory does not belong in this pull request. It did not exist before, and the equivalent artifact for the teardown fix was dropped from this branch for the same reason: a revert patch and its runner are internal verification plumbing, not part of the change under review. The test itself is the deliverable; the pull request body already states the revert that reddens it, in one line a reviewer can follow. The new test file was also not rustfmt-clean. It was added after the formatting gate last ran, and the detector script that did run does not check formatting. --- ci/revert-48.patch | 62 ------------- ci/verify-48.sh | 90 ------------------- .../hiroz-tests/tests/panic_guard_inline.rs | 10 ++- 3 files changed, 8 insertions(+), 154 deletions(-) delete mode 100644 ci/revert-48.patch delete mode 100755 ci/verify-48.sh diff --git a/ci/revert-48.patch b/ci/revert-48.patch deleted file mode 100644 index 8e1c0d800..000000000 --- a/ci/revert-48.patch +++ /dev/null @@ -1,62 +0,0 @@ -commit 04d8cf65682be393abe391f747431f3a4d7f957a -Author: yuanyuyuan -Date: Tue Aug 18 02:41:45 2026 +0800 - - fix(pubsub): guard the plain path's inline callback too - - The drain loop wraps the user callback in catch_unwind. The plain path - does not always use that loop: local_only_shim enqueues only the samples - the delivering thread published itself, and calls the handler inline for - every other one -- which is every sample that arrived over a transport. - - That inline branch is the DEFAULT profile. qos_needs_advanced is true - only for TransientLocal durability, and the ROS 2 default is Volatile, so - a default subscriber takes the plain path and its inter-process traffic - ran the callback with no guard at all. The guard protected the - non-default profile and the local case, and left the common one open. - - zenoh does not catch it either: there is no catch_unwind on its - subscriber-delivery path at 1.9.0, so the panic unwound out of hiroz and - into a receive worker. - - Part of #296 (tag G7). - -diff --git b/crates/hiroz/src/pubsub.rs a/crates/hiroz/src/pubsub.rs -index 9b35e384..406db22d 100644 ---- b/crates/hiroz/src/pubsub.rs -+++ a/crates/hiroz/src/pubsub.rs -@@ -532,33 +532,11 @@ impl CallbackDispatcher { - F: Fn(Sample) + Send + Sync + 'static, - { - let queue = self.queue.clone(); -- let topic = self.queue.topic.clone(); - move |sample: Sample| { - if local_publish_active() { - queue.enqueue(sample); - } else { -- // The inline branch runs on a zenoh RX worker, so it needs the -- // same guard the drain loop has. Without one, a panicking -- // callback unwinds out of hiroz and into zenoh's receive path. -- // -- // This is the DEFAULT profile, which is what makes it matter: -- // `qos_needs_advanced` is true only for `TransientLocal`, and -- // the ROS 2 default is `Volatile`. Guarding only the drain -- // thread therefore left every remote sample on the common path -- // unguarded -- the half that carries inter-process traffic. -- // -- // The `panic = "abort"` caveat on the drain loop's guard -- // applies here identically: a build that opts into aborting on -- // a panic has opted out of surviving one. -- if std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| handler(sample))) -- .is_err() -- { -- tracing::error!( -- topic = %topic, -- "subscriber callback panicked on the inline path; dropping the \ -- sample and continuing" -- ); -- } -+ handler(sample); - } - } - } diff --git a/ci/verify-48.sh b/ci/verify-48.sh deleted file mode 100755 index a69036e12..000000000 --- a/ci/verify-48.sh +++ /dev/null @@ -1,90 +0,0 @@ -#!/usr/bin/env bash -# Both-directions proof for the inline panic guard. -# -# Direction 1 (GREEN): the two tests pass as shipped. -# Direction 2 (RED): ci/revert-48.patch removes the catch_unwind from -# local_only_shim's else arm, and -# a_panicking_callback_on_a_remote_sample_does_not_stop_delivery -# must FAIL while the control still passes. -# -# The control passing in direction 2 is what separates "the guard was removed" -# from "this configuration cannot deliver at all". -# -# Read the RESULT line. The runner is `set -e` and this script ends in `echo`, -# so the job reports success either way. - -set +e -mkdir -p _tmp - -echo "HEAD=$(git rev-parse HEAD)" -FEATURES="ros-msgs,jazzy" -T=panic_guard_inline -PANIC_TEST=a_panicking_callback_on_a_remote_sample_does_not_stop_delivery -CONTROL_TEST=delivery_continues_without_a_panic - -# The file is gated `#![cfg(panic = "unwind")]`. Under an aborting profile it -# compiles to nothing and both directions would report 0 tests -- which must -# read as SKIPPED, never as passed. -run_both() { - cargo test -p hiroz-tests --features "$FEATURES" --test "$T" -- --test-threads=1 \ - > "$1" 2>&1 - echo $? -} - -echo "=== direction 1: as shipped ===" -GREEN=$(run_both _tmp/g48.log) -tail -20 _tmp/g48.log -# Count from the FAILURES block, not the per-test line: tracing output interleaves -# into stdout and splits `test ... ok` across two lines, so matching that -# pattern silently reports 0 for a test that passed. -G_PANIC_FAIL=$(grep -cE "^ $PANIC_TEST$" _tmp/g48.log) -G_CTRL_FAIL=$(grep -cE "^ $CONTROL_TEST$" _tmp/g48.log) -G_RAN=$(grep -cE "^test (a_panicking|delivery_continues)" _tmp/g48.log) -echo "GREEN=$GREEN G_RAN=$G_RAN G_PANIC_FAIL=$G_PANIC_FAIL G_CTRL_FAIL=$G_CTRL_FAIL" - -if [ "$G_RAN" -eq 0 ]; then - echo "RESULT48 VERDICT=SKIPPED reason=no_tests_ran_check_panic_strategy" - echo SCRIPT_DONE - exit 0 -fi - -git apply ci/revert-48.patch && APPLIED=yes || APPLIED=no -echo "REVERT_APPLIED=$APPLIED" -if [ "$APPLIED" != "yes" ]; then - echo "RESULT48 VERDICT=ABORT reason=revert_would_not_apply" - echo SCRIPT_DONE - exit 0 -fi - -echo "=== direction 2: guard removed from the inline branch ===" -RED=$(run_both _tmp/r48.log) -tail -30 _tmp/r48.log -R_PANIC_FAIL=$(grep -cE "^ $PANIC_TEST$" _tmp/r48.log) -R_CTRL_FAIL=$(grep -cE "^ $CONTROL_TEST$" _tmp/r48.log) -R_RAN=$(grep -cE "^test (a_panicking|delivery_continues)" _tmp/r48.log) -echo "RED=$RED R_RAN=$R_RAN R_PANIC_FAIL=$R_PANIC_FAIL R_CTRL_FAIL=$R_CTRL_FAIL" - -git apply -R ci/revert-48.patch -echo "REVERT_REVERSED_DIRTY=$(git status --porcelain crates/ | wc -l)" - -# A compile failure in direction 2 gives RED!=0 with R_RAN=0. Reading that as -# "the detector fired" is the trap rules/verification.md records: cargo prints -# `error: test failed` when tests fail, so a non-zero status proves nothing. -if [ "$R_RAN" -eq 0 ]; then - echo "RESULT48 VERDICT=INCONCLUSIVE reason=reverted_build_ran_no_tests green=$GREEN red=$RED" - echo SCRIPT_DONE - exit 0 -fi - -if [ "$GREEN" -eq 0 ] && [ "$G_PANIC_FAIL" -eq 0 ] && [ "$R_PANIC_FAIL" -eq 1 ] && [ "$R_CTRL_FAIL" -eq 0 ]; then - V=PROVEN -elif [ "$GREEN" -eq 0 ] && [ "$R_PANIC_FAIL" -eq 0 ]; then - V=DETECTOR_DEAD -else - V=INCONCLUSIVE -fi - -echo "RESULT48 VERDICT=$V GREEN=$GREEN RED=$RED \ -G_PANIC_FAIL=$G_PANIC_FAIL G_CTRL_FAIL=$G_CTRL_FAIL R_PANIC_FAIL=$R_PANIC_FAIL R_CTRL_FAIL=$R_CTRL_FAIL \ -HEAD=$(git rev-parse HEAD)" -echo SCRIPT_DONE diff --git a/crates/hiroz-tests/tests/panic_guard_inline.rs b/crates/hiroz-tests/tests/panic_guard_inline.rs index 197ae165b..fafe78691 100644 --- a/crates/hiroz-tests/tests/panic_guard_inline.rs +++ b/crates/hiroz-tests/tests/panic_guard_inline.rs @@ -151,12 +151,18 @@ fn deliver_remote(endpoint: &str, topic: &str, panic_at: Option) -> Observe c_seen.lock().expect("seen poisoned").insert(msg.counter); c_recv.fetch_add(1, Ordering::Relaxed); if panic_at == Some(msg.counter) { - panic!("deliberate panic in a subscriber callback, counter={}", msg.counter); + panic!( + "deliberate panic in a subscriber callback, counter={}", + msg.counter + ); } }) .expect("subscriber"); - let zpub = pub_node.create_pub::(topic).build().expect("publisher"); + let zpub = pub_node + .create_pub::(topic) + .build() + .expect("publisher"); // Let discovery settle so the first samples are not lost to a race. A lost // early sample would weaken the "delivery started" half of the assertion. From 115889f3eb50e5d50c0ec15de7c07558ca2cc74c Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 18 Aug 2026 13:07:30 +0800 Subject: [PATCH 29/30] test(pubsub): migrate the teardown test to close(deadline) The interop job caught this: transient_local_subscriber_drop_shuts_down_delivery_thread asserted that the delivery thread had torn down immediately after drop(sub). That held while Drop joined. It does not hold now -- Drop guarantees no NEW callback starts, not that a running one has finished, so the thread winds down asynchronously and the assertion raced. The test now uses close(deadline) and asserts Joined, which is exactly the migration BC5 asks of any caller who relied on the old behaviour. The test is therefore also the worked example for that row. Two doc comments stated the old contract and are corrected: the self-drop case no longer has a self-join to detect, because nothing joins. My local gate ran only 'cargo test -p hiroz --lib pubsub::tests::' -- fourteen in-module unit tests -- and never -p hiroz-tests, which is why this reached CI instead of the worker. --- crates/hiroz-tests/tests/reentrant_publish.rs | 33 ++++++++++++++----- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/crates/hiroz-tests/tests/reentrant_publish.rs b/crates/hiroz-tests/tests/reentrant_publish.rs index f29df1b26..4dc4407e5 100644 --- a/crates/hiroz-tests/tests/reentrant_publish.rs +++ b/crates/hiroz-tests/tests/reentrant_publish.rs @@ -38,6 +38,7 @@ use std::{ use common::{TestRouter, create_hiroz_context_with_endpoint}; use hiroz::{ Builder, TypeHash, + pubsub::CloseOutcome, qos::{QosDurability, QosProfile}, ros_msg::MessageTypeInfo, }; @@ -701,7 +702,13 @@ fn transient_local_delivery_preserves_order() { }); } -/// Dropping the subscriber must shut the delivery thread down — no leak, no hang. +/// `close(deadline)` must shut the delivery thread down — no leak, no hang. +/// +/// `Drop` alone no longer does this. It guarantees that no *new* callback +/// starts, not that a running one has finished, so the thread winds down +/// asynchronously and this assertion would race. `close` is the barrier, and +/// this test is the migration BC5 asks callers who relied on the old behaviour +/// to make. /// /// The sentinel is owned by the user callback, which the delivery thread owns in /// turn, so its `Drop` firing proves the thread actually exited and released the @@ -759,12 +766,18 @@ fn transient_local_subscriber_drop_shuts_down_delivery_thread() { "callback was dropped while the subscriber was still alive" ); - // If `Drop` deadlocked or the join hung, the deadline guard fails the test. - drop(sub); + // `close` is the barrier; `Drop` is not. If it hung, the deadline guard + // fails the test. + let outcome = sub.close(Duration::from_secs(5)); + assert_eq!( + outcome, + CloseOutcome::Joined, + "close() timed out: the drain thread did not exit within the deadline" + ); assert!( callback_dropped.load(Ordering::SeqCst), - "subscriber dropped without shutting down its delivery thread — \ + "close() reported Joined but the callback was never dropped — \ the thread is leaked" ); }); @@ -772,9 +785,12 @@ fn transient_local_subscriber_drop_shuts_down_delivery_thread() { /// Dropping the subscriber *from inside its own callback* must not deadlock. /// -/// That drop runs on the delivery thread itself, so joining the thread would be -/// a self-join. The dispatcher detects this and lets the thread wind itself down -/// instead. +/// That drop runs on the delivery thread itself. `Drop` detaches rather than +/// joining, so there is no self-join to detect and no special case guarding it — +/// the thread observes `closed` and winds itself down once the callback returns. +/// +/// Calling `close` here instead would report `TimedOut` at once: a thread cannot +/// wait for itself. #[test] #[serial] fn transient_local_subscriber_dropped_inside_its_own_callback_does_not_deadlock() { @@ -804,7 +820,8 @@ fn transient_local_subscriber_dropped_inside_its_own_callback_does_not_deadlock( .create_sub::("/reentrant_tl_self_drop") .with_qos(transient_local()) .build_with_callback(move |_msg: Tick| { - // Runs on the delivery thread; this drop is the self-join case. + // Runs on the delivery thread. The drop detaches; the thread + // exits once this callback returns. let taken = cb_slot.lock().unwrap().take(); drop(taken); cb_dropped.store(true, Ordering::SeqCst); From 14d4a8e06c408aa7c78b92447abbe3b1ca3f8ad8 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 18 Aug 2026 13:14:13 +0800 Subject: [PATCH 30/30] style(tests): rustfmt the panic guard test --- .../hiroz-tests/tests/panic_guard_inline.rs | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/crates/hiroz-tests/tests/panic_guard_inline.rs b/crates/hiroz-tests/tests/panic_guard_inline.rs index fafe78691..4a2985b1b 100644 --- a/crates/hiroz-tests/tests/panic_guard_inline.rs +++ b/crates/hiroz-tests/tests/panic_guard_inline.rs @@ -127,8 +127,14 @@ fn deliver_remote(endpoint: &str, topic: &str, panic_at: Option) -> Observe let sub_ctx = create_hiroz_context_with_endpoint(endpoint).expect("subscriber context"); let pub_ctx = create_hiroz_context_with_endpoint(endpoint).expect("publisher context"); - let sub_node = sub_ctx.create_node("panic_guard_sub").build().expect("sub node"); - let pub_node = pub_ctx.create_node("panic_guard_pub").build().expect("pub node"); + let sub_node = sub_ctx + .create_node("panic_guard_sub") + .build() + .expect("sub node"); + let pub_node = pub_ctx + .create_node("panic_guard_pub") + .build() + .expect("pub node"); let seen: Arc>> = Arc::new(Mutex::new(BTreeSet::new())); let threads: Arc>> = Arc::new(Mutex::new(BTreeSet::new())); @@ -142,12 +148,10 @@ fn deliver_remote(endpoint: &str, topic: &str, panic_at: Option) -> Observe let _sub = sub_node .create_sub::(topic) .build_with_callback(move |msg: Seq| { - c_threads.lock().expect("threads poisoned").insert( - thread::current() - .name() - .unwrap_or("") - .to_string(), - ); + c_threads + .lock() + .expect("threads poisoned") + .insert(thread::current().name().unwrap_or("").to_string()); c_seen.lock().expect("seen poisoned").insert(msg.counter); c_recv.fetch_add(1, Ordering::Relaxed); if panic_at == Some(msg.counter) {