Fixes spin_until_future_complete removing node it didn't add

[jmblixt3]

Closes #1313
Current if spin_unitl_future_complete is called inside a nodes callback it removes the node from the executor
This results in any subsiquent waitables to never be checked by the node since the node is no longer in the executor
This aims to fix that by only removing the node from the executor if it wasn't already present

[fujitatomoya]

@jmblixt3

does this really resolve the #1313 (comment)? i think you are using the same callback group, and rcl_wait with recursive executor spin. seems pretty much you are hitting the situation described in https://docs.ros.org/en/rolling/How-To-Guides/Using-callback-groups.html#avoiding-deadlocks?

even if that works, waiting on the future completion synchronously in the callback would block the other entities with the same callback group to be blocked, i would not think this is good design practice for the application.

i may be missing something, i would like to get more comments from others.

[jmblixt3]

Looking at rclcpp clcpp/executors.hpp, there's a similar TODO to the issue I described. I'm not sure which version of test code you looked at, but this is what I used to validate the issue beforehand, and how to show it was fixed after. Looking at the original issue see the usage of fibonacci_action_server, fibonacci_action_hybrid, and ros2 action send_goal. The last version of the hybrid is below

fibomnacci_action_hybrid.py

import rclpy
from rclpy import Future
from rclpy.action import ActionServer, ActionClient
from rclpy.action.client import ClientGoalHandle
from rclpy.action.server import ServerGoalHandle
import rclpy.logging
from rclpy.node import Node

from action_tutorials_interfaces.action import Fibonacci

from rclpy.executors import ExternalShutdownException

class FibonacciActionServer(Node):
    def __init__(self):
        super().__init__("fibonacci_action_server")
        # self.client_node = rclpy.create_node('fibonacci_action_server_client')
        self._action_server = ActionServer(
            self, Fibonacci, "some_action", self.execute_callback
        )
        self._action_client = ActionClient(
            self, Fibonacci, "fibonacci"
        )

    def execute_callback(self, goal_handle: ServerGoalHandle):
        self.get_logger().info("Requesting goal...")
        request_future = self._action_client.send_goal_async(Fibonacci.Goal(order=goal_handle.request.order))
        rclpy.spin_until_future_complete(self,request_future)
        self.get_logger().info("Recieved request result...")
        spin_handle: ClientGoalHandle = request_future.result()
        result_future: Future = spin_handle.get_result_async()
        rclpy.spin_until_future_complete(self,result_future)
        self.get_logger().info("Recieved final result...")
        result = Fibonacci.Result()
        if not result_future.cancelled():
            result = result_future.result().result
        goal_handle.succeed()
        return result



def main(args=None):
    try:
        with rclpy.init(args=args):
            fibonacci_action_server = FibonacciActionServer()
            rclpy.spin(fibonacci_action_server)
    except (KeyboardInterrupt, ExternalShutdownException):
        pass


if __name__ == '__main__':
    main()

[MichaelOrlov]

@sloretz It seems we did talk about this issue before. Could you please follow up on it and give some comments?

[sloretz]

The fix LGTM, but please use coroutines for waiting for results inside a callback #1313 (comment)

[sloretz]

@Mergifyio update

[mergify]

update

✅ Branch has been successfully updated

[sloretz]

Pulls: #1316
Gist: https://gist.githubusercontent.com/sloretz/450ced2a382151503e56242c22eeccf7/raw/cae0f91ede29d09f58213d1e855db468c8081b45/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/14465

• Linux (Infra issue; rerun: )
• Linux-aarch64
• Linux-rhel
• Windows

[sloretz]

Thanks for the PR!

[jmblixt3]

@sloretz can this be back ported?

[sloretz]

@Mergifyio backport jazzy iron humble

[mergify]

backport jazzy iron humble

✅ Backports have been created

• #1341 Fixes spin_until_future_complete removing node it didn't add (backport #1316) has been created for branch jazzy
• #1342 Fixes spin_until_future_complete removing node it didn't add (backport #1316) has been created for branch iron
• #1343 Fixes spin_until_future_complete removing node it didn't add (backport #1316) has been created for branch humble