-
-
Notifications
You must be signed in to change notification settings - Fork 35.7k
quic: Implement callback for http/3 settings/application options #63558
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
martenrichter
wants to merge
6
commits into
nodejs:main
Choose a base branch
from
martenrichter:http3settings
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+167
−5
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
12245aa
quic: impl. cb for http/3 settings/app. options
martenrichter d214360
quic: add documentation for OnApplicationCallback
martenrichter a4ec0c0
quic: Session::EmitApplication optimization
martenrichter 5385665
quic: http/3 settings bug fixes and added tests
martenrichter 3d5d691
quic: Fix http/3 settings test
martenrichter 9c5942b
quic: Fix linting
martenrichter File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/remoteSettingsin 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. Justonsettingswould 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 itonsettings, which would be my preferred choice for http/3 settings, or stick with the generic termapplicationoptions(though I shortened it toonapplicationas 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor stuff should be all covered. I am now waiting for consensus on the naming scheme.