quic: Implement callback for http/3 settings/application options#63558
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
martenrichter
force-pushed
the
http3settings
branch
3 times, most recently
from
d6acf53 to
eb500d1
Compare
Implements a callback that is invoked once http/3 settings are received. Background, http/3 settings usually arrive a bit later than connection establishment, and e.g. for webtransport these settings are used to indicate support. So e.g. the examples for quiche from google, wait for the settings to arrive. (This is different to http/2). The implemented callback mechanism allows to wait for the settings to arrive until connection attempts are made. As settings are stored in the generic applications option object, the callback's name refers to the application rather than the settings. Whether this is a good choice is debatable. Fixes: nodejs#63553 Signed-off-by: Marten Richter <marten.richter@freenet.de>
martenrichter
force-pushed
the
http3settings
branch
from
eb500d1 to
12245aa
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #63558 +/- ##
==========================================
+ Coverage 90.14% 90.34% +0.19%
==========================================
Files 718 730 +12
Lines 227984 234547 +6563
Branches 42835 43909 +1074
==========================================
+ Hits 205522 211906 +6384
- Misses 14235 14383 +148
- Partials 8227 8258 +31
🚀 New features to boost your workflow:
|
|
OK, I am ready, so can someone from you push it? |
bjohansebas
added
request-ci
|
@nodejs/quic |
github-actions
Bot
removed
the
request-ci
pimterry
left a comment
pimterry
left a comment
There was a problem hiding this comment.
Thanks @martenrichter! This'll definitely be useful.
I'd prefer to change the name, I could be persuaded there if others disagree though.
The other comments are a couple of clear little bugs we should definitely fix.
| The endpoint that created this session. Returns `null` if the session | ||
| has been destroyed. Read only. | ||
|
|
||
| ### `session.onapplication` |
There was a problem hiding this comment.
The naming here is confusing, as you mentioned in the description, since it doesn't emit an application as such. This isn't a generic QUIC thing at all (transport parameters are a separate thing with a different flow) and right now this won't work for anything except HTTP/3. While some other protocols have equivalents it's not clear how closely they would all map to the model, and they definitely all have different names & structures anyway.
For HTTP/3, the RFC calls those 'settings', we use localSettings/remoteSettings in our existing HTTP/2 API. Since this is basically HTTP only and there's no umbrella term, I'd suggest this say 'settings' somewhere explicitly. Just onsettings would work fine from my pov. The rest of our HTTP-specific stuff (onheaders) doesn't explicitly name HTTP in the API naming so that seems consistent.
Beyond this PR: the HTTP/QUIC ambiguity here in the API ties a bit into my thoughts about the request timeout stuff @jasnell - starting to wonder whether we should separate the QUIC & HTTP/3 APIs more explicitly, to make this kind of thing cleaner. The distinction is very blurred together right now and we do lots of HTTP-shaped things for QUIC. Looks doable to split it a bit, really just an API restructuring rather than internals, but there'd be plenty of details to work out. Not required for this PR at all, just fyi, when I have time next week I'll take a look around it as a more general topic and try to open a rough draft to discuss, I have some ideas that'd be fun to explore 😄.
There was a problem hiding this comment.
The problem with the naming is, that it dates back to the choice that the http/3 settings are all inside the quic.ApplicationOptions . So I had to decide to call it onsettings , which would be my preferred choice for http/3 settings, or stick with the generic term applicationoptions (though I shortened it to onapplication as it was already very long).
So I think it is kind of a general decision. I have so far just flipped a coin.
Regarding the HTTP/QUIC ambiguity, I am playing around with how to implement webtransport and ngtcp2, and I think you can even wrap the WT streams to the QuicStream and have a mixture of streams. Even if it is confusing, it works some way.
P.S: I try to cover the minor stuff. As I do it in my free time, and the holiday day today is over it may take until next weekend.
There was a problem hiding this comment.
Minor stuff should be all covered. I am now waiting for consensus on the naming scheme.
pimterry
left a comment
pimterry
left a comment
There was a problem hiding this comment.
Fixes look good. I'd prefer something with 'settings' in the name, but happy to merge as is & change it later if there's no immediate consensus.
|
Let's see if someone else has an opinion. (I do not have a one...., of course I am happy with no renaming work yet). |
|
Ok, as there was no further feedback regarding renaming. Can someone with permissions merge it? (Or say that I should change it). This would be nice. |
|
@martenrichter we need to wait for @pimterry. |
pimterry
added
commit-queue
|
Oh, I did approve before already. Happy to go ahead as is, I'll commit-queue now. I have some broader plans for the HTTP/3 APIs in progress anyway (splitting them a bit from QUIC) which might juggle naming around, so happy to avoid bike shedding it in the meantime. |
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/63558 ✔ Done loading data for nodejs/node/pull/63558 ----------------------------------- PR info ------------------------------------ Title quic: Implement callback for http/3 settings/application options (#63558) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch martenrichter:http3settings -> nodejs:main Labels c++, lib / src, author ready, needs-ci, quic, commit-queue-squash Commits 6 - quic: impl. cb for http/3 settings/app. options - quic: add documentation for OnApplicationCallback - quic: Session::EmitApplication optimization - quic: http/3 settings bug fixes and added tests - quic: Fix http/3 settings test - quic: Fix linting Committers 1 - Marten Richter <marten.richter@tu-berlin.de> PR-URL: https://github.com/nodejs/node/pull/63558 Fixes: https://github.com/nodejs/node/issues/63553 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/63558 Fixes: https://github.com/nodejs/node/issues/63553 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> -------------------------------------------------------------------------------- ℹ This PR was created on Mon, 25 May 2026 12:45:25 GMT ✔ Approvals: 4 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/63558#pullrequestreview-4357448045 ✔ - Tim Perry (@pimterry): https://github.com/nodejs/node/pull/63558#pullrequestreview-4362329168 ✔ - Stephen Belanger (@Qard): https://github.com/nodejs/node/pull/63558#pullrequestreview-4378160632 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/63558#pullrequestreview-4459792383 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2026-05-25T17:19:03Z: https://ci.nodejs.org/job/node-test-pull-request/73717/ ⚠ Commits were pushed after the last Full PR CI run: ⚠ - quic: http/3 settings bug fixes and added tests ⚠ - quic: Fix http/3 settings test ⚠ - quic: Fix linting - Querying data for job/node-test-pull-request/73717/ ✔ Build data downloaded - Querying failures of job/node-test-commit/88213/ ✔ Data downloaded ✘ 2 failure(s) on the last Jenkins CI run -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/27216816696 |
|
I have some broader plans for the HTTP/3 APIs in progress anyway (splitting them a bit from QUIC) which might juggle naming around, so happy to avoid bike shedding it in the meantime.
Regarding this, I am playing around locally with webtransport based on the nghttp3 pr on this topic. Required changes are minimal, would it make sense to share it know, even if it does not work everything, so that you can consider it in these changes?
|
pimterry
added
request-ci
That'd definitely be interesting to look at, absolutely. Right now while everything is still early and relatively malleable the more use cases & API inspiration the better imo. |
github-actions
Bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
pimterry
added
the
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in e6a8d06 |
7 participants
Implements a callback that is invoked once http/3 settings are received.
Background, http/3 settings usually arrive a bit later than connection establishment, and e.g. for
webtransport these settings are used to indicate support. So e.g. the examples for quiche from google, wait for the settings to arrive. (This is different to http/2).
The implemented callback mechanism allows to wait for the settings to arrive until connection attempts are made.
As settings are stored in the generic applications option object, the callback's name refers to the application rather than the settings.
Whether this is a good choice is debatable.
Fixes: #63553