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

Synchronous Windows stdin pipes broke tokio #97124

Closed
pietroalbini opened this issue May 17, 2022 · 4 comments
Closed

Synchronous Windows stdin pipes broke tokio #97124

pietroalbini opened this issue May 17, 2022 · 4 comments
Labels
C-bug Category: This is a bug. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Milestone

Comments

@pietroalbini
Copy link
Member

pietroalbini commented May 17, 2022

It was reported in tokio-rs/tokio#4670 that tokio::process::Command broke between nightly-2022-04-29 and nightly-2022-04-30 on Windows. According to the tokio authors that struct is a wrapper over the standard library's std::process::Command. Looking at the PRs between those two nightlies the likely cause seems #96441, as it touches pipes on the affected platform.

@pietroalbini pietroalbini added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 17, 2022
@pietroalbini pietroalbini added this to the 1.62.0 milestone May 17, 2022
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label May 17, 2022
@pietroalbini
Copy link
Member Author

We also hit this in #97088 (comment) when bumping RLS (which depends on tokio). @Mark-Simulacrum is going to revert this in the upcoming beta, and post a revert PR for master soon.

@Mark-Simulacrum
Copy link
Member

Posted master revert (#97127) and added it to #97088 (targeting 1.62) as well.

@Mark-Simulacrum Mark-Simulacrum added regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels May 17, 2022
@m-ou-se
Copy link
Member

m-ou-se commented May 18, 2022

As I posted on tokio-rs/tokio#4670 (comment):

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.

@m-ou-se m-ou-se added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels May 18, 2022
@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label May 18, 2022
JohnTitor added a commit to JohnTitor/rust that referenced this issue May 18, 2022
…u-se

Revert "Auto merge of rust-lang#96441 - ChrisDenton:sync-pipes, r=m-ou-se"

This reverts commit ddb7fbe.

Partially addresses rust-lang#97124, but not marking as fixed as we're still pending on a beta backport (for 1.62, which is happening in rust-lang#97088).

r? `@m-ou-se` `@ChrisDenton`
JohnTitor added a commit to JohnTitor/rust that referenced this issue May 18, 2022
…u-se

Revert "Auto merge of rust-lang#96441 - ChrisDenton:sync-pipes, r=m-ou-se"

This reverts commit ddb7fbe.

Partially addresses rust-lang#97124, but not marking as fixed as we're still pending on a beta backport (for 1.62, which is happening in rust-lang#97088).

r? ``@m-ou-se`` ``@ChrisDenton``
@dtolnay
Copy link
Member

dtolnay commented Jun 17, 2022

Closing in favor of keeping discussion about the path forward in tokio-rs/tokio#4670 + #97149 + #97150.

@dtolnay dtolnay closed this as completed Jun 17, 2022
workingjubilee pushed a commit to tcdi/postgrestd that referenced this issue Sep 15, 2022
Revert "Auto merge of #96441 - ChrisDenton:sync-pipes, r=m-ou-se"

This reverts commit c5c49a0441c0fee2a6a8bf6e007cdf34b6f6f542.

Partially addresses rust-lang/rust#97124, but not marking as fixed as we're still pending on a beta backport (for 1.62, which is happening in rust-lang/rust#97088).

r? ``@m-ou-se`` ``@ChrisDenton``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants