GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — 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

Conversation

@iritkatriel

@iritkatriel iritkatriel commented Dec 2, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@iritkatriel
bpo-45711: Remove unnecessary normalization of exc_info
847fcdb
@iritkatriel
iritkatriel requested a review from cjerdonek December 2, 2021 12:37
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Dec 2, 2021
@bedevere-bot bedevere-bot added the awaiting core review label Dec 2, 2021
@iritkatriel iritkatriel added the skip news label Dec 2, 2021
@iritkatriel
iritkatriel requested a review from gvanrossum December 2, 2021 12:38
@iritkatriel
iritkatriel marked this pull request as draft December 2, 2021 13:12
@iritkatriel

iritkatriel commented Dec 2, 2021

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iritkatriel
put the traceback update back in _PyErr_ChainStackItem
acc65e6
@iritkatriel

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iritkatriel
iritkatriel requested a review from asvetlov December 2, 2021 16:27
@iritkatriel
normalize exceptions after Fetch when they are about to be stored as …
a995431 
…a StackItem (exc_info)
@iritkatriel iritkatriel removed the skip news label Dec 2, 2021
@iritkatriel 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
@blurb-it
📜🤖 Added by blurb_it.
6ce3848
@iritkatriel
iritkatriel marked this pull request as ready for review December 2, 2021 17:27
@iritkatriel
iritkatriel requested a review from 1st1 as a code owner December 2, 2021 17:27
asvetlov
asvetlov approved these changes Dec 2, 2021
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Dec 2, 2021
gvanrossum
gvanrossum reviewed Dec 2, 2021
View reviewed changes
Comment thread Python/errors.c
if (tb2 != NULL) {
PyException_SetTraceback(val2, tb2);
}

@gvanrossum gvanrossum Dec 2, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iritkatriel iritkatriel Dec 2, 2021

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iritkatriel iritkatriel Dec 2, 2021

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@iritkatriel
Revert "bpo-45711: Remove unnecessary normalization of exc_info"
e1a719a 
This reverts commit 847fcdb.
gvanrossum
gvanrossum approved these changes Dec 3, 2021
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
1st1
1st1 approved these changes Dec 3, 2021
View reviewed changes
@iritkatriel
iritkatriel merged commit 2ff758b into python:main Dec 3, 2021
@bedevere-bot bedevere-bot removed the awaiting merge label Dec 3, 2021
@iritkatriel
iritkatriel deleted the 45711-exc_info_does_not_need_normalizing branch December 8, 2021 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@iritkatriel @1st1 @asvetlov @gvanrossum @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.