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.

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

Add asyncio backend #930

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

MarkusSintonen
Copy link
Contributor

@MarkusSintonen MarkusSintonen commented Jun 16, 2024

Summary

Adds back native asyncio backend based on commit where it was removed here. Adjusts it to work with the new interfaces. This also adds some additional integration testing.

Doesn't make the new backend the default.

AnyIO has considerable overhead so this allows again using the native backend. Also this is going to allow removal of the AnyIO as the dependency (when the synchronization PR goes in also).

Overhead is about 1.45x here (the synchronization has a lot more overhead in the other PR).

Previously (is master):
old

Now:
asyncio

Related discussion encode/httpx#3215 (comment).

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

return backend_name, options


class Server(uvicorn.Server):
Copy link
Contributor Author

@MarkusSintonen MarkusSintonen Jun 16, 2024

Choose a reason for hiding this comment

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

Seems the httpbin fixture doesnt work well for pool related testing as it always closes the connections immeaditely... So adding this for testing those aspects in integration testing file

@MarkusSintonen MarkusSintonen force-pushed the asyncio-backend2 branch 3 times, most recently from 009249a to 56447f4 Compare June 17, 2024 09:50
@tomchristie
Copy link
Member

tomchristie commented Jun 17, 2024

In order to land this PR I'd suggest adding an asyncio native backend without any other changes.

The default backend should not change as part of the pull request, and the existing anyio backend should not be modified. I'd expect the only modified code files to be __init__.py and httpcore/_backends/asyncio.py.

The documentation for https://www.encode.io/httpcore/network-backends/#async-network-backends should also be modified to include the native asyncio backend, demo'ing it's usage with...

    network_backend = httpcore.AsyncIOBackend()
    async with httpcore.AsyncConnectionPool(network_backend=network_backend) as http:
        ...

Similar to the existing AnyIOBackend example.

(Minimal incremental changes ftw)

@MarkusSintonen
Copy link
Contributor Author

The default backend should not change as part of the pull request

Yep that I didnt change as you previously suggested

@MarkusSintonen
Copy link
Contributor Author

In order to land this PR I'd suggest adding an asyncio native backend without any other changes.

The default backend should not change as part of the pull request, and the existing anyio backend should not be modified. I'd expect the only modified code files to be __init__.py and httpcore/_backends/asyncio.py.

This is now done, so there is only the new backend and export. + The tests I used add the full coverage and verify it works

@MarkusSintonen
Copy link
Contributor Author

The documentation for https://www.encode.io/httpcore/network-backends/#async-network-backends should also be modified to include the native asyncio backend, demo'ing it's usage with

Added this

@MarkusSintonen
Copy link
Contributor Author

Asyncio-backend performs also better when all other current optimization PRs are applied (benchmark params REPEATS=100 REQUESTS=50 CONCURRENCY=20):

With AnyIO-backend:
anyio

With asyncio-backend:
asyncio

@tomchristie
Copy link
Member

The code & docs changes look reasonable.
Not obvious that the tests cover the functionality added?

@MarkusSintonen
Copy link
Contributor Author

MarkusSintonen commented Jun 18, 2024

Not obvious that the tests cover the functionality added?

