Asyncronous wait_for_service about rclpy HOT 3 OPEN

@sloretz can you also open a PR against system_tests to update the requester_py.py file to use this new feature. I would expect this to reduce the flakiness of these tests significantly.
Thanks!

from rclpy.

@ros2/team Mind giving some feedback on this idea?

TL;DR I want to eliminate the GraphListener using coroutines.

Currently #127 implements wait_for_service using a GraphListener class. GraphListener has its own thread and waits on node graph guard conditions plus timers for the wait_for_service timeout. This enables a user to call client.wait_for_service() inside of a callback without blocking forever due to starvation of a SingleThreadedExecutor.

def my_callback(self, msg):
   if self.client.wait_for_service():
      # do something with service

Alternatively, GraphListener can be eliminated using the coroutine support in #135. wait_for_service() would return a Future object. If a user wants to wait in a callback they must make it a coroutine and await the Future. The executor would wait on the node graph guard conditions, and execute callbacks like normal. The SingleThreadedExecutor would not be blocked by the user's callback because the future yields back to the executor if the service isn't ready.

async def my_callback(self, msg):
   result = await self.client.wait_for_service()
   if result:
       # do something with service

The trouble happens when trying to offer both options. The executor cannot wait on the node graph guard conditions because rcl_wait says the same entity cannot be waited on at the same time in two wait sets. It is possible to make a second method wait_for_service_async that returns a Future which uses the GraphListener, but it means the same pattern has to be used anywhere a Future is returned. Client.call() and Client.call_async() would need a ClientListener managing a separate thread to wait on clients.

I see a few options. I like the first option the most.

1. Offer only wait_for_service that returns a Future. Eliminate GraphListener. If a user wants to block in a callback they must know about python coroutines.
2. Offer both wait_for_service and wait_for_service_async. Don't Elimate GraphListener. Create ClientListener in the future.
3. Offer both wait_for_service and wait_for_service_async. Eliminate GraphListener. Don't try to protect the user from starvation. If they block in a callback without using a coroutine that's their problem.

from rclpy.

I think it would be good to eliminate the GraphListener to reduce complexity. Imo it is reasonable to expect users to know / learn about coroutines for that specific use case. But I think offering both as described in option 3 would be nice for the users. It allows them to use a synchronous call if they want to. And if they use it wrongly it might not work. So my preference would be option 3 slightly before option 1.

from rclpy.