quic: complete the QUIC/HTTP3 implementation

[jasnell]

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:

• QUIC Server and Client
• HTTP/3 Server and Client (confirmed interop with Chrome's QUIC client)
• Diagnostics Channels and Performance API integration

The implementation is still gated behind the --experimental-quic compile flag.

$ ./configure --ninja --experimental-quic
$ make
$ ./node --experimental-quic

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-quic compilation 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/iter API 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

[nodejs-github-bot]

Review requested:

• @nodejs/gyp
• @nodejs/net
• @nodejs/quic
• @nodejs/security-wg

[Qard]

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]

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]

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.

[pimterry]

This is surprising, and doesn't match the docs which say async write with drain wait. My understanding is it should be adding a pending write to the queue and returning a promise if so, and only throwing if the pending queue is full. There's no await in the whole async function so it never waits at all.

[jasnell]

No, per the stream/iter default, backpressure enforcement is strict. If the buffer is full, additional writes are immediately rejected not queued/buffered. This includes the case where the write would cause the highwatermark to be exceeded if it were accepted.

[pimterry]

Hmm, I understand it's strict, and maybe I've missed something, but the docs and core stream API currently implies any failing write will always have a preceeding promise that you should have awaited to know when to write safely and I don't see how that works here.

https://nodejs.org/api/stream_iter.html#strict-default says:

If you properly await each write, you can only ever have one pending write at a time (yours), so you never hit the pending writes limit.

That's not true in this case, including with the changes just pushed. Even the very first write to a stream can hit the limit & throw instead of returning a promise, as can any future write, even if you exactly follow the example in the strict backpressure example code there and await correctly. Given the defaults I think this applies for any write with a chunk >64KB.

To use this API safely, every call to write() must be preceeded by a writer.desiredSize check that checks the size of each chunk, and then maybe a buffer slice to make your input data fit. If you don't do that, this can throw an exception unpredictably at any time. And then if it hits zero you need to read & wait for drainableProtocol before continuing.

Notably that means you can't ever safely pipe any stream/iter readable to this, since pipeTo etc all expect write to follow the above, and they don't compare desiredSize to individual chunk sizes. If the readable includes a chunk larger than the desired size then pipeTo will still call stream.write(chunk), and that'll throw and reject the whole pipe.

Am I missing something here? I think the key to my confusion here is because highwatermark in stream/iter is entirely about chunk counts, but the limit here is all bytes. Write() is normally safely awaitable because there's a minimum highwater mark of 1 and a single chunk of any size = 1, but that's not true once we start counting bytes. If we just added a 1 (or more) chunk buffer here to always allow one pending write then I think that'd resolve this, but of course that implies size-unbounded buffering.

A quick failing test where pipeTo can't deal with this to repro:

// Flags: --experimental-quic --experimental-stream-iter --no-warnings
import { hasQuic, skip, mustCall } from '../common/index.mjs';
import assert from 'node:assert';

if (!hasQuic) {
  skip('QUIC is not enabled');
}

const { listen, connect } = await import('../common/quic.mjs');
const { from, bytes, pipeTo } = await import('stream/iter');

// A single 100KB chunk exceeds the default 64KB highWaterMark.
const chunk = new Uint8Array(100 * 1024);
chunk.fill(0x42);

const serverDone = Promise.withResolvers();

const serverEndpoint = await listen(mustCall((serverSession) => {
  serverSession.onstream = mustCall(async (stream) => {
    const received = await bytes(stream);
    assert.strictEqual(received.byteLength, chunk.byteLength);
    stream.writer.endSync();
    await stream.closed;
    serverSession.close();
    serverDone.resolve();
  });
}));

const clientSession = await connect(serverEndpoint.address);
await clientSession.opened;

const stream = await clientSession.createBidirectionalStream();

const input = from([chunk]);
await pipeTo(input, stream.writer); // This throws ERR_INVALID_STATE

[jasnell]

Yeah, this is one of the aspects of the strict backpressure handling we need to discuss and refine. The implementation will not allow the high water mark to be exceeded. Even with a single write, since the chunk exceeds the high water mark the strict backpressure handling rejects it following that rule. The backpressure policy in stream/iter writers (like Stream.push) are per chunk but a custom Writer implementation is free to apply other kinds of policies... and in this case it's total bytes. That's because the chunks are flushed out to the underlying DataQueue synchronously and doesn't count individual chunks. I've unresolved this so we can keep discussing further. Essentially this isn't a bug; it's just a design decision we need to make.

[jasnell]

@RafaelGSS:

... 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?

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)

[codecov]

Codecov Report

❌ Patch coverage is 54.08320% with 298 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.59%. Comparing base (b2248fd) to head (babb19d).
⚠️ Report is 16 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/quic/state.js	38.52%	150 Missing ⚠️
lib/internal/blob.js	11.26%	63 Missing ⚠️
lib/internal/quic/stats.js	51.80%	40 Missing ⚠️
src/node_sockaddr.cc	82.05%	11 Missing and 10 partials ⚠️
src/node_blob.cc	33.33%	7 Missing and 5 partials ⚠️
src/node_util.cc	25.00%	6 Missing ⚠️
src/dataqueue/queue.cc	58.33%	4 Missing and 1 partial ⚠️
lib/internal/perf/observe.js	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #62876      +/-   ##
==========================================
- Coverage   89.61%   89.59%   -0.03%     
==========================================
  Files         706      707       +1     
  Lines      219144   221495    +2351     
  Branches    41985    41919      -66     
==========================================
+ Hits       196391   198449    +2058     
- Misses      14629    14946     +317     
+ Partials     8124     8100      -24     

Files with missing lines	Coverage Δ
lib/internal/errors.js	97.64% <100.00%> (+<0.01%)	⬆️
lib/internal/fs/promises.js	92.95% <100.00%> (+<0.01%)	⬆️
lib/internal/quic/diagnostics.js	100.00% <100.00%> (ø)
lib/internal/quic/quic.js	100.00% <ø> (ø)
lib/internal/quic/symbols.js	100.00% <100.00%> (ø)
src/debug_utils.h	80.00% <ø> (ø)
src/node_blob.h	37.50% <ø> (ø)
src/node_file.h	78.43% <ø> (ø)
src/node_sockaddr.h	38.23% <ø> (ø)
lib/internal/perf/observe.js	91.42% <66.66%> (-0.13%)	⬇️
... and 7 more

... and 61 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.