Subject: Re: [quicwg/base-drafts] Split error code space (#722)

From: Mark Nottingham <[email-masked]>
Date: Mon, 04 Sep 2017 01:28:29 +0000 (UTC)

@martinthomson I think CI needs to be re-run; seems to have been a transient error from GH.

-- 
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/722#issuecomment-326846744

---

From: Martin Thomson <[email-masked]>
Date: Mon, 04 Sep 2017 02:13:54 +0000 (UTC)

Rebased it.  I've changed the config so that intermittent error won't cause failures (it's the issue sync, which isn't critical).

-- 
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/722#issuecomment-326851013

---

From: janaiyengar <[email-masked]>
Date: Wed, 06 Sep 2017 00:46:33 +0000 (UTC)

janaiyengar commented on this pull request.

A few comments. This needs to be rebased and the resolution on the list will cause this to change further, I expect.

> @@ -844,7 +844,8 @@ explained in more detail as they are referenced later in the document.
 |:------------|:------------------|:----------------------------|
 | 0x00        | PADDING           | {{frame-padding}}           |
 | 0x01        | RST_STREAM        | {{frame-rst-stream}}        |
-| 0x02        | CONNECTION_CLOSE  | {{frame-connection-close}}  |
+| 0x02        | TRANSPORT_CLOSE   | {{frame-transport-close}}   |

I prefer connection close -- a connection is  a transport notion anyways. CONNECTION_CLOSE and APPLICATION_CLOSE ought to be clear enough, IMO.

> @@ -1609,7 +1635,7 @@ The RST_STREAM frame is as follows:
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 |                        Stream ID (32)                         |
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                        Error Code (32)                        |
+|                  App Protocol Error Code (32)                 |

nit: spell out "Application"

> @@ -1623,43 +1649,51 @@ Stream ID:
 
 : The 32-bit Stream ID of the stream being terminated.
 
-Error code:
+App Protocol Error Code:

ditto

 Error Code:
 
 : A 32-bit error code which indicates the reason for closing this connection.
+  TRANSPORT_CLOSE uses codes from the space defined in {{error-codes}};
+  APPLICATION_CLOSE uses codes from the application protocol error code space
+  ({{app-error-codes}}).

I would at least parenthesize it then.

> @@ -2608,11 +2654,10 @@ discarded upon receipt.  This avoids potential ambiguity about which STREAM
 frames count toward flow control.
 
 Upon receipt of a STOP_SENDING frame on a stream in the "open" or "half-closed
-(remote)" states, an endpoint MUST send a RST_STREAM with an error code of
-QUIC_RECEIVED_RST.  If the STOP_SENDING frame is received on a stream that is
-already in the "half-closed (local)" or "closed" states, a RST_STREAM frame MAY
-still be sent in order to cancel retransmission of previously-sent STREAM
-frames.
+(remote)" states, an endpoint MUST send a RST_STREAM frame.  If the STOP_SENDING

I understand, but there's an issue here. If the transport receives a STOP_SENDING and MUST send a RST_STREAM, there's nothing in here that requires the application to do something in between the receiving of the STOP_SENDING and sending of the RST_STREAM. If it's not a transport requirement to send a RST_STREAM, then the "MUST" doesn't belong here. So I would remove any mention of RST_STREAM being sent here; that's an application's prerogative. The transport does not need to ensure this.

That said, I think I'm with you on STOP_SENDING. It's an HTTP-ism, so it probably belongs in the application. I'll chime in on the list.

-- 
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/722#pullrequestreview-60777325

---

From: Martin Thomson <[email-masked]>
Date: Tue, 05 Sep 2017 18:20:25 -0700

martinthomson commented on this pull request.

I agree.  I think that I'd rather do the move of STOP_SENDING, but I'll tweak the text here in case someone objects to the other PR.

-- 
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/722#discussion_r137152479

---

From: Martin Thomson <[email-masked]>
Date: Wed, 06 Sep 2017 01:21:45 +0000 (UTC)

@martinthomson pushed 1 commit.

2cff85c  Jana's comments

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/722/files/d687a9aae47258dca51db9b5a3d63494a1014fbf..2cff85c9d30eb4701b83890e36dec159e8208cc1

---

From: Mike Bishop <[email-masked]>
Date: Thu, 07 Sep 2017 17:08:49 +0000 (UTC)

MikeBishop commented on this pull request.

Change looks fine.

> @@ -1067,7 +1070,7 @@ The HTTP/2 error codes defined in Section 7 of {{!RFC7540}} map to QUIC error
 codes as follows:
 
 NO_ERROR (0x0):
-: QUIC_NO_ERROR
+: HTTP_NO_ERROR

"...in {{http-error-codes}}." for consistency with the others.

-- 
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/722#pullrequestreview-61286145

---

From: Martin Thomson <[email-masked]>
Date: Thu, 07 Sep 2017 15:26:10 -0700

48d141c  Small tweak at Mike's suggestion

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/722/files/2cff85c9d30eb4701b83890e36dec159e8208cc1..48d141c2e80a2e1795dc10665b883abd1076d7c5

---

From: janaiyengar <[email-masked]>
Date: Thu, 07 Sep 2017 23:28:48 +0000 (UTC)

So, the only thing I have a concern about is that if the transport is required to send a RST_STREAM in response to a STOP_SENDING, what's it to send without an error code? I would remove that requirement. Of course, this puts the onus on the application, but I think that's a natural outcome. (I think it also clarifies that STOP_SENDING is really an application signal.)

-- 
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/722#pullrequestreview-61379914

---

From: Martin Thomson <[email-masked]>
Date: Thu, 07 Sep 2017 17:03:47 -0700

Yeah, for that though I would prefer to just merge #759.

-- 
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/722#issuecomment-327962527

---

From: Martin Thomson <[email-masked]>
Date: Thu, 07 Sep 2017 17:09:22 -0700

4fa1f68  Merge branch 'master' into split-errors

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/722/files/48d141c2e80a2e1795dc10665b883abd1076d7c5..4fa1f6853a69e7e9305e5d52a44da1366c9f2ae7

---

From: janaiyengar <[email-masked]>
Date: Fri, 08 Sep 2017 00:16:02 +0000 (UTC)

I agree, but I don't think we've reached a resolution there yet. In the meanwhile, this PR could remove the requirement within QUIC to send a RST_STREAM in response to a STOP_SENDING, since it doesn't make sense anyways.

-- 
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/722#issuecomment-327964752

---

From: Martin Thomson <[email-masked]>
Date: Fri, 08 Sep 2017 02:57:25 +0000 (UTC)

Busywork like that is low priority, but you are welcome to make a change to the branch if you feel strongly about it.

-- 
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/722#issuecomment-327985832

---

From: janaiyengar <[email-masked]>
Date: Fri, 08 Sep 2017 22:27:30 +0000 (UTC)

Then let's wait for #759 to land first.

-- 
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/722#issuecomment-328228350

---

From: Martin Thomson <[email-masked]>
Date: Wed, 27 Sep 2017 18:22:17 -0700

@janaiyengar, #759 actually depends on this, so merging that first would be hard.  Though we could wait until we agree on that, it seems like we're not going to agree without some discussion.

In the meantime, I've looked at the text around STOP_SENDING and I think that I have addressed your concerns.  Actually, the rebase for this seems to have picked up other changes that did most of the work.  The most recent commit just shuffles things around slightly and makes a few editorial corrections.

I think that this is ready, but am happy to defer until Monday to discuss if you have any lingering reservations.

-- 
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/722#issuecomment-332700067