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

Avoid calling send() on awaiting native coroutine objects #963

Closed
wants to merge 1 commit into from

Conversation

m2-farzan
Copy link

Fixes #962

Rationale

If a native coroutine (i.e. async def) is to be called manually (i.e. using u = f(); while True: u.send(None), instead of asyncio.create_task), like in the case of our Task class, then the send call should only be done when the coroutine is ready to execute code (i.e. when it has finished an await line). This can be examined by storing the future object returned by the send function.

Disclaimer

Please review carefully (especially if this is to be backported), as I'm not too confident about the side-effects this one might have. Thanks!

@audrow audrow changed the base branch from master to rolling June 28, 2022 14:21
@sloretz sloretz self-requested a review July 8, 2022 21:19
if self._future is None or self._future.done():
if self._future and self._future.exception():
raise self._future.exception()
self._future = self._handler.send(None)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this can assume a coroutine will return a future. It seems like the behavior is specific to the async library being used (see #962 (comment)). This change is interesting though because it seems like it would both satisfy an rclpy.Future by calling send(None) until it raised StopIteration, and would only call asyncio.Future.__await__ once.

It still needs a way to wake the executor when an asyncio future completes, and given that asyncio isn't thread safe I bet there would be problems if a multithreaded executor was used.

@sloretz
Copy link
Contributor

sloretz commented Jul 28, 2022

I'll close this one in favor of #971 for now. It can be reopened if this ends up looking like a better way forward.

@sloretz sloretz closed this Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Runtime error when a long async callback is triggered
2 participants