Skip to content

Commit 1f04eb3

Browse files
committed
fix(pubsub): stop running subscriber callbacks on the publishing thread
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.
1 parent 8c7920e commit 1f04eb3

9 files changed

Lines changed: 2190 additions & 40 deletions

File tree

crates/hiroz-py/src/node.rs

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ use hiroz::node::ZNode;
1616
use pyo3::prelude::*;
1717
use std::any::Any;
1818
use std::sync::Arc;
19+
use zenoh_buffers::buffer::SplitBuffer;
1920

2021
/// Try to extract type info from a message class.
2122
///
@@ -233,9 +234,25 @@ impl PyZNode {
233234
// matching rmw_zenoh_cpp's NodeData::subs_ pattern. The caller does not
234235
// need to assign the returned PyZSubscriber to keep the subscription active.
235236
let type_name = msg_type_str.clone();
237+
// Sample-level callback, not `build_with_callback`. The typed form
238+
// would route through `RawBytesCdrSerdes::deserialize`, whose
239+
// `Output` is an owned `RawBytesMessage` and so must `to_vec()` the
240+
// whole payload before this closure runs — a full copy per message,
241+
// scaling with payload size, immediately discarded once msgspec has
242+
// decoded it. Taking the `Sample` lets the decode read straight out
243+
// of the network buffer, and matches what the polling `recv()` path
244+
// in `pubsub.rs` already does.
236245
let zsub = sub_builder
237-
.build_with_callback(move |raw_msg: RawBytesMessage| {
238-
let payload = raw_msg.0;
246+
.build_with_sample_callback(move |sample| {
247+
// Same zero-copy setup as `PyZSubscriber::recv`: the ZBuf is
248+
// cheap Arc clones, and publishing it as the deserializer's
249+
// source lets `bytes`-typed fields become sub-ZSlices of the
250+
// received buffer instead of copies.
251+
let payload_zbuf: zenoh_buffers::ZBuf = sample.payload().clone().into();
252+
hiroz_cdr::ZBUF_DESER_SOURCE.with(|cell| {
253+
*cell.borrow_mut() = Some(payload_zbuf.clone());
254+
});
255+
let payload = payload_zbuf.contiguous();
239256
Python::with_gil(|py| {
240257
match hiroz_msgs::deserialize_from_cdr(&type_name, py, &payload) {
241258
Ok(obj) => {
@@ -248,6 +265,9 @@ impl PyZNode {
248265
}
249266
}
250267
});
268+
hiroz_cdr::ZBUF_DESER_SOURCE.with(|cell| {
269+
*cell.borrow_mut() = None;
270+
});
251271
})
252272
.map_err(|e| e.into_pyerr())?;
253273

crates/hiroz-py/src/pubsub.rs

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,20 +24,30 @@ impl PyZPublisher {
2424
/// Publish a message
2525
///
2626
/// Serializes the Python message (msgspec.Struct) to ZBuf and publishes (zero-copy path)
27-
unsafe fn publish(&self, _py: Python, data: &Bound<'_, PyAny>) -> PyResult<()> {
28-
// Serialize Python message directly to ZBuf (zero-copy)
27+
unsafe fn publish(&self, py: Python, data: &Bound<'_, PyAny>) -> PyResult<()> {
28+
// Serialize Python message directly to ZBuf (zero-copy). This touches
29+
// Python objects, so it must run with the GIL held.
2930
let zbuf = hiroz_msgs::serialize_to_zbuf(&self.type_name, data)?;
3031

31-
// Publish the ZBuf directly
32-
self.inner.publish(zbuf.into()).map_err(|e| e.into_pyerr())
32+
// Release the GIL for the publish itself. Zenoh delivers samples to
33+
// local subscribers synchronously on the publishing thread, so a publish
34+
// issued from inside a subscriber callback can block here; holding the
35+
// GIL across it would freeze the whole interpreter — no exception, no
36+
// traceback — instead of blocking just this thread.
37+
py.allow_threads(|| self.inner.publish(zbuf.into()))
38+
.map_err(|e| e.into_pyerr())
3339
}
3440

3541
/// Publish pre-serialized CDR bytes directly
3642
///
3743
/// Use this for zero-copy forwarding of received messages (e.g., in a pong responder).
3844
/// The bytes should be in CDR format (as returned by recv_serialized/try_recv_serialized).
39-
fn publish_raw(&self, data: &[u8]) -> PyResult<()> {
40-
self.inner.publish(data.into()).map_err(|e| e.into_pyerr())
45+
fn publish_raw(&self, py: Python, data: &[u8]) -> PyResult<()> {
46+
// Copy out of the Python buffer before dropping the GIL, then publish
47+
// without it — see `publish` for why.
48+
let payload: zenoh::bytes::ZBytes = data.into();
49+
py.allow_threads(|| self.inner.publish(payload))
50+
.map_err(|e| e.into_pyerr())
4151
}
4252

4353
/// Get the topic name (for debugging)

0 commit comments

Comments
 (0)