-
Notifications
You must be signed in to change notification settings - Fork 205
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
Replace SETTINGS with EXTENDED_SETTINGS #80
Conversation
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.
This looks good enough to merge. A few nits though.
announced in a field of an extension frame, or in a value in SETTINGS.) | ||
|
||
Different values for the same parameter can be advertised by each peer. For | ||
example, a server might support many different signing algorithms, while a |
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.
I wouldn't use signature algorithms as an example here.
|
||
The SETTINGS frame defines the following flag: | ||
|
||
REQUEST_ACK (0x1): |
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.
I think that this will be formatted strangely. You should confirm with the manual and maybe check the build output.
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.
Build output looks fine to me -- is there something specific you're concerned about?
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.
You indented here, but I don't believe that this will result in any indenting for the REQUEST_ACK line.
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.
Ah, I see. It's indented in the HTML, but not indented in the TXT. That's consistent with the rest of the document, and with RFC 7540, which uses the same non-indentation for definition lists. I'm inclined to leave it; if it distresses you, feel free to suggest a change. ;-)
|
||
A zero-length content indicates that the setting value is a Boolean given by the | ||
B bit. If Length is not zero, the B bit MUST be zero, and MUST be ignored by | ||
receivers. The initial value of each setting is "false." |
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 initial value of a Boolean setting is "false".
@@ -447,13 +497,12 @@ superseded by QUIC transport parameters in HTTP-over-QUIC. Below is a listing of | |||
how each HTTP/2 SETTINGS parameter is mapped: | |||
|
|||
SETTINGS_HEADER_TABLE_SIZE: | |||
: Sent in SETTINGS frame. | |||
: An integer with a maximum value of 2^32 - 1. |
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.
We need a definition of how to encode integers in the field. Do we permit leading zero octets?
|
||
SETTINGS_ENABLE_PUSH: | ||
: Sent in SETTINGS frame (TBD, currently set using QUIC "SPSH" connection | ||
option) | ||
: A Boolean |
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.
This is unfortunate. Your change to booleans has made this have a new default.
Nit: missing period.
Updated to address your comments, @martinthomson . |
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.
I think this is definitely a positive change.. included a few minor comments for consideration.
can also be referred to as a "setting". | ||
|
||
SETTINGS parameters are not negotiated; they describe characteristics of the | ||
sending peer, which are used by the receiving peer. However, a negotiation can |
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.
s/are/may be/
be implied by the use of SETTINGS -- a peer uses SETTINGS to advertise a set of | ||
supported values. The recipient can then choose which entries from this list are | ||
also acceptable and proceed with the value it has chosen. (This choice could be | ||
announced in a field of an extension frame, or in a value in SETTINGS.) |
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.
s/in/in its own/
@@ -455,13 +515,13 @@ superseded by QUIC transport parameters in HTTP-over-QUIC. Below is a listing of | |||
how each HTTP/2 SETTINGS parameter is mapped: | |||
|
|||
SETTINGS_HEADER_TABLE_SIZE: | |||
: Sent in SETTINGS frame. | |||
: An integer with a maximum value of 2^32 - 1. |
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.
now that these are variable length does it need a max size? We can merge extended settings and talk about that as a different issue too
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.
Well, I think that's already a ridiculous size for a header table (64KB should be enough for anyone!), but I defined it that way mostly to retain parity with HTTP/2. For existing settings which are defined as integers already, might as well keep the same valid range where reasonable.
|
||
Once all values have been processed, if the REQUEST_ACK flag was set, the | ||
recipient MUST immediately emit a SETTINGS_ACK frame listing the identifiers |
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.
"immediately" has such a real time feel to it - while the requirement is really about ordering of when the new settings can have an impact on the semantics of future I/O.. it should probably be written like that
SETTING_ACK across all streams -- for #75
EXTENDED_SETTINGS was proposed in the HTTP WG as an extension providing an alternative to the HTTP/2 SETTINGS frame. While feedback was that it was "the design we should have had for HTTP/2... more general, saves space in most cases, and includes a flag to request acknowledgment", there was also a feeling that as an extension it was overkill since most things that needed settings already used SETTINGS. The advice was to hold it for HTTP/QUIC and make it the default SETTINGS frame there. So here it is.
Note that while it uses a slightly different ACK mechanism than HTTP/2 SETTINGS, it's subject to the same timing issues as HTTP/2 SETTINGS ACK over QUIC, namely that you can't know when the setting change has been applied on each stream. This pull request doesn't fix that; it's tracked by #75 still.