-
Notifications
You must be signed in to change notification settings - Fork 203
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
Immutable settings with no ACKs #236
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.
I like the amount of red in this PR. But it needs a little more green.
This needs a discussion of settings in the context of 0-RTT. Because the rule is that a server cannot speak until it learns the client settings, a server is never in a situation where it could be surprised.
On the other hand, a client that uses 0-RTT has to open and use streams, or there wouldn't be much point. What assumptions can a client make about server settings?
In #126 (comment) we decided to recommend caching, but there would be defaults and that servers would have to be prepared to accept either the cached value or the default. I assume that we'd probably want the same conclusion here.
draft-ietf-quic-http.md
Outdated
receivers. The initial value of each setting is "false" unless otherwise | ||
specified by the definition of the setting. | ||
A zero-length content indicates that the setting value is a Boolean and true. | ||
(False is indicated by the absence of the 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.
Remove parentheses.
draft-ietf-quic-http.md
Outdated
Parameters are processed in the order in which they appear, and the value of a | ||
SETTINGS parameter is the last value that is seen by a receiver. The receiver of | ||
a SETTINGS frame does not need to maintain any state other than the last value | ||
of a given parameter. |
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.
Can we forbid duplicates? We wouldn't necessarily require enforcement, but it would avoid one of the nasty cases where there a bad actor can cause an unbounded amount of work to be performed (assuming here that skipping unsupported values is essentially free).
@@ -577,17 +569,15 @@ value. | |||
0 1 2 3 | |||
0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 | |||
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | |||
| Identifier (16) |B| Length (15) | | |||
| Identifier (16) | Length (16) | |
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.
SETTINGS only appears in one place, so why does it need an identifier? That is, you could start the stream with a length (or counter) directly. Then it's not really a frame type.
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.
that seems like a really minor gain for abandoning consistency..
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.
That's an interesting discussion. (I'd agree with Patrick, it's not worth the inconsistency.) However, this Identifier field says which setting the value goes with, so it's definitely necessary.
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 comment is in the wrong place. Sure. The hazard with defining a frame type is that you now need handling for it. If it wasn't a frame, there would be no possibility of error.
HTTP_INTERRUPTED_HEADERS (0x0E): | ||
: A HEADERS frame without the End Header Block flag was followed by a frame | ||
other than HEADERS. | ||
|
||
HTTP_SETTINGS_ON_WRONG_STREAM (0x0F): | ||
: A SETTINGS frame was received on a request control stream. | ||
|
||
HTTP_MULTIPLE_SETTINGS (0x10): | ||
: More than one SETTINGS frame was received. |
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.
See above suggestion.
09761e2
to
42f9f47
Compare
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'm fairly sure that we could iterate on this once merged, but I've noted a few more issues, in particular with 0-RTT.
@@ -577,17 +569,15 @@ value. | |||
0 1 2 3 | |||
0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 | |||
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | |||
| Identifier (16) |B| Length (15) | | |||
| Identifier (16) | Length (16) | |
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 comment is in the wrong place. Sure. The hazard with defining a frame type is that you now need handling for it. If it wasn't a frame, there would be no possibility of error.
SETTINGS_ENABLE_PUSH: | ||
: Transmitted as a Boolean. The default remains "true" as specified in | ||
{{!RFC7540}}. | ||
SETTINGS_DISABLE_PUSH: |
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 we can afford to spend a few octets on enabling push.
draft-ietf-quic-http.md
Outdated
defaults during the initial flight, even if the client behavior exceeds its | ||
current configuration. If the connection is closed because these or other | ||
constraints were violated during the 0-RTT flight, clients MAY retry using the | ||
settings sent by the server on the closed connection. |
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.
It's not obvious that it is safe to retry. The key insight (that took me a while to latch onto) is that 0-RTT data is replayable, so it's OK to assume that it's safe to retry. You should say that and help people avoid having to chase down that logical rabbit every time they read this. I also think that this needs to be clearer about reconnecting as opposed to retrying any requests. That is "[...] a client MAY establish a new connection and retry any 0-RTT requests. This assumes that only requests that are safe to retry are sent in 0-RTT, as required by [...something]."
Separately, it's not obvious here that the client is able to learn the new settings if the server terminates the connection. You have to explain that the server cannot generate an error code until it completes the handshake and therefore the client could learn updated settings. However, it's not guaranteed that the client will get the updated settings. Because hq errors are signaled at the transport layer, the server code might not send the stream data carrying the SETTINGS frame before sending a CONNECTION_CLOSE unless we insist that they do so (I'm not sure that would be a good idea).
Finally, you need some rules about how to proceed in the case that assumed values (cached or defaults) exceed or violate actual values. I assume that a client can't add to the header table and is encouraged to remove entries immediately. Header list size seems like an easy one.
This design implies that the definition of new settings have extra requirements. Each setting needs:
- a default value
- a rule defining whether it can be carried across into 0-RTT
- rules defining how to move between assumed value and an actual value for 0-RTT
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.
Silly thought: It would be nice if we could get QUIC to guarantee delivery of a certain payload that explains the connection close. This feels very close to the reason phrase, but isn't just human-readable text.
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 there are actually two categories here. Exceeding max header size, for example, only causes your request to be refused, not the entire connection to be reset. You can tolerate that and retry. (If, that is, you can programmatically reduce the size of your header set!)
The HPACK limit will cause the connection to be reset. We can fix that with a punitively-small value if you haven't cached something -- which is okay, because 0-RTT means you've spoken to the server before, so you had the opportunity to cache. If the client's cached value is now too big, there are a couple of options:
- Server can be permissive. Client MUST apply the settings as soon as it knows them. There is no delete in HPACK, so the client would just announce a table size change in its next frame. Once we have QPACK or a cousin, we can bring back the text on mid-stream table size changes for this case.
- Server can reject
- Client saw SETTINGS: Retry and use those
- Client didn't see SETTINGS: Retry and use the horrific default
Yes, this means that servers which both reduce their settings and refuse to honor the old values will be imposing a one-time penalty on clients, but it's strictly better than 1-RTT.
draft-ietf-quic-http.md
Outdated
|
||
- SETTINGS_HEADER_TABLE_SIZE: 4,096 octets | ||
- SETTINGS_HEADER_TABLE_SIZE: 0 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.
👍
Fixes #181, which was discussed in Tokyo.