Skip to content

Synchronous Windows stdin pipes broke tokio #97124

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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``
# for free to join this conversation on GitHub. Already have an account? # 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