deprecate rclcpp::spin_some and rclcpp::spin_all

rclcpp/include/rclcpp/executors.hpp

@@ -28,24 +28,82 @@
 namespace rclcpp
 {
 
-/// Create a default single-threaded executor and execute all available work exhaustively.
-/** \param[in] node_ptr Shared pointer to the node to spin. */
+/**
+ * @brief Create a default single-threaded executor and execute all available work exhaustively.
+ * @param node_ptr Shared pointer to the base interface of the node to spin.
+ * @param max_duration max duration to spin
+ *
+ * This method is deprecated because it can lead to very bad performance if used in a loop:
+ * each call will create a new executor and register the node, which is an expensive operation.
+ * It's recommended to always manually instantiate an executor and call the methods with
+ * the same name on it.
+ * For example:
+ *     SingleThreadedExecutor executor;
+ *     executor.add_node(node_ptr);
+ *     executor.spin_all(max_duration);
+ * If you are using a non-default context, this should be passed to the executor's constructor.
+ */
+[[deprecated("use SingleThreadedExecutor::spin_all instead")]]
 RCLCPP_PUBLIC
 void
 spin_all(
   rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node_ptr,
   std::chrono::nanoseconds max_duration);
 
+/**
+ * @brief Create a default single-threaded executor and execute all available work exhaustively.
+ * @param node_ptr Shared pointer to the node to spin.
+ * @param max_duration max duration to spin
+ *
+ * This method is deprecated because it can lead to very bad performance if used in a loop:
+ * each call will create a new executor and register the node, which is an expensive operation.
+ * It's recommended to always manually instantiate an executor and call the methods with
+ * the same name on it.
+ * For example:
+ *     SingleThreadedExecutor executor;
+ *     executor.add_node(node_ptr);
+ *     executor.spin_all(max_duration);
+ * If you are using a non-default context, this should be passed to the executor's constructor.
+ */
+[[deprecated("use SingleThreadedExecutor::spin_all instead")]]
 RCLCPP_PUBLIC
 void
 spin_all(rclcpp::Node::SharedPtr node_ptr, std::chrono::nanoseconds max_duration);
 
-/// Create a default single-threaded executor and execute any immediately available work.
-/** \param[in] node_ptr Shared pointer to the node to spin. */
+/**
+ * @brief Create a default single-threaded executor and execute any immediately available work.
+ * @param node_ptr Shared pointer to the base interface of the node to spin.
+ *
+ * This method is deprecated because it can lead to very bad performance if used in a loop:
+ * each call will create a new executor and register the node, which is an expensive operation.
+ * It's recommended to always manually instantiate an executor and call the methods with
+ * the same name on it.
+ * For example:
+ *     SingleThreadedExecutor executor;
+ *     executor.add_node(node_ptr);
+ *     executor.spin_some();
+ * If you are using a non-default context, this should be passed to the executor's constructor.
+ */
+ [[deprecated("use SingleThreadedExecutor::spin_some instead")]]
 RCLCPP_PUBLIC
 void
 spin_some(rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node_ptr);
 
+/**
+ * @brief Create a default single-threaded executor and execute any immediately available work.
+ * @param node_ptr Shared pointer to the node to spin.
+ *
+ * This method is deprecated because it can lead to very bad performance if used in a loop:
+ * each call will create a new executor and register the node, which is an expensive operation.
+ * It's recommended to always manually instantiate an executor and call the methods with
+ * the same name on it.
+ * For example:
+ *     SingleThreadedExecutor executor;
+ *     executor.add_node(node_ptr);
+ *     executor.spin_some();
+ * If you are using a non-default context, this should be passed to the executor's constructor.
+ */
+ [[deprecated("use SingleThreadedExecutor::spin_some instead")]]
 RCLCPP_PUBLIC
 void
 spin_some(rclcpp::Node::SharedPtr node_ptr);

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

@@ -64,17 +64,17 @@ class EventsExecutor : public rclcpp::Executor
 
   /// Default constructor. See the default constructor for Executor.
   /**
+   * \param[in] options Options used to configure the executor.
    * \param[in] events_queue The queue used to store events.
    * \param[in] execute_timers_separate_thread If true, timers are executed in a separate
    * thread. If false, timers are executed in the same thread as all other entities.
-   * \param[in] options Options used to configure the executor.
    */
-  RCLCPP_PUBLIC
-  EventsExecutor(
-    rclcpp::experimental::executors::EventsQueue::UniquePtr events_queue = std::make_unique<
-      rclcpp::experimental::executors::SimpleEventsQueue>(),
-    bool execute_timers_separate_thread = false,
-    const rclcpp::ExecutorOptions & options = rclcpp::ExecutorOptions());
+   RCLCPP_PUBLIC
+   EventsExecutor(
+    const rclcpp::ExecutorOptions & options = rclcpp::ExecutorOptions(),
+     rclcpp::experimental::executors::EventsQueue::UniquePtr events_queue = std::make_unique<
+       rclcpp::experimental::executors::SimpleEventsQueue>(),
+     bool execute_timers_separate_thread = false);
 
   /// Default destructor.
   RCLCPP_PUBLIC

rclcpp/include/rclcpp/rclcpp.hpp

@@ -68,8 +68,6 @@
  *
  * - Executors (responsible for execution of callbacks through a blocking spin):
  *   - rclcpp::spin()
- *   - rclcpp::spin_some()
- *   - rclcpp::spin_until_future_complete()
  *   - rclcpp::executors::SingleThreadedExecutor
  *   - rclcpp::executors::SingleThreadedExecutor::add_node()
  *   - rclcpp::executors::SingleThreadedExecutor::spin()

rclcpp/src/rclcpp/executor.cpp

@@ -52,6 +52,7 @@ class rclcpp::ExecutorImplementation {};
 
 Executor::Executor(const std::shared_ptr<rclcpp::Context> & context)
 : spinning(false),
+  context_(context),
   entities_need_rebuild_(true),
   collector_(nullptr),
   wait_set_({}, {}, {}, {}, {}, {}, context)
@@ -186,6 +187,12 @@ Executor::add_callback_group(
 void
 Executor::add_node(rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node_ptr, bool notify)
 {
+  if (node_ptr->get_context() != context_)
+  {
+    throw std::runtime_error(
+      "add_node() called with a node with a different context from this executor");
+  }
+
   this->collector_.add_node(node_ptr);
 
   try {

rclcpp/src/rclcpp/executors.cpp

@@ -13,6 +13,7 @@
 // limitations under the License.
 
 #include "rclcpp/executors.hpp"
+#include "rcpputils/compile_warnings.hpp"
 
 void
 rclcpp::spin_all(
@@ -25,12 +26,6 @@ rclcpp::spin_all(
   exec.spin_node_all(node_ptr, max_duration);
 }
 
-void
-rclcpp::spin_all(rclcpp::Node::SharedPtr node_ptr, std::chrono::nanoseconds max_duration)
-{
-  rclcpp::spin_all(node_ptr->get_node_base_interface(), max_duration);
-}
-
 void
 rclcpp::spin_some(rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node_ptr)
 {
@@ -40,12 +35,6 @@ rclcpp::spin_some(rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node_ptr
   exec.spin_node_some(node_ptr);
 }
 
-void
-rclcpp::spin_some(rclcpp::Node::SharedPtr node_ptr)
-{
-  rclcpp::spin_some(node_ptr->get_node_base_interface());
-}
-
 void
 rclcpp::spin(rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node_ptr)
 {
@@ -62,3 +51,19 @@ rclcpp::spin(rclcpp::Node::SharedPtr node_ptr)
 {
   rclcpp::spin(node_ptr->get_node_base_interface());
 }
+
+void
+rclcpp::spin_all(rclcpp::Node::SharedPtr node_ptr, std::chrono::nanoseconds max_duration)
+{
+  RCPPUTILS_DEPRECATION_WARNING_OFF_START
+  rclcpp::spin_all(node_ptr->get_node_base_interface(), max_duration);
+  RCPPUTILS_DEPRECATION_WARNING_OFF_STOP
+}
+
+void
+rclcpp::spin_some(rclcpp::Node::SharedPtr node_ptr)
+{
+  RCPPUTILS_DEPRECATION_WARNING_OFF_START
+  rclcpp::spin_some(node_ptr->get_node_base_interface());
+  RCPPUTILS_DEPRECATION_WARNING_OFF_STOP
+}

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

@@ -25,9 +25,9 @@ using namespace std::chrono_literals;
 using rclcpp::experimental::executors::EventsExecutor;
 
 EventsExecutor::EventsExecutor(
+  const rclcpp::ExecutorOptions & options,
   rclcpp::experimental::executors::EventsQueue::UniquePtr events_queue,
-  bool execute_timers_separate_thread,
-  const rclcpp::ExecutorOptions & options)
+  bool execute_timers_separate_thread)
 : rclcpp::Executor(options)
 {
   // Get ownership of the queue used to store events.

rclcpp/test/benchmark/benchmark_service.cpp

@@ -125,19 +125,21 @@ BENCHMARK_F(ServicePerformanceTest, async_send_response)(benchmark::State & stat
   auto service = node->create_service<test_msgs::srv::Empty>(empty_service_name, callback);
 
   reset_heap_counters();
+  rclcpp::executors::SingleThreadedExecutor executor;
+  executor.add_node(node->get_node_base_interface());
   for (auto _ : state) {
     (void)_;
     state.PauseTiming();
     // Clear executor queue
-    rclcpp::spin_some(node->get_node_base_interface());
+    executor.spin_some();
 
     auto request = std::make_shared<test_msgs::srv::Empty::Request>();
     auto future = empty_client->async_send_request(request);
     state.ResumeTiming();
     benchmark::DoNotOptimize(service);
     benchmark::ClobberMemory();
 
-    rclcpp::spin_until_future_complete(node->get_node_base_interface(), future);
+    executor.spin_until_future_complete(future);
   }
   if (callback_count == 0) {
     state.SkipWithError("Service callback was not called");

rclcpp/test/rclcpp/executors/test_executors.cpp

@@ -804,27 +804,33 @@ TEST(TestExecutors, testSpinUntilFutureCompleteNodePtr)
 }
 
 // Check spin functions with non default context
-TEST(TestExecutors, testSpinWithNonDefaultContext)
+TYPED_TEST(TestExecutors, testSpinWithNonDefaultContext)
 {
+  using ExecutorType = TypeParam;
+
   auto non_default_context = std::make_shared<rclcpp::Context>();
   non_default_context->init(0, nullptr);
 
   {
     auto node =
       std::make_unique<rclcpp::Node>("node", rclcpp::NodeOptions().context(non_default_context));
 
-    EXPECT_NO_THROW(rclcpp::spin_some(node->get_node_base_interface()));
+    rclcpp::ExecutorOptions options;
+    options.context = non_default_context;
+    ExecutorType executor(options);
+    EXPECT_NO_THROW(executor.add_node(node->get_node_base_interface()));
+
+    EXPECT_NO_THROW(executor.spin_some());
 
-    EXPECT_NO_THROW(rclcpp::spin_all(node->get_node_base_interface(), 1s));
+    EXPECT_NO_THROW(executor.spin_all(1s));
 
     auto check_spin_until_future_complete = [&]() {
         std::promise<bool> promise;
         std::future<bool> future = promise.get_future();
         promise.set_value(true);
 
         auto shared_future = future.share();
-        auto ret = rclcpp::spin_until_future_complete(
-          node->get_node_base_interface(), shared_future, 1s);
+        auto ret = executor.spin_until_future_complete(shared_future, 1s);
         EXPECT_EQ(rclcpp::FutureReturnCode::SUCCESS, ret);
       };
     EXPECT_NO_THROW(check_spin_until_future_complete());

rclcpp/test/rclcpp/test_client_common.cpp

@@ -112,10 +112,12 @@ class TestAllClientTypesWithServer : public ::testing::Test
     auto req_id = client->async_send_request(request, std::move(callback));
 
     auto start = std::chrono::steady_clock::now();
+    rclcpp::executors::SingleThreadedExecutor executor;
+    executor.add_node(node);
     while (!received_response &&
       (std::chrono::steady_clock::now() - start) < timeout)
     {
-      rclcpp::spin_some(node);
+      executor.spin_some();
     }
 
     if (!received_response) {
@@ -356,10 +358,12 @@ TYPED_TEST(TestAllClientTypesWithServer, on_new_response_callback)
 
   this->template async_send_request<ClientType, test_msgs::srv::Empty::Request>(client, request);
   auto start = std::chrono::steady_clock::now();
+  rclcpp::executors::SingleThreadedExecutor executor;
+  executor.add_node(server_node);
   while (server_requests_count == 0 &&
     (std::chrono::steady_clock::now() - start) < std::chrono::seconds(10))
   {
-    rclcpp::spin_some(server_node);
+    executor.spin_some();
   }
 
   ASSERT_EQ(server_requests_count, 1u);
@@ -380,7 +384,7 @@ TYPED_TEST(TestAllClientTypesWithServer, on_new_response_callback)
   while (server_requests_count == 1 &&
     (std::chrono::steady_clock::now() - start) < std::chrono::seconds(10))
   {
-    rclcpp::spin_some(server_node);
+    executor.spin_some();
   }
 
   ASSERT_EQ(server_requests_count, 2u);
@@ -402,7 +406,7 @@ TYPED_TEST(TestAllClientTypesWithServer, on_new_response_callback)
   while (server_requests_count < 5 &&
     (std::chrono::steady_clock::now() - start) < std::chrono::seconds(10))
   {
-    rclcpp::spin_some(server_node);
+    executor.spin_some();
   }
 
   ASSERT_EQ(server_requests_count, 5u);
@@ -492,10 +496,12 @@ void client_async_send_request_callback_with_request(
   auto req_id = client->async_send_request(request, std::move(callback));
 
   auto start = std::chrono::steady_clock::now();
+  rclcpp::executors::SingleThreadedExecutor executor;
+  executor.add_node(node);
   while (!received_response &&
     (std::chrono::steady_clock::now() - start) < std::chrono::seconds(1))
   {
-    rclcpp::spin_some(node);
+    executor.spin_some();
   }
   EXPECT_TRUE(received_response);
   EXPECT_FALSE(client->remove_pending_request(req_id));
@@ -553,25 +559,29 @@ void client_qos_depth(rclcpp::Node::SharedPtr node)
   }
 
   auto start = std::chrono::steady_clock::now();
+  rclcpp::executors::SingleThreadedExecutor server_executor;
+  server_executor.add_node(server_node);
   while ((server_cb_count_ < client_requests) &&
     (std::chrono::steady_clock::now() - start) < 2s)
   {
-    rclcpp::spin_some(server_node);
+    server_executor.spin_some();
     std::this_thread::sleep_for(2ms);
   }
 
   EXPECT_GT(server_cb_count_, client_qos_profile.depth());
 
   start = std::chrono::steady_clock::now();
+  rclcpp::executors::SingleThreadedExecutor client_executor;
+  client_executor.add_node(node);
   while ((client_cb_count_ < client_qos_profile.depth()) &&
     (std::chrono::steady_clock::now() - start) < 1s)
   {
-    rclcpp::spin_some(node);
+    client_executor.spin_some();
   }
 
   // Spin an extra time to check if client QoS depth has been ignored,
   // so more client callbacks might be called than expected.
-  rclcpp::spin_some(node);
+  client_executor.spin_some();
 
   EXPECT_EQ(client_cb_count_, client_qos_profile.depth());
 }

rclcpp/test/rclcpp/test_create_timer.cpp

@@ -42,7 +42,9 @@ TEST(TestCreateTimer, timer_executes)
       timer->cancel();
     });
 
-  rclcpp::spin_some(node);
+  rclcpp::executors::SingleThreadedExecutor executor;
+  executor.add_node(node);
+  executor.spin_some();
 
   ASSERT_TRUE(got_callback);
   rclcpp::shutdown();