Subject: [quicwg/base-drafts] Define three QPACK error types, refine error handling. (#1726)

From: Bence Béky <[email-masked]>
Date: Thu, 06 Sep 2018 10:55:56 -0700

Fixes #1683.

Split QPACK errors into three types: decompression failure, encoder
stream error, decoder stream error.  Note that with HPACK only the
decoder could signal an error, so any error indicates a problem between
the decoder of the sender of the error and the encoder of the receiver.
However, in QPACK either the encoder or the receiver can detect an
error, so these types will help pinpoint the context of the error.

Also, specify error signalling behavior upon receiving invalid indices
and invalid decoder stream instructions.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Define three QPACK error types, refine error handling.

-- File Changes --

    M draft-ietf-quic-qpack.md (89)

-- Patch Links --

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

---

From: Bence Béky <[email-masked]>
Date: Thu, 06 Sep 2018 10:56:48 -0700

bencebeky commented on this pull request.

> @@ -120,6 +120,12 @@ which has a fixed index over time.  Its entries are defined in Appendix A of
 {{!RFC7541}}. Note that because HPACK did not use zero-based references, there
 is no value at index zero of the static table.
 
+A decoder that encounters an invalid static table index on a request stream or
+push stream MUST treat this as an error of type

As I am proposing to allow peers to signal a connection error instead of a stream error, I am not specifying the kind of error here and elsewhere where stream error would be sufficient.

-- 
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/1726#pullrequestreview-153050237

---

From: Bence Béky <[email-masked]>
Date: Thu, 06 Sep 2018 10:59:44 -0700

> @@ -270,8 +279,8 @@ immediately. A stream becomes unblocked when the greatest absolute index in the
 dynamic table becomes greater than or equal to the Largest Reference for all
 header blocks the decoder has started reading from the stream.  If a decoder
 encounters a header block where the actual largest reference is not equal to the
-Largest Reference declared in the prefix, it MAY treat this as a stream error of
-type HTTP_QPACK_DECOMPRESSION_FAILED.
+Largest Reference declared in the prefix, it MUST treat this as an error

My reason for changing this to MUST is the following: if this is a MAY, potentially implementation get deployed that do not signal an error, which allows implementation that have a larger Largest Reference declared to be deployed, and once there are enough implementations out there, if a new one signals an error in conformance to the specification, it will break things, and we'll be stuck with an ecosystem that diverged from the spec.

An encoder should never declare a Largest Reference larger than the actual largest reference, let's encourage decoders to enforce it.

(Note that smaller Largest Reference values cannot be sent because there is already a MUST clause for that in 2.2.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/1726#pullrequestreview-153051665

---

From: Bence Béky <[email-masked]>
Date: Thu, 06 Sep 2018 11:00:59 -0700

> @@ -291,7 +300,7 @@ An encoder MUST limit the number of streams which could become blocked to the
 value of SETTINGS_QPACK_BLOCKED_STREAMS at all times. Note that the decoder
 might not actually become blocked on every stream which risks becoming blocked.
 If the decoder encounters more blocked streams than it promised to support, it
-SHOULD treat this as a stream error of type HTTP_QPACK_DECOMPRESSION_FAILED.
+MUST treat this as an error of type HTTP_QPACK_DECOMPRESSION_FAILED.

Same argument as above: I am worried that decoder implementations that do not do what they SHOULD do will allow the proliferation of non-conformant encoder implementations.

-- 
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/1726#pullrequestreview-153052187

---

From: Bence Béky <[email-masked]>
Date: Thu, 06 Sep 2018 11:03:45 -0700

>  QPACK which prevent the stream or connection from continuing:
 
-HTTP_QPACK_DECOMPRESSION_FAILED (0x06):
-: QPACK failed to decompress a frame and cannot continue.
+HTTP_QPACK_DECOMPRESSION_FAILED (TBD):
+: The decoder failed to interpret an instruction on a request or push stream and
+  is not able to continue decoding that header block.  This situation MAY be

I would like to allow decoder implementations to close the connection even if technically closing the stream would be enough.  This is because stream errors are more easy to hide by retry logic that some products have.  Really, a well-behaved encoder implementation should never cause a decoder to signal an error, so if it does, that should be surfaced as much as possible.

-- 
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/1726#pullrequestreview-153053381

---

From: Mike Bishop <[email-masked]>
Date: Thu, 06 Sep 2018 11:05:06 -0700

MikeBishop commented on this pull request.

That's one way to deal with it.  In H2 and HQ, I think we've been leaning on the text that an implementation can always "upgrade" a stream error to a connection error at its discretion.  So even if we say stream, there's an implied choice.

-- 
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/1726#discussion_r215723929

---

From: afrind <[email-masked]>
Date: Thu, 06 Sep 2018 13:26:32 -0700

afrind commented on this pull request.

Thanks for putting this up.

I think it would be more clear to use stream error here, and if an implementation wants to make it a connection error they can.

Would it be better to have this text under the Absolute and Relative Indexing section, rather than here?

> @@ -248,11 +254,14 @@ d = count of entries dropped
 ~~~~~
 {: title="Example Dynamic Table Indexing - Post-Base Index on Request Stream"}
 
-If the decoder encounters a reference to an entry which has already been dropped
-from the table or which is greater than the declared Largest Reference (see
-{{absolute-index}}), this MUST be treated as a stream error of type
-`HTTP_QPACK_DECOMPRESSION_FAILED` error code.  If this reference occurs on the
-encoder stream, this MUST be treated as a session error.
+If the decoder encounters a reference on a request or push stream to a dynamic
+table entry which has already been dropped or which has an absolute index
+greater than the declared Largest Reference (see {{absolute-index}}), this MUST
+be treated as an error of type `HTTP_QPACK_DECOMPRESSION_FAILED`.
+
+If the decoder encounters a reference on the encoder stream to a dynamic table
+entry which has already been dropped or which has not yet been inserted, this

This was already a problem, but there's not correct agreement in these sentences.  '...if the decoder...this must be treated' should be  '...if the decoder... it must treat'.  If you don't feel like mucking with it we can clean it up later.

> @@ -248,11 +254,14 @@ d = count of entries dropped
 ~~~~~
 {: title="Example Dynamic Table Indexing - Post-Base Index on Request Stream"}
 
-If the decoder encounters a reference to an entry which has already been dropped
-from the table or which is greater than the declared Largest Reference (see
-{{absolute-index}}), this MUST be treated as a stream error of type
-`HTTP_QPACK_DECOMPRESSION_FAILED` error code.  If this reference occurs on the
-encoder stream, this MUST be treated as a session error.
+If the decoder encounters a reference on a request or push stream to a dynamic
+table entry which has already been dropped or which has an absolute index
+greater than the declared Largest Reference (see {{absolute-index}}), this MUST
+be treated as an error of type `HTTP_QPACK_DECOMPRESSION_FAILED`.
+
+If the decoder encounters a reference on the encoder stream to a dynamic table
+entry which has already been dropped or which has not yet been inserted, this

Also, I don't think you can make a reference on the encoder stream to a header that has not yet been inserted.  Index 0 is the most recently inserted entry.

See the discussion of MUST vs. MAY for this case in https://github.com/quicwg/base-drafts/issues/1404.

I was for MUST here previously.  I think the argument against it was that it's kind of annoying to check it potentially, and some organizations require all MUSTs to be implemented or have a 'really good reason' (TM).

I'm inclined to agree with you. Will let the MAY/SHOULD side weigh in.

> @@ -602,6 +609,17 @@ blocks within a stream have been fully processed.
 ~~~~~~~~~~
 {:#fig-header-ack title="Header Acknowledgement"}
 
+The same Stream ID can be identified multiple times, as multiple header blocks
+can be sent on a single stream in the case of intermediate responses, trailers,
+and pushed requests.  Since header frames on each stream are received and
+processed in order, this gives the encoder precise feedback on which header
+blocks within a stream have been fully processed.
+
+If an encoder receives a Header Acknowledgement instruction referring to a
+stream on which every header block with non-zero Largest Reference has already

Does 'with *a* non-zero Largest Reference' sound better?

I think this is already the intent, but I don't think it hurts to spell it out here.  Also, maybe we can get rid of the word situation in all three error descriptions?

Did you mean to remove the error code value?

-- 
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/1726#pullrequestreview-153097791

---

From: Bence Béky <[email-masked]>
Date: Thu, 06 Sep 2018 16:19:11 -0700

Okay, that sounds like a smoother solution.  Let me change all these occurrences in the text to "stream error" then, and add text allowing implementations to "upgrade".  BTW where are these sections in H2 and HQ?

-- 
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/1726#discussion_r215806188

---

From: Bence Béky <[email-masked]>
Date: Thu, 06 Sep 2018 16:22:57 -0700

Regarding the Absolute and Relative Indexing section, I think this section here specifically deals with the static table, while the Absolute and Relative Indexing one with the dynamic table, so this clause concerning the static table fits better here.

-- 
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/1726#discussion_r215806673

---

From: Bence Béky <[email-masked]>
Date: Thu, 06 Sep 2018 16:26:54 -0700

I understand.  Sorry, I was not aware of that previous discussion.  Let me revert this to MAY.

-- 
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/1726#discussion_r215807321

---

From: Bence Béky <[email-masked]>
Date: Thu, 06 Sep 2018 16:32:30 -0700

Regarding stream vs connection errors, let me reword this section and every non-reference to stream error elsewhere in the document to flow more smoothly.

Regarding removing the error code, yes, it was intentional.  My understanding is that this is the registry at https://quicwg.org/base-drafts/draft-ietf-quic-http.html#rfc.section.10.5, where currently the next available error code is 0x0012, so this would need to be renumbered (by the RFC editor if the two documents get in the queue at the same time?) anyway.

-- 
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/1726#discussion_r215808267

---

From: Bence Béky <[email-masked]>
Date: Thu, 06 Sep 2018 16:50:57 -0700

I can get rid of the word "situation" three times by removing those three sentences.  They are covered by other parts of the document anyway.  Also, neither https://quicwg.org/base-drafts/draft-ietf-quic-http.html#rfc.section.6.1 nor https://httpwg.org/specs/rfc7540.html#ErrorCodes bothers to spell out which error codes are for stream errors, which ones for connection errors, and which could be used for both.  So why should we?

-- 
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/1726#discussion_r215810877

---

From: Bence Béky <[email-masked]>
Date: Thu, 06 Sep 2018 16:59:02 -0700

~~~~~

Done.

-- 
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/1726#discussion_r215812092

---

From: Bence Béky <[email-masked]>
Date: Thu, 06 Sep 2018 17:00:14 -0700

~~~~~

Sorry, I totally missed that.  You are right.

-- 
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/1726#discussion_r215812267

---

From: Bence Béky <[email-masked]>
Date: Thu, 06 Sep 2018 17:01:33 -0700

~~~~~~~~~~

Definitely, good catch!

-- 
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/1726#discussion_r215812470

---

From: Bence Béky <[email-masked]>
Date: Fri, 07 Sep 2018 07:08:33 -0700

@bencebeky pushed 1 commit.

4bd62e7  Address some comments by afrind@ and MikeBishop@.

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1726/files/4d04f8d22ccca1035cdd30c6688a05c00cc5bc51..4bd62e76ac0200393f3ff9813ba3d4986b582658

---

From: Kazuho Oku <[email-masked]>
Date: Fri, 07 Sep 2018 21:39:09 -0700

I am not sure if I like the change due to the following two reasons:

* my understanding is that the general architecture we expect is to have components (e.g, `qpack_decode_int` function) return an error code that is a constant, rather than returning an error code depending on how the component is being used
*  this seems like a proposal to having different error code for each stream

If we are to agree on having this level of granularity, I would prefer adding a `stream_id` field to the APPLICATION_CLOSE frame for indicating the stream that the error was caused by. That would give us the ability detect the stream that caused the connection to terminate, not limited to QPACK.

-- 
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/1726#issuecomment-419612428

---

From: Bence Béky <[email-masked]>
Date: Mon, 10 Sep 2018 07:30:28 -0700

@kazuho Since the opcodes on the decoder stream and the request/header stream are different anyway (because the instructions have different semantics), I see no problem with `qpack_decode_int` returning one kind of error but the caller translating it into different QUIC error codes depending on context.

That being said, you are absolutely right that a `stream_id` field in APPLICATION_CLOSE would buy us the same extra resolution as splitting the QPACK error code into three.

-- 
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/1726#issuecomment-419933768

---

From: Mike Bishop <[email-masked]>
Date: Mon, 10 Sep 2018 12:50:26 -0700

I agree that the change to APPLICATION_CLOSE would be separately useful, though it might not make the v1 bar at this point.  However, I don't think having separate errors is that problematic.  In my QPACK implementation, there are separate functions processing the encoder stream, decoder stream, and compressed header sets, so returning a single error code from each function is not problematic.

-- 
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/1726#issuecomment-420037880

---

From: afrind <[email-masked]>
Date: Mon, 10 Sep 2018 13:02:12 -0700

Merged #1726 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/1726#event-1836926647