-
Notifications
You must be signed in to change notification settings - Fork 108
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
base: master
Are you sure you want to change the base?
Add asyncio
backend
#930
Conversation
0d3a1f4
to
db58896
Compare
return backend_name, options | ||
|
||
|
||
class Server(uvicorn.Server): |
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.
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
009249a
to
56447f4
Compare
In order to land this PR I'd suggest adding an The default backend should not change as part of the pull request, and the existing The documentation for https://www.encode.io/httpcore/network-backends/#async-network-backends should also be modified to include the native network_backend = httpcore.AsyncIOBackend()
async with httpcore.AsyncConnectionPool(network_backend=network_backend) as http:
... Similar to the existing (Minimal incremental changes ftw) |
56447f4
to
81b6b1f
Compare
Yep that I didnt change as you previously suggested |
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 |
Added this |
The code & docs changes look reasonable. |
9f13ff5
to
3814cf4
Compare
Previously the 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 |
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.
Looks great, thanks so much.
Naming... should we use AsyncIOStream
and AsyncIOBackend
or AsyncioStream
and AsyncioBackend
?
Thanks for doing this. I think this will close out the discussion I opened #893 |
No problem! Its now renamed to |
Fantastic, appreciate your time on this @MarkusSintonen. We could either get #957 released first then merge this, or include this now and bump the version. |
@tomchristie no problem! Would it be possible to include also the asyncio-based synchronization changes as part of this? |
httpcore/_backends/asyncio.py
Outdated
self._read_lock = asyncio.Lock() | ||
self._write_lock = asyncio.Lock() |
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.
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)
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.
Done, they are now gone
httpcore/_backends/asyncio.py
Outdated
self._read_lock = asyncio.Lock() | ||
self._write_lock = asyncio.Lock() |
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.
self._read_lock = asyncio.Lock() | |
self._write_lock = asyncio.Lock() |
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.
Done
httpcore/_backends/asyncio.py
Outdated
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 |
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.
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 |
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.
Done
httpcore/_backends/asyncio.py
Outdated
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) |
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.
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) |
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.
Done
Thanks for the ask, let's not do that. Mixing too many different concerns. (Minimal incremental changes ftw) |
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):
Now:
Related discussion encode/httpx#3215 (comment).
Checklist