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

Fix ctrl-c causing reads of stdin to return empty on Windows. #89433

Merged
merged 1 commit into from
Oct 14, 2021

Conversation

arlosi
Copy link
Contributor

@arlosi arlosi commented Oct 1, 2021

Pressing ctrl+c (or ctrl+break) on Windows caused a blocking read of stdin to unblock and return empty, unlike other platforms which continue to block.

On ctrl-c, ReadConsoleW will return success, but also set LastError to ERROR_OPERATION_ABORTED.

This change detects this case, and re-tries the call to ReadConsoleW.

Fixes #89177. See issue for further details.

Tested on Windows 7 and Windows 10 with both MSVC and GNU toolchains

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 1, 2021
@fee1-dead
Copy link
Member

Can you add an ui test for this? (see this for how to add one yourself)

Ideally we would want to make sure this behavior is consistent across all the platforms that we support.

It would probably look like this file, although I am not sure how to programmatically send Ctrl-C to a process... (and there might be platforms that do not care about Ctrl-Cs)

@Mark-Simulacrum
Copy link
Member

I think a test will be difficult -- maybe a run-make test with a kill command sending the appropriate signal? Seems complicated and may not be warranted.

I'm going to r? @dtolnay since this seems like it'll require a T-libs-api (or so) FCP, as a breaking change.

@arlosi
Copy link
Contributor Author

arlosi commented Oct 2, 2021

Yes, adding a test is rather difficult. Windows doesn't really have a kill like syscall that sends SIGINT.

The closest thing seems to be setting up separate process group for a subprocess, then calling the Windows GenerateConsoleCtrlEvent API on the group. However, I wasn't able to reproduce the current behavior using this approach -- and even if we could make it work, the test would be very Windows-specific.

@dtolnay dtolnay added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 3, 2021
@dtolnay
Copy link
Member

dtolnay commented Oct 3, 2021

This PR closes an inconsistency between how the standard library implements reading from stdin on Windows vs other platforms. #89177 (comment) is the clearest example of the discrepancy: ctrl+C during a read would previously cause the read to return on Windows, but not other platforms, while the ctrl+C handler runs.

I don't believe we want to keep this as a platform-specific inconsistency. As for which behavior to go with, I would biasedly say the non-Windows behavior seems better to me, which this PR makes the Windows implementation consistent with.

@rust-lang/libs-api
@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Oct 3, 2021

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Oct 3, 2021
@BurntSushi
Copy link
Member

This seems okay to me from a consistency perspective, but I wonder how other ecosystems handle this? Has anyone done a survey?

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Oct 4, 2021
@rfcbot
Copy link

rfcbot commented Oct 4, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Oct 4, 2021
@arlosi
Copy link
Contributor Author

arlosi commented Oct 4, 2021

I tested a few different languages on Windows 10 with the program:

  • Set signal handler to catch ctrl-c and print a message
  • Read a line of text

The behavior of the readline call after pressing ctrl-c are a mixture on Windows:

Language readline keeps blocking readline raises error
Rust (now)
Rust (this PR) ✔️
Go ✔️ (EOF)
Node (JavaScript) ✔️
Ruby ✔️
.NET Core
Python ✔️ (Exception)

All test programs have the same behavior on Linux (keep blocking, no error). For consistency, converging on "keep blocking, no error" (as this PR does) makes the most sense to me.

The alternative of returning an error in Rust can cause problems if no signal handler is set up, since Windows kills the thread after a few milliseconds, sometimes causing a half-printed error message.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Oct 14, 2021
@rfcbot
Copy link

rfcbot commented Oct 14, 2021

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Oct 14, 2021
@joshtriplett
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 14, 2021

📌 Commit 273e522 has been approved by joshtriplett

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 14, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 14, 2021
Fix ctrl-c causing reads of stdin to return empty on Windows.

Pressing ctrl+c (or ctrl+break) on Windows caused a blocking read of stdin to unblock and return empty, unlike other platforms which continue to block.

On ctrl-c, `ReadConsoleW` will return success, but also set `LastError` to `ERROR_OPERATION_ABORTED`.

This change detects this case, and re-tries the call to `ReadConsoleW`.

Fixes rust-lang#89177. See issue for further details.

Tested on Windows 7 and Windows 10 with both MSVC and GNU toolchains
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 14, 2021
…askrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#89390 (Fix incorrect Box::pin suggestion)
 - rust-lang#89433 (Fix ctrl-c causing reads of stdin to return empty on Windows.)
 - rust-lang#89823 (Switch order of terms to prevent overflow)
 - rust-lang#89865 (Allow static linking LLVM with ThinLTO)
 - rust-lang#89873 (Add missing word to `FromStr` trait documentation)
 - rust-lang#89878 (Fix missing remaining compiler specific cfg information)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d177791 into rust-lang:master Oct 14, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 14, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Execution continues after stdin read_line when ctrl-c is pressed on Windows