fixups

Signed-off-by: William Woodall <william@osrfoundation.org>

README.md

@@ -8,7 +8,7 @@ rclcpp provides the standard C++ API for interacting with ROS 2.
 
 `#include "rclcpp/rclcpp.hpp"` allows use of the most common elements of the ROS 2 system.
 
-The link to the latest API documentation can be found on the [rclcpp package info page](https://docs.ros.org/en/rolling/p/rclcpp).
+The link to the latest API documentation can be found on the rclcpp package info page, at the [ROS Index](https://index.ros.org/p/rclcpp/).
 
 
 ### Examples

rclcpp/CHANGELOG.rst

@@ -2,161 +2,6 @@
 Changelog for package rclcpp
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-28.3.0 (2024-06-17)
--------------------
-* Add test creating two content filter topics with the same topic name (`#2546 <https://github.com/ros2/rclcpp/issues/2546>`_) (`#2549 <https://github.com/ros2/rclcpp/issues/2549>`_)
-* add impl pointer for ExecutorOptions (`#2523 <https://github.com/ros2/rclcpp/issues/2523>`_)
-* Fixup Executor::spin_all() regression fix (`#2517 <https://github.com/ros2/rclcpp/issues/2517>`_)
-* Add 'mimick' label to tests which use Mimick (`#2516 <https://github.com/ros2/rclcpp/issues/2516>`_)
-* Contributors: Alejandro Hernández Cordero, Scott K Logan, William Woodall
-
-28.2.0 (2024-04-26)
--------------------
-* Check for negative time in rclcpp::Time(int64_t nanoseconds, ...) constructor (`#2510 <https://github.com/ros2/rclcpp/issues/2510>`_)
-* Revise the description of service configure_introspection() (`#2511 <https://github.com/ros2/rclcpp/issues/2511>`_)
-* Contributors: Barry Xu, Sharmin Ramli
-
-28.1.0 (2024-04-16)
--------------------
-* Remove references to index.ros.org. (`#2504 <https://github.com/ros2/rclcpp/issues/2504>`_)
-* Reduce overhead for inheriting from rclcpp::Executor when base functionality is not reused (`#2506 <https://github.com/ros2/rclcpp/issues/2506>`_)
-* Contributors: Chris Lalancette, William Woodall, jmachowinski
-
-28.0.1 (2024-04-16)
--------------------
-* [wjwwood] Updated "Data race fixes" (`#2500 <https://github.com/ros2/rclcpp/issues/2500>`_)
-  * Fix callback group logic in executor
-  * fix: Fixed unnecessary copy of wait_set
-  * fix(executor): Fixed race conditions with rebuild of wait_sets
-  Before this change, the rebuild of wait set would be triggered
-  after the wait set was waken up. With bad timing, this could
-  lead to the rebuild not happening with multi threaded executor.
-  * fix(Executor): Fixed lost of entities rebuild request
-  * chore: Added assert for not set callback_group in execute_any_executable
-  * Add test for cbg getting reset
-  Co-authored-by: Janosch Machowinski <j.machowinski@nospam.org>
-  * chore: renamed test cases to snake_case
-  * style
-  * fixup test to avoid polling and short timeouts
-  * fix: Use correct notify_waitable\_ instance
-  * fix(StaticSingleThreadedExecutor): Added missing special case handling for current_notify_waitable\_
-  * fix(TestCallbackGroup): Fixed test after change to timers
-  ---------
-  Co-authored-by: Janosch Machowinski <j.machowinski@cellumation.com>
-  Co-authored-by: Michael Carroll <mjcarroll@intrinsic.ai>
-  Co-authored-by: Janosch Machowinski <j.machowinski@nospam.org>
-* fixup var names to snake case (`#2501 <https://github.com/ros2/rclcpp/issues/2501>`_)
-* Added optional TimerInfo to timer callback (`#2343 <https://github.com/ros2/rclcpp/issues/2343>`_)
-  Co-authored-by: Alexis Tsogias <a.tsogias@cellumation.com>
-  Co-authored-by: Janosch Machowinski <J.Machowinski@cellumation.com>
-* Fix uninitialized memory in test (`#2498 <https://github.com/ros2/rclcpp/issues/2498>`_)
-  When I added in the tests for large messages, I made a mistake and reserved space in the strings, but didn't actually expand it.  Thus, we were writing into uninitialized memory.  Fix this by just using the correct constructor for string, which will allocate and initialize the memory properly.
-* Ensure waitables handle guard condition retriggering (`#2483 <https://github.com/ros2/rclcpp/issues/2483>`_)
-  Co-authored-by: Michael Carroll <mjcarroll@intrinsic.ai>
-* fix: init concatenated_vector with begin() & end() (`#2492 <https://github.com/ros2/rclcpp/issues/2492>`_)
-  * this commit will fix the warning [-Wstringop-overflow=] `#2461 <https://github.com/ros2/rclcpp/issues/2461>`_
-* Use the same context for the specified node in rclcpp::spin functions (`#2433 <https://github.com/ros2/rclcpp/issues/2433>`_)
-  * Use the same conext for the specified node in rclcpp::spin_xx functions
-  * Add test for spinning with non-default-context
-  * Format code
-  ---------
-* Disable compare-function-pointers in test_utilities (`#2489 <https://github.com/ros2/rclcpp/issues/2489>`_)
-* address ambiguous auto variable. (`#2481 <https://github.com/ros2/rclcpp/issues/2481>`_)
-* Increase the cppcheck timeout to 1200 seconds (`#2484 <https://github.com/ros2/rclcpp/issues/2484>`_)
-* Removed test_timers_manager clang warning (`#2479 <https://github.com/ros2/rclcpp/issues/2479>`_)
-* Flaky timer test fix (`#2469 <https://github.com/ros2/rclcpp/issues/2469>`_)
-  * fix(time_source): Fixed possible race condition
-  * fix(test_executors_time_cancel_behaviour): Fixed multiple race conditions
-  ---------
-  Co-authored-by: Janosch Machowinski <j.machowinski@nospam.org>
-* Add tracepoint for generic publisher/subscriber (`#2448 <https://github.com/ros2/rclcpp/issues/2448>`_)
-* update rclcpp::Waitable API to use references and const (`#2467 <https://github.com/ros2/rclcpp/issues/2467>`_)
-* Utilize rclcpp::WaitSet as part of the executors (`#2142 <https://github.com/ros2/rclcpp/issues/2142>`_)
-  * Deprecate callback_group call taking context
-  * Add base executor objects that can be used by implementors
-  * Template common operations
-  * Address reviewer feedback:
-  * Add callback to EntitiesCollector constructor
-  * Make function to check automatically added callback groups take a list
-  * Lint
-  * Address reviewer feedback and fix templates
-  * Lint and docs
-  * Make executor own the notify waitable
-  * Add pending queue to collector, remove from waitable
-  Also change node's get_guard_condition to return shared_ptr
-  * Change interrupt guard condition to shared_ptr
-  Check if guard condition is valid before adding it to the waitable
-  * Lint and docs
-  * Utilize rclcpp::WaitSet as part of the executors
-  * Don't exchange atomic twice
-  * Fix add_node and add more tests
-  * Make get_notify_guard_condition follow API tick-tock
-  * Improve callback group tick-tocking
-  * Don't lock twice
-  * Address reviewer feedback
-  * Add thread safety annotations and make locks consistent
-  * @wip
-  * Reset callback groups for multithreaded executor
-  * Avoid many small function calls when building executables
-  * Re-trigger guard condition if buffer has data
-  * Address reviewer feedback
-  * Trace points
-  * Remove tracepoints
-  * Reducing diff
-  * Reduce diff
-  * Uncrustify
-  * Restore tests
-  * Back to weak_ptr and reduce test time
-  * reduce diff and lint
-  * Restore static single threaded tests that weren't working before
-  * Restore more tests
-  * Fix multithreaded test
-  * Fix assert
-  * Fix constructor test
-  * Change ready_executables signature back
-  * Don't enforce removing callback groups before nodes
-  * Remove the "add_valid_node" API
-  * Only notify if the trigger condition is valid
-  * Only trigger if valid and needed
-  * Fix spin_some/spin_all implementation
-  * Restore single threaded executor
-  * Picking ABI-incompatible executor changes
-  * Add PIMPL
-  * Additional waitset prune
-  * Fix bad merge
-  * Expand test timeout
-  * Introduce method to clear expired entities from a collection
-  * Make sure to call remove_expired_entities().
-  * Prune queued work when callback group is removed
-  * Prune subscriptions from dynamic storage
-  * Styles fixes.
-  * Re-trigger guard conditions
-  * Condense to just use watiable.take_data
-  * Lint
-  * Address reviewer comments (nits)
-  * Lock mutex when copying
-  * Refactors to static single threaded based on reviewers
-  * More small refactoring
-  * Lint
-  * Lint
-  * Add ready executable accessors to WaitResult
-  * Make use of accessors from wait_set
-  * Fix tests
-  * Fix more tests
-  * Tidy up single threaded executor implementation
-  * Don't null out timer, rely on call
-  * change how timers are checked from wait result in executors
-  * peak -> peek
-  * fix bug in next_waitable logic
-  * fix bug in StaticSTE that broke the add callback groups to executor tests
-  * style
-  ---------
-  Co-authored-by: Chris Lalancette <clalancette@gmail.com>
-  Co-authored-by: William Woodall <william@osrfoundation.org>
-* fix flakiness in TestTimersManager unit-test (`#2468 <https://github.com/ros2/rclcpp/issues/2468>`_)
-  the previous version of the test was relying on the assumption that a timer with 1ms period gets called at least 6 times if the main thread waits 15ms. this is true most of the times, but it's not guaranteed, especially when running the test on windows CI servers. the new version of the test makes no assumptions on how much time it takes for the timers manager to invoke the timers, but rather focuses on ensuring that they are called the right amount of times, which is what's important for the purpose of the test
-* Contributors: Alberto Soragna, Alejandro Hernández Cordero, Chris Lalancette, Homalozoa X, Kotaro Yoshimoto, Michael Carroll, Tomoya Fujita, William Woodall, h-suzuki-isp, jmachowinski
-
 28.0.0 (2024-03-28)
 -------------------
 * fix spin_some_max_duration unit-test for events-executor (`#2465 <https://github.com/ros2/rclcpp/issues/2465>`_)

rclcpp/CMakeLists.txt

@@ -63,7 +63,6 @@ set(${PROJECT_NAME}_SRCS
   src/rclcpp/exceptions/exceptions.cpp
   src/rclcpp/executable_list.cpp
   src/rclcpp/executor.cpp
-  src/rclcpp/executor_options.cpp
   src/rclcpp/executors.cpp
   src/rclcpp/executors/executor_entities_collection.cpp
   src/rclcpp/executors/executor_entities_collector.cpp

rclcpp/README.md

@@ -2,7 +2,7 @@
 
 The ROS client library in C++.
 
-The link to the latest rclcpp API documentation, which includes a complete list of its main components and features, can be found on the [rclcpp package info page](https://docs.ros.org/en/rolling/p/rclcpp).
+The link to the latest rclcpp API documentation, which includes a complete list of its main components and features, can be found on the rclcpp package info page, at the [ROS Index](https://index.ros.org/p/rclcpp/).
 
 ## Quality Declaration
 

rclcpp/include/rclcpp/clock.hpp

@@ -193,16 +193,6 @@ public:
   bool
   ros_time_is_active();
 
-  /**
-   * Cancels an ongoing or future sleep operation of one thread.
-   *
-   * This function can be used by one thread, to wakeup another thread that is
-   * blocked using any of the sleep_ or wait_ methods of this class.
-   */
-  RCLCPP_PUBLIC
-  void
-  cancel_sleep_or_wait();
-
   /// Return the rcl_clock_t clock handle
   RCLCPP_PUBLIC
   rcl_clock_t *

rclcpp/include/rclcpp/executor.hpp

@@ -82,6 +82,19 @@ public:
   virtual void
   spin() = 0;
 
+  /// Version of spin() that takes an exception handler to be called when a callback throws.
+  /**
+   * \sa rclcpp::Executor:spin() for more details
+   * \throws std::runtime_error when spin() called while already spinning
+   * \param[in] exception_handler will be called for every exception in the processing threads
+   *
+   * The exception_handler can be called from multiple threads at the same time.
+   * The exception_handler shall rethrow the exception it if wants to terminate the program.
+   */
+  RCLCPP_PUBLIC
+  virtual void
+  spin(const std::function<void(const std::exception & e)> & exception_handler) = 0;
+
   /// Add a callback group to an executor.
   /**
    * An executor can have zero or more callback groups which provide work during `spin` functions.
@@ -272,12 +285,12 @@ public:
    * \param[in] node Shared pointer to the node to add.
    */
   RCLCPP_PUBLIC
-  virtual void
+  void
   spin_node_some(rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node);
 
   /// Convenience function which takes Node and forwards NodeBaseInterface.
   RCLCPP_PUBLIC
-  virtual void
+  void
   spin_node_some(std::shared_ptr<rclcpp::Node> node);
 
   /// Collect work once and execute all available work, optionally within a max duration.
@@ -307,14 +320,14 @@ public:
    * \param[in] node Shared pointer to the node to add.
    */
   RCLCPP_PUBLIC
-  virtual void
+  void
   spin_node_all(
     rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node,
     std::chrono::nanoseconds max_duration);
 
   /// Convenience function which takes Node and forwards NodeBaseInterface.
   RCLCPP_PUBLIC
-  virtual void
+  void
   spin_node_all(std::shared_ptr<rclcpp::Node> node, std::chrono::nanoseconds max_duration);
 
   /// Collect and execute work repeatedly within a duration or until no more work is available.
@@ -366,12 +379,52 @@ public:
     const FutureT & future,
     std::chrono::duration<TimeRepT, TimeT> timeout = std::chrono::duration<TimeRepT, TimeT>(-1))
   {
-    return spin_until_future_complete_impl(
-      std::chrono::duration_cast<std::chrono::nanoseconds>(timeout),
-      [&future](std::chrono::nanoseconds wait_time) {
-        return future.wait_for(wait_time);
+    // TODO(wjwwood): does not work recursively; can't call spin_node_until_future_complete
+    // inside a callback executed by an executor.
+
+    // Check the future before entering the while loop.
+    // If the future is already complete, don't try to spin.
+    std::future_status status = future.wait_for(std::chrono::seconds(0));
+    if (status == std::future_status::ready) {
+      return FutureReturnCode::SUCCESS;
+    }
+
+    auto end_time = std::chrono::steady_clock::now();
+    std::chrono::nanoseconds timeout_ns = std::chrono::duration_cast<std::chrono::nanoseconds>(
+      timeout);
+    if (timeout_ns > std::chrono::nanoseconds::zero()) {
+      end_time += timeout_ns;
+    }
+    std::chrono::nanoseconds timeout_left = timeout_ns;
+
+    if (spinning.exchange(true)) {
+      throw std::runtime_error("spin_until_future_complete() called while already spinning");
+    }
+    RCPPUTILS_SCOPE_EXIT(this->spinning.store(false); );
+    while (rclcpp::ok(this->context_) && spinning.load()) {
+      // Do one item of work.
+      spin_once_impl(timeout_left);
+
+      // Check if the future is set, return SUCCESS if it is.
+      status = future.wait_for(std::chrono::seconds(0));
+      if (status == std::future_status::ready) {
+        return FutureReturnCode::SUCCESS;
       }
-    );
+      // If the original timeout is < 0, then this is blocking, never TIMEOUT.
+      if (timeout_ns < std::chrono::nanoseconds::zero()) {
+        continue;
+      }
+      // Otherwise check if we still have time to wait, return TIMEOUT if not.
+      auto now = std::chrono::steady_clock::now();
+      if (now >= end_time) {
+        return FutureReturnCode::TIMEOUT;
+      }
+      // Subtract the elapsed time from the original timeout.
+      timeout_left = std::chrono::duration_cast<std::chrono::nanoseconds>(end_time - now);
+    }
+
+    // The future did not complete before ok() returned false, return INTERRUPTED.
+    return FutureReturnCode::INTERRUPTED;
   }
 
   /// Cancel any running spin* function, causing it to return.
@@ -380,7 +433,7 @@ public:
    * \throws std::runtime_error if there is an issue triggering the guard condition
    */
   RCLCPP_PUBLIC
-  virtual void
+  void
   cancel();
 
   /// Returns true if the executor is currently spinning.
@@ -393,14 +446,6 @@ public:
   is_spinning();
 
 protected:
-  /// Constructor that will not initialize any non-trivial members.
-  /**
-  * This constructor is intended to be used by any derived executor
-  * that explicitly does not want to use the default implementation provided
-  * by this class.
-  */
-  explicit Executor(const std::shared_ptr<rclcpp::Context> & context);
-
   /// Add a node to executor, execute the next available unit of work, and remove the node.
   /**
    * Implementation of spin_node_once using std::chrono::nanoseconds
@@ -415,23 +460,6 @@ protected:
     rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node,
     std::chrono::nanoseconds timeout);
 
-  /// Spin (blocking) until the future is complete, it times out waiting, or rclcpp is interrupted.
-  /**
-   * \sa spin_until_future_complete()
-   * The only difference with spin_until_future_complete() is that the future's
-   * type is obscured through a std::function which lets you wait on it
-   * reguardless of type.
-   *
-   * \param[in] timeout see spin_until_future_complete() for details
-   * \param[in] wait_for_future function to wait on the future and get the
-   *   status after waiting
-   */
-  RCLCPP_PUBLIC
-  virtual FutureReturnCode
-  spin_until_future_complete_impl(
-    std::chrono::nanoseconds timeout,
-    const std::function<std::future_status(std::chrono::nanoseconds wait_time)> & wait_for_future);
-
   /// Collect work and execute available work, optionally within a duration.
   /**
    * Implementation of spin_some and spin_all.
@@ -447,7 +475,7 @@ protected:
 
   /// Find the next available executable and do the work associated with it.
   /**
-   * \param[in] any_exec Union structure that can hold any executable type (timer, subscription,
+   * \param[in] any_exec Structure that can hold any executable type (timer, subscription,
    * service, client).
    * \throws std::runtime_error if there is an issue triggering the guard condition
    */
@@ -455,6 +483,19 @@ protected:
   void
   execute_any_executable(AnyExecutable & any_exec);
 
+  /// Find the next available executable and do the work associated with it.
+  /**
+   * \param[in] any_exec Structure that can hold any executable type (timer, subscription,
+   * service, client).
+   * \param[in] exception_handler will be called for every exception in the processing threads
+   * \throws std::runtime_error if there is an issue triggering the guard condition
+   */
+  RCLCPP_PUBLIC
+  void
+  execute_any_executable(
+    AnyExecutable & any_exec,
+    const std::function<void(const std::exception & e)> & exception_handler);
+
   /// Run subscription executable.
   /**
    * Do necessary setup and tear-down as well as executing the subscription.

rclcpp/include/rclcpp/executor_options.hpp

@@ -15,8 +15,6 @@
 #ifndef RCLCPP__EXECUTOR_OPTIONS_HPP_
 #define RCLCPP__EXECUTOR_OPTIONS_HPP_
 
-#include <memory>
-
 #include "rclcpp/context.hpp"
 #include "rclcpp/contexts/default_context.hpp"
 #include "rclcpp/memory_strategies.hpp"
@@ -26,30 +24,18 @@
 namespace rclcpp
 {
 
-class ExecutorOptionsImplementation;
-
 /// Options to be passed to the executor constructor.
 struct ExecutorOptions
 {
-  RCLCPP_PUBLIC
-  ExecutorOptions();
-
-  RCLCPP_PUBLIC
-  virtual ~ExecutorOptions();
-
-  RCLCPP_PUBLIC
-  ExecutorOptions(const ExecutorOptions &);
-
-  RCLCPP_PUBLIC
-  ExecutorOptions & operator=(const ExecutorOptions &);
+  ExecutorOptions()
+  : memory_strategy(rclcpp::memory_strategies::create_default_strategy()),
+    context(rclcpp::contexts::get_global_default_context()),
+    max_conditions(0)
+  {}
 
   rclcpp::memory_strategy::MemoryStrategy::SharedPtr memory_strategy;
   rclcpp::Context::SharedPtr context;
   size_t max_conditions;
-
-private:
-  /// Pointer to implementation
-  std::unique_ptr<ExecutorOptionsImplementation> impl_;
 };
 
 }  // namespace rclcpp

rclcpp/include/rclcpp/executors/multi_threaded_executor.hpp

@@ -69,6 +69,18 @@ public:
   void
   spin() override;
 
+  /**
+   * \sa rclcpp::Executor:spin() for more details
+   * \throws std::runtime_error when spin() called while already spinning
+   * \param[in] exception_handler will be called for every exception in the processing threads
+   *
+   * The exception_handler can be called from multiple threads at the same time.
+   * The exception_handler shall rethrow the exception it if wants to terminate the program.
+   */
+  RCLCPP_PUBLIC
+  void
+  spin(const std::function<void(const std::exception & e)> & exception_handler) override;
+
   RCLCPP_PUBLIC
   size_t
   get_number_of_threads();
@@ -76,7 +88,9 @@ public:
 protected:
   RCLCPP_PUBLIC
   void
-  run(size_t this_thread_number);
+  run(
+    size_t this_thread_number,
+    const std::function<void(const std::exception & e)> & exception_handler);
 
 private:
   RCLCPP_DISABLE_COPY(MultiThreadedExecutor)

rclcpp/include/rclcpp/executors/single_threaded_executor.hpp

@@ -65,6 +65,18 @@ public:
   void
   spin() override;
 
+  /// Single-threaded implementation of spin.
+  /**
+   * \sa rclcpp::SingleThreadedExecutor:spin() for more details
+   * \throws std::runtime_error when spin() called while already spinning
+   * \param[in] exception_handler will be called for every exception in the processing threads
+   *
+   * The exception_handler shall rethrow the exception it if wants to terminate the program.
+   */
+  RCLCPP_PUBLIC
+  void
+  spin(const std::function<void(const std::exception & e)> & exception_handler) override;
+
 private:
   RCLCPP_DISABLE_COPY(SingleThreadedExecutor)
 };

rclcpp/include/rclcpp/executors/static_single_threaded_executor.hpp

@@ -68,6 +68,16 @@ public:
   void
   spin() override;
 
+  /**
+   * \sa rclcpp::SingleThreadedExecutor:spin() for more details
+   * \throws std::runtime_error when spin() called while already spinning
+   * \param[in] exception_handler will be called for every exception in the processing threads
+   *
+   * The exception_handler shall rethrow the exception it if wants to terminate the program.
+   */RCLCPP_PUBLIC
+  virtual void
+  spin(const std::function<void(const std::exception & e)> & exception_handler) override;
+
   /// Static executor implementation of spin some
   /**
    * This non-blocking function will execute entities that
@@ -125,6 +135,11 @@ protected:
   void
   spin_once_impl(std::chrono::nanoseconds timeout) override;
 
+  void
+  spin_once_impl(
+    std::chrono::nanoseconds timeout,
+    const std::function<void(const std::exception & e)> & exception_handler);
+
   std::optional<rclcpp::WaitResult<rclcpp::WaitSet>>
   collect_and_wait(std::chrono::nanoseconds timeout);
 

rclcpp/include/rclcpp/experimental/executors/events_executor/events_executor.hpp

@@ -92,6 +92,18 @@ public:
   void
   spin() override;
 
+  /**
+   * \sa rclcpp::Executor:spin() for more details
+   * \throws std::runtime_error when spin() called while already spinning
+   * \param exception_handler will be called for every exception in the processing threads
+   *
+   * The exception_handler can be called from multiple threads at the same time.
+   * The exception_handler shall rethrow the exception it if wants to terminate the program.
+   */
+  RCLCPP_PUBLIC
+  void
+  spin(const std::function<void(const std::exception & e)> & exception_handler) override;
+
   /// Events executor implementation of spin some
   /**
    * This non-blocking function will execute the timers and events

rclcpp/include/rclcpp/experimental/timers_manager.hpp

@@ -125,9 +125,14 @@ public:
   /**
    * @brief Starts a thread that takes care of executing the timers stored in this object.
    * Function will throw an error if the timers thread was already running.
+   *
+   * @param exception_handler if valid, the execution of the timer will be done in a try catch block,
+   *                          and any occurring exception will be passed to the given handler
    */
   RCLCPP_PUBLIC
-  void start();
+  void start(
+    const std::function<void(const std::exception & e)> & exception_handler = std::function<void(
+      const std::exception & e)>());
 
   /**
    * @brief Stops the timers thread.
@@ -511,6 +516,11 @@ private:
    */
   void run_timers();
 
+  /**
+   * @brief calls run_timers with a try catch block.
+   */
+  void run_timers(const std::function<void(const std::exception & e)> & exception_handler);
+
   /**
    * @brief Get the amount of time before the next timer triggers.
    * This function is not thread safe, acquire a mutex before calling it.
@@ -528,7 +538,7 @@ private:
    * while keeping the heap correctly sorted.
    * This function is not thread safe, acquire the timers_mutex_ before calling it.
    */
-  void execute_ready_timers_unsafe();
+  void execute_ready_timers_unsafe(std::function<void(const std::exception & e)> exception_handler);
 
   // Callback to be called when timer is ready
   std::function<void(const rclcpp::TimerBase *,

rclcpp/include/rclcpp/service.hpp

@@ -499,7 +499,7 @@ public:
     }
   }
 
-  /// Configure service introspection.
+  /// Configure client introspection.
   /**
    * \param[in] clock clock to use to generate introspection timestamps
    * \param[in] qos_service_event_pub QoS settings to use when creating the introspection publisher

rclcpp/include/rclcpp/time.hpp

@@ -49,7 +49,6 @@ public:
   /**
    * \param nanoseconds since time epoch
    * \param clock_type clock type
-   * \throws std::runtime_error if nanoseconds are negative
    */
   RCLCPP_PUBLIC
   explicit Time(int64_t nanoseconds = 0, rcl_clock_type_t clock_type = RCL_SYSTEM_TIME);

rclcpp/package.xml

@@ -2,7 +2,7 @@
 <?xml-model href="http://download.ros.org/schema/package_format2.xsd" schematypens="http://www.w3.org/2001/XMLSchema"?>
 <package format="2">
   <name>rclcpp</name>
-  <version>28.3.0</version>
+  <version>28.0.0</version>
   <description>The ROS client library in C++.</description>
 
   <maintainer email="ivanpauno@ekumenlabs.com">Ivan Paunovic</maintainer>

rclcpp/src/rclcpp/clock.cpp

@@ -49,10 +49,6 @@ public:
 
   rcl_clock_t rcl_clock_;
   rcl_allocator_t allocator_;
-  bool stop_sleeping_ = false;
-  bool shutdown_ = false;
-  std::condition_variable cv_;
-  std::mutex wait_mutex_;
   std::mutex clock_mutex_;
 };
 
@@ -83,20 +79,8 @@ Clock::now() const
   return now;
 }
 
-void
-Clock::cancel_sleep_or_wait()
-{
-  {
-    std::unique_lock lock(impl_->wait_mutex_);
-    impl_->stop_sleeping_ = true;
-  }
-  impl_->cv_.notify_one();
-}
-
 bool
-Clock::sleep_until(
-  Time until,
-  Context::SharedPtr context)
+Clock::sleep_until(Time until, Context::SharedPtr context)
 {
   if (!context || !context->is_valid()) {
     throw std::runtime_error("context cannot be slept with because it's invalid");
@@ -107,14 +91,12 @@ Clock::sleep_until(
   }
   bool time_source_changed = false;
 
+  std::condition_variable cv;
+
   // Wake this thread if the context is shutdown
   rclcpp::OnShutdownCallbackHandle shutdown_cb_handle = context->add_on_shutdown_callback(
-    [this]() {
-      {
-        std::unique_lock lock(impl_->wait_mutex_);
-        impl_->shutdown_ = true;
-      }
-      impl_->cv_.notify_one();
+    [&cv]() {
+      cv.notify_one();
     });
   // No longer need the shutdown callback when this function exits
   auto callback_remover = rcpputils::scope_exit(
@@ -130,24 +112,22 @@ Clock::sleep_until(
     const std::chrono::steady_clock::time_point chrono_until =
       chrono_entry + std::chrono::nanoseconds(delta_t.nanoseconds());
 
-    // loop over spurious wakeups but notice shutdown or stop of sleep
-    std::unique_lock lock(impl_->wait_mutex_);
-    while (now() < until && !impl_->stop_sleeping_ && !impl_->shutdown_ && context->is_valid()) {
-      impl_->cv_.wait_until(lock, chrono_until);
+    // loop over spurious wakeups but notice shutdown
+    std::unique_lock lock(impl_->clock_mutex_);
+    while (now() < until && context->is_valid()) {
+      cv.wait_until(lock, chrono_until);
     }
-    impl_->stop_sleeping_ = false;
   } else if (this_clock_type == RCL_SYSTEM_TIME) {
     auto system_time = std::chrono::system_clock::time_point(
       // Cast because system clock resolution is too big for nanoseconds on some systems
       std::chrono::duration_cast<std::chrono::system_clock::duration>(
         std::chrono::nanoseconds(until.nanoseconds())));
 
-    // loop over spurious wakeups but notice shutdown or stop of sleep
-    std::unique_lock lock(impl_->wait_mutex_);
-    while (now() < until && !impl_->stop_sleeping_ && !impl_->shutdown_ && context->is_valid()) {
-      impl_->cv_.wait_until(lock, system_time);
+    // loop over spurious wakeups but notice shutdown
+    std::unique_lock lock(impl_->clock_mutex_);
+    while (now() < until && context->is_valid()) {
+      cv.wait_until(lock, system_time);
     }
-    impl_->stop_sleeping_ = false;
   } else if (this_clock_type == RCL_ROS_TIME) {
     // Install jump handler for any amount of time change, for two purposes:
     // - if ROS time is active, check if time reached on each new clock sample
@@ -159,12 +139,11 @@ Clock::sleep_until(
     threshold.min_forward.nanoseconds = 1;
     auto clock_handler = create_jump_callback(
       nullptr,
-      [this, &time_source_changed](const rcl_time_jump_t & jump) {
+      [&cv, &time_source_changed](const rcl_time_jump_t & jump) {
         if (jump.clock_change != RCL_ROS_TIME_NO_CHANGE) {
-          std::lock_guard<std::mutex> lk(impl_->wait_mutex_);
           time_source_changed = true;
         }
-        impl_->cv_.notify_one();
+        cv.notify_one();
       },
       threshold);
 
@@ -174,25 +153,19 @@ Clock::sleep_until(
         std::chrono::duration_cast<std::chrono::system_clock::duration>(
           std::chrono::nanoseconds(until.nanoseconds())));
 
-      // loop over spurious wakeups but notice shutdown, stop of sleep or time source change
-      std::unique_lock lock(impl_->wait_mutex_);
-      while (now() < until && !impl_->stop_sleeping_ && !impl_->shutdown_ && context->is_valid() &&
-        !time_source_changed)
-      {
-        impl_->cv_.wait_until(lock, system_time);
+      // loop over spurious wakeups but notice shutdown or time source change
+      std::unique_lock lock(impl_->clock_mutex_);
+      while (now() < until && context->is_valid() && !time_source_changed) {
+        cv.wait_until(lock, system_time);
       }
-      impl_->stop_sleeping_ = false;
     } else {
       // RCL_ROS_TIME with ros_time_is_active.
       // Just wait without "until" because installed
       // jump callbacks wake the cv on every new sample.
-      std::unique_lock lock(impl_->wait_mutex_);
-      while (now() < until && !impl_->stop_sleeping_ && !impl_->shutdown_ && context->is_valid() &&
-        !time_source_changed)
-      {
-        impl_->cv_.wait(lock);
+      std::unique_lock lock(impl_->clock_mutex_);
+      while (now() < until && context->is_valid() && !time_source_changed) {
+        cv.wait(lock);
       }
-      impl_->stop_sleeping_ = false;
     }
   }
 

rclcpp/src/rclcpp/executor.cpp

@@ -50,14 +50,6 @@ static constexpr rclcpp::SubscriptionWaitSetMask kDefaultSubscriptionMask = {tru
 
 class rclcpp::ExecutorImplementation {};
 
-Executor::Executor(const std::shared_ptr<rclcpp::Context> & context)
-: spinning(false),
-  entities_need_rebuild_(true),
-  collector_(nullptr),
-  wait_set_({}, {}, {}, {}, {}, {}, context)
-{
-}
-
 Executor::Executor(const rclcpp::ExecutorOptions & options)
 : spinning(false),
   interrupt_guard_condition_(std::make_shared<rclcpp::GuardCondition>(options.context)),
@@ -128,8 +120,7 @@ Executor::~Executor()
   }
 }
 
-void
-Executor::trigger_entity_recollect(bool notify)
+void Executor::trigger_entity_recollect(bool notify)
 {
   this->entities_need_rebuild_.store(true);
 
@@ -249,59 +240,6 @@ Executor::spin_node_once_nanoseconds(
   this->remove_node(node, false);
 }
 
-rclcpp::FutureReturnCode
-Executor::spin_until_future_complete_impl(
-  std::chrono::nanoseconds timeout,
-  const std::function<std::future_status(std::chrono::nanoseconds wait_time)> & wait_for_future)
-{
-  // TODO(wjwwood): does not work recursively; can't call spin_node_until_future_complete
-  // inside a callback executed by an executor.
-
-  // Check the future before entering the while loop.
-  // If the future is already complete, don't try to spin.
-  std::future_status status = wait_for_future(std::chrono::seconds(0));
-  if (status == std::future_status::ready) {
-    return FutureReturnCode::SUCCESS;
-  }
-
-  auto end_time = std::chrono::steady_clock::now();
-  std::chrono::nanoseconds timeout_ns = std::chrono::duration_cast<std::chrono::nanoseconds>(
-    timeout);
-  if (timeout_ns > std::chrono::nanoseconds::zero()) {
-    end_time += timeout_ns;
-  }
-  std::chrono::nanoseconds timeout_left = timeout_ns;
-
-  if (spinning.exchange(true)) {
-    throw std::runtime_error("spin_until_future_complete() called while already spinning");
-  }
-  RCPPUTILS_SCOPE_EXIT(this->spinning.store(false); );
-  while (rclcpp::ok(this->context_) && spinning.load()) {
-    // Do one item of work.
-    spin_once_impl(timeout_left);
-
-    // Check if the future is set, return SUCCESS if it is.
-    status = wait_for_future(std::chrono::seconds(0));
-    if (status == std::future_status::ready) {
-      return FutureReturnCode::SUCCESS;
-    }
-    // If the original timeout is < 0, then this is blocking, never TIMEOUT.
-    if (timeout_ns < std::chrono::nanoseconds::zero()) {
-      continue;
-    }
-    // Otherwise check if we still have time to wait, return TIMEOUT if not.
-    auto now = std::chrono::steady_clock::now();
-    if (now >= end_time) {
-      return FutureReturnCode::TIMEOUT;
-    }
-    // Subtract the elapsed time from the original timeout.
-    timeout_left = std::chrono::duration_cast<std::chrono::nanoseconds>(end_time - now);
-  }
-
-  // The future did not complete before ok() returned false, return INTERRUPTED.
-  return FutureReturnCode::INTERRUPTED;
-}
-
 void
 Executor::spin_node_some(rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node)
 {
@@ -366,52 +304,24 @@ Executor::spin_some_impl(std::chrono::nanoseconds max_duration, bool exhaustive)
   }
   RCPPUTILS_SCOPE_EXIT(this->spinning.store(false); );
 
-  // clear the wait result and wait for work without blocking to collect the work
-  // for the first time
-  // both spin_some and spin_all wait for work at the beginning
-  wait_result_.reset();
-  wait_for_work(std::chrono::milliseconds(0));
-  bool just_waited = true;
-
-  // The logic of this while loop is as follows:
-  //
-  // - while not shutdown, and spinning (not canceled), and not max duration reached...
-  // - try to get an executable item to execute, and execute it if available
-  // - otherwise, reset the wait result, and ...
-  // - if there was no work available just after waiting, break the loop unconditionally
-  //   - this is appropriate for both spin_some and spin_all which use this function
-  // - else if exhaustive = true, then wait for work again
-  //   - this is only used for spin_all and not spin_some
-  // - else break
-  //   - this only occurs with spin_some
-  //
-  // The logic of this loop is subtle and should be carefully changed if at all.
-  // See also:
-  //   https://github.com/ros2/rclcpp/issues/2508
-  //   https://github.com/ros2/rclcpp/pull/2517
   while (rclcpp::ok(context_) && spinning.load() && max_duration_not_elapsed()) {
+    if (!wait_result_.has_value()) {
+      wait_for_work(std::chrono::milliseconds(0));
+    }
+
     AnyExecutable any_exec;
     if (get_next_ready_executable(any_exec)) {
       execute_any_executable(any_exec);
-      just_waited = false;
     } else {
-      // if nothing is ready, reset the result to clear it
+      // If nothing is ready, reset the result to signal we are
+      // ready to wait again
       wait_result_.reset();
+    }
 
-      if (just_waited) {
-        // there was no work after just waiting, always exit in this case
-        // before the exhaustive condition can be checked
-        break;
-      }
-
-      if (exhaustive) {
-        // if exhaustive, wait for work again
-        // this only happens for spin_all; spin_some only waits at the start
-        wait_for_work(std::chrono::milliseconds(0));
-        just_waited = true;
-      } else {
-        break;
-      }
+    if (!wait_result_.has_value() && !exhaustive) {
+      // In the case of spin some, then we can exit
+      // In the case of spin all, then we will allow ourselves to wait again.
+      break;
     }
   }
 }
@@ -485,6 +395,69 @@ Executor::execute_any_executable(AnyExecutable & any_exec)
   any_exec.callback_group->can_be_taken_from().store(true);
 }
 
+template<typename Function>
+void
+execute_guarded(
+  const Function & function,
+  const std::function<void(const std::exception & e)> & exception_handler)
+{
+  try {
+    function();
+  } catch (const std::exception & e) {
+    RCLCPP_ERROR_STREAM(
+      rclcpp::get_logger("rclcpp"),
+      "Exception while spinning: " << e.what());
+
+    exception_handler(e);
+  }
+}
+
+void
+Executor::execute_any_executable(
+  AnyExecutable & any_exec,
+  const std::function<void(const std::exception & e)> & exception_handler)
+{
+  if (!spinning.load()) {
+    return;
+  }
+
+  assert(
+    (void("cannot execute an AnyExecutable without a valid callback group"),
+    any_exec.callback_group));
+
+  if (any_exec.timer) {
+    TRACETOOLS_TRACEPOINT(
+      rclcpp_executor_execute,
+      static_cast<const void *>(any_exec.timer->get_timer_handle().get()));
+    execute_guarded([&any_exec]() {
+        execute_timer(any_exec.timer, any_exec.data);
+      }, exception_handler);
+  }
+  if (any_exec.subscription) {
+    TRACETOOLS_TRACEPOINT(
+      rclcpp_executor_execute,
+      static_cast<const void *>(any_exec.subscription->get_subscription_handle().get()));
+    execute_guarded(
+      [&any_exec]() {
+        execute_subscription(any_exec.subscription);
+      }, exception_handler);
+  }
+  if (any_exec.service) {
+    execute_guarded([&any_exec]() {execute_service(any_exec.service);}, exception_handler);
+  }
+  if (any_exec.client) {
+    execute_guarded([&any_exec]() {execute_client(any_exec.client);}, exception_handler);
+  }
+  if (any_exec.waitable) {
+    execute_guarded([&any_exec]() {
+        const std::shared_ptr<void> & const_data = any_exec.data;
+        any_exec.waitable->execute(const_data);
+    }, exception_handler);
+  }
+  // Reset the callback_group, regardless of type
+  any_exec.callback_group->can_be_taken_from().store(true);
+}
+
 template<typename Taker, typename Handler>
 static
 void

rclcpp/src/rclcpp/executor_options.cpp

@@ -1,55 +0,0 @@
-// Copyright 2024 Open Source Robotics Foundation, Inc.
-//
-// 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 "rclcpp/executor_options.hpp"
-
-using rclcpp::ExecutorOptions;
-
-namespace rclcpp
-{
-
-class ExecutorOptionsImplementation {};
-
-}  // namespace rclcpp
-
-ExecutorOptions::ExecutorOptions()
-: memory_strategy(rclcpp::memory_strategies::create_default_strategy()),
-  context(rclcpp::contexts::get_global_default_context()),
-  max_conditions(0),
-  impl_(nullptr)
-{}
-
-ExecutorOptions::~ExecutorOptions()
-{}
-
-ExecutorOptions::ExecutorOptions(const ExecutorOptions & other)
-{
-  *this = other;
-}
-
-ExecutorOptions & ExecutorOptions::operator=(const ExecutorOptions & other)
-{
-  if (this == &other) {
-    return *this;
-  }
-
-  this->memory_strategy = other.memory_strategy;
-  this->context = other.context;
-  this->max_conditions = other.max_conditions;
-  if (nullptr != other.impl_) {
-    this->impl_ = std::make_unique<ExecutorOptionsImplementation>(*other.impl_);
-  }
-
-  return *this;
-}

rclcpp/src/rclcpp/executors/multi_threaded_executor.cpp

@@ -51,6 +51,12 @@ MultiThreadedExecutor::~MultiThreadedExecutor() {}
 
 void
 MultiThreadedExecutor::spin()
+{
+  spin([](const std::exception & e) {throw e;});
+}
+
+void
+MultiThreadedExecutor::spin(const std::function<void(const std::exception & e)> & exception_handler)
 {
   if (spinning.exchange(true)) {
     throw std::runtime_error("spin() called while already spinning");
@@ -61,12 +67,12 @@ MultiThreadedExecutor::spin()
   {
     std::lock_guard wait_lock{wait_mutex_};
     for (; thread_id < number_of_threads_ - 1; ++thread_id) {
-      auto func = std::bind(&MultiThreadedExecutor::run, this, thread_id);
+      auto func = std::bind(&MultiThreadedExecutor::run, this, thread_id, exception_handler);
       threads.emplace_back(func);
     }
   }
 
-  run(thread_id);
+  run(thread_id, exception_handler);
   for (auto & thread : threads) {
     thread.join();
   }
@@ -79,7 +85,9 @@ MultiThreadedExecutor::get_number_of_threads()
 }
 
 void
-MultiThreadedExecutor::run(size_t this_thread_number)
+MultiThreadedExecutor::run(
+  size_t this_thread_number,
+  const std::function<void(const std::exception & e)> & exception_handler)
 {
   (void)this_thread_number;
   while (rclcpp::ok(this->context_) && spinning.load()) {
@@ -97,7 +105,7 @@ MultiThreadedExecutor::run(size_t this_thread_number)
       std::this_thread::yield();
     }
 
-    execute_any_executable(any_exec);
+    execute_any_executable(any_exec, exception_handler);
 
     if (any_exec.callback_group &&
       any_exec.callback_group->type() == CallbackGroupType::MutuallyExclusive)

rclcpp/src/rclcpp/executors/single_threaded_executor.cpp

@@ -26,6 +26,13 @@ SingleThreadedExecutor::~SingleThreadedExecutor() {}
 
 void
 SingleThreadedExecutor::spin()
+{
+  spin([](const std::exception & e) {throw e;});
+}
+
+void
+SingleThreadedExecutor::spin(
+  const std::function<void(const std::exception &)> & exception_handler)
 {
   if (spinning.exchange(true)) {
     throw std::runtime_error("spin() called while already spinning");
@@ -39,7 +46,7 @@ SingleThreadedExecutor::spin()
   while (rclcpp::ok(this->context_) && spinning.load()) {
     rclcpp::AnyExecutable any_executable;
     if (get_next_executable(any_executable)) {
-      execute_any_executable(any_executable);
+      execute_any_executable(any_executable, exception_handler);
     }
   }
 }

rclcpp/src/rclcpp/executors/static_single_threaded_executor.cpp

@@ -29,6 +29,14 @@ StaticSingleThreadedExecutor::~StaticSingleThreadedExecutor() {}
 
 void
 StaticSingleThreadedExecutor::spin()
+{
+  spin([](const std::exception & e) {throw e;});
+}
+
+
+void
+StaticSingleThreadedExecutor::spin(
+  const std::function<void(const std::exception & e)> & exception_handler)
 {
   if (spinning.exchange(true)) {
     throw std::runtime_error("spin() called while already spinning");
@@ -39,7 +47,7 @@ StaticSingleThreadedExecutor::spin()
   // except we need to keep the wait result to reproduce the StaticSingleThreadedExecutor
   // behavior.
   while (rclcpp::ok(this->context_) && spinning.load()) {
-    this->spin_once_impl(std::chrono::nanoseconds(-1));
+    this->spin_once_impl(std::chrono::nanoseconds(-1), exception_handler);
   }
 }
 
@@ -97,12 +105,32 @@ StaticSingleThreadedExecutor::spin_some_impl(std::chrono::nanoseconds max_durati
 
 void
 StaticSingleThreadedExecutor::spin_once_impl(std::chrono::nanoseconds timeout)
+{
+  spin_once_impl(timeout, std::function<void(const std::exception & e)>());
+}
+
+void
+StaticSingleThreadedExecutor::spin_once_impl(
+  std::chrono::nanoseconds timeout,
+  const std::function<void(const std::exception & e)> & exception_handler)
 {
   if (rclcpp::ok(context_) && spinning.load()) {
     std::lock_guard<std::mutex> guard(mutex_);
     auto wait_result = this->collect_and_wait(timeout);
     if (wait_result.has_value()) {
-      this->execute_ready_executables(current_collection_, wait_result.value(), true);
+      if(exception_handler) {
+        try {
+          this->execute_ready_executables(current_collection_, wait_result.value(), true);
+        } catch (const std::exception & e) {
+          RCLCPP_ERROR_STREAM(
+            rclcpp::get_logger("rclcpp"),
+            "Exception while spinning : " << e.what());
+
+          exception_handler(e);
+        }
+      } else {
+        this->execute_ready_executables(current_collection_, wait_result.value(), true);
+      }
     }
   }
 }

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

@@ -126,6 +126,35 @@ EventsExecutor::spin()
   }
 }
 
+void
+EventsExecutor::spin(const std::function<void(const std::exception & e)> & exception_handler)
+{
+  if (spinning.exchange(true)) {
+    throw std::runtime_error("spin() called while already spinning");
+  }
+  RCPPUTILS_SCOPE_EXIT(this->spinning.store(false); );
+
+  timers_manager_->start(exception_handler);
+  RCPPUTILS_SCOPE_EXIT(timers_manager_->stop(); );
+
+  while (rclcpp::ok(context_) && spinning.load()) {
+    // Wait until we get an event
+    ExecutorEvent event;
+    bool has_event = events_queue_->dequeue(event);
+    if (has_event) {
+      try {
+        this->execute_event(event);
+      } catch (const std::exception & e) {
+        RCLCPP_ERROR_STREAM(
+          rclcpp::get_logger("rclcpp"),
+          "Exception while spinning : " << e.what());
+
+        exception_handler(e);
+      }
+    }
+  }
+}
+
 void
 EventsExecutor::spin_some(std::chrono::nanoseconds max_duration)
 {

rclcpp/src/rclcpp/experimental/timers_manager.cpp

@@ -20,6 +20,7 @@
 #include <iostream>
 #include <memory>
 #include <stdexcept>
+#include <rclcpp/logging.hpp>
 
 #include "rcpputils/scope_exit.hpp"
 
@@ -71,14 +72,17 @@ void TimersManager::add_timer(rclcpp::TimerBase::SharedPtr timer)
   }
 }
 
-void TimersManager::start()
+void TimersManager::start(const std::function<void(const std::exception & e)> & exception_handler)
 {
   // Make sure that the thread is not already running
   if (running_.exchange(true)) {
     throw std::runtime_error("TimersManager::start() can't start timers thread as already running");
   }
 
-  timers_thread_ = std::thread(&TimersManager::run_timers, this);
+  timers_thread_ = std::thread(
+    [this, exception_handler]() {
+      run_timers(exception_handler);
+    });
 }
 
 void TimersManager::stop()
@@ -203,7 +207,8 @@ std::optional<std::chrono::nanoseconds> TimersManager::get_head_timeout_unsafe()
   return head_timer->time_until_trigger();
 }
 
-void TimersManager::execute_ready_timers_unsafe()
+void TimersManager::execute_ready_timers_unsafe(
+  std::function<void(const std::exception & e)> exception_handler)
 {
   // We start by locking the timers
   TimersHeap locked_heap = weak_timers_heap_.validate_and_lock();
@@ -226,7 +231,19 @@ void TimersManager::execute_ready_timers_unsafe()
       if (on_ready_callback_) {
         on_ready_callback_(head_timer.get(), data);
       } else {
-        head_timer->execute_callback(data);
+        if (exception_handler) {
+          try {
+            head_timer->execute_callback(data);
+          } catch (const std::exception & e) {
+            RCLCPP_ERROR_STREAM(
+              rclcpp::get_logger("rclcpp"),
+              "Exception while spinning : " << e.what());
+
+            exception_handler(e);
+          }
+        } else {
+          head_timer->execute_callback(data);
+        }
       }
     } else {
       // someone canceled the timer between is_ready and call
@@ -245,7 +262,8 @@ void TimersManager::execute_ready_timers_unsafe()
   weak_timers_heap_.store(locked_heap);
 }
 
-void TimersManager::run_timers()
+void TimersManager::run_timers(
+  const std::function<void(const std::exception & e)> & exception_handler)
 {
   // Make sure the running flag is set to false when we exit from this function
   // to allow restarting the timers thread.
@@ -289,7 +307,7 @@ void TimersManager::run_timers()
     timers_updated_ = false;
 
     // Execute timers
-    this->execute_ready_timers_unsafe();
+    this->execute_ready_timers_unsafe(exception_handler);
   }
 }
 

rclcpp/src/rclcpp/time.cpp

@@ -60,10 +60,6 @@ Time::Time(int32_t seconds, uint32_t nanoseconds, rcl_clock_type_t clock_type)
 Time::Time(int64_t nanoseconds, rcl_clock_type_t clock_type)
 : rcl_time_(init_time_point(clock_type))
 {
-  if (nanoseconds < 0) {
-    throw std::runtime_error("cannot store a negative time point in rclcpp::Time");
-  }
-
   rcl_time_.nanoseconds = nanoseconds;
 }
 
@@ -253,9 +249,6 @@ Time::operator+=(const rclcpp::Duration & rhs)
   }
 
   rcl_time_.nanoseconds += rhs.nanoseconds();
-  if (rcl_time_.nanoseconds < 0) {
-    throw std::runtime_error("cannot store a negative time point in rclcpp::Time");
-  }
 
   return *this;
 }
@@ -271,9 +264,6 @@ Time::operator-=(const rclcpp::Duration & rhs)
   }
 
   rcl_time_.nanoseconds -= rhs.nanoseconds();
-  if (rcl_time_.nanoseconds < 0) {
-    throw std::runtime_error("cannot store a negative time point in rclcpp::Time");
-  }
 
   return *this;
 }

rclcpp/test/benchmark/benchmark_parameter_client.cpp

@@ -46,7 +46,10 @@ public:
       remote_node_name, rclcpp::NodeOptions().context(remote_context));
     remote_executor->add_node(remote_node);
 
-    remote_thread = std::thread(&rclcpp::executors::SingleThreadedExecutor::spin, remote_executor);
+    remote_thread =
+      std::thread(
+      static_cast<void (rclcpp::executors::SingleThreadedExecutor::*)()>(&rclcpp::
+      executors::SingleThreadedExecutor::spin), remote_executor);
   }
 
   void TearDown(benchmark::State &)

rclcpp/test/rclcpp/CMakeLists.txt

@@ -31,7 +31,6 @@ endif()
 ament_add_gtest(
   test_exceptions
   exceptions/test_exceptions.cpp)
-ament_add_test_label(test_exceptions mimick)
 if(TARGET test_exceptions)
   target_link_libraries(test_exceptions ${PROJECT_NAME} mimick)
 endif()
@@ -53,15 +52,9 @@ if(TARGET test_any_subscription_callback)
   target_link_libraries(test_any_subscription_callback ${PROJECT_NAME} ${test_msgs_TARGETS})
 endif()
 ament_add_gtest(test_client test_client.cpp)
-ament_add_test_label(test_client mimick)
 if(TARGET test_client)
   target_link_libraries(test_client ${PROJECT_NAME} mimick ${rcl_interfaces_TARGETS} ${test_msgs_TARGETS})
 endif()
-ament_add_gtest(test_clock test_clock.cpp)
-ament_add_test_label(test_clock mimick)
-if(TARGET test_clock)
-  target_link_libraries(test_clock ${PROJECT_NAME} mimick ${rcl_interfaces_TARGETS} ${test_msgs_TARGETS})
-endif()
 ament_add_gtest(test_copy_all_parameter_values test_copy_all_parameter_values.cpp)
 if(TARGET test_copy_all_parameter_values)
   target_link_libraries(test_copy_all_parameter_values ${PROJECT_NAME})
@@ -72,7 +65,6 @@ if(TARGET test_create_timer)
   target_include_directories(test_create_timer PRIVATE ./)
 endif()
 ament_add_gtest(test_generic_client test_generic_client.cpp)
-ament_add_test_label(test_generic_client mimick)
 if(TARGET test_generic_client)
   target_link_libraries(test_generic_client ${PROJECT_NAME}
     mimick
@@ -84,7 +76,6 @@ if(TARGET test_generic_client)
   )
 endif()
 ament_add_gtest(test_client_common test_client_common.cpp)
-ament_add_test_label(test_client_common mimick)
 if(TARGET test_client_common)
   target_link_libraries(test_client_common ${PROJECT_NAME}
     mimick
@@ -111,7 +102,6 @@ function(test_add_callback_groups_to_executor_for_rmw_implementation)
 endfunction()
 call_for_each_rmw_implementation(test_add_callback_groups_to_executor_for_rmw_implementation)
 ament_add_gtest(test_expand_topic_or_service_name test_expand_topic_or_service_name.cpp)
-ament_add_test_label(test_expand_topic_or_service_name mimick)
 if(TARGET test_expand_topic_or_service_name)
   target_link_libraries(test_expand_topic_or_service_name ${PROJECT_NAME} mimick rcl::rcl rmw::rmw)
 endif()
@@ -143,7 +133,6 @@ if(TARGET test_intra_process_buffer)
 endif()
 
 ament_add_gtest(test_loaned_message test_loaned_message.cpp)
-ament_add_test_label(test_loaned_message mimick)
 target_link_libraries(test_loaned_message ${PROJECT_NAME} mimick ${test_msgs_TARGETS})
 
 ament_add_gtest(test_memory_strategy test_memory_strategy.cpp)
@@ -153,7 +142,6 @@ ament_add_gtest(test_message_memory_strategy test_message_memory_strategy.cpp)
 target_link_libraries(test_message_memory_strategy ${PROJECT_NAME} ${test_msgs_TARGETS})
 
 ament_add_gtest(test_node test_node.cpp TIMEOUT 240)
-ament_add_test_label(test_node mimick)
 if(TARGET test_node)
   target_link_libraries(test_node ${PROJECT_NAME} mimick rcpputils::rcpputils rmw::rmw ${test_msgs_TARGETS})
 endif()
@@ -165,7 +153,6 @@ if(TARGET test_node_interfaces__get_node_interfaces)
 endif()
 ament_add_gtest(test_node_interfaces__node_base
   node_interfaces/test_node_base.cpp)
-ament_add_test_label(test_node_interfaces__node_base mimick)
 if(TARGET test_node_interfaces__node_base)
   target_link_libraries(test_node_interfaces__node_base ${PROJECT_NAME} mimick rcl::rcl rmw::rmw)
 endif()
@@ -177,7 +164,6 @@ endif()
 ament_add_gtest(test_node_interfaces__node_graph
   node_interfaces/test_node_graph.cpp
   TIMEOUT 120)
-ament_add_test_label(test_node_interfaces__node_graph mimick)
 if(TARGET test_node_interfaces__node_graph)
   target_link_libraries(test_node_interfaces__node_graph ${PROJECT_NAME} mimick rcl::rcl ${test_msgs_TARGETS})
 endif()
@@ -188,25 +174,21 @@ if(TARGET test_node_interfaces__node_interfaces)
 endif()
 ament_add_gtest(test_node_interfaces__node_parameters
   node_interfaces/test_node_parameters.cpp)
-ament_add_test_label(test_node_interfaces__node_parameters mimick)
 if(TARGET test_node_interfaces__node_parameters)
   target_link_libraries(test_node_interfaces__node_parameters ${PROJECT_NAME} mimick rcpputils::rcpputils)
 endif()
 ament_add_gtest(test_node_interfaces__node_services
   node_interfaces/test_node_services.cpp)
-ament_add_test_label(test_node_interfaces__node_services mimick)
 if(TARGET test_node_interfaces__node_services)
   target_link_libraries(test_node_interfaces__node_services ${PROJECT_NAME} mimick rcl::rcl)
 endif()
 ament_add_gtest(test_node_interfaces__node_timers
   node_interfaces/test_node_timers.cpp)
-ament_add_test_label(test_node_interfaces__node_timers mimick)
 if(TARGET test_node_interfaces__node_timers)
   target_link_libraries(test_node_interfaces__node_timers ${PROJECT_NAME} mimick rcl::rcl)
 endif()
 ament_add_gtest(test_node_interfaces__node_topics
   node_interfaces/test_node_topics.cpp)
-ament_add_test_label(test_node_interfaces__node_topics mimick)
 if(TARGET test_node_interfaces__node_topics)
   target_link_libraries(test_node_interfaces__node_topics ${PROJECT_NAME} mimick rcl::rcl ${test_msgs_TARGETS})
 endif()
@@ -217,7 +199,6 @@ if(TARGET test_node_interfaces__node_type_descriptions)
 endif()
 ament_add_gtest(test_node_interfaces__node_waitables
   node_interfaces/test_node_waitables.cpp)
-ament_add_test_label(test_node_interfaces__node_waitables mimick)
 if(TARGET test_node_interfaces__node_waitables)
   target_link_libraries(test_node_interfaces__node_waitables ${PROJECT_NAME} mimick rcl::rcl)
 endif()
@@ -253,12 +234,10 @@ if(TARGET test_node_global_args)
   target_link_libraries(test_node_global_args ${PROJECT_NAME})
 endif()
 ament_add_gtest(test_node_options test_node_options.cpp)
-ament_add_test_label(test_node_options mimick)
 if(TARGET test_node_options)
   target_link_libraries(test_node_options ${PROJECT_NAME} mimick rcl::rcl)
 endif()
 ament_add_gtest(test_init_options test_init_options.cpp)
-ament_add_test_label(test_init_options mimick)
 if(TARGET test_init_options)
   target_link_libraries(test_init_options ${PROJECT_NAME} mimick rcl::rcl)
 endif()
@@ -287,7 +266,6 @@ if(TARGET test_parameter_map)
   target_link_libraries(test_parameter_map ${PROJECT_NAME} rcl::rcl rcl_yaml_param_parser::rcl_yaml_param_parser rcutils::rcutils)
 endif()
 ament_add_gtest(test_publisher test_publisher.cpp TIMEOUT 120)
-ament_add_test_label(test_publisher mimick)
 if(TARGET test_publisher)
   target_link_libraries(test_publisher ${PROJECT_NAME} mimick rcl::rcl rcutils::rcutils ${test_msgs_TARGETS})
 endif()
@@ -353,7 +331,6 @@ function(test_qos_event_for_rmw_implementation)
   ament_add_gmock(test_qos_event${target_suffix} test_qos_event.cpp
     ENV ${rmw_implementation_env_var}
   )
-  ament_add_test_label(test_qos_event${target_suffix} mimick)
   if(TARGET test_qos_event${target_suffix})
     target_link_libraries(test_qos_event${target_suffix} ${PROJECT_NAME} mimick rcutils::rcutils rmw::rmw ${test_msgs_TARGETS})
   endif()
@@ -381,18 +358,15 @@ if(TARGET test_serialized_message)
   target_link_libraries(test_serialized_message ${PROJECT_NAME} rcpputils::rcpputils ${test_msgs_TARGETS})
 endif()
 ament_add_gtest(test_service test_service.cpp)
-ament_add_test_label(test_service mimick)
 if(TARGET test_service)
   target_link_libraries(test_service ${PROJECT_NAME} mimick ${rcl_interfaces_TARGES} ${test_msgs_TARGETS})
 endif()
 ament_add_gmock(test_service_introspection test_service_introspection.cpp)
-ament_add_test_label(test_service_introspection mimick)
 if(TARGET test_service_introspection)
   target_link_libraries(test_service_introspection ${PROJECT_NAME} mimick ${service_msgs_TARGETS} ${test_msgs_TARGETS})
 endif()
 # Creating and destroying nodes is slow with Connext, so this needs larger timeout.
 ament_add_gtest(test_subscription test_subscription.cpp TIMEOUT 120)
-ament_add_test_label(test_subscription mimick)
 if(TARGET test_subscription)
   target_link_libraries(test_subscription ${PROJECT_NAME} mimick ${test_msgs_TARGETS})
 endif()
@@ -443,7 +417,6 @@ endif()
 
 ament_add_gtest(test_timer test_timer.cpp
   APPEND_LIBRARY_DIRS "${append_library_dirs}")
-ament_add_test_label(test_timer mimick)
 if(TARGET test_timer)
   target_link_libraries(test_timer ${PROJECT_NAME} mimick rcl::rcl)
 endif()
@@ -462,7 +435,6 @@ endif()
 
 ament_add_gtest(test_utilities test_utilities.cpp
   APPEND_LIBRARY_DIRS "${append_library_dirs}")
-ament_add_test_label(test_utilities mimick)
 if(TARGET test_utilities)
   target_link_libraries(test_utilities ${PROJECT_NAME} mimick rcl::rcl)
 endif()
@@ -521,7 +493,6 @@ endif()
 
 ament_add_gtest(test_static_single_threaded_executor executors/test_static_single_threaded_executor.cpp
   APPEND_LIBRARY_DIRS "${append_library_dirs}")
-ament_add_test_label(test_static_single_threaded_executor mimick)
 if(TARGET test_static_single_threaded_executor)
   target_link_libraries(test_static_single_threaded_executor ${PROJECT_NAME} mimick ${test_msgs_TARGETS})
 endif()
@@ -540,7 +511,6 @@ endif()
 
 ament_add_gtest(test_executor_notify_waitable executors/test_executor_notify_waitable.cpp
   APPEND_LIBRARY_DIRS "${append_library_dirs}" TIMEOUT 120)
-ament_add_test_label(test_executor_notify_waitable mimick)
 if(TARGET test_executor_notify_waitable)
   target_link_libraries(test_executor_notify_waitable ${PROJECT_NAME} mimick rcpputils::rcpputils)
 endif()
@@ -558,7 +528,6 @@ endif()
 
 ament_add_gtest(test_guard_condition test_guard_condition.cpp
   APPEND_LIBRARY_DIRS "${append_library_dirs}")
-ament_add_test_label(test_guard_condition mimick)
 if(TARGET test_guard_condition)
   target_link_libraries(test_guard_condition ${PROJECT_NAME} mimick)
 endif()
@@ -592,7 +561,6 @@ if(TARGET test_dynamic_storage)
 endif()
 
 ament_add_gtest(test_storage_policy_common wait_set_policies/test_storage_policy_common.cpp)
-ament_add_test_label(test_storage_policy_common mimick)
 if(TARGET test_storage_policy_common)
   target_link_libraries(test_storage_policy_common ${PROJECT_NAME} mimick ${test_msgs_TARGETS})
 endif()
@@ -625,13 +593,11 @@ endif()
 ament_add_gtest(test_executor test_executor.cpp
   APPEND_LIBRARY_DIRS "${append_library_dirs}"
   TIMEOUT 120)
-ament_add_test_label(test_executor mimick)
 if(TARGET test_executor)
   target_link_libraries(test_executor ${PROJECT_NAME} mimick)
 endif()
 
 ament_add_gtest(test_graph_listener test_graph_listener.cpp)
-ament_add_test_label(test_graph_listener mimick)
 if(TARGET test_graph_listener)
   target_link_libraries(test_graph_listener ${PROJECT_NAME} mimick)
 endif()
@@ -643,7 +609,6 @@ function(test_subscription_content_filter_for_rmw_implementation)
     ENV ${rmw_implementation_env_var}
     TIMEOUT 120
   )
-  ament_add_test_label(test_subscription_content_filter${target_suffix} mimick)
   if(TARGET test_subscription_content_filter${target_suffix})
     target_link_libraries(test_subscription_content_filter${target_suffix} ${PROJECT_NAME} mimick ${test_msgs_TARGETS})
   endif()

rclcpp/test/rclcpp/executors/test_executors.cpp

@@ -133,6 +133,52 @@ TYPED_TEST(TestExecutors, emptyExecutor)
   spinner.join();
 }
 
+TYPED_TEST(TestExecutors, catch_exception) {
+  using ExecutorType = TypeParam;
+  ExecutorType executor;
+
+  std::shared_ptr<rclcpp::Node> node =
+    std::make_shared<rclcpp::Node>("test_executor_catch_exception");
+
+  const std::string test_reason = "test exception";
+
+  std::atomic_bool timer_executed_after_exception = false;
+
+  size_t cnt = 0;
+
+  auto timer = node->create_wall_timer(
+    std::chrono::milliseconds(1), [test_reason, &timer_executed_after_exception, &executor, &cnt]()
+    {
+      if (cnt == 0) {
+        cnt++;
+        throw std::runtime_error(test_reason);
+      }
+
+      timer_executed_after_exception = true;
+
+      executor.cancel();
+    });
+
+  std::atomic_bool caught_exception = false;
+
+  executor.add_node(node);
+  executor.spin(
+    [&caught_exception, &test_reason](const std::exception & e)
+    {
+      const std::runtime_error * runtime_error = dynamic_cast<const std::runtime_error *>(&e);
+      ASSERT_NE(runtime_error, nullptr);
+
+      ASSERT_EQ(runtime_error->what(), test_reason);
+
+      caught_exception = true;
+    }
+  );
+
+  ASSERT_TRUE(caught_exception);
+  ASSERT_TRUE(timer_executed_after_exception);
+}
+
+
 // Check executor throws properly if the same node is added a second time
 TYPED_TEST(TestExecutors, addNodeTwoExecutors)
 {
@@ -357,7 +403,6 @@ public:
   bool
   is_ready(const rcl_wait_set_t & wait_set) override
   {
-    is_ready_count_++;
     for (size_t i = 0; i < wait_set.size_of_guard_conditions; ++i) {
       auto rcl_guard_condition = wait_set.guard_conditions[i];
       if (&gc_.get_rcl_guard_condition() == rcl_guard_condition) {
@@ -425,15 +470,8 @@ public:
     return count_;
   }
 
-  size_t
-  get_is_ready_call_count() const
-  {
-    return is_ready_count_;
-  }
-
 private:
   std::atomic<size_t> trigger_count_ = 0;
-  std::atomic<size_t> is_ready_count_ = 0;
   std::atomic<size_t> count_ = 0;
   rclcpp::GuardCondition gc_;
   std::function<void()> on_execute_callback_ = nullptr;
@@ -877,155 +915,3 @@ TEST(TestExecutors, testSpinWithNonDefaultContext)
 
   rclcpp::shutdown(non_default_context);
 }
-
-template<typename T>
-class TestBusyWaiting : public ::testing::Test
-{
-public:
-  void SetUp() override
-  {
-    rclcpp::init(0, nullptr);
-
-    const auto test_info = ::testing::UnitTest::GetInstance()->current_test_info();
-    std::stringstream test_name;
-    test_name << test_info->test_case_name() << "_" << test_info->name();
-    node = std::make_shared<rclcpp::Node>("node", test_name.str());
-    callback_group = node->create_callback_group(
-      rclcpp::CallbackGroupType::MutuallyExclusive,
-      /* automatically_add_to_executor_with_node =*/ false);
-
-    auto waitable_interfaces = node->get_node_waitables_interface();
-    waitable = std::make_shared<TestWaitable>();
-    waitable_interfaces->add_waitable(waitable, callback_group);
-
-    executor = std::make_shared<T>();
-    executor->add_callback_group(callback_group, node->get_node_base_interface());
-  }
-
-  void TearDown() override
-  {
-    rclcpp::shutdown();
-  }
-
-  void
-  set_up_and_trigger_waitable(std::function<void()> extra_callback = nullptr)
-  {
-    this->has_executed = false;
-    this->waitable->set_on_execute_callback([this, extra_callback]() {
-        if (!this->has_executed) {
-        // trigger once to see if the second trigger is handled or not
-        // this follow up trigger simulates new entities becoming ready while
-        // the executor is executing something else, e.g. subscription got data
-        // or a timer expired, etc.
-        // spin_some would not handle this second trigger, since it collects
-        // work only once, whereas spin_all should handle it since it
-        // collects work multiple times
-          this->waitable->trigger();
-          this->has_executed = true;
-        }
-        if (nullptr != extra_callback) {
-          extra_callback();
-        }
-    });
-    this->waitable->trigger();
-  }
-
-  void
-  check_for_busy_waits(std::chrono::steady_clock::time_point start_time)
-  {
-    // rough time based check, since the work to be done was very small it
-    // should be safe to check that we didn't use more than half the
-    // max duration, which itself is much larger than necessary
-    // however, it could still produce a false-positive
-    EXPECT_LT(
-      std::chrono::steady_clock::now() - start_time,
-      max_duration / 2)
-      << "executor took a long time to execute when it should have done "
-      << "nothing and should not have blocked either, but this could be a "
-      << "false negative if the computer is really slow";
-
-    // this check is making some assumptions about the implementation of the
-    // executors, but it should be safe to say that a busy wait may result in
-    // hundreds or thousands of calls to is_ready(), but "normal" executor
-    // behavior should be within an order of magnitude of the number of
-    // times that the waitable was executed
-    ASSERT_LT(waitable->get_is_ready_call_count(), 10u * this->waitable->get_count());
-  }
-
-  static constexpr auto max_duration = 10s;
-
-  rclcpp::Node::SharedPtr node;
-  rclcpp::CallbackGroup::SharedPtr callback_group;
-  std::shared_ptr<TestWaitable> waitable;
-  std::chrono::steady_clock::time_point start_time;
-  std::shared_ptr<T> executor;
-  bool has_executed;
-};
-
-TYPED_TEST_SUITE(TestBusyWaiting, ExecutorTypes, ExecutorTypeNames);
-
-TYPED_TEST(TestBusyWaiting, test_spin_all)
-{
-  this->set_up_and_trigger_waitable();
-
-  auto start_time = std::chrono::steady_clock::now();
-  this->executor->spin_all(this->max_duration);
-  this->check_for_busy_waits(start_time);
-  // this should get the initial trigger, and the follow up from in the callback
-  ASSERT_EQ(this->waitable->get_count(), 2u);
-}
-
-TYPED_TEST(TestBusyWaiting, test_spin_some)
-{
-  this->set_up_and_trigger_waitable();
-
-  auto start_time = std::chrono::steady_clock::now();
-  this->executor->spin_some(this->max_duration);
-  this->check_for_busy_waits(start_time);
-  // this should get the inital trigger, but not the follow up in the callback
-  ASSERT_EQ(this->waitable->get_count(), 1u);
-}
-
-TYPED_TEST(TestBusyWaiting, test_spin)
-{
-  std::condition_variable cv;
-  std::mutex cv_m;
-  bool first_check_passed = false;
-
-  this->set_up_and_trigger_waitable([&cv, &cv_m, &first_check_passed]() {
-      cv.notify_one();
-      if (!first_check_passed) {
-        std::unique_lock<std::mutex> lk(cv_m);
-        cv.wait_for(lk, 1s, [&]() {return first_check_passed;});
-      }
-  });
-
-  auto start_time = std::chrono::steady_clock::now();
-  std::thread t([this]() {
-      this->executor->spin();
-    });
-
-  // wait until thread has started (first execute of waitable)
-  {
-    std::unique_lock<std::mutex> lk(cv_m);
-    cv.wait_for(lk, 10s);
-  }
-  EXPECT_GT(this->waitable->get_count(), 0u);
-
-  first_check_passed = true;
-  cv.notify_one();
-
-  // wait until the executor has finished (second execute of waitable)
-  {
-    std::unique_lock<std::mutex> lk(cv_m);
-    cv.wait_for(lk, 10s);
-  }
-  EXPECT_EQ(this->waitable->get_count(), 2u);
-
-  this->executor->cancel();
-  t.join();
-
-  this->check_for_busy_waits(start_time);
-  // this should get the initial trigger, and the follow up from in the callback
-  ASSERT_EQ(this->waitable->get_count(), 2u);
-}

rclcpp/test/rclcpp/executors/test_executors_callback_group_behavior.cpp

@@ -42,6 +42,8 @@ public:
 
   void spin() override {}
 
+  void spin(const std::function<void(const std::exception & e)> &) override {}
+
   void collect()
   {
     this->collect_entities();

rclcpp/test/rclcpp/test_clock.cpp

@@ -1,229 +0,0 @@
-// Copyright 2024 Cellumation GmbH
-//
-// 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 "rcl/error_handling.h"
-#include "rcl/time.h"
-#include "rclcpp/clock.hpp"
-#include "rclcpp/rclcpp.hpp"
-#include "rclcpp/time_source.hpp"
-
-#include "../utils/rclcpp_gtest_macros.hpp"
-
-using namespace std::chrono_literals;
-
-class TestClockWakeup : public ::testing::TestWithParam<rcl_clock_type_e>
-{
-public:
-  void test_wakeup_before_sleep(const rclcpp::Clock::SharedPtr & clock)
-  {
-    std::atomic_bool thread_finished = false;
-
-    std::thread wait_thread = std::thread(
-      [&clock, &thread_finished]()
-      {
-        // make sure the thread starts sleeping late
-        std::this_thread::sleep_for(std::chrono::milliseconds(100));
-        clock->sleep_until(clock->now() + std::chrono::seconds(3));
-        thread_finished = true;
-      });
-
-    // notify the clock, that the sleep shall be interrupted
-    clock->cancel_sleep_or_wait();
-
-    auto start_time = std::chrono::steady_clock::now();
-    auto cur_time = start_time;
-    while (!thread_finished && start_time + std::chrono::seconds(1) > cur_time) {
-      std::this_thread::sleep_for(std::chrono::milliseconds(10));
-      cur_time = std::chrono::steady_clock::now();
-    }
-
-    wait_thread.join();
-
-    EXPECT_TRUE(thread_finished);
-    EXPECT_LT(cur_time, start_time + std::chrono::seconds(1));
-  }
-
-  void test_wakeup_after_sleep(const rclcpp::Clock::SharedPtr & clock)
-  {
-    std::atomic_bool thread_finished = false;
-
-    std::thread wait_thread = std::thread(
-      [&clock, &thread_finished]()
-      {
-        clock->sleep_until(clock->now() + std::chrono::seconds(3));
-        thread_finished = true;
-      });
-
-    // make sure the thread is already sleeping before we send the cancel
-    std::this_thread::sleep_for(std::chrono::milliseconds(100));
-
-    // notify the clock, that the sleep shall be interrupted
-    clock->cancel_sleep_or_wait();
-
-    auto start_time = std::chrono::steady_clock::now();
-    auto cur_time = start_time;
-    while (!thread_finished && start_time + std::chrono::seconds(1) > cur_time) {
-      std::this_thread::sleep_for(std::chrono::milliseconds(10));
-      cur_time = std::chrono::steady_clock::now();
-    }
-
-    wait_thread.join();
-
-    EXPECT_TRUE(thread_finished);
-    EXPECT_LT(cur_time, start_time + std::chrono::seconds(1));
-  }
-
-protected:
-  static void SetUpTestCase()
-  {
-    rclcpp::init(0, nullptr);
-  }
-
-  static void TearDownTestCase()
-  {
-    rclcpp::shutdown();
-  }
-
-  void SetUp()
-  {
-    node = std::make_shared<rclcpp::Node>("my_node");
-  }
-
-  void TearDown()
-  {
-    node.reset();
-  }
-
-  rclcpp::Node::SharedPtr node;
-};
-
-INSTANTIATE_TEST_SUITE_P(
-  Clocks,
-  TestClockWakeup,
-  ::testing::Values(
-    RCL_SYSTEM_TIME, RCL_ROS_TIME, RCL_STEADY_TIME
-));
-
-TEST_P(TestClockWakeup, wakeup_sleep) {
-  auto clock = std::make_shared<rclcpp::Clock>(GetParam());
-  test_wakeup_after_sleep(clock);
-  test_wakeup_before_sleep(clock);
-}
-
-TEST_F(TestClockWakeup, wakeup_sleep_ros_time_active) {
-  node->set_parameter({"use_sim_time", true});
-  auto clock = std::make_shared<rclcpp::Clock>(RCL_ROS_TIME);
-  rclcpp::TimeSource time_source(node);
-  time_source.attachClock(clock);
-
-  EXPECT_TRUE(clock->ros_time_is_active());
-
-  test_wakeup_after_sleep(clock);
-  test_wakeup_before_sleep(clock);
-}
-
-TEST_F(TestClockWakeup, no_wakeup_on_sim_time) {
-  node->set_parameter({"use_sim_time", true});
-  auto clock = std::make_shared<rclcpp::Clock>(RCL_ROS_TIME);
-  rclcpp::TimeSource time_source(node);
-  time_source.attachClock(clock);
-
-  EXPECT_TRUE(clock->ros_time_is_active());
-
-  std::atomic_bool thread_finished = false;
-
-  std::thread wait_thread = std::thread(
-    [&clock, &thread_finished]()
-    {
-      // make sure the thread starts sleeping late
-      clock->sleep_until(clock->now() + std::chrono::milliseconds(10));
-      thread_finished = true;
-    });
-
-  // make sure, that the sim time clock does not wakeup, as no clock is provided
-  std::this_thread::sleep_for(std::chrono::milliseconds(500));
-  EXPECT_FALSE(thread_finished);
-
-  // notify the clock, that the sleep shall be interrupted
-  clock->cancel_sleep_or_wait();
-
-  auto start_time = std::chrono::steady_clock::now();
-  auto cur_time = start_time;
-  while (!thread_finished && start_time + std::chrono::seconds(1) > cur_time) {
-    std::this_thread::sleep_for(std::chrono::milliseconds(10));
-    cur_time = std::chrono::steady_clock::now();
-  }
-
-  wait_thread.join();
-
-  EXPECT_TRUE(thread_finished);
-  EXPECT_LT(cur_time, start_time + std::chrono::seconds(1));
-}
-
-TEST_F(TestClockWakeup, multiple_threads_wait_on_one_clock) {
-  auto clock = std::make_shared<rclcpp::Clock>(RCL_ROS_TIME);
-
-  std::vector<bool> thread_finished(10, false);
-
-  std::vector<std::thread> threads;
-
-  for (size_t nr = 0; nr < thread_finished.size(); nr++) {
-    threads.push_back(
-      std::thread(
-        [&clock, &thread_finished, nr]()
-        {
-          // make sure the thread starts sleeping late
-          clock->sleep_until(clock->now() + std::chrono::seconds(10));
-          thread_finished[nr] = true;
-        }));
-  }
-
-  // wait a bit so all threads can execute the sleep_until
-  std::this_thread::sleep_for(std::chrono::milliseconds(500));
-
-  for (const bool & finished : thread_finished) {
-    EXPECT_FALSE(finished);
-  }
-
-  rclcpp::shutdown();
-
-  auto start_time = std::chrono::steady_clock::now();
-  auto cur_time = start_time;
-  bool threads_finished = false;
-  while (!threads_finished && start_time + std::chrono::seconds(1) > cur_time) {
-    threads_finished = true;
-    for (const bool finished : thread_finished) {
-      if (!finished) {
-        threads_finished = false;
-      }
-    }
-
-    std::this_thread::sleep_for(std::chrono::milliseconds(10));
-    cur_time = std::chrono::steady_clock::now();
-  }
-
-  for (const bool finished : thread_finished) {
-    EXPECT_TRUE(finished);
-  }
-
-  for (auto & thread : threads) {
-    thread.join();
-  }
-
-  EXPECT_LT(cur_time, start_time + std::chrono::seconds(1));
-}

rclcpp/test/rclcpp/test_executor.cpp

@@ -42,6 +42,10 @@ public:
   {
   }
 
+  void spin(const std::function<void(const std::exception & e)> &) override
+  {
+  }
+
   void spin_nanoseconds(rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node)
   {
     spin_node_once_nanoseconds(node, std::chrono::milliseconds(100));

rclcpp/test/rclcpp/test_subscription_content_filter.cpp

@@ -310,25 +310,3 @@ TEST_F(CLASSNAME(TestContentFilterSubscription, RMW_IMPLEMENTATION), content_fil
     }
   }
 }
-
-TEST_F(
-  CLASSNAME(
-    TestContentFilterSubscription,
-    RMW_IMPLEMENTATION), create_two_content_filters_with_same_topic_name_and_destroy) {
-
-  // Create another content filter
-  auto options = rclcpp::SubscriptionOptions();
-
-  std::string filter_expression = "int32_value > %0";
-  std::vector<std::string> expression_parameters = {"4"};
-
-  options.content_filter_options.filter_expression = filter_expression;
-  options.content_filter_options.expression_parameters = expression_parameters;
-
-  auto callback = [](std::shared_ptr<const test_msgs::msg::BasicTypes>) {};
-  auto sub_2 = node->create_subscription<test_msgs::msg::BasicTypes>(
-    "content_filter_topic", qos, callback, options);
-
-  EXPECT_NE(nullptr, sub_2);
-  sub_2.reset();
-}

rclcpp/test/rclcpp/test_time.cpp

@@ -138,8 +138,6 @@ TEST_F(TestTime, conversions) {
 
     EXPECT_ANY_THROW(rclcpp::Time(-1, 1));
 
-    EXPECT_ANY_THROW(rclcpp::Time(-1));
-
     EXPECT_ANY_THROW(
     {
       rclcpp::Time assignment(1, 2);
@@ -170,6 +168,48 @@ TEST_F(TestTime, conversions) {
     EXPECT_EQ(time_msg.nanosec, HALF_SEC_IN_NS);
     EXPECT_EQ(rclcpp::Time(time_msg).nanoseconds(), ONE_AND_HALF_SEC_IN_NS);
   }
+
+  {
+    // Can rclcpp::Time be negative or not? The following constructor works:
+    rclcpp::Time time(-HALF_SEC_IN_NS);
+    auto time_msg = static_cast<builtin_interfaces::msg::Time>(time);
+    EXPECT_EQ(time_msg.sec, -1);
+    EXPECT_EQ(time_msg.nanosec, HALF_SEC_IN_NS);
+
+    // The opposite conversion throws...
+    EXPECT_ANY_THROW(
+    {
+      rclcpp::Time negative_time(time_msg);
+    });
+  }
+
+  {
+    // Can rclcpp::Time be negative or not? The following constructor works:
+    rclcpp::Time time(-ONE_SEC_IN_NS);
+    auto time_msg = static_cast<builtin_interfaces::msg::Time>(time);
+    EXPECT_EQ(time_msg.sec, -1);
+    EXPECT_EQ(time_msg.nanosec, 0u);
+
+    // The opposite conversion throws...
+    EXPECT_ANY_THROW(
+    {
+      rclcpp::Time negative_time(time_msg);
+    });
+  }
+
+  {
+    // Can rclcpp::Time be negative or not? The following constructor works:
+    rclcpp::Time time(-ONE_AND_HALF_SEC_IN_NS);
+    auto time_msg = static_cast<builtin_interfaces::msg::Time>(time);
+    EXPECT_EQ(time_msg.sec, -2);
+    EXPECT_EQ(time_msg.nanosec, HALF_SEC_IN_NS);
+
+    // The opposite conversion throws...
+    EXPECT_ANY_THROW(
+    {
+      rclcpp::Time negative_time(time_msg);
+    });
+  }
 }
 
 TEST_F(TestTime, operators) {
@@ -286,18 +326,31 @@ TEST_F(TestTime, overflow_detectors) {
 
 TEST_F(TestTime, overflows) {
   rclcpp::Time max_time(std::numeric_limits<rcl_time_point_value_t>::max());
+  rclcpp::Time min_time(std::numeric_limits<rcl_time_point_value_t>::min());
   rclcpp::Duration one(1ns);
   rclcpp::Duration two(2ns);
 
-  // Cross max
+  // Cross min/max
   EXPECT_THROW(max_time + one, std::overflow_error);
+  EXPECT_THROW(min_time - one, std::underflow_error);
+  EXPECT_THROW(max_time - min_time, std::overflow_error);
+  EXPECT_THROW(min_time - max_time, std::underflow_error);
   EXPECT_THROW(rclcpp::Time(max_time) += one, std::overflow_error);
+  EXPECT_THROW(rclcpp::Time(min_time) -= one, std::underflow_error);
   EXPECT_NO_THROW(max_time - max_time);
+  EXPECT_NO_THROW(min_time - min_time);
 
-  // Cross zero
+  // Cross zero in both directions
   rclcpp::Time one_time(1);
-  EXPECT_THROW(one_time - two, std::runtime_error);
-  EXPECT_THROW(rclcpp::Time(one_time) -= two, std::runtime_error);
+  EXPECT_NO_THROW(one_time - two);
+  EXPECT_NO_THROW(rclcpp::Time(one_time) -= two);
+
+  rclcpp::Time minus_one_time(-1);
+  EXPECT_NO_THROW(minus_one_time + two);
+  EXPECT_NO_THROW(rclcpp::Time(minus_one_time) += two);
+
+  EXPECT_NO_THROW(one_time - minus_one_time);
+  EXPECT_NO_THROW(minus_one_time - one_time);
 
   rclcpp::Time two_time(2);
   EXPECT_NO_THROW(one_time - two_time);
@@ -379,24 +432,41 @@ TEST_F(TestTime, test_overflow_underflow_throws) {
   RCLCPP_EXPECT_THROW_EQ(
     test_time = rclcpp::Time(INT64_MAX) + rclcpp::Duration(1ns),
     std::overflow_error("addition leads to int64_t overflow"));
+  RCLCPP_EXPECT_THROW_EQ(
+    test_time = rclcpp::Time(INT64_MIN) + rclcpp::Duration(-1ns),
+    std::underflow_error("addition leads to int64_t underflow"));
 
   RCLCPP_EXPECT_THROW_EQ(
     test_time = rclcpp::Time(INT64_MAX) - rclcpp::Duration(-1ns),
     std::overflow_error("time subtraction leads to int64_t overflow"));
+  RCLCPP_EXPECT_THROW_EQ(
+    test_time = rclcpp::Time(INT64_MIN) - rclcpp::Duration(1ns),
+    std::underflow_error("time subtraction leads to int64_t underflow"));
 
   test_time = rclcpp::Time(INT64_MAX);
   RCLCPP_EXPECT_THROW_EQ(
     test_time += rclcpp::Duration(1ns),
     std::overflow_error("addition leads to int64_t overflow"));
+  test_time = rclcpp::Time(INT64_MIN);
+  RCLCPP_EXPECT_THROW_EQ(
+    test_time += rclcpp::Duration(-1ns),
+    std::underflow_error("addition leads to int64_t underflow"));
 
   test_time = rclcpp::Time(INT64_MAX);
   RCLCPP_EXPECT_THROW_EQ(
     test_time -= rclcpp::Duration(-1ns),
     std::overflow_error("time subtraction leads to int64_t overflow"));
+  test_time = rclcpp::Time(INT64_MIN);
+  RCLCPP_EXPECT_THROW_EQ(
+    test_time -= rclcpp::Duration(1ns),
+    std::underflow_error("time subtraction leads to int64_t underflow"));
 
   RCLCPP_EXPECT_THROW_EQ(
     test_time = rclcpp::Duration::from_nanoseconds(INT64_MAX) + rclcpp::Time(1),
     std::overflow_error("addition leads to int64_t overflow"));
+  RCLCPP_EXPECT_THROW_EQ(
+    test_time = rclcpp::Duration::from_nanoseconds(INT64_MIN) + rclcpp::Time(-1),
+    std::underflow_error("addition leads to int64_t underflow"));
 }
 
 class TestClockSleep : public ::testing::Test

rclcpp_action/CHANGELOG.rst

@@ -3,46 +3,6 @@ Changelog for package rclcpp_action
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 
-28.3.0 (2024-06-17)
--------------------
-* Add 'mimick' label to tests which use Mimick (`#2516 <https://github.com/ros2/rclcpp/issues/2516>`_)
-* Contributors: Scott K Logan
-
-28.2.0 (2024-04-26)
--------------------
-
-28.1.0 (2024-04-16)
--------------------
-* Remove references to index.ros.org. (`#2504 <https://github.com/ros2/rclcpp/issues/2504>`_)
-* Contributors: Chris Lalancette
-
-28.0.1 (2024-04-16)
--------------------
-* Callback after cancel (`#2281 <https://github.com/ros2/rclcpp/issues/2281>`_)
-  * feat(Client): Added function to stop callbacks of a goal handle
-  This function allows us to drop the handle in a locked context.
-  If we do not do this within a lock, there will be a race condition between
-  the deletion of the shared_ptr of the handle and the result / feedback
-  callbacks.
-  * fix: make Client goal handle recursive
-  This fixes deadlocks due to release of goal handles in callbacks etc.
-  * fix(ActionGoalClient): Fixed memory leak for nominal case
-  This fixes a memory leak due to a self reference in the ClientGoalHandle.
-  Note, this fix will only work, if the ClientGoalHandle ever receives
-  a result callback.
-  * doc: Updated documentation of rclcpp_action::Client::async_send_goal
-  * docs: Made the async_send_goal documentation more explicit
-  Co-authored-by: Janosch Machowinski <J.Machowinski@cellumation.com>
-* Remake of "fix: Fixed race condition in action server between is_ready and take" (`#2495 <https://github.com/ros2/rclcpp/issues/2495>`_)
-  Some background information: is_ready, take_data and execute data
-  may be called from different threads in any order. The code in the old
-  state expected them to be called in series, without interruption.
-  This lead to multiple race conditions, as the state of the pimpl objects
-  was altered by the three functions in a non thread safe way.
-  Co-authored-by: Janosch Machowinski <j.machowinski@nospam.org>
-* update rclcpp::Waitable API to use references and const (`#2467 <https://github.com/ros2/rclcpp/issues/2467>`_)
-* Contributors: William Woodall, jmachowinski
-
 28.0.0 (2024-03-28)
 -------------------
 * Do not generate the exception when action service response timeout. (`#2464 <https://github.com/ros2/rclcpp/issues/2464>`_)

rclcpp_action/CMakeLists.txt

@@ -80,7 +80,6 @@ if(BUILD_TESTING)
   add_subdirectory(test/benchmark)
 
   ament_add_gtest(test_client test/test_client.cpp TIMEOUT 180)
-  ament_add_test_label(test_client mimick)
   if(TARGET test_client)
     target_link_libraries(test_client
       ${PROJECT_NAME}
@@ -94,7 +93,6 @@ if(BUILD_TESTING)
   endif()
 
   ament_add_gtest(test_server test/test_server.cpp TIMEOUT 180)
-  ament_add_test_label(test_server mimick)
   if(TARGET test_server)
     target_link_libraries(test_server
       ${PROJECT_NAME}
@@ -106,7 +104,6 @@ if(BUILD_TESTING)
   endif()
 
   ament_add_gtest(test_server_goal_handle test/test_server_goal_handle.cpp)
-  ament_add_test_label(test_server_goal_handle mimick)
   if(TARGET test_server_goal_handle)
     target_link_libraries(test_server_goal_handle
       ${PROJECT_NAME}

rclcpp_action/README.md

@@ -2,7 +2,7 @@
 
 Adds action APIs for C++.
 
-The link to the latest rclcpp_action API documentation, which includes a complete list of its main components and features, can be found on the [rclcpp_action package info page](https://docs.ros.org/en/rolling/p/rclcpp_action).
+The link to the latest rclcpp_action API documentation, which includes a complete list of its main components and features, can be found on the rclcpp_action package info page, at the [ROS Index](https://index.ros.org/p/rclcpp_action/).
 For more information about Actions in ROS 2, see the [design document](http://design.ros2.org/articles/actions.html).
 
 ## Quality Declaration

rclcpp_action/include/rclcpp_action/client.hpp

@@ -405,22 +405,12 @@ public:
 
   /// Send an action goal and asynchronously get the result.
   /**
-   * If the goal is accepted by an action server, the returned future is set to a `GoalHandle::SharedPtr`.
+   * If the goal is accepted by an action server, the returned future is set to a `ClientGoalHandle`.
    * If the goal is rejected by an action server, then the future is set to a `nullptr`.
    *
-   * The goal handle in the future is used to monitor the status of the goal and get the final result.
-   *
-   * If callbacks were set in @param options, you will receive callbacks, as long as you hold a reference
-   * to the shared pointer contained in the returned future, or rclcpp_action::Client is destroyed. Dropping
-   * the shared pointer to the goal handle will not cancel the goal. In order to cancel it, you must explicitly
-   * call async_cancel_goal.
-   *
-   * WARNING this method has inconsistent behaviour and a memory leak bug.
-   * If you set the result callback in @param options, the handle will be self referencing, and you will receive
-   * callbacks even though you do not hold a reference to the shared pointer. In this case, the self reference will
-   * be deleted if the result callback was received. If there is no result callback, there will be a memory leak.
-   *
-   * To prevent the memory leak, you may call stop_callbacks() explicit. This will delete the self reference.
+   * The returned goal handle is used to monitor the status of the goal and get the final result.
+   * It is valid as long as you hold a reference to the shared pointer or until the
+   * rclcpp_action::Client is destroyed at which point the goal status will become UNKNOWN.
    *
    * \param[in] goal The goal request.
    * \param[in] options Options for sending the goal request. Contains references to callbacks for
@@ -458,7 +448,7 @@ public:
         std::shared_ptr<GoalHandle> goal_handle(
           new GoalHandle(goal_info, options.feedback_callback, options.result_callback));
         {
-          std::lock_guard<std::recursive_mutex> guard(goal_handles_mutex_);
+          std::lock_guard<std::mutex> guard(goal_handles_mutex_);
           goal_handles_[goal_handle->get_goal_id()] = goal_handle;
         }
         promise->set_value(goal_handle);
@@ -476,7 +466,7 @@ public:
     // To prevent the list from growing out of control, forget about any goals
     // with no more user references
     {
-      std::lock_guard<std::recursive_mutex> guard(goal_handles_mutex_);
+      std::lock_guard<std::mutex> guard(goal_handles_mutex_);
       auto goal_handle_it = goal_handles_.begin();
       while (goal_handle_it != goal_handles_.end()) {
         if (!goal_handle_it->second.lock()) {
@@ -506,7 +496,7 @@ public:
     typename GoalHandle::SharedPtr goal_handle,
     ResultCallback result_callback = nullptr)
   {
-    std::lock_guard<std::recursive_mutex> lock(goal_handles_mutex_);
+    std::lock_guard<std::mutex> lock(goal_handles_mutex_);
     if (goal_handles_.count(goal_handle->get_goal_id()) == 0) {
       throw exceptions::UnknownGoalHandleError();
     }
@@ -541,7 +531,7 @@ public:
     typename GoalHandle::SharedPtr goal_handle,
     CancelCallback cancel_callback = nullptr)
   {
-    std::lock_guard<std::recursive_mutex> lock(goal_handles_mutex_);
+    std::lock_guard<std::mutex> lock(goal_handles_mutex_);
     if (goal_handles_.count(goal_handle->get_goal_id()) == 0) {
       throw exceptions::UnknownGoalHandleError();
     }
@@ -572,63 +562,6 @@ public:
     return async_cancel(cancel_request, cancel_callback);
   }
 
-  /// Stops the callbacks for the goal in a thread safe way
-  /**
-   * This will NOT cancel the goal, it will only stop the callbacks.
-   *
-   * After the call to this function, it is guaranteed that there
-   * will be no more callbacks from the goal. This is not guaranteed
-   * if multiple threads are involved, and the goal_handle is just
-   * dropped.
-   *
-   * \param[in] goal_handle The goal were the callbacks shall be stopped
-   */
-  void stop_callbacks(typename GoalHandle::SharedPtr goal_handle)
-  {
-    goal_handle->set_feedback_callback(typename GoalHandle::FeedbackCallback());
-    goal_handle->set_result_callback(typename GoalHandle::ResultCallback());
-
-    std::lock_guard<std::recursive_mutex> guard(goal_handles_mutex_);
-    const GoalUUID & goal_id = goal_handle->get_goal_id();
-    auto it = goal_handles_.find(goal_id);
-    if (goal_handles_.end() == it) {
-      // someone else already deleted the entry
-      // e.g. the result callback
-      RCLCPP_DEBUG(
-        this->get_logger(),
-        "Given goal is unknown. Ignoring...");
-      return;
-    }
-    goal_handles_.erase(it);
-  }
-
-  /// Stops the callbacks for the goal in a thread safe way
-  /**
-   * For futher information see stop_callbacks(typename GoalHandle::SharedPtr goal_handle)
-   */
-  void stop_callbacks(const GoalUUID & goal_id)
-  {
-    typename GoalHandle::SharedPtr goal_handle;
-    {
-      std::lock_guard<std::recursive_mutex> guard(goal_handles_mutex_);
-      auto it = goal_handles_.find(goal_id);
-      if (goal_handles_.end() == it) {
-        // someone else already deleted the entry
-        // e.g. the result callback
-        RCLCPP_DEBUG(
-          this->get_logger(),
-          "Given goal is unknown. Ignoring...");
-        return;
-      }
-
-      goal_handle = it->lock();
-    }
-
-    if (goal_handle) {
-      stop_callbacks(goal_handle);
-    }
-  }
-
   /// Asynchronously request all goals at or before a specified time be canceled.
   /**
    * \param[in] stamp The timestamp for the cancel goal request.
@@ -657,7 +590,7 @@ public:
   virtual
   ~Client()
   {
-    std::lock_guard<std::recursive_mutex> guard(goal_handles_mutex_);
+    std::lock_guard<std::mutex> guard(goal_handles_mutex_);
     auto it = goal_handles_.begin();
     while (it != goal_handles_.end()) {
       typename GoalHandle::SharedPtr goal_handle = it->second.lock();
@@ -704,7 +637,7 @@ private:
   void
   handle_feedback_message(std::shared_ptr<void> message) override
   {
-    std::lock_guard<std::recursive_mutex> guard(goal_handles_mutex_);
+    std::lock_guard<std::mutex> guard(goal_handles_mutex_);
     using FeedbackMessage = typename ActionT::Impl::FeedbackMessage;
     typename FeedbackMessage::SharedPtr feedback_message =
       std::static_pointer_cast<FeedbackMessage>(message);
@@ -741,7 +674,7 @@ private:
   void
   handle_status_message(std::shared_ptr<void> message) override
   {
-    std::lock_guard<std::recursive_mutex> guard(goal_handles_mutex_);
+    std::lock_guard<std::mutex> guard(goal_handles_mutex_);
     using GoalStatusMessage = typename ActionT::Impl::GoalStatusMessage;
     auto status_message = std::static_pointer_cast<GoalStatusMessage>(message);
     for (const GoalStatus & status : status_message->status_list) {
@@ -790,7 +723,7 @@ private:
           wrapped_result.goal_id = goal_handle->get_goal_id();
           wrapped_result.code = static_cast<ResultCode>(result_response->status);
           goal_handle->set_result(wrapped_result);
-          std::lock_guard<std::recursive_mutex> lock(goal_handles_mutex_);
+          std::lock_guard<std::mutex> lock(goal_handles_mutex_);
           goal_handles_.erase(goal_handle->get_goal_id());
         });
     } catch (rclcpp::exceptions::RCLError & ex) {
@@ -822,7 +755,7 @@ private:
   }
 
   std::map<GoalUUID, typename GoalHandle::WeakPtr> goal_handles_;
-  std::recursive_mutex goal_handles_mutex_;
+  std::mutex goal_handles_mutex_;
 };
 }  // namespace rclcpp_action
 

rclcpp_action/include/rclcpp_action/client_goal_handle_impl.hpp

@@ -75,7 +75,6 @@ ClientGoalHandle<ActionT>::set_result(const WrappedResult & wrapped_result)
   result_promise_.set_value(wrapped_result);
   if (result_callback_) {
     result_callback_(wrapped_result);
-    result_callback_ = ResultCallback();
   }
 }
 

rclcpp_action/package.xml

@@ -2,7 +2,7 @@
 <?xml-model href="http://download.ros.org/schema/package_format2.xsd" schematypens="http://www.w3.org/2001/XMLSchema"?>
 <package format="2">
   <name>rclcpp_action</name>
-  <version>28.3.0</version>
+  <version>28.0.0</version>
   <description>Adds action APIs for C++.</description>
 
   <maintainer email="ivanpauno@ekumenlabs.com">Ivan Paunovic</maintainer>

rclcpp_components/CHANGELOG.rst

@@ -2,20 +2,6 @@
 Changelog for package rclcpp_components
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-28.3.0 (2024-06-17)
--------------------
-
-28.2.0 (2024-04-26)
--------------------
-
-28.1.0 (2024-04-16)
--------------------
-* Remove references to index.ros.org. (`#2504 <https://github.com/ros2/rclcpp/issues/2504>`_)
-* Contributors: Chris Lalancette
-
-28.0.1 (2024-04-16)
--------------------
-
 28.0.0 (2024-03-28)
 -------------------
 * Add EXECUTOR docs (`#2440 <https://github.com/ros2/rclcpp/issues/2440>`_)

rclcpp_components/README.md

@@ -2,7 +2,7 @@
 
 Package containing tools for dynamically loadable components.
 
-The link to the latest rclcpp_components API documentation, which includes a complete list of its main components and features, can be found on the [rclcpp_components package info page](https://docs.ros.org/en/rolling/p/rclcpp_components).
+The link to the latest rclcpp_components API documentation, which includes a complete list of its main components and features, can be found on the rclcpp_components package info page, at the [ROS Index](https://index.ros.org/p/rclcpp_components/).
 
 ## Quality Declaration
 

rclcpp_components/package.xml

@@ -2,7 +2,7 @@
 <?xml-model href="http://download.ros.org/schema/package_format2.xsd" schematypens="http://www.w3.org/2001/XMLSchema"?>
 <package format="2">
   <name>rclcpp_components</name>
-  <version>28.3.0</version>
+  <version>28.0.0</version>
   <description>Package containing tools for dynamically loadable components</description>
 
   <maintainer email="ivanpauno@ekumenlabs.com">Ivan Paunovic</maintainer>

rclcpp_lifecycle/CHANGELOG.rst

@@ -3,32 +3,6 @@ Changelog for package rclcpp_lifecycle
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 
-28.3.0 (2024-06-17)
--------------------
-* revert call shutdown in LifecycleNode destructor (`#2557 <https://github.com/ros2/rclcpp/issues/2557>`_)
-* LifecycleNode shutdown on dtor only with valid context. (`#2545 <https://github.com/ros2/rclcpp/issues/2545>`_)
-* call shutdown in LifecycleNode dtor to avoid leaving the device in unknown state (2nd) (`#2528 <https://github.com/ros2/rclcpp/issues/2528>`_)
-* rclcpp::shutdown should not be called before LifecycleNode dtor. (`#2527 <https://github.com/ros2/rclcpp/issues/2527>`_)
-* Revert "call shutdown in LifecycleNode dtor to avoid leaving the device in un… (`#2450 <https://github.com/ros2/rclcpp/issues/2450>`_)" (`#2522 <https://github.com/ros2/rclcpp/issues/2522>`_)
-* Add 'mimick' label to tests which use Mimick (`#2516 <https://github.com/ros2/rclcpp/issues/2516>`_)
-* Contributors: Chris Lalancette, Scott K Logan, Tomoya Fujita
-
-28.2.0 (2024-04-26)
--------------------
-
-28.1.0 (2024-04-16)
--------------------
-* Remove references to index.ros.org. (`#2504 <https://github.com/ros2/rclcpp/issues/2504>`_)
-* Contributors: Chris Lalancette
-
-28.0.1 (2024-04-16)
--------------------
-* call shutdown in LifecycleNode dtor to avoid leaving the device in un… (`#2450 <https://github.com/ros2/rclcpp/issues/2450>`_)
-  * call shutdown in LifecycleNode dtor to avoid leaving the device in unknown state.
-  * add test to verify LifecycleNode::shutdown is called on destructor.
-  ---------
-* Contributors: Tomoya Fujita
-
 28.0.0 (2024-03-28)
 -------------------
 * Update quality declaration documents (`#2427 <https://github.com/ros2/rclcpp/issues/2427>`_)

rclcpp_lifecycle/CMakeLists.txt

@@ -95,7 +95,6 @@ if(BUILD_TESTING)
   endif()
 
   ament_add_gtest(test_lifecycle_node test/test_lifecycle_node.cpp TIMEOUT 120)
-  ament_add_test_label(test_lifecycle_node mimick)
   if(TARGET test_lifecycle_node)
     target_link_libraries(test_lifecycle_node ${PROJECT_NAME} mimick rcl_lifecycle::rcl_lifecycle rclcpp::rclcpp rcutils::rcutils)
   endif()
@@ -104,7 +103,6 @@ if(BUILD_TESTING)
     target_link_libraries(test_lifecycle_publisher ${PROJECT_NAME} rcl_lifecycle::rcl_lifecycle rclcpp::rclcpp ${test_msgs_TARGETS})
   endif()
   ament_add_gtest(test_lifecycle_service_client test/test_lifecycle_service_client.cpp TIMEOUT 120)
-  ament_add_test_label(test_lifecycle_service_client mimick)
   if(TARGET test_lifecycle_service_client)
     target_link_libraries(test_lifecycle_service_client
       ${PROJECT_NAME}
@@ -115,7 +113,6 @@ if(BUILD_TESTING)
       rcutils::rcutils)
   endif()
   ament_add_gtest(test_client test/test_client.cpp TIMEOUT 120)
-  ament_add_test_label(test_client mimick)
   if(TARGET test_client)
     target_link_libraries(test_client
       ${PROJECT_NAME}
@@ -124,7 +121,6 @@ if(BUILD_TESTING)
       rclcpp::rclcpp)
   endif()
   ament_add_gtest(test_service test/test_service.cpp TIMEOUT 120)
-  ament_add_test_label(test_service mimick)
   if(TARGET test_service)
     target_link_libraries(test_service
       ${PROJECT_NAME}
@@ -149,7 +145,6 @@ if(BUILD_TESTING)
     target_link_libraries(test_state_wrapper ${PROJECT_NAME} rcl_lifecycle::rcl_lifecycle rclcpp::rclcpp)
   endif()
   ament_add_gtest(test_transition_wrapper test/test_transition_wrapper.cpp)
-  ament_add_test_label(test_transition_wrapper mimick)
   if(TARGET test_transition_wrapper)
     target_link_libraries(test_transition_wrapper ${PROJECT_NAME} mimick rcl_lifecycle::rcl_lifecycle rclcpp::rclcpp rcutils::rcutils)
     target_compile_definitions(test_transition_wrapper

rclcpp_lifecycle/README.md

@@ -2,7 +2,7 @@
 
 Package containing a prototype for lifecycle implementation.
 
-The link to the latest rclcpp_lifecycle API documentation, which includes a complete list of its main components and features, can be found on the [rclcpp_lifecycle package info page](https://docs.ros.org/en/rolling/p/rclcpp_lifecycle).
+The link to the latest rclcpp_lifecycle API documentation, which includes a complete list of its main components and features, can be found on the rclcpp_lifecycle package info page, at the [ROS Index](https://index.ros.org/p/rclcpp_lifecycle/).
 For more information about LifeCycle in ROS 2, see the [design document](http://design.ros2.org/articles/node_lifecycle.html).
 
 ## Quality Declaration

rclcpp_lifecycle/package.xml

@@ -2,7 +2,7 @@
 <?xml-model href="http://download.ros.org/schema/package_format2.xsd" schematypens="http://www.w3.org/2001/XMLSchema"?>
 <package format="2">
   <name>rclcpp_lifecycle</name>
-  <version>28.3.0</version>
+  <version>28.0.0</version>
   <description>Package containing a prototype for lifecycle implementation</description>
 
   <maintainer email="ivanpauno@ekumenlabs.com">Ivan Paunovic</maintainer>

rclcpp_lifecycle/src/lifecycle_node.cpp

@@ -152,6 +152,22 @@ LifecycleNode::LifecycleNode(
 
 LifecycleNode::~LifecycleNode()
 {
+  // shutdown if necessary to avoid leaving the device in unknown state
+  if (LifecycleNode::get_current_state().id() !=
+    lifecycle_msgs::msg::State::PRIMARY_STATE_FINALIZED)
+  {
+    auto ret = rclcpp_lifecycle::node_interfaces::LifecycleNodeInterface::CallbackReturn::ERROR;
+    auto finalized = LifecycleNode::shutdown(ret);
+    if (finalized.id() != lifecycle_msgs::msg::State::PRIMARY_STATE_FINALIZED ||
+      ret != rclcpp_lifecycle::node_interfaces::LifecycleNodeInterface::CallbackReturn::SUCCESS)
+    {
+      RCLCPP_WARN(
+        rclcpp::get_logger("rclcpp_lifecycle"),
+        "Shutdown error in destruction of LifecycleNode: final state(%s)",
+        finalized.label().c_str());
+    }
+  }
+
   // release sub-interfaces in an order that allows them to consult with node_base during tear-down
   node_waitables_.reset();
   node_time_source_.reset();

rclcpp_lifecycle/test/benchmark/benchmark_lifecycle_client.cpp

@@ -203,7 +203,7 @@ public:
     executor = std::make_shared<rclcpp::executors::SingleThreadedExecutor>();
     executor->add_node(lifecycle_node->get_node_base_interface());
     executor->add_node(lifecycle_client->get_node_base_interface());
-    spinner_ = std::thread(&rclcpp::executors::SingleThreadedExecutor::spin, executor);
+    spinner_ = std::thread([this]() {executor->spin();});
     performance_test_fixture::PerformanceTest::SetUp(state);
   }
 

rclcpp_lifecycle/test/test_lifecycle_node.cpp

@@ -447,6 +447,146 @@ TEST_F(TestDefaultStateMachine, bad_mood) {
   EXPECT_EQ(1u, test_node->number_of_callbacks);
 }
 
+
+TEST_F(TestDefaultStateMachine, shutdown_from_each_primary_state) {
+  auto success = rclcpp_lifecycle::node_interfaces::LifecycleNodeInterface::CallbackReturn::SUCCESS;
+  auto reset_key = rclcpp_lifecycle::node_interfaces::LifecycleNodeInterface::CallbackReturn::ERROR;
+
+  // PRIMARY_STATE_UNCONFIGURED to shutdown
+  {
+    auto ret = reset_key;
+    auto test_node = std::make_shared<EmptyLifecycleNode>("testnode");
+    auto finalized = test_node->shutdown(ret);
+    EXPECT_EQ(success, ret);
+    EXPECT_EQ(finalized.id(), State::PRIMARY_STATE_FINALIZED);
+  }
+
+  // PRIMARY_STATE_INACTIVE to shutdown
+  {
+    auto ret = reset_key;
+    auto test_node = std::make_shared<EmptyLifecycleNode>("testnode");
+    auto configured = test_node->configure(ret);
+    EXPECT_EQ(success, ret);
+    EXPECT_EQ(configured.id(), State::PRIMARY_STATE_INACTIVE);
+    ret = reset_key;
+    auto finalized = test_node->shutdown(ret);
+    EXPECT_EQ(success, ret);
+    EXPECT_EQ(finalized.id(), State::PRIMARY_STATE_FINALIZED);
+  }
+
+  // PRIMARY_STATE_ACTIVE to shutdown
+  {
+    auto ret = reset_key;
+    auto test_node = std::make_shared<EmptyLifecycleNode>("testnode");
+    auto configured = test_node->configure(ret);
+    EXPECT_EQ(success, ret);
+    EXPECT_EQ(configured.id(), State::PRIMARY_STATE_INACTIVE);
+    ret = reset_key;
+    auto activated = test_node->activate(ret);
+    EXPECT_EQ(success, ret);
+    EXPECT_EQ(activated.id(), State::PRIMARY_STATE_ACTIVE);
+    ret = reset_key;
+    auto finalized = test_node->shutdown(ret);
+    EXPECT_EQ(success, ret);
+    EXPECT_EQ(finalized.id(), State::PRIMARY_STATE_FINALIZED);
+  }
+
+  // PRIMARY_STATE_FINALIZED to shutdown
+  {
+    auto ret = reset_key;
+    auto test_node = std::make_shared<EmptyLifecycleNode>("testnode");
+    auto configured = test_node->configure(ret);
+    EXPECT_EQ(success, ret);
+    EXPECT_EQ(configured.id(), State::PRIMARY_STATE_INACTIVE);
+    ret = reset_key;
+    auto activated = test_node->activate(ret);
+    EXPECT_EQ(success, ret);
+    EXPECT_EQ(activated.id(), State::PRIMARY_STATE_ACTIVE);
+    ret = reset_key;
+    auto finalized = test_node->shutdown(ret);
+    EXPECT_EQ(success, ret);
+    EXPECT_EQ(finalized.id(), State::PRIMARY_STATE_FINALIZED);
+    ret = reset_key;
+    auto finalized_again = test_node->shutdown(ret);
+    EXPECT_EQ(reset_key, ret);
+    EXPECT_EQ(finalized_again.id(), State::PRIMARY_STATE_FINALIZED);
+  }
+}
+
+TEST_F(TestDefaultStateMachine, test_shutdown_on_dtor) {
+  auto success = rclcpp_lifecycle::node_interfaces::LifecycleNodeInterface::CallbackReturn::SUCCESS;
+  auto reset_key = rclcpp_lifecycle::node_interfaces::LifecycleNodeInterface::CallbackReturn::ERROR;
+
+  bool shutdown_cb_called = false;
+  auto on_shutdown_callback =
+    [&shutdown_cb_called](const rclcpp_lifecycle::State &) ->
+    rclcpp_lifecycle::node_interfaces::LifecycleNodeInterface::CallbackReturn {
+      shutdown_cb_called = true;
+      return rclcpp_lifecycle::node_interfaces::LifecycleNodeInterface::CallbackReturn::SUCCESS;
+    };
+
+  // PRIMARY_STATE_UNCONFIGURED to shutdown via dtor
+  shutdown_cb_called = false;
+  {
+    auto test_node = std::make_shared<rclcpp_lifecycle::LifecycleNode>("testnode");
+    test_node->register_on_shutdown(std::bind(on_shutdown_callback, std::placeholders::_1));
+    EXPECT_EQ(State::PRIMARY_STATE_UNCONFIGURED, test_node->get_current_state().id());
+    EXPECT_FALSE(shutdown_cb_called);
+  }
+  EXPECT_TRUE(shutdown_cb_called);
+
+  // PRIMARY_STATE_INACTIVE to shutdown via dtor
+  shutdown_cb_called = false;
+  {
+    auto ret = reset_key;
+    auto test_node = std::make_shared<rclcpp_lifecycle::LifecycleNode>("testnode");
+    test_node->register_on_shutdown(std::bind(on_shutdown_callback, std::placeholders::_1));
+    auto configured = test_node->configure(ret);
+    EXPECT_EQ(success, ret);
+    EXPECT_EQ(configured.id(), State::PRIMARY_STATE_INACTIVE);
+    EXPECT_FALSE(shutdown_cb_called);
+  }
+  EXPECT_TRUE(shutdown_cb_called);
+
+  // PRIMARY_STATE_ACTIVE to shutdown via dtor
+  shutdown_cb_called = false;
+  {
+    auto ret = reset_key;
+    auto test_node = std::make_shared<rclcpp_lifecycle::LifecycleNode>("testnode");
+    test_node->register_on_shutdown(std::bind(on_shutdown_callback, std::placeholders::_1));
+    auto configured = test_node->configure(ret);
+    EXPECT_EQ(success, ret);
+    EXPECT_EQ(configured.id(), State::PRIMARY_STATE_INACTIVE);
+    ret = reset_key;
+    auto activated = test_node->activate(ret);
+    EXPECT_EQ(success, ret);
+    EXPECT_EQ(activated.id(), State::PRIMARY_STATE_ACTIVE);
+    EXPECT_FALSE(shutdown_cb_called);
+  }
+  EXPECT_TRUE(shutdown_cb_called);
+
+  // PRIMARY_STATE_FINALIZED to shutdown via dtor
+  shutdown_cb_called = false;
+  {
+    auto ret = reset_key;
+    auto test_node = std::make_shared<rclcpp_lifecycle::LifecycleNode>("testnode");
+    test_node->register_on_shutdown(std::bind(on_shutdown_callback, std::placeholders::_1));
+    auto configured = test_node->configure(ret);
+    EXPECT_EQ(success, ret);
+    EXPECT_EQ(configured.id(), State::PRIMARY_STATE_INACTIVE);
+    ret = reset_key;
+    auto activated = test_node->activate(ret);
+    EXPECT_EQ(success, ret);
+    EXPECT_EQ(activated.id(), State::PRIMARY_STATE_ACTIVE);
+    ret = reset_key;
+    auto finalized = test_node->shutdown(ret);
+    EXPECT_EQ(success, ret);
+    EXPECT_EQ(finalized.id(), State::PRIMARY_STATE_FINALIZED);
+    EXPECT_TRUE(shutdown_cb_called);  // should be called already
+  }
+  EXPECT_TRUE(shutdown_cb_called);
+}
+
 TEST_F(TestDefaultStateMachine, lifecycle_subscriber) {
   auto test_node = std::make_shared<MoodyLifecycleNode<GoodMood>>("testnode");
 

rclcpp_lifecycle/test/test_lifecycle_publisher.cpp

@@ -55,6 +55,12 @@ public:
   explicit EmptyLifecycleNode(const std::string & node_name, const TimerType & timer_type)
   : rclcpp_lifecycle::LifecycleNode(node_name)
   {
+    rclcpp::PublisherOptionsWithAllocator<std::allocator<void>> options;
+    publisher_ =
+      std::make_shared<rclcpp_lifecycle::LifecyclePublisher<test_msgs::msg::Empty>>(
+      get_node_base_interface().get(), std::string("topic"), rclcpp::QoS(10), options);
+    add_managed_entity(publisher_);
+
     // For coverage this is being added here
     switch (timer_type) {
       case TimerType::WALL_TIMER:
@@ -71,6 +77,14 @@ public:
         }
     }
   }
+
+  std::shared_ptr<rclcpp_lifecycle::LifecyclePublisher<test_msgs::msg::Empty>> publisher()
+  {
+    return publisher_;
+  }
+
+private:
+  std::shared_ptr<rclcpp_lifecycle::LifecyclePublisher<test_msgs::msg::Empty>> publisher_;
 };
 
 class TestLifecyclePublisher : public ::testing::TestWithParam<TimerType>
@@ -79,103 +93,95 @@ public:
   void SetUp()
   {
     rclcpp::init(0, nullptr);
+    node_ = std::make_shared<EmptyLifecycleNode>("node", GetParam());
   }
 
   void TearDown()
   {
     rclcpp::shutdown();
   }
+
+protected:
+  std::shared_ptr<EmptyLifecycleNode> node_;
 };
 
 TEST_P(TestLifecyclePublisher, publish_managed_by_node) {
-  auto node = std::make_shared<EmptyLifecycleNode>("node", GetParam());
-
-  rclcpp::PublisherOptionsWithAllocator<std::allocator<void>> options;
-  std::shared_ptr<rclcpp_lifecycle::LifecyclePublisher<test_msgs::msg::Empty>> publisher =
-    node->create_publisher<test_msgs::msg::Empty>(std::string("topic"), rclcpp::QoS(10), options);
-
   // transition via LifecycleNode
   auto success = rclcpp_lifecycle::node_interfaces::LifecycleNodeInterface::CallbackReturn::SUCCESS;
   auto reset_key = rclcpp_lifecycle::node_interfaces::LifecycleNodeInterface::CallbackReturn::ERROR;
   auto ret = reset_key;
 
-  EXPECT_EQ(State::PRIMARY_STATE_UNCONFIGURED, node->get_current_state().id());
-  node->trigger_transition(
+  EXPECT_EQ(State::PRIMARY_STATE_UNCONFIGURED, node_->get_current_state().id());
+  node_->trigger_transition(
     rclcpp_lifecycle::Transition(Transition::TRANSITION_CONFIGURE), ret);
   ASSERT_EQ(success, ret);
   ret = reset_key;
-  node->trigger_transition(
+  node_->trigger_transition(
     rclcpp_lifecycle::Transition(Transition::TRANSITION_ACTIVATE), ret);
   ASSERT_EQ(success, ret);
   ret = reset_key;
-  EXPECT_TRUE(publisher->is_activated());
+  EXPECT_TRUE(node_->publisher()->is_activated());
   {
     auto msg_ptr = std::make_unique<test_msgs::msg::Empty>();
-    EXPECT_NO_THROW(publisher->publish(*msg_ptr));
+    EXPECT_NO_THROW(node_->publisher()->publish(*msg_ptr));
   }
   {
     auto msg_ptr = std::make_unique<test_msgs::msg::Empty>();
-    EXPECT_NO_THROW(publisher->publish(std::move(msg_ptr)));
+    EXPECT_NO_THROW(node_->publisher()->publish(std::move(msg_ptr)));
   }
   {
-    auto loaned_msg = publisher->borrow_loaned_message();
-    EXPECT_NO_THROW(publisher->publish(std::move(loaned_msg)));
+    auto loaned_msg = node_->publisher()->borrow_loaned_message();
+    EXPECT_NO_THROW(node_->publisher()->publish(std::move(loaned_msg)));
   }
-  node->trigger_transition(
+  node_->trigger_transition(
     rclcpp_lifecycle::Transition(Transition::TRANSITION_DEACTIVATE), ret);
   ASSERT_EQ(success, ret);
   ret = reset_key;
   (void)ret;  // Just to make clang happy
-  EXPECT_FALSE(publisher->is_activated());
+  EXPECT_FALSE(node_->publisher()->is_activated());
   {
     auto msg_ptr = std::make_unique<test_msgs::msg::Empty>();
-    EXPECT_NO_THROW(publisher->publish(*msg_ptr));
+    EXPECT_NO_THROW(node_->publisher()->publish(*msg_ptr));
   }
   {
     auto msg_ptr = std::make_unique<test_msgs::msg::Empty>();
-    EXPECT_NO_THROW(publisher->publish(std::move(msg_ptr)));
+    EXPECT_NO_THROW(node_->publisher()->publish(std::move(msg_ptr)));
   }
   {
-    auto loaned_msg = publisher->borrow_loaned_message();
-    EXPECT_NO_THROW(publisher->publish(std::move(loaned_msg)));
+    auto loaned_msg = node_->publisher()->borrow_loaned_message();
+    EXPECT_NO_THROW(node_->publisher()->publish(std::move(loaned_msg)));
   }
 }
 
 TEST_P(TestLifecyclePublisher, publish) {
-  auto node = std::make_shared<EmptyLifecycleNode>("node", GetParam());
-
-  rclcpp::PublisherOptionsWithAllocator<std::allocator<void>> options;
-  std::shared_ptr<rclcpp_lifecycle::LifecyclePublisher<test_msgs::msg::Empty>> publisher =
-    node->create_publisher<test_msgs::msg::Empty>(std::string("topic"), rclcpp::QoS(10), options);
-
   // transition via LifecyclePublisher
-  publisher->on_deactivate();
-  EXPECT_FALSE(publisher->is_activated());
+  node_->publisher()->on_deactivate();
+  EXPECT_FALSE(node_->publisher()->is_activated());
   {
     auto msg_ptr = std::make_unique<test_msgs::msg::Empty>();
-    EXPECT_NO_THROW(publisher->publish(*msg_ptr));
+    EXPECT_NO_THROW(node_->publisher()->publish(*msg_ptr));
   }
   {
     auto msg_ptr = std::make_unique<test_msgs::msg::Empty>();
-    EXPECT_NO_THROW(publisher->publish(std::move(msg_ptr)));
+    EXPECT_NO_THROW(node_->publisher()->publish(std::move(msg_ptr)));
   }
   {
-    auto loaned_msg = publisher->borrow_loaned_message();
-    EXPECT_NO_THROW(publisher->publish(std::move(loaned_msg)));
+    auto loaned_msg = node_->publisher()->borrow_loaned_message();
+    EXPECT_NO_THROW(node_->publisher()->publish(std::move(loaned_msg)));
   }
-  publisher->on_activate();
-  EXPECT_TRUE(publisher->is_activated());
+  node_->publisher()->on_activate();
+  EXPECT_TRUE(node_->publisher()->is_activated());
   {
     auto msg_ptr = std::make_unique<test_msgs::msg::Empty>();
-    EXPECT_NO_THROW(publisher->publish(*msg_ptr));
+    EXPECT_NO_THROW(node_->publisher()->publish(*msg_ptr));
   }
   {
     auto msg_ptr = std::make_unique<test_msgs::msg::Empty>();
-    EXPECT_NO_THROW(publisher->publish(std::move(msg_ptr)));
+    EXPECT_NO_THROW(node_->publisher()->publish(std::move(msg_ptr)));
   }
   {
-    auto loaned_msg = publisher->borrow_loaned_message();
-    EXPECT_NO_THROW(publisher->publish(std::move(loaned_msg)));
+    auto loaned_msg = node_->publisher()->borrow_loaned_message();
+    EXPECT_NO_THROW(node_->publisher()->publish(std::move(loaned_msg)));
   }
 }