quic: more implementation

[jasnell]

There's a lot more to do to get things fully working but this makes more incremental progress.

Several key bits:

• This adds the --experimental-quic CLI flag and the node:quic module behind it.
• This also starts to add the quic.md documentation
• Look at test-quic-handshake for a basic idea of the current state of the API. It's still rough but it's enough to start getting feedback.

Key question for reviewers: Looking at the API documented in quic.md and demonstrated in test-quic-handshake, what API do you want to see here? This is focusing ONLY on the QUIC API at this point and does NOT touch on the http3 API.

For the server-side, the API is essentially:

const { QuicEndpoint } = require('node:quic');
const keys = getKeysSomehow();
const certs = getCertsSomehow();
const endpoint = new QuicEndpoint();
endpoint.listen((session) => {
  session.onstream = (stream) => {
    stream.setOutbound(Buffer.from('hello world'));
    stream.closed.then(() => {
      session.close();
    });
    readStreamSomehow(stream);
  });
}, { keys, certs });

For the client side it is essentially:

const { QuicEndpoint } = require('node:quic');
const endpoint = new QuicEndpoint();
const session = endpoint.connect('123.123.123.123:8888');
const stream = session.openUnidirectionalStream({
  body: Buffer.from('hello there');
});
await stream.closed;
endpoint.close();
session.close();

The API still feels rather awkward. For review, contrast with what Deno is doing here: https://github.com/denoland/deno/pull/21942/files#diff-1645ba9a2a2aac4440671da62b81ab9a723f50faffff0d0e8d016a1f991961a3

At this point there are bugs in the implementation still, yes, but let's focus on the API design at the moment. Please focus review comments on that.

A couple of bits to consider for the reivew...

In this API design, the QuicEndpoint represents the local binding the local UDP port. It supports being both a server and client at the same time. Use can be a bit awkward though so we need to carefully review. Specifically, a single QuicEndpoint can support any number of QuicSessions (both client and server at the same time). This gives maximum flexibility but means a more complicated API. For example, let's take a closer look at the client side example:

const endpoint = new QuicEndpoint();
const session = endpoint.connect('123.123.123.123:8888');

// We're opening the stream here before the session handshake has had a chance to start.
// The actual underlying QUIC stream will be opened as soon as the TLS handshake completes.
// This is good in that it allows us to optimistically provide data to the pending stream so that
// the underlying implementation can start sending data as early and as quickly as possible.
session.openUnidirectionalStream({ body: Buffer.from('hello there'); });

// However, if I close the session or endpoint here, then the pending stream will be canceled.
// That is, while the `open...Stream(...)` variations have a pending effect, close() and destroy()
// do not.

// `session.close()` is defined as a graceful close. In other words, it should destroy the session
// only after all open streams are closed. Should pending streams count as opened streams?
// Should close() wait to close the session only when the pending stream has been opened
// then closed? I think the answer is yes but let's confirm.

await Promise.all([endpoint.close(), session.close()]);

The key question here is: does it make sense to expose QuicEndpoint like this? If we look at the Deno API, as an alternative, we see something like:

const conn = await Deno.connectQuic({
      hostname: "localhost",
      port: listener.addr.port,
      alpnProtocols: ["deno-test"],
    });
const stream = await conn.createUnidirectionalStream();
const writer = stream.writable.getWriter();
const enc = new TextEncoder();
await writer.write(enc.encode('hello there'));
await conn.close({ closeCode: 0, reason: '' });

In Deno's API, they've effectively hidden QuicEndpoint under their notion of QuicConn. While it might be the case that the same underlying UDP port is used when ever connectQuic(...) is called, if it is that ends up being an internal implementation detail and not something that is exposed to users. Which approach is better? Exposing QuicEndpoint the way that I am means we have more flexibility but also more complexity that may not be worth it?

Alternatively, we could go with a simplified API where every call to connect(...) uses a separate QuicEndpoint, so, for instance:

import { connect } from 'node:quic';

const client = connect('123.123.123.123:8888');
client.openUnidirectionalStream({ body: Buffer.from('hello there'); });
await client.close();

We could still allow for direct reuse of QuicEndpoint in this model by allowing a QuicEndpoint instance to be passed as an option to connect(...), like:

import { connect, QuicEndpoint } from 'node:quic';

const endpoint = new QuicEndpoint();

const conn1 = connect('123.123.123.123:1234', { endpoint });
const conn2 = connect('123.123.123.123:4321', { endpoint });

await Promise.all([ conn1.close(), conn2.close() ]);

// notice I'm not closing the endpoint here...  The sessions will have been closed/destroyed...
// should the endpoint be closed here? If I don't close it, should the node.js process hang because
// the endpoint is still actively bound to the local udp port? 

But the question here, whenever client connections are sharing a single endpoint is (a) when should the endpoint be closed, (b) should it ever be closed automatically?, (c) should it be unref'd so that if there are no active sessions but the client-side endpoint has not yet been closed, should it keep the node.js process from exiting?

Basically, looking for feedback on the API design so if you have other ideas for what color to paint the bikeshed, now is the time to discuss it.

@Qard @mcollina @anonrig @nodejs/quic

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/http2
• @nodejs/loaders
• @nodejs/net
• @nodejs/startup

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/64140/

[codecov]

Codecov Report

Attention: Patch coverage is 65.18375% with 540 lines in your changes missing coverage. Please review.

