[WIP] Add HTTP/3 support by jlaine · Pull Request #283 · encode/httpcore

jlaine · 2021-03-15T15:47:07Z

This PR is still a very rough draft, it:

lacks any kind of unittest
~~only implements asyncio~~
doesn't yet do packet retransmission
doesn't handle timeouts
doesn't respect the SSL configuration

Still I am able to use the example from the README to perform an HTTP/3 requests against quic.aiortc.org port 443.

florimondmanca

Pretty exciting. ❤️

Leaving some comments on the code for you.

Edit: please ignore my comments on timeouts, I should be reading TODOs before leaving reviews. :)

florimondmanca · 2021-03-15T22:32:53Z

+        """
+        Read some data from the network, and update the H2 state.
+        """
+        data, addr = await self.socket.receive()


Do we need to apply the read timeout here?

florimondmanca · 2021-03-15T22:33:27Z

+        # send datagrams
+        now = await self.backend.time()
+        for data, addr in self._quic.datagrams_to_send(now=now):
+            await self.socket.send(data, addr)


Do we need to apply the write timeout here?

florimondmanca · 2021-03-15T22:35:26Z

+
+        return data, addr
+
+    async def send(self, data: bytes, addr: SocketAddr) -> None:


Is there something we should be "waiting" for? In other backends this calls an async operation, IDK if asyncio exposes any kind of equivalent "actually wait for something to happen" API, instead of just scheduling something for later?

To my knowledge UDP APIs don't provide any kind of backpressure.

florimondmanca · 2021-03-15T22:37:30Z

+    async def aclose(self) -> None:
+        raise NotImplementedError()  # pragma: no cover
+
+    async def receive(self) -> Tuple[bytes, SocketAddr]:


For consistency, should these be named read() and write()? Otherwise we might consider to update the doc string for this class to use the same receive/send lingo.

I don't think read / write make sense here as we are also conveying another information: the remote address. The "classical" corresponding APIs are recvfrom and sendto but as I understand we're avoid abbreviations, I went for "receive" and "send".

anyio seems to have settled on the same names: https://github.com/agronholm/anyio/blob/3a0b805a3f4410009f2b6610d329fb9cc2b90e5a/src/anyio/_backends/_asyncio.py#L1258

florimondmanca · 2021-03-15T22:39:29Z

 Origin = Tuple[bytes, bytes, int]
 URL = Tuple[bytes, bytes, Optional[int], bytes]
 Headers = List[Tuple[bytes, bytes]]
+SocketAddr = Tuple


Anything more specific?

It varies.. you can receive Tuple[str, int] or Tuple[str, int, int, int]

florimondmanca · 2021-03-15T22:41:42Z

+        loop = asyncio.get_event_loop()
+
+        infos = await loop.getaddrinfo(hostname, port, type=socket.SOCK_DGRAM)
+        _, local_addr, remote_addr = _utils.udp_socket_parameters(infos[0][4])


Might be worth an intermediate variable for this obscure infos[0][4] thingy?

florimondmanca · 2021-03-15T22:48:16Z

As @tomchristie mentioned in the issue, I'm not sure if we should be adding this as experimental to HTTPCore itself right away, or host the code separately?

Perhaps we could start with a gist (although somewhat lengthy) that completely ignores any connection pooling, and just provides a fully functional standalone H3Connection transport for people to play with? Could even be hosted on a proper repo in the interim. Then we get some feedback and clean up some rough edges, and work from there? I'm sure there are ways to either copy paste some stable code or strip out things we don't actually need for a barebones standalone connection implementation, so the end result is realistic enough yet feasible as a separate experimental project at first.

I'd also be curious about some docs draft, see how we'd frame this for users?

jlaine · 2021-03-15T23:38:57Z

I'm not sure "hosting it externally" makes much sense, there has been an httpx demo in aioquic's example for over a year and a half, during which time it bit-rotted due to API refactoring in httpx. The same is likely to happen again with httpcore. I'm also not very keen on duplicating httpcore's mechanisms for cross-framework abstractions in an outside project.

I'm not sure I understand the rationale behind wanting to remove support for connection pooling, the behaviour is almost identical to HTTP/2?

florimondmanca · 2021-03-16T00:53:27Z

My suggestion to set pooling aside for now was to make separate hosting easier. :)

The difference with your aioquic proof of concept would be that we'd be documenting HTTP/3 experimental support anyway (so even if that's as a separate thing we'd be targeting traffic at it).

