Pass GuardCondition ptr instead of ref to remove_guard_condition

Before the api was taking a reference to a guard condition, then getting the address of it. But if a node had expired, we can't get the orig gc dereferencing a pointer, nor can we get an address of an out-of-scope guard condition. Signed-off-by: Mauro Passerino <[email protected]>
ros2 · Aug 19, 2021 · 9bff9c3 · 9bff9c3
1 parent ab313f2
commit 9bff9c3
Show file tree

Hide file tree

Showing 4 changed files with 16 additions and 16 deletions.
diff --git a/rclcpp/include/rclcpp/memory_strategy.hpp b/rclcpp/include/rclcpp/memory_strategy.hpp
@@ -68,7 +68,7 @@ class RCLCPP_PUBLIC MemoryStrategy
   add_guard_condition(const rclcpp::GuardCondition & guard_condition) = 0;
 
   virtual void
-  remove_guard_condition(const rclcpp::GuardCondition & guard_condition) = 0;
+  remove_guard_condition(const rclcpp::GuardCondition * guard_condition) = 0;
 
   virtual void
   get_next_subscription(

diff --git a/rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp b/rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp
@@ -72,10 +72,10 @@ class AllocatorMemoryStrategy : public memory_strategy::MemoryStrategy
     guard_conditions_.push_back(&guard_condition);
   }
 
-  void remove_guard_condition(const rclcpp::GuardCondition & guard_condition) override
+  void remove_guard_condition(const rclcpp::GuardCondition * guard_condition) override
   {
     for (auto it = guard_conditions_.begin(); it != guard_conditions_.end(); ++it) {
-      if (*it == &guard_condition) {
+      if (*it == guard_condition) {
         guard_conditions_.erase(it);
         break;
       }

diff --git a/rclcpp/src/rclcpp/executor.cpp b/rclcpp/src/rclcpp/executor.cpp
@@ -107,8 +107,8 @@ Executor::~Executor()
   weak_groups_to_nodes_associated_with_executor_.clear();
   weak_groups_to_nodes_.clear();
   for (const auto & pair : weak_nodes_to_guard_conditions_) {
-    auto & guard_condition = pair.second;
-    memory_strategy_->remove_guard_condition(*guard_condition);
+    auto guard_condition = pair.second;
+    memory_strategy_->remove_guard_condition(guard_condition);
   }
   weak_nodes_to_guard_conditions_.clear();
 
@@ -120,8 +120,8 @@ Executor::~Executor()
     rcl_reset_error();
   }
   // Remove and release the sigint guard condition
-  memory_strategy_->remove_guard_condition(*shutdown_guard_condition_.get());
-  memory_strategy_->remove_guard_condition(interrupt_guard_condition_);
+  memory_strategy_->remove_guard_condition(shutdown_guard_condition_.get());
+  memory_strategy_->remove_guard_condition(&interrupt_guard_condition_);
 
   // Remove shutdown callback handle registered to Context
   if (!context_->remove_on_shutdown_callback(shutdown_callback_handle_)) {
@@ -310,7 +310,7 @@ Executor::remove_callback_group_from_map(
                   "Failed to trigger guard condition on callback group remove: ") + ex.what());
       }
     }
-    memory_strategy_->remove_guard_condition(node_ptr->get_notify_guard_condition());
+    memory_strategy_->remove_guard_condition(&node_ptr->get_notify_guard_condition());
   }
 }
 
@@ -702,9 +702,9 @@ Executor::wait_for_work(std::chrono::nanoseconds timeout)
           invalid_group_ptrs.push_back(weak_group_ptr);
           auto node_guard_pair = weak_nodes_to_guard_conditions_.find(weak_node_ptr);
           if (node_guard_pair != weak_nodes_to_guard_conditions_.end()) {
-            const auto & guard_condition = node_guard_pair->second;
+            auto guard_condition = node_guard_pair->second;
             weak_nodes_to_guard_conditions_.erase(weak_node_ptr);
-            memory_strategy_->remove_guard_condition(*guard_condition);
+            memory_strategy_->remove_guard_condition(guard_condition);
           }
         }
       }

diff --git a/rclcpp/test/rclcpp/strategies/test_allocator_memory_strategy.cpp b/rclcpp/test/rclcpp/strategies/test_allocator_memory_strategy.cpp
@@ -470,15 +470,15 @@ TEST_F(TestAllocatorMemoryStrategy, add_remove_guard_conditions) {
   EXPECT_NO_THROW(allocator_memory_strategy()->add_guard_condition(guard_condition3));
   EXPECT_EQ(3u, allocator_memory_strategy()->number_of_guard_conditions());
 
-  EXPECT_NO_THROW(allocator_memory_strategy()->remove_guard_condition(guard_condition1));
-  EXPECT_NO_THROW(allocator_memory_strategy()->remove_guard_condition(guard_condition2));
-  EXPECT_NO_THROW(allocator_memory_strategy()->remove_guard_condition(guard_condition3));
+  EXPECT_NO_THROW(allocator_memory_strategy()->remove_guard_condition(&guard_condition1));
+  EXPECT_NO_THROW(allocator_memory_strategy()->remove_guard_condition(&guard_condition2));
+  EXPECT_NO_THROW(allocator_memory_strategy()->remove_guard_condition(&guard_condition3));
   EXPECT_EQ(0u, allocator_memory_strategy()->number_of_guard_conditions());
 
   // Removing second time should have no effect
-  EXPECT_NO_THROW(allocator_memory_strategy()->remove_guard_condition(guard_condition1));
-  EXPECT_NO_THROW(allocator_memory_strategy()->remove_guard_condition(guard_condition2));
-  EXPECT_NO_THROW(allocator_memory_strategy()->remove_guard_condition(guard_condition3));
+  EXPECT_NO_THROW(allocator_memory_strategy()->remove_guard_condition(&guard_condition1));
+  EXPECT_NO_THROW(allocator_memory_strategy()->remove_guard_condition(&guard_condition2));
+  EXPECT_NO_THROW(allocator_memory_strategy()->remove_guard_condition(&guard_condition3));
   EXPECT_EQ(0u, allocator_memory_strategy()->number_of_guard_conditions());
 }