Split error code space #722

martinthomson · 2017-08-11T05:02:23Z

This creates two orthogonal spaces for error codes. Application error codes
can be used for both connection and stream errors and are under the control of
the application protocol. Transport error codes are QUIC-controlled, but can
only terminate the connection.

To fix this, I had to add IANA considerations for error codes, plus a few extra
tweaks. I think that the error code space could be narrowed to 16 bits after
this, if only to keep things sane.

Closes #485, #184.

(I've stacked this on top of #721 because I don't like managing conflicts and this touches the same general area.)

janaiyengar

Comments inline. Can you also please not stack these PRs? Github is really difficult to use for review of large changes like this one, and stacking PRs makes it even harder. I understand the pains of merge conflicts, but honestly, that's a one-time pain during merge, not a continual pain during review.

janaiyengar · 2017-08-14T23:20:24Z

draft-ietf-quic-tls.md

-0xC0000000-0xFFFFFFFF. The following error codes are defined when TLS is used
-for the crypto handshake:
+This document defines error codes from the error code space used in
+{{QUIC-TRANSPORT}}.


This document --> This section.
Also, editorial nit: I like the mention of "crypto handshake when TLS is used for it".

The crypto handshake bit is on the very next line. The point of separation is that the codes are consumed even if TLS isn't used.

janaiyengar · 2017-08-14T23:26:47Z

draft-ietf-quic-transport.md


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


Seems unnecessary to mention APPLICATION_CLOSE in the definitoion of TRANSPORT_CLOSE.

I considered not doing that, but it seemed more helpful to highlight the distinction here given that I wasn't repeating the format stuff in the description of APPLICATION_CLOSE.

I would at least parenthesize it then.

janaiyengar · 2017-08-14T23:31:42Z

draft-ietf-quic-transport.md

-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


What's the proposed error code in the RST_STREAM now? If there's a requirement for QUIC to generate and send this frame, the error code has to be specified.

Actually, the point here is that the application sends the RST_STREAM. The transport can't send RST_STREAM. The transport can't do that without risking putting the application into a bad state. I will change this to make it clearer.

Actually, I'm going to raise this on the list. It seems like STOP_SENDING could be made an application-layer construct and we could avoid that complexity at the transport layer entirely. I'm trying to see how visibility at the transport layer would be of value, but I'm drawing a blank.

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.

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.

janaiyengar · 2017-08-14T23:35:42Z

draft-ietf-quic-transport.md

+APPLICATION_CLOSE risks a peer missing the first such packet.  The only
+mechanism available to an endpoint that continues to receive data for a
+terminated connection is to use the stateless reset process
+({{stateless-reset}}).


editorial nit: I would merge this para with the one above, since this one follows from the earlier.

janaiyengar · 2017-08-14T23:36:14Z

draft-ietf-quic-transport.md

-An endpoint that receives an invalid CONNECTION_CLOSE frame MUST NOT signal the
-existence of the error to its peer.
+An endpoint that chooses not to retransmit packets containing TRANSPORT_CLOSE or
+APPLICATION_CLOSE risks a peer missing the first such packet.  The only


Not just the first packet, but any packets lost in the network.

I'll remove this sentence, reword the remaining sentence, and merge.

janaiyengar · 2017-08-14T23:44:31Z

draft-ietf-quic-transport.md

-Error codes are 32 bits long, with the first two bits indicating the source of
-the error code:
+An endpoint that detects a stream error MAY choose to treat the error as a
+connection error and send an APPLICATION_CLOSE frame in place of RST_STREAM.


This is confusing. What is the "endpoint"? QUIC, or HTTP/QUIC? If QUIC is no longer generating RST_STREAM, then which stream errors are detected by QUIC? I suspect the answer is that the endpoint is HTTP/QUIC, in which case I would remove this sentence from here entirely.

janaiyengar · 2017-08-14T23:47:00Z

draft-ietf-quic-transport.md

+error codes are used for the RST_STREAM ({{frame-rst-stream}}) and
+APPLICATION_CLOSE ({{frame-application-close}}) frames.
+
+Application protocols SHOULD define an error codes for indicating no error and


