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

Correctly report Python exceptions set but not thrown during import. #1734

Closed
wants to merge 1 commit into from
Closed

Correctly report Python exceptions set but not thrown during import. #1734

wants to merge 1 commit into from

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Mar 20, 2019

Consider the following example:

#include <pybind11/pybind11.h>
PYBIND11_MODULE(python_example, m) {
    PyErr_SetString(PyExc_ImportError, "oops");
}

Currently, importing this module will result in

SystemError: initialization of python_example raised unreported exception

because the Python exception is not converted to a C++ one
(py::error_already_set). Fix that by checking the error flag, so that
one instead gets

ImportError: oops

Real use cases include numpy's import_array() or pycairo's
import_cairo(), which can set the exception flag (and return -1) on
failure. Currently the correct way to handle them with pycairo is

if (import_foo() < 0) { m.ptr() = nullptr; return; }

and this patch removes the need for m.ptr() = nullptr;.

No tests for now for the same reason as in #1362 (I think any proper unit test would require creating a new module just for that purpose, and I'm not particularly familiar with the testing machinery used by pybind, so guidance would be welcome here. But "it works for me"...).

Consider the following example:

    #include <pybind11/pybind11.h>
    PYBIND11_MODULE(python_example, m) {
        PyErr_SetString(PyExc_ImportError, "oops");
    }

Currently, importing this module will result in

    SystemError: initialization of python_example raised unreported exception

because the Python exception is not converted to a C++ one
(py::error_already_set).  Fix that by checking the error flag, so that
one instead gets

    ImportError: oops

Real use cases include numpy's `import_array()` or pycairo's
`import_cairo()`, which can set the exception flag (and return -1) on
failure.  Currently the correct way to handle them with pycairo is

    if (import_foo() < 0) { m.ptr() = nullptr; return; }

and this patch removes the need for `m.ptr() = nullptr;`.
@anntzer
Copy link
Contributor Author

anntzer commented Apr 9, 2019

Kindly bumping. To the best of my understanding, the test failures seem spurious.

@wjakob
Copy link
Member

wjakob commented Jun 10, 2019

Hi @anntzer,

the pybind11 way involves turning such errors into exceptions that are propagated and correctly, and setting PyErr_SetString directly within pybind11 code violates that contract. If you call into other libraries with that behavior, you'll have to check for errors and throw py::error_already_set.

Best,
Wenzel

@wjakob wjakob closed this Jun 10, 2019
@anntzer anntzer deleted the unthrown-exceptions-during-import branch June 10, 2019 20:49
@anntzer
Copy link
Contributor Author

anntzer commented Jun 10, 2019

Ah, I missed the possibility to just throw py::error_already_set in that case, thanks for pointing that out.

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