From 6f6b5f2e3608eafeadb908aa4f6eded5b38c3043 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Mon, 8 Jan 2024 08:23:33 -0500 Subject: [PATCH] Stop storing the context in the guard condition. (#2400) * Stop storing the context in the guard condition. This was creating a circular reference between GuardCondition and Context, so that Context would never be cleaned up. Since we never really need the GuardCondition to know about its own Context, remove that part of the circular reference. While we are in here, we also change the get_context() lambda to a straight weak_ptr; there is no reason for the indirection since the context for the guard condition cannot change at runtime. We also remove the deprecated version of the get_notify_guard_condition(). That's because there is no way to properly implement it in the new scheme, and it seems to be unused outside of rclcpp. Finally, we add in a test that guarantees the use_count is what we expect when inside and leaving a scope, ensuring that contexts will properly be destroyed. Signed-off-by: Chris Lalancette --- rclcpp/include/rclcpp/callback_group.hpp | 17 ++-------- rclcpp/include/rclcpp/guard_condition.hpp | 8 +---- rclcpp/src/rclcpp/callback_group.cpp | 34 ++----------------- rclcpp/src/rclcpp/guard_condition.cpp | 15 +++----- .../src/rclcpp/node_interfaces/node_base.cpp | 7 +--- rclcpp/test/rclcpp/test_context.cpp | 22 ++++++++++++ rclcpp/test/rclcpp/test_guard_condition.cpp | 10 ------ 7 files changed, 35 insertions(+), 78 deletions(-) diff --git a/rclcpp/include/rclcpp/callback_group.hpp b/rclcpp/include/rclcpp/callback_group.hpp index 43c7daa888..3f60ab5cbc 100644 --- a/rclcpp/include/rclcpp/callback_group.hpp +++ b/rclcpp/include/rclcpp/callback_group.hpp @@ -129,8 +129,7 @@ class CallbackGroup * added to the executor in either case. * * \param[in] group_type The type of the callback group. - * \param[in] get_node_context Lambda to retrieve the node context when - * checking that the creating node is valid and using the guard condition. + * \param[in] context A weak pointer to the context associated with this callback group. * \param[in] automatically_add_to_executor_with_node A boolean that * determines whether a callback group is automatically added to an executor * with the node with which it is associated. @@ -138,7 +137,7 @@ class CallbackGroup RCLCPP_PUBLIC explicit CallbackGroup( CallbackGroupType group_type, - std::function get_node_context, + rclcpp::Context::WeakPtr context, bool automatically_add_to_executor_with_node = true); /// Default destructor. @@ -228,16 +227,6 @@ class CallbackGroup bool automatically_add_to_executor_with_node() const; - /// Retrieve the guard condition used to signal changes to this callback group. - /** - * \param[in] context_ptr context to use when creating the guard condition - * \return guard condition if it is valid, otherwise nullptr. - */ - [[deprecated("Use get_notify_guard_condition() without arguments")]] - RCLCPP_PUBLIC - rclcpp::GuardCondition::SharedPtr - get_notify_guard_condition(const rclcpp::Context::SharedPtr context_ptr); - /// Retrieve the guard condition used to signal changes to this callback group. /** * \return guard condition if it is valid, otherwise nullptr. @@ -297,7 +286,7 @@ class CallbackGroup std::shared_ptr notify_guard_condition_ = nullptr; std::recursive_mutex notify_guard_condition_mutex_; - std::function get_context_; + rclcpp::Context::WeakPtr context_; private: template diff --git a/rclcpp/include/rclcpp/guard_condition.hpp b/rclcpp/include/rclcpp/guard_condition.hpp index 350f306010..0a57bace22 100644 --- a/rclcpp/include/rclcpp/guard_condition.hpp +++ b/rclcpp/include/rclcpp/guard_condition.hpp @@ -48,7 +48,7 @@ class GuardCondition */ RCLCPP_PUBLIC explicit GuardCondition( - rclcpp::Context::SharedPtr context = + const rclcpp::Context::SharedPtr & context = rclcpp::contexts::get_global_default_context(), rcl_guard_condition_options_t guard_condition_options = rcl_guard_condition_get_default_options()); @@ -57,11 +57,6 @@ class GuardCondition virtual ~GuardCondition(); - /// Return the context used when creating this guard condition. - RCLCPP_PUBLIC - rclcpp::Context::SharedPtr - get_context() const; - /// Return the underlying rcl guard condition structure. RCLCPP_PUBLIC rcl_guard_condition_t & @@ -128,7 +123,6 @@ class GuardCondition set_on_trigger_callback(std::function callback); protected: - rclcpp::Context::SharedPtr context_; rcl_guard_condition_t rcl_guard_condition_; std::atomic in_use_by_wait_set_{false}; std::recursive_mutex reentrant_mutex_; diff --git a/rclcpp/src/rclcpp/callback_group.cpp b/rclcpp/src/rclcpp/callback_group.cpp index 77f6c87bd9..e2bc473d54 100644 --- a/rclcpp/src/rclcpp/callback_group.cpp +++ b/rclcpp/src/rclcpp/callback_group.cpp @@ -31,12 +31,12 @@ using rclcpp::CallbackGroupType; CallbackGroup::CallbackGroup( CallbackGroupType group_type, - std::function get_context, + rclcpp::Context::WeakPtr context, bool automatically_add_to_executor_with_node) : type_(group_type), associated_with_executor_(false), can_be_taken_from_(true), automatically_add_to_executor_with_node_(automatically_add_to_executor_with_node), - get_context_(get_context) + context_(context) {} CallbackGroup::~CallbackGroup() @@ -123,40 +123,12 @@ CallbackGroup::automatically_add_to_executor_with_node() const return automatically_add_to_executor_with_node_; } -// \TODO(mjcarroll) Deprecated, remove on tock -rclcpp::GuardCondition::SharedPtr -CallbackGroup::get_notify_guard_condition(const rclcpp::Context::SharedPtr context_ptr) -{ - std::lock_guard lock(notify_guard_condition_mutex_); - if (notify_guard_condition_ && context_ptr != notify_guard_condition_->get_context()) { - if (associated_with_executor_) { - trigger_notify_guard_condition(); - } - notify_guard_condition_ = nullptr; - } - - if (!notify_guard_condition_) { - notify_guard_condition_ = std::make_shared(context_ptr); - } - - return notify_guard_condition_; -} - rclcpp::GuardCondition::SharedPtr CallbackGroup::get_notify_guard_condition() { std::lock_guard lock(notify_guard_condition_mutex_); - if (!this->get_context_) { - throw std::runtime_error("Callback group was created without context and not passed context"); - } - auto context_ptr = this->get_context_(); + rclcpp::Context::SharedPtr context_ptr = context_.lock(); if (context_ptr && context_ptr->is_valid()) { - if (notify_guard_condition_ && context_ptr != notify_guard_condition_->get_context()) { - if (associated_with_executor_) { - trigger_notify_guard_condition(); - } - notify_guard_condition_ = nullptr; - } if (!notify_guard_condition_) { notify_guard_condition_ = std::make_shared(context_ptr); } diff --git a/rclcpp/src/rclcpp/guard_condition.cpp b/rclcpp/src/rclcpp/guard_condition.cpp index 627644e602..4f9a85d959 100644 --- a/rclcpp/src/rclcpp/guard_condition.cpp +++ b/rclcpp/src/rclcpp/guard_condition.cpp @@ -23,16 +23,17 @@ namespace rclcpp { GuardCondition::GuardCondition( - rclcpp::Context::SharedPtr context, + const rclcpp::Context::SharedPtr & context, rcl_guard_condition_options_t guard_condition_options) -: context_(context), rcl_guard_condition_{rcl_get_zero_initialized_guard_condition()} +: rcl_guard_condition_{rcl_get_zero_initialized_guard_condition()} { - if (!context_) { + if (!context) { throw std::invalid_argument("context argument unexpectedly nullptr"); } + rcl_ret_t ret = rcl_guard_condition_init( &this->rcl_guard_condition_, - context_->get_rcl_context().get(), + context->get_rcl_context().get(), guard_condition_options); if (RCL_RET_OK != ret) { rclcpp::exceptions::throw_from_rcl_error(ret, "failed to create guard condition"); @@ -53,12 +54,6 @@ GuardCondition::~GuardCondition() } } -rclcpp::Context::SharedPtr -GuardCondition::get_context() const -{ - return context_; -} - rcl_guard_condition_t & GuardCondition::get_rcl_guard_condition() { diff --git a/rclcpp/src/rclcpp/node_interfaces/node_base.cpp b/rclcpp/src/rclcpp/node_interfaces/node_base.cpp index 7b13880157..5648290654 100644 --- a/rclcpp/src/rclcpp/node_interfaces/node_base.cpp +++ b/rclcpp/src/rclcpp/node_interfaces/node_base.cpp @@ -218,14 +218,9 @@ NodeBase::create_callback_group( rclcpp::CallbackGroupType group_type, bool automatically_add_to_executor_with_node) { - auto weak_context = this->get_context()->weak_from_this(); - auto get_node_context = [weak_context]() -> rclcpp::Context::SharedPtr { - return weak_context.lock(); - }; - auto group = std::make_shared( group_type, - get_node_context, + context_->weak_from_this(), automatically_add_to_executor_with_node); std::lock_guard lock(callback_groups_mutex_); callback_groups_.push_back(group); diff --git a/rclcpp/test/rclcpp/test_context.cpp b/rclcpp/test/rclcpp/test_context.cpp index 9d736f9db2..c8779371fe 100644 --- a/rclcpp/test/rclcpp/test_context.cpp +++ b/rclcpp/test/rclcpp/test_context.cpp @@ -214,3 +214,25 @@ TEST(TestContext, check_on_shutdown_callback_order_after_del) { EXPECT_TRUE(result[0] == 1 && result[1] == 3 && result[2] == 4 && result[3] == 0); } + +// This test checks that contexts will be properly destroyed when leaving a scope, after a +// guard condition has been created. +TEST(TestContext, check_context_destroyed) { + rclcpp::Context::SharedPtr ctx; + { + ctx = std::make_shared(); + ctx->init(0, nullptr); + + auto group = std::make_shared( + rclcpp::CallbackGroupType::MutuallyExclusive, + ctx->weak_from_this(), + false); + + rclcpp::GuardCondition::SharedPtr gc = group->get_notify_guard_condition(); + ASSERT_NE(gc, nullptr); + + ASSERT_EQ(ctx.use_count(), 1u); + } + + ASSERT_EQ(ctx.use_count(), 1u); +} diff --git a/rclcpp/test/rclcpp/test_guard_condition.cpp b/rclcpp/test/rclcpp/test_guard_condition.cpp index 1e72264869..90ac9ae2d9 100644 --- a/rclcpp/test/rclcpp/test_guard_condition.cpp +++ b/rclcpp/test/rclcpp/test_guard_condition.cpp @@ -66,16 +66,6 @@ TEST_F(TestGuardCondition, construction_and_destruction) { } } -/* - * Testing context accessor. - */ -TEST_F(TestGuardCondition, get_context) { - { - auto gc = std::make_shared(); - gc->get_context(); - } -} - /* * Testing rcl guard condition accessor. */