[pimterry]

This extracts dynamic application attach from #63995. This is still very much under debate, so I'll try to explain thoroughly:

(EDIT: updated to the connection/session distinction and updated autoStart design)

The visible API changes

• Added QuicConnection: this represents the core QUIC transport state and lifecycle - but not the application protocol communication on top.
• There's now two session classes (QuicSession and Http3Session) which each represent the application protocol on top of the transport, and expose the protocol-relevant stream/datagram APIs for that.
• A session is started on top of a connection. It either starts automatically based on the ALPN negotiation (autoStart: true, the default, same as main today) or it's started manually after setting autoStart: false.
• To manually start a session, you call Http3Session.start(conn) or QuicSession.start(conn). You must do this before the end of the tick when the connection is exposed (the tick when the server onsession callback fires, or the client opened promise resolves). If you don't, the connection is closed automatically.
• The session owns the connection: you can only have one per connection, in both manual & automatic cases it must always be started before any communication happens, and userland can reach the raw connection details via session.connection (but my expectation with the design here is that you mostly don't need to).
• autoStart changes the type of the client connect() call/server onsession callback: if you autoStart, it always returns/passes the started session instance - in practice, this behaves very similar to main. If you set autoStart: false, it always returns/passes a QuicConnection instead.
• alpn is now a required option. Even with autoStart, I really don't think we should default to HTTP/3 in an API that isn't called node:http3. In practice with autoStart connect({ alpn: 'h3' }) is now all you need to specify to get an Http3Session back, and that seems reasonable to me.

Changes

This implement this, this PR splits QuicSession into QuicConnection (lifecycle APIs, TLS & transport fields), an internal QuicSessionBase base class (the common parts of the app session APIs: mirroring some very common lifecycle APIs from the connection, some callback handling, mirroring datagram APIs for now) and QuicSession/Http3Session (inheriting from QuicSessionBase, exposing the protocol-specific APIs).

This doesn't change the C++ internal structure: most notably it doesn't refactor them to match the changed JS API, so C++ still just has Session + Application, which JS exposes as QuicConnection (part of the native session API) + Quic/Http3Session (the rest of the native session API, tightly integrated with the application behaviour). I've left that as is for now since it works and it keeps the diff manageable, but we could clean up to clarify this later.

There are some other smaller C++ changes though to support the actual change: exposing a manual application start API (StartApplication), making various APIs work if no session has been started yet during manual start (meaning no application is present), and shutting down the connection if there's no session started by the deadline (RequireApplication).

There was a complicated optimization here before, which is now gone: deferring manual session start so that it just set a state field from JS, and then the start happened later on when the application was actually required. This avoided the JS->native call, but added lots of complexity. Now that this is just opt-in for specialist use cases I don't think it's worth it - better to skip it & simplify.

Why add dynamic attach manual session start

• It's useful to be able to do raw QUIC with a HTTP/3 ALPN. This lets you test weird behaviours (much like to how our test suite occasionally drives HTTP/2 over raw TCP) or use an alternative HTTP/3 implementation.
• It's useful to be able to choose to do HTTP/3 with a non-HTTP/3 ALPN. For HTTP/2 this has been used quite a bit, e.g. weird internal protocols, gRPC ALPNs, etc. Even today for HTTP/3 it's been used for various HTTP/3 drafts as h3-*. It's perfectly valid to do this - if you agree on h3 via ALPN you should speak HTTP/3, but the inverse is not required.
• Controlling this from JS provides a structure to support splitting the APIs themselves, so we can separate the HTTP/3 & QUIC APIs (soon: QuicStream vs Http3Stream) and make them both easier to use (e.g. no non-functional HTTP/3 methods on pure QUIC streams).
• Dynamic configuration of the layers provides a staging point for any future work towards things like QMUX (HTTP/3 over QUIC-like APIs, not actual QUIC) where we'd need to split "application protocol" from "QUIC implementation" anyway.

Demo

await listen((connection) => {
  // Look at the connection and synchronously decide which session to start:
  if (connection.alpnProtocol === 'my-special-h3-alpn') {
    const h3Session = Http3Session.start(connection);
  } else {
    // Do raw QUIC everywhere else, even with h3 ALPN if you want:
    const rawQuic = QuicSession.start(connection);
  }
}, {
  alpn: ['h3', 'my-special-h3-alpn', 'other'],
  autoStart: false,
  ...
});

const clientConnection = await connect(address, {
  alpn: 'h3',
  autoStart: false
  ...
});

await clientConnection.opened;
// Synchronously start here after `opened` resolves and you have all conn details:
const h3Session = Http3Session.start(clientConnection);
// ... do client HTTP/3 things

In both cases, manually starting the session is possible strictly until the MakeCallback call for this existing event returns - there's no extra tick or delay introduced. If you don't do it by the, the connection is closed automatically and later calls fail.

Performance

Still no measureable impact I can see in either autoStart: true or autoStart: false cases, even despite removing the optimization.

What this PR does not do

There's many things not included here:

• It doesn't rename 100% of 'session' public APIs to 'connection'. I've done all the obvious and easy parts, but I've avoided some unnecessary changes just to keep the diff manageable, and some cases are ambiguous anyway e.g. diagnostic channels (still quic.session.*). Debatable, happy to change more..
• It doesn't change the HTTP/3 settings APIs. There's more to do here, not huge but it's not strictly tied to this and I'd like to try to keep this PR smaller if I can.
• It doesn't change the HTTP/3 streams. Bigger change, definitely interesting but leaving it for now.
• It doesn't refactor the C++ internals. There's some cleanup & clarification possible here to match this I think, and some improvements I'm interested in making to Application generally, but it's large and will take more debate - better to defer.
• Various other pre-existing small bugs & details I've run into - there are more things improve, but for basically any pre-existing issue I'd like to handle it separately so we can get this in first and work in parallel instead.

[nodejs-github-bot]

Review requested:

• @nodejs/performance
• @nodejs/quic
• @nodejs/startup

[jasnell]

Ok, so it's good to get this isolated. Trust me, I'm not trying to be difficult! :-) ... Before going through the code let's see if we can settle on this. And yes! I can be convinced but it might take some doing.

