Switch to using asyncio.timeout instead of asyncio.wait_for #1307

bdraco · 2023-03-07T21:56:41Z

asyncio.wait_for creates a task whereas asyncio.timeout avoids doing this which decreases latency.

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 · 2023-03-08T09:33:23Z

Thank you for the suggestion! I think it's a good idea. Let's do it.

Until now, websockets has no dependencies. This removes an entire category of concerns like managing versions that, as a single maintainer, I don't want to deal with. I understand that this may not look like a big benefit but it is valuable to me.

In order to keep this benefit, I am tempted to do one of the following:

create a small compatibility layer to use asyncio.wait_for on Python < 3.11 and asyncio.timeout on Python ≥ 3.12
vendor CPython's or async-timeout's implementation — they're probably identical if async-timeout was merged into CPython.

Option 1 could be as simple as:

if sys.version_info[:2] >= (3, 12):

    async def asyncio_wait_for(coro, timeout):
        async with asyncio.timeout(timeout):
            await coro

else:

    asyncio_wait_for = asyncio.wait_for

In all cases, version compatibility code should go in websockets.legacy.compatibility rather than a new module.

aaugustin · 2023-03-08T09:34:49Z

I'm aware that my option 1 doesn't yield performance benefits on Python < 3.12 while your version does. IMO those who care about performance optimizations available in Python 3.12 should upgrade to Python 3.12. It isn't my job to fake that for them.

bdraco · 2023-03-08T17:33:56Z

I'll work on option 2 since the races in async.wait_for and the issue tickets they generated for cpython were part of the motivation for cpython switching out the implementation. It think it would be best if everything behaved the same regardless of the python version.

ex:
python/cpython#86296
python/cpython#81839
python/cpython#96764

bdraco · 2023-03-08T18:57:32Z

Its passing locally now. Hopefully it should be good to go now

bdraco · 2023-03-08T18:58:36Z

tests/legacy/test_protocol.py

@@ -929,6 +929,7 @@ def test_answer_ping_does_not_crash_if_connection_closed(self):
        self.receive_frame(Frame(True, OP_PING, b"test"))
        self.receive_eof()

+        self.loop.run_until_complete(asyncio.sleep(MS))


Since there is no task being created in protocol.close() anymore its happening faster. The test previously relied on the wait_for yielding to the event loop

I'm not surprised that you're finding this sort of issue in the test suite.

It would be best to use self.run_loop_once(). It's faster than waiting 1ms.

Yes, it's just 1ms, but over hundreds of tests, delays add up!

aaugustin

Thank you again for the pull request. We're close to being able to merge it.

I left a few comments to polish it. Let me know if you'd like to do it or if I should finalize.

If you think this optimization deserves an entry in changelog.rst, feel free to add it.

aaugustin · 2023-03-29T05:14:01Z

tests/legacy/test_protocol.py

@@ -929,6 +929,7 @@ def test_answer_ping_does_not_crash_if_connection_closed(self):
        self.receive_frame(Frame(True, OP_PING, b"test"))
        self.receive_eof()

+        self.loop.run_until_complete(asyncio.sleep(MS))


It would be best to use self.run_loop_once(). It's faster than waiting 1ms.

Yes, it's just 1ms, but over hundreds of tests, delays add up!

aaugustin · 2023-03-29T05:15:41Z

src/websockets/legacy/async_timeout/__init__.py

I would like to put the code in src/websockets/legacy/async_timeout.py. We have no reason to nest it in under async_timeout/__init__.py.

aaugustin · 2023-03-29T05:19:17Z

src/websockets/legacy/compatibility.py

+
+if sys.version_info[:2] < (3, 11):
+    from .async_timeout import timeout as asyncio_timeout  # pragma: no cover
+else:  # pragma: no cover


I made changes in 399db68 so you can drop the # pragma: no cover on the two lines above.

aaugustin · 2023-03-29T05:20:48Z

src/websockets/legacy/compatibility.py

+    from asyncio import timeout as asyncio_timeout
+
+
+__all__ = ["asyncio_timeout", "loop_if_py_lt_38"]


This isn't needed because aren't doing from websockets.legacy.compatibility import * anywhere.

Did I miss a reason why it would be useful? Or did you just add it for consistency?

Ah, it avoids this warning from mypy:

src/websockets/legacy/protocol.py:56: error: Module "websockets.legacy.compatibility" does not explicitly export attribute "asyncio_timeout" [attr-defined]

aaugustin · 2023-03-29T05:23:04Z

tox.ini

@@ -18,13 +18,13 @@ commands = python -W error::DeprecationWarning -W error::PendingDeprecationWarni
 commands =
    python -m coverage erase
    python -m coverage run --source {envsitepackagesdir}/websockets,tests -m unittest {posargs}
-    python -m coverage report --show-missing --fail-under=100
+    python -m coverage report --show-missing --omit src/websockets/legacy/async_timeout/__init__.py  --fail-under=100


