MoveGroupInterface's asyncExecute is broken. #3160
Description
Problem Description
Before the ROS2 port, the MoveGroupInterface::getMoveGroupClient returned an actionlib::SimpleActionClient instance which provided state querying capabilities. After the port, it now returns an rclcpp_action::Client instance, which doesn't provide any way to query state without the active goal handle (which is not provided). This means one can't properly use the asyncExecute method in practice for anything but simple fire and forgets. As an example, we had issues using the async version for BehaviorTree integration. Instead we had to wrap the blocking version with a thread and do our own state keeping, complicating the solution.
During this attempt, I had a look at the execute method.
I noticed that local variables are captured by the action callbacks by reference.
Crucially, when the async variant is used, the function immediately returns, destroying the locals but the callbacks still reference and use them potentially leading to memory corruption.
The above issues make asyncExecute unusable.
Also, the done variable is not properly synchronized which technically affects the blocking version as well.
Fix suggestion
One could convert the state-tracking locals into member variables ensuring they are properly synchronized either through a mutex or ideally by making them atomic if possible.
Care should be taken to abort the action on object destruction in order to not lead to memory corruption.
Since the state-tracking variables would then be member variables, one could use them to provide some simple state-querying functions.
Another approach would be to create a wrapper over getMoveGroupClient 's returned rclcpp_action::Client that would use the variables and provide state querying in order to match the old behavior.