Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GH-96764: rewrite asyncio.wait_for to use asyncio.timeout #98518

Merged
merged 29 commits into from
Feb 16, 2023
Merged

GH-96764: rewrite asyncio.wait_for to use asyncio.timeout #98518

merged 29 commits into from
Feb 16, 2023

Conversation

kumaraditya303
Copy link
Contributor

@kumaraditya303 kumaraditya303 commented Oct 21, 2022 •
edited
Loading

This PR changes asyncio.wait_for to use asyncio.timeout as its underlying implementation. It simplifies the code and makes it easy to understand the cancellation semantics as both asyncio.timeout and asyncio.wait_for behaves similarly.

Fixes #86296
Fixes #81839
Fixes #96764

@kumaraditya303
Copy link
Contributor Author

This is more of a POC at this point. I think it is worth changing wait_for to use timeout in 3.12+. This seems to fix some of the open bugs of wait_for, I'll add those tests to make sure.

cc @gvanrossum

gvanrossum
gvanrossum reviewed Oct 21, 2022
View reviewed changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

You got a bit farther than I did when I tried this, but I'm still concerned why you had to modify several tests. (Also, there's one test that fails in CI, but doesn't appear to fail when I run it locally.)

Lib/asyncio/tasks.py Show resolved Hide resolved
Lib/test/test_asyncio/test_futures2.py Show resolved Hide resolved
Lib/test/test_asyncio/test_waitfor.py Show resolved Hide resolved
@gvanrossum gvanrossum mentioned this pull request Oct 21, 2022
Closed
gvanrossum
gvanrossum approved these changes Oct 24, 2022
View reviewed changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

LG (one naming nit).

Lib/test/test_asyncio/test_waitfor.py Outdated Show resolved Hide resolved
@kumaraditya303 kumaraditya303 marked this pull request as ready for review October 24, 2022 06:03
@kumaraditya303 kumaraditya303 added type-feature A feature request or enhancement topic-asyncio labels Oct 25, 2022
@gvanrossum
Copy link
Member

I had hoped to first merge one of the alternatives that can be backported to 3.10, so we can declare this fixed in 3.10. Then we would merge this one on top of that, but only in 3.11 and main.

But we could also just do a custom fix for 3.10 based on one of @twisteroidambassador's PRs. (I wish you had linked this PR to the same issue rather than creating a new issue, since it's all related.)

@gvanrossum
Copy link
Member

In particular, @twisteroidambassador has this PR: #98607

