Simplify error_already_set

[jagerman]

error_already_set is more complicated than it needs to be, partly because it manages reference counts itself rather than using py::object, and partly because it tries to do more exception clearing than is needed. This commit greatly simplifies it, and (hopefully) fixes #927.

I hope I'm not overlooking something, but I think the simplifications I'm making here make sense.

First, using py::object instead of PyObject * means we can rely on implicit copy/move constructors. (This necessitated a move into pytypes.h, but that doesn't seem a terrible fit given the purpose of error_already_set).

The current logic did both a PyErr_Clear on deletion and a PyErr_Fetch on creation. I can't see how the PyErr_Clear on deletion is ever useful: the Fetch on creation itself clears the error, so the only way doing a PyErr_Clear on deletion could do anything if is something else set the current python exception while the error_already_set object was alive--but in that case, clearing some potentially-unrelated other exception seems wrong. (Code that is worried about an exception handler raising another exception would already catch a second error_already_set from exception code).

The destructor itself called clear(), but clear() was a little bit convoluted: it called restore() to restore the currently captured error, but then immediately cleared it with PyErr_Clear();. The only point seems to be because PyErr_Restore decrements the reference counts on the held objects, but we can simply do that manually and avoid restoring the error just to clear it again. Or, actually, we can just not decrement them at all and let them be unreferenced (via the py::object destructor) when the error_already_set is itself destroyed.

clear() also had the side effect of clearing the current error, even if the current error_already_set didn't have a current error (e.g. because of a previous restore() or clear() call). I don't really see how clearing the error here is ever the right thing to do: since the error is cleared on construction, the only way there could be a current error is if you called restore() (in which case the current stored error-related members have already been released), or if some other code raised the error, in which case clear() on this object is clearing an error for which it shouldn't be responsible.

Neither of those seem like intentional or desirable features, and manually requesting deletion of the stored references similarly seems pointless, so I've just made clear() an empty method and marked it
deprecated in case any code out there is calling it.

[dean0x7d]

The extra work for exception clearing originates and is explained in 099d6e9. The first part about acquiring the GIL in ~error_already_set() is definitely important and should be added back to this PR. (Would be nice to add a regression test for that if it's not too elaborate.)

The same commit also makes mention of always using PyErr_Fetch and PyErr_Restore in pairs instead of simply decrementing the refcount. But I don't think this is necessary. Python's docs include examples where type, value, trace are simply decremented (e.g. here toward the end of the second code block). Avoiding the PyErr_Restore/PyErr_Clear combo definitely simplifies error_already_set, and it looks like that's fine with the CPython API.

So I'd say the py::object-base approach in this PR is pretty nice as long as the GIL destructor is added back. PyErr_Restore/PyErr_Clear is not necessary, unless I'm overlooking something.

[jagerman]

Ah, right, I suppose the error_already_set could be thrown from within a gil_scoped_acquire and caught outside it.

[jagerman]

@wjakob - do you recall what the motivation was to always match Fetch/Restore?

[wjakob]

IIRC the restore & clear on destruction was just paranoia regarding reference counting, I wasn't sure if decreasing the refcounts of the three objects representing the exception would be enough.

Another part that took a few iterations to work correctly is the propagation of stack traces through nested calls with C++ -> Python -> C++ -> .. transitions. As long as that still works with the proposed change, I'm fine with it.

[jagerman]

I've added some tests for the exception round-tripping, which seems to work as expected.

[dean0x7d]

I would very much like to recommend against the object.reset() function. It signals that it's expected that a py::object can be in a null state (in addition to None). It adds an additional level of uncertainty for users. If a function returns a py::object is it guaranteed to be in a valid state or must the user do an explicit check? So far, the answer has pretty much always been that it is a valid object and there's no convenient way to invalidate it on the user side (there is currently a way, just not a convenient one). Edit: scratch the last, part py::object::release() does make it convenient to invalidate an object, but it's probably better not to add any more ways to do it.

I think we should be working in the opposite direction and make sure that py::objects are never nullptr except after std::move.

[jagerman]

The point about not exposing it publically makes sense to me. In this particular case, though, I think I'd need either a o.release().dec_ref() or a o = object(); to clear the object before destruction. Any preference on either of those?

Another alternative is to make ~object() take out the gil_scoped_acquire before decrementing the reference. I'm not sure how much overhead this would add to the gil-already-head common case, though.

[dean0x7d]

o.release().dec_ref() seems good to me. Acquiring the GIL for every ~object() would add overhead even if the GIL is already held.

Edit: The existance of py::object::release() knocks down my previous point about "no convenient way to invalidate an object", but it's probably better not to add any more ways to do it.

[jagerman]

Acquiring the GIL for every ~object() would add overhead even if the GIL is already held.

More than you would think: I just tried it to see if it works and it segfaults: the gil_scoped_acquire constructor apparently triggers a py::object destruction, resulting in a segfault while attempting to obtain an infinite number of gil_scoped_acquires.