Fix multi-threaded race condition in client.call_async

[augustelalande]

Proposal to fix multi-threaded race condition in client.call_async as described in #870

[fujitatomoya]

i think this makes sense to avoid the possible racy condition in theory.

[fujitatomoya]

@ivanpauno @jacobperron could you take a look at this when you have time?

[fujitatomoya]

CI:

• Linux
• Linux-aarch64
• Windows

[jacobperron]

Tricky..

I suppose there are similar threading issues with the action client, wherever we are making service requests, for instance

rclpy/rclpy/rclpy/action/client.py

Line 435 in 289d7d3

self._pending_goal_requests[sequence_number] = future

has a race with

rclpy/rclpy/rclpy/executors.py

Line 383 in 7a906af

for future in waitable._futures:

It's possible the executor tries to process a request before the future is available in the list.

It's too bad we have to introduce a lock at the scope of sending the request, but I can't think of another way. Hopefully it won't have a significant impact to performance.

@sloretz Thoughts?

[augustelalande]

Tricky..

I suppose there are similar threading issues with the action client, wherever we are making service requests, for instance

rclpy/rclpy/rclpy/action/client.py

Line 435 in 289d7d3

self._pending_goal_requests[sequence_number] = future

has a race with

rclpy/rclpy/rclpy/executors.py

Line 383 in 7a906af

for future in waitable._futures:

It's possible the executor tries to process a request before the future is available in the list.

It's too bad we have to introduce a lock at the scope of sending the request, but I can't think of another way. Hopefully it won't have a significant impact to performance.

@sloretz Thoughts?

Yes this race condition definitely exists in the ActionClient. Actually, even #564 still exists there. The question is why is the Client functionality being reimplemented in ActionClient? Why not reuse the existing Client functionality by creating sub-clients to handle goal requests, cancel requests etc.?

I guess this should be a new issue.

[ivanpauno]

Yes this race condition definitely exists in the ActionClient. Actually, even #564 still exists there. The question is why is the Client functionality being reimplemented in ActionClient? Why not reuse the existing Client functionality by creating sub-clients to handle goal requests, cancel requests etc.?

👍, I'm not sure how easy is to do that but I think it would be good to reuse the existing client code.

I guess this should be a new issue.

Yes, I don't think we need to address that here.

[fujitatomoya]

CI:

• Linux
• Linux-aarch64
• Windows

[jacobperron]

I think the Action Client doesn't share the same logic as the Client because it is using the rcl_action C-API under the hood. Perhaps it's possible to refactor the code in rclpy such that they share the same high-level logic; I don't recall if this was considered when actions were implemented.

[jacobperron]

Testing rclpy and packages above:

• Linux
• Linux-aarch64
• Windows

[jacobperron]

I've rebased on master to get recent changes related to pybind11, hopefully this resolves CI failures:

• Linux
• Linux-aarch64
• Windows