Skip to content

Commit

Permalink
Stop storing the context in the guard condition. (#2400)
Browse files Browse the repository at this point in the history
* 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 <clalancette@gmail.com>
  • Loading branch information
clalancette authored Jan 8, 2024
1 parent 126d517 commit 6f6b5f2
Show file tree
Hide file tree
Showing 7 changed files with 35 additions and 78 deletions.
17 changes: 3 additions & 14 deletions rclcpp/include/rclcpp/callback_group.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -129,16 +129,15 @@ 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.
*/
RCLCPP_PUBLIC
explicit CallbackGroup(
CallbackGroupType group_type,
std::function<rclcpp::Context::SharedPtr(void)> get_node_context,
rclcpp::Context::WeakPtr context,
bool automatically_add_to_executor_with_node = true);

/// Default destructor.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -297,7 +286,7 @@ class CallbackGroup
std::shared_ptr<rclcpp::GuardCondition> notify_guard_condition_ = nullptr;
std::recursive_mutex notify_guard_condition_mutex_;

std::function<rclcpp::Context::SharedPtr(void)> get_context_;
rclcpp::Context::WeakPtr context_;

private:
template<typename TypeT, typename Function>
Expand Down
8 changes: 1 addition & 7 deletions rclcpp/include/rclcpp/guard_condition.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -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 &
Expand Down Expand Up @@ -128,7 +123,6 @@ class GuardCondition
set_on_trigger_callback(std::function<void(size_t)> callback);

protected:
rclcpp::Context::SharedPtr context_;
rcl_guard_condition_t rcl_guard_condition_;
std::atomic<bool> in_use_by_wait_set_{false};
std::recursive_mutex reentrant_mutex_;
Expand Down
34 changes: 3 additions & 31 deletions rclcpp/src/rclcpp/callback_group.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,12 @@ using rclcpp::CallbackGroupType;

CallbackGroup::CallbackGroup(
CallbackGroupType group_type,
std::function<rclcpp::Context::SharedPtr(void)> 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()
Expand Down Expand Up @@ -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<std::recursive_mutex> 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<rclcpp::GuardCondition>(context_ptr);
}

return notify_guard_condition_;
}

rclcpp::GuardCondition::SharedPtr
CallbackGroup::get_notify_guard_condition()
{
std::lock_guard<std::recursive_mutex> 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<rclcpp::GuardCondition>(context_ptr);
}
Expand Down
15 changes: 5 additions & 10 deletions rclcpp/src/rclcpp/guard_condition.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -53,12 +54,6 @@ GuardCondition::~GuardCondition()
}
}

rclcpp::Context::SharedPtr
GuardCondition::get_context() const
{
return context_;
}

rcl_guard_condition_t &
GuardCondition::get_rcl_guard_condition()
{
Expand Down
7 changes: 1 addition & 6 deletions rclcpp/src/rclcpp/node_interfaces/node_base.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<rclcpp::CallbackGroup>(
group_type,
get_node_context,
context_->weak_from_this(),
automatically_add_to_executor_with_node);
std::lock_guard<std::mutex> lock(callback_groups_mutex_);
callback_groups_.push_back(group);
Expand Down
22 changes: 22 additions & 0 deletions rclcpp/test/rclcpp/test_context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<rclcpp::Context>();
ctx->init(0, nullptr);

auto group = std::make_shared<rclcpp::CallbackGroup>(
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);
}
10 changes: 0 additions & 10 deletions rclcpp/test/rclcpp/test_guard_condition.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -66,16 +66,6 @@ TEST_F(TestGuardCondition, construction_and_destruction) {
}
}

/*
* Testing context accessor.
*/
TEST_F(TestGuardCondition, get_context) {
{
auto gc = std::make_shared<rclcpp::GuardCondition>();
gc->get_context();
}
}

/*
* Testing rcl guard condition accessor.
*/
Expand Down

0 comments on commit 6f6b5f2

Please sign in to comment.