From 7b458d4cdfb1a3da1368543e5787463fd48ddf3e Mon Sep 17 00:00:00 2001 From: Barry Xu Date: Tue, 22 Oct 2019 13:40:36 +0800 Subject: [PATCH 1/4] Clear sub contexts to make context shared_ptr not to be hold. Signed-off-by: Barry Xu --- rclcpp/src/rclcpp/context.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/rclcpp/src/rclcpp/context.cpp b/rclcpp/src/rclcpp/context.cpp index a93086aa5f..e8fbe31d07 100644 --- a/rclcpp/src/rclcpp/context.cpp +++ b/rclcpp/src/rclcpp/context.cpp @@ -162,6 +162,9 @@ Context::shutdown(const std::string & reason) ++it; } } + // clear sub contexts + std::lock_guard lock(sub_contexts_mutex_); + sub_contexts_.clear(); return true; } From 5bd65eb22ed71e74488481cbfdb89ec01feea9e3 Mon Sep 17 00:00:00 2001 From: Barry Xu Date: Tue, 19 Nov 2019 10:46:20 +0800 Subject: [PATCH 2/4] refactor to use weak_ptr to store sub_context instead of share_ptr Signed-off-by: Barry Xu --- rclcpp/include/rclcpp/context.hpp | 25 ++++++++++++++----------- rclcpp/src/rclcpp/context.cpp | 3 --- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/rclcpp/include/rclcpp/context.hpp b/rclcpp/include/rclcpp/context.hpp index dfc33d5e4f..f14812ae4d 100644 --- a/rclcpp/include/rclcpp/context.hpp +++ b/rclcpp/include/rclcpp/context.hpp @@ -305,18 +305,21 @@ class Context : public std::enable_shared_from_this std::type_index type_i(typeid(SubContext)); std::shared_ptr sub_context; auto it = sub_contexts_.find(type_i); - if (it == sub_contexts_.end()) { - // It doesn't exist yet, make it - sub_context = std::shared_ptr( - new SubContext(std::forward(args) ...), - [](SubContext * sub_context_ptr) { - delete sub_context_ptr; - }); - sub_contexts_[type_i] = sub_context; - } else { + if (it != sub_contexts_.end()) { // It exists, get it out and cast it. - sub_context = std::static_pointer_cast(it->second); + auto sub_context_ptr = it->second.lock(); + if (sub_context_ptr) { + return std::static_pointer_cast(sub_context_ptr); + } } + + // It doesn't exist yet, make it + sub_context = std::shared_ptr( + new SubContext(std::forward(args) ...), + [](SubContext * sub_context_ptr) { + delete sub_context_ptr; + }); + sub_contexts_[type_i] = sub_context; return sub_context; } @@ -338,7 +341,7 @@ class Context : public std::enable_shared_from_this rclcpp::InitOptions init_options_; std::string shutdown_reason_; - std::unordered_map> sub_contexts_; + std::unordered_map> sub_contexts_; // This mutex is recursive so that the constructor of a sub context may // attempt to acquire another sub context. std::recursive_mutex sub_contexts_mutex_; diff --git a/rclcpp/src/rclcpp/context.cpp b/rclcpp/src/rclcpp/context.cpp index e8fbe31d07..a93086aa5f 100644 --- a/rclcpp/src/rclcpp/context.cpp +++ b/rclcpp/src/rclcpp/context.cpp @@ -162,9 +162,6 @@ Context::shutdown(const std::string & reason) ++it; } } - // clear sub contexts - std::lock_guard lock(sub_contexts_mutex_); - sub_contexts_.clear(); return true; } From 65641cdec937af2c75f648139b64ccee16094d5d Mon Sep 17 00:00:00 2001 From: Barry Xu Date: Wed, 27 Nov 2019 18:33:32 +0800 Subject: [PATCH 3/4] revert previous modification, use weak_ptr to store context in graph listener Signed-off-by: Barry Xu --- rclcpp/include/rclcpp/context.hpp | 25 ++++++++++------------ rclcpp/include/rclcpp/graph_listener.hpp | 2 +- rclcpp/src/rclcpp/graph_listener.cpp | 27 ++++++++++++++++++++---- 3 files changed, 35 insertions(+), 19 deletions(-) diff --git a/rclcpp/include/rclcpp/context.hpp b/rclcpp/include/rclcpp/context.hpp index f14812ae4d..dfc33d5e4f 100644 --- a/rclcpp/include/rclcpp/context.hpp +++ b/rclcpp/include/rclcpp/context.hpp @@ -305,21 +305,18 @@ class Context : public std::enable_shared_from_this std::type_index type_i(typeid(SubContext)); std::shared_ptr sub_context; auto it = sub_contexts_.find(type_i); - if (it != sub_contexts_.end()) { + if (it == sub_contexts_.end()) { + // It doesn't exist yet, make it + sub_context = std::shared_ptr( + new SubContext(std::forward(args) ...), + [](SubContext * sub_context_ptr) { + delete sub_context_ptr; + }); + sub_contexts_[type_i] = sub_context; + } else { // It exists, get it out and cast it. - auto sub_context_ptr = it->second.lock(); - if (sub_context_ptr) { - return std::static_pointer_cast(sub_context_ptr); - } + sub_context = std::static_pointer_cast(it->second); } - - // It doesn't exist yet, make it - sub_context = std::shared_ptr( - new SubContext(std::forward(args) ...), - [](SubContext * sub_context_ptr) { - delete sub_context_ptr; - }); - sub_contexts_[type_i] = sub_context; return sub_context; } @@ -341,7 +338,7 @@ class Context : public std::enable_shared_from_this rclcpp::InitOptions init_options_; std::string shutdown_reason_; - std::unordered_map> sub_contexts_; + std::unordered_map> sub_contexts_; // This mutex is recursive so that the constructor of a sub context may // attempt to acquire another sub context. std::recursive_mutex sub_contexts_mutex_; diff --git a/rclcpp/include/rclcpp/graph_listener.hpp b/rclcpp/include/rclcpp/graph_listener.hpp index ca4f05c487..901ba469e3 100644 --- a/rclcpp/include/rclcpp/graph_listener.hpp +++ b/rclcpp/include/rclcpp/graph_listener.hpp @@ -165,7 +165,7 @@ class GraphListener : public std::enable_shared_from_this void __shutdown(bool); - rclcpp::Context::SharedPtr parent_context_; + rclcpp::Context::WeakPtr parent_context_; std::thread listener_thread_; bool is_started_; diff --git a/rclcpp/src/rclcpp/graph_listener.cpp b/rclcpp/src/rclcpp/graph_listener.cpp index b097564333..e86f208e0b 100644 --- a/rclcpp/src/rclcpp/graph_listener.cpp +++ b/rclcpp/src/rclcpp/graph_listener.cpp @@ -71,6 +71,11 @@ GraphListener::start_if_not_started() throw GraphListenerShutdownError(); } if (!is_started_) { + auto parent_context_ptr = parent_context_.lock(); + if (!parent_context_ptr) { + throw std::runtime_error( + "graph listener start called after destruction of parent context"); + } // Initialize the wait set before starting. rcl_ret_t ret = rcl_wait_set_init( &wait_set_, @@ -80,7 +85,7 @@ GraphListener::start_if_not_started() 0, // number_of_clients 0, // number_of_services 0, // number_of_events - this->parent_context_->get_rcl_context().get(), + parent_context_ptr->get_rcl_context().get(), rcl_get_default_allocator()); if (RCL_RET_OK != ret) { throw_from_rcl_error(ret, "failed to initialize wait set"); @@ -355,10 +360,24 @@ GraphListener::__shutdown(bool should_throw) throw_from_rcl_error(ret, "failed to finalize interrupt guard condition"); } if (shutdown_guard_condition_) { - if (should_throw) { - parent_context_->release_interrupt_guard_condition(&wait_set_); + auto parent_context_ptr = parent_context_.lock(); + if (parent_context_ptr) { + if (should_throw) { + parent_context_ptr->release_interrupt_guard_condition(&wait_set_); + } else { + parent_context_ptr->release_interrupt_guard_condition(&wait_set_, std::nothrow); + } } else { - parent_context_->release_interrupt_guard_condition(&wait_set_, std::nothrow); + ret = rcl_guard_condition_fini(shutdown_guard_condition_); + if (RCL_RET_OK != ret) { + if (should_throw) { + throw_from_rcl_error(ret, "failed to finalize shutdown guard condition"); + } else { + RCLCPP_ERROR( + rclcpp::get_logger("rclcpp"), + "failed to finalize shutdown guard condition"); + } + } } shutdown_guard_condition_ = nullptr; } From 06f549e1f16d18bffe754fee42f67b2ad9591584 Mon Sep 17 00:00:00 2001 From: Barry Xu Date: Thu, 28 Nov 2019 09:55:41 +0800 Subject: [PATCH 4/4] use interrupt_guard_condition_context_ directly Signed-off-by: Barry Xu --- rclcpp/src/rclcpp/graph_listener.cpp | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/rclcpp/src/rclcpp/graph_listener.cpp b/rclcpp/src/rclcpp/graph_listener.cpp index e86f208e0b..29369329e7 100644 --- a/rclcpp/src/rclcpp/graph_listener.cpp +++ b/rclcpp/src/rclcpp/graph_listener.cpp @@ -71,11 +71,6 @@ GraphListener::start_if_not_started() throw GraphListenerShutdownError(); } if (!is_started_) { - auto parent_context_ptr = parent_context_.lock(); - if (!parent_context_ptr) { - throw std::runtime_error( - "graph listener start called after destruction of parent context"); - } // Initialize the wait set before starting. rcl_ret_t ret = rcl_wait_set_init( &wait_set_, @@ -85,7 +80,7 @@ GraphListener::start_if_not_started() 0, // number_of_clients 0, // number_of_services 0, // number_of_events - parent_context_ptr->get_rcl_context().get(), + interrupt_guard_condition_context_.get(), rcl_get_default_allocator()); if (RCL_RET_OK != ret) { throw_from_rcl_error(ret, "failed to initialize wait set");