gh-111375: Fix handling of exceptions within @contextmanager-decorated functions

[pR0Ps]

Previously, when using a try...yield...except construct within a @contextmanager function, an exception raised by the yield wouldn't be properly cleared when handled by the except block. Instead, calling sys.exception() would continue to return the exception, leading to confusing tracebacks and logs.

This was happening due to the way that the @contextmanager decorator drives its decorated generator function. When an exception occurs, it uses .throw(exc) to throw the exception into the generator. However, even if the generator handles this exception, because the exception was thrown into it in the context of the @contextmanager decorator handling it (and not being finished yet), sys.exception() was not being reset.

For an example of this and more information, see #111375

In order to fix this, the exception context as the @contextmanager decorator is __enter__'d is stored and set as the current exception context just before throwing the new exception into the generator. Doing this means that after the generator handles the thrown exception, sys.exception() reverts back to what it was when the @contextmanager function was started.

Potential reviewers based on activity in the linked issue: @iritkatriel @ericsnowcurrently @ncoghlan

• Issue: Handling exceptions within a @contextmanager function doesn't clear sys.exception() #111375

[ericsnowcurrently]

Thanks for working on this. Mostly your solution fundamentally seems correct and specifically a good approach. I just have concerns about 2 points:

• relying on ctypes
• deleting self.exc_context instead of setting it back to None

[bedevere-app]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[iritkatriel]

The function _PyErr_GetTopmostException skips over Nones, and returns the first proper exception in the chain. I don't know what could get confused if we start setting the exception to None.

[pR0Ps]

@iritkatriel

The function _PyErr_GetTopmostException skips over Nones, and returns the first proper exception in the chain. I don't know what could get confused if we start setting the exception to None.

Hmm, yeah, now that you mention it, I don't know why this is currently working. Consider:

import sys
try:
    raise TypeError()
except TypeError:
    try:
        raise ValueError()
    except:
        assert isinstance(sys.exception(), ValueError)
        sys._set_exception(None)  # !!!
        assert sys.exception() is None  # !!!
    assert isinstance(sys.exception(), TypeError)

This works (using the latest commit on this branch), yet it does look like it shouldn't. It seems like sys._set_exception(None) should set tstate->exc_info->exc_value to Py_None and therefore should make PyErr_GetTopmostException skip over it and get the TypeError instead when assert sys.exception() is None runs.

Unless I'm misunderstanding something?

[iritkatriel]

@iritkatriel

The function _PyErr_GetTopmostException skips over Nones, and returns the first proper exception in the chain. I don't know what could get confused if we start setting the exception to None.

Hmm, yeah, now that you mention it, I don't know why this is currently working. Consider:

import sys
try:
    raise TypeError()
except TypeError:
    try:
        raise ValueError()
    except:
        assert isinstance(sys.exception(), ValueError)
        sys._set_exception(None)  # !!!
        assert sys.exception() is None  # !!!
    assert isinstance(sys.exception(), TypeError)

This works (using the latest commit on this branch), yet it does look like it shouldn't. It seems like sys._set_exception(None) should set tstate->exc_info->exc_value to Py_None and therefore should make PyErr_GetTopmostException skip over it and get the TypeError instead when assert sys.exception() is None runs.

Unless I'm misunderstanding something?

Try nesting it in yet another try-except?

[pR0Ps]

Try nesting it in yet another try-except?

I have not been able to break it, no matter how much nesting above or below is used. This also runs without asserting:

import sys
try:
    raise RuntimeError()
except:
    try:
        raise TypeError()
    except:
        try:
            raise ValueError()
        except:
            assert type(sys.exception()) == ValueError
            assert type(sys.exception().__context__) == TypeError
            assert type(sys.exception().__context__.__context__) == RuntimeError
            sys._set_exception(None)  # !!!
            assert sys.exception() is None  # !!!
            try:
                raise IndexError()
            except:
                assert type(sys.exception()) == IndexError
                assert sys.exception().__context__ is None
                try:
                    raise SyntaxError()
                except:
                    assert type(sys.exception()) == SyntaxError
                    assert type(sys.exception().__context__) == IndexError
                    assert sys.exception().__context__.__context__ is None
                assert type(sys.exception()) == IndexError
                assert sys.exception().__context__ is None
            assert sys.exception() is None  # !!!
        assert type(sys.exception()) == TypeError
        assert type(sys.exception().__context__) == RuntimeError
    assert isinstance(sys.exception(), RuntimeError)
assert sys.exception() is None

[pR0Ps]

The piece I was missing was that exc_info->previous_item is always NULL except if there are generators/coroutines involved.

