Shut down entities on the application thread when deleting them by BOURBONCASK · Pull Request #994 · ros2/rmw_zenoh

BOURBONCASK · 2026-06-12T04:14:00Z

Description

Fixes #993.

NodeData::delete_{pub,sub,service,client}_data only erased the owning shared_ptr from the map, leaving shutdown() to run from whichever thread drops the last reference. If an entity is destroyed while one of its zenoh callbacks is in flight, the callback's transient shared_ptr (locked from the closure's weak_ptr) is the last reference, so the destructor runs the blocking undeclare on the zenoh callback thread itself — which waits for that same callback to finish. The thread deadlocks permanently and takes the entity mutex (and, on a deployed system, the whole session inbound path) with it. Full stacks and a field incident are in #993.

This PR:

takes the entity out of the map under the lock, then shuts it down on the calling (application) thread outside the lock; the destructor's own shutdown() becomes a no-op via the existing is_shutdown_ CAS regardless of which thread runs it (rmw_node_data.cpp);
makes ClientData::shutdown() and ServiceData::shutdown() public, matching PublisherData / SubscriptionData;
sets initialized_ = true at the end of ServiceData::make(). It was only ever initialized to false, so both undeclares in ServiceData::shutdown() were unreachable and the queryable was still undeclared by the member destructor — i.e. on the callback thread — even after the change above (found by @otamachan, see below);
adds three deterministic regression tests in test_rmw_zenoh_cpp, one per entity kind that has a zenoh callback:
- test_client_destroy_during_reply: blocks inside the new-response callback (invoked synchronously from ClientData::add_new_reply while the reply closure still holds its strong reference), destroys the client from the main thread, releases the callback, and verifies the callback thread can still serve a probe request;
- test_service_destroy_during_request: same shape for a service, blocking inside the new-request callback invoked from ServiceData::add_new_query while the query closure holds its strong reference;
- test_subscription_destroy_during_sample: parks one publishing thread inside the new-message callback (invoked from add_new_message() with mutex_ held), starts the destroy so shutdown() queues on mutex_, then sends a second sample from another thread so a second callback is in flight when the undeclare runs. This guards the lock-release-before-undeclare ordering in SubscriptionData::shutdown() that Add support for rosidl::Buffer-aware per-endpoint pub/sub #930 introduced on rolling; it is the AB-BA @otamachan hit on jazzy, where that ordering does not exist yet.

Rebased onto current rolling.

Verification (Docker ros:rolling-ros-base, Ubuntu 26.04 / x86_64, rolling binaries from ros2-testing, rebased onto rolling at 7fe3e65):

