Merge pull request ros2#21 from alsora/asoragna/fix-unit-tests

fix unit tests

rclcpp/include/rclcpp/client.hpp

@@ -51,6 +51,11 @@ namespace node_interfaces
 class NodeBaseInterface;
 }  // namespace node_interfaces
 
+namespace executors
+{
+class EventsExecutor;
+}  // namespace executors
+
 class ClientBase
 {
 public:
@@ -154,12 +159,12 @@ class ClientBase
   RCLCPP_PUBLIC
   void
   set_events_executor_callback(
-    const void * executor_context,
+    const rclcpp::executors::EventsExecutor * executor,
     ExecutorEventCallback executor_callback) const;
 
   RCLCPP_PUBLIC
   void
-  set_on_destruction_callback(std::function<void (ClientBase*)> on_destruction_callback);
+  set_on_destruction_callback(std::function<void(ClientBase *)> on_destruction_callback);
 
 protected:
   RCLCPP_DISABLE_COPY(ClientBase)
@@ -176,7 +181,7 @@ class ClientBase
   const rcl_node_t *
   get_rcl_node_handle() const;
 
-  std::function<void (ClientBase*)> on_destruction_callback_;
+  std::function<void(ClientBase *)> on_destruction_callback_;
 
   rclcpp::node_interfaces::NodeGraphInterface::WeakPtr node_graph_;
   std::shared_ptr<rcl_node_t> node_handle_;

rclcpp/include/rclcpp/executors/events_executor.hpp

@@ -149,26 +149,37 @@ class EventsExecutor : public rclcpp::Executor
     this_executor->event_queue_cv_.notify_one();
   }
 
-  template <typename T>
+  // This function allows to remove an entity from the EventsExecutor.
+  // Entities are any of SubscriptionBase, PublisherBase, ClientBase, ServerBase, Waitable.
+  // After an entity has been removed using this API, it can be safely destroyed without the risk
+  // that the executor would try to access it again.
+  template<typename T>
   void
-  remove_entity(T* entity)
+  remove_entity(T * entity)
   {
     // We need to unset the callbacks to make sure that after removing events from the
     // queues, this entity will not push anymore before being completely destroyed.
+    // This assumes that all supported entities implement this function.
     entity->set_events_executor_callback(nullptr, nullptr);
 
     // Remove events associated with this entity from the event queue
     {
       std::unique_lock<std::mutex> lock(push_mutex_);
-      event_queue_.erase(std::remove_if(event_queue_.begin(), event_queue_.end(),
-        [&entity](ExecutorEvent event) { return event.entity == entity; }), event_queue_.end());
+      event_queue_.erase(
+        std::remove_if(
+          event_queue_.begin(), event_queue_.end(),
+          [&entity](ExecutorEvent event) {return event.entity == entity;}), event_queue_.end());
     }
 
     // Remove events associated with this entity from the local event queue
     {
       std::unique_lock<std::mutex> lock(execution_mutex_);
-      local_event_queue_.erase(std::remove_if(local_event_queue_.begin(), local_event_queue_.end(),
-        [&entity](ExecutorEvent event) { return event.entity == entity; }), local_event_queue_.end());
+      local_event_queue_.erase(
+        std::remove_if(
+          local_event_queue_.begin(), local_event_queue_.end(),
+          [&entity](ExecutorEvent event) {
+            return event.entity == entity;
+          }), local_event_queue_.end());
     }
   }
 

rclcpp/include/rclcpp/executors/timers_manager.hpp

@@ -125,7 +125,7 @@ class TimersManager
    * @brief Remove a single timer stored in the object, passed as a raw ptr.
    * @param timer the timer to remove.
    */
-  void remove_timer_raw(rclcpp::TimerBase* timer);
+  void remove_timer_raw(rclcpp::TimerBase * timer);
 
   // This is what the TimersManager uses to denote a duration forever.
   // We don't use std::chrono::nanoseconds::max because it will overflow.
@@ -179,12 +179,12 @@ class TimersManager
       std::make_heap(timers_.begin(), timers_.end(), timer_greater);
     }
 
-    TimerPtr& front()
+    TimerPtr & front()
     {
       return timers_.front();
     }
 
