rust-cli / human-panic

Panic messages for humans.
https://docs.rs/human-panic
Apache License 2.0
1.66k stars 65 forks source link

Consider double panic case #12

Closed killercup closed 6 years ago

killercup commented 6 years ago

In the callback given to set_hook, we have an explicit unwrap/expect:

https://github.com/yoshuawuyts/human-panic/blob/d86232967d3bf9dc868a4cd68bab2e1004b6d2dc/src/lib.rs#L47-L48

This can lead to panicking while handling a panic. I'm not entirely sure what happens then (we should probably test it), but we can easily mitigate this by just printing a generic error instead.

One step further would be to handle explicit error cases, e.g., not being able to write the temporary file and printing the report to stdout instead.

skade commented 6 years ago

This can lead to panicking while handling a panic. I'm not entirely sure what happens then (we should probably test it), but we can easily mitigate this by just printing a generic error instead.

Double panics abort immediately. The hook is already considered part of the panic.

https://play.rust-lang.org/?gist=da61185963bfcd469df767c48f450708&version=stable

The message is:

thread panicked while processing panic. aborting.
yoshuawuyts commented 6 years ago

One step further would be to handle explicit error cases, e.g., not being able to write the temporary file and printing the report to stdout instead.

Yeah, that sounds pretty good!

spacekookie commented 6 years ago

Fixed by #33