Avoid race that triggers timer too often (#621)

The two distinct operations of acquiring and subsequent checking of a timer have to be protected by one lock_guard against races with other threads. The releasing of a timer has to be protected by the same lock. Given this requirement there is no use for a second mutex. Signed-off-by: Marko Durkovic <[email protected]>
ros2 · Mar 23, 2019 · 0a44344 · 0a44344
1 parent 43f891d
commit 0a44344
Show file tree

Hide file tree

Showing 2 changed files with 1 addition and 3 deletions.
diff --git a/rclcpp/include/rclcpp/executors/multi_threaded_executor.hpp b/rclcpp/include/rclcpp/executors/multi_threaded_executor.hpp
@@ -78,7 +78,6 @@ class MultiThreadedExecutor : public executor::Executor
   size_t number_of_threads_;
   bool yield_before_execute_;
 
-  std::mutex scheduled_timers_mutex_;
   std::set<TimerBase::SharedPtr> scheduled_timers_;
 };
 

diff --git a/rclcpp/src/rclcpp/executors/multi_threaded_executor.cpp b/rclcpp/src/rclcpp/executors/multi_threaded_executor.cpp
@@ -82,7 +82,6 @@ MultiThreadedExecutor::run(size_t)
       }
       if (any_exec.timer) {
         // Guard against multiple threads getting the same timer.
-        std::lock_guard<std::mutex> lock(scheduled_timers_mutex_);
         if (scheduled_timers_.count(any_exec.timer) != 0) {
           continue;
         }
@@ -96,7 +95,7 @@ MultiThreadedExecutor::run(size_t)
     execute_any_executable(any_exec);
 
     if (any_exec.timer) {
-      std::lock_guard<std::mutex> lock(scheduled_timers_mutex_);
+      std::lock_guard<std::mutex> wait_lock(wait_mutex_);
       auto it = scheduled_timers_.find(any_exec.timer);
       if (it != scheduled_timers_.end()) {
         scheduled_timers_.erase(it);