Skip to content

Conversation

@iritkatriel
Copy link
Member

@iritkatrieliritkatriel commented Dec 2, 2021

The handled exception exc_info (unlike the in-flight exception currexc) is always normalized. Except in asyncio, where unnormalised exceptions are saved in a StackItem and need to be normalized when they are used.

This PR brings asyncio in line with the rest of the code, by normalising exceptions and setting the traceback when they are Fetched.

https://bugs.python.org/issue45711

@iritkatriel
Copy link
MemberAuthor

This broke a couple of the asyncio tests, I'm checking why.

@iritkatriel
Copy link
MemberAuthor

This broke a couple of the asyncio tests, I'm checking why.

Right, because I removed the traceback update, which is not part of the normalising. The tests pass if I put it back.

But I think the right place to do this is when the exception is captured, here

@iritkatrieliritkatriel changed the title bpo-45711: Remove unnecessary normalization of exc_infobpo-45711: [asyncio] Normalize exceptions immediately after Fetch, before they are stored as StackItem, which should be normalizedDec 2, 2021
@iritkatrieliritkatriel marked this pull request as ready for review December 2, 2021 17:27
if (tb2 != NULL){
PyException_SetTraceback(val2, tb2);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this deletion supposed to be part of this PR? (The PR title didn't indicate a cleanup in errors.c.)

@1st1 should probably review this.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is intended, I thought the error.c change doesn’t need to be in news because it’s a private function.

Yes, I’ll wait to hear from @1st1.

Once we have just exc_value in StackItem, I think we should add something like a PyErr_FetchNormalized(&exc) for these cases. Then you use PyErr_Fetch only for Fetch-Restore.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it's a good point, I'll remove this from this PR so it's just asyncio and @1st1 won't need to worry about this part.

Copy link
Member

@gvanrossumgvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG from me, not sure how long to wait for @1st1.

1st1
1st1 approved these changes Dec 3, 2021
@iritkatrieliritkatriel merged commit 2ff758b into python:mainDec 3, 2021
@iritkatrieliritkatriel deleted the 45711-exc_info_does_not_need_normalizing branch December 8, 2021 20:09
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@iritkatriel@1st1@asvetlov@gvanrossum@the-knights-who-say-ni@bedevere-bot