fix: stop calling user code under runtime/session locks - #2781
Draft
YuanYuYuan wants to merge 15 commits into
Draft
YuanYuYuan wants to merge 15 commits into
YuanYuYuan wants to merge 15 commits into
Conversation
Zenoh-core / macro plumbing only -- deliberately not touching zenoh-ext/src/advanced_subscriber.rs, so this branch has no dependency on the AdvancedSubscriber fix landing (eclipse-zenoh#2744).
A count of "1 guard held" is not actionable: it cannot distinguish a lock held deliberately from a defect, nor say which of several nested guards is the problem. Each guard now records where it was acquired, so a failure reads acquired at ["zenoh/src/api/session.rs:3100"]. The bigger change is LockKind. Two different jobs get done with a mutex here - guarding state, where calling user code under it is the hazard, and serialising delivery, where holding it across the call is the point. zlock_delivery! records the second, and assert_no_locks_held ignores it. That distinction is what makes the check usable rather than merely correct. Surveyed over the zenoh and zenoh-ext suites, 30 of 43 reported call-out sites were one delivery-ordering lock - the transport RX channel mutex, which keeps reliable delivery ordered across a transport's links. Undifferentiated, the report is 70% noise about a lock nobody should touch. That mutex is now taken with zlock_delivery! at its four sites. Tests cover both directions of the exemption: a delivery guard does not trip the check, and a state guard taken alongside one still does.
30 of 43 reports contained the transport lock; 26 contained nothing else. Only the 26 are suppressed by the exemption, so "30 were one lock" was the wrong number for the claim it was supporting.
43 reports without the distinction, 17 with it, measured over both suites. The 26 suppressed all held the transport RX mutex and nothing else; the 17 remaining involve six state locks and every one maps to a filed defect.
hiroz independently built the identical mechanism (thread-local guard count, per-thread stack, LockKind exemption). Rather than maintain two copies, both now depend on lock-tripwire, extracted from this module's own design (a strict superset already existed there: site attribution, LockKind, Condvar support, into_inner). tracking.rs deleted; `pub use lock_tripwire as tracking;` keeps zlock!/zread!/zwrite!/zlock_delivery! in macros.rs compiling unchanged, since they reference $crate::tracking::TrackedGuard and $crate::tracking::LockKind directly, both of which lock-tripwire exposes under the same names. zenoh-ext/tests/reentrancy.rs (already on this branch, unrelated to this commit) still passes: 3/3, 0.5s. zenoh-core/zenoh/zenoh-ext all build clean; fmt and clippy clean on the files this touches. Not done in this commit: tracking.rs's module docs (the 43->17 call-out-site measurement, the field-order rationale) are not yet carried into lock-tripwire's own crate docs, and the 43->17 figure itself was not re-measured -- the original survey methodology isn't preserved as a runnable script anywhere in this tree, only its result. Follow-up, not a regression: the property it measured (delivery-ordering exemption suppressing transport-lock noise) is unchanged code, just relocated.
Pre-existing gap, not introduced by the URL swap: the entry had name and version but no source, so cargo couldn't actually resolve it from a clean checkout.
Found by independent review: the nested4 benchmark called read() three times on the SAME RwLock while all three guards were live. std::sync:: RwLock documents recursive read() on one instance as unspecified -- "might panic or deadlock" if a writer queues in between -- and a realistic nesting scenario in this codebase is several distinct locks held together, not the same one re-entered.
Found by independent review, and confirmed with a real full-workspace build failure: TxHandoff::lock() transmutes a std::sync::MutexGuard into a 'static one to build a self-referential struct (LockedTxHandoff), and depends on the guard's exact layout. Wrapping it in TrackedGuard changed its size and broke the transmute outright. The guard here never survives across a user call-out (LockedTxHandoff's own field-order safety comment already establishes that), so there's no self-wait hazard at this site to catch -- bypass the macro and call the underlying Mutex directly.
cargo fmt --check flagged this after the rebase; unrelated to the tracking migration itself.
cancel_query, cancel_liveliness_query, and the liveliness-query reply path all held a write guard across a callback call-out or a Callback's Drop (which can run a user-supplied hook) -- a self-deadlock if that code re-enters and tries to take the same lock. Scope the guard to the remove() alone, or clone-and-drop-before-calling, matching the pattern this file's own undeclare_querier_inner already uses. new_unicast/new_multicast held transport_handlers across calling each registered handler -- same shape, same fix: collect the handlers first, release the lock, then call out.
CI runs rustfmt with imports_granularity=Crate, group_imports=StdExternalCrate on a pinned nightly toolchain -- reorders these two import blocks differently than a default cargo fmt does.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2781 +/- ##
==========================================
+ Coverage 74.72% 74.74% +0.02%
==========================================
Files 419 419
Lines 63955 63951 -4
==========================================
+ Hits 47790 47800 +10
+ Misses 16165 16151 -14 ☔ View full report in Codecov by Harness. |
YuanYuYuan
marked this pull request as draft
September 11, 2026 16:14
This was referenced Sep 11, 2026
match zread!(self.runtime).upgrade() { ... } does not release the
guard before the matched arm runs -- a match scrutinee's temporaries
live for the whole arm, regardless of whether the pattern borrows or
moves. Removing .as_ref() alone (the prior revision of this fix)
changed nothing: new_unicast/new_multicast still held self.runtime's
RwLock read guard across handler.new_unicast()/new_multicast(), the
exact user-code call-out this fix exists to protect.
Binding to a let statement first, then matching the local, actually
drops the guard at the let statement's semicolon -- the same shape
already fixed once in this project's history for an identical
Weak::upgrade() match-scrutinee case.
Verified: cargo check/clippy -p zenoh --features plugins,unstable,internal
clean, cargo fmt --check clean, the 3 existing net::runtime tests still
pass. No existing test proves the guard is released before the
callout specifically -- same gap as before this fix, not closed here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cancel_query,cancel_liveliness_query, the liveliness-query reply path, andRuntimeTransportEventHandler::new_unicast/new_multicastall held a write guard (self.0.stateortransport_handlers) across a call into user code — a callback invocation, or aCallback'sDrop(which can run a user-supplied hook). A callback that re-enters and takes the same lock self-deadlocks: one thread, no race, every time.Also adds
lock-tripwire, an opt-in, debug-time tripwire that asserts no tracked lock is held at the one call-out site this fix couldn't remove structurally (the liveliness-query reply), so a regression of this shape fails fast and by name instead of hanging.The fix
Each site now either scopes the guard to the
remove()/clone()call alone, or clones the callback and drops the guard before calling it — matching the pattern this file's ownundeclare_querier_inneralready uses:new_unicast/new_multicastcollect the registered handlers (cheapArcclones) before releasingtransport_handlers, then call each outside the lock.Test coverage: disclosed, not fabricated
No dedicated failing-baseline test accompanies this fix, for the same reason #2744 in this repo states for its own commits 3–5: each of these closes a race that needs a callback to land inside a window bounded by one guard release and the next acquisition. A test that hit the window would be timing-dependent; one that missed it would read as proof of absence. The argument for each site is the control flow, stated at the code — matching this repository's own precedent for exactly this class of fix.
The one site with a genuinely stable, always-reachable assertion (not a timing-dependent hang) is the liveliness-query reply path, which now asserts under
--features reentrancy-tripwire. The other three restructurings remove a lock-held-across-call-out shape that reappears identically at four sites in this same file'sundeclare_querier_inner— the same defect family, already fixed there.Overlap with YuanYuYuan#1 (lock-tripwire on top of #2744)
This PR introduces the same
commons/zenoh-coretracking plumbing as YuanYuYuan/zenoh#1, independently, because this fix has nothing to do withAdvancedSubscriberand shouldn't depend on #2744 merging first. Whichever of the two lands first should absorb the other's infra commits on rebase — flagging this now so a reviewer isn't surprised by the duplication.Before and after
cancel_query/cancel_liveliness_queryDropremove()statementcallback.call(reply)reentrancy-tripwirenew_unicast/new_multicasttransport_handlersheld across each handler callVerification
cargo check --workspace --locked --all-features: clean.cargo clippy --workspace --locked --all-features --all-targets -- -D warnings: clean.cargo fmt --check: clean.cargo test -p zenoh --lib --features unstable: 40 tests pass, including thelink_weights/interceptor_cachesuites (the ones a prior wiring attempt at one of these sites broke, so specifically checked).Breaking changes
None.
reentrancy-tripwireis opt-in and off by default.🏷️ Label-Based Checklist
Based on the labels applied to this PR, please complete these additional requirements:
Labels:
bug🐛 Bug Fix Requirements
Since this PR is labeled as a bug fix, please ensure:
Why this matters: Bugs without tests often reoccur.
Instructions:
- [ ]to- [x])This checklist updates automatically when labels change, but preserves your checked boxes.
Related work
This fix is one of a small, independent set of public fixes for the same underlying defect shape (a lock held across a call into user/foreign code) across projects in the zenoh/ROS 2 ecosystem, using lock-tripwire as the common diagnostic tool: YuanYuYuan/zenoh#1 (a related callback path in this same crate, stacked on #2744), ZettaScaleLabs/hiroz#351/#352 (the Rust RMW implementation
rmw-zenoh-rs), and ros2/rmw_zenoh#1061 (the C++ RMW implementation, same shape, independently re-derived).Correction, found by independent review
An earlier revision of
new_unicast/new_multicastreleasedtransport_handlerscorrectly (as described above) but left a second, outer guard held across the same user-code call:self.runtime'sRwLockread guard, taken byzread!(self.runtime).upgrade(). Writingmatch zread!(self.runtime).upgrade() { Some(runtime) => { ... handler.new_unicast(...) ... } }does not release that guard before the arm runs — amatchscrutinee's temporaries live for the whole matched arm, independent of whether the pattern borrows or moves what it produces. The fix now binds the upgraded value to aletstatement first, then matches the local, so the guard is genuinely dropped at theletstatement's semicolon beforehandler.new_unicast/new_multicast(user code) is called. No existing test in this repository proves the guard is released at this specific call site; that gap is unchanged by this fix.