fix(pubsub): stop running subscriber callbacks on the publishing thread by YuanYuYuan · Pull Request #250 · ZettaScaleLabs/hiroz

YuanYuYuan · 2026-07-27T19:59:39Z

Part of #282 — that issue states the defect class and the shared fix shape.

Summary

Publishing from inside a subscriber callback on the same session deadlocked deterministically. This PR moves session-local delivery off the publishing thread onto a bounded per-subscriber drain queue, and stops declaring zenoh-ext's AdvancedSubscriber for QoS profiles that do not need it.

Instance fix for #282. It targets main and uses no tracked lock types, so it can merge in any order relative to the keystone (#255).

The problem

Fixes #249.

zenoh-ext's AdvancedSubscriber invokes the user callback while holding its own state mutex. That mutex is a std::sync::Mutex, which is not re-entrant. zenoh delivers a session-local sample inline on the publishing thread. A callback that publishes to its own topic therefore blocks on a lock its own thread already holds.

sequenceDiagram
    autonumber
    participant App as User thread
    participant Ext as AdvancedSubscriber
    participant Cb as User callback

    App->>Ext: publish(topic, m1)
    activate Ext
    Ext->>Ext: lock state mutex
    Ext->>Cb: invoke callback — lock held
    activate Cb
    Cb->>Ext: publish(topic, m2) — same thread
    Ext--xExt: lock state mutex — already held
    Note over Ext: std::sync::Mutex is not re-entrant → deadlock
    deactivate Cb
    deactivate Ext

The fix

The callout now runs on a separate drain thread, holding nothing — the acq · rel · callout shape:

sequenceDiagram
    autonumber
    participant App as User thread
    participant Q as Bounded drain queue
    participant Drain as hiroz-sub-drain thread
    participant Cb as User callback

    App->>Q: publish(topic, m1) → enqueue, evicting if full
    Note over App: publish() returns immediately
    Q->>Drain: dequeue (state lock released before the callout)
    Drain->>Cb: invoke callback — holding nothing
    activate Cb
    Cb->>Q: publish(topic, m2) → enqueue
    Note over Q,Cb: no lock is held, so this returns.<br/>The loop iterates instead of recursing.
    deactivate Cb

Four changes:

#	Change	Why
1	Declare `AdvancedSubscriber` only when QoS needs it (`qos_needs_advanced`)	It is the component holding the non-reentrant lock across the callout. For `Volatile` — the ROS 2 default — the wrapper added no liveliness subscriber, heartbeat or detection token, so it was pure overhead plus that lock. `TransientLocal` keeps it, for history replay and miss recovery
2	Move session-local delivery onto a bounded drain queue (`CallbackDispatcher::spawn`)	The publishing thread no longer runs the callback, so it cannot re-enter its own lock
3	`Drop` discards the backlog and never joins; `close(deadline)` is the opt-in barrier	Draining at teardown would run a callback per queued sample — `backlog × callback_duration`, unbounded. Joining would wait on the in-flight one, which is the same unbounded callout at a different site
4	Guard the user callback on both delivery branches	The drain loop wraps it in `catch_unwind`. The plain path's inline branch — the default for remote samples — did not, so a panicking callback unwound into a zenoh receive worker

All four CallbackDispatcher::spawn sites pass dispatch_capacity — the plain and advanced arms of both the typed builder and the FFI raw subscriber.

Teardown, and why `drop` is not a barrier

No design keeps all three of these:

callbacks do not run inside the publishing thread's put
drop(sub) returns promptly
no callback runs after drop(sub) returns

main has (2) and (3) and lacks (1) — that is the deadlock. This PR takes (1) and (2). A callback may therefore still be running after drop(sub) returns. Joining with a deadline was considered and rejected: it still blocks and still permits a post-drop callback. Call close(deadline) when you need the barrier, and branch on Joined / TimedOut.

Before / after

What a user observes:

	Before	After
Publish from inside a subscriber callback	deadlocks, deterministically	returns; the loop iterates
`Volatile` subscriber	declares `AdvancedSubscriber`, which holds a non-reentrant mutex across the callout	declares a plain subscriber
Session-local delivery	runs the callback inline on the publishing thread	enqueues; a drain thread runs the callback holding no lock
`publish()` returning	means the subscriber has already run	means the sample is queued
Undelivered same-session samples	none exist — delivery is inline, so loss is structurally impossible	bounded at the history depth, drop-oldest, with an escalating `warn!`
`drop(subscriber)`	returns at once; there is no queue and no thread to join	returns at once, discarding the backlog; guarantees no new callback starts. `close(deadline)` is the barrier

