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: Windows tests failing on 1.62 #4801

Closed
Noah-Kennedy opened this issue Jul 1, 2022 · 7 comments · Fixed by #4803
Closed

process: Windows tests failing on 1.62 #4801

Noah-Kennedy opened this issue Jul 1, 2022 · 7 comments · Fixed by #4803

Comments

@Noah-Kennedy
Copy link
Contributor

With the new version of rust having landed, windows tests are failing. Specifically, it seems that the pipe_from_one_command_to_another test is failing.

I'm currently investigating what exactly is prompting this to fail on windows. Once the root cause is identified, we can figure out how to proceed from there. It may be an std issue or it may be on us.

CI build failure: https://github.com/tokio-rs/tokio/runs/7155496538?check_suite_focus=true#step:8:228

@Noah-Kennedy Noah-Kennedy linked a pull request Jul 1, 2022 that will close this issue
@pie-flavor
Copy link

pie-flavor commented Jul 1, 2022

The error message is generated from this change right here
rust-lang/rust#95469

@Noah-Kennedy
Copy link
Contributor Author

Thanks!

@ChrisDenton
Copy link

Hi, the linked PR fixed a soundness issue reported by Microsoft in rust-lang/rust#81357

The short version is that passing async file handles through stdio is unsound. They must be synchronous. The "fix" was to abort the process when stdio reads or writes do not complete synchronously.

This did however have the effect of breaking code. Including within the stdlib itself. A rather inelegant "pipe relay" (rust-lang/rust#95841) had to be added to turn async pipes synchronous.

Iirc in tokio's case it's mio giving out async pipe handles, which will cause problems when passed to stdin or stdout.

See also: #4670 (aka my failed attempt to use more synchronous pipes in stdlib).

@Noah-Kennedy
Copy link
Contributor Author

Thanks @ChrisDenton!

I actually read that, and just wrote a change that should fix this failing test, however I now need to write up a bunch of documentation changes.

Noah-Kennedy added a commit that referenced this issue Jul 2, 2022
…implementation

This fixes #4801, where, as a result of rust-lang/rust#95469, our implementation of cat used for this test no longer works, as stdio functions on windows now can abort the process if the pipe is set to nonblocking mode.

Unfortunately in windows, setting one end of the pipe to be nonblocking makes the whole thing nonblocking, so when, in tokio::process we set the child pipes to nonblocking mode, it causes serious problems for any rust program at the other end.

Fixing this issue is for another day, but fixing the tests is for today.
Noah-Kennedy added a commit that referenced this issue Jul 2, 2022
…implementation (#4803)

This fixes #4801, where, as a result of rust-lang/rust#95469, our implementation of cat used for this test no longer works, as stdio functions on windows now can abort the process if the pipe is set to nonblocking mode.

Unfortunately in windows, setting one end of the pipe to be nonblocking makes the whole thing nonblocking, so when, in tokio::process we set the child pipes to nonblocking mode, it causes serious problems for any rust program at the other end.

Fixing this issue is for another day, but fixing the tests is for today.
@Noah-Kennedy
Copy link
Contributor Author

@ChrisDenton I don't see this in the rust release notes, did I miss something, or was it omitted?

@ChrisDenton
Copy link

Hm you're right. The pipe relay thing was added to the notes but the issue it was working around wasn't. That appears to have been an oversight. I'll see to fixing that.

@Noah-Kennedy
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants