Fix bug in timers lifecycle for events executor (#2586)

* Remove expired timers before updating the collection

Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com>

* Add regression test for reinitialized timers bug

Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com>

* Add missing includes

Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com>

* Relocate test under the executors directory

Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com>

* Extend test to run with all supported executors

Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com>

* Adjust comment in fix to make it more generic

Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com>

* Apply ament clang format to test

Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com>

* Fix uncrustify findings

Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com>

---------

Signed-off-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com>
Co-authored-by: Alexis Pojomovsky <apojomovsky@ekumenlabs.com>

Author: apojomovsky

rclcpp/src/rclcpp/experimental/executors/events_executor/events_executor.cpp

@@ -341,6 +341,11 @@ EventsExecutor::refresh_current_collection(
   // Acquire lock before modifying the current collection
   std::lock_guard<std::mutex> guard(mutex_);
 
+  // Remove expired entities to ensure re-initialized objects
+  // are updated. This fixes issues with stale state entities.
+  // See: https://github.com/ros2/rclcpp/pull/2586
+  current_collection_.remove_expired_entities();
+
   current_collection_.timers.update(
     new_collection.timers,
     [this](rclcpp::TimerBase::SharedPtr timer) {timers_manager_->add_timer(timer);},

rclcpp/test/rclcpp/CMakeLists.txt

@@ -450,6 +450,13 @@ if(TARGET test_interface_traits)
   target_link_libraries(test_interface_traits ${PROJECT_NAME})
 endif()
 
+ament_add_gtest(test_reinitialized_timers
+  executors/test_reinitialized_timers.cpp
+  TIMEOUT 30)
+if(TARGET test_reinitialized_timers)
+  target_link_libraries(test_reinitialized_timers ${PROJECT_NAME})
+endif()
+
 ament_add_gtest(
   test_executors
   executors/test_executors.cpp

rclcpp/test/rclcpp/executors/test_reinitialized_timers.cpp

@@ -0,0 +1,92 @@
+// Copyright 2024 iRobot Corporation.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include <gtest/gtest.h>
+
+#include <chrono>
+#include <cstddef>
+#include <memory>
+#include <thread>
+
+#include "rclcpp/executors/multi_threaded_executor.hpp"
+#include "rclcpp/executors/single_threaded_executor.hpp"
+#include "rclcpp/executors/static_single_threaded_executor.hpp"
+#include "rclcpp/experimental/executors/events_executor/events_executor.hpp"
+#include "rclcpp/rclcpp.hpp"
+
+template<typename ExecutorType>
+class TestTimersLifecycle : public testing::Test
+{
+public:
+  void SetUp() override {rclcpp::init(0, nullptr);}
+
+  void TearDown() override {rclcpp::shutdown();}
+};
+
+using ExecutorTypes = ::testing::Types<
+  rclcpp::executors::SingleThreadedExecutor, rclcpp::executors::MultiThreadedExecutor,
+  rclcpp::executors::StaticSingleThreadedExecutor, rclcpp::experimental::executors::EventsExecutor>;
+
+TYPED_TEST_SUITE(TestTimersLifecycle, ExecutorTypes);
+
+TYPED_TEST(TestTimersLifecycle, timers_lifecycle_reinitialized_object)
+{
+  auto timers_period = std::chrono::milliseconds(50);
+  auto node = std::make_shared<rclcpp::Node>("test_node");
+  auto executor = std::make_unique<TypeParam>();
+
+  executor->add_node(node);
+
+  size_t count_1 = 0;
+  auto timer_1 = rclcpp::create_timer(
+    node, node->get_clock(), rclcpp::Duration(timers_period), [&count_1]() {count_1++;});
+
+  size_t count_2 = 0;
+  auto timer_2 = rclcpp::create_timer(
+    node, node->get_clock(), rclcpp::Duration(timers_period), [&count_2]() {count_2++;});
+
+  {
+    std::thread executor_thread([&executor]() {executor->spin();});
+
+    while (count_2 < 10u) {
+      std::this_thread::sleep_for(std::chrono::milliseconds(10));
+    }
+    executor->cancel();
+    executor_thread.join();
+
+    EXPECT_GE(count_2, 10u);
+    EXPECT_LE(count_2 - count_1, 1u);
+  }
+
+  count_1 = 0;
+  timer_1 = rclcpp::create_timer(
+    node, node->get_clock(), rclcpp::Duration(timers_period), [&count_1]() {count_1++;});
+
+  count_2 = 0;
+  timer_2 = rclcpp::create_timer(
+    node, node->get_clock(), rclcpp::Duration(timers_period), [&count_2]() {count_2++;});
+
+  {
+    std::thread executor_thread([&executor]() {executor->spin();});
+
+    while (count_2 < 10u) {
+      std::this_thread::sleep_for(std::chrono::milliseconds(10));
+    }
+    executor->cancel();
+    executor_thread.join();
+
+    EXPECT_GE(count_2, 10u);
+    EXPECT_LE(count_2 - count_1, 1u);
+  }
+}