undo changes from initial implementation

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>

rclcpp/include/rclcpp/create_timer.hpp

@@ -90,7 +90,8 @@ create_timer(
   rclcpp::Clock::SharedPtr clock,
   rclcpp::Duration period,
   CallbackT && callback,
-  rclcpp::CallbackGroup::SharedPtr group = nullptr)
+  rclcpp::CallbackGroup::SharedPtr group = nullptr,
+  bool start_canceled = false)
 {
   return create_timer(
     clock,
@@ -98,7 +99,8 @@ create_timer(
     std::forward<CallbackT>(callback),
     group,
     node_base.get(),
-    node_timers.get());
+    node_timers.get(),
+    start_canceled);
 }
 
 /// Create a timer with a given clock
@@ -109,7 +111,8 @@ create_timer(
   rclcpp::Clock::SharedPtr clock,
   rclcpp::Duration period,
   CallbackT && callback,
-  rclcpp::CallbackGroup::SharedPtr group = nullptr)
+  rclcpp::CallbackGroup::SharedPtr group = nullptr,
+  bool start_canceled = false)
 {
   return create_timer(
     clock,
@@ -117,7 +120,8 @@ create_timer(
     std::forward<CallbackT>(callback),
     group,
     rclcpp::node_interfaces::get_node_base_interface(node).get(),
-    rclcpp::node_interfaces::get_node_timers_interface(node).get());
+    rclcpp::node_interfaces::get_node_timers_interface(node).get(),
+    start_canceled);
 }
 
 /// Convenience method to create a general timer with node resources.