Breaking changes

BC1–BC6 follow from moving session-local delivery off the publishing thread. BC7 and BC8 are API surface.

tag	What changes	Who is affected	Before → after	Action
BC1	Session-local delivery is asynchronous	Anyone publishing and then asserting on a same-session side effect	`publish()` returning meant the subscriber had run → it does not	Synchronise explicitly
BC2	Same-session samples can be dropped	Callback subscribers with `KeepLast(depth)`	Inline delivery made loss structurally impossible → the queue is bounded at the history depth and drops oldest, with an escalating `warn!`	Use `KeepAll` if losslessness is required
BC3	Subscriber callbacks are no longer mutually excluded	Callbacks doing a non-atomic read-modify-write	zenoh-ext invoked the callback under its `Mutex<State>`, so callbacks were serialised by construction → they are not now	Add your own synchronisation
BC4	Local and remote publications on one topic are no longer ordered relative to each other	Plain (non-`TransientLocal`) subscribers	Single interleaved order → two independent paths	Do not rely on cross-source ordering
BC5	Dropping a subscriber discards its undelivered backlog, and a callback already running may outlive the drop	Anyone dropping a subscriber; anyone relying on teardown draining or fencing	Every accepted sample was delivered inline before `publish` returned → the backlog is discarded, as destroying an rclcpp subscription does, and `Drop` guarantees only that no new callback starts	Call `close(deadline)` and branch on `Joined` / `TimedOut`. Drain before dropping if the backlog matters
BC6	New public types	`ffi` consumers	`SubscriberHandle` and `CallbackDispatcher` are new public items; `RawSubscriber::inner` changes from `AdvancedSubscriber<()>` to `SubscriberHandle`	Update any direct use of `inner`
BC7	Synchronous local delivery is gone as a guarantee	Downstream code only	Nothing in this repo depended on it — `rmw-zenoh-rs` uses `build_with_notifier`, and `wait_for_subscription` reads the graph	—
BC8	New public API: `close(deadline)` and `CloseOutcome`	Glob-importers of `hiroz::prelude`	absent → present on `CallbackDispatcher`, `ZSub` and `SubscriberHandle`	None, unless a local type named `CloseOutcome` collides

Important

Scope qualifier on BC2. It applies to every sample only on the advanced (TransientLocal) path, where the shim enqueues unconditionally. On the plain path only locally published samples pass through the queue; a remote sample runs the callback inline and is neither queued nor dropped. See L1 below.

Comparison with `rmw_zenoh_cpp`

Read at e95c62d. Upstream's zenoh callback only calls SubscriptionData::add_new_message, which locks, bounds, enqueues and notifies. User code runs later on the rclcpp executor thread, never on a zenoh delivery thread.

Aligned: the bound itself, drop-oldest ordering, KEEP_ALL → unbounded, no TransientLocal exemption, the advanced-subscriber cache depth, and teardown semantics.

Four rows differ:

	`rmw_zenoh_cpp`	hiroz after this PR
what the bound covers	every arriving sample, remote included	advanced path yes; plain path local samples only	divergent — L1
notifier on the delivery thread	`trigger_callback()` runs under `mutex_`	`notifier()` runs after `push` returns, holding nothing	hiroz better — upstream has the #282 shape here
publisher backpressure	`BLOCK` only for `RELIABLE && KEEP_ALL`	`BLOCK` for every `Reliable`	hiroz worse, pre-existing — L3
advanced subscriber	declared unconditionally	plain subscriber for `Volatile`	divergent by design — change 1

Testing

17 integration tests across reentrant_publish.rs (11), dispatch_backpressure.rs (4) and panic_guard_inline.rs (2); 16 unit tests in pubsub.rs covering the QoS gating, the teardown contract and close(deadline); 6 parametrised Python cases.

Each covers a property with a stated revert that reddens it:

