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

Show the original error if calling director method failed #1096

Closed
wants to merge 1 commit into from

Conversation

vadz
Copy link
Member

@vadz vadz commented Sep 16, 2017

Previously, if calling the director method failed, the precise error
message was lost and just "Error detected" was shown to the user, making
it difficult to understand what the problem actually was.

Change the code to show the contents of the last Python error too to
make finding and fixing errors in Python director methods much simpler.

Previously, if calling the director method failed, the precise error
message was lost and just "Error detected" was shown to the user, making
it difficult to understand what the problem actually was.

Change the code to show the contents of the last Python error too to
make finding and fixing errors in Python director methods much simpler.
@vadz vadz added the Python label Sep 16, 2017
PyErr_NormalizeException(&exception, &v, &tb);

if (PyObject *s = PyObject_Str(v)) {
if (const char *cstr = SWIG_Python_str_AsChar(s)) {
Copy link
Member

Choose a reason for hiding this comment

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

How likely are these checks to fail? We could avoid converting msg to std::string and back when we're just going to use it unaltered.

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIK they can never fail at all, the checks are just there to account for the "impossible" failures.

Copy link
Member

Choose a reason for hiding this comment

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

OK, then there's no point complicating things.

It'd be good to have a testcase covering this change, but otherwise it looks sensible to me.

@letmaik
Copy link

letmaik commented Oct 31, 2017

See my comment at #1117 (comment). I don't think this PR is the right approach.

@degasus
Copy link
Contributor

degasus commented Jul 7, 2020

I have the opposite opinion of letmaik here. There are valid reasons why some exceptions must be handled in pure C++ (e.g. async worker threads calling broken python code), however I want to ask if the error message fetching should be done in the what() method instead. This removes the overhead of forwarded exceptions, and at the same time, it provides a way for pure C++ handlers to understand the issue.

@ojwb
Copy link
Member

ojwb commented Aug 4, 2022

In #1117 wsfulton said:

I agree that we do need a better default and the default should be just to [throw] the original exception. The behaviour should be fully customisable too, for example it should be possible to not have any C++ exceptions as requested in #315.

Based on that I'm closing this as not the preferred approach.

@degasus said above:

There are valid reasons why some exceptions must be handled in pure C++ (e.g. async worker threads calling broken python code), however I want to ask if the error message fetching should be done in the what() method instead. This removes the overhead of forwarded exceptions, and at the same time, it provides a way for pure C++ handlers to understand the issue.

I think you'd need to provide a custom director:except typemap if you want that.

@ojwb ojwb closed this Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants