quic: complete the QUIC/HTTP3 implementation#62876
quic: complete the QUIC/HTTP3 implementation#62876jasnell wants to merge 7 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.
RafaelGSS
left a comment
There was a problem hiding this comment.
Could you share a bit more about your expected transition to a runtime flag instead of a build-time flag? Do you plan to use it on any product on Cloudflare? If not, why not have it on a runtime flag only?
I'm asking this because it's unlikely that people would build Node.js from scratch to have this feature enabled, and even after some months of this feature on main, we won't have feedback about its bugs.
One good approach that comes to mind when talking about build-time features would be evaluating it's security and spec compliance with some tools before moving it to runtime-flag only. Is it your plan?
pimterry
left a comment
There was a problem hiding this comment.
I'm trying to break this up into lots of reviews, I'm thinking to aim for vertical slices, exploring functionality in isolation so it fits in my head. Going to be difficult to do the lot quickly but I'll try to keep steadily grinding through it 😄
Starting here reading through quic.js, and streams & writer backpressure especially, there's a good set of low-level issues here to start with.
The compile time flag is temporary. It was added back only because we were receiving vuln reports on the unfinished implementation. Once this is deemed ready to go and we're sure that it's working on all of the Node.js supported platforms the compile flag will be removed and it'll just be the experimental runtime flag required until this is ready to graduate to supported (which is likely to take some time given the overall complexity) |
27c512d to
0281f3f
Compare
0281f3f to
350bda3
Compare
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