Property	Test
A callback publishing to its own topic iterates instead of recursing, on both paths	`self_feeding_callback_loop_iterates_without_a_depth_cap`, `transient_local_self_feeding_callback_loop_iterates`, `intra_closed_loop_runs_iteratively`
`AdvancedSubscriber` is declared only for `TransientLocal`	`volatile_does_not_need_an_advanced_subscriber`, `transient_local_needs_an_advanced_subscriber`
`KeepLast(depth)` drops oldest; `KeepAll` is lossless	the four `dispatch_backpressure` tests, on both paths
`drop(subscriber)` returns while a callback is still running	`drop_returns_while_a_callback_is_still_running`
`close(deadline)` waits, reports a timeout rather than blocking forever, and discards the backlog	the three `close_*` tests
A panicking callback on a remote sample does not stop delivery	`a_panicking_callback_on_a_remote_sample_does_not_stop_delivery`, with `delivery_continues_without_a_panic` as the positive control
Publishing from a callback does not hold the GIL	`test_interpreter_stays_alive_during_reentrant_publish`

This PR changes one existing test: transient_local_subscriber_drop_shuts_down_delivery_thread moves from drop(sub) to close(deadline), which is the migration BC5 asks of any caller who relied on the old behaviour.

Known limitations

Each is either pre-existing or a consequence of the fix. Follow-up PRs are planned for L1, L3 and L4.

tag	Limitation	Status
L1	The plain path bounds locally published samples only. A remote sample runs the callback inline on a receive worker and never enters the queue. Upstream bounds every arriving sample	Follow-up planned. Measured cost of closing it: +13.5 µs per sample at 50 % CPU load. The benefit is isolation — with two slow callbacks occupying both receive workers, an unrelated subscriber in the same process degrades from 78 µs to 10.7 s and loses half its samples
L2	`LocalPublishGuard` is `pub(crate)`, so its requirement that every publish path take it is unenforceable outside the crate. One out-of-crate `session.put` exists, in the WASM plugin host, which opens a session of its own and whose subscribers are raw zenoh handles rather than `ZSub`s	Enforcement gap, no reachable failure
L3	Pre-existing: `Reliable` maps to `CongestionControl::Block` on every history policy, where upstream uses `Block` only for `RELIABLE && KEEP_ALL`	Follow-up planned
L4	Pre-existing: `destroy_subscriber` resolves by a per-node id carrying no node identity, so two nodes in one interpreter can both mint `owned_id == 0`	Follow-up planned
L5	Each callback subscriber gets its own drain thread, spawned at build time: +12 KB RSS and +66 MB of address space per subscriber, bounded and non-leaking. `MALLOC_ARENA_MAX=2` removes 64 of the 66 MB. The rmw path is unaffected — `rmw-zenoh-rs` uses `build_with_notifier`, so no dispatcher is created	Tracked in #302
L6	The panic guard is inert wherever `panic = "abort"` is set, which `[profile.opt]` does. The drain-loop guard also has no test in the tree; only the inline branch is pinned	—

Also filed rather than fixed here: #290 (notifier subscribers skip the dispatcher on an unenforced claim), #291 (the gate builds ffi but never lints or tests it), #292 (hiroz never raises MESSAGE_LOST).

#296 records the coverage gaps this change leaves — properties argued in doc comments that no test pins.

Copilot

Pull request overview

Fixes subscriber callback deadlocks by moving hazardous delivery off publishing threads and updating Python bindings accordingly.

Changes:

Adds QoS-aware subscriber selection and callback dispatch queues.
Releases Python’s GIL during publishing and removes a receive-side payload copy.
Adds Rust and Python regression and backpressure tests.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`crates/hiroz/src/pubsub.rs`	Implements dispatching and QoS-aware subscribers.
`crates/hiroz/src/node.rs`	Applies dispatching to raw subscribers.
`crates/hiroz/src/ffi/subscriber.rs`	Stores generalized subscriber handles.
`crates/hiroz/src/common.rs`	Identifies handlers that execute user code.
`crates/hiroz-tests/tests/reentrant_publish.rs`	Tests reentrant publishing and teardown.
`crates/hiroz-tests/tests/dispatch_backpressure.rs`	Tests dispatcher queue bounds.
`crates/hiroz-py/tests/test_reentrant_publish.py`	Tests Python reentrancy and thread cleanup.
`crates/hiroz-py/src/pubsub.rs`	Releases the GIL while publishing.
`crates/hiroz-py/src/node.rs`	Uses borrowed sample payloads in callbacks.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

crates/hiroz/src/pubsub.rs:1325