The following shows the issue @iritkatriel raised (I've added the state of the exc_info stack as comments):

import sys

def gen():
    # exc_info: None
    yield
    # exc_info: NULL --> ZeroDivisionError('division by zero')
    assert type(sys.exception()) == ZeroDivisionError
    sys._set_exception(None)
    # exc_info: None --> ZeroDivisionError('division by zero')
    assert sys.exception() is None  # 💣💣💣
    yield

g = gen()
next(g)
try:
    1/0
except:
    next(g)

This breaks because generators push a new exc_info onto the tstate->exc_info stack. Because PyErr_GetTopmostException skips over NULL/None exc_values, sys.exception() still returns the ZeroDivisionError. In this state, sys._set_exception(None) is essentially a no-op since the top-level exc_info->exc_value already was NULL/None. The assert then fails on the next line.

This maybe brings up another "is it a bug or just a documentation issue" question (expand me)

In Doc/c-api/exceptions.rst PyErr_SetHandledException says the following:

To clear the exception state, pass NULL.

and

it can be used when code needs to save and restore the exception state temporarily. Use PyErr_GetHandledException to get the exception state.

This doesn't convey the nuance here. Passing NULL to clear the exception state only works if the current exception isn't "hidden" under a NULL/None already. Also, an uninformed implementation of saving and restoring exception state using PyErr_GetHandledException/PyErr_SetHandledException will also have issues if the top-level exception is NULL/None since it will effectively duplicate the exception. For example:

// exc_info state: None -> Exception
PyObject *obj = PyErr_GetHandledException();
// obj: Exception
/* <snip some code> */
PyErr_SetHandledException(obj)
// exc_info state: Exception -> Exception

Maybe the documentation should just be expanded to cover this edge case, but it seems to me that it's actually a bug that PyErr_SetHandledException doesn't set the same exception that PyErr_GetHandledException returns.

I've just pushed a commit that fixes this issue by making sure that PyErr_SetHandledException always sets the exception that would've been returned by PyErr_GetHandledException, which ensures that the currently-handled exception is the one that it sets. I understand that this may not be something that can be changed due to backwards compatibility concerns but thought I'd propose it anyway and see what you all think.

Open to feedback/suggestions. Also, since I think I've addressed everything at this point: I have made the requested changes; please review again.

[iritkatriel]

Thanks for the analysis! Yes, I think you found the issue.

but it seems to me that it's actually a bug that PyErr_SetHandledException doesn't set the same exception that PyErr_GetHandledException returns.

I agree, and wouldn't mind seeing this fixed. Would it break anything? If you change the exception stack from [exc, None, None] to [exc, None, exc] - it look like it's semantically the same.

[pR0Ps]

Would it break anything? If you change the exception stack from [exc, None, None] to [exc, None, exc] - it look like it's semantically the same.

I've messed around with this some more and I think you're right in that it's exactly the same from the perspective of everything just using _PyErr_GetTopmostException so it doesn't matter.

It doesn't seem to break anything (nothing in the test suite at least). The danger here is that it now makes it possible for PyErr_SetHandledException being used within a coroutine to change the exception in the calling function instead of being limited to only being able to affect change within the context of the coroutine.

As far as I can tell, the only place the interpreter uses PyErr_SetHandledException is to set the handled exception when processing an except* block so the context within the block only has the matched exceptions. This only happens within the context of an exception being handled so there's no danger of the top of the stack being None and it overwriting the caller's exception. That being said, if some external code relies on being in a coroutine and having the exc_info stack in a state where the top exception is None and uses this to add an exception on top instead setting an exception, the change I made will cause issues. It will cause the function to replace the exception of the calling function instead of adding the exception to the top of the stack. Ex:

// <in a coroutine with the caller handling an Exception>
// exc_info state: None --> Exception
PyObject *value_err = make_some_ValueError();
PyErr_SetHandledException(value_err);
// PREVIOUS: ValueError --> Exception
// NEW: None -> ValueError

Considering all the above, I'm reconsidering the change to PyErr_SetHandledException. Even though it seems unexpected that using it to set the handled exception to NULL/None doesn't do anything in a coroutine, I think it's much more unexpected that it can reach up out of its current context and affect the caller's exception.

While looking into this, I also realized that the change to PyErr_SetHandledException wasn't even a complete fix for the To clear the exception state, pass NULL. documentation and the @contextmanager issue. Having it set the handled exception to NULL/None doesn't clear the exception, it removes the current topmost one. If there was another exception under there, it would be returned by sys.exception() anyway. I've added a test case in the latest commit that tests this situation.

Sorry for all the back and forth on this - I'm discovering how all this works as I go.

---

Assuming that the exception stack having duplicates in it is fine, the remaining issue (documentation and otherwise) is that passing in NULL/None does not actually clear the exception state - it only removes the current top. If called within a coroutine, this is either a no-op or potentially exposes the caller exception state instead.

Since it's desired behavior to have sys.exception() return the exception of the caller within a coroutine that hasn't raised an exception, _PyErr_GetTopmostException's traversing up the stack of exceptions can't be removed. In order to be able to fully clear the exception state, this means that there has to be some way of signaling to _PyErr_GetTopmostException that it shouldn't traverse up the exception stack in the specific case where the exception has been explicitly cleared.

One way to do this that looks like it might work is to use the difference between NULL and None in the exception stack. If the exception is NULL (ie. unset) then it's skipped, but if it's set to None then it's a valid non-exception and is returned as-is as the top-level exception. This allows PyErr_SetHandledException(Py_None) to set None on the top of the stack and mask all the exceptions under it. Then, when the coroutine exits, the None is popped off the exception stack as normal, exposing the other exceptions under it.

I've implemented the above in my latest commit and it seems to work (all test cases pass). However, I'm definitely a bit out of my depth here so I'm not sure if this is something that can/should be changed or if I've overlooked an edge case with it.

[iritkatriel]

I think it's much more unexpected that it can reach up out of its current context and affect the caller's exception.

Good point.

This is all useful analysis. Can you start by making some doc enhancements with this point you feel are missing? (If we end up making code changes we can change the docs as well, but it's useful to have the current state documented in any case).

[pR0Ps]

Can you start by making some doc enhancements with this point you feel are missing?

Sure, should I do them in this PR or submit a new one?

[iritkatriel]

Maybe a new one.

[pR0Ps]

In parallel to that, I think I've resolved all the issues that were brought up here so @bedevere-bot : I have made the requested changes; please review again.

EDIT: ok, no idea how to get the label to change back to awaiting review so I've just manually requested a re-revew. Sorry for the spam

[pR0Ps]

Anything else I need to do for this?

[iritkatriel]

As far as I recall, the fix described in #111676 (comment) needed to be reverted so we still have a problem with the None/NULL exc_info. No?

[pR0Ps]

I think the changes I made in the following commit of 36b6177 solved that problem. Previously, a None/NULL on the exception stack meant that _PyErr_GetTopmostException would skip it. With the new changes, _PyErr_GetTopmostException no longer skips Nones and returns them instead. This allows sys._set_exception(None) to add a None to the exception stack, effectively clearing it.

To support this change, I modified the sections of code that add exceptions to the exception stack so that they always insert NULL in cases where there was no exception instead of None as they did previously.

[iritkatriel]

I think the changes I made in the following commit of 36b6177 solved that problem. Previously, a None/NULL on the exception stack meant that _PyErr_GetTopmostException would skip it. With the new changes, _PyErr_GetTopmostException no longer skips Nones and returns them instead. This allows sys._set_exception(None) to add a None to the exception stack, effectively clearing it.

To support this change, I modified the sections of code that add exceptions to the exception stack so that they always insert NULL in cases where there was no exception instead of None as they did previously.

This makes sense, but since it's a deep subtle change I think we should split it into two PRs. That would make it easier in the future to see what happened. Both PRs can be attached to this issue.

The first PR would get rid of the NULL/None ambiguity. Add an assertion in _PyErr_GetTopmostException that the exception is never None. The second PR can contain the rest.

[pR0Ps]

The first PR would get rid of the NULL/None ambiguity. Add an assertion in _PyErr_GetTopmostException that the exception is never None. The second PR can contain the rest.

I've created #113302 that fixes the NULL/None ambiguity and adds the assert and rebased this PR on top of that branch. I also took the opportunity while I was rewriting history to fix the merge conflicts with main and rewrite the commits here into something that's hopefully more understandable to reviewers. It now only includes 2 commits: one to add sys._set_exception() and the Py_None exception masking, and one to use that functionality to fix the @contextmanager behavior.

Apologies for this, I know rebasing PRs during development is generally frowned upon, but figured the alternative of having the same functionality implemented in different ways across multiple branches, having merge conflicts with main, and not being able to have it cleanly apply on #113302 was worse in this case.

[github-actions]

This PR is stale because it has been open for 30 days with no activity.

[pR0Ps]

I have implemented the change where the exception context is passed in via generator.throw(exc, exc_context=context). I'm aware that more work needs to be done for that change to be merged (presumably docs, news, more tests), but wanted to be sure that this is the way we want to proceed before doing that work.

Because it's been a while and things have changed a bunch in Objects/genobject.c, I rebased this branch on the latest main. I also reorganized the commits a bit to hopefully make it easier to review, as well as make it easier to pivot to another implementation method. Basically the commits are ordered as follows:

• Add sys._set_exception()
• Move that function to _contextlib._setup_genmgr_ctx
• Revert all changes that expose the functionality to directly set the exception state
• Add a single commit that adds a exc_context kwarg to generator.throw() and uses it to implement the fix.