Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Asyncronous wait_for_service #58

Open
mikaelarguedas opened this issue Dec 7, 2016 · 3 comments
Open

Asyncronous wait_for_service #58

mikaelarguedas opened this issue Dec 7, 2016 · 3 comments
Labels
enhancement New feature or request

Comments

@mikaelarguedas
Copy link
Member

mikaelarguedas commented Dec 7, 2016

and remove all sleep calls made to compensate (e.g. https://github.com/ros2/examples/pull/140/files/247edc9b17cdcfc46ee7de8d9275b240c29d4156#diff-3915e3a2b8f90923e42c5386534ed581R32)

@mikaelarguedas
Copy link
Member Author

@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!

@sloretz sloretz added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Nov 9, 2017
@sloretz
Copy link
Contributor

sloretz commented Nov 15, 2017

@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.

@dirk-thomas
Copy link
Member

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.

@sloretz sloretz changed the title Add wait_for_service Asyncronous wait_for_service Mar 22, 2018
@sloretz sloretz removed the in progress Actively being worked on (Kanban column) label May 11, 2018
@sloretz sloretz removed their assignment Feb 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants