Subject: Re: [quicwg/base-drafts] Prune transport error codes (#467)

From: janaiyengar <[email-masked]>
Date: Mon, 01 May 2017 18:20:29 -0700

janaiyengar commented on this pull request.

I need a bit more time to go through the errors here. I'll get back by  tomorrow.

-- 
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/467#pullrequestreview-35683305

---

From: janaiyengar <[email-masked]>
Date: Thu, 11 May 2017 16:35:28 -0700

I don't think we should replace the finer-grained errors that are replaced with PROTOCOL_VIOLATION_ERROR or FRAME_FORMAT_ERROR. Having details is very useful to a peer that can monitor these errors and  debug quickly. Otherwise, knowing there was a frame that wasn't formatted correctly is almost useless information.

I'll come up with a list of things that I think we should retain, and I have an idea for how to simplify the description. I'll give it a shot this evening.

> @@ -2522,13 +2520,12 @@ An endpoint MUST NOT send data on a stream at or beyond the final offset.
 
 Once a final offset for a stream is known, it cannot change.  If a RST_STREAM or
 STREAM frame causes the final offset to change for a stream, an endpoint SHOULD

this isn't part of your PR, but SHOULD --> MUST

> @@ -2565,6 +2562,10 @@ 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 send a
 Public Reset packet.
 
+An endpoint that receives an invalid error code in a CONNECTION_CLOSE frame MUST
+NOT signal the existence of the error to its peer.  It MAY treat the error as an
+INTERNAL_ERROR on the basis that there is some fault with the peer.

I don't think I understand what this para is trying to say... do you mean "existence of the error" or "absence of the error"? What's this trying to achieve?

> @@ -2575,7 +2576,7 @@ affected stream.
 
 Stream 1 is critical to the functioning of the entire connection.  If stream 1
 is closed with either a RST_STREAM or STREAM frame bearing the FIN flag, an
-endpoint MUST generate a connection error of type QUIC_CLOSED_CRITICAL_STREAM.
+endpoint MUST generate a connection error of type PROTOCOL_VIOLATION.

Why remove this error? It's strictly more information, and is surely useful for debugging on the side that's likely messing up.

-QUIC_NETWORK_IDLE_TIMEOUT (0x80000019):
-: The connection timed out due to no network activity.
+ENHANCE_YOUR_CALM (0x80000002):

Where's this intended to be used? It's not clear to me what exactly the endpoint wants the peer to reduce generation of -- Data? Acks? Window updates? -- and what the peer is expected to do when this error code is received. Is this a stream or connection error? It would be an odd time to say reduce generation  of something if used as a connection error... the connection is now closed. Does this error code mean that the peer SHOULD NOT reconnect?

-QUIC_TOO_MANY_RTOS (0x80000055):
-: QUIC timed out after too many RTOs.
+: An endpoint received a frame that was badly formatted.  For instance, an empty
+  STREAM frame that omitted the FIN flag, or an ACK frame that has more
+  acknowledgment ranges than the remainder of the packet could carry.

These are very different errors in terms of what a peer would do with this information. I don't think collapsing these two examples is a good idea, since the peer can actually do something concrete with this information -- separating STREAM_FRAME_FORMAT_ERROR from ACK_FRAME_FORMAT_ERROR is a useful for monitoring and debugging.

-- 
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/467#pullrequestreview-37720557

---

From: Martin Thomson <[email-masked]>
Date: Thu, 11 May 2017 17:42:20 -0700

martinthomson commented on this pull request.

Let's fix/discuss that separately.

-- 
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/467#discussion_r116133053

---

From: Martin Thomson <[email-masked]>
Date: Thu, 11 May 2017 17:47:49 -0700

See comment below.

-- 
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/467#discussion_r116133580

---

From: Martin Thomson <[email-masked]>
Date: Thu, 11 May 2017 17:48:47 -0700

Yeah, this needs work.  It's trying to say that you can't respond to a CONNECTION_CLOSE with a PROTOCOL_VIOLATION.  Give me another shot at this.

-- 
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/467#discussion_r116133654

---

From: Martin Thomson <[email-masked]>
Date: Thu, 11 May 2017 17:49:39 -0700

Gah, this is already rotten :(

I think that you might have misunderstood the approach that I've taken.  The point is to identify classes of errors that can have operational practices associated with them.  Finer grained errors are certainly useful during the development of a protocol, but I don't know how someone would deploy an operational practice to distinguish between a cases where it sent a badly formatted STREAM frame vs. a badly formatted ACK frame.  Those errors are going to need someone to sit down with traces and work out what is going on.

I'm not suggesting that an implementation couldn't generate different error codes for local consumption.  That's great for debugging.  But we're talking protocol signals here.  And when we use a different code point in a protocol, we have to have something in those signals that can be acted upon differently.

-- 
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/467#issuecomment-300954441

---

From: ianswett <[email-masked]>
Date: Thu, 11 May 2017 18:11:49 -0700

ianswett commented on this pull request.

Most of the removals seem good, but I think you went too far in removing some error codes.

> @@ -675,9 +675,9 @@ increase by at least one after sending any packet.
 
 A QUIC endpoint MUST NOT reuse a packet number within the same connection (that
 is, under the same cryptographic keys).  If the packet number for sending
-reaches 2^64 - 1, the sender MUST close the connection by sending a
-CONNECTION_CLOSE frame with the error code QUIC_SEQUENCE_NUMBER_LIMIT_REACHED
-(connection termination is described in {{termination}}.)
+reaches 2^64 - 1, the sender MUST close the connection without sending a
+CONNECTION_CLOSE frame or any further packets; the sender MAY send a Public
+Reset packet in response to further packets that it receives.

Shouldn't it use the last packet number to send a connection close?

Agreed, this does not seem helpful.

+1

-- 
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/467#pullrequestreview-37735438

---

From: ianswett <[email-masked]>
Date: Thu, 11 May 2017 18:12:17 -0700

Generating different error codes for local consumption is not a direction I'd like to go in if I can avoid it.

We have a huge error code space available, so as long as it's clear when one should use an error code, I'm not sure why we wouldn't want a relatively complete 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/467#issuecomment-300957388

---

From: Martin Thomson <[email-masked]>
Date: Thu, 11 May 2017 18:15:01 -0700

See the analogous code in h2.  There isn't a whole lot of text in QUIC for this, but you can find the sorts of applications we use this for in h2.

-- 
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/467#discussion_r116135936

---

From: Martin Thomson <[email-masked]>
Date: Thu, 11 May 2017 18:16:28 -0700

Well, that is a possibility, but think about what it will take to reach this number.  We could put lots of effort into dealing with this case, but you have to ask yourself if that's worth 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/467#discussion_r116136085

---

From: ianswett <[email-masked]>
Date: Thu, 11 May 2017 18:21:03 -0700

If it's already in H2, should it be in the http mapping?  I can't find a good link for H2's usage, the best I found was it's informal use by twitter: https://en.wikipedia.org/wiki/List_of_HTTP_status_codes

And why is it not error code 420? 

-- 
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/467#discussion_r116136507

---

From: Martin Thomson <[email-masked]>
Date: Thu, 11 May 2017 18:26:13 -0700

My understanding is that it is part of the transport in h2, not the application.  If you disagree with that assessment, let me know.

It was 420, for a while, but then we decided that a contiguous error space was more valuable.  However, it's *not* a status code.

-- 
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/467#discussion_r116136978

---

From: janaiyengar <[email-masked]>
Date: Thu, 11 May 2017 21:33:41 -0700

Ah, ok, that makes sense.

-- 
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/467#discussion_r116151510

---

From: janaiyengar <[email-masked]>
Date: Thu, 11 May 2017 21:36:51 -0700

If this has a specific meaning, I would want to name it as such. "ENHANCE_YOUR_CALM" is not a helpful name... but I still don't understand what it stands for. What is this used for in HTTP/2?

-- 
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/467#discussion_r116151729

---

From: janaiyengar <[email-masked]>
Date: Thu, 11 May 2017 21:42:10 -0700

It is very helpful operationally to have these codes available, since a server, for instance, can monitor the different error codes that are received. A spike in FRAME_ERROR for a STREAM frame is much easier to diagnose than a general FRAME_ERROR. As to why this may be useful later and not just during development -- any time you touch code, you can inadvertently introduce bugs. A server that does not want to monitor does not have to, but having the error codes makes it possible for interested parties to do so.

-- 
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/467#issuecomment-300982243

---

From: Martin Thomson <[email-masked]>
Date: Thu, 11 May 2017 21:46:44 -0700

Better that you search the RFC for the code and read it yourself.

-- 
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/467#discussion_r116152342

---

From: Martin Thomson <[email-masked]>
Date: Thu, 11 May 2017 21:49:20 -0700

@martinthomson pushed 1 commit.

c0a6acf  Simplify CONNECTION_CLOSE error handling

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/467/files/640d6d254e6f0e3eee034ce329c5d5456e721b2e..c0a6acfcd45f2c91de7b6213ca62f41b210be45d

---

From: janaiyengar <[email-masked]>
Date: Thu, 11 May 2017 22:11:19 -0700

I don't think I said anything to warrant a rude response. Perhaps you didn't mean it to be rude, but it sure sounds it.

I know of the error code from HTTP/2, and I looked it up before my first comment on this PR. RFC 7540 is not helpful in answering the questions I'm asking.

-- 
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/467#discussion_r116153898

---

From: Martin Thomson <[email-masked]>
Date: Thu, 11 May 2017 22:20:47 -0700

I apologize, I just think that me trying to restate its purpose wouldn't really help because it's got a lot of context attached to 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/467#discussion_r116154552

---

From: ianswett <[email-masked]>
Date: Fri, 12 May 2017 06:53:55 -0700

Ok, fair enough.  I'm happy handling that edge case in whatever way is easiest.

-- 
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/467#discussion_r116235839

---

From: ianswett <[email-masked]>
Date: Fri, 12 May 2017 06:54:37 -0700

link?

-- 
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/467#discussion_r116236004

---

From: janaiyengar <[email-masked]>
Date: Fri, 12 May 2017 12:10:31 -0700

Yeah, I agree. I also realized that this max packet number is known to the receiver, and the receiver knows that the sender is near the max, so clearly it knows that the sender would close the connection.

-- 
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/467#discussion_r116304353

---

From: ianswett <[email-masked]>
Date: Sun, 14 May 2017 18:38:57 -0700

> @@ -2858,173 +2858,74 @@ CONNECTION_CLOSE or RST_STREAM frame. Error codes share a common code space.
 Some error codes apply only to either streams or the entire connection and have
 no defined semantics in the other context.
 
-QUIC_INTERNAL_ERROR (0x80000001):

you'll want this or some equivalent.

> -QUIC_MULTIPLE_TERMINATION_OFFSETS (0x80000005):
-: Multiple final offset values were received on the same stream
-
-QUIC_STREAM_CANCELLED (0x80000006):
-: The stream was cancelled
-
-QUIC_CLOSED_CRITICAL_STREAM (0x80000007):
-: A stream that is critical to the protocol was closed.
-
-QUIC_MISSING_PAYLOAD (0x80000030):
-: The packet contained no payload.
-
-QUIC_INVALID_STREAM_DATA (0x8000002E):
-: STREAM frame data is malformed.
-
-QUIC_UNENCRYPTED_STREAM_DATA (0x8000003D):

We definitely want this.

> -QUIC_INVALID_STREAM_DATA (0x8000002E):
-: STREAM frame data is malformed.
-
-QUIC_UNENCRYPTED_STREAM_DATA (0x8000003D):
-: Received STREAM frame data is not encrypted.
-
-QUIC_MAYBE_CORRUPTED_MEMORY (0x80000059):
-: Received a frame which is likely the result of memory corruption.
-
-QUIC_INVALID_RST_STREAM_DATA (0x80000006):
-: RST_STREAM frame data is malformed.
-
-QUIC_INVALID_CONNECTION_CLOSE_DATA (0x80000007):
-: CONNECTION_CLOSE frame data is malformed.
-
-QUIC_INVALID_GOAWAY_DATA (0x80000008):

I do want an error code per frame type.  It really helps narrow down issues.

Still confusing, particularly on a transport level.

And if we're going to add it, we're going to need to restate it's purpose, we can't just assume readers know what it means.

-- 
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/467#pullrequestreview-38015390

---

From: Sean Turner <[email-masked]>
Date: Tue, 16 May 2017 15:31:05 -0700

seanturner commented on this pull request.

-QUIC_NETWORK_IDLE_TIMEOUT (0x80000019):
-: The connection timed out due to no network activity.
+ENHANCE_YOUR_CALM (0x80000002):

>From RFC 7540:

    ENHANCE_YOUR_CALM (0xb):  The endpoint detected that its peer is
    exhibiting a behavior that might be generating excessive load.

-- 
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/467#discussion_r116874021