Subject: Re: [quicwg/base-drafts] Push ID (#701)

From: Mike Bishop <[email-masked]>
Date: Wed, 02 Aug 2017 22:00:32 +0000 (UTC)

MikeBishop commented on this pull request.

   Stream Dependency:
-  : A 32-bit stream identifier for the request stream that this stream depends
-    on (see {{priority}} and {{!RFC7540}}, Section 5.3).
+  : A 32-bit stream identifier for a dependent request.  This contains the
+    stream ID of a request stream when the PUSH_DEPENDENT flag is clear, or a
+    Push ID when the PUSH_DEPENDENT flag is set.  A request stream ID of 0
+    indicates a dependency on the root stream. For details of dependencies,
+    see {{priority}} and {!RFC7540}}, Section 5.3.

Unbalanced braces

-- 
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/701#pullrequestreview-53945691

---

From: Mike Bishop <[email-masked]>
Date: Wed, 02 Aug 2017 22:05:12 +0000 (UTC)

Weight:
   : An unsigned 8-bit integer representing a priority weight for the stream (see
     {{!RFC7540}}, Section 5.3). Add one to the value to obtain a weight between
     1 and 256.
 
+A PRIORITY frame identifies a request to priotize, and a request upon which that
+request is dependent.  A request is identified by the stream ID of a request
+when the corresponding PUSH_PRIORITIZED or PUSH_DEPENDENT flag is not set.
+Setting the PUSH or PUSH_DEPENDENT flag causes the frame to identify a
+PUSH_PROMISE using a Push ID (see {{frame-push-promise}} for details).
+
+A PRIORITY frame MAY identify no request in the Prioritized Request field by
+using a stream ID of 0; as in {{!RFC7540}}, this makes the request dependent on
+the root of the dependency tree.

"Prioritized Request" is the request whose priority is being modified.  Per the following paragraph, this cannot be zero (or one).  I think you can delete this paragraph / merge it with the following one.

>  The length of a PRIORITY frame is 9 octets.  A PRIORITY frame with any other
 length MUST be treated as a connection error of type HTTP_MALFORMED_PRIORITY.

+### CANCEL_PUSH {#frame-cancel-push}
+
+The CANCEL_PUSH frame (type=0x3) is used to request cancellation of server push
+prior to the push stream being created.  The CANCEL_REQUEST frame identifies a

CANCEL_REQUEST => CANCEL_PUSH

-- 
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/701#pullrequestreview-53946202

---

From: Martin Thomson <[email-masked]>
Date: Thu, 03 Aug 2017 00:01:50 -0700

@martinthomson pushed 1 commit.

6813597  Editorial cleanup

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/701/files/5f1ebb5ac2ea89439c1eb4d9ffe391abb740eb09..6813597969768f9e88a5c7f505e402b344f94177

---

From: Mike Bishop <[email-masked]>
Date: Thu, 03 Aug 2017 17:25:56 +0000 (UTC)

@MikeBishop pushed 1 commit.

794d914  Merge remote-tracking branch 'origin/master' into push-id

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/701/files/6813597969768f9e88a5c7f505e402b344f94177..794d9146bc132b558fe8d510cb06f2b612943f84

---

From: ianswett <[email-masked]>
Date: Thu, 03 Aug 2017 20:12:18 +0000 (UTC)

ianswett commented on this pull request.

One nit.

> -form a dependency tree. The structure of the dependency tree changes as PRIORITY
-frames add, remove, or change the dependency links between streams.
+this priority scheme, a given request can be designated as dependent upon
+another request, which expresses the preference that the latter stream (the
+"parent" request) be allocated resources before the former stream (the
+"dependent" request). Taken together, the dependencies across all requests in a
+connection form a dependency tree. The structure of the dependency tree changes
+as PRIORITY frames add, remove, or change the dependency links between requests.
+
+HTTP/2 defines its priorities in terms of streams whereas HTTP over QUIC
+identifies requests.  The PRIORITY frame {{frame-priority}} identifies a request
+either by identifying the stream that carries a request or by using a Push ID
+({{frame-push-promise}}).  Other than the means of identifying requests, the
+prioritization system is identical to that in HTTP/2.
+
+Only a client can prioritize requests.  A server MUST NOT send a PRIORITY frame.

