Subject: [quicwg/base-drafts] Limit pushes using MAX_PUSH_ID (#711)

From: Martin Thomson <[email-masked]>
Date: Tue, 08 Aug 2017 08:39:19 +0000 (UTC)

This repurposes SETTINGS_ENABLE_PUSH as the initial limit.

A client can use this to limit the number of outstanding pushes.  I've included
advice on increasing the limit, not as the pushes are first used, but as the
pushes are completed (or cancelled).

There wasn't a rule for handling a truncated push stream header, so I added
that as well.  The HTTP_DUPLICATE_PUSH error was reclaimed for signaling all
three types of error with a push stream header: truncation, duplicate Push ID,
and exceeding MAX_PUSH_ID.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Limit pushes using MAX_PUSH_ID

-- File Changes --

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

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/711.patch
https://github.com/quicwg/base-drafts/pull/711.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/711

---

From: Martin Thomson <[email-masked]>
Date: Tue, 08 Aug 2017 01:51:52 -0700

@martinthomson pushed 1 commit.

7640b12  Fix error codes, prohibit reduction

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/711/files/f348e1c4a71e314b142f82030c4b1a200c3b4c03..7640b1248234efd28db914a92055a13393e2450f

---

From: Ryan Hamilton <[email-masked]>
Date: Tue, 08 Aug 2017 20:47:50 +0000 (UTC)

RyanAtGoogle commented on this pull request.

> @@ -794,6 +815,37 @@ the server MAY send another GOAWAY frame with an updated last stream identifier.
 This ensures that a connection can be cleanly shut down without losing requests.

+# MAX_PUSH_ID {#frame-max-push-id}
+
+The MAX_PUSH_ID frame (type=0xA) is used by clients to control the number of
+server pushes that the server can initiate.  This sets the maximum value for a
+Push ID that the server can use in a PUSH_PROMISE frame.  Consequently, this
+also limits the number of push streams that the server can initiate.

I'm not sure I'm following this completely. Clearly MAX_PUSH_ID limits the number of *promises* that the server can make. But isn't the number of actual push streams limited by the MAX_STREAM_ID? 

(This makes me wonder if "Promise ID" might be a better name than "Push ID" but I may well just be confused.)

-- 
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/711#pullrequestreview-55062620

---

From: Mike Bishop <[email-masked]>
Date: Tue, 08 Aug 2017 21:47:56 +0000 (UTC)

MikeBishop commented on this pull request.

You can't deliver a push that hasn't been promised, so the actual limit is whichever is smaller.

-- 
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/711#discussion_r132042047

---

From: Mike Bishop <[email-masked]>
Date: Tue, 08 Aug 2017 15:00:47 -0700

Generally good; minor nits other than the frame ID.

> @@ -362,9 +362,20 @@ This header consists of a 32-bit Push ID, which identifies a server push (see
 ~~~~~
 {: #fig-push-stream-header title="Push Stream Header"}
 
+A push stream always starts with a 32-bit Push ID.  A client MUST treat
+receiving a push stream that contains fewer than 4 octets as a connection error
+of type HTTP_MALFORMED_PUSH.
+
+A server SHOULD use Push IDs sequentially, starting at 0.  A client uses the
+MAX_PUSH_ID frame ({{frame-max-push-id}}) and SETTINGS_MAX_PUSH_ID

Nit:  I'd mention these in the order they're used; the setting to provide an initial value and the frame to increment it later.

> @@ -693,6 +708,12 @@ Push ID:
 Header Block:
 : HPACK-compressed request headers for the promised response.
 
+A server MUST NOT use a Push ID that is larger than the client has provided in a
+MAX_PUSH_ID frame ({{frame-max-push-id}}) or the SETTINGS_MAX_PUSH_ID setting
+({{settings-parameters}}).  A client that receives a PUSH_PROMISE that contains
+a larger Push ID than is allowed MUST be treated as a connection error of type
+HTTP_MALFORMED_PUSH_PROMISE.

"is allowed" makes it sound like an absolute limit; might be worth rewording slightly.

Note that you're now conflicting with the frame type for the ALTSVC frame in the HTTP/2 registry.  ORIGIN is using 0xC, which suggests someone has 0xB but I can't think of who off the top of my head right now.  Might want to jump out to at least 0xD to be safe, or (ab)use 0x8 instead.

+A server MUST NOT send a MAX_PUSH_ID frame.  A client MUST treat the receipt of
+a MAX_PUSH_ID frame as a connection error of type HTTP_MALFORMED_MAX_PUSH_ID.
+
+The initial value for the maximum Push ID is determined by the
+SETTINGS_MAX_PUSH_ID setting ({{settings-parameters}}).  A client that wishes to
+manage the number of promised server pushes can increase the maximum Push ID by
+sending a MAX_PUSH_ID frame as the server fulfills or cancels server pushes.
+
+The MAX_PUSH_ID frame has no defined flags.
+
+The MAX_PUSH_ID frame carries a 32-bit Push ID that identifies the maximum value
+for a Push ID that the server can use (see {{frame-push-promise}}).  A
+MAX_PUSH_ID frame cannot reduce the maximum Push ID; receipt of a MAX_PUSH_ID
+that contains a smaller value than previously received MUST be treated as a
+connection error of type HTTP_MALFORMED_MAX_PUSH_ID.

General comment:  Because these are on the control stream, which means guaranteed in-order delivery, we *could* use the HTTP/2 model of sending an increment rather than an absolute number.  I'm not sold that the few bytes saved are worth it, though.

-- 
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/711#pullrequestreview-55082288

---

From: Ryan Hamilton <[email-masked]>
Date: Tue, 08 Aug 2017 22:02:02 +0000 (UTC)

Good point. (Though I think it common practice we expect the promise limit to be higher than the stream limit so we would not expect this to be the lower of the two.) Perhaps the text could say:

Consequently, this also limits the number of push streams that the server can initiate in conjunction with MAX_STREAM_ID.

(or some such)

-- 
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/711#discussion_r132045017

---

From: Mike Bishop <[email-masked]>
Date: Tue, 08 Aug 2017 22:13:03 +0000 (UTC)

> @@ -857,14 +909,17 @@ 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_WRONG_STREAM (0x0F):
+: A frame was received on a different stream to where it is permitted.

"different... to" is British; "different... than" is American.  "Different... from" is neutral.  Maybe we should use that, or just sidestep the issue with "on a stream where it is not permitted."

-- 
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/711#pullrequestreview-55087621

---

From: Martin Thomson <[email-masked]>
Date: Wed, 09 Aug 2017 00:43:47 +0000 (UTC)

martinthomson commented on this pull request.

I think that we can make a bigger saving here by not sending the frame as often, noting that MAX_STREAM_ID covers the resource exhaustion issue.

-- 
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/711#discussion_r132067932

---

From: Martin Thomson <[email-masked]>
Date: Wed, 09 Aug 2017 00:44:55 +0000 (UTC)

I learn something every day.  I say that in its most Lovecraftian meaning, of course.

-- 
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/711#discussion_r132068043

---

From: Martin Thomson <[email-masked]>
Date: Wed, 09 Aug 2017 00:45:20 +0000 (UTC)

b6bcd43  Review comments

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/711/files/7640b1248234efd28db914a92055a13393e2450f..b6bcd43af27b55c2a035aee6a27a268999b1a813

---

From: Mike Bishop <[email-masked]>
Date: Wed, 09 Aug 2017 16:10:49 +0000 (UTC)

~~~~~
 
+A push stream always starts with a 32-bit Push ID.  A client MUST treat
+receiving a push stream that contains fewer than 4 octets as a connection error
+of type HTTP_MALFORMED_PUSH.
+
+A server SHOULD use Push IDs sequentially, starting at 0.  A client uses the
+SETTINGS_MAX_PUSH_ID ({{settings-parameters}}) setting and the MAX_PUSH_ID frame
+({{frame-max-push-id}}) to limit the number of pushes that a server can promise
+initiate.  A client MUST treat receipt of a push stream with a Push ID that is

"promise initiate" needs to have only one word or a conjunction.

-- 
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/711#pullrequestreview-55289662

---

From: janaiyengar <[email-masked]>
Date: Wed, 09 Aug 2017 22:01:02 +0000 (UTC)

janaiyengar commented on this pull request.

Thanks, Martin! Looks great. A few comments.

I don't think the savings are worth it, since this shouldn't be sent that often really as it is. 

On resource exhaustion, I think both MAX_STREAM_ID and MAX_PUSH_ID both cover that issue, just at different levels. QUIC decides MAX_STREAM_ID based on its resource constraints for streams, and the HTTP "layer" decides MAX_PUSH_ID based on its constraints for handling pushed resources. I think these are independent.

> @@ -857,14 +910,17 @@ HTTP_INTERRUPTED_HEADERS (0x0E):
 
-HTTP_SETTINGS_ON_WRONG_STREAM (0x0F):
-: A SETTINGS frame was received on a request control stream.
+HTTP_WRONG_STREAM (0x0F):
+: A frame was received on a different stream from where it is permitted.

from -> than ? I mostly don't care because it might be about local idioms, but it took me a moment to parse :-)
Offline, Ryan suggested "A frame was received on stream where it is not permitted", which SGTM as well (though it is semantically slightly different).

> @@ -632,8 +643,12 @@ The following settings are defined in HTTP/QUIC:
   SETTINGS_HEADER_TABLE_SIZE (0x1):
   : An integer with a maximum value of 2^32 - 1.  This value MUST be zero.
 
-  SETTINGS_ENABLE_PUSH (0x2):
-  : Transmitted as a Boolean
+  SETTINGS_MAX_PUSH_ID (0x2):
+  : An integer with a maximum value of 2^32 - 1.  This determines the initial
+    maximum Push ID that a server can use.  This setting only has meaning when
+    set by a client; a server MUST NOT provide a value for this setting.  A
+    client MUST treat receipt of this setting from a server as a connection
+    error of type HTTP_MALFORMED_SETTINGS.

Can this be 0? I assume so, since there's no other way to disable PUSH now. Can you state this explicitly here (that a client can disable push by setting this value to 0)?

> @@ -794,6 +815,38 @@ the server MAY send another GOAWAY frame with an updated last stream identifier.

+# MAX_PUSH_ID {#frame-max-push-id}
+
+The MAX_PUSH_ID frame (type=0xD) is used by clients to control the number of
+server pushes that the server can initiate.  This sets the maximum value for a
+Push ID that the server can use in a PUSH_PROMISE frame.  Consequently, this
+also limits the number of push streams that the server can initiate in addition
+to the limit set by the QUIC MAX_STREAM_ID frame.

It seems silly but possible to send this with 0. Can you add a sentence that says this MUST be > 0?

> +type HTTP_WRONG_STREAM.
+

I would explicitly state that this previous value may have been received via MAX_PUSH_ID or SETTINGS_MAX_PUSH_ID. Perhaps "a smaller value than that received via a previous MAX_PUSH_ID or SETTINGS_MAX_PUSH_ID ..." ? 

-- 
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/711#pullrequestreview-55379023

---

From: Martin Thomson <[email-masked]>
Date: Thu, 10 Aug 2017 00:46:11 +0000 (UTC)

I took Ryan's text, but I moved that to #712.  This PR will appear to be include the change, but that's just because I rebased on top of #712 on the assumption that it will be merged.

-- 
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/711#discussion_r132342017

---

From: janaiyengar <[email-masked]>
Date: Thu, 10 Aug 2017 00:48:17 +0000 (UTC)

Heh -- thanks for rephrasing, it's one of those trivial yet annoying things.

On Wed, Aug 9, 2017 at 5:46 PM, Martin Thomson <[email-masked]>
wrote:

> *@martinthomson* commented on this pull request.
> ------------------------------
>
> In draft-ietf-quic-http.md
> <https://github.com/quicwg/base-drafts/pull/711#discussion_r132342017>:
>
> I took Ryan's text, but I moved that to #712
> <https://github.com/quicwg/base-drafts/pull/712>. This PR will appear to
> be include the change, but that's just because I rebased on top of #712
> <https://github.com/quicwg/base-drafts/pull/712> on the assumption that
> it will be merged.
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/quicwg/base-drafts/pull/711#discussion_r132342017>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AKjg1CBuuPYrMA7q8eURZwd82JARTWNcks5sWlLTgaJpZM4OwaxD>
> .
>

-- 
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/711#issuecomment-321419821

---

From: Mike Bishop <[email-masked]>
Date: Wed, 16 Aug 2017 17:09:55 +0000 (UTC)

+

Sending a frame with a value less than you provided in transport settings is a clear error, actually, since you know the transport settings necessarily came before the frame.

-- 
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/711#discussion_r133510003

---

From: Mike Bishop <[email-masked]>
Date: Wed, 16 Aug 2017 17:11:42 +0000 (UTC)

@MikeBishop pushed 1 commit.

494a48f  Merge branch 'master' into push-id-seq

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/711/files/b38ae536b3440ac70850bc9237c490bb729b28aa..494a48f206e71955250a32a834b602f231335472

---

From: Mike Bishop <[email-masked]>
Date: Wed, 16 Aug 2017 10:13:07 -0700

Merged #711.

-- 
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/711#event-1208701887