quic: defer server session emit until TLS ClientHello is processed - #64132

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
pimterry:defer-quic-session-emit
Jul 23, 2026
Merged

quic: defer server session emit until TLS ClientHello is processed#64132
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
pimterry:defer-quic-session-emit

Conversation

@pimterry

@pimterrypimterry commented Jun 25, 2026

Copy link
Copy Markdown
Member

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:

  • It doesn't wait for handshake completion. It's just processing of the client hello. Completion would imply an extra round trip, and thereby defeat 0RTT benefits entirely.
  • It doesn't improve security (though it helps avoid some footguns). Existing mechanisms already ensure we can't read or write data that isn't correctly within the TLS handshake (deferred data on new connections until the handshake completes, or allowing early data that's explicitly marked as such on resumed connections)
  • It doesn't change client behaviour. In the client case, the session is available on creation, and you always have to wait for 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.
  • It doesn't expose these failed TLS sessions in any way. In future we will probably want a tlsClientError analogue, 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.

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>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/quic

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Jun 25, 2026
Comment threaddoc/api/quic.md Outdated
Comment threadlib/internal/quic/quic.js Outdated
Comment threadsrc/quic/session.cc
auto emits = std::move(impl_->deferred_emits_);
for (auto& emit : emits) {
if (is_destroyed()) return;
emit();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be looked at later tho.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left this for now. It looks likely to be a little fiddly, and I think it rarely comes up (just a couple of 0RTT events, or keylog/qlog when they're enabled) but we can explore as an optimization later.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

…ssed
Cache ALPN & servername once set, update field types and docs.
@pimterry
pimterry requested a review from jasnellJuly 8, 2026 14:46
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@codecov

codecovBot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.24%. Comparing base (e6a8d06) to head (8d11c96).
⚠️ Report is 420 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #64132 +/- ##
==========================================
- Coverage 92.01% 90.24% -1.78% 
==========================================
Files 379 741 +362 Lines 166972 241037 +74065 Branches 25554 45411 +19857 ==========================================
+ Hits 153639 217513 +63874 - Misses 13041 15108 +2067 - Partials 292 8416 +8124 
Files with missing linesCoverage Δ
lib/internal/quic/quic.js100.00% <100.00%> (ø)

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

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@pimterry

Copy link
Copy Markdown
MemberAuthor

@jasnell when you have a sec, I'd love a re-review here to land this. That last commit is just fixes for the points from your review.

@pimterrypimterry added the commit-queue PRs queued for automated landing through the Commit Queue. label Jul 23, 2026
@nodejs-github-botnodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jul 23, 2026
@nodejs-github-bot
nodejs-github-bot merged commit f9715fc into nodejs:mainJul 23, 2026
68 checks passed
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in f9715fc

aduh95 pushed a commit that referenced this pull request Jul 30, 2026
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>
PR-URL: #64132
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 pushed a commit that referenced this pull request Aug 6, 2026
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>
PR-URL: #64132
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues and PRs involving general changes in the lib/ or src/ directories.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@pimterry@nodejs-github-bot@jasnell
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

quic: defer server session emit until TLS ClientHello is processed - #64132

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
pimterry:defer-quic-session-emit
Jul 23, 2026
Merged

quic: defer server session emit until TLS ClientHello is processed#64132
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
pimterry:defer-quic-session-emit

Conversation

@pimterry

@pimterrypimterry commented Jun 25, 2026

Copy link
Copy Markdown
Member

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:

  • It doesn't wait for handshake completion. It's just processing of the client hello. Completion would imply an extra round trip, and thereby defeat 0RTT benefits entirely.
  • It doesn't improve security (though it helps avoid some footguns). Existing mechanisms already ensure we can't read or write data that isn't correctly within the TLS handshake (deferred data on new connections until the handshake completes, or allowing early data that's explicitly marked as such on resumed connections)
  • It doesn't change client behaviour. In the client case, the session is available on creation, and you always have to wait for 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.
  • It doesn't expose these failed TLS sessions in any way. In future we will probably want a tlsClientError analogue, 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.

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>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/quic

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Jun 25, 2026
Comment threaddoc/api/quic.md Outdated
Comment threadlib/internal/quic/quic.js Outdated
Comment threadsrc/quic/session.cc
auto emits = std::move(impl_->deferred_emits_);
for (auto& emit : emits) {
if (is_destroyed()) return;
emit();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be looked at later tho.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left this for now. It looks likely to be a little fiddly, and I think it rarely comes up (just a couple of 0RTT events, or keylog/qlog when they're enabled) but we can explore as an optimization later.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

…ssed
Cache ALPN & servername once set, update field types and docs.
@pimterry
pimterry requested a review from jasnellJuly 8, 2026 14:46
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@codecov

codecovBot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.24%. Comparing base (e6a8d06) to head (8d11c96).
⚠️ Report is 420 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #64132 +/- ##
==========================================
- Coverage 92.01% 90.24% -1.78% 
==========================================
Files 379 741 +362 Lines 166972 241037 +74065 Branches 25554 45411 +19857 ==========================================
+ Hits 153639 217513 +63874 - Misses 13041 15108 +2067 - Partials 292 8416 +8124 
Files with missing linesCoverage Δ
lib/internal/quic/quic.js100.00% <100.00%> (ø)

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

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@pimterry

Copy link
Copy Markdown
MemberAuthor

@jasnell when you have a sec, I'd love a re-review here to land this. That last commit is just fixes for the points from your review.

@pimterrypimterry added the commit-queue PRs queued for automated landing through the Commit Queue. label Jul 23, 2026
@nodejs-github-botnodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jul 23, 2026
@nodejs-github-bot
nodejs-github-bot merged commit f9715fc into nodejs:mainJul 23, 2026
68 checks passed
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in f9715fc

aduh95 pushed a commit that referenced this pull request Jul 30, 2026
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>
PR-URL: #64132
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 pushed a commit that referenced this pull request Aug 6, 2026
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>
PR-URL: #64132
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues and PRs involving general changes in the lib/ or src/ directories.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@pimterry@nodejs-github-bot@jasnell
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

quic: defer server session emit until TLS ClientHello is processed - #64132

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
pimterry:defer-quic-session-emit
Jul 23, 2026
Merged

quic: defer server session emit until TLS ClientHello is processed#64132
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
pimterry:defer-quic-session-emit

Conversation

@pimterry

@pimterrypimterry commented Jun 25, 2026

Copy link
Copy Markdown
Member

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:

  • It doesn't wait for handshake completion. It's just processing of the client hello. Completion would imply an extra round trip, and thereby defeat 0RTT benefits entirely.
  • It doesn't improve security (though it helps avoid some footguns). Existing mechanisms already ensure we can't read or write data that isn't correctly within the TLS handshake (deferred data on new connections until the handshake completes, or allowing early data that's explicitly marked as such on resumed connections)
  • It doesn't change client behaviour. In the client case, the session is available on creation, and you always have to wait for 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.
  • It doesn't expose these failed TLS sessions in any way. In future we will probably want a tlsClientError analogue, 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.

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>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/quic

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Jun 25, 2026
Comment threaddoc/api/quic.md Outdated
Comment threadlib/internal/quic/quic.js Outdated
Comment threadsrc/quic/session.cc
auto emits = std::move(impl_->deferred_emits_);
for (auto& emit : emits) {
if (is_destroyed()) return;
emit();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be looked at later tho.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left this for now. It looks likely to be a little fiddly, and I think it rarely comes up (just a couple of 0RTT events, or keylog/qlog when they're enabled) but we can explore as an optimization later.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

…ssed
Cache ALPN & servername once set, update field types and docs.
@pimterry
pimterry requested a review from jasnellJuly 8, 2026 14:46
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@codecov

codecovBot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.24%. Comparing base (e6a8d06) to head (8d11c96).
⚠️ Report is 420 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #64132 +/- ##
==========================================
- Coverage 92.01% 90.24% -1.78% 
==========================================
Files 379 741 +362 Lines 166972 241037 +74065 Branches 25554 45411 +19857 ==========================================
+ Hits 153639 217513 +63874 - Misses 13041 15108 +2067 - Partials 292 8416 +8124 
Files with missing linesCoverage Δ
lib/internal/quic/quic.js100.00% <100.00%> (ø)

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

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@pimterry

Copy link
Copy Markdown
MemberAuthor

@jasnell when you have a sec, I'd love a re-review here to land this. That last commit is just fixes for the points from your review.

@pimterrypimterry added the commit-queue PRs queued for automated landing through the Commit Queue. label Jul 23, 2026
@nodejs-github-botnodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jul 23, 2026
@nodejs-github-bot
nodejs-github-bot merged commit f9715fc into nodejs:mainJul 23, 2026
68 checks passed
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in f9715fc

aduh95 pushed a commit that referenced this pull request Jul 30, 2026
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>
PR-URL: #64132
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 pushed a commit that referenced this pull request Aug 6, 2026
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>
PR-URL: #64132
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues and PRs involving general changes in the lib/ or src/ directories.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@pimterry@nodejs-github-bot@jasnell
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

quic: defer server session emit until TLS ClientHello is processed - #64132

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
pimterry:defer-quic-session-emit
Jul 23, 2026
Merged

quic: defer server session emit until TLS ClientHello is processed#64132
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
pimterry:defer-quic-session-emit

Conversation

@pimterry

@pimterrypimterry commented Jun 25, 2026

Copy link
Copy Markdown
Member

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:

  • It doesn't wait for handshake completion. It's just processing of the client hello. Completion would imply an extra round trip, and thereby defeat 0RTT benefits entirely.
  • It doesn't improve security (though it helps avoid some footguns). Existing mechanisms already ensure we can't read or write data that isn't correctly within the TLS handshake (deferred data on new connections until the handshake completes, or allowing early data that's explicitly marked as such on resumed connections)
  • It doesn't change client behaviour. In the client case, the session is available on creation, and you always have to wait for 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.
  • It doesn't expose these failed TLS sessions in any way. In future we will probably want a tlsClientError analogue, 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.

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>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/quic

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Jun 25, 2026
Comment threaddoc/api/quic.md Outdated
Comment threadlib/internal/quic/quic.js Outdated
Comment threadsrc/quic/session.cc
auto emits = std::move(impl_->deferred_emits_);
for (auto& emit : emits) {
if (is_destroyed()) return;
emit();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be looked at later tho.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left this for now. It looks likely to be a little fiddly, and I think it rarely comes up (just a couple of 0RTT events, or keylog/qlog when they're enabled) but we can explore as an optimization later.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

…ssed
Cache ALPN & servername once set, update field types and docs.
@pimterry
pimterry requested a review from jasnellJuly 8, 2026 14:46
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@codecov

codecovBot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.24%. Comparing base (e6a8d06) to head (8d11c96).
⚠️ Report is 420 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #64132 +/- ##
==========================================
- Coverage 92.01% 90.24% -1.78% 
==========================================
Files 379 741 +362 Lines 166972 241037 +74065 Branches 25554 45411 +19857 ==========================================
+ Hits 153639 217513 +63874 - Misses 13041 15108 +2067 - Partials 292 8416 +8124 
Files with missing linesCoverage Δ
lib/internal/quic/quic.js100.00% <100.00%> (ø)

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

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@pimterry

Copy link
Copy Markdown
MemberAuthor

@jasnell when you have a sec, I'd love a re-review here to land this. That last commit is just fixes for the points from your review.

@pimterrypimterry added the commit-queue PRs queued for automated landing through the Commit Queue. label Jul 23, 2026
@nodejs-github-botnodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jul 23, 2026
@nodejs-github-bot
nodejs-github-bot merged commit f9715fc into nodejs:mainJul 23, 2026
68 checks passed
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in f9715fc

aduh95 pushed a commit that referenced this pull request Jul 30, 2026
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>
PR-URL: #64132
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 pushed a commit that referenced this pull request Aug 6, 2026
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>
PR-URL: #64132
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues and PRs involving general changes in the lib/ or src/ directories.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@pimterry@nodejs-github-bot@jasnell
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

quic: defer server session emit until TLS ClientHello is processed - #64132

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
pimterry:defer-quic-session-emit
Jul 23, 2026
Merged

quic: defer server session emit until TLS ClientHello is processed#64132
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
pimterry:defer-quic-session-emit

Conversation

@pimterry

@pimterrypimterry commented Jun 25, 2026

Copy link
Copy Markdown
Member

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:

  • It doesn't wait for handshake completion. It's just processing of the client hello. Completion would imply an extra round trip, and thereby defeat 0RTT benefits entirely.
  • It doesn't improve security (though it helps avoid some footguns). Existing mechanisms already ensure we can't read or write data that isn't correctly within the TLS handshake (deferred data on new connections until the handshake completes, or allowing early data that's explicitly marked as such on resumed connections)
  • It doesn't change client behaviour. In the client case, the session is available on creation, and you always have to wait for 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.
  • It doesn't expose these failed TLS sessions in any way. In future we will probably want a tlsClientError analogue, 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.

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>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/quic

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Jun 25, 2026
Comment threaddoc/api/quic.md Outdated
Comment threadlib/internal/quic/quic.js Outdated
Comment threadsrc/quic/session.cc
auto emits = std::move(impl_->deferred_emits_);
for (auto& emit : emits) {
if (is_destroyed()) return;
emit();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be looked at later tho.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left this for now. It looks likely to be a little fiddly, and I think it rarely comes up (just a couple of 0RTT events, or keylog/qlog when they're enabled) but we can explore as an optimization later.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

…ssed
Cache ALPN & servername once set, update field types and docs.
@pimterry
pimterry requested a review from jasnellJuly 8, 2026 14:46
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@codecov

codecovBot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.24%. Comparing base (e6a8d06) to head (8d11c96).
⚠️ Report is 420 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #64132 +/- ##
==========================================
- Coverage 92.01% 90.24% -1.78% 
==========================================
Files 379 741 +362 Lines 166972 241037 +74065 Branches 25554 45411 +19857 ==========================================
+ Hits 153639 217513 +63874 - Misses 13041 15108 +2067 - Partials 292 8416 +8124 
Files with missing linesCoverage Δ
lib/internal/quic/quic.js100.00% <100.00%> (ø)

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

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@pimterry

Copy link
Copy Markdown
MemberAuthor

@jasnell when you have a sec, I'd love a re-review here to land this. That last commit is just fixes for the points from your review.

@pimterrypimterry added the commit-queue PRs queued for automated landing through the Commit Queue. label Jul 23, 2026
@nodejs-github-botnodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jul 23, 2026
@nodejs-github-bot
nodejs-github-bot merged commit f9715fc into nodejs:mainJul 23, 2026
68 checks passed
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in f9715fc

aduh95 pushed a commit that referenced this pull request Jul 30, 2026
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>
PR-URL: #64132
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 pushed a commit that referenced this pull request Aug 6, 2026
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>
PR-URL: #64132
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues and PRs involving general changes in the lib/ or src/ directories.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@pimterry@nodejs-github-bot@jasnell
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

quic: defer server session emit until TLS ClientHello is processed - #64132

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
pimterry:defer-quic-session-emit
Jul 23, 2026
Merged

quic: defer server session emit until TLS ClientHello is processed#64132
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
pimterry:defer-quic-session-emit

Conversation

@pimterry

@pimterrypimterry commented Jun 25, 2026

Copy link
Copy Markdown
Member

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:

  • It doesn't wait for handshake completion. It's just processing of the client hello. Completion would imply an extra round trip, and thereby defeat 0RTT benefits entirely.
  • It doesn't improve security (though it helps avoid some footguns). Existing mechanisms already ensure we can't read or write data that isn't correctly within the TLS handshake (deferred data on new connections until the handshake completes, or allowing early data that's explicitly marked as such on resumed connections)
  • It doesn't change client behaviour. In the client case, the session is available on creation, and you always have to wait for 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.
  • It doesn't expose these failed TLS sessions in any way. In future we will probably want a tlsClientError analogue, 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.

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>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/quic

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Jun 25, 2026
Comment threaddoc/api/quic.md Outdated
Comment threadlib/internal/quic/quic.js Outdated
Comment threadsrc/quic/session.cc
auto emits = std::move(impl_->deferred_emits_);
for (auto& emit : emits) {
if (is_destroyed()) return;
emit();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be looked at later tho.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left this for now. It looks likely to be a little fiddly, and I think it rarely comes up (just a couple of 0RTT events, or keylog/qlog when they're enabled) but we can explore as an optimization later.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

…ssed
Cache ALPN & servername once set, update field types and docs.
@pimterry
pimterry requested a review from jasnellJuly 8, 2026 14:46
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@codecov

codecovBot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.24%. Comparing base (e6a8d06) to head (8d11c96).
⚠️ Report is 420 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #64132 +/- ##
==========================================
- Coverage 92.01% 90.24% -1.78% 
==========================================
Files 379 741 +362 Lines 166972 241037 +74065 Branches 25554 45411 +19857 ==========================================
+ Hits 153639 217513 +63874 - Misses 13041 15108 +2067 - Partials 292 8416 +8124 
Files with missing linesCoverage Δ
lib/internal/quic/quic.js100.00% <100.00%> (ø)

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

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@pimterry

Copy link
Copy Markdown
MemberAuthor

@jasnell when you have a sec, I'd love a re-review here to land this. That last commit is just fixes for the points from your review.

@pimterrypimterry added the commit-queue PRs queued for automated landing through the Commit Queue. label Jul 23, 2026
@nodejs-github-botnodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jul 23, 2026
@nodejs-github-bot
nodejs-github-bot merged commit f9715fc into nodejs:mainJul 23, 2026
68 checks passed
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in f9715fc

aduh95 pushed a commit that referenced this pull request Jul 30, 2026
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>
PR-URL: #64132
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 pushed a commit that referenced this pull request Aug 6, 2026
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>
PR-URL: #64132
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues and PRs involving general changes in the lib/ or src/ directories.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@pimterry@nodejs-github-bot@jasnell
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

quic: defer server session emit until TLS ClientHello is processed - #64132

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
pimterry:defer-quic-session-emit
Jul 23, 2026
Merged

quic: defer server session emit until TLS ClientHello is processed#64132
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
pimterry:defer-quic-session-emit

Conversation

@pimterry

@pimterrypimterry commented Jun 25, 2026

Copy link
Copy Markdown
Member

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:

  • It doesn't wait for handshake completion. It's just processing of the client hello. Completion would imply an extra round trip, and thereby defeat 0RTT benefits entirely.
  • It doesn't improve security (though it helps avoid some footguns). Existing mechanisms already ensure we can't read or write data that isn't correctly within the TLS handshake (deferred data on new connections until the handshake completes, or allowing early data that's explicitly marked as such on resumed connections)
  • It doesn't change client behaviour. In the client case, the session is available on creation, and you always have to wait for 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.
  • It doesn't expose these failed TLS sessions in any way. In future we will probably want a tlsClientError analogue, 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.

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>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/quic

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Jun 25, 2026
Comment threaddoc/api/quic.md Outdated
Comment threadlib/internal/quic/quic.js Outdated
Comment threadsrc/quic/session.cc
auto emits = std::move(impl_->deferred_emits_);
for (auto& emit : emits) {
if (is_destroyed()) return;
emit();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be looked at later tho.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left this for now. It looks likely to be a little fiddly, and I think it rarely comes up (just a couple of 0RTT events, or keylog/qlog when they're enabled) but we can explore as an optimization later.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

…ssed
Cache ALPN & servername once set, update field types and docs.
@pimterry
pimterry requested a review from jasnellJuly 8, 2026 14:46
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@codecov

codecovBot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.24%. Comparing base (e6a8d06) to head (8d11c96).
⚠️ Report is 420 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #64132 +/- ##
==========================================
- Coverage 92.01% 90.24% -1.78% 
==========================================
Files 379 741 +362 Lines 166972 241037 +74065 Branches 25554 45411 +19857 ==========================================
+ Hits 153639 217513 +63874 - Misses 13041 15108 +2067 - Partials 292 8416 +8124 
Files with missing linesCoverage Δ
lib/internal/quic/quic.js100.00% <100.00%> (ø)

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

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@pimterry

Copy link
Copy Markdown
MemberAuthor

@jasnell when you have a sec, I'd love a re-review here to land this. That last commit is just fixes for the points from your review.

@pimterrypimterry added the commit-queue PRs queued for automated landing through the Commit Queue. label Jul 23, 2026
@nodejs-github-botnodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jul 23, 2026
@nodejs-github-bot
nodejs-github-bot merged commit f9715fc into nodejs:mainJul 23, 2026
68 checks passed
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in f9715fc

aduh95 pushed a commit that referenced this pull request Jul 30, 2026
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>
PR-URL: #64132
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 pushed a commit that referenced this pull request Aug 6, 2026
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>
PR-URL: #64132
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues and PRs involving general changes in the lib/ or src/ directories.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@pimterry@nodejs-github-bot@jasnell
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

quic: defer server session emit until TLS ClientHello is processed - #64132

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
pimterry:defer-quic-session-emit
Jul 23, 2026
Merged

quic: defer server session emit until TLS ClientHello is processed#64132
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
pimterry:defer-quic-session-emit

Conversation

@pimterry

@pimterrypimterry commented Jun 25, 2026

Copy link
Copy Markdown
Member

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:

  • It doesn't wait for handshake completion. It's just processing of the client hello. Completion would imply an extra round trip, and thereby defeat 0RTT benefits entirely.
  • It doesn't improve security (though it helps avoid some footguns). Existing mechanisms already ensure we can't read or write data that isn't correctly within the TLS handshake (deferred data on new connections until the handshake completes, or allowing early data that's explicitly marked as such on resumed connections)
  • It doesn't change client behaviour. In the client case, the session is available on creation, and you always have to wait for 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.
  • It doesn't expose these failed TLS sessions in any way. In future we will probably want a tlsClientError analogue, 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.

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>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/quic

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Jun 25, 2026
Comment threaddoc/api/quic.md Outdated
Comment threadlib/internal/quic/quic.js Outdated
Comment threadsrc/quic/session.cc
auto emits = std::move(impl_->deferred_emits_);
for (auto& emit : emits) {
if (is_destroyed()) return;
emit();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be looked at later tho.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left this for now. It looks likely to be a little fiddly, and I think it rarely comes up (just a couple of 0RTT events, or keylog/qlog when they're enabled) but we can explore as an optimization later.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

…ssed
Cache ALPN & servername once set, update field types and docs.
@pimterry
pimterry requested a review from jasnellJuly 8, 2026 14:46
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@codecov

codecovBot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.24%. Comparing base (e6a8d06) to head (8d11c96).
⚠️ Report is 420 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #64132 +/- ##
==========================================
- Coverage 92.01% 90.24% -1.78% 
==========================================
Files 379 741 +362 Lines 166972 241037 +74065 Branches 25554 45411 +19857 ==========================================
+ Hits 153639 217513 +63874 - Misses 13041 15108 +2067 - Partials 292 8416 +8124 
Files with missing linesCoverage Δ
lib/internal/quic/quic.js100.00% <100.00%> (ø)

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

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@pimterry

Copy link
Copy Markdown
MemberAuthor

@jasnell when you have a sec, I'd love a re-review here to land this. That last commit is just fixes for the points from your review.

@pimterrypimterry added the commit-queue PRs queued for automated landing through the Commit Queue. label Jul 23, 2026
@nodejs-github-botnodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jul 23, 2026
@nodejs-github-bot
nodejs-github-bot merged commit f9715fc into nodejs:mainJul 23, 2026
68 checks passed
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in f9715fc

aduh95 pushed a commit that referenced this pull request Jul 30, 2026
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>
PR-URL: #64132
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 pushed a commit that referenced this pull request Aug 6, 2026
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>
PR-URL: #64132
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues and PRs involving general changes in the lib/ or src/ directories.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@pimterry@nodejs-github-bot@jasnell