httpcore icon indicating copy to clipboard operation
httpcore copied to clipboard

Add `asyncio` backend

Open MarkusSintonen opened this issue 1 year ago • 12 comments

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 https://github.com/encode/httpx/issues/3215#issuecomment-2155630018.

Checklist

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

MarkusSintonen avatar Jun 16 '24 18:06 MarkusSintonen

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)

lovelydinosaur avatar Jun 17 '24 09:06 lovelydinosaur

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

Yep that I didnt change as you previously suggested

MarkusSintonen avatar Jun 17 '24 12:06 MarkusSintonen

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 avatar Jun 17 '24 13:06 MarkusSintonen

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 avatar Jun 17 '24 13:06 MarkusSintonen

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

MarkusSintonen avatar Jun 18 '24 08:06 MarkusSintonen

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

lovelydinosaur avatar Jun 18 '24 08:06 lovelydinosaur

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)

MarkusSintonen avatar Jun 18 '24 13:06 MarkusSintonen

Thanks for doing this. I think this will close out the discussion I opened https://github.com/encode/httpcore/discussions/893

bdraco avatar Sep 29 '24 02:09 bdraco

Looks great, thanks so much.

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

No problem! Its now renamed to AsyncIOBackend. Thanks

MarkusSintonen avatar Oct 01 '24 07:10 MarkusSintonen

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.

lovelydinosaur avatar Oct 01 '24 10:10 lovelydinosaur

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

MarkusSintonen avatar Oct 01 '24 11:10 MarkusSintonen

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)

lovelydinosaur avatar Oct 01 '24 11:10 lovelydinosaur

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Apr 26 '25 04:04 stale[bot]