Provide a C helper function to chain raised (but not yet caught) exceptions

[ncoghlan]

BPO	23188
Nosy	@ncoghlan, @pitrou, @vstinner, @cjerdonek, @ericsnowcurrently, @serhiy-storchaka
PRs	gh-67377: Document that PyErr_SetString, etc. chain exceptions #20329

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2015-01-08.03:17:20.994>
labels = ['type-feature', '3.8']
title = 'Provide a C helper function to chain raised (but not yet caught) exceptions'
updated_at = <Date 2020-05-23.04:08:02.797>
user = 'https://github.com/ncoghlan'

bugs.python.org fields:

activity = <Date 2020-05-23.04:08:02.797>
actor = 'chris.jerdonek'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = []
creation = <Date 2015-01-08.03:17:20.994>
creator = 'ncoghlan'
dependencies = []
files = []
hgrepos = []
issue_num = 23188
keywords = ['patch']
message_count = 17.0
messages = ['233619', '233625', '233626', '233628', '233657', '233733', '233789', '233817', '233818', '278880', '278967', '317307', '317314', '317335', '317402', '369657', '369691']
nosy_count = 6.0
nosy_names = ['ncoghlan', 'pitrou', 'vstinner', 'chris.jerdonek', 'eric.snow', 'serhiy.storchaka']
pr_nums = ['20329']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue23188'
versions = ['Python 3.8']

[ncoghlan]

There's currently an internal helper API for chaining exceptions from C code, _PyErr_ChainExceptions.

It would be good to have a public API that offered equivalent functionality for use in third party C extensions.

[ncoghlan]

After looking into this further, PyErr_SetObject (and other APIs like PyErr_SetString which call that internally) aim to handle the chaining automatically, but they don't handle exceptions which haven't been normalized yet.

PyErr_SetObject should probably normalise the exception at the start of the call f ithe exception type is set on the thread state, but not the exception value.

[ncoghlan]

The documentation of PyErr_SetObject, PyErr_SetString et al should also be updated to mention exception chaining.

[ncoghlan]

Thinking about it a bit further, I suspect implicit normalisation of chained exceptions could cause problems when we only want to set __cause__ without setting __context__ (e.g. codec exception chaining).

I'm going to ponder this one for a while, but happy to hear anyone else's feedback.

At the very least, I think the C API docs could use a bit more clarity on the intricacies of C level exception chaining.

[serhiy-storchaka]

_PyErr_ChainExceptions() was added because exceptions raised in C code are not implicitly chained. The code for explicit chaining is error prone, so it was extracted in separate function. Even with _PyErr_ChainExceptions() chaining exceptions look complex. May be implicit chaining all exceptions would be better.

[ncoghlan]

The interesting discovery I made while reviewing the patch for bpo-22906 is that there apparently *is* implicit chaining support in PyErr_SetObject: https://hg.python.org/cpython/file/default/Python/errors.c#l70

Chris indicates that it doesn't seem to be triggering for his patch, though (even after normalising and then restoring the exception state), and I haven't fully dug into why yet. Preliminary explorations show that the last two functional modifications were a fix for a crash bug in bpo-3611, and Antoine's original implementation of exception chaining in bpo-3108.

I've added Antoine to the nosy list, as my main takeaway at the moment is that I *don't* currently understand what's going on with the exception chaining, and I'd like to before we merge the PEP-479 patch.

[pitrou]

The interesting discovery I made while reviewing the patch for issue
22906 is that there apparently *is* implicit chaining support in
PyErr_SetObject

Indeed, there is, and it should work properly (AFAIR there was quite a bit of debugging to make this work). Also, note that normalizing is already handled.

I'm not sure what you're witnessing that doesn't work as expected. I suspect that you may be confusing "an exception has been caught" (and is ready to be chained from) with "an exception has been raised" (i.e. PyErr_Occurred() is true).

[ncoghlan]