@bedevere-bot bedevere-bot mentioned this pull request Nov 30, 2022
Closed
@robsdedude robsdedude mentioned this pull request Dec 14, 2022
Closed
@python python deleted a comment from netlify bot Dec 14, 2022
@bdraco bdraco mentioned this pull request Mar 8, 2023
Closed
mdonoughe pushed a commit to gurumitts/pylutron-caseta that referenced this pull request Mar 11, 2023
@bdraco
Switch to using asyncio.timeout instead of asyncio.wait_for (#138)
c11a6b0 
* Switch to using asyncio.timeout instead of asyncio.wait_for

`asyncio.wait_for` creates a task whereas `asyncio.timeout` avoids doing this.

Fallback to using `async_timeout` when the python version is too old (<3.11)

`asyncio.timeout` will become the underlying implementation for `async.wait_for` in cpython 3.12
python/cpython#98518
aaugustin pushed a commit to python-websockets/websockets that referenced this pull request Apr 2, 2023
@bdraco @aaugustin
Use asyncio.timeout instead of asyncio.wait_for.
21ea6d5 
asyncio.wait_for creates a task whereas asyncio.timeout doesn't.

Fallback to a vendored version of async_timeout on Python < 3.11.

async.timeout will become the underlying implementation for
async.wait_for in Python 3.12:
python/cpython#98518
aaugustin pushed a commit to bdraco/websockets that referenced this pull request Apr 2, 2023
@bdraco @aaugustin
Use asyncio.timeout instead of asyncio.wait_for.
2d75f03 
asyncio.wait_for creates a task whereas asyncio.timeout doesn't.

Fallback to a vendored version of async_timeout on Python < 3.11.

async.timeout will become the underlying implementation for
async.wait_for in Python 3.12:
python/cpython#98518
aaugustin pushed a commit to python-websockets/websockets that referenced this pull request Apr 2, 2023
@bdraco @aaugustin
Use asyncio.timeout instead of asyncio.wait_for.
5113cd3 
asyncio.wait_for creates a task whereas asyncio.timeout doesn't.

Fallback to a vendored version of async_timeout on Python < 3.11.

async.timeout will become the underlying implementation for
async.wait_for in Python 3.12:
python/cpython#98518
@puddly puddly mentioned this pull request Apr 4, 2023
Merged
This was referenced Apr 9, 2023
Closed
Closed
bdraco added a commit to bdraco/aiolifx that referenced this pull request Apr 10, 2023
@bdraco
Switch to using async_timeout/asyncio.timeout
c2f2fa0 
`asyncio.wait_for` creates another tasks which leads to some
race conditions in cancelation and a performance hit

cpython 3.12 will change the underlying implementation of
`asyncio.wait_for` to use `asyncio.wait` but that is still
a long way off for many people:

python/cpython#98518
This was referenced Apr 10, 2023
Merged
Merged
Merged
@farmio farmio mentioned this pull request Apr 13, 2023
Merged
7 tasks
@willingc willingc mentioned this pull request Apr 26, 2023
Open
bdraco added a commit to bdraco/python-kasa that referenced this pull request Jul 21, 2023
@bdraco
Replace asyncio.wait_for with asyncio.timeout
f8335d3 
Fallback to using async_timeout on older python

asyncio.wait_for has some underlying problems that are only fixed in
cpython 3.12. See python/cpython#98518
@bdraco bdraco mentioned this pull request Jul 21, 2023
Merged
bdraco added a commit to bdraco/python-kasa that referenced this pull request Jul 21, 2023
@bdraco
Replace asyncio.wait_for with asyncio.timeout
f0630f5 
Fallback to using async_timeout on older python

asyncio.wait_for has some underlying problems that are only fixed in
cpython 3.12. See python/cpython#98518
bdraco added a commit to bdraco/python-kasa that referenced this pull request Jul 21, 2023
@bdraco
Replace asyncio.wait_for with asyncio.timeout
dae93fd 
Fallback to using async_timeout on older python

asyncio.wait_for has some underlying problems that are only fixed in
cpython 3.12. See python/cpython#98518
bdraco added a commit to bdraco/python-kasa that referenced this pull request Jul 21, 2023
@bdraco
Replace asyncio.wait_for with asyncio.timeout
fe6a6f5 
Fallback to using async_timeout on older python

asyncio.wait_for has some underlying problems that are only fixed in
cpython 3.12. See python/cpython#98518
rytilahti pushed a commit to python-kasa/python-kasa that referenced this pull request Jul 21, 2023
@bdraco
Replace asyncio.wait_for with async-timeout (#480)
117a7ac 
asyncio.wait_for has some underlying problems that are only fixed in cpython 3.12.

Use async_timeout instead until the minimum supported version is 3.11+ and it can be replaced with asyncio.timeout

See python/cpython#98518
bdraco added a commit to bdraco/ha-HAP-python that referenced this pull request Jul 23, 2023
@bdraco
Switch timeouts to use async_timeout
b1e0a11 
async_timeout does not suffer from the same race
problems as asyncio.wait_for

see python/cpython#98518
for more details
@bdraco bdraco mentioned this pull request Jul 23, 2023
Merged
SomberNight added a commit to SomberNight/electrum that referenced this pull request Aug 4, 2023
@SomberNight
asyncio.wait_for() is too buggy. use util.wait_for2() instead
1b1b79e 
wasted some time because asyncio.wait_for() was suppressing cancellations. [0][1][2]
deja vu... [3]

Looks like this is finally getting fixed in cpython 3.12 [4]
So far away...
In attempt to avoid encountering this again, let's try using
asyncio.timeout in 3.11, which is how upstream reimplemented wait_for in 3.12 [4], and
aiorpcx.timeout_after in 3.8-3.10.

[0] python/cpython#86296
[1] https://bugs.python.org/issue42130
[2] https://bugs.python.org/issue45098
[3] kyuupichan/aiorpcX#44
[4] python/cpython#98518
SomberNight added a commit to spesmilo/electrum that referenced this pull request Aug 4, 2023
@SomberNight
asyncio.wait_for() is too buggy. use util.wait_for2() instead
d51f00e 
wasted some time because asyncio.wait_for() was suppressing cancellations. [0][1][2]
deja vu... [3]

Looks like this is finally getting fixed in cpython 3.12 [4]
So far away...
In attempt to avoid encountering this again, let's try using
asyncio.timeout in 3.11, which is how upstream reimplemented wait_for in 3.12 [4], and
aiorpcx.timeout_after in 3.8-3.10.

[0] python/cpython#86296
[1] https://bugs.python.org/issue42130
[2] https://bugs.python.org/issue45098
[3] kyuupichan/aiorpcX#44
[4] python/cpython#98518
bdraco added a commit to bdraco/python-androidtv that referenced this pull request Aug 31, 2023
@bdraco
Switch usage of asyncio.wait_for to async_timeout
0d228fb 
`asyncio.wait_for` creates another tasks which leads to some race conditions in cancelation and a performance hit

cpython 3.12 will change the underlying implementation of `asyncio.wait_for` to use `asyncio.wait` but that is still a long way off for many people:

python/cpython#98518
@bdraco bdraco mentioned this pull request Aug 31, 2023
Merged
JeffLIrion pushed a commit to JeffLIrion/python-androidtv that referenced this pull request Sep 1, 2023
@bdraco
Switch usage of asyncio.wait_for to async_timeout (#337)
a7b1e51 
* Switch usage of asyncio.wait_for to async_timeout

`asyncio.wait_for` creates another tasks which leads to some race conditions in cancelation and a performance hit

cpython 3.12 will change the underlying implementation of `asyncio.wait_for` to use `asyncio.wait` but that is still a long way off for many people:

python/cpython#98518

* adjust ci
@cdce8p cdce8p mentioned this pull request Sep 18, 2023
Closed
32 tasks
@cdce8p cdce8p mentioned this pull request Sep 27, 2023
Merged
20 tasks
@bdraco bdraco mentioned this pull request Nov 8, 2023
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@twisteroidambassador twisteroidambassador twisteroidambassador left review comments

@gvanrossum gvanrossum gvanrossum approved these changes

@akulakov akulakov akulakov left review comments

@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner

@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

Assignees
No one assigned
Labels
topic-asyncio type-feature A feature request or enhancement
Projects
None yet
Milestone
No milestone
5 participants
@kumaraditya303 @gvanrossum @akulakov @twisteroidambassador @bedevere-bot