Fix data race in Actions: Part 2 (ros2#148)

* Fix data race in Actions: Part 2 * Fix warning - copy elision --------- Co-authored-by: Mauro Passerino <mpasserino@irobot.com>
mauropasse · Aug 13, 2024 · 6a95d80 · 6a95d80
1 parent a2859eb
commit 6a95d80
Show file tree

Hide file tree

Showing 3 changed files with 13 additions and 32 deletions.
diff --git a/rclcpp/include/rclcpp/experimental/action_client_intra_process_base.hpp b/rclcpp/include/rclcpp/experimental/action_client_intra_process_base.hpp
@@ -158,17 +158,6 @@ class ActionClientIntraProcessBase : public rclcpp::Waitable
     event_info_multi_map_.erase(goal_id);
   }
 
-  bool has_goal_id(size_t goal_id) const
-  {
-    // Check if the intra-process client has this goal_id
-    auto it = event_info_multi_map_.find(goal_id);
-    if (it != event_info_multi_map_.end()) {
-      return true;
-    }
-
-    return false;
-  }
-
 private:
   std::string action_name_;
   QoS qos_profile_;

diff --git a/rclcpp/include/rclcpp/experimental/intra_process_manager.hpp b/rclcpp/include/rclcpp/experimental/intra_process_manager.hpp
@@ -1061,7 +1061,7 @@ class IntraProcessManager
           auto ptr = MessageAllocTraits::allocate(allocator, 1);
           MessageAllocTraits::construct(allocator, ptr, *message);
 
-          subscription->provide_intra_process_data(std::move(MessageUniquePtr(ptr, deleter)));
+          subscription->provide_intra_process_data(MessageUniquePtr(ptr, deleter));
         }
 
         continue;
@@ -1104,7 +1104,7 @@ class IntraProcessManager
             MessageAllocTraits::construct(allocator, ptr, *message);
 
             ros_message_subscription->provide_intra_process_message(
-              std::move(MessageUniquePtr(ptr, deleter)));
+              MessageUniquePtr(ptr, deleter));
           }
         }
       }

diff --git a/rclcpp_action/include/rclcpp_action/client.hpp b/rclcpp_action/include/rclcpp_action/client.hpp
@@ -523,17 +523,13 @@ class Client : public ClientBase
       // If there's not, we fall back into inter-process communication, since
       // the server might be available in another process or was configured to not use IPC.
       if (intra_process_server_available) {
-        bool goal_sent_by_ipc = ipc_action_client_->has_goal_id(hashed_guuid);
+        ipc_action_client_->store_goal_response_callback(
+          hashed_guuid, goal_response_callback);
 
-        if (goal_sent_by_ipc) {
-          ipc_action_client_->store_goal_response_callback(
-            hashed_guuid, goal_response_callback);
-
-          intra_process_send_done = ipm->template intra_process_action_send_goal_request<ActionT>(
-              ipc_action_client_id_,
-              std::move(goal_request),
-              hashed_guuid);
-        }
+        intra_process_send_done = ipm->template intra_process_action_send_goal_request<ActionT>(
+            ipc_action_client_id_,
+            std::move(goal_request),
+            hashed_guuid);
       }
     }
 
@@ -850,7 +846,8 @@ class Client : public ClientBase
         if (intra_process_server_available) {
           size_t hashed_guuid = std::hash<GoalUUID>()(goal_handle->get_goal_id());
 
-          bool goal_sent_by_ipc = ipc_action_client_->has_goal_id(hashed_guuid);
+          // Determine if goal was sent through inter or intra process by checking the goal ID
+          bool goal_sent_by_ipc = ipm->get_action_client_id_from_goal_uuid(hashed_guuid);
 
           if (goal_sent_by_ipc) {
             ipc_action_client_->store_result_response_callback(
@@ -910,17 +907,12 @@ class Client : public ClientBase
       // the server might be available in another process or was configured to not use IPC.
       if (intra_process_server_available) {
         size_t hashed_guuid = std::hash<GoalUUID>()(cancel_request->goal_info.goal_id.uuid);
+        ipc_action_client_->store_cancel_goal_callback(
+          hashed_guuid, cancel_goal_callback);
 
-        bool goal_sent_by_ipc = ipc_action_client_->has_goal_id(hashed_guuid);
-
-        if (goal_sent_by_ipc) {
-          ipc_action_client_->store_cancel_goal_callback(
-            hashed_guuid, cancel_goal_callback);
-
-          intra_process_send_done = ipm->template intra_process_action_send_cancel_request<ActionT>(
+        intra_process_send_done = ipm->template intra_process_action_send_cancel_request<ActionT>(
             ipc_action_client_id_,
             std::move(cancel_request));
-        }
       }
     }