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 phraseI 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.

The function_PyErr_GetTopmostException skips overNones, 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.

@iritkatriel

The function_PyErr_GetTopmostException skips overNones, 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:

importsystry:raiseTypeError()exceptTypeError:try:raiseValueError()except:assertisinstance(sys.exception(),ValueError)sys._set_exception(None)# !!!assertsys.exception()isNone# !!!assertisinstance(sys.exception(),TypeError)

This works (using the latest commit on this branch), yet itdoes look like it shouldn't. It seems likesys._set_exception(None) should settstate->exc_info->exc_value toPy_None and therefore should makePyErr_GetTopmostException skip over it and get theTypeError instead whenassert sys.exception() is None runs.

Unless I'm misunderstanding something?

@iritkatriel

The function_PyErr_GetTopmostException skips overNones, 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:

importsystry:raiseTypeError()exceptTypeError:try:raiseValueError()except:assertisinstance(sys.exception(),ValueError)sys._set_exception(None)# !!!assertsys.exception()isNone# !!!assertisinstance(sys.exception(),TypeError)

This works (using the latest commit on this branch), yet itdoes look like it shouldn't. It seems likesys._set_exception(None) should settstate->exc_info->exc_value toPy_None and therefore should makePyErr_GetTopmostException skip over it and get theTypeError instead whenassert sys.exception() is None runs.

Unless I'm misunderstanding something?

Try nesting it in yet another try-except?

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:

importsystry:raiseRuntimeError()except:try:raiseTypeError()except:try:raiseValueError()except:asserttype(sys.exception())==ValueErrorasserttype(sys.exception().__context__)==TypeErrorasserttype(sys.exception().__context__.__context__)==RuntimeErrorsys._set_exception(None)# !!!assertsys.exception()isNone# !!!try:raiseIndexError()except:asserttype(sys.exception())==IndexErrorassertsys.exception().__context__isNonetry:raiseSyntaxError()except:asserttype(sys.exception())==SyntaxErrorasserttype(sys.exception().__context__)==IndexErrorassertsys.exception().__context__.__context__isNoneasserttype(sys.exception())==IndexErrorassertsys.exception().__context__isNoneassertsys.exception()isNone# !!!asserttype(sys.exception())==TypeErrorasserttype(sys.exception().__context__)==RuntimeErrorassertisinstance(sys.exception(),RuntimeError)assertsys.exception()isNone

The piece I was missing was thatexc_info->previous_item is alwaysNULL except if there are generators/coroutines involved.

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

importsysdefgen():# exc_info: Noneyield# exc_info: NULL --> ZeroDivisionError('division by zero')asserttype(sys.exception())==ZeroDivisionErrorsys._set_exception(None)# exc_info: None --> ZeroDivisionError('division by zero')assertsys.exception()isNone# 💣💣💣yieldg=gen()next(g)try:1/0except:next(g)

This breaks because generators push a newexc_info onto thetstate->exc_info stack. BecausePyErr_GetTopmostException skips overNULL/Noneexc_values,sys.exception() still returns theZeroDivisionError. In this state,sys._set_exception(None) is essentially a no-op since the top-levelexc_info->exc_value already wasNULL/None. The assert then fails on the next line.

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

I've just pushed a commit that fixes this issue by making sure thatPyErr_SetHandledException always sets the exception that would've been returned byPyErr_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.

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.

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 forPyErr_SetHandledException being used within a coroutine to change the exception in the calling function instead of being limited toonly being able to affect change within the context of the coroutine.

As far as I can tell, the only place the interpreter usesPyErr_SetHandledException is to set the handled exception when processing anexcept* 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 beingNone and it overwriting the caller's exception. That being said, if some external code relies on being in a coroutine and having theexc_info stack in a state where the top exception isNone and uses this toadd an exception on top insteadsetting 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 --> ExceptionPyObject*value_err=make_some_ValueError();PyErr_SetHandledException(value_err);// PREVIOUS: ValueError --> Exception// NEW: None -> ValueError

Considering all the above, I'm reconsidering the change toPyErr_SetHandledException. Even though it seems unexpected that using it to set the handled exception toNULL/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 toPyErr_SetHandledException wasn't even a complete fix for theTo clear the exception state, pass NULL. documentation and the@contextmanager issue. Having it set the handled exception toNULL/None doesn't clear the exception, it removes the current topmost one. If there was another exception under there, it would be returned bysys.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 inNULL/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 havesys.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 betweenNULL andNone in the exception stack. If the exception isNULL (ie. unset) then it's skipped, but if it's set toNone then it's a valid non-exception and is returned as-is as the top-level exception. This allowsPyErr_SetHandledException(Py_None) to setNone on the top of the stack and mask all the exceptions under it. Then, when the coroutine exits, theNone 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.

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).

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?

Maybe a new one.

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

Anything else I need to do for this?

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?

I think the changes I made in the following commit of36b6177 solved that problem. Previously, aNone/NULL on the exception stack meant that_PyErr_GetTopmostException would skip it. With the new changes,_PyErr_GetTopmostException no longer skipsNones and returns them instead. This allowssys._set_exception(None) to add aNone 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 insertNULL in cases where there was no exception instead ofNone as they did previously.

I think the changes I made in the following commit of36b6177 solved that problem. Previously, aNone/NULL on the exception stack meant that_PyErr_GetTopmostException would skip it. With the new changes,_PyErr_GetTopmostException no longer skipsNones and returns them instead. This allowssys._set_exception(None) to add aNone 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 insertNULL in cases where there was no exception instead ofNone 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 theNULL/None ambiguity. Add an assertion in_PyErr_GetTopmostException that the exception is neverNone. The second PR can contain the rest.

The first PR would get rid of theNULL/None ambiguity. Add an assertion in_PyErr_GetTopmostException that the exception is neverNone. The second PR can contain the rest.

I've created#113302 that fixes theNULL/None ambiguity and adds theassert and rebased this PR on top of that branch. I also took the opportunity while I was rewriting history to fix the merge conflicts withmain and rewrite the commits here into something that's hopefully more understandable to reviewers. It now only includes 2 commits: one to addsys._set_exception() and thePy_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 withmain, and not being able to have it cleanly apply on#113302 was worse in this case.