[SIDDARTHAREDDY8]

Description

Fixes #2962

TimeSource::NodeState::destroy_clock_sub() held clock_sub_lock_ while calling clock_executor_thread_.join(). If the executor thread's shutdown path contends on the same lock, the join deadlocks: the main thread waits for the executor thread to finish while the executor thread waits for the main thread to release the lock (see the gdb trace in #2962, where the main thread is stuck in join() holding clock_sub_lock_).

The fix: move the thread, executor, callback group and subscription into locals under the lock, release the lock, then cancel and join outside the critical section.

Two details beyond the approach sketched in the earlier attempts (#3110, #3167):

1. The subscription is moved, not reset, under the lock. Fix potential deadlock in TimeSource::destroy_clock_sub (#2962) #3167 kept clock_subscription_ alive until after the join and reset it under a second lock acquisition. As @sylvesterkaczmarek noted there, a concurrent create_clock_sub() can then see the still-live subscription and return early, and this path resets it after the join, leaving sim time enabled with no subscription. Moving the subscription to a local under the first lock closes that window: a concurrent create sees no subscription and builds a fresh one. The old subscription is kept alive in a local until after the join, so the executor thread can finish any in-flight callback without touching a destroyed subscription.
2. The teardown repeats if a concurrent create_clock_sub() installed a new subscription/thread while the old thread was being joined. This is only possible when a parameter callback races detachNode() (post-set-parameter callbacks are serialized by the node parameters mutex), but destroy_clock_sub() must never return with a clock executor thread still spinning behind it: detachNode() resets the node interfaces right after, and the executor thread's lambda captures this.

The clock executor thread now also captures its executor and callback group by value instead of reading the clock_executor_ / clock_callback_group_ members: those members are moved out by destroy_clock_sub(), and a thread starting after the move would otherwise dereference a null executor. (I hit this as a segfault in local testing of the first version of this patch; the by-value capture fixed it.)

Adds test_time_source_deadlock.cpp: rapid construct/destroy with use_sim_time=true, concurrent construct/destroy across threads, toggling use_sim_time while spinning, no lingering thread after destroy, and rapid construct/destroy under live /clock traffic. If the deadlock is present these hang and trip the test timeout.

Note on the earlier attempts: #3110 was closed over an AI-disclosure violation (disclosure is given below); #3167 was closed by its author on the belief that #3174 fixed this issue, but #3174 fixed #3172 (honoring cancel() issued before spin_until_future_complete()), not #2962, which is still open.

Is this user-facing behavior change?

No, except that node teardown with use_sim_time no longer sporadically hangs.

Did you use Generative AI?

Yes, Muse Spark (Meta) was used as an assistive coding tool for drafting this change and the regression tests. I reviewed the code, ran the tests below, and take responsibility for the contribution's validity.

Test plan

Built rclcpp from source (rolling HEAD 917c1240) and ran, with RMW_IMPLEMENTATION=rmw_zenoh_cpp:

• test_time_source_deadlock: 5/5 pass, 10 consecutive runs, plus 36 parallel runs under load -- no hangs
• test_time_source: 18/18 pass
• test_node: 50/50 pass
• ament_uncrustify and ament_cpplint clean on the changed files

Honest note on reproduction: I could not reproduce the hang on this machine (100+ stress runs on the unfixed code, including parallel runs under load, all passed). That matches the issue thread itself: the reporter reproduced it on one laptop but not on others, and a maintainer could not reproduce it in 100 runs on a 28-core machine. The race is timing/hardware dependent; the stress tests exercise the exact teardown window and guard against regressions.

[sylvesterkaczmarek]

Thanks for revisiting #2962 and incorporating the concurrent-create concern from #3167, @SIDDARTHAREDDY8. Moving the subscription under the lock and capturing the executor/group by value look like useful improvements. One interleaving may still need guarding: detachNode() calls destroy_clock_sub(), whose final empty-state check can return; a concurrent post_set_parameters(true) could then enter create_clock_sub() before detachNode() acquires node_base_lock_ and clears the interfaces. That can leave a newly started clock thread using the interfaces during detach. Would a detaching flag coordinated with clock_sub_lock_ (and a test for this exact ordering) make the shutdown invariant robust? Also, the DCO check is action_required: the Signed-off-by identity differs from the commits' GitHub noreply author identity. I reviewed the code but haven't run a ROS build locally.

[SIDDARTHAREDDY8]

Addressed both review points in b23fc6f (the branch is now a single commit on the same rolling base, same 3 files):

• Detach/create race: added a detaching_ flag on NodeState, guarded by clock_sub_lock_. detachNode() sets it before any teardown, create_clock_sub() returns early while it is set, and attachNode() clears it so a re-attach can create the subscription again. That closes the window where a concurrent post_set_parameters(true) could start a fresh clock executor thread after destroy_clock_sub() returned but before the node interfaces were cleared. Added detach_races_sim_time_enable to test_time_source_deadlock.cpp, which hammers use_sim_time from a second thread while detaching, then re-attaches and detaches cleanly.
• DCO: the commit now carries both Signed-off-by lines (gmail identity and the 75976672+SIDDARTHAREDDY8@users.noreply.github.com author identity). The DCO check on the new head is green.

Verified locally with a rolling build: test_time_source_deadlock 6/6 (6 runs), test_time_source 18/18, test_node 50/50, full colcon build --packages-select rclcpp clean.

[sylvesterkaczmarek]

Thanks for the follow-up, @SIDDARTHAREDDY8. Rechecked b23fc6f: detaching_ is set under clock_sub_lock_ before teardown and prevents create_clock_sub() from restarting the executor in the window I flagged. The new detach/enable stress case covers that path, and DCO plus the hosted build now pass. That resolves both issues I raised. I have not run a separate ROS build locally; no remaining blocker from this review.