Previously the @pytest.mark.anyio runned the tests as parametrized by asyncio-anyio and trio (which are the default). But now via the conftest, we specify the test parametrization to also include the plain asyncio test mode (as documented in AnyIO fixture docs https://anyio.readthedocs.io/en/stable/testing.html#specifying-the-backends-to-run-on).

With this and the added integration testing (eg UDS, socket options) we get close to 100% coverage for the new network backend. (Actually we get even more coverage for anyio/trio backends so there is some no longer needed nocoverage)

Copy link
Member

@tomchristie tomchristie left a comment

Choose a reason for hiding this comment

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

Looks great, thanks so much.

Naming... should we use AsyncIOStream and AsyncIOBackend or AsyncioStream and AsyncioBackend?

@bdraco
Copy link
Contributor

bdraco commented Sep 29, 2024

Thanks for doing this. I think this will close out the discussion I opened #893

@MarkusSintonen
Copy link
Contributor Author

Looks great, thanks so much.

Naming... should we use AsyncIOStream and AsyncIOBackend or AsyncioStream and AsyncioBackend?

No problem! Its now renamed to AsyncIOBackend. Thanks

@tomchristie
Copy link
Member

Fantastic, appreciate your time on this @MarkusSintonen.
Following SEMVER we'll want this in a 1.1.0 release. (New functionality)

We could either get #957 released first then merge this, or include this now and bump the version.

@MarkusSintonen
Copy link
Contributor Author

@tomchristie no problem! Would it be possible to include also the asyncio-based synchronization changes as part of this?

Comment on lines 25 to 26
self._read_lock = asyncio.Lock()
self._write_lock = asyncio.Lock()
Copy link
Member

Choose a reason for hiding this comment

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

We don't use locking at the stream layer, let's remove these.

(I think we had locking at this layer in the past, the locking is now handled at the HTTP11Connection/HTTP2Connection layer)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, they are now gone

Comment on lines 25 to 26
self._read_lock = asyncio.Lock()
self._write_lock = asyncio.Lock()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self._read_lock = asyncio.Lock()
self._write_lock = asyncio.Lock()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 79 to 95
async with self._read_lock:
with map_exceptions(exc_map):
try:
return await asyncio.wait_for(
self._stream_reader.read(max_bytes), timeout
)
except AttributeError as exc: # pragma: nocover
if "resume_reading" in str(exc):
# Python's asyncio has a bug that can occur when a
# connection has been closed, while it is paused.
# See: https://github.com/encode/httpx/issues/1213
#
# Returning an empty byte-string to indicate connection
# close will eventually raise an httpcore.RemoteProtocolError
# to the user when this goes through our HTTP parsing layer.
return b""
raise
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
async with self._read_lock:
with map_exceptions(exc_map):
try:
return await asyncio.wait_for(
self._stream_reader.read(max_bytes), timeout
)
except AttributeError as exc: # pragma: nocover
if "resume_reading" in str(exc):
# Python's asyncio has a bug that can occur when a
# connection has been closed, while it is paused.
# See: https://github.com/encode/httpx/issues/1213
#
# Returning an empty byte-string to indicate connection
# close will eventually raise an httpcore.RemoteProtocolError
# to the user when this goes through our HTTP parsing layer.
return b""
raise
with map_exceptions(exc_map):
try:
return await asyncio.wait_for(
self._stream_reader.read(max_bytes), timeout
)
except AttributeError as exc: # pragma: nocover
if "resume_reading" in str(exc):
# Python's asyncio has a bug that can occur when a
# connection has been closed, while it is paused.
# See: https://github.com/encode/httpx/issues/1213
#
# Returning an empty byte-string to indicate connection
# close will eventually raise an httpcore.RemoteProtocolError
# to the user when this goes through our HTTP parsing layer.
return b""
raise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 105 to 108
async with self._write_lock:
with map_exceptions(exc_map):
self._stream_writer.write(data)
return await asyncio.wait_for(self._stream_writer.drain(), timeout)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
async with self._write_lock:
with map_exceptions(exc_map):
self._stream_writer.write(data)
return await asyncio.wait_for(self._stream_writer.drain(), timeout)
with map_exceptions(exc_map):
self._stream_writer.write(data)
return await asyncio.wait_for(self._stream_writer.drain(), timeout)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@tomchristie
Copy link
Member

tomchristie commented Oct 1, 2024

Would it be possible to include also the asyncio-based synchronization changes as part of this?

Thanks for the ask, let's not do that. Mixing too many different concerns.
This is a self-contained PR and we can deal with it in isolation.

(Minimal incremental changes ftw)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants