From c4a424f15b73748591449a2774be7cf0f563f460 Mon Sep 17 00:00:00 2001 From: Mauro Passerino Date: Fri, 13 Aug 2021 15:39:23 +0100 Subject: [PATCH] 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 --- rclcpp/include/rclcpp/memory_strategy.hpp | 2 +- .../strategies/allocator_memory_strategy.hpp | 4 ++-- rclcpp/src/rclcpp/executor.cpp | 14 +++++++------- .../strategies/test_allocator_memory_strategy.cpp | 12 ++++++------ 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/rclcpp/include/rclcpp/memory_strategy.hpp b/rclcpp/include/rclcpp/memory_strategy.hpp index eadba3d6c7..0993c223c9 100644 --- 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 index 27faa73552..d0bb9c7df2 100644 --- 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 index c35224dd14..d3f6cb361f 100644 --- 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 index 30d30d220b..c068e8f640 100644 --- 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()); }