Skip to content

Notifier subscribers are exempt from the dispatcher on an unenforced claim #290

Description

@YuanYuYuan

Part of #282 — the defect class and its fix shape are stated there.

Residual, pre-existing. Found by an adversarial review of #250, which converts this from an accident into a documented exemption.

The exemption

DataHandler::runs_user_code() decides whether a subscriber gets a CallbackDispatcher — i.e. whether its handler runs on its own thread or inline on the zenoh delivery thread.

// crates/hiroz/src/common.rs
pub(crate) fn runs_user_code(&self) -> bool {
    matches!(self, DataHandler::Callback(_))
}

QueueWithNotifier is deliberately excluded, on this stated grounds:

QueueWithNotifier's notifier is deliberately not counted: it is the rmw layer's wait-set wake, which must run promptly on the delivery thread and does not call back into hiroz.

Both halves of that justification are unenforced

claim reality
"it is the rmw layer's wait-set wake" ZSub::build_with_notifier is public and accepts any F: Fn() + Send + Sync + 'static. Nothing restricts it to the rmw layer.
"does not call back into hiroz" The actual rmw notifier ends in callback_fn(user_data_ptr, 1) — an rmw_subscription_new_message_callback_t, which is literally one of the three escape shapes property P enumerates (extern "C" fn).

Reproduction

A TRANSIENT_LOCAL rmw subscription — /tf_static, /robot_description, the latched topics — whose on-new-message callback publishes on the same session:

  1. runs_user_code() is false → dispatcher: None
  2. the handler runs inline inside zenoh-ext's sub_callback, under zlock!(statesref)
  3. the publish re-enters that mutex on the same thread → self-deadlock

That is #249's mechanism verbatim, on a path #250 does not cover.

On the Volatile queue path the same callback instead recurses inline on the publishing thread until stack exhaustion — mechanism 3 of #249.

Why this is filed rather than fixed in #250

Relative to main it is pre-existing: main also ran the notifier under the advanced lock, because main ran everything inline. #250 does not make it worse. But it does make it a deliberate, documented exemption from the class the PR claims to remove, which is a different thing from an oversight and deserves to be tracked as such.

No test exercises a notifier at all, in either direction.

Acceptance

  • Either include QueueWithNotifier in runs_user_code(), or state the real precondition at the site — that the exemption is sound only because the rmw notifier does not publish, and that build_with_notifier does not enforce it
  • If the exemption stays, build_with_notifier's doc says so, since it is public API
  • A test that drives a notifier which re-enters, demonstrated to fail without whichever fix is chosen

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