ctor of object wrapper should propagate possible python exceptions

[MichaelSuen-thePointer]

Issue description

In the ctor of every pybind object wrapper, if PyXXX_New returns nullptr, pybind will invoke pybind11_fail which throws std::runtime_error, which doesn't propagate the already set python MemoryError

pybind should throw a std::bad_alloc or at least py::error_already_set if it detects a python exception is set

py::tuple as a example

Some other places in the code also not propagating MemoryError, like pybind11.hpp#340

Reproducible example code

#include <pybind11/pybind11.h>

namespace py = pybind11;

inline py::tuple make_huge_tuple_pybind() {
    return py::tuple(2147483647);
}

inline py::object make_huge_tuple_cpy() {
    auto tup = PyTuple_New(2147483647);
    if (!tup) {
        throw py::error_already_set();
    }
    return py::reinterpret_steal<py::object>(tup);
}

PYBIND11_PLUGIN(pybindtest) {
    py::module m("pybindtest", "pybindtest");

    m.def("make_huge_tuple_pybind", make_huge_tuple_pybind);
    m.def("make_huge_tuple_cpy", make_huge_tuple_cpy);

    return m.ptr();
}

calling make_huge_tuple_pybind in python got RuntimeError

calling make_buge_tuple_cpy in python got MemoryError

[YannickJadoul]

This seems like a possibility, yes. Quite a corner case because this shouldn't really happen anyway, but fair enough, it would be better.

Do you have time to make a PR of the places where you found this? We'd also need to check if every time a nullptr is returned by the Python C API, whether the API docs specify that an error will be set internally by CPython, or otherwise, throwing py::error_already_set will result in an even more obscure error.

[bstaletic]

Do you have time to make a PR of the places where you found this?

This is the collection of all things PyThing_NewWhatever().

