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

Futures and coroutines #166

Merged
merged 9 commits into from
Jan 17, 2018
Merged

Futures and coroutines #166

merged 9 commits into from
Jan 17, 2018

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Jan 4, 2018

This adds coroutine support to the executor and is part of service feature parity #123 and wait_for_service without a GraphListener #58.

Any callback can now be a coroutine function (async def). If the callback is a coroutine and it becomes blocked the executor will move on and resume executing it later. The details of this are handled in the class Task which is a class very similar to asyncio.Task. asyncio can't be used directly because it is not thread safe.

This PR also adds a Future class that represents the result of a stored task. It is not used in this PR, but an example of using it is in #170.

CI ( with 5f2cb1e)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@sloretz sloretz added the in progress Actively being worked on (Kanban column) label Jan 4, 2018
@sloretz sloretz self-assigned this Jan 4, 2018
Adds Task and Future classes based on asyncio
Callbacks can be coroutines, the executor runs them to completion
@sloretz sloretz force-pushed the rclpy_future_coroutine branch from a0d88c8 to d29fa97 Compare January 4, 2018 20:39
If a task is executing, acquired its lock, but not set _executing = True
yet and another thread calls the same task it is possible for the second
thread to be blocked while the first thread executes. The intention is
for the second thread to return immediately.
@sloretz sloretz added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jan 5, 2018
@sloretz
Copy link
Contributor Author

sloretz commented Jan 5, 2018

Oops, looks like the description I put on this task was completely wrong and from another PR. It's fixed now.

@sloretz sloretz added in review Waiting for review (Kanban column) and removed in review Waiting for review (Kanban column) labels Jan 8, 2018
@sloretz
Copy link
Contributor Author

sloretz commented Jan 12, 2018

@ros2/team Friendly ping. I would appreciate a review of this one. It is a prerequisite to multiple concurrent client requests (#170) and a wait_for_service implementation that returns asap instead of sleep/polling.

Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

The patch looks reasonable to me. I haven't tried to use the new API though.

"""
Add a callback or coroutine to be executed during :meth:`spin` and return a Future.

Arguments to this function are passed to the callback.
Copy link
Member

Choose a reason for hiding this comment

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

Could this also permit keyword arguments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, Added in 5f2cb1e

Copy link
Member

Choose a reason for hiding this comment

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

I didn't think about it before. It might be necessary to pass executor using a setter function instead to avoid a collision if the callback wants to use a keyword argument with that name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In 5f2cb1e the expansion of args and kwargs in Task.__init__() was removed to prevent that collision. The expansion on executor.create_task() cannot conflict because they're now passed to Task.__init__() as a sequence and dictionary.

Copy link
Member

Choose a reason for hiding this comment

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

Perfect!

@sloretz sloretz merged commit ddc33ad into master Jan 17, 2018
@sloretz sloretz removed the in review Waiting for review (Kanban column) label Jan 17, 2018
@sloretz sloretz deleted the rclpy_future_coroutine branch January 18, 2018 21:12
@samiamlabs
Copy link

Hi @sloretz!

Is it possible/easy to make the equivalent of asyncio.sleep using the coroutine support from this PR?

(We have been trying to find a robust pattern for waiting in the execute functions of action servers in rclpy. The only pattern that works for us 100% reliably so far is using a SingleThreadedExecutor and spin_once while waiting in the execute callback. It just seems wrong to spin in callbacks though...)

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.

3 participants