Conversation
|
Review requested:
|
nodejs-github-bot
added
lib / src
da14bdc to
77cdc45
Compare
|
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:
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.
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.
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 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 Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #65993 +/- ##
==========================================
- Coverage 90.28% 90.25% -0.04%
==========================================
Files 790 791 +1
Lines 271642 271983 +341
Branches 51835 51847 +12
==========================================
+ Hits 245265 245491 +226
- Misses 16885 16989 +104
- Partials 9492 9503 +11
🚀 New features to boost your workflow:
|
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 static constexpr uint8_t kTypePending = 0x80The 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.
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.
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.
In short, I think:
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:
|
26811a1 to
f5437de
Compare
Previously, ALPN decided automatically which application protocol implementation was used. QuicSession was used everywhere, but its actual behaviour and API changes implicitly based on the wire traffic involved. Now, QuicSession is used for pure QUIC only, and Http3Session is used for HTTP/3 sessions only. To do HTTP/3 on a connection, you enable it explicitly by wrapping a QUIC session in Http3Session. Doing so attaches the internal protocol application handling so everything is HTTP/3 on that session from that point onwards. For now, this only changes the application selection process and the top-level APIs involved, but none of the details. In a future PR, we can introduce Http3Stream and migrate other HTTP/3 specific functionality (e.g. SETTINGS & GOAWAY handling) out of the QUIC API. Signed-off-by: Tim Perry <pimterry@gmail.com>
f5437de to
d5c2d52
Compare
| const client = await connect('123.123.123.123:8888', { | ||
| alpn: 'h3', | ||
| endpoint, | ||
| }); |
There was a problem hiding this comment.
As discussed in the call, let's have an autoWrap option.
autoWrap: true|false would essentially be "Treat h3 (and other known h3-* alpns as HTTP3 and automatically wrap the session as Http3Session"... The server-side callback and the client side connect would return Http3Session rather than QuicSession already wrapped.
If/when additional types of applications are built in, it would mean wrapping in whatever *Session is specific to the known alpn for that application.
| `closed`, `destroy()`, `destroyed`, `ephemeralKeyInfo`, `onerror`, `opened`, | ||
| `peerCertificate`, `servername`, and `stats`. | ||
|
|
||
| ### `http3session.createBidirectionalStream([options])` |
There was a problem hiding this comment.
Wonder if we should go ahead and just call this request?
| [`session.createBidirectionalStream()`][] on the underlying session. | ||
|
|
||
| HTTP/3 has no server-initiated request streams, so calling this on a server | ||
| session throws `ERR_INVALID_STATE`. |
There was a problem hiding this comment.
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.
| * Type: {Function} | ||
|
|
||
| Called with each request stream the peer opens, as a {quic.QuicStream}. See | ||
| [`session.onstream`][]. |
|
|
||
| * Type: {quic.QuicSession} | ||
|
|
||
| The QUIC session on which this HTTP/3 session is running. |
There was a problem hiding this comment.
If we're forwarding the accessors anyway, it might be better not to expose this.
| const out = { __proto__: null }; | ||
| for (let n = 0; n < kNumericSettings.length; n++) { | ||
| const name = kNumericSettings[n]; | ||
| const value = settings[name]; |
There was a problem hiding this comment.
I'd prefer if we extracted the values all at once first, then validated them. Each settings[name] can invoke javascript if the [name] happens to be a getter. That would mean that a value that is validated in one iteration can be modified in the next iteration, leading to TOCTOU issues.
class MaliciousSettings {
#a = 1;
#b = 2;
get a() { return this.#a; }
get b() { this.#a = 2; return this.#b; }
};
const m = new MaliciousSettings();
console.log(m.a); // 1
console.log(m.b); // 2
console.log(m.a); // 2!
|
|
||
| // Validate settings and return them as a plain null-proto object: | ||
| function prepareH3Settings(settings) { | ||
| const out = { __proto__: null }; |
There was a problem hiding this comment.
Building up the object this way means out will change shape with every setting, causing a perf hit. We know what fields we are exporting, it's better to extract the known fields from settings up front, validate each, then return a literal, e.g.
const { a, b } = settings; // destructure
validateA(a);
validateB(b);
return { a, b }; // return a consistently shaped result
| * @param {Function} [options.onsettings] | ||
| */ | ||
| constructor(session, options = kEmptyObject) { | ||
| if (!(session instanceof QuicSession)) { |
There was a problem hiding this comment.
We should use an isQuicSession brand check and avoid using the slower instanceof
| this.#session = session; | ||
| if (ongoaway !== undefined) this.ongoaway = ongoaway; | ||
| if (onorigin !== undefined) this.onorigin = onorigin; | ||
| if (onsettings !== undefined) this.onsettings = onsettings; |
There was a problem hiding this comment.
Since this.on... accessors can be user-patched, let's just set the private fields directly here rather than going through the public accessors.
| get [kSessionHandle]() { | ||
| assertIsQuicSession(this); | ||
| return this.#handle; | ||
| } |
There was a problem hiding this comment.
Rather than symbol properties for internals, I've been leaning more towards static reach ins... e.g.
let getSessionHandle;
class Session {
#handle;
static {
getSessionHandle = (session) => {
assertIsQuicSession(session);
return session.#handle;
};
}
}Then you can extract the handle via getSessionHandle(session)
This prevents the kSessionHandle symbol from bleeding out to users via reflection APIs.
| 'application attached. Create streams through the application ' + | ||
| 'interface (e.g. Http3Session) instead'); | ||
| } | ||
| } |
There was a problem hiding this comment.
This feels... awkward... and is one of the reasons I'm still not entirely happy with the Http3Session wrapper approach. With Http3Application we can let this fall through to the attached application to handle.
| } | ||
|
|
||
| // True once an application has actually been attached (not just requested) | ||
| get #hasApplication() { |
There was a problem hiding this comment.
Nit: The SessionState class is not exposed to users. Making this private probably isn't necessary.
| QuicError::ForApplication(GetInternalErrorCode())); | ||
| if (!session().is_destroyed()) { | ||
| session().EmitEarlyDataRejected(); | ||
| } |
There was a problem hiding this comment.
It's not immediately obvious why this is removed here.
| // application attach later, SetApplication clears it by writing the bare | ||
| // Type. Keeping type & pending/installed in one place avoids duplicating | ||
| // type definition and simplifies application checks. | ||
| static constexpr uint8_t kTypePending = 0x80; |
There was a problem hiding this comment.
What's the functional difference between Type::NONE and kTypePending? We could just rename NONE to PENDING
| // Not installed yet. If an attach has been scheduled, its settings are | ||
| // already known and can be reported before the install happens. | ||
| if ((session->application_type() & ~Application::kTypePending) != | ||
| static_cast<uint8_t>(Application::Type::HTTP3)) { |
There was a problem hiding this comment.
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.
| #include <node_sockaddr.h> | ||
| #include <timer_wrap.h> | ||
| #include <util.h> | ||
| #include <memory> |
There was a problem hiding this comment.
Nit: shouldn't be necessary to add here.
| const Config& config() const; | ||
| const Options& options() const; | ||
|
|
||
| uint8_t application_type() const; |
There was a problem hiding this comment.
Better to return the enum here.
| } | ||
| } | ||
|
|
||
| class Http3Session { |
There was a problem hiding this comment.
Http3Session likely should impl inspect.custom and Symbol.asyncDispose
| Each of the following behaves exactly as the member of the same name on the | ||
| underlying [`QuicSession`][]: `alpnProtocol`, `certificate`, `close()`, | ||
| `closed`, `destroy()`, `destroyed`, `ephemeralKeyInfo`, `onerror`, `opened`, | ||
| `peerCertificate`, `servername`, and `stats`. |
There was a problem hiding this comment.
The this in onerror .. is that going to be QuicSession or Http3Session ... need to be clear about it here and in the other callbacks.
| In practice this means a server session should be attached synchronously | ||
| inside the [`quic.listen()`][] callback, and a client session should be | ||
| attached synchronously when the [`session.opened`][] promise resolves (or | ||
| before), and in both cases before anything is sent on the session. |
There was a problem hiding this comment.
We say should... but this is really a must, isn't it? For proper handling, the application needs to be installed during the initial bootstrap. It really cannot be done lazily as it may miss or mishandle things like the opening http3 control unidirectional streams.
| added: REPLACEME | ||
| --> | ||
|
|
||
| * Type: {Function} |
There was a problem hiding this comment.
Already existing todo... we need to document the signatures.
|
|
||
| The HTTP/3 settings in effect, including any update received from the peer's | ||
| SETTINGS frame, which may arrive after the session opens. `null` once the | ||
| session is destroyed. |
There was a problem hiding this comment.
Need to indicate if this is read-only or not. If mutable, what effect does it have changing it after the session is created.
Also, should it be quic.Http3Options specifically since it's Http3Session?
| const endpoint = await listen((quicSession) => { | ||
| // Attaching HTTP/3 has to happen here, synchronously, before the | ||
| // callback returns. | ||
| const session = new Http3Session(quicSession); |
There was a problem hiding this comment.
What happens if I do...
quicSession.onstream = (stream) => { ... };
const session = new Http3Session(quicSession);
session.onstream = (stream) => { ... };Or attach onerror handlers to both the quicSession and the session?
We need to more clearly define what the effect of wrapping actually is here and whether wrapping supersedes anything installed before wrapping.
| // This method is called at any point where we need an application to be | ||
| // attached. It checks whether JS has requested a specific implementation, | ||
| // and either installs that, or the default (raw QUIC) application. | ||
| bool Session::EnsureApplication() { |
There was a problem hiding this comment.
If I'm reading things correctly, a server can resume the handshake with no application installed, which will trigger an assert when the handshake continues.
| ]; | ||
|
|
||
| // Validate settings and return them as a plain null-proto object: | ||
| function prepareH3Settings(settings) { |
There was a problem hiding this comment.
Not sure if this is adequately tested.
| createBidirectionalStream(options) { | ||
| if (getQuicSessionState(this.#session).isServer) { | ||
| throw new ERR_INVALID_STATE( | ||
| 'Server sessions cannot open HTTP/3 request streams'); |
There was a problem hiding this comment.
This should return a rejected promise not throw synchronously
|
|
||
| The QUIC session on which this HTTP/3 session is running. | ||
|
|
||
| ### `http3session.settings` |
There was a problem hiding this comment.
This is an existing issue, but this is mixing local and peer settings
|
That's all for my first pass. I'll do a second pass (and have an AI agent do a pass) once these are addressed. |
3 participants
This extracts dynamic application attach from #63995. This is still very much under debate, so I'll try to explain thoroughly:
Terminology
Sessionmakes calls into its Application any time it needs anything protocol-specific.Current state
Right now, we have fixed rules that lock which Application is used according to the ALPN negotation on the socket. We use ALPN to make this decision and attach the Application at the earliest possible moment.
For clients that means in the Session constructor (clients currently only support one fixed ALPN) and for servers it means in
OnClientHello, which is the moment we know which ALPN protocol we'll select.Why add dynamic attach
h3-*. It's perfectly valid to do this - if you agree onh3via ALPN you should speak HTTP/3, but the inverse is not required.How this works
This change drops the fixed Application selection & default ALPN configuration, and provides a new
Http3SessionAPI which lets JS control which Application is used directly. You wrap aQuicSessionin anHttp3Sessionand open your streams through the latter if you want to speak HTTP/3.It does this without changing the fundamental steps involved, just by reordering the work within. There's no additional callbacks, and no extra boundary crossing anywhere. Almost everything here is deferring work we're already doing (by moving application selection to the last minute) and offering JS APIs to preconfigure the attach logic.
What happens is:
session.opened).new Http3Session(quicSession)to use HTTP/3. This class:Sessionreads later when it needs an Application:session->EnsureApplication(). If no application has been attached yet, that checksstate.applicationType. If there's a pending HTTP/3 flag, it attaches the HTTP/3 application. If not, it attaches the default raw QUIC application. Attachment itself is the same as before, it just happens later, and checks the state flags instead of ALPN.The actual timing of the calls to
EnsureApplication()are what make this practical: it runs immediately afterMakeCallbackfires thelisten(cb)callback for server sessions, or afterclient.openedresolves. That guarantees a synchronous+microtasks window where the handshake information is available to JS but the application hasn't attached yet. So you can do this:This is possible strictly until the MakeCallback call for this existing event returns - there's no extra tick or delay introduced.
Performance
Benchmarks on my machine show no measurable impact on either the handshake (raw or H3) or H3 request (1RTT or 0RTT) benchmarks. Varying positive/negative on different runs, but always <0.5% and no significant results.
As noted above, this doesn't change when or how we cross between JS & C++. It primarily changes when the Application attach is done, not how. The implementation isn't literally free, but doesn't do anything substantial and I don't see any real-world impact in testing.
Other related changes
node:tls), so HTTP/3 is fully opt-in.Http3Session(this is slightly contrived now, since it could work, but sets a clear model and becomes more useful later).applicationoption for QuicSessions is moved to Http3Session assettings. This exclusively supports HTTP/3 settings, and makes no sense in its current form for anything else (QUIC can't use or validate it by itself even if it wanted to).What this PR does not do
There's many things from #63995 not included here, which I would like to do later, including:
Session. That means the session has to look up and verify the supported features before every HTTP call, store bits of HTTP-only state, and general be a bit inelegant in thinking about both protocols at the same time.connectHttp3orlistenHttp3. We could create API methods like this which preconfigure the ALPN & automatically attach the HTTP/3 session, as a more convenient API sugar. I think the conclusion was we'd rather not and focus on primitives instead, but personally I don't feel strongly either way.