quic: defer server session emit until TLS ClientHello is processed#64132
Open
pimterry wants to merge 1 commit into
Open
quic: defer server session emit until TLS ClientHello is processed#64132pimterry wants to merge 1 commit into
pimterry wants to merge 1 commit into
Conversation
This ensures we don't fire session events for totally invalid TLS handshakes - fundamental errors, bad SNI/ALPN values, or anything else that our TLS config would reject. Instead, it means servers can access servername & alpnProtocol synchronously as soon as the event is fired - all key session data is available and it's immediately usable. We don't want to defer further to handshake completed, since that'd be an extra RT, and defeat 0RTT benefits entirely. ClientHello processed without errors is sufficient for now. This isn't a security mechanism. Existing structures will defer actually sending & receiving anything that's not marked explicitly as early data until the handshake completes anyway. Signed-off-by: Tim Perry <pimterry@gmail.com>
Collaborator
|
Review requested:
|
nodejs-github-bot
added
c++
jasnell
reviewed
Jul 6, 2026
|
|
||
| The SNI (Server Name Indication) host name associated with the session, or | ||
| `undefined` if none was set. On a client this is the `servername` requested via | ||
| [`quic.connect()`][]. On a server it is the name sent by the peer. |
Member
There was a problem hiding this comment.
Wondering if string|null might be more semantically correct. Just a nit tho... feel free to ignore.
Either way, this should explain what undefined means.
jasnell
reviewed
Jul 6, 2026
| get servername() { | ||
| assertIsQuicSession(this); | ||
| if (this.destroyed) return undefined; | ||
| return this.#handle.getServername(); |
Member
There was a problem hiding this comment.
Let's cache these values after the first call to avoid the repeated calls across the C++ boundary.
jasnell
reviewed
Jul 6, 2026
| auto emits = std::move(impl_->deferred_emits_); | ||
| for (auto& emit : emits) { | ||
| if (is_destroyed()) return; | ||
| emit(); |
Member
There was a problem hiding this comment.
Since each of these is a C++-to-JavaScript call, I wonder if it would be possible to batch them to a new callback. Essentially, rather than enqueuing a function, enqueue the arguments and type of emit, send them all to JavaScript at once and process each one there instead. Calls from C++-to-JavaScript can be a bit expensive to set up.
Member
There was a problem hiding this comment.
This can be looked at later tho.
jasnell
approved these changes
Jul 6, 2026
This comment was marked as outdated.
This comment was marked as outdated.
Collaborator
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extracted from #63995. It's not identical to the implementation there, I did a little more cleanup and refactoring here, and dropped some parts that aren't relevant without the latest changes in that PR, but it's reviewable standalone.
This change means servers can access servername & alpnProtocol synchronously as soon as the event is fired - all key session data is available and it's immediately usable. New getters are exposed for that. This also ensures we don't fire session events for totally invalid TLS handshakes - fundamental errors, bad SNI/ALPN values, or anything else that our TLS config would reject.
It does so without waiting for any more round trips that before, in any normal flow: it just defers the session event to the end of the client hello processing which should be momentarily after the previous behaviour. In very strange flows (if the first flight from the client doesn't contain the full client hello) then this could introduce a more significant delay, but AFAICT no normal client should ever do that (and we'd have to do some kind of wait regardless eventually if they did - we can't really do anything useful without a complete client hello).
This is just intended to improve UX and smooth the path towards dynamic Application selection (as used in #63995). A few things this doesn't do:
opened. Dynamic app selection there would wait for full handshake completion anyway, since that's the only way to know the confirmed ALPN, which you'll want in most cases (though we don't currently support multiple ALPN for clients anyway). All deferred for later.tlsClientErroranalogue, but imo that was true before as well and we can continue to defer it for now. Most servers aren't that interested in clients who fail to connect, it's not a top priority.