Skip to content

Fixing SystemError when nb_bool/nb_nonzero sets a Python exception in type_caster<bool>::load #1976

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

Merged
merged 1 commit into from
Nov 14, 2019

Conversation

YannickJadoul
Copy link
Collaborator

If the error indicator is set by a failing conversion in type_caster<bool>::load (e.g., by trying to convert a numpy.ndarray of 2 or more values, resulting in ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()), the error is not noticed and Python complains that a non-NULL value is returned while an error has been set (SystemError: [...] returned a result with an error set).

This PR should fix that.

NOTE: Without the changes to cast.h, the newly added test fails:

terminate called after throwing an instance of 'pybind11::error_already_set'
  what():  SystemError: <class 'type'> returned a result with an error set

Is that not unexpected as well? Shouldn't all error_already_set exceptions be caught by pybind11?

@YannickJadoul
Copy link
Collaborator Author

YannickJadoul commented Oct 31, 2019

(Thanks to @tdegeus for reporting on Gitter and sticking along while debugging, btw!)

@YannickJadoul
Copy link
Collaborator Author

YannickJadoul commented Oct 31, 2019

Is that not unexpected as well? Shouldn't all error_already_set exceptions be caught by pybind11?

OK, this is because py::repr and py::str are called in cpp_function::dispatcher when trying to report that the function could not be called, but this is outside the big try-catch. Somewhere in there, Python notices that there's been an error, returns NULL as result of PyObject_Repr, and pybind11 throws an error_already_set, causing the Python interpreter to crash.
Not entirely clean/expected, but independent from the problem in this PR.

Ready to merge, as far as I'm concerned, then!

@tdegeus
Copy link
Contributor

tdegeus commented Nov 1, 2019

Thanks @YannickJadoul !

@wjakob
Copy link
Member

wjakob commented Nov 1, 2019

What strikes me as odd about this PR is why this is something specific to the bool caster? Can't these types of errors crop up elsewhere as well?

@YannickJadoul
Copy link
Collaborator Author

YannickJadoul commented Nov 1, 2019

@wjakob The main problem is calling into the C API without checking the result (i.e., if it's -1, in this case). If that call goes wrong, PyErr_Occurred() will be not-NULL, but pybind11 continues because it expects errors to throw a error_already_set exception.

When other casters call into the C API, they do check the result for errors and clear if necessary. For example:

integer-types caster, type_caster<T, enable_if_t<std::is_arithmetic<T>::value && !is_std_char_type<T>::value>>:

bool py_err = py_value == (py_type) -1 && PyErr_Occurred();

string_caster<typename StringType, bool IsView = false>:

if (!temp) { PyErr_Clear(); return false; }

@wjakob
Copy link
Member

wjakob commented Nov 14, 2019

Ok, fair enough -- merged, thanks!

@wjakob wjakob merged commit d8edcd7 into pybind:master Nov 14, 2019
wjakob pushed a commit that referenced this pull request Nov 14, 2019
@YannickJadoul
Copy link
Collaborator Author

The two Travis CI jobs are failing because something (brew in one case) times out; it shouldn't be a problem with this PR.

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.

3 participants