Repository navigation
Suspected PyErr_Fetch() behavior change #102594
Description
Activity
Thanks @corona10 . A cc in a comment is enough to bring an issue to someone's attention (assigned means something different and could prevent others from taking a look).
Reacted by Donghee NaOoop sorry. I will care!
@rwgk If not with the python unit test as described above, a c program reproducing the issue would also be helpful.
I created two PRs:
- gh-102594: test_fetch_exception_with_broken_init @ 3.12.0a3 #102606 Working as expected.
- gh-102594: test_fetch_exception_with_broken_init @ main HEAD #102607 Behavior changed.
GHA for #102606 never finished, but local testing passes.
GHA result for the #102607:
====================================================================== FAIL: test_fetch_exception_with_broken_init (test.test_capi.test_exceptions.Test_ErrSetAndRestore.test_fetch_exception_with_broken_init) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/test_capi/test_exceptions.py", line 179, in test_fetch_exception_with_broken_init self.assertEqual(fetched_type_name, 'FlakyException') AssertionError: 'ValueError' != 'FlakyException' - ValueError + FlakyException ----------------------------------------------------------------------FWIW, I also tested locally with efc985a: Behavior also/already changed.
Thank you for the full repro. When I tried to just call
_testcapi.exc_set_object_fetch(FlakyException, ())from python I didn't see the change, because the ValueError gets normalised while it's being raised.The change in behaviour is in SetObject - the exception is now being normalised there, and stored in the interpreter in its normalised form, instead of being stored in the denormalised form and needing to be normalised after Fetch.
In the case of your exception, which raises a value error from its
__init__, normalising it after fetch would result in the ValueError, so there is no difference when you are in python, but if you just call PyErr_Fetch and look at the result you see a change in behaviour, because you can observe the unnormalised exception. Note that PyErr_Fetch is deprecated in 3.12 in favour of PyErr_GetRaisedException(), which always returns a normalised exception.I think we should also deprecate PyErr_SetObject, and direct users to use PyErr_SetRaisedException() instead (so they will have to create the exception instance beforehand, and there will be no room for confusion about which exception is being set as raised).
As for the change in behaviour reported here - I don't see a way to make this snippet behave as before, without reverting #101607. And I don't think we want to revert that for this edge case. I think we should document the change and deprecate PyErr_SetObject. There is a precedent for something similar in 3.11 - see the edge cases mentioned as changed due to bpo-45711 in the release notes.
CC @markshannon
CC @Yhg1s as release manager.The docs are vague.
I think the old behavior was broken and that it is now fixed.
- The docs use the term, "error indicator" singular, which would imply that an exception is created by
PyErr_SetObject()from thetype,valuearguments. - The new behavior is consistent with Python:
>>> class BorkedException(Exception): ... def __init__(self): ... raise ValueError("wot?") ... >>> raise BorkedException Traceback (most recent call last): File "<stdin>", line 1, in <module> File "<stdin>", line 3, in __init__ ValueError: wot?
Issue about the strange behavior of
PyErr_SetObject()#101578+1 to deprecating
PyErr_SetObject().- The docs use the term, "error indicator" singular, which would imply that an exception is created by
The new behavior masks errors in the error handling, which is ... terrible, in lack of a more diplomatic word.
A behavior change seems desirable, but if the behavior is changed, I think it's important to make a decision based on a careful and thorough assessment of the pros and cons.
When I was working on pybind/pybind11#1895 last year, I was struck by the apparent blissful ignorance in the PyErr_* APIs. Implicitly the APIs assume that the error handling code is bug free. That's of course not a reality, in particular in large-scale systems (I'm at Google). I know from first-hand experience that masked errors in the error handling can take weeks to troubleshoot, especially in a complex system with hundreds of thousands of dependencies (no exaggeration). (Actually, we never got to the bottom of one particular problem that was triggered by a change I made deep down in the core of our stack. I wound up spending a couple months revamping some core components around PyCLIF, and then sinking a couple weeks just into pybind/pybind11#1895, in preparation for making pybind11 a critical part of the Google infrastructure.)
This code is the result of extensive experiments and discussions (under pybind/pybind11#1895) to find a way to NOT mask errors in the error handling:
I believe
- the PyErr_* APIs need to be extended to clearly diagnose and report errors in the error handling, similar in spirit to what's done in that pybind11 code (reporting a
MISMATCH of original and normalized active exception typesor better, e.g. terminating the process might be safest, although the fallout will probably be somewhere between expensive and impractical to handle). - Until there is a plan to do that cleanly, it will be best to not let apparently unintentional behavior changes slip in, because that will most likely become an additional obstacle on a path to a clean solution.
>>> raise BorkedException Traceback (most recent call last): File "<stdin>", line 1, in <module> File "<stdin>", line 3, in __init__ ValueError: wot?That's not good, too, for the same reasons. It masks errors and can potentially add weeks of delays to projects like upgrading from Python 3.N to 3.N+1, or modernizing core components more generally.
- the PyErr_* APIs need to be extended to clearly diagnose and report errors in the error handling, similar in spirit to what's done in that pybind11 code (reporting a
The new APIs don’t need to deal with mismatch between normalised and non-normalised exceptions because they only have normalised exceptions now. We are in the process of deprecating the non-normalised form.
The new behavior masks errors in the error handling
I don't think it does. If instantiation of the
BorkedExceptionraises aValueError, then thatValueErroris what you get as the exception. Setting the exception toBorkedExceptionwould mask the error.What did you expect to happen in this case?
A behavior change seems desirable, but if the behavior is changed, I think it's important to make a decision based on a careful and thorough assessment of the pros and cons.
We did. There really don't seem to be any cons. The earlier normalization seems to surface latent bugs (such as the one in your
FlakyException).If you are handling flaky exception classes, why not create the exception first, then set the exception?
Here's the code to create, then set the exception:
PyObject *ex = PyObject_CallOneArg(exc_type, value); if (ex == NULL) { /* handle the error */ } else { PyErr_SetRaisedException(ex); }
4 remaining items
I don't consider process termination when exception normalization fails acceptable
Was anyone suggesting that?
The OP:
the PyErr_* APIs need to be extended to clearly diagnose and report errors in the error handling, similar in spirit to what's done in that pybind11 code (reporting a MISMATCH of original and normalized active exception types or better, e.g. terminating the process might be safest, although the fallout will probably be somewhere between expensive and impractical to handle).
I don't consider process termination when exception normalization fails acceptable
Was anyone suggesting that?
The OP:
the PyErr_* APIs need to be extended to clearly diagnose and report errors in the error handling, similar in spirit to what's done in that pybind11 code (reporting a MISMATCH of original and normalized active exception types or better, e.g. terminating the process might be safest, although the fallout will probably be somewhere between expensive and impractical to handle).
I like
ExceptionNormalizationFailedmuch better. (Terminating the process is only better than masking, IMO.)If you are handling flaky exception classes, why not create the exception first, then set the exception?
I think in the context of pybind11 that's not really an option, because that would push the responsibility to the user code.
The pattern is:
... user code does something (anything) ... throw pybind11::error_already_set();When
pybind11::error_already_setis constructed, the very first interaction with the Python C API isPyErr_Fetch()(here).I really like this suggestion and would be more than happy to tweak pybind11 accordingly.
If you're already tweaking pybind11, wouldn't you rather follow @markshannon 's suggestion and have full control over how you handle errors?
I see the advantage of ExceptionNormalizationFailed in that it can provide full information about what happened in programs that call PyErr_SetObject as is. But really we should deprecate construct+set in one function.
How about if instead of ExceptionNormalizationFailed, we raise the exception that came from
__init__with a PEP-678 note about the normalisation failure?A note sounds great because it is much simpler than the proposed new exception.
A note sounds great because it is much simpler than the proposed new exception.
Sounds good to me, too. (I didn't know about notes until just now TBH.)
As long as there is a discoverable trace of what happened, ideally with the suggestedreprof the original args, I think we have what we need to avoid extremely expensive troubleshooting.What if
reprof the args raises? It's quite common to have user classes with poorly behaving__repr__. Or evenreprof the intended class? (Harder but possible using a metaclass.) Or of the actual exception? We could just make the note contain a placeholder like "???" instead then.I’ll check what the traceback printing code does with such errors and do the same thing.
What if
reprof the args raises? It's quite common to have user classes with poorly behaving__repr__. Or evenreprof the intended class? (Harder but possible using a metaclass.) Or of the actual exception? We could just make the note contain a placeholder like "???" instead then.Sounds good. The approach I took for something similar/related in pybind11 is to generate messages like
MESSAGE UNAVAILABLE DUE TO ANOTHER EXCEPTION(here).I made an attempt to generate messages that can relatively easily be pin-pointed in potentially gigantic log files.
- added a commit that references this issue
on Mar 16, 2023 - added a commit that references this issue
on Mar 17, 2023
Bug report
While testing pybind11 with Python 3.12alpha6 I ran into what looks like a
PyErr_Fetch()behavior change. I also tried with the main branch @ c6858d1.I think the issue reduces to:
PyErr_SetObject()as e.g. here: https://github.com/pybind/pybind11/blob/442261da585536521ff459b1457b2904895f23b4/tests/test_exceptions.cpp#L309Followed more-or-less immediately by a
PyErr_Fetch()as e.g. here: https://github.com/pybind/pybind11/blob/442261da585536521ff459b1457b2904895f23b4/include/pybind11/pytypes.h#L482The special twist is that the exception type involved raises a
ValueErrorin its__init__:For easy reference the same code copy-pasted here:
Up to and including 3.12alpha3:
PyErr_Fetch()produces theFlakyExceptiontype (this here).With 3.12alpha6:
PyErr_Fetch()produces theValueErrortype instead.Additional detail:
Up to and including 3.12alpha3: only
PyErr_NormalizeException()hits theValueErrorinFlakyException._init__, but notPyErr_Fetch().Your environment
Linked PRs