From b9846ac92b7925bd27bb442b493ee904a59c6816 Mon Sep 17 00:00:00 2001 From: Hans-Joachim Krauch Date: Wed, 26 Jun 2024 18:44:25 +0000 Subject: [PATCH 1/6] fix rolling test failure by using global executor --- ros2_foxglove_bridge/tests/smoke_test.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/ros2_foxglove_bridge/tests/smoke_test.cpp b/ros2_foxglove_bridge/tests/smoke_test.cpp index 0d8f83f..b398300 100644 --- a/ros2_foxglove_bridge/tests/smoke_test.cpp +++ b/ros2_foxglove_bridge/tests/smoke_test.cpp @@ -21,6 +21,8 @@ constexpr uint8_t HELLO_WORLD_BINARY[] = {0, 1, 0, 0, 12, 0, 0, 0, constexpr auto ONE_SECOND = std::chrono::seconds(1); constexpr auto DEFAULT_TIMEOUT = std::chrono::seconds(10); +rclcpp::executors::SingleThreadedExecutor::SharedPtr executor = nullptr; + class ParameterTest : public ::testing::Test { public: using PARAM_1_TYPE = std::string; @@ -266,8 +268,7 @@ TEST(SmokeTest, testPublishing) { advertisement.topic, 10, [&msgPromise](std::shared_ptr msg) { msgPromise.set_value(msg->data); }); - rclcpp::executors::SingleThreadedExecutor executor; - executor.add_node(node); + executor->add_node(node); // Set up the client, advertise and publish the binary message auto client = std::make_shared>(); @@ -283,8 +284,8 @@ TEST(SmokeTest, testPublishing) { client->unadvertise({advertisement.channelId}); // Ensure that we have received the correct message via our ROS subscriber - const auto ret = executor.spin_until_future_complete(msgFuture, ONE_SECOND); - ASSERT_EQ(rclcpp::FutureReturnCode::SUCCESS, ret); + ASSERT_EQ(std::future_status::ready, msgFuture.wait_for(DEFAULT_TIMEOUT)); + executor->remove_node(node); EXPECT_EQ("hello world", msgFuture.get()); } @@ -732,9 +733,7 @@ int main(int argc, char** argv) { testing::InitGoogleTest(&argc, argv); rclcpp::init(argc, argv); - const size_t numThreads = 2; - auto executor = - rclcpp::executors::MultiThreadedExecutor::make_shared(rclcpp::ExecutorOptions{}, numThreads); + executor = rclcpp::executors::SingleThreadedExecutor::make_shared(); rclcpp_components::ComponentManager componentManager(executor, "ComponentManager"); const auto componentResources = componentManager.get_component_resources("foxglove_bridge"); @@ -752,13 +751,14 @@ int main(int argc, char** argv) { auto node = componentFactory->create_node_instance(nodeOptions); executor->add_node(node.get_node_base_interface()); - std::thread spinnerThread([&executor]() { + std::thread spinnerThread([]() { executor->spin(); }); const auto testResult = RUN_ALL_TESTS(); executor->cancel(); spinnerThread.join(); + executor.reset(); rclcpp::shutdown(); return testResult; From 2b35879c385f0931edd8fb404144a1e493af40a6 Mon Sep 17 00:00:00 2001 From: Hans-Joachim Krauch Date: Wed, 26 Jun 2024 19:08:51 +0000 Subject: [PATCH 2/6] avoid global executor --- ros2_foxglove_bridge/tests/smoke_test.cpp | 29 ++++++++++++++++++----- 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/ros2_foxglove_bridge/tests/smoke_test.cpp b/ros2_foxglove_bridge/tests/smoke_test.cpp index b398300..d7c6b5f 100644 --- a/ros2_foxglove_bridge/tests/smoke_test.cpp +++ b/ros2_foxglove_bridge/tests/smoke_test.cpp @@ -21,7 +21,24 @@ constexpr uint8_t HELLO_WORLD_BINARY[] = {0, 1, 0, 0, 12, 0, 0, 0, constexpr auto ONE_SECOND = std::chrono::seconds(1); constexpr auto DEFAULT_TIMEOUT = std::chrono::seconds(10); -rclcpp::executors::SingleThreadedExecutor::SharedPtr executor = nullptr; +class TestWithExecutor : public testing::Test { +protected: + TestWithExecutor() { + this->_executorThread = std::thread([this]() { + this->executor.spin(); + }); + } + + ~TestWithExecutor() override { + this->executor.cancel(); + this->_executorThread.join(); + } + + rclcpp::executors::SingleThreadedExecutor executor; + +private: + std::thread _executorThread; +}; class ParameterTest : public ::testing::Test { public: @@ -253,7 +270,7 @@ TEST(SmokeTest, testSubscriptionParallel) { } } -TEST(SmokeTest, testPublishing) { +TEST_F(TestWithExecutor, testPublishing) { foxglove::ClientAdvertisement advertisement; advertisement.channelId = 1; advertisement.topic = "/foo"; @@ -268,7 +285,7 @@ TEST(SmokeTest, testPublishing) { advertisement.topic, 10, [&msgPromise](std::shared_ptr msg) { msgPromise.set_value(msg->data); }); - executor->add_node(node); + this->executor.add_node(node); // Set up the client, advertise and publish the binary message auto client = std::make_shared>(); @@ -285,7 +302,7 @@ TEST(SmokeTest, testPublishing) { // Ensure that we have received the correct message via our ROS subscriber ASSERT_EQ(std::future_status::ready, msgFuture.wait_for(DEFAULT_TIMEOUT)); - executor->remove_node(node); + this->executor.remove_node(node); EXPECT_EQ("hello world", msgFuture.get()); } @@ -733,7 +750,7 @@ int main(int argc, char** argv) { testing::InitGoogleTest(&argc, argv); rclcpp::init(argc, argv); - executor = rclcpp::executors::SingleThreadedExecutor::make_shared(); + auto executor = rclcpp::executors::SingleThreadedExecutor::make_shared(); rclcpp_components::ComponentManager componentManager(executor, "ComponentManager"); const auto componentResources = componentManager.get_component_resources("foxglove_bridge"); @@ -751,7 +768,7 @@ int main(int argc, char** argv) { auto node = componentFactory->create_node_instance(nodeOptions); executor->add_node(node.get_node_base_interface()); - std::thread spinnerThread([]() { + std::thread spinnerThread([&executor]() { executor->spin(); }); From 4fd843f6141be9a06222cb474a61e2919be74b42 Mon Sep 17 00:00:00 2001 From: Hans-Joachim Krauch Date: Wed, 26 Jun 2024 19:15:09 +0000 Subject: [PATCH 3/6] Derive ParameterTest from TestWithExecutor --- ros2_foxglove_bridge/tests/smoke_test.cpp | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/ros2_foxglove_bridge/tests/smoke_test.cpp b/ros2_foxglove_bridge/tests/smoke_test.cpp index d7c6b5f..901ce6d 100644 --- a/ros2_foxglove_bridge/tests/smoke_test.cpp +++ b/ros2_foxglove_bridge/tests/smoke_test.cpp @@ -40,7 +40,7 @@ class TestWithExecutor : public testing::Test { std::thread _executorThread; }; -class ParameterTest : public ::testing::Test { +class ParameterTest : public TestWithExecutor { public: using PARAM_1_TYPE = std::string; inline static const std::string NODE_1_NAME = "node_1"; @@ -82,25 +82,15 @@ class ParameterTest : public ::testing::Test { _paramNode2->declare_parameter(PARAM_3_NAME, PARAM_3_DEFAULT_VALUE); _paramNode2->declare_parameter(PARAM_4_NAME, PARAM_4_DEFAULT_VALUE); - _executor.add_node(_paramNode1); - _executor.add_node(_paramNode2); - _executorThread = std::thread([this]() { - _executor.spin(); - }); + executor.add_node(_paramNode1); + executor.add_node(_paramNode2); _wsClient = std::make_shared>(); ASSERT_EQ(std::future_status::ready, _wsClient->connect(URI).wait_for(DEFAULT_TIMEOUT)); } - void TearDown() override { - _executor.cancel(); - _executorThread.join(); - } - - rclcpp::executors::SingleThreadedExecutor _executor; rclcpp::Node::SharedPtr _paramNode1; rclcpp::Node::SharedPtr _paramNode2; - std::thread _executorThread; std::shared_ptr> _wsClient; }; From 6efc532270452f2c6f6fbe9e1129503d3f3e63eb Mon Sep 17 00:00:00 2001 From: Hans-Joachim Krauch Date: Wed, 26 Jun 2024 20:59:19 +0000 Subject: [PATCH 4/6] call remove_node on teardown, cleanup --- ros2_foxglove_bridge/tests/smoke_test.cpp | 30 +++++++++-------------- 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/ros2_foxglove_bridge/tests/smoke_test.cpp b/ros2_foxglove_bridge/tests/smoke_test.cpp index 901ce6d..cb5b09c 100644 --- a/ros2_foxglove_bridge/tests/smoke_test.cpp +++ b/ros2_foxglove_bridge/tests/smoke_test.cpp @@ -89,12 +89,17 @@ class ParameterTest : public TestWithExecutor { ASSERT_EQ(std::future_status::ready, _wsClient->connect(URI).wait_for(DEFAULT_TIMEOUT)); } + void TearDown() override { + executor.remove_node(_paramNode1); + executor.remove_node(_paramNode2); + } + rclcpp::Node::SharedPtr _paramNode1; rclcpp::Node::SharedPtr _paramNode2; std::shared_ptr> _wsClient; }; -class ServiceTest : public ::testing::Test { +class ServiceTest : public TestWithExecutor { public: inline static const std::string SERVICE_NAME = "/foo_service"; @@ -107,26 +112,19 @@ class ServiceTest : public ::testing::Test { res->message = "hello"; res->success = req->data; }); - - _executor.add_node(_node); - _executorThread = std::thread([this]() { - _executor.spin(); - }); + executor.add_node(_node); } void TearDown() override { - _executor.cancel(); - _executorThread.join(); + executor.remove_node(_node); } - rclcpp::executors::SingleThreadedExecutor _executor; rclcpp::Node::SharedPtr _node; rclcpp::ServiceBase::SharedPtr _service; - std::thread _executorThread; std::shared_ptr> _wsClient; }; -class ExistingPublisherTest : public ::testing::Test { +class ExistingPublisherTest : public TestWithExecutor { public: inline static const std::string TOPIC_NAME = "/some_topic"; @@ -135,21 +133,15 @@ class ExistingPublisherTest : public ::testing::Test { _node = rclcpp::Node::make_shared("node"); _publisher = _node->create_publisher(TOPIC_NAME, rclcpp::SystemDefaultsQoS()); - _executor.add_node(_node); - _executorThread = std::thread([this]() { - _executor.spin(); - }); + executor.add_node(_node); } void TearDown() override { - _executor.cancel(); - _executorThread.join(); + executor.remove_node(_node); } - rclcpp::executors::SingleThreadedExecutor _executor; rclcpp::Node::SharedPtr _node; rclcpp::PublisherBase::SharedPtr _publisher; - std::thread _executorThread; }; template From 80a5bbcc8d50acb9e405a4a32020c545d2b1dc1e Mon Sep 17 00:00:00 2001 From: Hans-Joachim Krauch Date: Thu, 27 Jun 2024 05:06:08 +0000 Subject: [PATCH 5/6] fix bridge node not shutting down unless rclcpp::shutdown is called --- .../include/foxglove_bridge/ros2_foxglove_bridge.hpp | 1 + ros2_foxglove_bridge/src/ros2_foxglove_bridge.cpp | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/ros2_foxglove_bridge/include/foxglove_bridge/ros2_foxglove_bridge.hpp b/ros2_foxglove_bridge/include/foxglove_bridge/ros2_foxglove_bridge.hpp index 4a09ab6..3db76bb 100644 --- a/ros2_foxglove_bridge/include/foxglove_bridge/ros2_foxglove_bridge.hpp +++ b/ros2_foxglove_bridge/include/foxglove_bridge/ros2_foxglove_bridge.hpp @@ -82,6 +82,7 @@ class FoxgloveBridge : public rclcpp::Node { std::atomic _subscribeGraphUpdates = false; bool _includeHidden = false; std::unique_ptr _fetchAssetQueue; + std::atomic _shuttingDown = false; void subscribeConnectionGraph(bool subscribe); diff --git a/ros2_foxglove_bridge/src/ros2_foxglove_bridge.cpp b/ros2_foxglove_bridge/src/ros2_foxglove_bridge.cpp index 19b2c7d..6c9d499 100644 --- a/ros2_foxglove_bridge/src/ros2_foxglove_bridge.cpp +++ b/ros2_foxglove_bridge/src/ros2_foxglove_bridge.cpp @@ -135,6 +135,7 @@ FoxgloveBridge::FoxgloveBridge(const rclcpp::NodeOptions& options) } FoxgloveBridge::~FoxgloveBridge() { + _shuttingDown = true; RCLCPP_INFO(this->get_logger(), "Shutting down %s", this->get_name()); if (_rosgraphPollThread) { _rosgraphPollThread->join(); @@ -148,7 +149,7 @@ void FoxgloveBridge::rosgraphPollThread() { updateAdvertisedServices(); auto graphEvent = this->get_graph_event(); - while (rclcpp::ok()) { + while (!_shuttingDown && rclcpp::ok()) { try { this->wait_for_graph_change(graphEvent, 200ms); bool triggered = graphEvent->check_and_clear(); From dc03b84edebc2b47c2de70f357f7a02efaa6aa1e Mon Sep 17 00:00:00 2001 From: Hans-Joachim Krauch Date: Thu, 27 Jun 2024 05:07:13 +0000 Subject: [PATCH 6/6] simplify smoke_test by using bridge node directly --- CMakeLists.txt | 2 +- ros2_foxglove_bridge/tests/smoke_test.cpp | 26 +++++++---------------- 2 files changed, 9 insertions(+), 19 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index f0adae8..06259c4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -241,7 +241,7 @@ elseif(ROS_BUILD_TYPE STREQUAL "ament_cmake") ament_add_gtest(smoke_test ros2_foxglove_bridge/tests/smoke_test.cpp) ament_target_dependencies(smoke_test rclcpp rclcpp_components std_msgs std_srvs) - target_link_libraries(smoke_test foxglove_bridge_base) + target_link_libraries(smoke_test foxglove_bridge_component) enable_strict_compiler_warnings(smoke_test) ament_add_gtest(utils_test ros2_foxglove_bridge/tests/utils_test.cpp) diff --git a/ros2_foxglove_bridge/tests/smoke_test.cpp b/ros2_foxglove_bridge/tests/smoke_test.cpp index cb5b09c..8112e78 100644 --- a/ros2_foxglove_bridge/tests/smoke_test.cpp +++ b/ros2_foxglove_bridge/tests/smoke_test.cpp @@ -4,11 +4,11 @@ #include #include -#include #include #include #include +#include #include #include @@ -732,33 +732,23 @@ int main(int argc, char** argv) { testing::InitGoogleTest(&argc, argv); rclcpp::init(argc, argv); - auto executor = rclcpp::executors::SingleThreadedExecutor::make_shared(); - - rclcpp_components::ComponentManager componentManager(executor, "ComponentManager"); - const auto componentResources = componentManager.get_component_resources("foxglove_bridge"); - - if (componentResources.empty()) { - RCLCPP_INFO(componentManager.get_logger(), "No loadable resources found"); - return EXIT_FAILURE; - } - - auto componentFactory = componentManager.create_component_factory(componentResources.front()); + rclcpp::executors::SingleThreadedExecutor executor; rclcpp::NodeOptions nodeOptions; // Explicitly allow file:// asset URIs for testing purposes. nodeOptions.append_parameter_override("asset_uri_allowlist", std::vector({"file://.*"})); - auto node = componentFactory->create_node_instance(nodeOptions); - executor->add_node(node.get_node_base_interface()); + foxglove_bridge::FoxgloveBridge node(nodeOptions); + executor.add_node(node.get_node_base_interface()); std::thread spinnerThread([&executor]() { - executor->spin(); + executor.spin(); }); const auto testResult = RUN_ALL_TESTS(); - executor->cancel(); + + executor.cancel(); spinnerThread.join(); - executor.reset(); - rclcpp::shutdown(); + executor.remove_node(node.get_node_base_interface()); return testResult; }