Not to be pedantic, but a server can definitely prioritize requests, even if it's not via the PRIORITY frame.  How about saying "Only the client can SEND a PRIORITY 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/701#pullrequestreview-53971824

---

From: janaiyengar <[email-masked]>
Date: Thu, 03 Aug 2017 17:15:57 -0700

janaiyengar commented on this pull request.

A bunch of editorial comments, and one material one.
1. I think we need to be able to handle potential reordering between PUSH_PROMISE and CANCEL_PUSH.
2. I don't think it makes sesne to say "push response" and "server push request", so I propose "promised resource".
3. There's awkward phrasing around Prioritized Request and Dependent Request in text surrounding the PRIORITY frame. I propose adding "ID" to these field names to make the text less awkward.

+1. I think it's more precise to say PRIORITY frame. 
FWIW, I think it's a good idea that we're adding this requirement to avoid ambiguity and further down below, to avoid having to deal with reordering between PUSH_PROMISE and PRIORITY frames from server to client.

> @@ -402,15 +431,25 @@ without EHB set, followed by a HEADERS frame with EHB set.
 ### PRIORITY {#frame-priority}
 
 The PRIORITY (type=0x02) frame specifies the sender-advised priority of a stream
-and is substantially different from {{!RFC7540}}. In order to support ordering,
-it MUST be sent only on the control stream. The format has been modified to
-accommodate not being sent on-stream and the larger stream ID space of QUIC.
+and is substantially different in format from {{!RFC7540}}.  In order to ensure

nit: remove "substantially"

   Weight:
 
-A PRIORITY frame MAY identify a dependent stream with a stream ID of 0; as in

I don't think it identifies the PUSH_PROMISE, but the promised resource, right?

   Weight:
 
-A PRIORITY frame MAY identify a dependent stream with a stream ID of 0; as in
+A PRIORITY frame identifies a request to priotize, and a request upon which that
+request is dependent.  A request is identified by the stream ID of a request
+when the corresponding PUSH_PRIORITIZED or PUSH_DEPENDENT flag is not set.

I'm not sure I understand this sentence. Can you rephrase? Specifically "A request is identified by the stream ID of a request" is really awkward. Perhaps saying "prioritized request" and "dependent request" might help? I believe what you're trying to say is: "A prioritized request is identified by its stream ID when the corresponding PRIORITY frame does not have PUSH_PRIORITIZED or PUSH_DEPENDENT flags set. Setting these flags causes the PRIORITY frame to identify a pushed resource using a Push ID (see {{frame-push-promise}} for details)." WDYT?

> @@ -430,29 +469,79 @@ The flags defined are:
 
 The PRIORITY frame payload has the following fields:
 
-  Prioritized Stream:
-  : A 32-bit stream identifier for the request stream whose priority is being
-    updated.
+  Prioritized Request:
+  : A 32-bit identifier for a request.  This contains the stream ID of a request
+    stream when the PUSH_PRIORITIZED flag is clear, or a Push ID when the
+    PUSH_PRIORITIZED flag is set.
 
   Stream Dependency:

This should be Dependent Request. "Stream Dependency" is not a field in the frame.

   Weight:
 
-A PRIORITY frame MAY identify a dependent stream with a stream ID of 0; as in
+A PRIORITY frame identifies a request to priotize, and a request upon which that
+request is dependent.  A request is identified by the stream ID of a request
+when the corresponding PUSH_PRIORITIZED or PUSH_DEPENDENT flag is not set.
+Setting the PUSH or PUSH_DEPENDENT flag causes the frame to identify a
+PUSH_PROMISE using a Push ID (see {{frame-push-promise}} for details).
+
+A PRIORITY frame MAY identify a Stream Dependency with a stream ID of 0; as in

Suggestion: "A PRIORITY frame MAY identify a Dependent Request ID of 0; ...". I'd prefer this because the value is not always stream ID.

