Skip to content

bpo-33672: Fix Task.__repr__ crash with Cython's bogus coroutines#7161

Merged
1st1 merged 2 commits intopython:masterfrom
1st1:fixcy
May 28, 2018
Merged

bpo-33672: Fix Task.__repr__ crash with Cython's bogus coroutines#7161
1st1 merged 2 commits intopython:masterfrom
1st1:fixcy

Conversation

@1st1
Copy link
Copy Markdown
Member

@1st1 1st1 commented May 28, 2018 •
edited by bedevere-bot
Loading

@1st1 1st1 requested a review from asvetlov as a code owner May 28, 2018 16:30
scoder
scoder reviewed May 28, 2018
View reviewed changes
Lib/asyncio/coroutines.py Outdated
Copy link
Copy Markdown
Contributor

@scoder scoder May 28, 2018

Choose a reason for hiding this comment

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

Do you have any insights into the conditions under which this can be observed? Is this a problem with older versions of Cython, or is there anything we can do to improve the situation?

Copy link
Copy Markdown
Member Author

@1st1 1st1 May 28, 2018

Choose a reason for hiding this comment

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

Not sure about __qualname__ and __name__, I got reports from uvloop users about this a couple of months ago. __code__ set to None should be easily reproducible with latest Cython -- ideally, __code__ should either be set to a code-like object or not set at all.

asvetlov
asvetlov reviewed May 28, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

I have some minor questions and ideas for improvement.
Would be nice to address them before merging.

Lib/asyncio/coroutines.py Outdated
Copy link
Copy Markdown
Contributor

@asvetlov asvetlov May 28, 2018

Choose a reason for hiding this comment

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

I don't follow why not push all this logic into format_helpers._format_callback?

Copy link
Copy Markdown
Member Author

@1st1 1st1 May 28, 2018

Choose a reason for hiding this comment

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

format_callback expects a Python function. This works on a coroutine object. This whole function should be moved to format_helpers.py though, I just don't want to do this in 3.7

Copy link
Copy Markdown
Contributor

@asvetlov asvetlov May 28, 2018

Choose a reason for hiding this comment

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

Ok, got it.

Lib/asyncio/coroutines.py Outdated
Copy link
Copy Markdown
Contributor

@asvetlov asvetlov May 28, 2018

Choose a reason for hiding this comment

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

No need a nested function here, top-level private _is_running is enough.

Copy link
Copy Markdown
Member Author

@1st1 1st1 May 28, 2018

Choose a reason for hiding this comment

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

I want to group all these functions somehow -- this whole formatting code should probably be rewritten or simplified in 3.8

Lib/asyncio/coroutines.py Outdated
Copy link
Copy Markdown
Contributor

@asvetlov asvetlov May 28, 2018

Choose a reason for hiding this comment

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

From my perspective replacement running variable with explicit return coro.cr_running etc is more readable.
BTW when running is not bool?

Copy link
Copy Markdown
Member Author

@1st1 1st1 May 28, 2018

Choose a reason for hiding this comment

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

cr_running and gi_running should be bools or 0 or 1... So shouldn't be a problem.

asvetlov
asvetlov approved these changes May 28, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

LGTM.
I share the opinion that the code could be improved/rewritten but for sake of minimal Python 3.7 impact let's keep changes small.

Lib/asyncio/coroutines.py Outdated
Copy link
Copy Markdown
Contributor

@asvetlov asvetlov May 28, 2018

Choose a reason for hiding this comment

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

Ok, got it.

@1st1 1st1 merged commit 989b9e0 into python:master May 28, 2018
@1st1 1st1 deleted the fixcy branch May 28, 2018 20:27
@miss-islington
Copy link
Copy Markdown
Contributor

miss-islington commented May 28, 2018

Thanks @1st1 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

@miss-islington
Copy link
Copy Markdown
Contributor

miss-islington commented May 28, 2018

Thanks @1st1 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@miss-islington
Copy link
Copy Markdown
Contributor

miss-islington commented May 28, 2018

Sorry, @1st1, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 989b9e0e6d7dd2fa911f9bfd4744e7f3a82d6006 3.6

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 28, 2018
@1st1 @miss-islington
bpo-33672: Fix Task.__repr__ crash with Cython's bogus coroutines (py…
5a2a44d 
…thonGH-7161)

(cherry picked from commit 989b9e0)

Co-authored-by: Yury Selivanov <yury@magic.io>
@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented May 28, 2018

GH-7173 is a backport of this pull request to the 3.7 branch.

1st1 added a commit that referenced this pull request May 28, 2018
@miss-islington @1st1
bpo-33672: Fix Task.__repr__ crash with Cython's bogus coroutines (GH…
075c662 
…-7161) (GH-7173)

(cherry picked from commit 989b9e0)

Co-authored-by: Yury Selivanov <yury@magic.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@scoder scoder scoder left review comments

@asvetlov asvetlov asvetlov approved these changes

Assignees

@1st1 1st1

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@1st1 @miss-islington @bedevere-bot @asvetlov @scoder @the-knights-who-say-ni