Let's take your points:

It's useful to be able to do raw QUIC with a HTTP/3 ALPN. This lets you test weird behaviours (much like to how our test suite occasionally drives HTTP/2 over raw TCP) or use an alternative HTTP/3 implementation.

Useful in our tests does not mean useful in the general API That We Have To Support Indefinitely sense. Also, HTTP/2 is easier to do over raw TCP since it's just framing with no TLS, flow control, retransmission framing, etc.

That said, we can do raw QUIC with a HTTP/3 ALPN without this kind of architectural change. It could be as simple as a boolean configuration option that says "use the default application with h3" in which case we simply don't install the Http3Application.

Essentially, this point alone does not justify the re-architecture.

It's useful to be able to choose to do HTTP/3 with a non-HTTP/3 ALPN. For HTTP/2 this has been used quite a bit, e.g. weird internal protocols, gRPC ALPNs, etc. Even today for HTTP/3 it's been used for various HTTP/3 drafts as h3-*. It's perfectly valid to do this - if you agree on h3 via ALPN you should speak HTTP/3, but the inverse is not required.

This is also something that could just be a config option. "Treat ALPN {FOO} like H3" in which case we install the Http3Application.

Or if someone really wanted to implement all the H3 semantics themselves, see point one above... it could be as simple as a boolean configuration option. Still not a strong motivation for the re-architecture.

Controlling this from JS provides a structure to support splitting the APIs themselves, so we can separate the HTTP/3 & QUIC APIs (soon: QuicStream vs Http3Stream) and make them both easier to use (e.g. no non-functional HTTP/3 methods on pure QUIC streams).
Dynamic attachment of the layers provides a staging point for any future work towards things like QMUX (HTTP/3 over QUIC-like APIs, not actual QUIC) where we'd need to split "application protocol" from "QUIC implementation" anyway.

We can split QuicStream and Http3Stream purely at the JS level without any more C++ side redesign AND make it possible for applications that want to just handle raw QUIC but implement their own HTTP3 semantics to use Http3Stream without going with the dynamic attach path.

I'm fine with splitting things at the JS level. You'll not get much argument from me there, but I strongly prefer the existing DefaultApplication/Http3Application/QuicSession split. Sure, there are details and nits that could be refined but I think the architecture works well. I want to avoid re-architecting for the sake of re-architecting when what is there still meets the goal.

So my ask would be this: please take a moment to explain why the current architecture needs to be changed to meet the goals. What goals simply cannot be met with the current architecture or why does it make it harder? etc. What I'm not saying is that your suggested approach is wrong, not by any means... I just think there's more than one correct approach to achieve the goal and I haven't yet seen why the current approach isn't workable.

[codecov]

Codecov Report

❌ Patch coverage is 89.79310% with 74 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.67%. Comparing base (933cb11) to head (8977eb9).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/quic/http3.js	57.95%	74 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #65993      +/-   ##
==========================================
- Coverage   90.69%   90.67%   -0.02%     
==========================================
  Files         801      802       +1     
  Lines      281875   282179     +304     
  Branches    54317    54322       +5     
