Cannot run any command with piped stdin with Nightly toolchain

[marc2332]

Version tokio v1.18.2

Toolchains

cargo 1.60.0 (d1fd9fe2c 2022-03-01)
cargo 1.62.0-nightly (a44758ac8 2022-05-04)

Platform Windows 11

Description I cannot spawn any tokio::process::Comand with a piped stdin in the Nightly toolchain.

I tried this code:

use tokio::process::Command;
use std::process::Stdio;

#[tokio::main]
async fn main() {
    Command::new("cmd")
        .stdin(Stdio::piped())
        .stdout(Stdio::piped())
        .stderr(Stdio::piped())
        .spawn()
        .unwrap();
}

I did not expect any output because it's being spawned.

Running on the stable toolchain:

Instead, when running with the nightly toolchain, it throws this error:

If I make it inherit stdin (default behavior):

Command::new("cmd")
        .stdout(Stdio::piped())
        .stderr(Stdio::piped())
        .spawn()
        .unwrap();

It will run just fine in the nightly toolchain:

Am I missing something maybe ? 🤔

Thanks!

[Darksonn]

Your code appears to be using std::process. Can you please double check whether this is an issue specific to Tokio. If it isn't, this should be reported to the standard library instead — note that Tokio's process module is a wrapper around the std one.

[marc2332]

Your code appears to be using std::process. Can you please double check whether this is an issue specific to Tokio. If it isn't, this should be reported to the standard library instead — note that Tokio's process module is a wrapper around the std one.

Oh I think I didnt update the example accordingly. I am pretty sure it was failing with tokio's Command too. If thats the case its probably a std bug since tokio's is just a wrapper. I will check it again asap!

[Darksonn]

If it fails in the same error with both the standard library and Tokio, then it's a standard library bug. If that's the case, please close this issue (but feel free to add a link to the std issue).

[marc2332]

I just checked my example of above, and I am correctly using Tokio's Command, not std's. The only thing I import from std is the Stdio struct, which is correct (according for example to this)

That said, I tried using std's Command and it worked fine in both stable and nightly.

So, looks like it only panics with tokio's Command running on nightly

[Darksonn]

Thoughts? @ipetkov

[ipetkov]

Hmm no idea why a nightly toolchain would cause such failures, unless something has changed in the standard library which doesn't play nicely with tokio's integration.

@marc2332 do you know which specific nightly version introduced the regression? alternatively, could you also try with tokio 1.17 to see if it reproduces? (we bumped mio to 0.8.1 with the 1.18 release)

[marc2332]

Hmm no idea why a nightly toolchain would cause such failures, unless something has changed in the standard library which doesn't play nicely with tokio's integration.

@marc2332 do you know which specific nightly version introduced the regression? alternatively, could you also try with tokio 1.17 to see if it reproduces? (we bumped mio to 0.8.1 with the 1.18 release)

I will check at what version it stopped working and get back to you :) Regarding tokio 1.17, I was actually using that version and when the error appeared I upgraded to 1.18 (just in case), where it still threw the same error. I will try with 1.17 again anyway, just to make sure

I will get back to you asap :-)

[Darksonn]

I'm pretty sure mio was bumbed in 1.17.

[marc2332]

Well, now I am more confused than I was before:

I tried downloading some nightly builds, and it stopped working on the one released on 2022-04-30.... or maybe not?

I tried checking the versions of both 2022-04-29 and 2022-04-30 and, they both are running the same build (2022-04-28), yet the one from day 30 fails? Am I going crazy?

[pietroalbini]

We hit this when preparing the 1.62 beta release (RLS broke likely due to this issue), and it seems to be caused by https://github.com/rust-lang/rust/pull/96441. We're still discussing in the Rust release team what to do with that PR.

I tried checking the versions of both 2022-05-29 and 2022-05-30 and, they both are running the same build (2022-05-28), yet the one from day 30 fails? Am I going crazy?

That output is indeed quite confusing. cargo -V outputs the version number, commit and commit date of Cargo, not the ones of Rust. Between those nightlies there was no Cargo change, so the version number was the same. If you instead run rustc -V it will show each nightly is built off a different commit :slightly_smiling_face:

[pietroalbini]

Filed a regression issue in https://github.com/rust-lang/rust/issues/97124. We'll revert the PR on the upcoming 1.62 beta release, and most likely on nightly as well.

[marc2332]

We hit this when preparing the 1.62 beta release (RLS broke likely due to this issue), and it seems to be caused by rust-lang/rust#96441. We're still discussing in the Rust release team what to do with that PR.

I tried checking the versions of both 2022-05-29 and 2022-05-30 and, they both are running the same build (2022-05-28), yet the one from day 30 fails? Am I going crazy?

That output is indeed quite confusing. cargo -V outputs the version number, commit and commit date of Cargo, not the ones of Rust. Between those nightlies there was no Cargo change, so the version number was the same. If you instead run rustc -V it will show each nightly is built off a different commit 🙂

Ohhh!

Here are the exact commits in case it helps :')

[ChrisDenton]

Hi, I'm the one to blame here. So first a bit of background before I get to what caused this issue. Sorry if this ends up being a bit of a journey.

Windows requires you to be upfront about whether or not you plan to do async reads/writes. So if doing async I/O a pipe or a file has to be opened for async access. If doing synchronous I/O it has to be opened for synchronous access.

The I/O handles that are sent to a child process (stdin/out/err) must be opened for synchronous access. Doing otherwise can cause unsound behaviour in the child because they will almost certainly be performing synchronous reads or writes. However, there's nothing wrong with the standard library using async for its side of the pipe because it knows it is async. So that's what it did. The child end is synchronous and our end is asynchronous.

This worked fine until we realised there was a case where our async side of the pipe is given to a child process. That is when chaining processes. We give one (synchronous) side of the pipe to one child and the other (async) side to another child. A workaround was found to ensure only synchronous pipe handles are ever passed to child processes. This appears to have worked fine and hasn't caused problems yet (assuming I haven't just jinxed it).

However, turning an async pipe handle synchronous is a bit awkward so a follow up PR tried to make both ends be created synchronous in some places. Which is fine and dandy for the standard library itself but the twist is we give access to third parties via as_raw_handle. This is what I think is causing the trouble. tokio (probably through mio) is expecting this handle to be async but it's now (in nightly) synchronous.

So that's the full story, as I understand it so far.

[m-ou-se]

It's unfortunate that libraries depend on the child process pipes being asynchronous ones, since that wasn't a documented promise in std. It seems we have two ways forward:

1. Document that the .as_raw_handle() from a std::process::ChildStd{in,out,err} is always an asynchronous pipe on Windows. This means std has to spawn a thread to copy over data from the async pipe into a new sync pipe when the pipe is given to another Command. Or:
2. Document that we don't provide that guarantee, and wait for existing libraries to adapt before changing the actual implementation a few releases later.

For 2, we could add something like a .async_pipes(true) to std::os::windows::process::CommandExt so there's a way to explicitly request async pipes.

[Darksonn]

If you provide an async_pipes config option, then it shouldn't be a problem to add calls to it. We should be able to use a build script to detect the version of Rust and only call it on versions where it exists.

Unfortunately, I don't think it's possible to change whether Tokio wraps the standard library Command struct due to backwards compatibility.

[ipetkov]

With #4824 merged tokio will use synchronous file handles for doing I/O with child processes (read/write operations will be done using the blocking threadpool)