typo: remove "an"

janaiyengar · 2017-08-14T23:47:31Z

draft-ietf-quic-transport.md

+
+## Application Protocol Error Codes {#app-error-codes}
+
+Application protocol error codes are 32-bits long, but the management of


janaiyengar · 2017-08-14T23:52:36Z

draft-ietf-quic-transport.md

@@ -1596,7 +1622,7 @@ The RST_STREAM frame is as follows:
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 |                        Stream ID (32)                         |
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                        Error Code (32)                        |
+| App Protocol Error Code (16)  |


This is still 32 bits in this PR, right?

Yes, I must have split this from the other PR poorly.

janaiyengar · 2017-08-14T23:54:24Z

draft-ietf-quic-transport.md

-
-## Error Codes
+A stream error is always an application-layer construct.  A RST_STREAM MUST NOT
+be generated by the QUIC transport layer.  Resetting a stream at the transport


drop "transport layer".

Also, it's odd to say QUIC MUST not generate a RST_STREAM, since it is a QUIC frame and QUIC does in fact generate it. How about "RST_STREAM MUST be instigated by the application and MUST carry an application error code."?

martinthomson

I haven't seen a review for #721 yet. I apologize for stacking like this, but #721 is pretty self-contained, so the amount of leakage from that is small. That said, I tried removing it from this changeset and found that a lot of the text is intertwined. I've started stacking PRs differently now, so the reviewing should be easier.

martinthomson · 2017-08-15T00:14:40Z

draft-ietf-quic-transport.md

@@ -1596,7 +1622,7 @@ The RST_STREAM frame is as follows:
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 |                        Stream ID (32)                         |
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|                        Error Code (32)                        |
+| App Protocol Error Code (16)  |


Yes, I must have split this from the other PR poorly.

martinthomson · 2017-08-15T00:15:33Z

draft-ietf-quic-transport.md


 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 considered not doing that, but it seemed more helpful to highlight the distinction here given that I wasn't repeating the format stuff in the description of APPLICATION_CLOSE.

martinthomson · 2017-08-15T00:16:33Z

draft-ietf-quic-transport.md

-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


Actually, the point here is that the application sends the RST_STREAM. The transport can't send RST_STREAM. The transport can't do that without risking putting the application into a bad state. I will change this to make it clearer.

Actually, I'm going to raise this on the list. It seems like STOP_SENDING could be made an application-layer construct and we could avoid that complexity at the transport layer entirely. I'm trying to see how visibility at the transport layer would be of value, but I'm drawing a blank.

martinthomson · 2017-08-15T00:20:53Z

draft-ietf-quic-tls.md

-0xC0000000-0xFFFFFFFF. The following error codes are defined when TLS is used
-for the crypto handshake:
+This document defines error codes from the error code space used in
+{{QUIC-TRANSPORT}}.


The crypto handshake bit is on the very next line. The point of separation is that the codes are consumed even if TLS isn't used.

martinthomson · 2017-08-15T00:27:16Z

draft-ietf-quic-transport.md

-An endpoint that receives an invalid CONNECTION_CLOSE frame MUST NOT signal the
-existence of the error to its peer.
+An endpoint that chooses not to retransmit packets containing TRANSPORT_CLOSE or
+APPLICATION_CLOSE risks a peer missing the first such packet.  The only


I'll remove this sentence, reword the remaining sentence, and merge.

mnot · 2017-09-04T01:28:27Z

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

martinthomson · 2017-09-04T02:13:51Z

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

janaiyengar

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

janaiyengar · 2017-09-06T00:32:38Z

draft-ietf-quic-transport.md

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

janaiyengar · 2017-09-06T00:35:19Z

draft-ietf-quic-transport.md

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

janaiyengar · 2017-09-06T00:35:25Z

draft-ietf-quic-transport.md

@@ -1623,43 +1649,51 @@ Stream ID:

 : The 32-bit Stream ID of the stream being terminated.

-Error code:
+App Protocol Error Code:


janaiyengar · 2017-09-06T00:36:48Z

