quic: complete the QUIC/HTTP3 implementation#62876
quic: complete the QUIC/HTTP3 implementation#62876jasnell wants to merge 4 commits intonodejs:mainfrom
Conversation
|
Review requested:
|
Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode:Opus 4.6
Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode:Opus 4.6
Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode:Opus 4.6
9e6e4d0 to
01d06b2
Compare
Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode:Opus 4.6
01d06b2 to
bbd0da0
Compare
Qard
left a comment
There was a problem hiding this comment.
Got through a bit of it, but still lots more to review. Seems generally reasonable so far, though I haven't got through all the implementation code yet.
| * `incremental` {boolean} When `true`, data from this stream may be | ||
| interleaved with data from other streams of the same priority level. | ||
| When `false`, the stream should be completed before same-priority peers. | ||
| **Default:** `false`. |
There was a problem hiding this comment.
Is this a return of head-of-line blocking? Is that a requirement of http/3 somewhere? Do we have any mechanisms of mitigating the blocking problems or at least containing them as much as possible?
There was a problem hiding this comment.
https://datatracker.ietf.org/doc/html/rfc9218
The idea is that HOL blocking is actually correct when the client is unable to incrementally process resources of the same priority. This provides flexibility to mark priorities stream-by-stream rather than imposing HOL on everything regardless of priority.
There was a problem hiding this comment.
For http3, the implementation currently relies on nghttp3's default internal prioritization to determine which stream's data to send at any given time. It will require experimentation to determine how well it performs when interacting with node.js applications using streaming data sources.
| * `Promise` — Awaited; the resolved value is used as the body (subject | ||
| to the same type rules, with a nesting depth limit). |
There was a problem hiding this comment.
What nesting is there to limit? Promises would flatten themselves automatically, so is there something else those interact with that could allow more than one level of nesting here? 🤔
This PR is necessarily quite large, and I apologize for that.
This gets the QUIC and HTTP/3 implementations to a functional state with a complete suite of tests and docs.
The PR contains the necessary fixes and improvements in both the C++ and JavaScript layers, along with 214 individual test files.
What's working:
The implementation is still gated behind the
--experimental-quiccompile flag.Which means this can safely land in without impacting CI.
I will need to thoroughly test on all of the Node.js supported platforms so I will be asking the @nodejs/build team if we can get a CI job set up to support an experimental build that sets the
--experimental-quiccompilation flag.How to review:
The PR is divided in to four distinct commits. The first focuses entirely on the C++ layer. If you're familiar with Node.js C++ code, this is where I would start. The bulk of the implementation is here. The organization of the code is straightforward and there are tons of code comments to act as sign posts. The second commits focuses entirely on the JS layer. This essentially acts as a thin layer on top of the C++ bits. The third commit are the doc updates. If you need a high level overview of the module before digging into details, start with this commit. The fourth is all the tests.
In this PR, the tests alone account for 15,831 LOC in 214 files.
The total actual implementation is around 15,831 LOC across C++ and JS (including code comments).
The docs are around 3,179 LOC.
I fully understand that reviewing this is a giant task. Understand that writing it has been a giant task. Had I split this up into a bunch of individual PRs the review process would not have been any easier and would have taken 10 times longer. There's no easy way to get a substystem like this correctly implemented.
I'm happy to jump on calls to walk people through this. I'm happy to explain every part of it as necessary. I'm happy to review potential design changes to the API.
As a side note: the updated implementation makes use of the new experimental
stream/iterAPI as the stream interface.And yes, given the sheer size of this, I used Opencode/Opus to help get things finished. I read and reviewed every LOC modified.
Signed-off-by: James M Snell jasnell@gmail.com
Assisted-by: Opencode:Opus 4.6