Honestly though, given how self-contained and additive this pull request is (and will likely remain), I'm personally pretty open to having this as experimental in core. We accepted the AnyIO backend as a similar low-complexity additive change. And although I share the same reservations as Tom in terms getting the framing right for users (ie "you probably don't need the complexity that comes with HTTP/3"), I'm quite excited at the idea of demonstrating functional HTTP/3 support in HTTPCore/HTTPX. Would kind of prove the point that this "transport" architecture allows the project to evolve with industry trends.

lovelydinosaur · 2021-03-16T10:20:14Z

I'm not sure I understand the rationale behind wanting to remove support for connection pooling, the behaviour is almost identical to HTTP/2?

So, the behaviour with this pull request between http2=True and http3=True is actually pretty different.

With http2=True enabled, you'll get optional HTTP/2 support, with either HTTP/1.1 or HTTP/2 connections being opened and connection pooled. The determination will vary per-server connection, depending on the ALPS negotiation. (IE. http2=True enables HTTP/2 support, rather than mandating it.)

In contrast, setting http3=True here always attempts an HTTP/3 connection, and will fail on requests against HTTP/1.1 or HTTP/2 servers.

Really what we'd like to do with HTTP/3 support is determine if support is available or not based on the AltSrv header, and only switch up to HTTP/3 where available. But that's a whole other kettle of fish.

What I would like to understand first, is what are the blockers to us instead starting with an incremental approach of http3-as-a-third-party-transport-to-start-with, and being able to document something like...

import httpx, aioquic

# Use HTTP/3 support.
async with httpx.AsyncClient(transport=aioquic.HTTP3Transport()) as client:
    ...

Working from that perspective is really important to me, because there's always a huge volume of unsurfaced complexities that come along with pulling in something mega like HTTP/3 support, and the incremental approach almost always does a better job of helping thrash those out in a manageable step-by-step way.

There's a big difference between us advertising that support on https://www.python-httpx.org/ vs. example code in the aioquic GitHub repo.

The obvious blockers I can see would be...

It'd potentially be asyncio-only to start with (because replicating our backend switching would be an unwanted pain.)
There'd need to be a bit of connection pooling work put directly in to HTTP3Transport.

jlaine · 2021-03-16T11:27:20Z

OK in essence we'll just maintain a variant of the status quo: the "transport" is a demo provided in the aioquic repo. It might graduate to being shipped with aioquic in an asyncio.httpx.H3Transport.

However note that it brings us no nearer to an eventual integration in httpcore or httpx, because the two are implemented very differently. This PR demonstrates my point, compare this code to the demo code in aioquic.

For for the demo I don't have any incentive to use anything looking like the eventual "UDP socket abstraction" or "timer abstraction". If it's going to be async-io only, I can keep using the higher-level QuicConnectionProtocol which ships with aioquic.

lovelydinosaur · 2021-03-16T12:30:06Z

For for the demo I don't have any incentive to use anything looking like the eventual "UDP socket abstraction" or "timer abstraction". If it's going to be async-io only, I can keep using the higher-level QuicConnectionProtocol which ships with aioquic.

Right, which from my POV is actually okay, because we can start with thrashing out any remaining issues in an asyncio-only context to begin with. Later once we're super happy with the state of all that and we're getting real world feedback, we can look at the next steps of a built-in integration.

I'm a bit absolute on the "take the smallest steps forward possible at each point" incremental approach. Anything else always seems to come back and bite ya. Hard.

jlaine marked this pull request as draft March 15, 2021 15:47

jlaine mentioned this pull request Mar 15, 2021

HTTP/3 support #173

Closed

jlaine force-pushed the http3 branch 5 times, most recently from 0778d66 to b46a2c5 Compare March 15, 2021 20:34

jlaine added 2 commits March 15, 2021 22:47

Add a DatagramSocket abstraction

4ab181c

WIP: add HTTP/3 backend

3e1a307

jlaine force-pushed the http3 branch from b46a2c5 to 3e1a307 Compare March 15, 2021 21:48

florimondmanca reviewed Mar 15, 2021

View reviewed changes

jlaine closed this Mar 16, 2021


		return data, addr

		async def send(self, data: bytes, addr: SocketAddr) -> None:

Conversation

jlaine commented Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

florimondmanca left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

florimondmanca commented Mar 15, 2021

Uh oh!

jlaine commented Mar 15, 2021

Uh oh!

florimondmanca commented Mar 16, 2021

Uh oh!

lovelydinosaur commented Mar 16, 2021

Uh oh!

jlaine commented Mar 16, 2021

Uh oh!

lovelydinosaur commented Mar 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

jlaine commented Mar 15, 2021 •

edited

Loading

florimondmanca left a comment •

edited

Loading