-
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
Default settings in HTTP #2038
Default settings in HTTP #2038
Conversation
This is glorious! I'd not at all been looking forward to adding support for a HOL-blocking SETTINGS frame to our QUIC implementation and am thrilled to not have to do that at all! :) |
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 can tolerate this.
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 great!!!
I like the PR because the approach provides the most flexibility; a client that wants to wait for SETTINGS can wait. A client that wants to not wait can start sending requests using the default settings parameters, then update as it receives the SETTINGS frame.
Should we state that the QPACK encoder stream MUST NOT be opened if the current value of SETTINGS_HEADER_TABLE_SIZE is zero, and that the decoder stream MUST NOT be opened until the peer opens the encoder stream?
I ask this because avoiding the cost of maintaining unused but open streams would be beneficial for using HTTP/3 on memory constrained devices. I do not think requiring that would be a burden because an endpoint nevertheless needs to wait for the arrival of the SETTINGS frame before starting to send dynamic table entries on the encoder stream. For the decoder stream, implementations can simply create them lazily.
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.
At a high level this seems ok. I've made some minor comments, feel free to accept/ignore at your leisure.
My only concern with this is that HTTP/3 extensions that require SETTINGS will introduce some blocking when compared to the vanilla HTTP/3 - this might make extensions less attractive to deploy. However, I'd expect most vanilla HTTP/3 deployments to want QPACK, and require SETTINGS exchange... my concerns are probably misplaced
draft-ietf-quic-http.md
Outdated
@@ -660,7 +658,7 @@ HTTP_MALFORMED_FRAME. | |||
The following settings are defined in HTTP/3: | |||
|
|||
SETTINGS_NUM_PLACEHOLDERS (0x3): | |||
: This value SHOULD be non-zero. The default value is 16. | |||
: This value SHOULD be non-zero. The default value is 0. |
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.
So I never understood why the old value was 16. Zero seems ok but this line, on naive, inspection reads strangely "Should not be zero but is zero by default"
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.
Per the "only increase" principal, however, this has to start at zero and be increased to the real value, even though 16 is a more appropriate value. Are you suggesting more text to remind people?
draft-ietf-quic-http.md
Outdated
based on its current understanding of the peer's settings. All settings begin | ||
at an initial value, and are updated upon receipt of a SETTINGS frame. For | ||
servers, the initial values of the client's settings are the defaults of each | ||
setting. |
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.
Could you improve this with a more concise statement, such as "For servers, the initial value of each client setting is its default value"?
draft-ietf-quic-http.md
Outdated
setting. | ||
|
||
For clients using a 1-RTT QUIC connection, the initial values of the server's | ||
settings are the defaults of each setting. When a 0-RTT QUIC connection is being |
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.
Change to be consistent with above (if above was accepted)
"the initial value of each server setting is its default value"
@@ -1014,12 +1014,12 @@ represented as an 8-bit prefix string literal. | |||
QPACK defines two settings which are included in the HTTP/QUIC SETTINGS frame. | |||
|
|||
SETTINGS_HEADER_TABLE_SIZE (0x1): | |||
: An integer with a maximum value of 2^30 - 1. The default value is 4,096 | |||
: An integer with a maximum value of 2^30 - 1. The default value is zero | |||
bytes. See {{table-dynamic}} for usage. |
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.
So I've just noticed this setting defines the default to have units of bytes. Do settings have units? Suggest you lose the units.
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 an interesting question. Section 2.2.2 of QPACK-03 begins as follows:
The size of the dynamic table is the sum of the size of its entries.
The size of an entry is the sum of its name's length in octets (as defined in Section 5.1.2), its value's length in octets, and 32.
My reading of this is that the table size is measured in octets.
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.
Units to be used in interpreting a setting need to be in the definition of the setting. The alternative, I suppose, would be to move the units to the section that defines how the integer is used, but I imagine someone looking to see what to put here would appreciate knowing whether to send 65.535 or 64.
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.
Also, it appears that @martinthomson's change to replace octets with bytes last month was global, so that text doesn't match the current editor's copy of QPACK.
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 a good change.
Shouldn’t octets be bytes for consistency with transport? |
@LPardue, I think the key point here is in the text added to the extensions section: "SETTINGS does not provide a mechanism to identify when the choice takes effect." There are really three types of extensions:
I think this really only harms the last category, and the easiest solution seems to be jumping into the previous category for most extensions I can envision. For example, redefining DATA to be a reference to a unidirectional stream instead of carrying payload would fall in the third bucket and be hard. But if you instead defined a new EXTERNAL_DATA frame, you've moved into the second bucket. |
That's a great write up but it's higher level than I was thinking.
If (and it's a big if) implementations decided it was more efficient to run
H3 on defaults without exchange of SETTINGS, we lose an effective
mechanism. This was part of the reason behind adding greasing. In practice,
I don't think this concern would play out but I wanted to identify it
before the change landed
…On Mon, 26 Nov 2018, 19:33 Mike Bishop ***@***.*** wrote:
@LPardue <https://github.com/LPardue>, I think the key point here is in
the text added to the extensions section: "SETTINGS does not provide a
mechanism to identify when the choice takes effect."
There are really three types of extensions:
- *Advisory, purely-optional frames / streams:* Send speculatively; if
the peer doesn't understand, they'll ignore them. Once you see SETTINGS, if
it's not supported, you can stop sending to save bytes.
- *Non-optional semantic-changing frames / streams:* Send only once
you've seen SETTINGS, but okay to use as soon as you see SETTINGS -- you
know the peer will handle them on receipt. Need to be willing to handle
"surprise" arrival of these frames / streams before SETTINGS, unless
they're frames on control streams.
- *Breaking changes to interpretation of existing frames/streams:*
Here's the sticky one. There's no coordination point to know when the
change to existing elements takes effect, so the peer could interpret what
you send under the old or new scheme unless the extension itself provides a
way to identify this.
I think this really only harms the last category, and the easiest solution
seems to be jumping into the previous category for most extensions I can
envision. For example, redefining DATA to be a reference to a
unidirectional stream instead of carrying payload would fall in the third
bucket and be hard. But if you instead defined a new EXTERNAL_DATA frame,
you've moved into the second bucket.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2038 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AGRFtbwaELdpzLfcYyLWHz4_Ycq1k86Fks5uzEIMgaJpZM4YuNMm>
.
|
You're still required to send/consume SETTINGS -- you're just allowed to use a basic version of the protocol for the (hopefully very brief) period where you haven't seen it yet. I'm dubious anyone would decide it's better to send an empty SETTINGS frame and continue using the defaults forever, certainly not to the point that extensibility withers. (And hopefully extensions will be compelling enough that they justify sending and looking for a setting.) |
The document currently says just this about new settings:
I think that you need to expand that to say that new extensions should include default values that assume minimal requirements on a endpoint, or something along those lines. |
In Section 7, it says:
I'll add a forward-reference to that section, assuming that text covers your concerns. |
@MikeBishop wrote:
You're correct "Each side MUST initiate a single control stream at the beginning of the connection and send its SETTINGS frame as the first frame on this stream." |
Fixes #1846.
This is an attempt to remove the head-of-line blocking on SETTINGS in HTTP. The principle is:
This results in most settings defaulting to zero. A client or server can wait for the SETTINGS frame to arrive (for example, in hopes of learning that the peer supports a non-zero QPACK table), or can proceed based on what it knows -- i.e. no QPACK table, no placeholders.
I expanded the discussion of how negotiation works in a purely declarative world, noting that there's no synchronization marker unless the extension itself creates one. (For an incompatible frame type, the marker would obviously be that you start using it. For a change to an existing frame type, things get complicated. Don't do that.)
(It's worth noting that we could go a step further -- if an endpoint doesn't want to change from the initial values, it currently MUST still send an empty SETTINGS frame as the first frame of the control stream. We could instead say that if the first frame isn't SETTINGS, the initial values become final.)
The sole setting that breaks this pattern is SETTINGS_MAX_HEADER_LIST_SIZE -- the default is unlimited, and a change from the default will clearly be less permissive, which is otherwise prohibited. But defaulting it to zero would prohibit sending any requests until the SETTINGS frame is received. 7540 terms this setting as "advisory," so I've expanded text noting that this is purely a courtesy to the other side and not binding.