Repository navigation
Re-entrancy: test-coverage gaps left open by #250 #296
Description
Part of #282.
#250 fixes #249 and its CI is green. This issue records what that green does not prove.
Each item below is a revert that should fail the suite and does not. None of them is a defect in #250 — they mark the places where a future regression would pass unnoticed. Filed so the list survives the merge of #250, where it would otherwise stop being read.
This issue closes nothing. It is the honest remainder.
G1 — the gating decision is pinned; the wiring is not
Narrowed by #250 at 5aa7e383. Two unit tests now pin qos_needs_advanced in both directions: volatile_does_not_need_an_advanced_subscriber and transient_local_needs_an_advanced_subscriber. Forcing the function to return true unconditionally — the shape of #249 — fails the first and passes the second. Both directions were run, each asserting a non-zero test count.
What is still unpinned: no test asserts that the decision reaches the subscriber. ZSub holds its SubscriberHandle privately, so nothing can assert matches!(handle, SubscriberHandle::Plain { .. }) for a Volatile subscriber. A rewiring that ignored qos_needs_advanced and declared an AdvancedSubscriber anyway would still leave the suite green.
Fix: expose the handle, or add a test-only accessor, then assert the variant for both durabilities.
G1 was the highest-value item here while the decision itself was unpinned. It is not any longer. G2 now is: teardown discarding the backlog is unpinned, and it is the behaviour behind the one hazard #250 discloses on a public API.
Unpinned production changes
| tag | Production change | Revert that stays green | Why nothing catches it |
|---|---|---|---|
| G2 | Teardown discards the backlog | Pop pending before checking closed |
No test drops a dispatcher with a non-empty queue, and none asserts on teardown duration |
| G3 | destroy_subscriber releases the GIL before dropping |
Restore the bare swap_remove |
Its only test uses lambda _msg: None on topics no publisher writes to, so the drain thread never needs the GIL |
| G4 | impl Drop for PyZNode releases the GIL |
Delete the impl | No Python test drops a node or shuts the interpreter down with a callback in flight |
| G5 | publish_raw releases the GIL |
Delete py.allow_threads |
No test calls publish_raw. (publish is pinned, by test_interpreter_stays_alive_during_reentrant_publish) |
| G6 | LocalPublishGuard on publish_serialized and publish_sample |
Delete either guard | Neither method is called by any test or example, so #249 stays reachable through two of the five public publish paths |
| G7 | catch_unwind around the user callback |
Call (*handler)(sample) directly |
Nothing in the suite panics inside a callback. Without the guard the drain thread dies and the subscriber goes silent forever, with no log |
| G8 | Teardown and self-drop on the plain/Volatile path | — | Both teardown tests are transient_local_*. On the plain path a remote sample runs the callback on an RX worker, so the join actually runs. This was checked by hand and does not deadlock — zenoh holds no lock across the callout — but nothing pins it. Volatile is the ROS 2 default |
| G9 | Drop-order of SubscriberHandle's fields |
Swap the two fields | No failure could be constructed: enqueue returns early once closed is set. Untested, not defective |
| G10 | The escalating drop/backlog warn! |
Remove either warning | No test asserts a warning is emitted. This is the only user-visible signal for the silent same-session sample loss #250 introduces |
Test-quality gaps
| tag | Gap | Effect |
|---|---|---|
| G11 | transient_local_delivery_preserves_order is vacuous at one delivery — windows(2) yields nothing on a 1-element slice, and the only floor is !is_empty() |
A run delivering 1 of 500 samples passes and reports ordering preserved, having observed no pair. A len() >= 2 floor restores the claim |
| G12 | Both new harnesses treat recv_timeout(..).is_err() as "deadlocked" |
A failing assert! drops the sender, so Disconnected is reported as a deadlock. #260 fixed this shape on main; these files reintroduce it. Wrong message, not a wrong verdict |
| G13 | async_publish_delivers_off_the_publishing_thread asserts assert_ne! on thread ids where the name is available |
If inline delivery stopped and the sample returned on an RX worker, it still passes. Asserting Some("hiroz-sub-drain") asserts the value rather than a difference |
| G14 | The two new queue.rs tests pin BoundedQueue, not DispatchQueue |
dispatch_capacity's doc cites zero_capacity_retains_one_sample as evidence about its own zero-depth behaviour. The test constructs the other type. Correct tests, wrong type for the claim |
Detected, for contrast
Inverting either if runs_user_code branch is caught — one flip deadlocks transient_local_callback_republishing_on_same_topic_does_not_deadlock, the other kills self_feeding_callback_loop_iterates_without_a_depth_cap by stack overflow.
Genuinely undetected is only the narrower direction: making runs_user_code() return true for the queue variants too adds a needless thread and nothing fails.
Properties argued in comments, pinned by nothing
The items above are reverts that stay green. These are different in kind: each is a property #250 argues in a doc comment at its call site, and which its description once listed under a heading called "Evidence". Reading the code is not evidence.
| tag | Property | Holds because of a dependency? | Test sketch |
|---|---|---|---|
| G15 | No lost wakeup between enqueue and dequeue |
no | Enqueue from a second thread with the drain thread already parked in wait. Assert the callback runs within a deadline. Repeat the race |
| G16 | Drop does not hold the queue lock across join() |
no | Park a callback on a barrier the test controls. Drop the subscriber from another thread. Release the barrier. Assert the drop returns |
| G17 | No thread leaked when declare_subscriber fails |
no | Force the declare to fail. Assert the thread count returns to baseline |
| G18 | The plain path holds no zenoh lock when the callback runs inline | yes — zenoh | Publish from inside a callback fed by a remote sample. It must not deadlock |
| G19 | No lock-order cycle with zenoh-ext's statesref |
yes — zenoh-ext | Stress concurrent remote delivery and callback republication on a TransientLocal profile, under a deadline |
| G20 | KeepLast(usize::MAX) is safe end to end |
yes — zenoh-ext | Build such a subscriber. Assert it builds, delivers and drops |
| G21 | The escalating warn counters cannot wrap | no | Set the counters near u64::MAX through a test-only constructor. Assert the warning still fires. Extends G10, which covers emission rather than arithmetic |
| G22 | The action client's two dispatcher threads cannot hang teardown | no | Drop an action client with feedback in flight. Assert teardown returns within a deadline and the terminal status is observed |
Important
G18, G19 and G20 hold because of how a dependency behaves today. A zenoh or zenoh-ext upgrade can break any of them with no change to hiroz, and nothing in the suite would notice. The other five break only when someone edits hiroz. Order the work accordingly.
Two further properties of this kind were already recorded above rather than given new tags: the panic guard is G7, and SubscriberHandle's field order is G9.
Suggested order
- G18, G19, G20 — the three a dependency upgrade can break silently, with no hiroz change to review.
- G11, G12 — the two gaps that let a test report a property it did not observe.
- G2, G7, G10 — the silent-failure paths (teardown, panicking callback, lost warning).
- G1's remainder — asserting the gating decision reaches
SubscriberHandle::Plain. - The rest as convenient.
Related
- Notifier subscribers are exempt from the dispatcher on an unenforced claim #290 — notifier subscribers skip the dispatcher on an unenforced claim
- The ffi feature is built by no crate on the PR gate, and its re-entrancy detector was deleted #291 — the
ffifeature is built on the gate but never linted or tested - RMW_EVENT_MESSAGE_LOST is plumbed but never raised #292 —
MESSAGE_LOSTis never raised
Activity
fix(pubsub): disclose the teardown block and pin the gating decision
fix(pubsub): stop making drop(subscriber) a barrier on the in-flight …
fix(pubsub): guard the plain path's inline callback too
fix(pubsub): disclose the teardown block and pin the gating decision
fix(pubsub): stop making drop(subscriber) a barrier on the in-flight …
fix(pubsub): guard the plain path's inline callback too