From 6737773a5d8831957676586148056cf5307a7da7 Mon Sep 17 00:00:00 2001 From: Tomoya Fujita Date: Wed, 12 Jun 2024 09:51:49 -0700 Subject: [PATCH] revert call shutdown in LifecycleNode destructor (Humble) (#2560) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Revert "lifecycle node dtor shutdown should be called only in primary state. (#2544)" This reverts commit 595badb55ced4e802e90c3d0959c6f0f8475f9b9. Signed-off-by: Tomoya Fujita * Revert "call shutdown in LifecycleNode dtor to avoid leaving the device in un… (#2450) (#2491)" This reverts commit 0f9604d1b712b154cad32dfe4b4a6bfed2924436. Signed-off-by: Tomoya Fujita --------- Signed-off-by: Tomoya Fujita --- rclcpp_lifecycle/src/lifecycle_node.cpp | 29 ---- rclcpp_lifecycle/test/test_lifecycle_node.cpp | 140 ------------------ 2 files changed, 169 deletions(-) diff --git a/rclcpp_lifecycle/src/lifecycle_node.cpp b/rclcpp_lifecycle/src/lifecycle_node.cpp index 4b0bf53a2c..333edf41b8 100644 --- a/rclcpp_lifecycle/src/lifecycle_node.cpp +++ b/rclcpp_lifecycle/src/lifecycle_node.cpp @@ -133,34 +133,6 @@ LifecycleNode::LifecycleNode( LifecycleNode::~LifecycleNode() { - auto current_state = LifecycleNode::get_current_state().id(); - // shutdown if necessary to avoid leaving the device in any other primary state - if (current_state < lifecycle_msgs::msg::State::PRIMARY_STATE_FINALIZED) { - if (node_base_->get_context()->is_valid()) { - 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()); - } - } else { - // TODO(fujitatomoya): consider when context is gracefully shutdown before. - RCLCPP_DEBUG( - rclcpp::get_logger("rclcpp_lifecycle"), - "Context invalid error in destruction of LifecycleNode: Node still in transition state(%u)", - current_state); - } - } else if (current_state > lifecycle_msgs::msg::State::PRIMARY_STATE_FINALIZED) { - RCLCPP_WARN( - rclcpp::get_logger("rclcpp_lifecycle"), - "Shutdown error in destruction of LifecycleNode: Node still in transition state(%u)", - current_state); - } - // release sub-interfaces in an order that allows them to consult with node_base during tear-down node_waitables_.reset(); node_time_source_.reset(); @@ -171,7 +143,6 @@ LifecycleNode::~LifecycleNode() node_timers_.reset(); node_logging_.reset(); node_graph_.reset(); - node_base_.reset(); } const char * diff --git a/rclcpp_lifecycle/test/test_lifecycle_node.cpp b/rclcpp_lifecycle/test/test_lifecycle_node.cpp index 5fbe733eb2..5a3054781b 100644 --- a/rclcpp_lifecycle/test/test_lifecycle_node.cpp +++ b/rclcpp_lifecycle/test/test_lifecycle_node.cpp @@ -435,146 +435,6 @@ 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("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("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("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("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("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("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("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("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>("testnode");