Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.7k
gh-105331: Fix asyncio.sleep() bug#105513
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
bedevere-bot commentedJun 8, 2023
Most changes to Pythonrequire a NEWS entry. Please add it using theblurb_it web app or theblurb command-line tool. |
| raiseTypeError(f"{type(delay)} object cannot be interpreted as an integer") | ||
| # According to Modules/timemodule.c | ||
| if (delay>sys.maxsize/SEC_TO_NS): |
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 incorrect because of 32-bit platform (and that's why asyncio test failing on Windows 32-bit).sys.maxsize on 32-bit platform equal to2 ** 31 -1.
So, in that casesys.maxsize / SEC_TO_NS is equal to2.147483647
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.
Oh!
I forgot about this matter.
Intimemodule.c, INT64_MAX is used and is not subject to the limitation of 32-bit.
If I write it asINT64_MAX = 2**64 - 1, how would it be?
if (delay > INT64_MAX / SEC_TO_NS)
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.
Oh! I forgot about this matter. In
timemodule.c, INT64_MAX is used and is not subject to the limitation of 32-bit. If I write it asINT64_MAX = 2**64 - 1, how would it be?
if (delay > INT64_MAX / SEC_TO_NS)
I think it's correct solution, but I'm not expert in this area. Let's wait for other opinions
| if (delay<0): | ||
| raiseValueError("sleep length must be non-negative") | ||
| if (math.isnan(delay)): |
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.
Also, seems there need to add check that delay is finite number. Use something likeif not math.isfinite(delay): ... for resolve that.
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.
This appears to be more accurate.
Thank you!
gvanrossum commentedJun 10, 2023
This PR has some scope creep compared to the issue it purports to fix -- it changes the behavior if the delay is negative. While this was discussed in the issue, no conclusion was drawn (there were conflicting opinions). If it wasjust the NaN check I'd approve it after fixing the grammar, punctuation and markup in the NEWS file, but as it is, I can't. |
fix asyncio.sleep(float('nan')) does not raise ValueError