Project coverage is 88.67%. Comparing base (ca69d0a) to head (2570ff3).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/quic/http3.cc	7.23%	217 Missing and 1 partial ⚠️
src/quic/streams.cc	43.39%	89 Missing and 31 partials ⚠️
src/quic/application.cc	39.06%	63 Missing and 15 partials ⚠️
lib/internal/quic/state.js	59.00%	41 Missing ⚠️
src/quic/endpoint.cc	55.42%	28 Missing and 9 partials ⚠️
src/quic/packet.cc	37.50%	17 Missing and 8 partials ⚠️
src/quic/transportparams.cc	54.54%	4 Missing and 1 partial ⚠️
src/quic/data.cc	33.33%	2 Missing and 2 partials ⚠️
src/node_http_common-inl.h	0.00%	2 Missing ⚠️
src/quic/application.h	50.00%	2 Missing ⚠️
... and 5 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56328      +/-   ##
==========================================
+ Coverage   88.54%   88.67%   +0.13%     
==========================================
  Files         657      659       +2     
  Lines      190393   191099     +706     
  Branches    36552    36475      -77     
==========================================
+ Hits       168582   169457     +875     
+ Misses      14998    14452     -546     
- Partials     6813     7190     +377     

Files with missing lines	Coverage Δ
lib/internal/bootstrap/realm.js	95.41% <100.00%> (-0.62%)	⬇️
lib/internal/process/pre_execution.js	90.39% <100.00%> (-0.31%)	⬇️
lib/internal/quic/quic.js	100.00% <100.00%> (ø)
lib/internal/quic/stats.js	96.00% <100.00%> (+0.95%)	⬆️
lib/internal/quic/symbols.js	100.00% <100.00%> (ø)
lib/quic.js	100.00% <100.00%> (ø)
src/node_builtins.cc	78.48% <100.00%> (-0.19%)	⬇️
src/node_options.cc	88.03% <100.00%> (+0.01%)	⬆️
src/node_options.h	98.31% <100.00%> (+<0.01%)	⬆️
src/quic/bindingdata.h	33.33% <ø> (ø)
... and 25 more

... and 108 files with indirect coverage changes

[pimterry]

This is very exciting! Nice work @jasnell, it's great to see the progress here. Some thoughts:

It feels like this API is sort-of upside-down, compared to others we have. Elsewhere (dgram and net + derivatives) you create a client/session, and you then specify the local binding (e,g. socket.connect({ ... localPort ... }) or dgramSocket.bind(...)) via various parameters in setup. Here you create a representation of the local endpoint first, and then you build the client/server from that. I understand it makes sense from an internal implementation POV and for advanced cases, but it doesn't feel consistent with other APIs or the mental model that (imo) most users will have.

There's also no representation of a 'server' in this API. There's only the endpoint itself (not quite the same). This means there's no good place to hang APIs to manage the state tied to the listening process - e.g. there seems to be no 'stop listening' API right now other than closing the endpoint completely (which also kills client connections), and there's no way to independently listen to any server events other than the initially registered callback. I suspect as this API develops, not having a server representation in the model will become inconvenient.

I think initially users will reasonably expect something with roughly the shape & semantics of the simple example above:

import { connect } from 'node:quic';

const client = connect('123.123.123.123:8888'); // <-- Implicit create an endpoint
client.openUnidirectionalStream({ body: Buffer.from('hello there'); });
await client.close();

And I also think something roughly equivalent applies on the server-side too:

import { createServer } from 'node:quic';

const keys = getKeysSomehow();
const certs = getCertsSomehow();

const server = createServer({ keys, certs }, (session) => {
  session.onstream = (stream) => {
    stream.setOutbound(Buffer.from('hello world'));
    stream.closed.then(() => {
      session.close();
    });
    readStreamSomehow(stream);
  };
});

await server.listen('123.123.123.123:8888'); // <-- Implicit create an endpoint
// ...
await server.close();

Supporting advanced use cases too is good but I really think a lot of people are going to come to this expecting that kind of API to be available, and making that possible will be hugely helpful.

That's not to say that the endpoint model doesn't add something, it's just that AFAICT for the most common use cases it'll be slightly confusing extra complexity initially. Do we have any good examples of when & how direct endpoint control will be useful for users? How common is the use case where sharing one endpoint between multiple client connections and/or a server is important?

For cases that do need endpoint control, I'd suggest exposing that optionally but flipping the API, so that an endpoint becomes a parameter of clients/servers rather than being the source of them (similar to the last example in the description). That means for advanced use cases, this would look (very roughly) like:

import { QuicEndpoint, connect, createServer } from 'node:quic';

const endpoint = new QuicEndpoint();

const client = connect('123.123.123.123:8888', { endpoint });
client.openUnidirectionalStream({ body: Buffer.from('hello there'); });
await client.close();

const server = createServer();
server.listen('123.123.123.123:8888', { endpoint });

The internal endpoint model remains the same I think, this is intended mostly as a shift in how that's exposed and where the methods go, with implicit endpoint creation whenever it's not specified.

Regarding closing endpoints, I think in the simple case users definitely won't expect to close this themselves (because the endpoint is invisible) so tidy auto-closing is unavoidable. Even in the advanced case, I can see reasons to want both auto or manual endpoint closing. How about:

• We add an autoClose boolean for endpoints. If set, when the last session and server (i.e. listen binding) is closed, so the Endpoint becomes unused, close it automatically.
• Set autoClose to true when implicitly creating an endpoint in connect/listen, but default to false when creating a QuicEndpoint manually.