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: terminal clobbering #142

Merged
merged 1 commit into from
Sep 21, 2023

Conversation

kzantow
Copy link
Contributor

@kzantow kzantow commented Sep 20, 2023

This PR applies the fix from anchore/grype#1505 to avoid potential terminal clobbering

Signed-off-by: Keith Zantow <[email protected]>
@kzantow kzantow added the bug Something isn't working label Sep 20, 2023
} else {
m.program.Kill()
}
_ = runWithTimeout(250*time.Millisecond, func() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to make sure I understand the change, we're basically saying, "if we're exiting in a controlled manner, wait for the UI to finish. Otherwise, wait 250 milliseconds for the UI to finish," so that the UI has a better chance to restore the terminal to its normal state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. If we're in a "force shutdown" mode, wait only a short time for the handler and UI to complete. This short time, however, is ages longer than the near instantaneous end of the main thread in the program, which was the culprit for leaving the terminal in a bad state.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation!

@kzantow kzantow merged commit 968adaa into anchore:main Sep 21, 2023
6 checks passed
@kzantow kzantow deleted the fix/terminal-clobbering branch September 21, 2023 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants