Subject: [quicwg/base-drafts] Remove DUPLICATE_PUSH and allow duplicate PUSH_PROMISE (#3309)

From: Ryan Hamilton <[email-masked]>
Date: Fri, 20 Dec 2019 08:06:25 -0800

As proposed on the issue, this removes the DUPLICATE_PUSH frame and permits duplicate PUSH_PROMISE frames which are required to have identical headers.

Fixes #3275.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Remove DUPLICATE_PUSH and allow duplicate PUSH_PROMISE

-- File Changes --

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

-- Patch Links --

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

---

From: Ryan Hamilton <[email-masked]>
Date: Fri, 20 Dec 2019 09:10:36 -0800

@RyanAtGoogle pushed 1 commit.

d91dd854b95329493dd6fb9e5833989293a58ba9  wrap

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3309/files/22782188e6c53fd1fd50ad584db9c868963bbed9..d91dd854b95329493dd6fb9e5833989293a58ba9

---

From: ianswett <[email-masked]>
Date: Fri, 20 Dec 2019 09:18:28 -0800

ianswett approved this pull request.

I think this LG, but certainly @MikeBishop should take a look

> -become available, or can initiate requests for discovered resources and cancel
-the requests if the requested resource is already being pushed. When a client
-receives a new push stream with an as-yet-unknown Push ID, both the associated
-client request and the pushed request headers are unknown.  The client can
-buffer the stream data in expectation of the matching PUSH_PROMISE. The client
-can use stream flow control (see section 4.1 of {{QUIC-TRANSPORT}}) to limit the
-amount of data a server may commit to the pushed stream.
+Due to reordering, push stream data can arrive before the corresponding
+PUSH_PROMISE frame.  When a client receives a new push stream with an
+as-yet-unknown Push ID, both the associated client request and the pushed
+request headers are unknown.  The client can buffer the stream data in
+expectation of the matching PUSH_PROMISE. The client can use stream flow control
+(see section 4.1 of {{QUIC-TRANSPORT}}) to limit the amount of data a server may
+commit to the pushed stream.
+
+If the same Push ID is promised on multiple request streams, the decompressed

I might be inclined to put this at the end of the paragraph on lines 623/624?

-- 
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/3309#pullrequestreview-335391434

---

From: Lucas Pardue <[email-masked]>
Date: Fri, 20 Dec 2019 11:16:55 -0800

LPardue commented on this pull request.

Didnt we say on the issue that push promise  should include a URL in order to improve the chances of detecting conflicts if we remove duplicate 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/3309#pullrequestreview-335468778

---

From: Ryan Hamilton <[email-masked]>
Date: Fri, 20 Dec 2019 12:54:37 -0800

> Didnt we say on the issue that push promise should include a URL in order to improve the chances of detecting conflicts if we remove duplicate push?

The PUSH_PROMISE includes the full request headers, including the URL, right?

-- 
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/3309#issuecomment-568083521

---

From: Lucas Pardue <[email-masked]>
Date: Fri, 20 Dec 2019 12:56:55 -0800

Oh oops :D

-- 
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/3309#issuecomment-568085098

---

From: Kazuho Oku <[email-masked]>
Date: Fri, 20 Dec 2019 15:36:20 -0800

kazuho commented on this pull request.

LGTM modulo one editorial point.

> @@ -603,11 +602,10 @@ client making the indicated request.  This trades off network usage against a
 potential latency gain.  HTTP/3 server push is similar to what is described in
 HTTP/2 {{!HTTP2}}, but uses different mechanisms.
 
-Each server push is identified by a unique Push ID. This Push ID is used in a
-single PUSH_PROMISE frame (see {{frame-push-promise}}) which carries the request
-headers, possibly included in one or more DUPLICATE_PUSH frames (see
-{{frame-duplicate-push}}), then included with the push stream which ultimately
-fulfills those promises.
+Each server push is identified by a unique Push ID. This Push ID is used in all
+PUSH_PROMISE frames (see {{frame-push-promise}}) which carries the request
+headers, then included with the push stream which ultimately fulfills those
+promises.

It might be better to more explicit to say "one or more" than "all", like following, clarifying that the values MUST be the same when sent more than once.

 _This Push ID is used in one or more PUSH_PROMISE frames (see {{frame-push-promise}}) that carry the request headers, then included with the push stream which ultimately fulfills those promises. When the same Push ID is promised on multiple request streams, the decompressed request header fields MUST be byte-for-byte identical._

-- 
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/3309#pullrequestreview-335565662

---

From: Ryan Hamilton <[email-masked]>
Date: Fri, 20 Dec 2019 18:27:50 -0800

d7668a54c20039ccfd6dc7132e55c0f0ffc87047  one or more

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

---

From: Ryan Hamilton <[email-masked]>
Date: Fri, 20 Dec 2019 18:28:26 -0800

RyanAtGoogle commented on this pull request.

Good call. Done.

-- 
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/3309#discussion_r360623716

---

From: Mike Bishop <[email-masked]>
Date: Fri, 27 Dec 2019 08:03:51 -0800

MikeBishop requested changes on this pull request.

This basically puts it back to the way it was before, and thereby reintroduces the problems we were trying to get away from then.  Those problems aren't insurmountable, so let's fix them in this PR.

> @@ -379,10 +379,9 @@ An HTTP message (request or response) consists of:
 3. optionally, trailing headers, if present (see {{!RFC7230}}, Section 4.1.2),
    sent as a single HEADERS frame.
 
-A server MAY send one or more PUSH_PROMISE (see {{frame-push-promise}}) or
-DUPLICATE_PUSH (see {{frame-duplicate-push}}) frames before, after, or
-interleaved with the frames of a response message. These PUSH_PROMISE and
-DUPLICATE_PUSH frames are not part of the response; see {{server-push}} for
+A server MAY send one or more PUSH_PROMISE (see {{frame-push-promise}}) frames
+frames before, after, or interleaved with the frames of a response message.

```suggestion
before, after, or interleaved with the frames of a response message.
```

> @@ -603,11 +602,11 @@ client making the indicated request.  This trades off network usage against a
 
-Each server push is identified by a unique Push ID. This Push ID is used in a
-single PUSH_PROMISE frame (see {{frame-push-promise}}) which carries the request
-headers, possibly included in one or more DUPLICATE_PUSH frames (see
-{{frame-duplicate-push}}), then included with the push stream which ultimately
-fulfills those promises.
+Each server push is identified by a unique Push ID. This Push ID is used in one
+or more PUSH_PROMISE frames (see {{frame-push-promise}}) that carry the request
+headers, then included with the push stream which ultimately fulfills those
+promises. When the same Push ID is promised on multiple request streams, the
+decompressed request header fields MUST be byte-for-byte identical.

Technically, this is probably true, because odds are good the data needs to match regardless of whether you're serializing to text or to an internal object.  But without knowing details of internal objects, I don't know for sure what it means to be byte-for-byte identical.  We might want to frame this in terms of structure and contents instead.

I think what it means is that the header set contains the same set of fields in the same order, and that in each field both the name and and value are byte-for-byte matches.

> @@ -623,31 +622,28 @@ allows the server push to be associated with a client request.  Promised
 requests MUST conform to the requirements in Section 8.2 of {{!HTTP2}}.
 
 The same server push can be associated with additional client requests using a
-DUPLICATE_PUSH frame (see {{frame-duplicate-push}}).
+PUSH_PROMISE frame with the same push id on multiple request stream.

```suggestion
PUSH_PROMISE frame with the same Push ID on multiple request stream.
```

> @@ -1238,9 +1233,10 @@ MAX_PUSH_ID frame ({{frame-max-push-id}}). A client MUST treat receipt of a
 PUSH_PROMISE frame that contains a larger Push ID than the client has advertised
 as a connection error of H3_ID_ERROR.
 
-A server MUST NOT use the same Push ID in multiple PUSH_PROMISE frames. A client
-MUST treat receipt of a Push ID which has already been promised as a connection
-error of type H3_ID_ERROR.
+A server MAY use the same Push ID in multiple PUSH_PROMISE frames. If so, the
+decompressed request headers MUST be idential.  A client MUST treat receipt of a
+Push ID which has already been promised with different headers than the previous
+promise as a connection error of type H3_ID_ERROR.

MUST-error requires that the client keep the request headers for the lifetime of the connection in case it ever sees that Push ID again.  Instead, I think we want to say that it MUST error if it detects a mismatch, and MUST ignore the new PUSH_PROMISE if it has not retained the previous request header set.

-- 
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/3309#pullrequestreview-336806725