>     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-   |                    Dependent Stream (32)                      |
+   |                    Dependent Request (32)                     |

I'll propose that you add ID to both these fields. (Prioritized Request ID and Dependent Request ID). It makes references to these identifiers in the text easier to write.

>  {{!RFC7540}}, this makes the request dependent on the root of the dependency
-tree.  Stream ID 0 and stream ID 1 cannot be reprioritized; an attempt to
-reprioritize these stream MUST be treated as a connection error of type
+tree.
+
+Stream ID 0 and stream ID 1 cannot be reprioritized. A Prioritized Request that
+identifies Stream 0 or 1 MUST be treated as a connection error of type

ditto -- "A Prioritized Request ID of 0 or 1"

+### CANCEL_PUSH {#frame-cancel-push}
+
+The CANCEL_PUSH frame (type=0x3) is used to request cancellation of server push
+prior to the push stream being created.  The CANCEL_PUSH frame identifies a
+server push request by Push ID (see {{frame-push-promise}}).

how about "promised resource" instead of "server push request"?

+### CANCEL_PUSH {#frame-cancel-push}
+
+The CANCEL_PUSH frame (type=0x3) is used to request cancellation of server push
+prior to the push stream being created.  The CANCEL_PUSH frame identifies a
+server push request by Push ID (see {{frame-push-promise}}).
+
+When a server receives this frame, it aborts sending the response for the
+identified server push.  If the server has not yet started to send the server

similarly, "sending the promised resource"

> +sending CANCEL_PUSH has no effect on the state of the push stream.  A QUIC
+RST_STREAM frame SHOULD be used instead to cancel transmission of the server
+push response.
+
+A CANCEL_PUSH frame is sent on the control stream.  Sending a CANCEL_PUSH frame
+on a stream other than the control stream MUST be treated as a stream error of
+type HTTP_WRONG_STREAM.
+
+The CANCEL_PUSH frame has no defined flags.
+
+The CANCEL_PUSH frame carries a 32-bit Push ID that identifies the server push
+that is being cancelled (see {{frame-push-promise}}).
+
+A server MUST treat a CANCEL_PUSH frame payload that is other than 4 octets in
+length, or a CANCEL_PUSH frame that identifies an unknown Push ID as a
+connection error of type HTTP_MALFORMED_CANCEL_PUSH.

A CANCEL_PUSH from a server could be reordered with a PUSH_PROMISE... I'm not sure what to do about this but I wanted to raise it.

> @@ -583,12 +673,34 @@ server to client, as in HTTP/2.  It defines no flags.
 
 The payload consists of:
 
-  Promised Stream ID:
-  : A 32-bit Stream ID indicating the QUIC stream on which the response will be
-    sent
+Push ID:
+: A 32-bit identifier for the server push request.  A push ID is used in push

It really sounds odd to say "server push request". It's not a request... how about "promised resource"?

-  Payload:
-  : HPACK-compressed request headers for the promised response.
+A server MAY use the same Push ID in multiple PUSH_PROMISE frames.  This allows
+the server to use the same server push in response to multiple concurrent
+requests.  Referencing the same server push ensures that a PUSH_PROMISE can be

"Referencing the same promised resource"

-  Payload:
-  : HPACK-compressed request headers for the promised response.
+A server MAY use the same Push ID in multiple PUSH_PROMISE frames.  This allows
+the server to use the same server push in response to multiple concurrent
+requests.  Referencing the same server push ensures that a PUSH_PROMISE can be
+made in relation to every response in which server push might be needed without
+causing duplicating pushes.

"causing duplicating pushes" --> "duplicating pushes"