-    const TimerPtr& front() const
+    const TimerPtr & front() const
     {
       return timers_.front();
     }
@@ -199,7 +199,6 @@ class TimersManager
       timers_.clear();
     }
 
-  private:
     static bool timer_greater(TimerPtr a, TimerPtr b)
     {
       return a->time_until_trigger() > b->time_until_trigger();
@@ -263,7 +262,7 @@ class TimersManager
   // Flag used as predicate by timers_cv
   bool timers_updated_ {false};
   // Indicates whether the timers thread is currently running or requested to stop
-  bool running_ {false};
+  std::atomic<bool> running_ {false};
   // Context of the parent executor
   std::shared_ptr<rclcpp::Context> context_;
   // MinHeap of timers

rclcpp/include/rclcpp/experimental/subscription_intra_process_base.hpp

@@ -73,7 +73,7 @@ class SubscriptionIntraProcessBase : public rclcpp::Waitable
   RCLCPP_PUBLIC
   void
   set_events_executor_callback(
-    void * executor_context,
+    const rclcpp::executors::EventsExecutor * executor,
     ExecutorEventCallback executor_callback) const override;
 
 protected:

rclcpp/include/rclcpp/qos_event.hpp

@@ -30,6 +30,11 @@
 namespace rclcpp
 {
 
+namespace executors
+{
+class EventsExecutor;
+}  // namespace executors
+
 using QOSDeadlineRequestedInfo = rmw_requested_deadline_missed_status_t;
 using QOSDeadlineOfferedInfo = rmw_offered_deadline_missed_status_t;
 using QOSLivelinessChangedInfo = rmw_liveliness_changed_status_t;
@@ -152,11 +157,11 @@ class QOSEventHandler : public QOSEventHandlerBase
   RCLCPP_PUBLIC
   void
   set_events_executor_callback(
-    void * executor_context,
+    const rclcpp::executors::EventsExecutor * executor,
     ExecutorEventCallback executor_callback) const override
   {
     rcl_ret_t ret = rcl_event_set_events_executor_callback(
-      executor_context,
+      executor,
       executor_callback,
       this,
       &event_handle_,

rclcpp/include/rclcpp/service.hpp

@@ -39,6 +39,11 @@
 namespace rclcpp
 {
 
+namespace executors
+{
+class EventsExecutor;
+}  // namespace executors
+
 class ServiceBase
 {
 public:
@@ -124,12 +129,12 @@ class ServiceBase
   RCLCPP_PUBLIC
   void
   set_events_executor_callback(
-    const void * executor_context,
+    const rclcpp::executors::EventsExecutor * executor,
     ExecutorEventCallback executor_callback) const;
 
   RCLCPP_PUBLIC
   void
-  set_on_destruction_callback(std::function<void (ServiceBase*)> on_destruction_callback);
+  set_on_destruction_callback(std::function<void(ServiceBase *)> on_destruction_callback);
 
 protected:
   RCLCPP_DISABLE_COPY(ServiceBase)
@@ -142,7 +147,7 @@ class ServiceBase
   const rcl_node_t *
   get_rcl_node_handle() const;
 
-  std::function<void (ServiceBase*)> on_destruction_callback_;
+  std::function<void(ServiceBase *)> on_destruction_callback_;
 
   std::shared_ptr<rcl_node_t> node_handle_;
 

rclcpp/include/rclcpp/subscription_base.hpp

@@ -46,6 +46,11 @@ namespace node_interfaces
 class NodeBaseInterface;
 }  // namespace node_interfaces
 
+namespace executors
+{
+class EventsExecutor;
+}  // namespace executors
+
 namespace experimental
 {
 /**
@@ -267,12 +272,12 @@ class SubscriptionBase : public std::enable_shared_from_this<SubscriptionBase>
   RCLCPP_PUBLIC
   void
   set_events_executor_callback(
-    const void * executor_context,
+    const rclcpp::executors::EventsExecutor * executor,
     ExecutorEventCallback executor_callback) const;
 
   RCLCPP_PUBLIC
   void
-  set_on_destruction_callback(std::function<void (SubscriptionBase*)> on_destruction_callback);
+  set_on_destruction_callback(std::function<void(SubscriptionBase *)> on_destruction_callback);
 
 protected:
   template<typename EventCallbackT>
@@ -316,7 +321,7 @@ class SubscriptionBase : public std::enable_shared_from_this<SubscriptionBase>
   rosidl_message_type_support_t type_support_;
   bool is_serialized_;
 
-  std::function<void (SubscriptionBase*)> on_destruction_callback_;
+  std::function<void(SubscriptionBase *)> on_destruction_callback_;
 
   std::atomic<bool> subscription_in_use_by_wait_set_{false};
   std::atomic<bool> intra_process_subscription_waitable_in_use_by_wait_set_{false};

rclcpp/include/rclcpp/timer.hpp

@@ -139,12 +139,12 @@ class TimerBase
 
   RCLCPP_PUBLIC
   void
-  set_on_destruction_callback(std::function<void (TimerBase*)> on_destruction_callback);
+  set_on_destruction_callback(std::function<void(TimerBase *)> on_destruction_callback);
 
 protected:
   Clock::SharedPtr clock_;
   std::shared_ptr<rcl_timer_t> timer_handle_;
-  std::function<void (TimerBase*)> on_destruction_callback_;
+  std::function<void(TimerBase *)> on_destruction_callback_;
 
   std::atomic<bool> in_use_by_wait_set_{false};
 };

rclcpp/include/rclcpp/waitable.hpp

@@ -26,6 +26,11 @@
 namespace rclcpp
 {
 
+namespace executors
+{
+class EventsExecutor;
+}  // namespace executors
+
 class Waitable
 {
 public:
@@ -170,15 +175,15 @@ class Waitable
   virtual
   void
   set_events_executor_callback(
-    void * executor_context,
+    const rclcpp::executors::EventsExecutor * executor,
     ExecutorEventCallback executor_callback) const;
 
   RCLCPP_PUBLIC
   void
-  set_on_destruction_callback(std::function<void (Waitable*)> on_destruction_callback);
+  set_on_destruction_callback(std::function<void(Waitable *)> on_destruction_callback);
 
 private:
-  std::function<void (Waitable*)> on_destruction_callback_;
+  std::function<void(Waitable *)> on_destruction_callback_;
   std::atomic<bool> in_use_by_wait_set_{false};
 };  // class Waitable
 

rclcpp/src/rclcpp/client.cpp

@@ -204,11 +204,11 @@ ClientBase::exchange_in_use_by_wait_set_state(bool in_use_state)
 
 void
 ClientBase::set_events_executor_callback(
-  const void * executor_context,
+  const rclcpp::executors::EventsExecutor * executor,
   ExecutorEventCallback executor_callback) const
 {
   rcl_ret_t ret = rcl_client_set_events_executor_callback(
-    executor_context,
+    executor,
     executor_callback,
     this,
     client_handle_.get());
@@ -218,7 +218,9 @@ ClientBase::set_events_executor_callback(
   }
 }
 
-void ClientBase::set_on_destruction_callback(std::function<void (ClientBase*)> on_destruction_callback)
+void
+ClientBase::set_on_destruction_callback(
+  std::function<void(ClientBase *)> on_destruction_callback)
 {
   on_destruction_callback_ = on_destruction_callback;
 }

rclcpp/src/rclcpp/executors/events_executor.cpp

@@ -164,7 +164,6 @@ EventsExecutor::spin_all(std::chrono::nanoseconds max_duration)
 
   // Keep executing until work available or timeout expired
   while (rclcpp::ok(context_) && spinning.load() && max_duration_not_elapsed()) {
-
     std::unique_lock<std::mutex> push_lock(push_mutex_);
     std::unique_lock<std::mutex> execution_lock(execution_mutex_);
     std::swap(local_event_queue_, event_queue_);

rclcpp/src/rclcpp/executors/events_executor_entities_collector.cpp

@@ -47,7 +47,7 @@ void
 EventsExecutorEntitiesCollector::execute()
 {
   // This function is called when the associated executor is notified that something changed.
-  // We do not know if an entity has been added or remode so we have to rebuild everything.
+  // We do not know if an entity has been added or removed so we have to rebuild everything.
 
   timers_manager_->clear();