Skip to content
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

Move server's SETTINGS to transport parameters #2958

Closed
wants to merge 3 commits into from

Conversation

nharper
Copy link
Contributor

@nharper nharper commented Aug 8, 2019

Fixes #2790 and #2945

Changes the outcome of #2985

Copy link
Contributor

@ianswett ianswett left a comment

Choose a reason for hiding this comment

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

This LG, but I would like it more if it was symmetric, with the client sending its SETTINGS in transport params as well.

Copy link
Member

@LPardue LPardue left a comment

Choose a reason for hiding this comment

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

Some nits. Also for completeness you'll want to fixup the QPACK spec's reference to SETTINGS frame as well.

connection error of type HTTP_SETTINGS_ERROR.

Each parameter consists of a setting identifier and a value, both encoded as
QUCI variable-length integers, as shown in {{fig-ext-settings}}. A settings
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
QUCI variable-length integers, as shown in {{fig-ext-settings}}. A settings
QUIC variable-length integers, as shown in {{fig-ext-settings}}. A settings

Comment on lines 1076 to 1081
| DATA | No | Yes | Yes | {{frame-data}} |
| HEADERS | No | Yes | Yes | {{frame-headers}} |
| CANCEL_PUSH | Yes | No | No | {{frame-cancel-push}} |
| SETTINGS | Yes (1) | No | No | {{frame-settings}} |
| PUSH_PROMISE | No | Yes | No | {{frame-push-promise}} |
| GOAWAY | Yes | No | No | {{frame-goaway}} |
| MAX_PUSH_ID | Yes | No | No | {{frame-max-push-id}} |
Copy link
Member

Choose a reason for hiding this comment

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

Note that the side effect of this change is that is removes the only remaining instance of (1), which is described underneath the table ("Certain frames can only occur as the first frame of a particular stream type"). So we probably want to remove that foot note as part of this PR.

(see {{control-streams}}), an unknown frame type does not satisfy that
requirement and SHOULD be treated as an error.
location, an unknown frame type does not satisfy that requirement and SHOULD be
treated as an error.
Copy link
Member

Choose a reason for hiding this comment

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

With removing of SETTINGS there is no other case in the core HTTP/3 document that requires specific frame location. So I think this whole "However" clause can just be removed and make the text clearer.


Each application protocol MAY specify use of application parameters to be sent
by one of or both the client and server. If not specified, an application
parameter MUST NOT be sent for tht application's ALPN. For each application
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
parameter MUST NOT be sent for tht application's ALPN. For each application
parameter MUST NOT be sent for that application's ALPN. For each application

@nharper
Copy link
Contributor Author

nharper commented Oct 17, 2019

I think I've addressed the nits and updated QPACK.

Security/privacy considerations still need to be updated in HTTP and transport drafts.

@nharper
Copy link
Contributor Author

nharper commented Oct 18, 2019

Closing this PR since #3086 is being punted to v2.

@nharper nharper closed this Oct 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Binding settings into session tickets
3 participants