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

implement TryFrom so that std::process:ChildStdxx can be converted to tokio::process:ChildStdxx #2837

Closed
wangbj opened this issue Sep 15, 2020 · 5 comments · Fixed by #4045
Closed
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-process Module: tokio/process

Comments

@wangbj
Copy link

wangbj commented Sep 15, 2020

Is your feature request related to a problem? Please describe.
Both std and tokio have Stdin/Stdout/Stderr type, IMO tokio ones are more powerful because they implements AsyncRead.

In tokio/src/process/unix/mod.rs, tokio's Stdxx are wrappers around std::process::Stdxx:

pub(crate) type ChildStdin = PollEvented<Fd<std::process::ChildStdin>>;
pub(crate) type ChildStdout = PollEvented<Fd<std::process::ChildStdout>>;
pub(crate) type ChildStderr = PollEvented<Fd<std::process::ChildStderr>>;

So it should be really easy to add impl TryFrom<std::process::ChildStdin> for ChildStdin for example, not to mention tokio has function like fn stdio<T>(option: Option<T>) -> io::Result<Option<PollEvented<Fd<T>>>>.

Describe the solution you'd like
Add impl From or TryFrom so that it is possible to convert std::process::Stdxx into tokio::process::Stdxx.

Additional context
Right now it is impossible (from external API) to construct tokio::process::Stdxx without execve (via Command). it would be valuable to construct them with nix::unistd::fork. Since std::process::Stdxx is just a thin-wrapper around FileDesc, thanks to rust's zero cost abstraction, they're identical to c_int (32-bit, or HANDLE/64-bit on Windows). But this is not true for tokio::process::Stdxx, if we can convert std::process::Stdxx into tokio::process::Stdxx, then we can do IO redirections with unistd::fork.

@wangbj wangbj added A-tokio Area: The main tokio crate C-feature-request Category: A feature request. labels Sep 15, 2020
@Darksonn Darksonn added the M-process Module: tokio/process label Sep 15, 2020
@Darksonn
Copy link
Contributor

This seems reasonable to me.

@ipetkov
Copy link
Member

ipetkov commented Sep 16, 2020

IMO there isn't a ton of value in converting an std::ChildStd{in, out, err} to a tokio equivalent in the same sense that there isn't a ton of value in converting a raw file descriptor into an std::ChildStd{in, out, err}, since the latter delegates to the former.

The more interesting feature ask here, in my opinion, would be to support ergonomically converting any raw file descriptor equivalent into something that is Async{Read, Write}.

Doing so today isn't "hard", but it does require knowing about mio's Evented type, making a wrapper around the file descriptor, delegating read/write impls, making the descriptor non-blocking, and passing it into PollEvented (after all, this is what tokio does for the child stdio handles); it would be good to avoid all this boilerplate in general rather than just solving it for the process handles.

I know that our plans for 0.3 involve hiding mio from the public interfaces (with a migration to mio 0.7 to boot). Maybe there are already planned utilities for this, but we should have some sort of api that takes any T: IntoRawFd and converts it to some PollEvented equivalent

@wangbj
Copy link
Author

wangbj commented Sep 16, 2020

The more interesting feature ask here, in my opinion, would be to support ergonomically converting any raw file descriptor equivalent into something that is Async{Read, Write}.

Agreed, I'm already using mio (0.6 because tokio 0.2 depends on it), would this be reasonable feature to add?

@Darksonn
Copy link
Contributor

In my opinion, both make sense.

@ipetkov
Copy link
Member

ipetkov commented Oct 3, 2020

Relevant #2728 and #2903

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-feature-request Category: A feature request. M-process Module: tokio/process
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants