Do not clear entities callbacks on destruction (ros2#2002)

* Do not clear entities callbacks on destruction Removing these clearings since they were not necessary, since the objects are being destroyed anyway. Signed-off-by: Mauro Passerino <mpasserino@irobot.com> * Fix CI Signed-off-by: Mauro Passerino <mpasserino@irobot.com> * Restore clear_on_ready_callback on ~QOSEventHandlerBase Needed since QOSEventHandlerBase does not own the pub/sub listeners. So the QOSEventHandler can be destroyed while the corresponding listeners are still alive, so we need to clear these callbacks. Signed-off-by: Mauro Passerino <mpasserino@irobot.com> * Add coment on clearing callback for QoS event Signed-off-by: Mauro Passerino <mpasserino@irobot.com> Signed-off-by: Mauro Passerino <mpasserino@irobot.com> Co-authored-by: Mauro Passerino <mpasserino@irobot.com>
irobot-ros · Apr 29, 2023 · 5ee345b · 5ee345b
1 parent 735a784
commit 5ee345b
Show file tree

Hide file tree

Showing 7 changed files with 6 additions and 20 deletions.
diff --git a/rclcpp/include/rclcpp/experimental/subscription_intra_process_base.hpp b/rclcpp/include/rclcpp/experimental/subscription_intra_process_base.hpp
@@ -52,7 +52,7 @@ class SubscriptionIntraProcessBase : public rclcpp::Waitable
   {}
 
   RCLCPP_PUBLIC
-  virtual ~SubscriptionIntraProcessBase();
+  virtual ~SubscriptionIntraProcessBase() = default;
 
   RCLCPP_PUBLIC
   size_t

diff --git a/rclcpp/src/rclcpp/publisher_base.cpp b/rclcpp/src/rclcpp/publisher_base.cpp
@@ -102,11 +102,6 @@ PublisherBase::PublisherBase(
 
 PublisherBase::~PublisherBase()
 {
-  for (const auto & pair : event_handlers_) {
-    rcl_publisher_event_type_t event_type = pair.first;
-    clear_on_new_qos_event_callback(event_type);
-  }
-
   // must fini the events before fini-ing the publisher
   event_handlers_.clear();
 

diff --git a/rclcpp/src/rclcpp/qos_event.cpp b/rclcpp/src/rclcpp/qos_event.cpp
@@ -35,6 +35,11 @@ UnsupportedEventTypeException::UnsupportedEventTypeException(
 
 QOSEventHandlerBase::~QOSEventHandlerBase()
 {
+  // Since the rmw event listener holds a reference to
+  // this callback, we need to clear it on destruction of this class.
+  // This clearing is not needed for other rclcpp entities like pub/subs, since
+  // they do own the underlying rmw entities, which are destroyed
+  // on their rclcpp destructors, thus no risk of dangling pointers.
   if (on_new_event_callback_) {
     clear_on_ready_callback();
   }

diff --git a/rclcpp/src/rclcpp/subscription_base.cpp b/rclcpp/src/rclcpp/subscription_base.cpp
@@ -87,13 +87,6 @@ SubscriptionBase::SubscriptionBase(
 
 SubscriptionBase::~SubscriptionBase()
 {
-  clear_on_new_message_callback();
-
-  for (const auto & pair : event_handlers_) {
-    rcl_subscription_event_type_t event_type = pair.first;
-    clear_on_new_qos_event_callback(event_type);
-  }
-
   if (!use_intra_process_) {
     return;
   }

diff --git a/rclcpp/src/rclcpp/subscription_intra_process_base.cpp b/rclcpp/src/rclcpp/subscription_intra_process_base.cpp
@@ -17,11 +17,6 @@
 
 using rclcpp::experimental::SubscriptionIntraProcessBase;
 
-SubscriptionIntraProcessBase::~SubscriptionIntraProcessBase()
-{
-  clear_on_ready_callback();
-}
-
 void
 SubscriptionIntraProcessBase::add_to_wait_set(rcl_wait_set_t * wait_set)
 {

diff --git a/rclcpp_action/src/client.cpp b/rclcpp_action/src/client.cpp
@@ -136,7 +136,6 @@ ClientBase::ClientBase(
 
 ClientBase::~ClientBase()
 {
-  clear_on_ready_callback();
 }
 
 bool

diff --git a/rclcpp_action/src/server.cpp b/rclcpp_action/src/server.cpp
@@ -132,7 +132,6 @@ ServerBase::ServerBase(
 
 ServerBase::~ServerBase()
 {
-  clear_on_ready_callback();
 }
 
 size_t
-Original file line number
+Diff line change
@@ Expand Up / @@ -136,7 +136,6 @@ ClientBase::ClientBase( @@
     ClientBase::~ClientBase()
     {
-      clear_on_ready_callback();
     }
     bool
@@ Expand Down @@