Subject: Re: [quicwg/base-drafts] Recommend careful behavior around push streams (#4931)

From: Mike Bishop <[email-masked]>
Date: Sun, 01 Aug 2021 18:54:07 -0700

@martinthomson, can you expand on why you think the latter changes aren't good?  Noting that subpar things might happen if you abort reading seems like the RFC2119 definition of SHOULD NOT abort reading.  Certainly we can explain the SHOULD NOT more than we currently do.

-- 
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/4931#issuecomment-890652322

---

From: Martin Thomson <[email-masked]>
Date: Sun, 01 Aug 2021 19:34:44 -0700

Fact is: clients will want to discard responses when the thing that asked for that response goes away.  You are saying that they SHOULD NOT do that.  But they are going to anyway.  I would rather we just say that if you have push enabled and you do that, you might be relying more heavily on your push ID timeouts to ensure that you properly handle missing push information.

-- 
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/4931#issuecomment-890666729

---

From: Mike Bishop <[email-masked]>
Date: Mon, 02 Aug 2021 07:23:52 -0700

I'm not saying anything about request streams; I agree that clients will want to discard requests they don't need any more and will potentially be discarding PUSH_PROMISE frames along with them.  The timeout in that case is fine.

What I'm specifically recommending in the text is that endpoints read far enough into a unidirectional stream (1-9 bytes) to discover the push ID and mark it consumed.  If they don't, the server might eventually become unable to push; no timeout will recover from that.

-- 
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/4931#issuecomment-891070530

---

From: Tatsuhiro Tsujikawa <[email-masked]>
Date: Mon, 02 Aug 2021 08:29:00 -0700

@tatsuhiro-t approved this pull request.

-- 
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/4931#pullrequestreview-720333125

---

From: afrind <[email-masked]>
Date: Mon, 02 Aug 2021 15:22:59 -0700

@afrind commented on this pull request.

> @@ -1132,6 +1133,10 @@ an error code of H3_STREAM_CREATION_ERROR or a reserved error code
 ({{http-error-codes}}), but MUST NOT consider such streams to be a connection
 error of any kind.
 
+As certain stream types can affect connection state, a recipient SHOULD NOT
+discard data from incoming unidirectional streams prior to reading the type
+byte.

Type can be longer than 1 byte (it's a varint)

> @@ -884,7 +884,8 @@ as-yet-unknown Push ID, both the associated client request and the pushed
 request header fields 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.
+commit to the pushed stream.  Clients SHOULD discard data from push streams if

Does the client also abort reading the push stream if it is incomplete?

> @@ -1132,6 +1133,10 @@ an error code of H3_STREAM_CREATION_ERROR or a reserved error code
 ({{http-error-codes}}), but MUST NOT consider such streams to be a connection
 error of any kind.
 
+As certain stream types can affect connection state, a recipient SHOULD NOT
+discard data from incoming unidirectional streams prior to reading the type
+byte.

Same question about 'discard data' - is the client expected to read this stream until FIN/RST or can it abort?

-- 
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/4931#pullrequestreview-720628082

---

From: Martin Thomson <[email-masked]>
Date: Mon, 02 Aug 2021 15:38:15 -0700

@martinthomson approved this pull request.

OK, with the explanation, this works for me.

-- 
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/4931#pullrequestreview-720642844

---

From: Mike Bishop <[email-masked]>
Date: Tue, 03 Aug 2021 07:20:12 -0700

@MikeBishop commented on this pull request.

The client is expected to read until it knows the type of the stream.  At that point, it's free to abort or continue reading based on that information.

-- 
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/4931#discussion_r681805973

---

From: Mike Bishop <[email-masked]>
Date: Tue, 03 Aug 2021 07:20:45 -0700

If what is incomplete?  The Push ID?  The response?

-- 
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/4931#discussion_r681806545

---

From: Mike Bishop <[email-masked]>
Date: Tue, 03 Aug 2021 07:52:50 -0700

@MikeBishop pushed 1 commit.

c3b56a8677822a54c17ac47c1f0de5e562851599  Type is not a byte

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4931/files/1f39e3d9a76142d475450d4a48ab9cc1964c6b55..c3b56a8677822a54c17ac47c1f0de5e562851599

---

From: afrind <[email-masked]>
Date: Tue, 03 Aug 2021 14:49:11 -0700

I think the PR is clear that a client needs to process any push stream at least as far as the push ID.  

The new text says the client should 'discard data' if the matching PUSH_PROMISE doesn't arrive in a reasonable time.  Should the client also send a STOP_SENDING in this case?  By incomplete I meant 'had received the Push ID but not the FIN'

-- 
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/4931#discussion_r682125371

---

From: Mike Bishop <[email-masked]>
Date: Wed, 04 Aug 2021 08:02:30 -0700

0ce850eb478ccd9bfac17b6759d7bca119b96d7c  Explicit about aborting and discarding

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4931/files/c3b56a8677822a54c17ac47c1f0de5e562851599..0ce850eb478ccd9bfac17b6759d7bca119b96d7c

---

From: Mike Bishop <[email-masked]>
Date: Wed, 04 Aug 2021 08:12:43 -0700

Ah, I see.  Thanks for clarifying!  I think 0ce850eb addresses this better.

-- 
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/4931#discussion_r682711300

---

From: Lucas Pardue <[email-masked]>
Date: Tue, 10 Aug 2021 05:42:37 -0700

@LPardue commented on this pull request.

> @@ -1193,6 +1198,10 @@ Push Stream Header {
 ~~~~~~~~~~
 {: #fig-push-stream-header title="Push Stream Header"}
 
+A client SHOULD NOT abort reading on a push stream prior to reading the Push ID,

Other parts of the doc use the style "reading on", so I'll remove this suggestion.

-- 
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/4931#pullrequestreview-726383766

---

From: Lucas Pardue <[email-masked]>
Date: Tue, 31 Aug 2021 03:02:55 -0700

@LPardue approved this pull request.

-- 
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/4931#pullrequestreview-742526853