Subject: [quicwg/base-drafts] Additional error cases for CANCEL_PUSH (#3083)

From: Mike Bishop <[email-masked]>
Date: Tue, 08 Oct 2019 11:31:00 -0700

Fixes #2819.  Open to discussion about whether all of these are ID_ERROR or not.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Additional error cases for CANCEL_PUSH

-- File Changes --

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

-- Patch Links --

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

---

From: ianswett <[email-masked]>
Date: Thu, 10 Oct 2019 09:40:08 -0700

ianswett commented on this pull request.

> @@ -1071,7 +1071,8 @@ 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
 push, it can use the receipt of a CANCEL_PUSH frame to avoid opening a push
 stream.  If the push stream has been opened by the server, the server SHOULD

"has been opened" is now a bit odd without any qualification, since it's possible it's been opened and subsequently closed.

How about "If the push stream is open, the server SHOULD abruptly terminate that stream."

-- 
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/3083#pullrequestreview-300204866

---

From: ianswett <[email-masked]>
Date: Thu, 10 Oct 2019 11:44:45 -0700

> @@ -1093,10 +1094,15 @@ error of type HTTP_FRAME_UNEXPECTED.
 
 The CANCEL_PUSH frame carries a Push ID encoded as a variable-length integer.
 The Push ID identifies the server push that is being cancelled (see
-{{frame-push-promise}}).
+{{frame-push-promise}}).  If a CANCEL_PUSH frame is received which references a
+Push ID greater than currently allowed on the connection, this MUST be treated

Should this only be enforced by the client, given it's the one limiting the number of pushes?  The next paragraph discusses the server closing the connection when the client sends a CANCEL_PUSH mentioning a push ID that's never been used 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/3083#pullrequestreview-300277199

---

From: ianswett <[email-masked]>
Date: Thu, 10 Oct 2019 17:27:06 -0700

How about reorganizing this section with one paragraph like this that starts with "When a server receives this frame, ..." and have another with "When a client receives this frame, ..." since the roles are completely asymmetric I believe?

-- 
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/3083#pullrequestreview-300417977

---

From: Lucas Pardue <[email-masked]>
Date: Fri, 11 Oct 2019 03:10:57 -0700

LPardue commented on this pull request.

I think there are two effective limits - the maximum allowed push ID and the maximum used push ID. So if I understand Ian's point, there is not a situation where a client can issue a CANCEL_PUSH that violates the maximum allowed push ID without first triggering the violation of the maximum used 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/3083#discussion_r333921201

---

From: Mike Bishop <[email-masked]>
Date: Fri, 11 Oct 2019 08:13:25 -0700

MikeBishop commented on this pull request.

The logic there is that, even if the stream is closed (FIN sent), there might still be value in sending a RESET_STREAM to abort any retransmissions.

-- 
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/3083#discussion_r334041847

---

From: Mike Bishop <[email-masked]>
Date: Fri, 11 Oct 2019 15:16:27 +0000 (UTC)

The server can issue a CANCEL_PUSH for a push it wasn't allowed to send, which the client should treat as an error.  And since CANCEL_PUSH and MAX_PUSH_ID are on the control stream, if the client uses a Push ID greater than it has permitted, that also seems invalid.

However, in the latter case, I can believe there are implementations where that ordering isn't maintained within the client, so perhaps we should relax that one.

-- 
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/3083#discussion_r334043260

---

From: Mike Bishop <[email-masked]>
Date: Fri, 11 Oct 2019 08:26:39 -0700

@MikeBishop pushed 1 commit.

065cd1ebddd882cf855b029cdf2916d3375c0ae3  Ian's parallel construction

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3083/files/7cefe142cfdddbb81617b031b7457c526b3f220d..065cd1ebddd882cf855b029cdf2916d3375c0ae3

---

From: ianswett <[email-masked]>
Date: Fri, 11 Oct 2019 14:35:18 -0700

ianswett approved this pull request.

SG, I think I'm ok with this then.  I still find this section a bit of a hard read, but I don't have any specific suggestions.

-- 
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/3083#pullrequestreview-300938096

---

From: Mike Bishop <[email-masked]>
Date: Mon, 14 Oct 2019 11:49:46 -0700

I've tried to improve the readability of this section a bit; I turned Ian's suggested parallel paragraphs around to talk about why client and server would send the frame (and the corresponding responses) instead of talking about behavior on receipt.

Please take a fresh look, @LPardue and @ianswett.

-- 
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/3083#issuecomment-541854025

---

From: Mike Bishop <[email-masked]>
Date: Mon, 14 Oct 2019 11:53:19 -0700

263dcedc17281832707abbc15c53c9197ebcb374  Improve readability

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3083/files/065cd1ebddd882cf855b029cdf2916d3375c0ae3..263dcedc17281832707abbc15c53c9197ebcb374

---

From: ianswett <[email-masked]>
Date: Mon, 14 Oct 2019 16:00:25 -0700

> -stream.  If the push stream has been opened by the server, the server SHOULD
-abruptly terminate that stream.
-
-A server can send the CANCEL_PUSH frame to indicate that it will not be
-fulfilling a promise prior to creation of a push stream.  Once the push stream
-has been created, sending CANCEL_PUSH has no effect on the state of the push
-stream.  The server SHOULD abruptly terminate the push stream instead.
+When a client sends this frame, it is indicating that it does not wish to
+receive the promised resource.  The server SHOULD abort sending the resource,
+but the mechanism to do so depends on the state of the corresponding push
+stream.  If the server has not yet created a push stream, it does not create
+one.  If the push stream is open, the server SHOULD abruptly terminate that
+stream.  If the push stream has already closed, the server MAY abruptly
+terminate the stream (cancelling any retransmissions) or MAY take no action.
+
+When a server sends this frame, it is indicating that it will not be fulfilling

```suggestion
When a server sends CANCEL_PUSH, it is indicating that it will not be fulfilling
```

-- 
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/3083#pullrequestreview-301579960

---

From: ianswett <[email-masked]>
Date: Mon, 14 Oct 2019 16:04:02 -0700

> -identified server push.  If the server has not yet started to send the server
-push, it can use the receipt of a CANCEL_PUSH frame to avoid opening a push

```suggestion
terminate the stream or MAY take no action.
```

-- 
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/3083#pullrequestreview-301580924

---

From: Mike Bishop <[email-masked]>
Date: Mon, 14 Oct 2019 16:07:19 -0700

6fd1a1ed765debeba0b5b82679b9307da372b9f0  Review feedback

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3083/files/263dcedc17281832707abbc15c53c9197ebcb374..6fd1a1ed765debeba0b5b82679b9307da372b9f0

---

From: Mike Bishop <[email-masked]>
Date: Mon, 14 Oct 2019 16:09:34 -0700

02114fe16fca1503239352bf9af5978304299ab7  Wordsmith

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3083/files/6fd1a1ed765debeba0b5b82679b9307da372b9f0..02114fe16fca1503239352bf9af5978304299ab7

---

From: Mike Bishop <[email-masked]>
Date: Tue, 15 Oct 2019 10:30:23 -0700

Merged #3083 into master.

-- 
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/3083#event-2714836211