execute() and stop() both call spin_some(), thus crash

[galou]

Description

Despite #503 and #506, you cannot call MoveGroupInterface::stop() in a separate thread while MoveGroupInterface::execute() is running. This is related to thread-safety but I couldn't find information about thread-safety of MoveGroupInterface.

Your environment

• ROS Distro: Galactic
• OS Version: Ubuntu 20.04
• Source build, branch main (419db3b).

Steps to reproduce

Call move_group->execute(), then, before execute returns, call move_group->stop() in another thread.

Expected behaviour

The executable should not crash and the trajectory execution should stop.

Actual behaviour

The executable crashes with

terminate called after throwing an instance of 'std::runtime_error'
what():  Node has already been added to an executor.

Backtrace or Console output

Backtrace.

[galou]

Please note that I'm willing to fix this myself (in the range of my knowledge of MoveIt) but I don't know exactly which strategy to use. I even think that this should be solved in rclcpp to allow calling spin_some() concurrently.

[henningkayser]

@galou I think the MoveGroupInterface was never designed to be thread-safe, and I don't think there is a good way to make the full API work reliably across multiple threads. What you can do is to use MoveGroupInterface::asyncExecute() for your use case. However, I agree that spinning should not be implemented explicitly inside MGI, but removing all spin calls is a somewhat bigger effort. Would you be interested in working on that?

[galou]

I can work on it but I have a question first. Why is there a call to spin_some() in MoveGroupInterface::stop()? Are there some callbacks involved? stop() just contains a call to publish().

My suggestion would be to add a std::atomic_bool is_spinning_ member variable and use it to conditionally call spin_some(). Would it be OK?

[henningkayser]

@galou #1305 should fix this bug. Could you verify and close this issue?

[henningkayser]

spin_some has been removed a while ago, closing