Subject: [quicwg/base-drafts] Immutable settings with no ACKs (#236)

From: Mike Bishop <[email-masked]>
Date: Wed, 25 Jan 2017 18:12:38 -0800

Fixes #181, which was discussed in Tokyo.
You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/236

-- Commit Summary --

  * Removed SETTINGS_ACK and made settings immutable
  * Prohibit server from sending until it sees SETTINGS

-- File Changes --

    M draft-ietf-quic-http.md (177)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/236.patch
https://github.com/quicwg/base-drafts/pull/236.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/236

---

From: Martin Thomson <[email-masked]>
Date: Mon, 30 Jan 2017 22:49:12 -0800

martinthomson requested changes on this pull request.

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 https://github.com/quicwg/base-drafts/issues/126#issuecomment-274733818 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.

>     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |                          Contents (?)                       ...
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 ~~~~~~~~~~~~~~~
 {: #fig-ext-settings title="SETTINGS value format"}
 
-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" 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.)

Remove parentheses.

> @@ -553,21 +554,12 @@ Different values for the same parameter can be advertised by each peer. For
 example, a client might permit a very large HPACK state table while a server
 chooses to use a small one to conserve memory.
 
-A SETTINGS frame MAY be sent at any time by either endpoint over the lifetime
-of the connection.
+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.

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)           |

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.

>  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.

See above suggestion.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/236#pullrequestreview-19251412

---

From: Patrick McManus <[email-masked]>
Date: Tue, 31 Jan 2017 02:14:11 -0800

mcmanus commented on this pull request.

     0                   1                   2                   3

that seems like a really minor gain for abandoning consistency..

---

From: Mike Bishop <[email-masked]>
Date: Tue, 31 Jan 2017 13:30:10 -0800

MikeBishop commented on this pull request.

     0                   1                   2                   3

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.

---

From: Mike Bishop <[email-masked]>
Date: Tue, 31 Jan 2017 13:47:36 -0800

@MikeBishop pushed 1 commit.

09761e2  Martin's feedback on SETTINGS_ACK removal

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/236/files/fca495be82790dc5a24502875c54264459501a54..09761e2b27cb19c59bdbc20366fb956cd8c9a2a7

---

From: Martin Thomson <[email-masked]>
Date: Tue, 31 Jan 2017 20:09:24 -0800

martinthomson commented on this pull request.

I'm fairly sure that we could iterate on this, but I've noted a few more issues, in particular with 0-RTT.

     0                   1                   2                   3

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.

> @@ -624,9 +616,8 @@ each HTTP/2 SETTINGS parameter is mapped:
   SETTINGS_HEADER_TABLE_SIZE:
   : An integer with a maximum value of 2^32 - 1.
 
-  SETTINGS_ENABLE_PUSH:
-  : Transmitted as a Boolean.  The default remains "true" as specified in
-    {{!RFC7540}}.
+  SETTINGS_DISABLE_PUSH:

I think that we can afford to spend a few octets on enabling push.

+Servers MAY continue processing data from clients which conform to these
+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.

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/236#pullrequestreview-19454415