deprecate rclcpp::spin_some and rclcpp::spin_all

Author: alsora

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/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

@@ -25,12 +25,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 +34,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 +50,30 @@ rclcpp::spin(rclcpp::Node::SharedPtr node_ptr)
 {
   rclcpp::spin(node_ptr->get_node_base_interface());
 }
+
+#if !defined(_WIN32)
+# pragma GCC diagnostic push
+# pragma GCC diagnostic ignored "-Wdeprecated-declarations"
+#else  // !defined(_WIN32)
+# pragma warning(push)
+# pragma warning(disable: 4996)
+#endif
+
+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::SharedPtr node_ptr)
+{
+  rclcpp::spin_some(node_ptr->get_node_base_interface());
+}
+
+// remove warning suppression
+#if !defined(_WIN32)
+# pragma GCC diagnostic pop
+#else  // !defined(_WIN32)
+# pragma warning(pop)
+#endif

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();

rclcpp/test/rclcpp/test_generic_pubsub.cpp

@@ -77,8 +77,10 @@ class RclcppGenericNodeFixture : public Test
         counter++;
       });
 
+    rclcpp::executors::SingleThreadedExecutor executor;
+    executor.add_node(node_);
     while (counter < expected_recv_msg_count) {
-      rclcpp::spin_some(node_);
+      executor.spin_some();
     }
     return messages;
   }
@@ -106,11 +108,13 @@ class RclcppGenericNodeFixture : public Test
   {
     using clock = std::chrono::system_clock;
     auto start = clock::now();
+    rclcpp::executors::SingleThreadedExecutor executor;
+    executor.add_node(node_);
     while (!condition()) {
       if ((clock::now() - start) > timeout) {
         return false;
       }
-      rclcpp::spin_some(node_);
+      executor.spin_some();
     }
     return true;
   }

rclcpp/test/rclcpp/test_generic_service.cpp

@@ -372,16 +372,18 @@ TEST_F(TestGenericService, generic_service_qos_depth) {
   }
 
   auto start = std::chrono::steady_clock::now();
+  rclcpp::executors::SingleThreadedExecutor executor;
+  executor.add_node(server_node);
   while ((server_cb_count_ < server_qos_profile.depth()) &&
     (std::chrono::steady_clock::now() - start) < 1s)
   {
-    rclcpp::spin_some(server_node);
+    executor.spin_some();
     std::this_thread::sleep_for(1ms);
   }
 
   // Spin an extra time to check if server QoS depth has been ignored,
   // so more server responses might be processed than expected.
-  rclcpp::spin_some(server_node);
+  executor.spin_some();
 
   EXPECT_EQ(server_cb_count_, server_qos_profile.depth());
 }