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

process: remove Future impl on Child #2796

Merged
merged 3 commits into from Sep 7, 2020
Merged

process: remove Future impl on Child #2796

merged 3 commits into from Sep 7, 2020

Conversation

ipetkov
Copy link
Member

@ipetkov ipetkov commented Aug 28, 2020

Motivation

The tokio::process::Child type implements Future directly which makes it awkward to use, especially when coming from the synchronous version from the standard library. For example, it is impossible to check the status of the command from outside of async contexts.

Solution

  • Remove the Future impl on Child in favor of an async .wait() method, which mirrors the stdlib API
  • Add a .try_wait() method similar to the stdlib one. This API will not register for wake ups and is safe to invoke from non-async code
  • Both of these methods are "fused" in the sense that repeatedly calling them will always yield the same exit code once the child process has been reaped
  • I have changed the Child::id method to return an Option<u32> to better note that once a child has been reaped, it may not be safe to reuse that identifier (e.g. if the caller wants to interact with the raw process directly). This is the only API which differs from the stdlib

Refs #2432

* This brings our APIs closer to those in std and it allows us to
  internally fuse the future so that repeated calls to `wait` result in
  the same value (similar to std) without forcing the caller to fuse the
  outer future
* Also change `Child::id` to return an Option result to avoid
  allowing the caller to accidentally use the pid on Unix systems after
  the child has been repaed
@Darksonn Darksonn added A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-process Module: tokio/process labels Aug 28, 2020
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

I had a read through, and it seems reasonable.

@ipetkov ipetkov merged commit 842d556 into tokio-rs:master Sep 7, 2020
@ipetkov ipetkov deleted the process-refactor branch September 7, 2020 03:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-process Module: tokio/process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants