Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bpo-45924: Fix asyncio incorrect traceback when future's exception is raised multiple times #30274

Merged
merged 32 commits into from
Jul 11, 2022
Merged

bpo-45924: Fix asyncio incorrect traceback when future's exception is raised multiple times #30274

merged 32 commits into from
Jul 11, 2022

Conversation

kumaraditya303
Copy link
Contributor

@kumaraditya303 kumaraditya303 commented Dec 27, 2021 •
edited by iritkatriel
Loading

@kumaraditya303 kumaraditya303 reopened this Jan 2, 2022
@erlend-aasland
Copy link
Contributor

erlend-aasland commented Jan 7, 2022 •
edited
Loading

BTW, GitHub does not play well with force pushes (contrary to GitLab). If you use git pull --no-ff main instead of git rebase main, changes will play well with the GitHub Pull Request ui, and the review process will be a lot easier. If you force push, review comments/discussions may end up in limbo.

Note: if there has been no comments on a PR, there is of course no harm in force-pushing.

@kumaraditya303 kumaraditya303 marked this pull request as ready for review January 10, 2022 10:04
@kumaraditya303 kumaraditya303 force-pushed the issue45924 branch 2 times, most recently from eeb4c56 to adb12a7 Compare January 19, 2022 12:20
@asvetlov
Copy link
Contributor

Fixed.
The test is very weak though:

  1. Adding/removing source lines before the test breaks it.
  2. Tested tracebacks are the same. I prefer that the test reflects new traceback substitution.
  3. Textual representation of stack trace was changed recently, new improvements can break the test again.
    Perhaps tracebacks should be analyzed, no printed exceptions.

@asvetlov
Copy link
Contributor

New test is a little harder to read but much stablier

@kumaraditya303 kumaraditya303 reopened this Jul 6, 2022
@kumaraditya303
Copy link
Contributor Author

Re-running that job didn't help. I don't know what it does. github-token not supplied -- what's that?

It was a recent github action which was broken, should be fixed now as I merged with main.

iritkatriel
iritkatriel approved these changes Jul 10, 2022
View reviewed changes
Copy link
Member

@iritkatriel iritkatriel left a comment

Choose a reason for hiding this comment

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

I'd put the subclasses right after the base class, like so.

Lib/test/test_asyncio/test_futures2.py Show resolved Hide resolved
Lib/test/test_asyncio/test_futures2.py Outdated Show resolved Hide resolved
@kumaraditya303
Copy link
Contributor Author

kumaraditya303 commented Jul 11, 2022 •
edited
Loading

Thanks for the reviews! I have addressed all the review comments and now I would like to see this merged and make anymore improvement if required in separate PRs.

Perfect is the enemy of good!

@iritkatriel
Copy link
Member

Thanks for the reviews! I have addressed all the review comments and [...]

I checked again and confirmed that both versions are tested now. I think this can be merged now.

Perfect is the enemy of good!

This is true with respect to many things (like homework - it doesn't really matter whether you graduate with a GPA of 85% or 90%). But when it comes to complex software you get "good" by aiming for "perfect".

@iritkatriel iritkatriel merged commit 86c1df1 into python:main Jul 11, 2022
@miss-islington
Copy link
Contributor

Thanks @kumaraditya303 for the PR, and @iritkatriel for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-94747 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed needs backport to 3.11 only security fixes needs backport to 3.10 only security fixes labels Jul 11, 2022
@bedevere-bot
Copy link

GH-94748 is a backport of this pull request to the 3.10 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 11, 2022
@kumaraditya303 @miss-islington
bpo-45924: Fix asyncio incorrect traceback when future's exception is…
cdad89b 
… raised multiple times (pythonGH-30274)

(cherry picked from commit 86c1df1)

Co-authored-by: Kumar Aditya <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 11, 2022
@kumaraditya303 @miss-islington
bpo-45924: Fix asyncio incorrect traceback when future's exception is…
b1d4f5e 
… raised multiple times (pythonGH-30274)

(cherry picked from commit 86c1df1)

Co-authored-by: Kumar Aditya <[email protected]>
pablogsal pushed a commit that referenced this pull request Jul 11, 2022
@miss-islington @kumaraditya303
[3.11] bpo-45924: Fix asyncio incorrect traceback when future's excep…
91176d3 
…tion is raised multiple times (GH-30274) (#94747)

Co-authored-by: Kumar Aditya <[email protected]>
@kumaraditya303 kumaraditya303 deleted the issue45924 branch July 11, 2022 13:50
pablogsal pushed a commit that referenced this pull request Jul 11, 2022
@miss-islington @kumaraditya303
bpo-45924: Fix asyncio incorrect traceback when future's exception is…
aa2142d 
… raised multiple times (GH-30274) (#94748)

(cherry picked from commit 86c1df1)

Co-authored-by: Kumar Aditya <[email protected]>

Co-authored-by: Kumar Aditya <[email protected]>
@kumaraditya303 kumaraditya303 mentioned this pull request Jul 12, 2022
Closed
@ordinary-jamie ordinary-jamie mentioned this pull request Jan 13, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asvetlov asvetlov asvetlov left review comments

@gvanrossum gvanrossum gvanrossum approved these changes

@sobolevn sobolevn sobolevn left review comments

@erlend-aasland erlend-aasland erlend-aasland left review comments

@iritkatriel iritkatriel iritkatriel approved these changes

@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner

Assignees

@gvanrossum gvanrossum

Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@kumaraditya303 @erlend-aasland @asvetlov @iritkatriel @gvanrossum @vstinner @bedevere-bot @miss-islington @sobolevn @the-knights-who-say-ni