rclcpp_action: take and execute service entities in priority.

ros2 / rclcpp

rclcpp (ROS Client Library for C++)

Apache License 2.0

536 stars 417 forks source link

rclcpp_action: take and execute service entities in priority. #2471

Closed fujitatomoya closed 1 month ago

fujitatomoya commented 6 months ago

address https://github.com/ros2/rclcpp/issues/2451 and https://github.com/ros2/rclcpp/issues/1679

Note: more tests need to be done before CI.

fujitatomoya commented 6 months ago

IMO, we do not guarantee all feedback messages must be delivered to the client. (if the goal accept response is not yet delivered, that would be dropped. but eventually, feedback messages will be received on the client once goal handle is being tracked.)
Service responses should be handled with priority than feedback and status topics because it changes internal action state in the client.

fujitatomoya commented 6 months ago

@alsora thanks for the review.

this will change the action client behavior internally so i need to do more verification locally, and i would like to have more feedback for this before further. please keep it open for a while.

jmachowinski commented 6 months ago

Uhm, this implementation is ontop of a racy state...

We should merge https://github.com/ros2/rclcpp/pull/2250 first.

jmachowinski commented 6 months ago

Regarding the issue itself. From my point of view, the design of the action is broken, as it uses multiple communication channels to communicate, thus creating this kind of problems.

Btw you can create the same bug by spamming feedback, and make an action never terminate.

fujitatomoya commented 6 months ago

@jmachowinski thanks for the comment. this one is not in the merge queue yet, i need to take some more time to verify this, since this changes the action behavior, i am expecting that userspace (tests) would break.

fujitatomoya commented 1 month ago

https://github.com/ros2/rclcpp/pull/2612 replaces this one.