Skip to content

Commit bc5e080

Browse files
Fix potential deadlock in TimeSource::destroy_clock_sub
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 exiting, this would deadlock: main thread waits for executor thread to finish, executor thread waits for main thread to release the lock. Fix by moving the thread, executor, and callback group into local variables under the lock, then releasing the lock before joining. This ensures the executor thread can complete its final callback without contending on clock_sub_lock_. Fixes #2962 Signed-off-by: Pavel Guzenfeld <me@pavelguzenfeld.com>
1 parent af78e01 commit bc5e080

File tree

1 file changed

+23
-6
lines changed

1 file changed

+23
-6
lines changed

rclcpp/src/rclcpp/time_source.cpp

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -422,13 +422,30 @@ class TimeSource::NodeState final
422422
// Destroy the subscription for the clock topic
423423
void destroy_clock_sub()
424424
{
425-
std::lock_guard<std::mutex> guard(clock_sub_lock_);
426-
if (clock_executor_thread_.joinable()) {
427-
clock_executor_->cancel();
428-
clock_executor_thread_.join();
429-
clock_executor_->remove_callback_group(clock_callback_group_);
425+
std::thread thread_to_join;
426+
std::shared_ptr<rclcpp::executors::SingleThreadedExecutor> executor_to_clean;
427+
rclcpp::CallbackGroup::SharedPtr callback_group_to_remove;
428+
429+
{
430+
std::lock_guard<std::mutex> guard(clock_sub_lock_);
431+
if (clock_executor_thread_.joinable()) {
432+
clock_executor_->cancel();
433+
// Move thread and executor out of lock scope to avoid holding
434+
// clock_sub_lock_ while joining. The executor thread's callbacks
435+
// may access state protected by clock_sub_lock_, which would
436+
// deadlock if we held it during join().
437+
thread_to_join = std::move(clock_executor_thread_);
438+
executor_to_clean = clock_executor_;
439+
callback_group_to_remove = clock_callback_group_;
440+
}
441+
clock_subscription_.reset();
442+
}
443+
444+
// Join and clean up outside the lock
445+
if (thread_to_join.joinable()) {
446+
thread_to_join.join();
447+
executor_to_clean->remove_callback_group(callback_group_to_remove);
430448
}
431-
clock_subscription_.reset();
432449
}
433450

434451
// On set Parameters callback handle

0 commit comments

Comments
 (0)