← 返回首页
bpo-45711: [asyncio] Normalize exceptions immediately after Fetch, before they are stored as StackItem, which should be normalized by iritkatriel · Pull Request #29890 · python/cpython · GitHub
Skip to content

Navigation Menu

Toggle navigation
Sign in
Appearance settings
Search or jump to...

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Include my email address so I can be contacted
Cancel Submit feedback

Saved searches

Use saved searches to filter your results more quickly

Cancel Create saved search
Appearance settings
Resetting focus

bpo-45711: [asyncio] Normalize exceptions immediately after Fetch, before they are stored as StackItem, which should be normalized#29890

Merged
iritkatriel merged 5 commits into
python:mainfrom
iritkatriel:45711-exc_info_does_not_need_normalizing
Dec 3, 2021
Merged

bpo-45711: [asyncio] Normalize exceptions immediately after Fetch, before they are stored as StackItem, which should be normalized#29890
iritkatriel merged 5 commits into
python:mainfrom
iritkatriel:45711-exc_info_does_not_need_normalizing

Conversation

iritkatriel commented Dec 2, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

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 requested a review from cjerdonek December 2, 2021 12:37
iritkatriel marked this pull request as draft December 2, 2021 13:12

iritkatriel commented Dec 2, 2021

Copy link
Copy Markdown
Member Author

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

iritkatriel commented Dec 2, 2021

Copy link
Copy Markdown
Member Author

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

cpython/Modules/_asynciomodule.c

Line 2705 in cb8f491

iritkatriel requested a review from asvetlov December 2, 2021 16:27
iritkatriel changed the title bpo-45711: Remove unnecessary normalization of exc_info bpo-45711: [asyncio] Normalize exceptions immediately after Fetch, before they are stored as StackItem, which should be normalized Dec 2, 2021
iritkatriel marked this pull request as ready for review December 2, 2021 17:27
iritkatriel requested a review from 1st1 as a code owner December 2, 2021 17:27
gvanrossum reviewed Dec 2, 2021
View reviewed changes
Comment thread Python/errors.c
if (tb2 != NULL) {
PyException_SetTraceback(val2, tb2);
}

gvanrossum Dec 2, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

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.

iritkatriel Dec 2, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

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.

iritkatriel Dec 2, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

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.

gvanrossum approved these changes Dec 3, 2021
View reviewed changes

gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

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

iritkatriel merged commit 2ff758b into python:main Dec 3, 2021
iritkatriel deleted the 45711-exc_info_does_not_need_normalizing branch December 8, 2021 20:09
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1st1 1st1 approved these changes
asvetlov asvetlov approved these changes
gvanrossum gvanrossum approved these changes
cjerdonek Awaiting requested review from cjerdonek

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Footer

© 2026 GitHub, Inc.