==========================================
+ Hits       255652   255879     +227     
- Misses      16348    16417      +69     
- Partials     9875     9883       +8     

Files with missing lines	Coverage Δ
lib/internal/quic/quic.js	100.00% <100.00%> (ø)
lib/internal/quic/state.js	100.00% <100.00%> (ø)
lib/internal/quic/stats.js	100.00% <100.00%> (ø)
lib/internal/validators.js	97.94% <100.00%> (+0.07%)	⬆️
src/node_builtins.cc	77.66% <ø> (ø)
lib/internal/quic/http3.js	57.95% <57.95%> (ø)

... and 32 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.

[pimterry]

I'm fine with splitting things at the JS level. You'll not get much argument from me there, but I strongly prefer the existing DefaultApplication/Http3Application/QuicSession split. Sure, there are details and nits that could be refined but I think the architecture works well. I want to avoid re-architecting for the sake of re-architecting when what is there still meets the goal.

So my ask would be this: please take a moment to explain why the current architecture needs to be changed to meet the goals. What goals simply cannot be met with the current architecture or why does it make it harder?

Starting here because I think it's the core point.

I think you're mostly referring to the previous PR here. In this PR, there's no real architectural change. It doesn't change the overall Application structure. The only code change in application.h is:

static constexpr uint8_t kTypePending = 0x80

The changes in C++ here are effectively moving the timing around (moving the code that triggers Application attach from earliest possible moment to first-use) and changing the details of that attach step internally (reading a flag and optionally SETTINGS config stored by JS, instead of just reading ALPN).

There are architectural changes I think are worth exploring later, but this PR doesn't do any of that.

Useful in our tests does not mean useful in the general API That We Have To Support Indefinitely sense.

Our tests are one example close to home, but there's plenty of other ecosystem test suites that do the same. Undici has a nice one here: https://github.com/nodejs/undici/blob/main/test/http2-goaway-refusal-storm.js.

That's a security bug fix, which does this specifically because it requires invalid HTTP/2 behaviour we don't support. This will apply similarly for roughly any repro of a security bug involving a badly behaved HTTP/3 peer, and any test covering those cases. Supporting this is useful.

HTTP/2 is easier to do over raw TCP since it's just framing with no TLS, flow control, retransmission framing, etc.

TLS, flow control & retransmission are all QUIC, so would still be handled by Node even in the "custom HTTP/3 implementation" case.

Some of this is actually easier than HTTP/2 - for H2 you do need flow control on top of TCP.

In practice I think it's a wash. Implementing HTTP/3 alone, you get to skip flow control and you can disable QPACK by sending capacity 0 (or alternatively, I built a working QPACK implementation in pure JS 😆 - works, but very slowly), but framing & encoding is more complicated.

Not suggesting this is a good idea for anything serious, it is very hard to do 100% correctly or make it fast, but it's very achievable for test cases.

it could be as simple as a boolean configuration option

In short, I think:

• This is hard to design well to cover the real-world cases.
• Even in the best case, it's strictly more limiting than arbitrary JS.
• In reality, this won't be much simpler.
• That won't be measurably faster in any case that matters.

In the implementation here, the runtime flow for the dynamic attach specifically is setting one flag in the shared state set from JS, and then doing one check against that in C++ on first use.

There's other changes to split logic into Http3Session, pass across (optionally) any custom SETTINGS configuration, etc, but the actual attach flow is just writing a flag and reading it later (instead of reading ALPN).

For an alternative design, for any non-trivial use cases, you need at least a mapping of ALPN to implementation (this is basically what we have today, but non-configurable) and probably an SNI + ALPN mapping, so this can vary by host. Something like:

listen(() => { ... }, {
  sessionTypeMapping: {
    'raw.example.com': {
      '*': 'quic'
    },
    'h3.example.com': {
      'h3': 'h3',
      'funky-h3-alpn': 'h3'
    }
  }
})

Implementing that mapping logic is going to be more complex than the single state flag set & read.

In effect this PR outsources "map connections to implementations" logic to user JS. If we want to take responsibility for that, we end up pulling more logic into core, not less.

Making a static API like the above would make the mapping faster (C++ checking of ALPN & SNI will be faster than JS doing it). This is only relevant to the unusual cases that do need this though (test suites, CVE repros, networking tools) who generally aren't very perf sensitive and would (imo) prefer API flexibility over a small speed boosts. It doesn't make the very common cases that do care about performance (always use Http3/always use QUIC) any faster.

More generally, we do support the "control it from JS" approach in basically every other API:

