Improved H3 for hypercorn. #201
Conversation
|
Also besides testing there are still some improvements needed for connection cleanup as the current method I'm using is inefficient. Finally we may want to add "retry mode" to the handshake, as this forces the client to prove they can roundtrip with you before you start serving them. I'm making this PR now just to give you a heads up and show you what I'm thinking early. Oh, and thanks for all the good stuff! |
rthalley
changed the title
|
The asyncio support is now hand tested and works, and state management is cleaner and cleanup is now efficient. I still have to add retry mode. |
|
Example performance difference: Stock hypercorn serving a 20 MiB file to curl: Note that it crashed after only about 8 MiB because all the spawned timers firing are happening so frequently that the loss detection code in aioquic (which isn't expecting to be called without a chance to progress) is backing off so much it exceeds the exponent representation size in a float. Aioquic should guard itself better here, but my point is we are at 3.2 seconds and haven't transferred much. Much of the time spent seems related to task scheduling. Here's the code from this PR doing the same thing: This is a successful transfer of all 20 MiB. Granted it's still only around 1/9 the speed of transferring from nginx, but it finishes correctly and is faster than the one that fails. |
|
Many thanks for doing this, I'm short of time to review it at the moment. |
|
No worries, it will be there when you are ready for it :) |
|
Thanks. I wanted to go in a slightly different direction and utilise a Hypercorn pattern for single tasks. I've done so in ab98383. Thoughts very welcome. I'm now looking at the retry code - why would this be a configuration option? Is there a reason to disable this? |
| if self.retry is not None: | ||
| if not header.token: | ||
| if header.version is None: | ||
| return |
There was a problem hiding this comment.
Why return here, is a missing version an indication of an error?
There was a problem hiding this comment.
Retry is an option because it trades off increased latency for ensuring that you can roundtrip to the client and aren't just being used to amplify an attack (with the start of the TLS handshake) by someone spoofing UDP. I usually turn it on.
Re the missing version... if there is no header token, then this isn't the client retrying, and we have to ask them to retry, but to do that we need the version. If there isn't a version, they've sent us a versionless "short header" packet which is not a sensible thing to do, so we just drop the packet.
|
I'm ok with going a different direction, assuming you reuse/adapt a few other parts of this patch:
|
|
I'll add the retry part in the next release, I've ran out of time today. |
I am a co-maintainer of aioquic, and am also using hypercorn w/ H3 in a project. I've noticed from testing related to an aioquic ticket that the way hypercorn's code is calling aioquic is suboptimal. It's creating a timer on every received packet, and this causes terrible slowdowns on large data transfers. This patch is the start of a revamp to handle timers much more efficiently, and to call aioquic in its preferred patterns. I've had good results interoperating with a trio-based server with hypercorn with Chrome and curl. I have written the asyncio port, which is just a mirror of the trio port, but I haven't tested it yet.