Push ID #701

martinthomson · 2017-07-27T06:23:37Z

As requested, I split this out of the unidirectional PR (#643).

There is a change here, that needs to be highlighted. @kazuho points out that without inter-stream ordering guarantees, requests might be triggered on multiple streams. The server then has an awkward choice: either push multiple times, or risk the client triggering requests when streams arrive out of order. This contains a fix for that bug.

The fix is to allow the same push ID to be used in multiple PUSH_PROMISE frames. Then, fulfillment happens at most once.

I think that this is a neat fix, but it creates two more problems:

How does a client know when the server will stop referencing a push ID in PUSH_PROMISE? With reordering, the pushed response might arrive and be consumed before a PUSH_PROMISE that references it. I think that it might be possible to add a counter to the push stream header that says how many times the server promised this response. The problem with that is that if a stream that carries the PUSH_PROMISE is reset, then that counter will be too high. I've decided to just explain the issue and add some weasel wording rather than provide a direct fix.
What if the server provides different headers for the same Push ID? I've chosen to fix this by simply forbidding variations in the content of the header block. I considered only allowing the headers to appear once, but then you have to deal with that stream being reset. Repetition should be rare enough, and header compression might be able to help with the extra bytes.

I think that those problems are both less serious than the bug this fixes. Besides, the first is really only an generalized form of a problem we have already: the stream carrying a PUSH_PROMISE can be reset. What currently happens is that the server more or less has to send a push stream anyway, even though it can't know whether the client got the PUSH_PROMISE. With this, a server can use CANCEL_REQUEST.

Closes #702, #281.

MikeBishop · 2017-07-27T17:35:17Z

I'll note that, with unidirectional streams, we could use a stream from the server for the request headers as well, and then only have them once. The problem with that is that, until they're processed, the client doesn't know what request it is that it shouldn't be making as it processes the original resource.

mcmanus · 2017-07-27T19:42:35Z

draft-ietf-quic-http.md

+prior to the response stream being created.  This frame can be used to cancel
+server push requests that are initiated with a PUSH_PROMISE.
+
+The frame identifies a server push request is identified by Push ID (see


There is a typo in here somewhere.. "request that is identified by" perhaps?

martinthomson · 2017-07-28T01:01:18Z

@MikeBishop, I considered that. Getting the ordering guarantees right here is tricky and I think that it's unfair on clients to say "I'm going to promise you something", but not tell them exactly what.

MikeBishop

All my feedback is editorial -- the core of the change looks good.

MikeBishop · 2017-07-28T16:32:13Z

draft-ietf-quic-http.md

-frames, and response body (if any) carried by DATA frames.
+After the push stream header, a push contains a response ({{request-response}}),
+with response headers and (if present) trailers carried by HEADERS frames sent
+on the control stream, and response body (if any) carried by DATA frames.


Not on the control stream.

Heh, you noticed that, but then you didn't notice that the ordering was completely wrong :)

MikeBishop · 2017-07-28T16:35:13Z

draft-ietf-quic-http.md

+
+  PUSH_DEPENDENT (0x02):
+  : Indicates a dependency on a pushed request.  If set, this flag indicates
+    that the Dependent Stream identifies a response stream and that the


Saying that it "identifies a response stream" in both these places feels a little odd. If it's the stream ID, then just say it's the stream ID and you don't even need the flags. If it's the Push ID, use the term here for clarity.

I think that I can remove the second sentence and rely on the description of the fields below.

MikeBishop · 2017-07-28T16:35:51Z

draft-ietf-quic-http.md

-    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


PUSH => PUSH_PRIORITIZED?

MikeBishop · 2017-07-28T16:37:51Z

draft-ietf-quic-http.md

+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 by using a stream ID of 0; as in


Since there are two references in a PRIORITY frame, and setting one of them to zero is prohibited by your other pull request, perhaps be more specific than "identify" here.

MikeBishop · 2017-07-28T16:41:49Z

draft-ietf-quic-http.md

+### CANCEL_REQUEST {#frame-cancel-request}
+
+The CANCEL_REQUEST frame (type=0x3) is used to request cancellation of a request
+prior to the response stream being created.  This frame can be used to cancel


Given that there aren't (currently 😉) separate request/response streams, this basically can only be used to cancel a push. Let's call it CANCEL_PUSH for now and not talk about requests in this section.

MikeBishop · 2017-07-28T16:42:49Z

draft-ietf-quic-http.md

+
+A server can send this frame to indicate that it won't be sending a response
+prior to creation of a push stream.  Once the push stream has been created, a
+RST_STREAM with a CANCELLED code can be used instead.


"can" feels like it needs an RFC2119 WORD, but I don't know which.

I've gone with SHOULD. There isn't any point in MUST here, because it's not a correctness issue, it's just pointless to use CANCEL_PUSH at this point.

MikeBishop · 2017-07-28T16:44:02Z

draft-ietf-quic-http.md

+RST_STREAM with a CANCELLED code can be used instead.
+
+A CANCEL_REQUEST frame is sent on the control stream.  Sending a CANCEL_REQUEST
+frame on a stream other than the control stream MUST be treated as a


"stream error of type X" avoids the argument of whether to do a/an before HTTP.

MikeBishop · 2017-07-28T16:44:43Z

draft-ietf-quic-http.md

+   |                         Push ID (32)                          |
+   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+~~~~~~~~~~
+{: #fig-cancel-request title="CANCEL_REQUEST frame payload"}


I've been trying to avoid drawings of things that have only a single field and call that single thing the payload of the frame.

MikeBishop · 2017-07-28T16:45:42Z

draft-ietf-quic-http.md

-  : HPACK-compressed request headers for the promised response.
+Push ID:
+: A 32-bit identifier for the server push request.  This identifier is used in
+  push stream header ({{server-push}}), CANCEL_REQUEST frames


"in the" or "in ... headers". "In ... header" sounds odd.

MikeBishop · 2017-07-28T16:51:04Z

Oh, and fill in the changelog.

MikeBishop · 2017-08-02T22:00:30Z

draft-ietf-quic-http.md

+    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

MikeBishop · 2017-08-02T22:03:00Z

draft-ietf-quic-http.md

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

MikeBishop · 2017-08-02T22:03:45Z

draft-ietf-quic-http.md

+### 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

ianswett

One nit.

ianswett · 2017-08-03T00:58:59Z

draft-ietf-quic-http.md

+({{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."

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

janaiyengar

A bunch of editorial comments, and one material one.

I think we need to be able to handle potential reordering between PUSH_PROMISE and CANCEL_PUSH.
I don't think it makes sesne to say "push response" and "server push request", so I propose "promised resource".
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.

janaiyengar · 2017-08-03T23:09:29Z

draft-ietf-quic-http.md

+({{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.


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

janaiyengar · 2017-08-03T23:28:33Z

draft-ietf-quic-http.md

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

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.

janaiyengar · 2017-08-03T23:32:05Z

draft-ietf-quic-http.md

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


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

janaiyengar · 2017-08-03T23:32:07Z

draft-ietf-quic-http.md

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

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

janaiyengar · 2017-08-03T23:35:20Z

draft-ietf-quic-http.md

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

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

janaiyengar · 2017-08-04T00:06:54Z

draft-ietf-quic-http.md

-  : 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"

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.

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

janaiyengar · 2017-08-04T00:07:22Z

draft-ietf-quic-http.md

+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"

janaiyengar · 2017-08-04T00:10:33Z

draft-ietf-quic-http.md

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

That's a completely different statement.

janaiyengar · 2017-08-04T00:11:29Z

draft-ietf-quic-http.md

+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?

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.

janaiyengar · 2017-08-04T00:12:52Z

draft-ietf-quic-http.md


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

MikeBishop · 2017-08-04T06:26:04Z

draft-ietf-quic-http.md

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

MikeBishop · 2017-08-04T06:27:34Z

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

martinthomson · 2017-08-04T06:59:13Z

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.

janaiyengar

LGTM. Looking forward to the follow on.

janaiyengar · 2017-08-05T01:02:49Z

draft-ietf-quic-http.md

-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


martinthomson added -http design An issue that affects the design of the protocol; resolution requires consensus. labels Jul 27, 2017

mcmanus reviewed Jul 27, 2017

View reviewed changes

martinthomson force-pushed the push-id branch from bbc331b to 58c67a3 Compare July 28, 2017 01:09

MikeBishop approved these changes Jul 28, 2017

View reviewed changes

martinthomson force-pushed the push-id branch from 58c67a3 to 5f1ebb5 Compare July 31, 2017 01:54

MikeBishop reviewed Aug 2, 2017

View reviewed changes

MikeBishop added the merge-imminent label Aug 3, 2017

ianswett reviewed Aug 3, 2017

View reviewed changes

janaiyengar reviewed Aug 4, 2017

View reviewed changes

martinthomson added 4 commits August 4, 2017 11:59

Define a Push ID

16b12a1

Mike's editorial comments

4b3fb33

Editorial cleanup

7343bd8

Edit's inspired by Jana's review

cb44d94

martinthomson force-pushed the push-id branch from 794d914 to cb44d94 Compare August 4, 2017 02:03

MikeBishop reviewed Aug 4, 2017

View reviewed changes

hyphenate

d7b14bf

MikeBishop merged commit 2a65451 into master Aug 4, 2017

MikeBishop deleted the push-id branch August 4, 2017 20:45

janaiyengar reviewed Aug 5, 2017

View reviewed changes

martinthomson mentioned this pull request Aug 14, 2017

Promising more than the client concurrency limit #281

Closed

MikeBishop mentioned this pull request Nov 11, 2017

Applications and Stream IDs #910

Closed

LPardue mentioned this pull request Mar 18, 2019

PUSH_ID as a frame #2526

Closed

Push ID #701

Push ID #701

Conversation

martinthomson commented Jul 27, 2017 • edited

MikeBishop commented Jul 27, 2017

Choose a reason for hiding this comment

martinthomson commented Jul 28, 2017

MikeBishop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeBishop commented Jul 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianswett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janaiyengar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeBishop commented Aug 4, 2017

martinthomson commented Aug 4, 2017

janaiyengar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinthomson commented Jul 27, 2017 •

edited