• When implementing ALPNCallback, we initially had a boolean flag and then went for the callback approach instead specifically because full control over ALPN behaviour is more flexible and the performance difference doesn't matter in cases like these.
• For HTTP/2, I even found a helpful comment where you pitched a dynamic attach API for HTTP/2 that basically matches the API here! Take a socket and wrap it with HTTP/2: We "need" a way to create an Http2Session from a socket #16256 (comment). That general idea was eventually implemented and we support it today for both client and server HTTP/2. This is roughly the HTTP/3 equivalent.

[jasnell]

Wonder if we should go ahead and just call this request?

[pimterry]

Yes, definitely, but in a later PR.

I'm trying to break up the work, and one of the next steps (see #63995 (comment) for the outline) is changing the stream side to Http3Stream, and I was going to touch the stream APIs like this as part of that. I think the current API is a reasonable intermediate state and it'll make more sense in that later PR, so I'd rather defer this.

[martenrichter]

No, for WebTransport it is not an request, but a bidirectional stream. But may be an alias request for it just for the http3 interface.

[jasnell]

This begs the question about whether we should consider a Http3ServerSession and Http3ClientSession split where this method just doesn't exist on the server. Given the idea that this is a low-level toolkit API for library authors, that might be going too far tho. Non-blocking.

[pimterry]

I think this could be quite useful, but I think it'll be easier to review separately in the H3 streams PR. The main (only?) real difference between H3 servers & clients is how they handle streams: clients don't need a stream callback at all, and servers don't need to open streams.

To make this possible though, and because it's a nicer API, I've changed the dynamic attach API from new Http3Session(quicSession) to Http3Session.from(quicSession) which gives us a bit more leeway for interesting games like this later where we return different types. Probably a better match for things we might do elsewhere for WebTransport too.

[jasnell]

Should we call this onrequest?

[jasnell]

And should it be Http3Stream?

[pimterry]

Same as for request(): I'd prefer to defer this to the stream work.

[jasnell]

It's not immediately obvious why this is removed here.

[pimterry]

Fair point. The original motivation was that in a previous design, EnsureApplication() was called for streams but not datagrams (as applications aren't involved in sending datagrams, for now) which meant you could get a 0RTT rejection with no application, which would break this behaviour as it's defined inside the application.

To fix that, I moved the logic into Session (session.cc:1647) so it doesn't need an application at all to fire this. It was duplicated in both two applications, and it's just generic QUIC logic: if early data is rejected, we should fire the session event. Doesn't matter which application is installed.

Then, before I opened the PR, I added EnsureApplication() to sending datagrams as well to match streaming. Even if not strictly necessary, it seems more consistent and prepares for H3 datagram support later, and I can't imagine a case where you want to start sending datagrams on a session before you know what protocol it's using.

That means this change isn't strictly required any more - the motivating bug no longer applies. Could revert it, but tbh I think it's an improvement anyway - it's just a couple of lines and deduplicates code from both applications that should really live in the Session imo. Can revert instead if you disagree.

[jasnell]

We should add a TODO in here to see if we can generalize this a bit more for when/if we have additional built-in applications beyond http3. Not critical to do now tho.

[pimterry]

Definitely agree there's more to do here. There's a later step in the plan to clean up settings API, aiming to migrate those into the HTTP/3 session API so this goes away. It's very awkward today as a generic API (hence the per-type if we need here) because it's fundamentally not something that's defined in QUIC at all.

Initially I did that inline in this PR but it's a bit fiddly, so I've avoided making settings API changes here (it changes settings internals, but just enough to the minimum required to make the session creation API functional) and we can work on h3 settings in parallel with the h3 streams API, once we have the session structure pinned down.

[martenrichter]

I am finished with the review.
Most comments are cosmetic; some require attention, but once this is done, it looks good...

[pimterry]

Thank @martenrichter! I've answered all the ones where I have answers, most of the rest are indeed things to fix, I'll do that shortly.

Yes, sorry for all the delay here, between the security carnage this week and a conference I had to prep for I've been off QUIC all week, but I'm on a plane back to normality as we speak (hopefully) so I'll resolve the other points & rebase this this week.

[pimterry]

I think I've handled all the outstanding comments here, and this is now rebased on top of the latest changes and ready to review again.

I've resolved every comment that's either handled or now irrelevant (due to the connection+session redesign) so the open comments mostly cover work I think we should punt to a future PR, and the open session/connection naming question.

There may be other small bits, but hopefully we've resolved most of the practical details. I think the main remaining question is whether @jasnell (and everybody else) is happy with the overall API design of connection + h3/quic session + autoStart.

PR description is up to date if you want a fresh summary of everything.