bpo-33672: Fix Task.__repr__ crash with Cython's bogus coroutines by 1st1 · Pull Request #7161 · python/cpython
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?
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.
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.
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?
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
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, got it.
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.
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
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?
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.
Yury Selivanov added 2 commits
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, got it.
Thanks @1st1 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖
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!
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
…thonGH-7161) (cherry picked from commit 989b9e0) Co-authored-by: Yury Selivanov <yury@magic.io>
1st1 pushed a commit that referenced this pull request
