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 custom panic handler (#4884) #8219

Merged
merged 1 commit into from Sep 4, 2019

Conversation

@gshuflin
Copy link
Contributor

commented Aug 28, 2019

Problem:
cf. #4884 , the custom panic handler was previously assuming that the payload of a panic was of type &str, and calling .unwrap() on the result of
downcast_ref::<&str>. However, depending on how panic! was invoked, the
type of the payload may be &str, String, or in principle some other
type. Since there are cases in the pants codebase where panic! is
invoked with format!-style macros, resuling in a String rather than &str
payload, these panics induced an additional panic in the .unwrap(),
which displayed "thread panicked while processing panic" and aborted,
rather than showing the proper error message.

Solution:
This is fixed by removing the .unwrap() and explicitly handling all possible cases.

Result:
&str and String panic messages are treated identically and the panic-in-panic-handler is not triggered.

@stuhood
Copy link
Member

left a comment

Thanks!

@stuhood

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

@gshuflin : For future reference, to indicate that a PR "fixes an issue", you can put text like "fixes #4884" in the PR description, and github will automatically resolve the linked ticket when the PR is merged. It might also work in titles, but it's better information to include in the body.

@cosmicexplorer
Copy link
Contributor

left a comment

Thank you so much! Is it possible to add a unit test for this handler (by extracting its logic into a separate function)?

Some(s) => format!("panic at '{}'", s),
None => "Non-string panic payload at unknown location".to_string(),
},
};

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Aug 28, 2019

Contributor

I believe this could also be written as:

let mut panic_str = payload.downcast_ref::<&str>().or_else(payload.downcast_ref::<String>)
  .map(|s| format!("panic at '{}'", s))
  .unwrap_or_else(|| "Non-string panic payload at unknown location".to_string)

which avoids the nested match.

Also, is it possible to provide any useful information if the payload isn't a string? Perhaps a memory address?

This comment has been minimized.

Copy link
@gshuflin

gshuflin Aug 29, 2019

Author Contributor

I didn't try this, but I don't think this will typecheck because it's ambiguous whether the |s| in the map is of type &str or String. The double-match seemed like the most straightforward way to make this work given that. The payload is of type Any and as far as I can tell the API doesn't give you a way to get the memory address (which I'm not sure would be helpful anyway), or any other interesting information about the value.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Aug 29, 2019

Contributor

You can add .map(|s| s.to_string()) to convert the &str to a String, I think. I think that this formulation is more readable than the nested match, which I prefer for code that runs in a panic handler.

And it looks like the {:p} format specifier might address the memory address issue: https://stackoverflow.com/a/27852760/2518889. It would at least tell us whether the payload is a null pointer (unless that's ensured by rust with .payload() returning a &?). The default python str() function for user-defined classes just prints the class name and the memory address, so there's some precedent for using the address as a default printable representation.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

Also, I would really deeply appreciate it if we could all follow the Problem/Solution/Result format for PR descriptions that github inserts by default. Many commits to the rust codebase don't have this, and it makes it significantly more difficult to understand what they're doing.

@gshuflin gshuflin force-pushed the gshuflin:fix_panic branch 6 times, most recently from 2de867d to 0de394e Aug 29, 2019

Fix custom panic handler (#4884)
The custom panic handler was previously assuming that the payload of a
panic was of type &str, and calling .unwrap() on the result of
downcast_ref::<&str>. However, depending on how panic! was invoked, the
type of the payload may be &str, String, or in principle some other
type. Since there are cases in the pants codebase where panic! is
invoked with format!-style macros, resuling in a String rather than &str
payload, these panics induced an additional panic in the .unwrap(),
which displayed "thread panicked while processing panic" and aborted,
rather than showing the proper error message. This is fixed by removing
the .unwrap() and explicitly handling all possible cases.
@stuhood

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

You have only known flakes, so I'll go ahead and merge this. Thanks!

@stuhood stuhood merged commit 674c91c into pantsbuild:master Sep 4, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@gshuflin gshuflin deleted the gshuflin:fix_panic branch Sep 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.