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.

Sign up for GitHub

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

Jump to bottom

Explicitly handle CtrlC on Windows #235

Merged
merged 2 commits into from
Feb 27, 2025
Merged

Explicitly handle CtrlC on Windows #235

merged 2 commits into from
Feb 27, 2025

Conversation

ericmarkmartin
Copy link
Contributor

Make the Windows implementation of read_single_key respect the ctrl_c_key argument by un-setting the ENABLE_PROCESS_INPUT flag on the console mode.

We use RAII here to make a best-effort at resetting the mode to its previous state when done so as not to interfere with other applications that might want Ctrl-C to be processed by windows.

Tested by modifying the keyboard example to accept a flag -r | --raw which tells the example to use read_key_raw instead of read_key for consuming input. When running the example with this arg passed, we can hit Ctrl-C and see CtrlC printed out.

@ericmarkmartin ericmarkmartin mentioned this pull request Feb 26, 2025
Merged
djc
djc reviewed Feb 26, 2025
View reviewed changes
@ericmarkmartin ericmarkmartin force-pushed the handle-windows-ctrlc branch 4 times, most recently from ed62e0c to b786dbb Compare February 27, 2025 02:29
@djc djc merged commit 3c7ba95 into console-rs:main Feb 27, 2025
15 checks passed
@djc
Copy link
Member

djc commented Feb 27, 2025

Thanks!

@djc djc mentioned this pull request Feb 27, 2025
Merged
@ericmarkmartin ericmarkmartin deleted the handle-windows-ctrlc branch February 28, 2025 02:25
This was referenced Mar 1, 2025
Closed
Merged
kodiakhq bot pushed a commit to pdylanross/fatigue that referenced this pull request Mar 3, 2025
@dependabot
chore: Bump console from 0.15.10 to 0.15.11 (#397)
4991af5 
Bumps console from 0.15.10 to 0.15.11.

Release notes
Sourced from console's releases.

0.15.11
What's Changed

Don't eagerly close tty fd in read_secure by @​Noratrieb in console-rs/console#222
Make functions on Style const by @​tamird in console-rs/console#220
Fix some typos by @​waywardmonkeys in console-rs/console#213
Improve type safety, extract identical code by @​tamird in console-rs/console#223
Unix tweaks by @​djc in console-rs/console#230
Fix WASI target name by @​djc in console-rs/console#236
Simplify & update by @​djc in console-rs/console#237
fix: 🐛 remove double-width characters correctly by @​bestgopher in console-rs/console#234
Explicitly handle CtrlC on Windows by @​ericmarkmartin in console-rs/console#235
Windows mode tweaks by @​djc in console-rs/console#239




Commits

c7002e3 Refer to GitHub Releases
73033ec Bump version to 0.15.11
4fa21ad Simplify windows read_single_key()
456eb78 Remove unused cruft
8623cd6 Simplify ConsoleModeGuard::set()
ca2e0f8 Use match for creation of new console mode
2f7184f Add docstring for Windows set_console_mode()
3c7ba95 Explicitly handle CtrlC on Windows (#235)
de16ee6 Run clippy on all major platforms
cb6126e Extend application of clippy
Additional commits viewable in compare view




Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

@dependabot rebase will rebase this PR
@dependabot recreate will recreate this PR, overwriting any edits that have been made to it
@dependabot merge will merge this PR after your CI passes on it
@dependabot squash and merge will squash and merge this PR after your CI passes on it
@dependabot cancel merge will cancel a previously requested merge and block automerging
@dependabot reopen will reopen this PR if it is closed
@dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
@dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency
@dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
charliermarsh pushed a commit to astral-sh/uv that referenced this pull request Mar 3, 2025
@ericmarkmartin
explicitly handle ctrl-c in confirmation prompt instead of signal han…
d57bb90 
…dler (#11897)

<!--
Thank you for contributing to uv! To help us out with reviewing, please
consider the following:

- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title?
- Does this pull request include references to any relevant issues?
-->

## Summary

Follow on to #11706. In the original PR, I tried to solve the issue by
getting rid of the `ctrlc::set_handler` call. Unfortunately, this didn't
work on windows due to an issue with the console crate. console 0.15.11
includes console-rs/console#235, which resolves
the issue, so now we can get rid of the call.

<!-- What's the purpose of the change? What does it do, and why? -->

This change is not super important but I still think it's worthwhile.
For one, spinning up a background thread to handle `SIGINT`s when we're
going to be raising the `SIGINT` from within the function is more
technical complexity than needed, now that there's an easy way to
explicitly catch the Ctrl-C from the terminal input. Secondly,
`ctrlc::set_handler`'s
[docs](https://docs.rs/ctrlc/3.4.5/ctrlc/fn.set_handler.html) advise
that you set the handler just once, at the beginning of the program, so
this use seems somewhat error prone. In fact, uv already has a second
[callsite](https://github.com/astral-sh/uv/blob/461f4d9007160f7061a4fc0c4a5a84c613fdbff7/crates/uv/src/commands/project/add.rs#L596-L611)
for this function (though I'm not sure if the two callsites could
currently ever both occur on the same run of uv)

## Test Plan

I've tested this manually on linux (WSL ubuntu) and windows, though not
on aarch64-apple-darwin as I don't have a machine running that. I would
appreciate if someone would double-check that it works on such machines.

As discussed in the original PR, this change is pretty hard to test due
to the fact that the behavior only occurs if stderr is connected to a
tty. I experimented with using pseudoterminals to test this but it's
still quite tricky due to the lack of x-platform non-blocking reads on
the pty.

<!-- How was it tested? -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djc djc djc left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ericmarkmartin @djc