MoveGroupInterface's asyncExecute is broken.

[liarokapisv]

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.

[mikeferguson]

I think we can probably just merge these comments with your other ticket and update the description to basically be "async interface not useable" - a quick at the code leads me to believe that is probably the case.

[liarokapisv]

I guess we could merge them. I figured these are technically separate issues, the asyncExecute method could very well just work without any state querying interface, which I guess is limiting but still.

Ofcourse, fixing this issue by making these member variables and adding proper synchronization would make adding some state query functions trivial.

Main issue is that the other ticker is a feature request, how would you like to proceed?

EDIT: Merged into this ticket instead.