draft-ietf-quic-transport.md


 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.

janaiyengar · 2017-09-06T00:45:36Z

draft-ietf-quic-transport.md

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

MikeBishop

Change looks fine.

MikeBishop · 2017-09-07T16:58:25Z

draft-ietf-quic-http.md

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

janaiyengar

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

martinthomson · 2017-09-08T00:03:44Z

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

janaiyengar · 2017-09-08T00:15:54Z

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.

martinthomson · 2017-09-08T02:57:22Z

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

janaiyengar · 2017-09-08T22:27:28Z

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

martinthomson · 2017-09-28T01:22:13Z

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

This creates two orthogonal spaces for error codes. Application error codes can be used for both connection and stream errors and are under the control of the application protocol. Transport error codes are QUIC-controlled, but can only terminate the connection. To fix this, I had to add IANA considerations for error codes, plus a few extra tweaks. I think that the error code space could be narrowed to 16 bits after this, if only to keep things sane. Closes #132.

janaiyengar

Good to finally have this done. Thanks for getting it sorted out!

martinthomson added -http -tls -transport design An issue that affects the design of the protocol; resolution requires consensus. labels Aug 11, 2017

martinthomson mentioned this pull request Aug 11, 2017

Make error codes smaller #723

Closed

janaiyengar reviewed Aug 15, 2017

View reviewed changes

martinthomson commented Aug 15, 2017

View reviewed changes

martinthomson force-pushed the split-errors branch from 3f1867a to d687a9a Compare September 4, 2017 02:11

martinthomson mentioned this pull request Sep 4, 2017

STOP_SENDING is an application protocol concept #758

Closed

janaiyengar reviewed Sep 6, 2017

View reviewed changes

MikeBishop reviewed Sep 7, 2017

View reviewed changes

janaiyengar reviewed Sep 7, 2017

View reviewed changes

mnot removed the design An issue that affects the design of the protocol; resolution requires consensus. label Sep 25, 2017

martinthomson added 10 commits September 28, 2017 11:39

s/Public Reset/Stateless Reset/

9bfaa6c

Get codes consistent

ba3e074

Make STOP_SENDING application-specific as well

3663e68

Split errors in HTTP too

1abd960

Refine description of HTTP codes

30661ba

Move TLS errors closer

7514803

Match the names

ec4c9ad

Add HTTP_NO_ERROR

aacc9bb

Review comments

390704b

martinthomson added 5 commits September 28, 2017 11:40

Jana's comments

3e670a1

Small tweak at Mike's suggestion

b38f611

Reword STOP_SENDING text to avoid implication of mandatory RST_STREAM

31fa531

Better track changes on master

37f6279

Reword advice on application error codes

78db6b2

martinthomson force-pushed the split-errors branch from 4fa1f68 to 78db6b2 Compare September 28, 2017 01:46

martinthomson added 4 commits October 4, 2017 12:42

Steal a code from the application

0d0b7ab

Simply reserve the RST_STREAM code of 0.

7278425

Application error code missed

0ed11a2

We really should just call this STOPPING

c302a45

janaiyengar approved these changes Oct 5, 2017

View reviewed changes

janaiyengar merged commit f0034b4 into master Oct 5, 2017

martinthomson mentioned this pull request Oct 11, 2017

Remove ACKs from draining period #853

Merged

martinthomson deleted the split-errors branch October 12, 2017 23:52


		## Application Protocol Error Codes {#app-error-codes}

		Application protocol error codes are 32-bits long, but the management of

Split error code space #722

Split error code space #722

Conversation

martinthomson commented Aug 11, 2017 • edited

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

martinthomson 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

mnot commented Sep 4, 2017

martinthomson commented Sep 4, 2017

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

MikeBishop left a 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

martinthomson commented Sep 8, 2017

janaiyengar commented Sep 8, 2017

martinthomson commented Sep 8, 2017

janaiyengar commented Sep 8, 2017

martinthomson commented Sep 28, 2017

janaiyengar left a comment

Choose a reason for hiding this comment

martinthomson commented Aug 11, 2017 •

edited