From 6240123323eb0479e980a6d6fd171bd4e133ea6b Mon Sep 17 00:00:00 2001 From: "Michael X. Grey" Date: Mon, 30 Aug 2021 23:52:51 +0800 Subject: [PATCH 1/7] Fix lifetime of context so it remains alive while its dependent node handles are still in use Signed-off-by: Michael X. Grey --- .../src/rclcpp/node_interfaces/node_base.cpp | 79 +++++++++++++++---- rclcpp/test/rclcpp/test_node.cpp | 25 ++++++ 2 files changed, 90 insertions(+), 14 deletions(-) diff --git a/rclcpp/src/rclcpp/node_interfaces/node_base.cpp b/rclcpp/src/rclcpp/node_interfaces/node_base.cpp index 5240c8c818..d84032ecb8 100644 --- a/rclcpp/src/rclcpp/node_interfaces/node_base.cpp +++ b/rclcpp/src/rclcpp/node_interfaces/node_base.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include "rclcpp/node_interfaces/node_base.hpp" @@ -31,6 +32,68 @@ using rclcpp::exceptions::throw_from_rcl_error; using rclcpp::node_interfaces::NodeBase; +namespace +{ +/// This class bundles together the lifetime of the rcl_node_t handle with the +/// lifetime of the RCL context. This ensures that the context will remain alive +/// for as long as the rcl_node_t handle is alive. +class NodeHandleWithContext +{ +public: + /// The make function returns a std::shared_ptr which is actually + /// an alias for a std::shared_ptr. There is no use for + /// accessing a NodeHandleWithContext instance directly, because its only job + /// is to couple together the lifetime of a context with the lifetime of a + /// node handle that depends on it. + static std::shared_ptr + make( + rclcpp::Context::SharedPtr context, + std::shared_ptr logging_mutex, + rcl_node_t * node_handle) + { + auto aliased_ptr = std::shared_ptr( + new NodeHandleWithContext( + std::move(context), + std::move(logging_mutex), + node_handle)); + + return std::shared_ptr(std::move(aliased_ptr), node_handle); + } + + ~NodeHandleWithContext() + { + std::lock_guard guard(*logging_mutex_); + // TODO(ivanpauno): Instead of mutually excluding rcl_node_fini with the global logger mutex, + // rcl_logging_rosout_fini_publisher_for_node could be decoupled from there and be called + // here directly. + if (rcl_node_fini(node_handle_) != RCL_RET_OK) { + RCUTILS_LOG_ERROR_NAMED( + "rclcpp", + "Error in destruction of rcl node handle: %s", rcl_get_error_string().str); + } + delete node_handle_; + } + +private: + /// The constructor is private because this class is only meant to be aliased + /// into a std::shared_ptr. + NodeHandleWithContext( + rclcpp::Context::SharedPtr context, + std::shared_ptr logging_mutex, + rcl_node_t * node_handle) + : context_(std::move(context)), + logging_mutex_(std::move(logging_mutex)), + node_handle_(node_handle) + { + // Do nothing + } + + rclcpp::Context::SharedPtr context_; + std::shared_ptr logging_mutex_; + rcl_node_t * node_handle_; +}; +} // anonymous namespace + NodeBase::NodeBase( const std::string & node_name, const std::string & namespace_, @@ -131,20 +194,8 @@ NodeBase::NodeBase( throw_from_rcl_error(ret, "failed to initialize rcl node"); } - node_handle_.reset( - rcl_node.release(), - [logging_mutex](rcl_node_t * node) -> void { - std::lock_guard guard(*logging_mutex); - // TODO(ivanpauno): Instead of mutually excluding rcl_node_fini with the global logger mutex, - // rcl_logging_rosout_fini_publisher_for_node could be decoupled from there and be called - // here directly. - if (rcl_node_fini(node) != RCL_RET_OK) { - RCUTILS_LOG_ERROR_NAMED( - "rclcpp", - "Error in destruction of rcl node handle: %s", rcl_get_error_string().str); - } - delete node; - }); + node_handle_ = NodeHandleWithContext::make( + context_, logging_mutex, rcl_node.release()); // Create the default callback group. using rclcpp::CallbackGroupType; diff --git a/rclcpp/test/rclcpp/test_node.cpp b/rclcpp/test/rclcpp/test_node.cpp index e034af16b2..d8c57c3605 100644 --- a/rclcpp/test/rclcpp/test_node.cpp +++ b/rclcpp/test/rclcpp/test_node.cpp @@ -97,6 +97,31 @@ TEST_F(TestNode, construction_and_destruction) { } } +/* + Testing lifecycles of subscriptions and publishers after node dies + */ +TEST_F(TestNode, pub_and_sub_lifecycles) { + using test_msgs::msg::Empty; + rclcpp::Publisher::SharedPtr pub; + rclcpp::Subscription::SharedPtr sub; + const auto callback = [](Empty::ConstSharedPtr) {}; + + { + // Create the node and context in a nested scope so that their + // std::shared_ptrs expire before we use pub and sub + auto context = std::make_shared(); + context->init(0, nullptr); + rclcpp::NodeOptions options; + options.context(context); + + const auto node = std::make_shared("my_node", "/ns", options); + pub = node->create_publisher("topic", 10); + sub = node->create_subscription("topic", 10, callback); + } + + pub->publish(Empty()); +} + TEST_F(TestNode, get_name_and_namespace) { { auto node = std::make_shared("my_node", "/ns"); From c454258a7c150405edca45a037c0350c3d443382 Mon Sep 17 00:00:00 2001 From: "Michael X. Grey" Date: Tue, 31 Aug 2021 10:00:08 +0800 Subject: [PATCH 2/7] Explicitly delete moving and assigning Signed-off-by: Michael X. Grey --- rclcpp/src/rclcpp/node_interfaces/node_base.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/rclcpp/src/rclcpp/node_interfaces/node_base.cpp b/rclcpp/src/rclcpp/node_interfaces/node_base.cpp index d84032ecb8..604a4200a0 100644 --- a/rclcpp/src/rclcpp/node_interfaces/node_base.cpp +++ b/rclcpp/src/rclcpp/node_interfaces/node_base.cpp @@ -60,6 +60,13 @@ class NodeHandleWithContext return std::shared_ptr(std::move(aliased_ptr), node_handle); } + // This class should not be copied or moved. It should only exist in the + // std::shared_ptr that it was originally provided in. + NodeHandleWithContext(const NodeHandleWithContext&) = delete; + NodeHandleWithContext(NodeHandleWithContext&&) = delete; + NodeHandleWithContext& operator=(const NodeHandleWithContext&) = delete; + NodeHandleWithContext& operator=(NodeHandleWithContext&&) = delete; + ~NodeHandleWithContext() { std::lock_guard guard(*logging_mutex_); From 8ea09ad1e0360f8fe4943ddd87ba95e4813c4490 Mon Sep 17 00:00:00 2001 From: "Michael X. Grey" Date: Tue, 31 Aug 2021 11:21:28 +0800 Subject: [PATCH 3/7] Satisfy uncrustify Signed-off-by: Michael X. Grey --- rclcpp/src/rclcpp/node_interfaces/node_base.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/rclcpp/src/rclcpp/node_interfaces/node_base.cpp b/rclcpp/src/rclcpp/node_interfaces/node_base.cpp index 604a4200a0..0f141c5868 100644 --- a/rclcpp/src/rclcpp/node_interfaces/node_base.cpp +++ b/rclcpp/src/rclcpp/node_interfaces/node_base.cpp @@ -62,10 +62,10 @@ class NodeHandleWithContext // This class should not be copied or moved. It should only exist in the // std::shared_ptr that it was originally provided in. - NodeHandleWithContext(const NodeHandleWithContext&) = delete; - NodeHandleWithContext(NodeHandleWithContext&&) = delete; - NodeHandleWithContext& operator=(const NodeHandleWithContext&) = delete; - NodeHandleWithContext& operator=(NodeHandleWithContext&&) = delete; + NodeHandleWithContext(const NodeHandleWithContext &) = delete; + NodeHandleWithContext(NodeHandleWithContext &&) = delete; + NodeHandleWithContext& operator=(const NodeHandleWithContext &) = delete; + NodeHandleWithContext& operator=(NodeHandleWithContext &&) = delete; ~NodeHandleWithContext() { From e3318e890cb20c65cecc8853cd1f4a8b733f9ceb Mon Sep 17 00:00:00 2001 From: "Michael X. Grey" Date: Tue, 31 Aug 2021 12:32:32 +0800 Subject: [PATCH 4/7] Fix more spacing complaints Signed-off-by: Michael X. Grey --- rclcpp/src/rclcpp/node_interfaces/node_base.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rclcpp/src/rclcpp/node_interfaces/node_base.cpp b/rclcpp/src/rclcpp/node_interfaces/node_base.cpp index 0f141c5868..ae49a5c8b9 100644 --- a/rclcpp/src/rclcpp/node_interfaces/node_base.cpp +++ b/rclcpp/src/rclcpp/node_interfaces/node_base.cpp @@ -64,8 +64,8 @@ class NodeHandleWithContext // std::shared_ptr that it was originally provided in. NodeHandleWithContext(const NodeHandleWithContext &) = delete; NodeHandleWithContext(NodeHandleWithContext &&) = delete; - NodeHandleWithContext& operator=(const NodeHandleWithContext &) = delete; - NodeHandleWithContext& operator=(NodeHandleWithContext &&) = delete; + NodeHandleWithContext & operator=(const NodeHandleWithContext &) = delete; + NodeHandleWithContext & operator=(NodeHandleWithContext &&) = delete; ~NodeHandleWithContext() { From c5ff0f4769bd4521a788d1b13f17d533f5b6582a Mon Sep 17 00:00:00 2001 From: "Michael X. Grey" Date: Tue, 14 Sep 2021 19:27:59 +0800 Subject: [PATCH 5/7] Fixing style Signed-off-by: Michael X. Grey --- .../src/rclcpp/node_interfaces/node_base.cpp | 34 +++++++++++-------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/rclcpp/src/rclcpp/node_interfaces/node_base.cpp b/rclcpp/src/rclcpp/node_interfaces/node_base.cpp index ae49a5c8b9..5c173bc6d4 100644 --- a/rclcpp/src/rclcpp/node_interfaces/node_base.cpp +++ b/rclcpp/src/rclcpp/node_interfaces/node_base.cpp @@ -34,17 +34,23 @@ using rclcpp::node_interfaces::NodeBase; namespace { -/// This class bundles together the lifetime of the rcl_node_t handle with the -/// lifetime of the RCL context. This ensures that the context will remain alive -/// for as long as the rcl_node_t handle is alive. +/// A class to manage the lifetime of a node handle and its context +/** + * This class bundles together the lifetime of the rcl_node_t handle with the + * lifetime of the RCL context. This ensures that the context will remain alive + * for as long as the rcl_node_t handle is alive. + */ class NodeHandleWithContext { public: - /// The make function returns a std::shared_ptr which is actually - /// an alias for a std::shared_ptr. There is no use for - /// accessing a NodeHandleWithContext instance directly, because its only job - /// is to couple together the lifetime of a context with the lifetime of a - /// node handle that depends on it. + /// Make an instance of a NodeHandleWithContext + /** + * The make function returns a std::shared_ptr which is actually + * an alias for a std::shared_ptr. There is no use for + * accessing a NodeHandleWithContext instance directly, because its only job + * is to couple together the lifetime of a context with the lifetime of a + * node handle that depends on it. + */ static std::shared_ptr make( rclcpp::Context::SharedPtr context, @@ -82,8 +88,9 @@ class NodeHandleWithContext } private: - /// The constructor is private because this class is only meant to be aliased - /// into a std::shared_ptr. + // The constructor is private because this class is only meant to be aliased + // into a std::shared_ptr and should not be constructed any other + // way. NodeHandleWithContext( rclcpp::Context::SharedPtr context, std::shared_ptr logging_mutex, @@ -91,9 +98,7 @@ class NodeHandleWithContext : context_(std::move(context)), logging_mutex_(std::move(logging_mutex)), node_handle_(node_handle) - { - // Do nothing - } + {} rclcpp::Context::SharedPtr context_; std::shared_ptr logging_mutex_; @@ -201,8 +206,7 @@ NodeBase::NodeBase( throw_from_rcl_error(ret, "failed to initialize rcl node"); } - node_handle_ = NodeHandleWithContext::make( - context_, logging_mutex, rcl_node.release()); + node_handle_ = NodeHandleWithContext::make(context_, logging_mutex, rcl_node.release()); // Create the default callback group. using rclcpp::CallbackGroupType; From c8716ac5e45d6173bab8ebea07a11968c6215caa Mon Sep 17 00:00:00 2001 From: "Michael X. Grey" Date: Wed, 22 Sep 2021 12:14:37 +0800 Subject: [PATCH 6/7] Provide a safe move constructor to avoid double-allocation Signed-off-by: Michael X. Grey --- .../src/rclcpp/node_interfaces/node_base.cpp | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/rclcpp/src/rclcpp/node_interfaces/node_base.cpp b/rclcpp/src/rclcpp/node_interfaces/node_base.cpp index 5c173bc6d4..9485ba0117 100644 --- a/rclcpp/src/rclcpp/node_interfaces/node_base.cpp +++ b/rclcpp/src/rclcpp/node_interfaces/node_base.cpp @@ -57,8 +57,8 @@ class NodeHandleWithContext std::shared_ptr logging_mutex, rcl_node_t * node_handle) { - auto aliased_ptr = std::shared_ptr( - new NodeHandleWithContext( + auto aliased_ptr = std::make_shared( + NodeHandleWithContext( std::move(context), std::move(logging_mutex), node_handle)); @@ -66,15 +66,29 @@ class NodeHandleWithContext return std::shared_ptr(std::move(aliased_ptr), node_handle); } - // This class should not be copied or moved. It should only exist in the + // This class should not be copied. It should only exist in the // std::shared_ptr that it was originally provided in. NodeHandleWithContext(const NodeHandleWithContext &) = delete; - NodeHandleWithContext(NodeHandleWithContext &&) = delete; NodeHandleWithContext & operator=(const NodeHandleWithContext &) = delete; NodeHandleWithContext & operator=(NodeHandleWithContext &&) = delete; + NodeHandleWithContext(NodeHandleWithContext && other) + : context_(std::move(other.context_)), + logging_mutex_(std::move(other.logging_mutex_)), + node_handle_(other.node_handle_) + { + other.node_handle_ = nullptr; + } + ~NodeHandleWithContext() { + if (!node_handle_) + { + // If the node_handle_ is null, then this object was moved-from. We don't + // need to do any cleanup. + return; + } + std::lock_guard guard(*logging_mutex_); // TODO(ivanpauno): Instead of mutually excluding rcl_node_fini with the global logger mutex, // rcl_logging_rosout_fini_publisher_for_node could be decoupled from there and be called From b17f7c5ffc1945f0f34303ef85c250165b97b033 Mon Sep 17 00:00:00 2001 From: "Michael X. Grey" Date: Wed, 22 Sep 2021 12:29:25 +0800 Subject: [PATCH 7/7] Fix uncrustify Signed-off-by: Michael X. Grey --- rclcpp/src/rclcpp/node_interfaces/node_base.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/rclcpp/src/rclcpp/node_interfaces/node_base.cpp b/rclcpp/src/rclcpp/node_interfaces/node_base.cpp index 9485ba0117..f6d851dc54 100644 --- a/rclcpp/src/rclcpp/node_interfaces/node_base.cpp +++ b/rclcpp/src/rclcpp/node_interfaces/node_base.cpp @@ -73,17 +73,16 @@ class NodeHandleWithContext NodeHandleWithContext & operator=(NodeHandleWithContext &&) = delete; NodeHandleWithContext(NodeHandleWithContext && other) - : context_(std::move(other.context_)), - logging_mutex_(std::move(other.logging_mutex_)), - node_handle_(other.node_handle_) + : context_(std::move(other.context_)), + logging_mutex_(std::move(other.logging_mutex_)), + node_handle_(other.node_handle_) { other.node_handle_ = nullptr; } ~NodeHandleWithContext() { - if (!node_handle_) - { + if (!node_handle_) { // If the node_handle_ is null, then this object was moved-from. We don't // need to do any cleanup. return;