I made changes in 399db68 to configure this in setup.cfg rather than in all coverage commands. It should be possible to remove this change.

aaugustin · 2023-03-29T06:15:06Z

Also, It would be great if you could rebase and squash into a final commit to clean up the PR.

bdraco · 2023-03-29T08:03:58Z

Late here. I'll take a look in the morning if the Home Assistant beta isn't too busy.

aaugustin · 2023-03-29T08:14:15Z

No hurry :-)

I just finished #1245. I'm planning to make a release in a few days. I'd like to include this in the release.

It doesn't matter if we need a few more days — the last release was in October :-)

aaugustin · 2023-04-02T05:46:31Z

I squashed your commits and I'm finalizing the changes here: #1325.

EDIT - I just found out that I could push to you branch. I'm closing the other PR.

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

Some instances were missed in a previous commit. Also update documentation.

They relied (accidentally) on wait_for() creating a task, causing the event loop to run once when calling close().

aaugustin · 2023-04-02T07:56:33Z

I filed nedbat/coveragepy#1598 for the coverage problem.

Thank you for your contribution!

bdraco · 2023-04-04T08:18:33Z

I had this on my followup list and just returned to it and found you already got it over the finish line. Many thanks 👍 🐬 🥇

aaugustin · 2023-07-29T06:46:03Z

This caused encode/uvicorn#1927.

bdraco · 2023-07-29T12:53:35Z

Looks like you have already determined whats going on in encode/uvicorn#1927 (comment)

Let me know if I can help in any way.

Cheers

bdraco force-pushed the async_timeout branch 3 times, most recently from c6e2028 to 03ede5b Compare March 7, 2023 22:02

bdraco marked this pull request as ready for review March 7, 2023 22:05

bdraco commented Mar 8, 2023

View reviewed changes

aaugustin reviewed Mar 29, 2023

View reviewed changes

bdraco and others added 4 commits April 2, 2023 09:15

Backport typing.final for compatibility with Python 3.7.

9cc6e53

Replace asyncio.wait_for with asyncio.timeout.

c647562

Some instances were missed in a previous commit. Also update documentation.

Restore semantics of tests.

0499308

They relied (accidentally) on wait_for() creating a task, causing the event loop to run once when calling close().

aaugustin force-pushed the async_timeout branch from d46b6da to 0499308 Compare April 2, 2023 07:15

Work around bug in coverage.

910e2fa

aaugustin merged commit 901e434 into python-websockets:main Apr 2, 2023

zifeitong mentioned this pull request Jun 18, 2024

[Bugfix] AsyncLLMEngine hangs with asyncio.run vllm-project/vllm#5654

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to using asyncio.timeout instead of asyncio.wait_for #1307

Switch to using asyncio.timeout instead of asyncio.wait_for #1307

bdraco commented Mar 7, 2023 •

edited

Loading

aaugustin commented Mar 8, 2023

aaugustin commented Mar 8, 2023

bdraco commented Mar 8, 2023

bdraco commented Mar 8, 2023

bdraco Mar 8, 2023 •

edited

Loading

aaugustin Mar 9, 2023

aaugustin Mar 29, 2023

aaugustin left a comment

aaugustin Mar 29, 2023

aaugustin Mar 29, 2023

aaugustin Mar 29, 2023

aaugustin Mar 29, 2023

aaugustin Apr 2, 2023

aaugustin Mar 29, 2023

aaugustin commented Mar 29, 2023

bdraco commented Mar 29, 2023

aaugustin commented Mar 29, 2023

aaugustin commented Apr 2, 2023 •

edited

Loading

aaugustin commented Apr 2, 2023

bdraco commented Apr 4, 2023

aaugustin commented Jul 29, 2023

bdraco commented Jul 29, 2023

		from asyncio import timeout as asyncio_timeout


		__all__ = ["asyncio_timeout", "loop_if_py_lt_38"]

Switch to using asyncio.timeout instead of asyncio.wait_for #1307

Switch to using asyncio.timeout instead of asyncio.wait_for #1307

Conversation

bdraco commented Mar 7, 2023 • edited Loading

aaugustin commented Mar 8, 2023

aaugustin commented Mar 8, 2023

bdraco commented Mar 8, 2023

bdraco commented Mar 8, 2023

bdraco Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaugustin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaugustin commented Mar 29, 2023

bdraco commented Mar 29, 2023

aaugustin commented Mar 29, 2023

aaugustin commented Apr 2, 2023 • edited Loading

aaugustin commented Apr 2, 2023

bdraco commented Apr 4, 2023

aaugustin commented Jul 29, 2023

bdraco commented Jul 29, 2023

bdraco commented Mar 7, 2023 •

edited

Loading

bdraco Mar 8, 2023 •

edited

Loading

aaugustin commented Apr 2, 2023 •

edited

Loading