Fix issue with MultiThreadedExecutor by adding lock to protect _futures

[brennanmk]

Description

The MultiThreadedExecutor contains a possible race condition. If _spin_once_impl is called more than once, it is possible that multiple threads may end up passing the condition on line 1008 and try to remove the same future on line 1009.

This solution proposes locking self._futures. This also has the benefit of removing the need to make a shallow copy of self._futures (#1129).

Replaces #1129
Fixes #1393

Is this user-facing behavior change?

No

Did you use Generative AI?

No

Additional Information

An alternative solution might involve placing a lock to prevent multiple threads from entering _spin_once_impl.

[fujitatomoya]

the change looks good to me to exclude the racy condition.

IMO, the right thing to do here is to add the spinning status for the Executor to check if it is already spinning just like rclcpp::Executor. looks like Executor of rclpy does not have this status at all.

[fujitatomoya]

i would like to have 2nd review for this before starting CI.

[cst0]

@brennanmk Is there a minimum reproducible example for this?

[brennanmk]

@cst0 it is relatively straight forward to reproduce the issue. If you call spin_once on a MultiThreadedExecutor a bunch of times in rapid succession (in different threads) eventually it will show up.

#1393 shows what the issue looks like when it appears.

[cst0]

Ah, had some trouble but just had to be more persistent. Looks good to me!

[brennanmk]

Actually, since I removed the creation of a shallow copy in the loop I think it makes sense to also lock the self._futures append call on line 1005. Otherwise it is possible that one thread appends to self._futures while another is in the loop. I do not think this would cause any observable errors, but it is still probably best practice. Alternatively I could add the creation of a shallow copy back, but this seems unnecessary.

It might make sense to do what @fujitatomoya discussed above and instead create a "spinning" variable like in rclcpp. Although this would not be quite as clean as in cpp because we would still need to add a lock to make the spinning variable atomic. It might make sense if we want to raise an exception (or warning) if _spin_once_impl is called multiple times. Happy to make this change if it would be beneficial.

[fujitatomoya]

Pulls: #1477
Gist: https://gist.githubusercontent.com/fujitatomoya/36b8e8c5c643de0b9572cd3eb716871a/raw/5452a7922c32aed9ca24965b37db679c3e299622/ros2.repos
BUILD args: --packages-above-and-dependencies rclpy
TEST args: --packages-above rclpy
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/16321

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[fujitatomoya]

@brennanmk thanks for the effort and explanation.

for adding the spinning check, probably it would be better to have consensus to implement that. besides, this PR does make sense. i say this is good to go with 2nd approval from other maintainers.

[szobov]

Hey there,
Thanks a lot for the effort.

What's needed to have this change merged?
Please let me know if I can assist with the review.

[szobov]

Hey @fujitatomoya
What's needed to make it merged?
We face this issue quite often in our system.
Let me know if I can help bring it into upstream in any way.

[fujitatomoya]

@Mergifyio rebase

[mergify]

rebase

✅ Branch has been successfully rebased

[fujitatomoya]

@szobov waiting for the another approval from maintainers.

[fujitatomoya]

@mjcarroll friendly ping.

[fujitatomoya]

Pulls: #1477
Gist: https://gist.githubusercontent.com/fujitatomoya/41f61146dde3a7c0029ae055a946b18b/raw/5452a7922c32aed9ca24965b37db679c3e299622/ros2.repos
BUILD args: --packages-above-and-dependencies rclpy
TEST args: --packages-above rclpy
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/17022

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[ahcorde]

Pulls: #1477
Gist: https://gist.githubusercontent.com/ahcorde/2dd35e155d331a60943382b0048bebd2/raw/5452a7922c32aed9ca24965b37db679c3e299622/ros2.repos
BUILD args: --packages-above-and-dependencies rclpy
TEST args: --packages-above rclpy
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/17109

• Linux
• Linux-aarch64
• Linux-rhel
• Windows