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