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

[afrind <[e-mail]>]

afrind commented on this pull request.

Thanks for putting this up.

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

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.

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

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.

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

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

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

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?

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

>  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

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