@@ -132,6 +136,7 @@ create_timer(
  * \param group callback group
  * \param node_base node base interface
  * \param node_timers node timer interface
+ * \param start_canceled whether the timer should be created as canceled
  * \return shared pointer to a generic timer
  * \throws std::invalid_argument if either clock, node_base or node_timers
  * are nullptr, or period is negative or too large
@@ -144,7 +149,8 @@ create_timer(
   CallbackT callback,
   rclcpp::CallbackGroup::SharedPtr group,
   node_interfaces::NodeBaseInterface * node_base,
-  node_interfaces::NodeTimersInterface * node_timers)
+  node_interfaces::NodeTimersInterface * node_timers,
+  bool start_canceled = false)
 {
   if (clock == nullptr) {
     throw std::invalid_argument{"clock cannot be null"};
@@ -161,6 +167,11 @@ create_timer(
   // Add a new generic timer.
   auto timer = rclcpp::GenericTimer<CallbackT>::make_shared(
     std::move(clock), period_ns, std::move(callback), node_base->get_context());
+  // Cancel the timer before registering it with the node to prevent race conditions with an
+  // executor potentially immediately trying to execute it.
+  if (start_canceled) {
+    timer->cancel();
+  }
   node_timers->add_timer(timer, group);
   return timer;
 }
@@ -176,6 +187,7 @@ create_timer(
  * \param group callback group
  * \param node_base node base interface
  * \param node_timers node timer interface
+ * \param start_canceled whether the timer should be created as canceled
  * \return shared pointer to a wall timer
  * \throws std::invalid_argument if either node_base or node_timers
  * are null, or period is negative or too large
@@ -187,7 +199,8 @@ create_wall_timer(
   CallbackT callback,
   rclcpp::CallbackGroup::SharedPtr group,
   node_interfaces::NodeBaseInterface * node_base,
-  node_interfaces::NodeTimersInterface * node_timers)
+  node_interfaces::NodeTimersInterface * node_timers,
+  bool start_canceled = false)
 {
   if (node_base == nullptr) {
     throw std::invalid_argument{"input node_base cannot be null"};
@@ -202,6 +215,11 @@ create_wall_timer(
   // Add a new wall timer.
   auto timer = rclcpp::WallTimer<CallbackT>::make_shared(
     period_ns, std::move(callback), node_base->get_context());
+  // Cancel the timer before registering it with the node to prevent race conditions with an
+  // executor potentially immediately trying to execute it.
+  if (start_canceled) {
+    timer->cancel();
+  }
   node_timers->add_timer(timer, group);
   return timer;
 }

rclcpp/include/rclcpp/node.hpp

@@ -232,26 +232,30 @@ public:
    * \param[in] period Time interval between triggers of the callback.
    * \param[in] callback User-defined callback function.
    * \param[in] group Callback group to execute this timer's callback in.
+   * \param[in] start_canceled Whether the timer should be created as already canceled.
    */
   template<typename DurationRepT = int64_t, typename DurationT = std::milli, typename CallbackT>
   typename rclcpp::WallTimer<CallbackT>::SharedPtr
   create_wall_timer(
     std::chrono::duration<DurationRepT, DurationT> period,
     CallbackT callback,
-    rclcpp::CallbackGroup::SharedPtr group = nullptr);
+    rclcpp::CallbackGroup::SharedPtr group = nullptr,
+    bool start_canceled = false);
 
   /// Create a timer that uses the node clock to drive the callback.
   /**
    * \param[in] period Time interval between triggers of the callback.
    * \param[in] callback User-defined callback function.
    * \param[in] group Callback group to execute this timer's callback in.
+   * \param[in] start_canceled Whether the timer should be created as already canceled.
    */
   template<typename DurationRepT = int64_t, typename DurationT = std::milli, typename CallbackT>
   typename rclcpp::GenericTimer<CallbackT>::SharedPtr
   create_timer(
     std::chrono::duration<DurationRepT, DurationT> period,
     CallbackT callback,
-    rclcpp::CallbackGroup::SharedPtr group = nullptr);
+    rclcpp::CallbackGroup::SharedPtr group = nullptr,
+    bool start_canceled = false);
 
   /// Create and return a Client.
   /**

rclcpp/include/rclcpp/node_impl.hpp

@@ -110,14 +110,16 @@ typename rclcpp::WallTimer<CallbackT>::SharedPtr
 Node::create_wall_timer(
   std::chrono::duration<DurationRepT, DurationT> period,
   CallbackT callback,
-  rclcpp::CallbackGroup::SharedPtr group)
+  rclcpp::CallbackGroup::SharedPtr group,
+  bool start_canceled)
 {
   return rclcpp::create_wall_timer(
     period,
     std::move(callback),
     group,
     this->node_base_.get(),
-    this->node_timers_.get());
+    this->node_timers_.get(),
+    start_canceled);
 }
 
 template<typename DurationRepT, typename DurationT, typename CallbackT>
@@ -125,7 +127,8 @@ typename rclcpp::GenericTimer<CallbackT>::SharedPtr
 Node::create_timer(
   std::chrono::duration<DurationRepT, DurationT> period,
   CallbackT callback,
-  rclcpp::CallbackGroup::SharedPtr group)
+  rclcpp::CallbackGroup::SharedPtr group,
+  bool start_canceled)
 {
   return rclcpp::create_timer(
     this->get_clock(),
@@ -133,7 +136,8 @@ Node::create_timer(
     std::move(callback),
     group,
     this->node_base_.get(),
-    this->node_timers_.get());
+    this->node_timers_.get(),
+    start_canceled);
 }
 
 template<typename ServiceT>

rclcpp/test/rclcpp/test_timer.cpp

@@ -319,6 +319,23 @@ TEST_P(TestTimer, test_failures_with_exceptions)
   }
 }
 
+/// Create a timer that starts as canceled
+TEST_P(TestTimer, test_start_canceled)
+{
+  // Store the timer type for use in TEST_P declarations.
+  constexpr bool START_CANCELED = true;
+  switch (timer_type) {
+    case TimerType::WALL_TIMER:
+      timer = test_node->create_wall_timer(0ms, []() {}, nullptr, START_CANCELED);
+      break;
+    case TimerType::GENERIC_TIMER:
+      timer = test_node->create_timer(0ms, []() {}, nullptr, START_CANCELED);
+      break;
+  }
+
+  EXPECT_TRUE(timer->is_canceled());
+}
+
 INSTANTIATE_TEST_SUITE_P(
   PerTimerType, TestTimer,
   ::testing::Values(TimerType::WALL_TIMER, TimerType::GENERIC_TIMER),

rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node.hpp

@@ -258,26 +258,30 @@ public:
    * \param[in] period Time interval between triggers of the callback.
    * \param[in] callback User-defined callback function.
    * \param[in] group Callback group to execute this timer's callback in.
+   * \param[in] start_canceled Whether the timer should be created as already canceled.
    */
   template<typename DurationRepT = int64_t, typename DurationT = std::milli, typename CallbackT>
   typename rclcpp::WallTimer<CallbackT>::SharedPtr
   create_wall_timer(
     std::chrono::duration<DurationRepT, DurationT> period,
     CallbackT callback,
-    rclcpp::CallbackGroup::SharedPtr group = nullptr);
+    rclcpp::CallbackGroup::SharedPtr group = nullptr,
+    bool start_canceled = false);
 
   /// Create a timer that uses the node clock to drive the callback.
   /**
    * \param[in] period Time interval between triggers of the callback.
    * \param[in] callback User-defined callback function.
    * \param[in] group Callback group to execute this timer's callback in.
+   * \param[in] start_canceled Whether the timer should be created as already canceled.
    */
   template<typename DurationRepT = int64_t, typename DurationT = std::milli, typename CallbackT>
   typename rclcpp::GenericTimer<CallbackT>::SharedPtr
   create_timer(
     std::chrono::duration<DurationRepT, DurationT> period,
     CallbackT callback,
-    rclcpp::CallbackGroup::SharedPtr group = nullptr);
+    rclcpp::CallbackGroup::SharedPtr group = nullptr,
+    bool start_canceled = false);
 
   /// Create and return a Client.
   /**

rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node_impl.hpp

@@ -94,14 +94,16 @@ typename rclcpp::WallTimer<CallbackT>::SharedPtr
 LifecycleNode::create_wall_timer(
   std::chrono::duration<DurationRepT, DurationT> period,
   CallbackT callback,
-  rclcpp::CallbackGroup::SharedPtr group)
+  rclcpp::CallbackGroup::SharedPtr group,
+  bool start_canceled)
 {
   return rclcpp::create_wall_timer(
     period,
     std::move(callback),
     group,
     this->node_base_.get(),
-    this->node_timers_.get());
+    this->node_timers_.get(),
+    start_canceled);
 }
 
 template<typename DurationRepT, typename DurationT, typename CallbackT>
@@ -109,7 +111,8 @@ typename rclcpp::GenericTimer<CallbackT>::SharedPtr
 LifecycleNode::create_timer(
   std::chrono::duration<DurationRepT, DurationT> period,
   CallbackT callback,
-  rclcpp::CallbackGroup::SharedPtr group)
+  rclcpp::CallbackGroup::SharedPtr group,
+  bool start_canceled)
 {
   return rclcpp::create_timer(
     this->get_clock(),
@@ -117,7 +120,8 @@ LifecycleNode::create_timer(
     std::move(callback),
     group,
     this->node_base_.get(),
-    this->node_timers_.get());
+    this->node_timers_.get(),
+    start_canceled);
 }
 
 template<typename ServiceT>