colcon test for rmw_zenoh_cpp + test_rmw_zenoh_cpp: 236 tests, 0 failures, linters included. The three regression tests take ~1.1 s each and passed 3/3 reruns.
Each test catches its bug when the corresponding fix is reverted:
- test_service_destroy_during_request fails after its 10 s watchdog ("sender thread never returned … deadlocked") without the initialized_ = true line;
- test_subscription_destroy_during_sample fails with "destroy returned: false, publisher B returned: false" when SubscriptionData::shutdown() is changed back to hold mutex_ across the undeclare (the pre-Add support for rosidl::Buffer-aware per-endpoint pub/sub #930 shape @otamachan hit on jazzy);
- test_client_destroy_during_reply hangs with the original delete_client_data (caught by the test timeout), as in the previous revision of this PR.

Is this user-facing behavior change?

No public API changes. It fixes a permanent deadlock/hang when a client, service or subscription is destroyed concurrently with an in-flight callback.

Did you use Generative AI?

Yes. Claude (claude-fable-5 / claude-fable-5-1 and claude-opus-4-8) via Claude Code was used to assist with root cause analysis, reproducing the deadlock, and creating an initial prototype of the changes in this PR.

https://claude.ai/code/session_01317fxjjrM6WrQipxhN8qtm

Yadunund · 2026-06-12T18:00:48Z

@BOURBONCASK thanks for reporting the issue and opening a PR! Do you mind updating your PR description to follow the default template?

BOURBONCASK · 2026-06-13T04:18:31Z

@BOURBONCASK thanks for reporting the issue and opening a PR! Do you mind updating your PR description to follow the default template?

done — updated the description to follow the template. Thanks for taking a look!

mergify · 2026-07-08T17:56:45Z

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

Queue this pull request

otamachan · 2026-09-06T07:30:02Z

We hit the same deadlock and tested this PR against a reproducer that destroys subscriptions and
services, not only clients. The client path is fixed; the subscription and service paths are not.
Applied the rmw_zenoh_cpp changes of this PR to the jazzy branch at 0.2.10.

Runs that deadlocked / runs:

	unpatched	this PR	this PR + the two changes below
destroy subscriptions	5/5	3/3	0/5
destroy clients	2/3	0/3	0/3
destroy services	5/5	3/3	0/5

Reproducer: https://gist.github.com/otamachan/515e3e699dbfe592207894789782eeb1

It runs in one process with mode="peer" and no router. A same-session publication is delivered
synchronously on the publishing thread (Session::resolve_put::call_local), so that thread takes
the role of the callback thread. Exit code 3 means the deadlock was hit.

1. Subscriptions: the self-deadlock becomes an AB-BA

SubscriptionData::shutdown() holds mutex_ across the undeclare, and the sample callback takes
the same mutex_ in add_new_message(). With this PR the application thread blocks in
SyncGroup::wait while holding mutex_, and the callback it waits for blocks on that mutex_.

Thread 13 (application thread):
#3  zenoh::api::cancellation::SyncGroup::wait
#4  ze_undeclare_advanced_subscriber
#6  rmw_zenoh_cpp::SubscriptionData::shutdown (this=0x5555556a73b0)   <- holds mutex_
#7  rmw_zenoh_cpp::NodeData::delete_sub_data                          rmw_node_data.cpp:284
#8  rmw_destroy_subscription

Thread 12 (in-flight sample callback):
#4  std::lock_guard<std::mutex>::lock_guard (__mutex=0x5555556a73c0)  <- SubscriptionData::mutex_
#5  rmw_zenoh_cpp::SubscriptionData::add_new_message (this=0x5555556a73b0)
#7  Closure<SubscriptionData::init()::<lambda(const zenoh::Sample&)>>::call
#9  zenoh_ext::advanced_subscriber::handle_sample

shutdown() needs to take mutex_, mark the shutdown, move token_ / sub_ / sess_ into
locals, release the lock, and undeclare outside it.

2. Services: `ServiceData::shutdown()` never undeclares anything

initialized_ is assigned only in the constructor's init list (false). ServiceData::make() does
not set it to true, and both undeclares in ServiceData::shutdown() are behind
if (initialized_). The queryable and the liveliness token are released by the member destructors
instead, so calling shutdown() from delete_service_data() does not move the undeclare off the
callback thread.

#3  zenoh::api::cancellation::SyncGroup::wait
#4  z_undeclare_queryable
#6  zenoh::Owned<z_owned_queryable_t>::~Owned
#13 std::optional<zenoh::Queryable<void> >::~optional
#14 rmw_zenoh_cpp::ServiceData::~ServiceData                 rmw_service_data.cpp:474
#17 std::_Sp_counted_base<...>::_M_release_last_use_cold
#18 <F as zenoh::api::handlers::callback::CallbackImpl<T>>::call
#19 zenoh::api::session::Session::handle_query
#23 rmw_zenoh_cpp::ClientData::send_request                  rmw_client_data.cpp:400

ServiceData::make() should set initialized_ = true before returning, as
SubscriptionData::init() and ClientData::make() do.

Both changes are small:
otamachan@733f0e0

That branch is jazzy with this PR cherry-picked on top plus that commit, which is what the numbers
above were measured on. We can send it as a follow-up PR against rolling, or you can fold the
changes in here.

Deterministic regression test for the self-deadlock where the last shared_ptr<ClientData> reference is dropped inside the client reply closure: ~ClientData() then runs the blocking querier undeclare (which drains in-flight callbacks) on the very thread executing that callback. The test blocks inside the new-response callback (invoked synchronously from ClientData::add_new_reply while the reply closure holds its strong reference), destroys the client from the main thread, releases the callback, and then verifies the callback thread can still serve a probe request. Signed-off-by: Yifei Ma <yifeima98@gmail.com>

NodeData::delete_{pub,sub,service,client}_data only erased the owning shared_ptr from the map, leaving shutdown() to run from whichever thread drops the last reference. When an entity is destroyed while one of its zenoh callbacks is in flight, that callback's transient shared_ptr (the locked weak_ptr) is the last reference, so the destructor runs the blocking undeclare on the zenoh callback thread itself. The undeclare waits for in-flight callbacks to finish, i.e. for itself: the thread deadlocks permanently, taking the entity mutex (and on a real system the whole session inbound path) with it. Take the entity out of the map under the lock, then shut it down on the calling (application) thread outside the lock. The destructor's own shutdown() then no-ops via the is_shutdown_ CAS regardless of which thread runs it. ClientData::shutdown() and ServiceData::shutdown() become public to match PublisherData and SubscriptionData. Observed in production as a zenoh client whose transport rx thread deadlocked in z_undeclare_querier inside its own reply callback after rmw_destroy_client raced an in-flight service reply; reproduced deterministically by test_client_destroy_during_reply. Signed-off-by: Yifei Ma <yifeima98@gmail.com>

ServiceData::initialized_ was only ever set to false in the constructor; ServiceData::make() never set it to true, so both undeclares in ServiceData::shutdown() were unreachable. The queryable and the liveliness token were still released by the member destructors, i.e. on whichever thread dropped the last reference. That defeats shutting the entity down on the application thread in NodeData::delete_service_data(): with an in-flight query, the callback thread still ran the blocking queryable undeclare on itself and deadlocked. Co-authored-by: otamachan <536660+otamachan@users.noreply.github.com> Claude-Session: https://claude.ai/code/session_01317fxjjrM6WrQipxhN8qtm

…ust not deadlock test_service_destroy_during_request mirrors the client test: it blocks inside the new-request callback, which ServiceData::add_new_query() invokes while the query closure still holds its strong reference, destroys the service from the main thread, releases the callback and checks that the callback thread returns. test_subscription_destroy_during_sample pins the lock ordering of SubscriptionData::shutdown(): one publisher thread is parked inside the new-message callback with mutex_ held, the destroyer queues on mutex_, and a second publisher thread queues a second in-flight sample callback. shutdown() must release mutex_ before it undeclares the subscriber, or the undeclare waits for a callback that is waiting for mutex_. Both tests use the same-process delivery path (the sending thread is the zenoh callback thread), so the races are deterministic. Claude-Session: https://claude.ai/code/session_01317fxjjrM6WrQipxhN8qtm

BOURBONCASK · 2026-09-07T06:46:15Z

@otamachan thanks for the reproducer and the measurements. I was able to confirm both on my side:

Service: folded into this PR. ServiceData::make() now sets initialized_ = true, so shutdown() really undeclares the queryable on the application thread.
Subscription: on rolling this appears to be covered already by Add support for rosidl::Buffer-aware per-endpoint pub/sub #930, which moved the undeclare in SubscriptionData::shutdown() outside mutex_. jazzy and kilted don't have that, and Add support for rosidl::Buffer-aware per-endpoint pub/sub #930 itself doesn't look backportable (it depends on rosidl_buffer_backend_registry, which only exists on rolling). So for jazzy / kilted the SubscriptionData::shutdown() part of your commit would be needed as a separate PR — happy to defer to the maintainers on how to sequence that with the backport of this one.

I also added regression tests for the service and subscription paths next to the client one. The subscription test deadlocks against the pre-#930 shutdown(), so a backport of this PR without that change would fail loudly rather than regress silently.

Rebased onto current rolling; all three tests pass here and each fails when its fix is reverted.

otamachan · 2026-09-07T10:51:27Z

Thanks for the regression tests and for tracking down #930.

I will open the SubscriptionData::shutdown() change as a separate PR against jazzy / kilted
after this one is backported, if it is still needed then.

We hit this deadlock in the field, so I hope this PR can be merged soon.

Yadunund self-requested a review June 12, 2026 18:00

mjcarroll assigned Yadunund Jun 18, 2026

YuanYuYuan mentioned this pull request Aug 4, 2026

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

Open

8 tasks

BOURBONCASK added 4 commits September 6, 2026 22:30

BOURBONCASK force-pushed the fix/rolling-entity-shutdown-on-destroy branch from 32cbb90 to 536c1dd Compare September 7, 2026 06:30

Navigation Menu

Shut down entities on the application thread when deleting them - #994

Shut down entities on the application thread when deleting them#994
BOURBONCASK wants to merge 4 commits into
ros2:rollingfrom
BOURBONCASK:fix/rolling-entity-shutdown-on-destroy

BOURBONCASK commented Jun 12, 2026 •

edited

Loading

Uh oh!

Yadunund commented Jun 12, 2026

Uh oh!

BOURBONCASK commented Jun 13, 2026

Uh oh!

mergify Bot commented Jul 8, 2026

Uh oh!

otamachan commented Sep 6, 2026 •

edited

Loading

Uh oh!

BOURBONCASK commented Sep 7, 2026

Uh oh!

otamachan commented Sep 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

BOURBONCASK commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Is this user-facing behavior change?

Did you use Generative AI?

Uh oh!

Yadunund commented Jun 12, 2026

Uh oh!

BOURBONCASK commented Jun 13, 2026

Uh oh!

mergify Bot commented Jul 8, 2026

Uh oh!

otamachan commented Sep 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. Subscriptions: the self-deadlock becomes an AB-BA

2. Services: ServiceData::shutdown() never undeclares anything

Uh oh!

BOURBONCASK commented Sep 7, 2026

Uh oh!

otamachan commented Sep 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

BOURBONCASK commented Jun 12, 2026 •

edited

Loading

otamachan commented Sep 6, 2026 •

edited

Loading

2. Services: `ServiceData::shutdown()` never undeclares anything