> +A server MAY use the same Push ID in multiple PUSH_PROMISE frames.  This allows
+the server to use the same server push in response to multiple concurrent
+requests.  Referencing the same server push ensures that a PUSH_PROMISE can be
+made in relation to every response in which server push might be needed without
+causing duplicating pushes.
+
+A server that uses the same Push ID in multiple PUSH_PROMISE frames MUST include
+the same header fields each time.  The octets of the header block MAY be
+different due to differing encoding, but the header fields and their values MUST
+be identical.  Note that ordering of header fields is significant.  A client
+MUST treat receipt of a PUSH_PROMISE with conflicting header field values for
+the same Push ID as a connection error of type HTTP_MALFORMED_PUSH_PROMISE.
+
+A server SHOULD avoid sending a PUSH_PROMISE that includes a Push ID that was
+fulfilled prior to the request being made.  Though a client needs to handle
+receiving a promised response prior to it being promised due to reordering of

Suggested rewording: "Due to the possibility of network reordering, a client must be able to receive a promised resource prior to the corresponding PUSH_PROMISE."

> +requests.  Referencing the same server push ensures that a PUSH_PROMISE can be
+made in relation to every response in which server push might be needed without
+causing duplicating pushes.
+
+A server that uses the same Push ID in multiple PUSH_PROMISE frames MUST include
+the same header fields each time.  The octets of the header block MAY be
+different due to differing encoding, but the header fields and their values MUST
+be identical.  Note that ordering of header fields is significant.  A client
+MUST treat receipt of a PUSH_PROMISE with conflicting header field values for
+the same Push ID as a connection error of type HTTP_MALFORMED_PUSH_PROMISE.
+
+A server SHOULD avoid sending a PUSH_PROMISE that includes a Push ID that was
+fulfilled prior to the request being made.  Though a client needs to handle
+receiving a promised response prior to it being promised due to reordering of
+stream delivery, clients might be unable to reuse a pushed response that was
+long since consumed. \[Editor's Note: should we include a count of the number of

Not sure what this phrase means "clients might be unable to reuse a pushed response that was long since consumed." Can you clarify?

 SETTINGS (0x4):
 : SETTINGS frames are sent only at the beginning of the connection.  See
   {{frame-settings}} and {{h2-settings}}.
 
 PUSH_PROMISE (0x5):
-: See {{frame-push-promise}}.
+: The PUSH_PROMISE does not reference the stream that carries the response to
+  the pushed request; instead the push stream references the PUSH_PROMISE frame.

reword. suggestion: "The PUSH_PROMISE does not reference the stream carrying the promised resource; instead the push stream references the PUSH_PROMISE 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/701#pullrequestreview-54246728

---

From: Martin Thomson <[email-masked]>
Date: Fri, 04 Aug 2017 01:44:46 +0000 (UTC)

martinthomson commented on this pull request.

 
-  Prioritized Stream:
 
   Stream Dependency:

Hmm, I think that "dependent request" means the stream that is dependent, so I changed the name.  I'll fix the picture.

-- 
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/701#discussion_r131296932

---

From: Martin Thomson <[email-masked]>
Date: Thu, 03 Aug 2017 18:48:22 -0700

Weight:

I've reworded a little.  I've used the field names more explicitly.  I didn't want to use "prioritized request" because that is too close to "Prioritized Request 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/701#discussion_r131297245

---

From: Martin Thomson <[email-masked]>
Date: Thu, 03 Aug 2017 18:49:31 -0700

Weight:

That would only work if we prohibit a Push ID of 0.

-- 
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/701#discussion_r131297342

---

From: Martin Thomson <[email-masked]>
Date: Thu, 03 Aug 2017 18:50:28 -0700

The resource is the thing identified by a URL.  I'm trying to be consistent here by always identifying requests, either those made by the client, or those synthesized/predicted by the server.

-- 
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/701#discussion_r131297405

---

From: Martin Thomson <[email-masked]>
Date: Fri, 04 Aug 2017 01:52:36 +0000 (UTC)

That's a completely different statement.

-- 
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/701#discussion_r131297577

---

From: Martin Thomson <[email-masked]>
Date: Fri, 04 Aug 2017 01:54:06 +0000 (UTC)

A server might promise with Push ID X and fulfill that promise.  Later, it could send the same PUSH_PROMISE for X and assume that the client already has the response.  A client can't be expected to retain the response.  I have a different proposal for handling this now that I've had time away from the problem, I'll write that down.

-- 
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/701#discussion_r131297698

---

From: Mike Bishop <[email-masked]>
Date: Thu, 03 Aug 2017 23:21:10 -0700

This is an echo of language elsewhere in the HTTP draft, indicating which frames are mostly-direct copies of the HTTP/2 equivalent, and which ones are fundamentally different.  Happy to discuss rephrasing that all-up in a separate editorial issue, but I think following the existing pattern in this PR is entirely appropriate.

-- 
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/701#discussion_r131319051

---

From: Mike Bishop <[email-masked]>
Date: Fri, 04 Aug 2017 06:22:18 +0000 (UTC)

The payload consists of:
 
-  Promised Stream ID:

It is a request -- the PUSH_PROMISE contains a request body that the server is telling the client it expects the client will want to make shortly.

-- 
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/701#discussion_r131319192

---

From: Mike Bishop <[email-masked]>
Date: Thu, 03 Aug 2017 23:23:09 -0700

At least one other place, you refer simply to "the push" -- after some initial definition, that might be less awkward, though it feels somewhat informal.

-- 
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/701#discussion_r131319268

---

From: Mike Bishop <[email-masked]>
Date: Fri, 04 Aug 2017 06:24:19 +0000 (UTC)

-  Payload:

Technically, there's nothing stopping you from having two different pushes for the same resource in flight at once.  I'm drawing a blank on why you would do that, but I'm not sure we want to foreclose it here.

-- 
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/701#discussion_r131319405

---

From: Mike Bishop <[email-masked]>
Date: Thu, 03 Aug 2017 23:26:06 -0700

> @@ -494,17 +495,19 @@ The PRIORITY frame payload has the following fields:
     1 and 256.
 
 A PRIORITY frame identifies a request to priotize, and a request upon which that
-request is dependent.  A request is identified by the stream ID of a request
-when the corresponding PUSH_PRIORITIZED or PUSH_DEPENDENT flag is not set.
-Setting the PUSH or PUSH_DEPENDENT flag causes the frame to identify a
-PUSH_PROMISE using a Push ID (see {{frame-push-promise}} for details).
-
-A PRIORITY frame MAY identify a Stream Dependency with a stream ID of 0; as in
-{{!RFC7540}}, this makes the request dependent on the root of the dependency
+request is dependent.  A Prioritized Request ID or Stream Dependency ID
+identifies a client initiated request using the corresponding stream ID when the

I believe client-initiated should be hyphenated.

-- 
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/701#pullrequestreview-54288088

---

From: Mike Bishop <[email-masked]>
Date: Fri, 04 Aug 2017 06:27:36 +0000 (UTC)

Random question while reading -- is '0' a valid Push ID?  The answer isn't entirely obvious, and I could see implementations differing painfully.

-- 
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/701#issuecomment-320167454

---

From: Martin Thomson <[email-masked]>
Date: Thu, 03 Aug 2017 23:58:10 -0700

d7b14bf  hyphenate

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/701/files/cb44d94019cfa2cc519f0394afbbcebcc2ab07a2..d7b14bf0b4348dce65e80ed4a1bbe6f693733a6b

---

From: Martin Thomson <[email-masked]>
Date: Fri, 04 Aug 2017 06:58:37 +0000 (UTC)

-  Payload:

That's another reason to talk about "requests" rather than resources.

-- 
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/701#discussion_r131323651

---

From: Martin Thomson <[email-masked]>
Date: Fri, 04 Aug 2017 06:59:15 +0000 (UTC)

I hope to fix the 0 thing in a follow-on.  See my response to Jana on the mailing list.  Right now, the entire 32-bit space is open for business.

-- 
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/701#issuecomment-320173143

---

From: Mike Bishop <[email-masked]>
Date: Fri, 04 Aug 2017 20:45:31 +0000 (UTC)

Merged #701.

-- 
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/701#event-1193788881

---

From: janaiyengar <[email-masked]>
Date: Fri, 04 Aug 2017 18:05:30 -0700

LGTM. Looking forward to the follow on.

     1 and 256.

+1

-- 
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/701#pullrequestreview-54503997