Skip to content

Re-entrancy: the guard stops at hiroz's own locks — widen the callout rule to the library boundary #284

Description

@YuanYuYuan

Part of #282, and the largest residual once #283 lands.

The bound on what #283 achieves

#283 makes this claim true: no new callout under a hiroz-owned lock can enter the codebase without CI failing. The qualifier is load-bearing. The guard counts hiroz's own lock guards, so the hazard is only visible while it stays inside hiroz.

It does not stay inside hiroz. rmw-zenoh-rs exists to call into zenoh, and control crosses that boundary in both directions. Two shapes escape the current mechanism.

Shape A — a library call that re-enters us

We hold a hiroz lock and call into the library; the library invokes one of our callbacks; that callback re-enters hiroz and blocks on the lock we are holding.

This is the same defect as #282's six instances, and the existing mechanism already handles it — but only if the call into the library is recognised as a callout. Today invoke_user_callback! is applied where we invoke a dyn Fn we were handed. It is not applied where we call a library function that may call us.

The fix is definitional, not mechanical: widen "callout" from "control escapes to unknown code we were handed" to "control escapes anywhere it may re-enter us", and route those call sites through the same assertion. Same macro, same counter, wider event set.

Shape B — a blocking teardown that waits on the callback running it

A thread inside a callback calls an entity teardown that blocks until in-flight callbacks of that entity finish — including the one currently executing. It waits for itself. No lock of ours is involved, so the counter is silent and always will be.

The C++ implementation has this today and it is open upstream (ros2/rmw_zenoh#993): ~ClientData() runs a blocking undeclare, "and the undeclare waits for in-flight callbacks of that querier to finish, i.e. for the very callback the thread is currently executing. The thread deadlocks permanently." The reported field incident took out the receive task, killing all inbound traffic for the session including link-lease handling, so it never reconnected. Only a process restart recovered it.

hiroz reaches the same library through different bindings, so this needs an audit, not an assumption. I am not claiming hiroz has this bug — I am claiming nothing in hiroz would detect it if it did.

The fix pattern already exists in this repo: #262 solves exactly this shape for the rmw executor callbacks, by having each dispatch register the thread performing it and having set wait for registrations on other threads only. A callback re-entering on its own thread never waits on itself. Generalising that registry to entity teardown is the answer to shape B — and it is caller-independent, unlike the upstream proposal (ros2/rmw_zenoh#994), which relocates the call and is correct only if the caller is not the callback thread.

Why the other planned work does not reach this

  • The runtime monitor counts hiroz guards. Shape B involves none.
  • A compile-time capability encoding governs locks taken through it; the library's internal locks are not, and no capability can be threaded through an extern "C" boundary anyway.
  • Lock-order tracking addresses ABBA, a different class.

This residual is closed by widening the definition of the event being guarded, not by strengthening the mechanism that guards it.

Scope

  • Enumerate the boundary. Which calls from hiroz / rmw-zenoh-rs into the library (a) may invoke a hiroz callback, or (b) may block on in-flight callbacks. Entity teardown — undeclare, close, and the Drop paths that reach them — is the place to start.
  • Shape A: route the (a) set through invoke_user_callback!, and extend Re-entrancy: close the coverage gap so an unconverted lock cannot bypass the guard #283's CI gate so a new call into that set from a callback-reachable file is caught the same way an unrouted dyn Fn call is.
  • Shape B: audit the (b) set for the self-wait. Where it exists, apply fix(rmw): stop invoking executor callbacks under a lock #262's in-flight-registry pattern — wait for other threads' dispatches, never your own.
  • Document the result honestly. If a call cannot be made safe, say so where a reader will find it, rather than leaving the boundary looking covered.

Acceptance criteria

  • The boundary set is enumerated in the repository, not in a review comment, so it can be re-derived
  • A callout that re-enters hiroz via a library call fails the same way a direct one does — demonstrated by writing the violation and watching it fire, not by inspection
  • Any self-wait found in the (b) set has a regression test that deadlocks without the fix
  • The coverage boundary written for Re-entrancy: close the coverage gap so an unconverted lock cannot bypass the guard #283 is updated to say which library calls are now covered and which are not

Ordering

After #283 — it extends #283's gate and reuses its instrumentation. The audit half can start earlier; it is reading, not converting.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions