Stop storing the context in the guard condition. (ros2#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 <[email protected]> Signed-off-by: Oren Bell <[email protected]>
nightduck · Jan 25, 2024 · 52ca2d8 · 52ca2d8
1 parent fdcabc7
commit 52ca2d8
Show file tree

Hide file tree

Showing 7 changed files with 35 additions and 78 deletions.
diff --git a/rclcpp/include/rclcpp/callback_group.hpp b/rclcpp/include/rclcpp/callback_group.hpp
@@ -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.
@@ -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<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>

diff --git 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<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_;

diff --git 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<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()
@@ -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);
     }

diff --git 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
@@ -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);

diff --git 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<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);
+}
diff --git 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<rclcpp::GuardCondition>();
-    gc->get_context();
-  }
-}
-
 /*
  * Testing rcl guard condition accessor.
  */