Fix potential deadlock in TimeSource::destroy_clock_sub

[PavelGuzenfeld]

Summary

Fixes #2962

destroy_clock_sub() held clock_sub_lock_ while calling clock_executor_thread_.join(). If the executor thread's callback tried to access state protected by clock_sub_lock_ before fully exiting, this produced a deadlock: the main thread waits for the executor thread to finish, while the executor thread waits for the main thread to release the lock.

The fix: Move the thread, executor, and callback group into local variables under the lock, release the lock, then join outside the critical section. This ensures the executor thread can complete its final callback without contending on clock_sub_lock_.

Before:

Main thread: lock(clock_sub_lock_) → cancel() → join() [BLOCKED] → unlock
Executor thread: callback needs clock_sub_lock_ [BLOCKED] → exit
→ DEADLOCK

After:

Main thread: lock(clock_sub_lock_) → cancel() → move thread out → unlock → join() [WAITS]
Executor thread: callback completes freely → exit
→ JOIN SUCCEEDS

Test plan

• test_time_source: 5/5 runs pass (16 tests each) — no freezes
• test_node construction/destruction: 3/3 runs pass — no freezes
• Full rclcpp build: clean

[christophebedard]

Thank you for the PR! Please see my note here: #3109 (comment)

[PavelGuzenfeld]

The CI failure is not caused by this PR. The build error is in subscription_base.cpp:486:

error: 'rcl_subscription_is_cft_supported' was not declared in this scope

This function was added to rclcpp rolling HEAD by #3089, but the CI's rcl package hasn't been updated to include the corresponding rcl_subscription_is_cft_supported API yet. The same failure affects all open rolling PRs — e.g., #3105 (unrelated to this PR) also fails identically.

This PR only modifies time_source.cpp and does not touch subscription_base.cpp. The failure will resolve once the CI's rcl package catches up with rolling HEAD.

[jmachowinski]

This PR would create a dangling thread in the test conditions from #2962.

[PavelGuzenfeld]

@jmachowinski Thanks for the review!

Regarding the "dangling thread" concern — the thread is moved to a local variable but is unconditionally joined before destroy_clock_sub() returns. It is never detached or leaked.

Improvement in this push (addressing your feedback): The subscription (clock_subscription_) is now kept alive until after the thread is joined. In the previous version, it was reset inside the lock before join, which could theoretically allow the executor thread to dispatch a callback on a destroyed subscription. The new order is:

cancel() → move thread out → release lock → join() → remove_callback_group()
→ reacquire lock → reset subscription

Why moving the thread is necessary:
Without moving, two concurrent calls to destroy_clock_sub() (e.g., from detachNode() and post_set_parameters()) could both see clock_executor_thread_.joinable() == true and attempt join() simultaneously — which is undefined behavior. Moving the thread ensures only one caller gets the joinable thread.

Added 4 stress tests (test_time_source_deadlock) that reproduce the conditions from #2962:

1. Rapid construct/destroy — 50 cycles of creating and immediately destroying sim-time nodes
2. Concurrent construct/destroy — 4 threads × 10 iterations, creating/destroying sim-time nodes
3. Toggle use_sim_time while spinning — exercises create_clock_sub/destroy_clock_sub interplay
4. No lingering thread — verifies the context is clean after destruction

All 4 tests pass. The full rclcpp suite (2962 tests) also passes with 0 errors. The existing test_time_source passes 5/5 consecutive runs with no deadlocks.

If this approach doesn't fully address the concern, I'm happy to explore adding a guard flag to prevent create_clock_sub() from running while a join is pending (Option B) as a follow-up.