Repository navigation
gh-105331: Fix asyncio.sleep() bug - #105513
weijay0804 wants to merge 2 commits into
Conversation
Sorry, something went wrong.
|
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
Sorry, something went wrong.
| raise TypeError(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.
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 to 2 ** 31 -1.
So, in that case sys.maxsize / SEC_TO_NS is equal to 2.147483647
Sorry, something went wrong.
There was a problem hiding this comment.
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 as INT64_MAX = 2**64 - 1, how would it be?
if (delay > INT64_MAX / SEC_TO_NS)
Sorry, something went wrong.
There was a problem hiding this comment.
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 as INT64_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
Sorry, something went wrong.
| if (delay < 0): | ||
| raise ValueError("sleep length must be non-negative") | ||
|
|
||
| if (math.isnan(delay)): |
There was a problem hiding this comment.
Also, seems there need to add check that delay is finite number. Use something like if not math.isfinite(delay): ... for resolve that.
Sorry, something went wrong.
There was a problem hiding this comment.
This appears to be more accurate.
Thank you!
Sorry, something went wrong.
|
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 was just 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. |
Sorry, something went wrong.
fix asyncio.sleep(float('nan')) does not raise ValueError