File	Line and Column Numbers	Line	Comment
include/pybind11/pytypes.h	1121 col 18	: object(PyWeakref_NewRef(obj.ptr(), callback.ptr()), stolen_t{}) {	If NULL, throws TypeError
include/pybind11/pytypes.h	1131 col 17	m_ptr = PySlice_New(start.ptr(), stop.ptr(), step.ptr());	If NULL, throws... nothing?
include/pybind11/pytypes.h	1157 col 18	: object(PyCapsule_New(const_cast<void *>(value), name, destructor), stolen_t{}) {	If NULL, throws... "an exception".
include/pybind11/pytypes.h	1164 col 18	: object(PyCapsule_New(const_cast<void*>(value), nullptr, destruct), stolen_t{}) {	If NULL, throws... "an exception".
include/pybind11/pytypes.h	1170 col 17	m_ptr = PyCapsule_New(const_cast<void *>(value), nullptr, [](PyObject *o) {	If NULL, throws... "an exception".
include/pybind11/pytypes.h	1184 col 17	m_ptr = PyCapsule_New(reinterpret_cast<void *>(destructor), nullptr, [](PyObject *o) {	If NULL, throws... "an exception".
include/pybind11/pytypes.h	1206 col 46	explicit tuple(size_t size = 0) : object(PyTuple_New((ssize_t) size), stolen_t{}) {	If NULL, throws... nothing?
include/pybind11/pytypes.h	1220 col 21	dict() : object(PyDict_New(), stolen_t{}) {	If NULL, throws... nothing?
include/pybind11/pytypes.h	1266 col 45	explicit list(size_t size = 0) : object(PyList_New((ssize_t) size), stolen_t{}) {	If NULL, throws... nothing?
include/pybind11/pytypes.h	1290 col 20	set() : object(PySet_New(nullptr), stolen_t{}) {	If NULL, throws... nothing?
include/pybind11/detail/class.h	442 col 16	dict = PyDict_New();	If NULL, throws... nothing?
include/pybind11/detail/common.h	168 col 51	#define PYBIND11_INSTANCE_METHOD_NEW(ptr, class_) PyInstanceMethod_New(ptr)	Doesn't specify that it can fail at all.
include/pybind11/detail/common.h	195 col 51	#define PYBIND11_INSTANCE_METHOD_NEW(ptr, class_) PyMethod_New(ptr, nullptr, class_)	Doesn't specify that it can fail at all.
include/pybind11/pybind11.h	356 col 21	m_ptr = PyCFunction_NewEx(rec->def, rec_capsule.ptr(), scope_module.ptr());	This isn't documented at all
include/pybind11/pybind11.h	1821 col 17	m_ptr = PyErr_NewException(const_cast<char *>(full_name.c_str()), base, NULL);	Doesn't specify that it can fail at all.
include/pybind11/pybind11.h	1945 col 22	tstate = PyThreadState_New(internals.istate);	Doesn't specify that it can fail at all.
include/pybind11/numpy.h	588 col 53	tmp = reinterpret_steal(api.PyArray_NewCopy_(tmp.ptr(), -1 /* any order */));	This is some numpy trickery...
include/pybind11/cast.h	77 col 24	list_ptr = PyList_New(1);	If NULL, throws... nothing?

To elaborate:

• PyWeakref_NewRef throws TypeError when
• "If NULL, throws... nothing?" means that the specs say that this can fail, but not what exception is set.
• PyCFunction_NewEx isn't documented and CPython says that extension modules don't need it.
• "Doesn't specify that it can fail at all." means that the specs don't mention any failure cases as far as I can tell.
• "This is some numpy trickery..." is interesting:
  • api is a reference to detail::npy_api::get() that just calls lookup().
  • lookup() imports numpy.core.multiarray._ARRAY_API as a py::capsule and then extracts void** from it.
  • Now we populate npy_api instance by...
    • Taking offsets into _ARRAY_API
    • and assigning them to members of _ARRAY_API
      • I'm not sure if this is what we're supposed to do. See https://github.com/numpy/numpy/blob/36e017194c32a53cf2965a513243cb3c348095df/numpy/core/code_generators/generate_numpy_api.py#L53
      • The offsets into the void** are wrong. Specifically this thing is at offset 96. Fortunately, that thing isn't being used.

[MichaelSuen-thePointer]

I recently found that the problem cannot solved by simply if (PyErr_Occured()) { throw py::error_already_set; }, I think the ultimate goal is:

1. to create a clear "exception boundary" between C(CPython), python and C++(pybind11)

if a C++ function is directly called by CPython, it must not throw anything. and if that function calls function which may throw, try-catch block should be used. This use case is especially common in defining type descriptor objects.

2. and can we ensure "exception neutrality" when both C++ exception and PyErr_* is propagating across modules.

for example if you have a call stack from C++(1) -> python -> C++(2), and the 'C++(2)' part throws invalid_argument, the python part will got ValueError raised, unhandled and propagate back to the 'C++(1)' part, then the 'C++(1)' part will receive py::error_alredy_set not invalid_argument, exception neutrality is not preserved

3. ensure there is no exception thrown in critical pathes

e.g. the initialization of Internals object
the function to generate better error message when throwing error_already_set

[bstaletic]

to create a clear "exception boundary" between C(CPython), python and C++(pybind11)

That should be fine. You're describing what already is happening in include/pybind11/detail/class.h:442.

and can we ensure "exception neutrality" when both C++ exception and PyErr_* is propagating across modules.

I understand what you would like to achieve here, but I'm not sure we can. At least not without breaking a ton of code, as the pybind docs say that any exception propagating from python will propagate as error_already_set. The following code is reasonable, considering the current documentation.

try {
    pyf(); // May reach into C++ through python
} catch( py::error_already_set ) { // No reason to catch anything else
}

ensure there is no exception thrown in critical pathes

That may be possible, but first we need to agree what is the critical path. Does import logic of extension modules count as critical? What about importing an embedded module? In both cases ImportError seems useful.

[YannickJadoul]

Thanks for the overview, @bstaletic! Great job, for reference :-)

I understand what you would like to achieve here, but I'm not sure we can. At least not without breaking a ton of code, as the pybind docs say that any exception propagating from python will propagate as error_already_set. The following code is reasonable, considering the current documentation.

I agree here. Also, there's a mapping defined from C++ exceptions into Python, but this is not a bijective one. So it's an additional difficulty to remember that a ValueError came from a invalid_argument C++ exception. That being said, there's also py::value_error, which is never translated into from a py::error_already_set?

At any rate, I would argue that this is a reasonably easy addition if needed in custom code: surround calls to Python with a catch statement for py::error_already_set and have a function somewhere that translates, and you're done?

[bstaletic]

At any rate, I would argue that this is a reasonably easy addition if needed in custom code: surround calls to Python with a catch statement for py::error_already_set and have a function somewhere that translates, and you're done?

Not quite. If you catch one thing, only to throw another, you're losing context of where that other exception came from. That original context is more important than the type.

[virtuald]

Not quite. If you catch one thing, only to throw another, you're losing context of where that other exception came from. That original context is more important than the type.

#2112 would allow users to chain exceptions. I'm using this like so in some C callbacks:

try {
  // some pybind code 
} catch (py::error_already_set &e) {
  e.caused(SomeExceptionType, "something");
  // do something with the error... pick your poison depending on your goals
  e.restore();
  PyErr_Print();
}

Yields a nice error/traceback with both messages in it.

[Skylion007]

Now that #2112 has merged in we should revisit this issue @rwgk @henryiii

[rwgk]

Now that #2112 has merged in we should revisit this issue @rwgk @henryiii

Agreed. I added the help wanted label.

[MichaelSuen-thePointer]

Seems you guys already got a good solution, I am closing the issue since I am not active on this project.