oconnor663 / os_pipe.rs

a cross-platform library for opening OS pipes in Rust
MIT License
100 stars 16 forks source link

Conversion to ChildStd{in,out,err} #23

Open Darksonn opened 3 years ago

Darksonn commented 3 years ago

I think it would be nice to provide a conversion into the standard library ChildStd{in,out,err} types. This could tie in nicely with https://github.com/tokio-rs/tokio/pull/4045 to allow converting these into non-blocking pipes usable in Tokio.

oconnor663 commented 3 years ago

I'm not sure this conversion is possible. ChildStdout etc. implement AsRawFd and IntoRawFd but not FromRawFd. And I think that's a reasonable choice for them to make. Implementing FromRawFd would be saying "we promise that no method of this type will ever do anything with a contained file descriptor that isn't safe for all possible file descriptors." That's a pretty big guarantee that the standard library would prefer not to make. Instead, currently, standard library code is able to assume that any instance of ChildStdout came from std::process::Child.

To support conversions like this, Tokio itself would need to store a more generic type. The approach that os_pipe uses is to just contain a regular File, which in practice is a transparent wrapper around a file descriptor. (File doesn't guarantee that, so technically this assumption could be broken in the future, but I think many other crates do something similar.) Alternatively, it could implement a wrapper type of its own, but this usually just ends up looking a lot like File.

Darksonn commented 3 years ago

That's unfortunate. I did look at the impls but apparently missed that it only had conversions in one direction.

I just came across issue #6. I'm not too familiar with the windows APIs, but I think I've read somewhere that you have to set the non-blocking flag on creation of the pipe. Is this correct?

oconnor663 commented 3 years ago

It seems to be a bit more complicated than that. What I wrote in #6 is based on this comment in the Rust standard library:

    // Note that we specifically do *not* use `CreatePipe` here because
    // unfortunately the anonymous pipes returned do not support overlapped
    // operations. Instead, we create a "hopefully unique" name and create a
    // named pipe which has overlapped operations enabled.
    //
    // Once we do this, we connect do it as usual via `CreateFileW`, and then
    // we return those reader/writer halves. Note that the `ours` pipe return
    // value is always the named pipe, whereas `theirs` is just the normal file.
    // This should hopefully shield us from child processes which assume their
    // stdout is a named pipe, which would indeed be odd!

The resulting code is substantially more complicated than creating a regular pipe. It seems like this is necessary for async IO, but I'm not clear on the details or to what extent this is still true on modern Windows.

Darksonn commented 3 years ago

I see. That does sound like it complicates the API, because you have say up front which pipes are intended for the new process, and which are intended for the async application.