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(console): exit crossterm before printing panic messages #307

Merged
merged 3 commits into from
Mar 9, 2022

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Mar 7, 2022

If the tokio-console CLI panics, we will eventually exit crossterm's
terminal capturing and return to the user's shell. However, this
currently happens after the panic message is printed. This is because
we crossterm in a drop guard that's held in main, but panic messages
are printed out before unwinding.

This means that for most panics, the panic message is never actually
displayed --- instead, the console just crashes to a blank screen, and
then returns to the user's shell with no output. Some manual testing
(e.g. putting panic!("fake panic"); in a few different places)
indicates that panics will only be displayed nicely if they occur prior
to the call to term::init_crossterm() in the main function.

This branch fixes this issue by changing the panic hook to explicitly
exit crossterm before printing the panic. This way, panics should
always be printed to stdout, regardless of where they occur in the
console. I factored out the code for exiting crossterm's terminal
capturing into a function that's now called in the drop guard and in
the panic hook.

I also added some code for logging the panic using tracing. That way,
even if we fail to exit crossterm's terminal capturing, but the
console's debug logs are being redirected to a file, we'll still get the
panic in the logs.

hawkw added 2 commits March 7, 2022 10:15
If the `tokio-console` CLI panics, we will eventually exit `crossterm`'s
terminal capturing and return to the user's shell. However, this
currently happens *after* the panic message is printed. This is because
we `crossterm` in a drop guard that's held in `main`, but panic messages
are printed out _before_ unwinding.

This means that for most panics, the panic message is never actually
displayed --- instead, the console just crashes to a blank screen, and
then returns to the user's shell with no output. Some manual testing
(e.g. putting `panic!("fake panic");` in a few different places)
indicates that panics will only be displayed nicely if they occur prior
to the call to `term::init_crossterm()` in the main function.

This branch fixes this issue by changing the panic hook to explicitly
exit `crossterm` _before_ printing the panic. This way, panics should
always be printed to stdout, regardless of where they occur in the
console. I factored out the code for exiting crossterm's terminal
capturing into a function that's now called in the drop guard _and_ in
the panic hook.

I also added some code for logging the panic using `tracing`. That way,
even if we fail to exit crossterm's terminal capturing, but the
console's debug logs are being redirected to a file, we'll still get the
panic in the logs.
@hawkw hawkw enabled auto-merge (squash) March 7, 2022 18:30
@hawkw hawkw requested a review from a team March 8, 2022 22:03
@hawkw hawkw merged commit 43606b9 into main Mar 9, 2022
@hawkw hawkw deleted the eliza/errors-good branch March 9, 2022 00:11
hawkw added a commit that referenced this pull request Mar 9, 2022
## 0.1.3 (2022-03-09)

#### Bug Fixes

*  exit crossterm before printing panic messages (#307)
   ([43606b9](43606b9))
*  prevent panics if subscriber reports out-of-order times (#295)
   ([80d7f42](80d7f42))

#### Features

*  add icon representing column sorting state (#301)
   ([b9e0a22](b9e0a22))
hawkw added a commit that referenced this pull request Mar 9, 2022
## 0.1.3 (2022-03-09)

#### Bug Fixes

*  exit crossterm before printing panic messages (#307)
   ([43606b9](43606b9))
*  prevent panics if subscriber reports out-of-order times (#295)
   ([80d7f42](80d7f42))

#### Features

*  add icon representing column sorting state (#301)
   ([b9e0a22](b9e0a22))
hawkw added a commit that referenced this pull request Mar 9, 2022
## 0.1.3 (2022-03-09)

#### Bug Fixes

*  exit crossterm before printing panic messages (#307)
   ([43606b9](43606b9))
*  prevent panics if subscriber reports out-of-order times (#295)
   ([80d7f42](80d7f42))

#### Features

*  add icon representing column sorting state (#301)
   ([b9e0a22](b9e0a22))
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.

2 participants