Skip to content

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

Merged
iritkatriel merged 32 commits intopython:mainfrom
kumaraditya303:issue45924
Jul 11, 2022
Merged

bpo-45924: Fix asyncio incorrect traceback when future's exception is raised multiple times#30274
iritkatriel merged 32 commits intopython:mainfrom
kumaraditya303:issue45924

Conversation

@kumaraditya303
Copy link
Contributor

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

@kumaraditya303 kumaraditya303 reopened this Jan 2, 2022
kumaraditya303
kumaraditya303 commented Jan 7, 2022
View reviewed changes
erlend-aasland
erlend-aasland reviewed Jan 7, 2022
View reviewed changes
@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
sobolevn
sobolevn reviewed Jan 19, 2022
View reviewed changes
asvetlov
asvetlov reviewed Feb 15, 2022
View reviewed changes
@asvetlov
Copy link
Contributor

asvetlov commented Feb 15, 2022

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

asvetlov commented Feb 15, 2022

New test is a little harder to read but much stablier

@kumaraditya303
Copy link
Contributor Author

kumaraditya303 commented Jul 6, 2022

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 reviewed Jul 6, 2022
View reviewed changes
iritkatriel
iritkatriel reviewed Jul 9, 2022
View reviewed changes
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.

kumaraditya303 and others added 3 commits July 11, 2022 11:10
@kumaraditya303 @iritkatriel
Update Lib/test/test_asyncio/test_futures2.py
bf757ce 
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
@kumaraditya303 @iritkatriel
Update Lib/test/test_asyncio/test_futures2.py
0eb9594 
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
@kumaraditya303
fix indent
c9a6fe7
@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

iritkatriel commented Jul 11, 2022

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

miss-islington commented Jul 11, 2022

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

bedevere-bot commented Jul 11, 2022

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

bedevere-bot commented Jul 11, 2022

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 <59607654+kumaraditya303@users.noreply.github.com>
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 <59607654+kumaraditya303@users.noreply.github.com>
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 <59607654+kumaraditya303@users.noreply.github.com>
@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 <59607654+kumaraditya303@users.noreply.github.com>

Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
@kumaraditya303 kumaraditya303 mentioned this pull request Jul 12, 2022
Closed
@ordinary-jamie ordinary-jamie mentioned this pull request Jan 13, 2024
Closed
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

topic-asyncio 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