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

Restore cursor if dialoguer Ctrl+C-ed #2559

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

itowlson
Copy link
Contributor

Fixes #2557.

At the moment, this prints Error: read interrupted after the Ctrl+C. I've tried some of the suggested fixes and they have either had the same quirk, or not fixed the problem, or both. So for now I'm accepting that ugliness as a compromise. Better offers eagerly accepted!

This PR sets the Ctrl+C handler up at the entry to every command that uses dialoguer hidden-cursor functions. This is safer than putting it near the point of use, because you can only set a Ctrl+C handler once per process; and we cannot put it at the top of the whole program because some commands need different/extra Ctrl+C processing (e.g. spin up tearing down triggers). This can result in setting up a handler that might never be needed (and it's not 100% clear if that could have unexpected side effects).

There is code duplication here: we could move it into common or terminal but that would introduce new dependencies into those crates, which are used quite widely. But maybe it's worth it.

@itowlson itowlson requested a review from lann June 13, 2024 22:48
@itowlson
Copy link
Contributor Author

Calling std::process::exit inside the Ctrl+C handler gets rid of the "read interrupted." (At least in some cases, and I can probably address the one where it doesn't.) Not sure if that's too blunt an instrument though?

@lann
Copy link
Collaborator

lann commented Jun 13, 2024

I don't think there is any problem with an ugly error when you rudely interrupt the process 🤷

I am a bit confused about where the error is coming from though...

@itowlson
Copy link
Contributor Author

Yeah I am starting to wonder if it's a dialoguer error - i.e. the program doesn't exit, because of the Ctrl+C, so dialoguer has to return an error, and we propagate the error in the usual way. That's what was going on with the free text looping issue.

@itowlson
Copy link
Contributor Author

Yeah that's it. Bother. Let me put this on hold and faff with it...

@itowlson itowlson marked this pull request as draft June 13, 2024 23:36
@itowlson
Copy link
Contributor Author

Okay, dialoguer returns a std::io::Error with a kind of ErrorKind::Interrupted. So we can handle this, but I'm not sure there's much more we can do than check for it at top level and... well... exit the process. At which point I'm not sure we gain anything over exiting in the Ctrl+C handler. I guess some more destructors run...

@lann
Copy link
Collaborator

lann commented Jun 14, 2024

Ohh so the ctrlc handler is just suppressing the default sigint handler but still causes the input to interrupt.

@itowlson itowlson marked this pull request as ready for review June 14, 2024 01:39
@itowlson
Copy link
Contributor Author

itowlson commented Jun 14, 2024

I don't think anybody is going to much like the fix for the "read interrupted" thing, so I've put it in a separate commit to make it easy to back out. But it seems to provide a reasonably graceful experience at least.

That said, I'm still wary of the Ctrl+C handler. How will it affect parts of the program that are not performing user interaction? I guess in those cases it will eventually cause an I/O error and the error will eventually bubble up and take the program down. So it is probably okay. Except we will get "Read interrupted" errors instead of just exiting. Ugh.

Feedback very welcome.

@lann
Copy link
Collaborator

lann commented Jun 14, 2024

That said, I'm still wary of the Ctrl+C handler. How will it affect parts of the program that are not performing user interaction?

Another option here would be to add a "sigint manager" that sets a single global interrupt handler which could itself be customized in a scoped way for certain cases, maybe something like:

fn ask_a_question() {
  let _sigint_guard = sigint_manager::set_handler_process_exit(1);
  ... dialog stuff ...
  // dropping _sigint_guard restores default handler
}

@itowlson
Copy link
Contributor Author

That would still require us to have a Ctrl+C handler set up at all times. It might be a do-nothing handler but it would still prevent the OS default handling. And that prevention of the OS default handling is what I'm worried about. Of course, we could simulate the OS default behaviour in our own default handler, as in my mention of calling std::process::exit. But in that case we might just as well hardwire it in after the "restore cursor" call.

@radu-matei
Copy link
Member

The outcome of not having to reset your terminal session after interrupting an interactive command is something very useful and I'd really like to see it merged in some way.

@lann lann removed their request for review November 21, 2024 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terminating the app while a prompt is waiting will not restore terminal settings
3 participants