Skip to content

Commit

Permalink
Fix data race in Actions: Part 3
Browse files Browse the repository at this point in the history
  • Loading branch information
Mauro Passerino committed Aug 22, 2024
1 parent 589885b commit 46a3f21
Showing 1 changed file with 5 additions and 2 deletions.
7 changes: 5 additions & 2 deletions rclcpp/include/rclcpp/experimental/intra_process_manager.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -577,11 +577,14 @@ class IntraProcessManager
auto server = get_matching_intra_process_action_server<ActionT>(ipc_action_client_id);

if (server) {
{
std::unique_lock<std::shared_timed_mutex> lock(mutex_);
clients_uuid_to_id_[goal_id] = ipc_action_client_id;
}

server->store_ipc_action_goal_request(

This comment has been minimized.

Sign in to view

Copy link
@alsora

alsora Aug 23, 2024

Collaborator

should this be also under the lock?
what happens if we set clients_uuid_to_id_[goal_id] and then someone tries to retrieve it before we executed server->store_ipc_action_goal_request ?

This comment has been minimized.

Sign in to view

Copy link
@mauropasse

mauropasse Aug 23, 2024 •
edited
Loading

Collaborator

So this is how things happen now:

  1. Client makes a request -> goes through IPC, ClientID is added to IPM here:

clients_uuid_to_id_[goal_id] = ipc_action_client_id;

  1. Then, store_ipc_action_goal_request() is called, which will generate an event on the server, which after processed, will go to the client, which will check clients_uuid_to_id_ to see if the goal was sent through IPC.

Before this commit, the client was checking for clients_uuid_to_id_ before it was added to the IPM (this only happened actually stressing the system such as threads were very busy!)

There's nothing to protect on store_ipc_action_goal_request with the mutex mutex_ , is just used to protect clients_uuid_to_id_. Not sure if this answers your question or there's still a concern?

This comment has been minimized.

Sign in to view

Copy link
@alsora

alsora Aug 23, 2024

Collaborator

Ok thanks!
ipc_action_client_id, std::move(goal_request));

std::unique_lock<std::shared_timed_mutex> lock(mutex_);
clients_uuid_to_id_[goal_id] = ipc_action_client_id;
return true;
}
return false;
Expand Down

0 comments on commit 46a3f21

Please sign in to comment.