Ah, confusion between "exception has been raised" and "we're in an active exception handler" is almost certainly what is happening. In both bpo-22906 and my previous codec exception wrapping work, we're still in the C code that raised the exception, *before* we make it back to the eval loop and any Python level exception handling.

So I think that's the distinction we need to ensure is documented, and potentially a new public helper function provided - if you're in a Python level exception handler, then exception context chaining will be handled automatically for you in PyErr_SetObject, but if you're still in C code and haven't made it back to the eval loop after raising an exception, then you're going to have to do any exception chaining explicitly.

[ncoghlan]

Updated the issue title and type again based on Antoine's explanation.

[ncoghlan]

Via bpo-28410, Serhiy is adding a private "_PyErr_FormatFromCause" helper API to make explicit C level exception chaining easier within the implementation of CPython (this helps resolve bpo-28214, an exception reporting problem in the new PEP-487 __set_name__ feature).

It may be sufficient to make that API public, and use that as the home of documentation for some of the subtleties discussed here.

[serhiy-storchaka]

This helper is convenient in many cases, but it is very limited. It raises an exception with single string argument. It doesn't work in cases when the exception doesn't take arguments (PyErr_SetNone) or takes multiple or non-string arguments (PyErr_SetFromErrnoWithFilenameObject, PyErr_SetImportError). I think for public API we need more general solution. Something like this:

    PyObject *cause = get_current_exception();
    PyErr_SetImportError(msg, name, path);
    set_cause_of_current_exception(cause);

[ericsnowcurrently]

FTR, see PEP-490 ("Chain exceptions at C level") which proposed implicitly chaining exceptions in the PyErr_* API.

While that PEP was rejected (not all exceptions should be chained), it does make a good point about the clunkiness of using _PyErr_ChainExceptions():

    PyObject *exc, *val, *tb;
    PyErr_Fetch(&exc, &val, &tb);
    PyErr_Format(ZipImportError, "can't open Zip file: %R", archive);
    _PyErr_ChainExceptions(exc, val, tb);

So if we are going to add a public helper function, let's consider adding one that simplifies usage. For instance, how about one that indicates the next exception set should chain:

    PyErr_ChainNext();
    PyErr_Format(ZipImportError, "can't open Zip file: %R", archive);

Or perhaps we should revive PEP-490 with an opt out mechanism for the cases where we don't want chaining:

    PyErr_NoChainNext();
    PyErr_Format(PyExc_RuntimeError, "uh-oh");

[serhiy-storchaka]

There is usually more complex code between PyErr_Fetch() and _PyErr_ChainExceptions():

        PyObject *exc, *val, *tb, *close_result;
        PyErr_Fetch(&exc, &val, &tb);
        close_result = _PyObject_CallMethodId(result, &PyId_close, NULL);
        _PyErr_ChainExceptions(exc, val, tb);
        Py_XDECREF(close_result);

Many exceptions can be raised and silenced or overridden between, but we are interesting in chaining the only latest exception (or restoring the original exception if all exceptions between were silenced).

[ericsnowcurrently]

good point :)

[ncoghlan]

Also see

cpython/Objects/exceptions.c

Line 2713 in 55edd0c

_PyErr_TrySetFromCause(const char *format, ...)

for the version we use in a few places to implicitly update the exception message, while keeping the exception type and state the same (and giving up and letting the exception through unchained if we can't work out how to do that in a reliable way).

[cjerdonek]

I just want to point out one difference between _PyErr_ChainExceptions and PyErr_SetObject that I encountered while working on this issue:
https://bugs.python.org/issue40696

While both functions set the context, only PyErr_SetObject does a check to prevent cycles from forming in the context chain (albeit an incomplete check, which can lead to hangs, which I mention in the issue linked above).

[cjerdonek]

The documentation of PyErr_SetObject, PyErr_SetString et al should also be updated to mention exception chaining.

I just posted a PR to do the above:
#20329