This branch runs before runs_user_code is checked, so every TransientLocal queue/notifier subscriber (including rmw's build_with_notifier path) gets an unbounded dispatcher even though its handler only enqueues. That delays the wait-set notification and permits an unbounded backlog ahead of the already bounded BoundedQueue, contradicting the queue-mode contract below. Split the advanced path on runs_user_code and wire queue handlers directly to the advanced subscriber callback.

        let inner = if qos_needs_advanced(&self.entity.qos) {

crates/hiroz/src/pubsub.rs:1526

build_internal already performs this encoding validation for every DataHandler at lines 1300–1320. Wrapping the new callback again parses each encoding twice and emits duplicate mismatch/unknown-format logs. Pass the callback directly to build_internal.

        let expected_encoding = self.expected_encoding.clone();
        let callback = Arc::new(move |sample: Sample| {

crates/hiroz-py/tests/test_reentrant_publish.py:132

This makes the Python suite fail unconditionally on macOS, although macOS Python wheels are supported (docs/bindings/python.md:13-14). /proc/self/task is Linux-only and is needed solely by the drain-thread leak detector; skip that one test on unsupported platforms (a skip is not a false pass) while still running the portable re-entrancy tests.

    # 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"
    )

crates/hiroz-py/src/pubsub.rs:38

The watchdog test does not independently exercise this GIL release: with dispatcher delivery enabled, the seed publish() returns quickly, and the test then sleeps for 300 ms before measuring progress, so reverting only allow_threads still passes. Add a detector that keeps the zenoh publish blocked while another Python thread must make progress; otherwise this regression can return unnoticed.

        py.allow_threads(|| self.inner.publish(zbuf.into()))
            .map_err(|e| e.into_pyerr())

The doc block moved here when this was split out of #250 referenced `CallbackDispatcher`, which #250 introduces and main does not have, so rustdoc could not resolve it and check-rustdoc-links failed. The sentence was also meaningless here for the same reason.

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.

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<State>`, 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.

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 <distro> 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.

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.

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.

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.

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<dyn Fn(Sample) + Send + Sync>.

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.

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.

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.

… depth" This reverts aa7b66d. 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.

…y depth" This reverts commit 4cb3424.

…ded 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.

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.

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.

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.

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 #249 at each site that describes the deadlock, and to #290 at the 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.

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.

…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).

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).

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.

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 <name> ... 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.

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.

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.

YuanYuYuan · 2026-08-18T11:07:46Z

The re-entrancy defect is upstream, and a fix is open there

Measured after this PR was opened. It does not change what this PR does, but it does change how permanent part of it is, so it belongs on the record.

The deadlock this PR works around is in zenoh-ext, not in hiroz. AdvancedSubscriber ran the user callback while holding its state mutex, so a callback that published re-entered a non-reentrant std::sync::Mutex on the thread that already held it. Upstream fix: eclipse-zenoh/zenoh#2744.

What the upstream fix alone does for hiroz

This PR's own re-entrancy suite, run against hiroz main — which carries none of this PR's implementation — toggling only the zenoh revision:

	zenoh `main`	zenoh `main` + the fix
passed	2 / 10	9 / 10
failed	5	1
timed out	3	0

Ten of this PR's eleven re-entrancy tests port unchanged. The eleventh needs close(), which this PR introduces, so it cannot run on main.

The single remaining failure fails in both columns: async_publish_delivers_off_the_publishing_thread. It asserts the dispatcher's threading model rather than deadlock freedom, so it is not a gap in the upstream fix — it is a feature only this PR provides.

One consequence worth noting for change 1

Four plain-path scenarios deadlock in the unfixed column — callback_republishing_on_same_topic, callback_cycle_across_two_topics, self_feeding_callback_loop_iterates_without_a_depth_cap and intra_closed_loop_runs_iteratively.

That is expected, because before this PR every hiroz subscriber was an AdvancedSubscriber, Volatile included. It means the upstream fix repairs re-entrancy on both QoS paths. So change 1 — declaring a plain subscriber for Volatile — no longer needs a deadlock justification once that fix ships. Its remaining justification is the overhead of the advanced entity, which stands on its own.

What this PR still uniquely delivers

	covered by the upstream fix
re-entrant publish, both QoS paths	yes
bounded dispatch queue, `KeepLast` / `KeepAll` depth mapping, loss reporting	no
`close(deadline)` / `CloseOutcome` teardown semantics	no
panic guard on the plain path's inline branch	no
delivery off the publishing thread	no
Python binding changes	no

Important

The upstream fix is not released. The latest zenoh tag is 1.9.0; the fix targets main, which is 1.10.0. hiroz depends on zenoh = "1.9.0" from crates.io. The measurement above required patching hiroz to a git revision, which is not something to ship.

What this means for this PR

Nothing to change now. This PR remains the only way hiroz gets re-entrancy safety on a released zenoh.

What it changes is the expected lifetime of one part of it. The TransientLocal dispatcher exists partly to work around an upstream defect that now has a fix in flight. Once that lands and hiroz moves to the release carrying it, the dispatcher's remaining justification is the queue bound, the teardown semantics and the panic guard — not re-entrancy. That is a smaller thing than it is today, and it is worth writing down now so the machinery is not later mistaken for permanently necessary.

YuanYuYuan force-pushed the pr/2-pubsub-reentrancy branch from 1f04eb3 to 1006b7d Compare July 28, 2026 08:06

YuanYuYuan requested a review from Copilot July 28, 2026 08:07

Copilot started reviewing on behalf of YuanYuYuan July 28, 2026 08:07 View session

Copilot AI reviewed Jul 28, 2026

View reviewed changes

Comment thread crates/hiroz/src/node.rs

Comment thread crates/hiroz/src/pubsub.rs Outdated

Comment thread crates/hiroz/src/pubsub.rs Outdated

YuanYuYuan requested a review from Copilot July 28, 2026 09:27

Copilot started reviewing on behalf of YuanYuYuan July 28, 2026 09:28 View session

Copilot AI reviewed Jul 28, 2026

View reviewed changes

YuanYuYuan force-pushed the pr/2-pubsub-reentrancy branch 4 times, most recently from 90092b2 to db3c6cc Compare July 29, 2026 09:13

This was referenced Jul 29, 2026

FFI module is never compiled, linted or tested; the "Go FFI tests" CI step silently skips #270

Open

test(ros): fail the interop job when it runs no tests #271

Merged

perf(py): decode subscriber samples without copying the payload #272

Closed

YuanYuYuan force-pushed the pr/2-pubsub-reentrancy branch 2 times, most recently from 85fb828 to 60a8dcd Compare August 5, 2026 16:54

YuanYuYuan force-pushed the pr/2-pubsub-reentrancy branch from 12a968a to d11d710 Compare August 6, 2026 10:35

This was referenced Aug 11, 2026

Re-entrancy: test-coverage gaps left open by #250 #296

Open

Re-entrancy: the fix shape costs one drain thread per callback subscriber #302

Open

YuanYuYuan added 17 commits August 14, 2026 21:06

style: wrap two long assert! calls in the queue tests

193076c

fix(pubsub): box both dispatcher-callback arms to a common type

e278884

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<dyn Fn(Sample) + Send + Sync>.

Reapply "fix(pubsub): bound the advanced dispatch queue at the histor…

c6ef483

…y depth" This reverts commit 4cb3424.

YuanYuYuan force-pushed the pr/2-pubsub-reentrancy branch from 8724f1a to d6d0f2d Compare August 14, 2026 13:09

YuanYuYuan added 11 commits August 14, 2026 21:25

docs(pubsub): state the drop-order obligation on Plain too

5aa7e38

style(node): rustfmt the ported import block

05e5f6c

style(tests): rustfmt the panic guard test

211a6e5

Navigation Menu

Uh oh!

fix(pubsub): stop running subscriber callbacks on the publishing thread - #250

fix(pubsub): stop running subscriber callbacks on the publishing thread#250
YuanYuYuan wants to merge 30 commits into
mainfrom
pr/2-pubsub-reentrancy

YuanYuYuan commented Jul 27, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

YuanYuYuan commented Aug 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

YuanYuYuan commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

The problem

The fix

Teardown, and why drop is not a barrier

Before / after

Breaking changes

Comparison with rmw_zenoh_cpp

Testing

Known limitations

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

YuanYuYuan commented Aug 18, 2026

The re-entrancy defect is upstream, and a fix is open there

What the upstream fix alone does for hiroz

One consequence worth noting for change 1

What this PR still uniquely delivers

What this means for this PR

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

YuanYuYuan commented Jul 27, 2026 •

edited

Loading

Teardown, and why `drop` is not a barrier

Comparison with `rmw_zenoh_cpp`