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

feat: improve ctrl-c handling to deal with blocking threads and terminal shenanigans #2950

Merged
merged 10 commits into from
Jun 7, 2023

Conversation

lemmih
Copy link
Contributor

@lemmih lemmih commented Jun 7, 2023

Summary of changes

Changes introduced in this pull request:

  • Always shut down tokio using shutdown_timeout. Otherwise, we will wait indefinitely for blocking threads.
  • Wrap blocking calls in spawn_blocking.
  • Clean up terminal settings if Forest is interrupted.
  • Replace rpassword with dialoguer.

Reference issue to close (if applicable)

Closes

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

@lemmih lemmih marked this pull request as draft June 7, 2023 14:12
@lemmih lemmih marked this pull request as ready for review June 7, 2023 14:22
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change should go in first, it monkey-patches ctrl+c handling for password prompts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this being #2893?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

@aatifsyed aatifsyed self-requested a review June 7, 2023 14:56
@lemmih lemmih enabled auto-merge (squash) June 7, 2023 16:10
Copy link
Member

@LesnyRumcajs LesnyRumcajs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to work flawlessly, thanks!

@lemmih lemmih merged commit be9d3a1 into main Jun 7, 2023
21 checks passed
@lemmih lemmih deleted the lemmih/better-ctrlc-handling branch June 7, 2023 16:36
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.

None yet

3 participants