Subject: Re: [quicwg/base-drafts] QPACK editorial revision (#2759)

From: ianswett <[email-masked]>
Date: Tue, 04 Jun 2019 16:07:44 -0700

ianswett approved this pull request.

A few comments, but I'd appreciate it if @bencebeky would take a look as well.

> @@ -82,25 +82,22 @@ code and issues list for this draft can be found at
 
 # Introduction
 
-The QUIC transport protocol was designed from the outset to support HTTP
-semantics, and its design subsumes many of the features of HTTP/2.  HTTP/2 uses
-HPACK ({{!RFC7541}}) for header compression, but QUIC's stream multiplexing
-comes into some conflict with HPACK.  A key goal of the design of QUIC is to
-improve stream multiplexing relative to HTTP/2 by reducing head-of-line
-blocking.  If HPACK were used for HTTP/3, it would induce head-of-line
-blocking due to built-in assumptions of a total ordering across frames on all
-streams.
-
-QUIC is described in {{QUIC-TRANSPORT}}.  The HTTP/3 mapping is described in
-{{HTTP3}}. For a full description of HTTP/2, see {{?RFC7540}}. The
-description of HPACK is {{!RFC7541}}.
+The QUIC transport protocol is designed to support HTTP semantics, and its

Maybe re-add a reference to {{QUIC-TRANSPORT}} here?

> -semantics, and its design subsumes many of the features of HTTP/2.  HTTP/2 uses
-HPACK ({{!RFC7541}}) for header compression, but QUIC's stream multiplexing
-comes into some conflict with HPACK.  A key goal of the design of QUIC is to
-improve stream multiplexing relative to HTTP/2 by reducing head-of-line
-blocking.  If HPACK were used for HTTP/3, it would induce head-of-line
-blocking due to built-in assumptions of a total ordering across frames on all
-streams.
-
-QUIC is described in {{QUIC-TRANSPORT}}.  The HTTP/3 mapping is described in
-{{HTTP3}}. For a full description of HTTP/2, see {{?RFC7540}}. The
-description of HPACK is {{!RFC7541}}.
+The QUIC transport protocol is designed to support HTTP semantics, and its
+design subsumes many of the features of HTTP/2.  HTTP/2 uses HPACK
+({{!RFC7541}}) for header compression.  If HPACK were used for HTTP/3, it would
+induce head-of-line blocking due to built-in assumptions of a total ordering
+across frames on all streams.
 
 QPACK reuses core concepts from HPACK, but is redesigned to allow correctness in

This transition is a bit awkward, but not any worse than the status quo.

 QPACK reuses core concepts from HPACK, but is redesigned to allow correctness in
 the presence of out-of-order delivery, with flexibility for implementations to
 balance between resilience against head-of-line blocking and optimal compression
 ratio.  The design goals are to closely approach the compression ratio of HPACK
 with substantially less head-of-line blocking under the same loss conditions.
 
+QUIC is described in {{QUIC-TRANSPORT}}.  The HTTP/3 mapping is described in
+{{HTTP3}}. For a full description of HTTP/2, see {{?RFC7540}}. The

I'd suggest folding these references into the first paragraph rather than adding another paragraph?

-- 
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/2759#pullrequestreview-245732558

---

From: Bence Béky <[email-masked]>
Date: Wed, 05 Jun 2019 07:36:57 -0700

bencebeky commented on this pull request.

> @@ -167,15 +167,8 @@ fields (some of them with an empty value).  The dynamic table (see
 {{table-dynamic}}) is built up over the course of the connection and can be used
 by the encoder to index header fields in the encoded header lists.
 
-QPACK instructions appear in three different types of streams:
-
-- The encoder uses a unidirectional stream to modify the state of the dynamic
-table without emitting header fields associated with any particular request.
-
-- Header blocks reference the table state without modifying it.
-
-- The decoder sends feedback to the encoder on a unidirectional stream.  This
-feedback enables the encoder to manage dynamic table state.
+QPACK defines unidirectional streams for sending instructions from encoder to
+decoder and vice-versa.

s/vice-versa/vice versa/ as per https://www.grammarly.com/blog/vice-versa/

-- 
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/2759#pullrequestreview-246050145

---

From: Bence Béky <[email-masked]>
Date: Wed, 05 Jun 2019 07:40:53 -0700

bencebeky approved this pull request.

LGTM.  Thank you, Alan, for this editorial overhaul greatly improving clarity!  I have no design concerns, and all my editorial comments are minor nits, so I'll wait for this to be merged and then proofread the whole document front to back and create a separate 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/2759#pullrequestreview-246052998

---

From: Martin Thomson <[email-masked]>
Date: Tue, 18 Jun 2019 15:41:42 -0700

martinthomson commented on this pull request.

 # Introduction

A reference to h3 might help more.

-- 
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/2759#discussion_r295058263

---

From: Lucas Pardue <[email-masked]>
Date: Thu, 20 Jun 2019 06:54:21 -0700

LPardue approved this pull request.

These changes all look helpful. I left some minor comments, that may be more about the spec than the changes (sorry).

A more general subjective opinion. In the HTTP/3 spec, each frame description starts as "The XYZ frame (type=0xABC) is used to do <stuff>". I find the style of describing instructions, "To do a thing, an instruction starting with the pattern ABC is used" a little jarring.

 ## Encoder
 
-An encoder compresses a header list by emitting either an indexed or a literal
-representation for each header field in the list.  References to the static
-table and literal representations do not require any dynamic state and never
-risk head-of-line blocking.  References to the dynamic table risk head-of-line
+An encoder compresses a header list into a header block by emitting either an
+indexed or a literal representation for each header field in the list (see

Does it always compress? I.e. could a dumb encoder make bad choices?

> @@ -327,37 +312,86 @@ instructions received on the encoder stream.
 The decoder MUST emit header fields in the order their representations appear in
 the input header block.
 
+### Blocked Decoding
+
+Upon receipt of a header block, the decoder examines the Required Insert Count.
+When the Required Insert Count is less than or equal to the decoder's Insert
+Count, the header block can be processed immediately.  Otherwise, the stream is

noob clarification question: I think this is the stream on which the header block was delivered, maybe it would help to be explicit?

> @@ -327,37 +312,86 @@ instructions received on the encoder stream.
 The decoder MUST emit header fields in the order their representations appear in
 the input header block.
 
+### Blocked Decoding
+
+Upon receipt of a header block, the decoder examines the Required Insert Count.
+When the Required Insert Count is less than or equal to the decoder's Insert
+Count, the header block can be processed immediately.  Otherwise, the stream is
+blocked.
+
+While blocked, header block data SHOULD remain in the blocked stream's flow
+control window.  A stream becomes unblocked when the Insert Count becomes
+greater than or equal to the Required Insert Count for all header blocks the
+decoder has started reading from the stream.
+<!-- doesn't the stream become unblocked when the encoder receives the acks? -->
+
+If the decoder encounters a header block with a Required Insert Count value
+larger than defined in {{blocked-streams}}, it MAY treat this as a connection

I'm not sure what this value is when I look back. Is the error when Required Insert Count is greater than "one larger than the largest absolute index"?

> +When the Required Insert Count is less than or equal to the decoder's Insert
+Count, the header block can be processed immediately.  Otherwise, the stream is
+blocked.
+
+While blocked, header block data SHOULD remain in the blocked stream's flow
+control window.  A stream becomes unblocked when the Insert Count becomes
+greater than or equal to the Required Insert Count for all header blocks the
+decoder has started reading from the stream.
+<!-- doesn't the stream become unblocked when the encoder receives the acks? -->
+
+If the decoder encounters a header block with a Required Insert Count value
+larger than defined in {{blocked-streams}}, it MAY treat this as a connection
+error of type HTTP_QPACK_DECOMPRESSION_FAILED.  If the decoder encounters a
+header block with a Required Insert Count value smaller than defined in
+{{blocked-streams}}, it MUST treat this as a connection error of type
+HTTP_QPACK_DECOMPRESSION_FAILED as prescribed in {{invalid-references}}.

Would it be clearer to phrase the paragraph something like: "when processing header blocks the decoder expects the Required Insert Count value to exactly match the value defined in {{blocked streams}. If it encounters a smaller value, it MUST treat this as a connection error of type HTTP_QPACK_DECOMPRESSION_FAILED. If it encounters a larger value, it MAY treat this as a connection error of type HTTP_QPACK_DECOMPRESSION_FAILED."

 ### State Synchronization
 
-The decoder instructions ({{decoder-instructions}}) signal key events at the
-decoder that permit the encoder to track the decoder's state.  These events are:
+The decoder signals the following key events by emitting decoder instructions
+({{decoder-instructions}}) on the decoder stream.
+
+#### Completed Processing of a Header Block
+
+When the decoder finishes decoding a header block containing dynamic table
+references, it emits a Header Acknowledgement instruction
+({{header-acknowledgement}}).  The same Stream ID can be identified by multiple
+Header Acknowledgement instructions, as multiple header blocks can be sent on a
+single stream in the case of intermediate responses, trailers, and pushed
+requests.  Since frames carrying header blocks 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 I understand this correctly, I offer the follow as a suggested change:

"When the decoder finishes decoding a header block containing dynamic table
references, it emits a Header Acknowledgement instruction
({{header-acknowledgement}}).  A stream may carry multiple header blocks
in the case of intermediate responses, trailers, and pushed
requests. Emitting multiple Header Acknowledgements for the same stream, 
when each frame is completely processed in order, gives the encoder precise
feedback.

> +
+When the decoder finishes decoding a header block containing dynamic table
+references, it emits a Header Acknowledgement instruction
+({{header-acknowledgement}}).  The same Stream ID can be identified by multiple
+Header Acknowledgement instructions, as multiple header blocks can be sent on a
+single stream in the case of intermediate responses, trailers, and pushed
+requests.  Since frames carrying header blocks 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.
+
+#### Abandonment of a Stream
+
+When an endpoint receives a stream reset before the end of a stream or before
+all header blocks are processed on that stream, or when it abandons reading of a
+stream, it generates a Stream Cancellation instruction (see
+{{stream-cancellation}}).  This signals to the encoder that all references to

is this all unacknowledged references?

> +
+When the decoder finishes decoding a header block containing dynamic table
+references, it emits a Header Acknowledgement instruction
+({{header-acknowledgement}}).  The same Stream ID can be identified by multiple
+Header Acknowledgement instructions, as multiple header blocks can be sent on a
+single stream in the case of intermediate responses, trailers, and pushed
+requests.  Since frames carrying header blocks 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.
+
+#### Abandonment of a Stream
+
+When an endpoint receives a stream reset before the end of a stream or before
+all header blocks are processed on that stream, or when it abandons reading of a
+stream, it generates a Stream Cancellation instruction (see
+{{stream-cancellation}}).  This signals to the encoder that all references to

Oh is that what the following paragraph is trying to articulate?

-The decoder chooses when to emit Insert Count Increment instructions (see
+After receiving new table entries on the encoder stream, the decoder chooses
+when to emit Insert Count Increment instructions (see
 {{insert-count-increment}}). Emitting an instruction after adding each new

should it be "this instruction" or could it be *any* instruction type.

>  ### Insert With Name Reference
 
-An addition to the header table where the header field name matches the header
-field name of an entry stored in the static table or the dynamic table starts
-with the '1' one-bit pattern.  The `S` bit indicates whether the reference is to
-the static (S=1) or dynamic (S=0) table. The 6-bit prefix integer (see Section
-5.1 of [RFC7541]) that follows is used to locate the table entry for the header
-name.  When S=1, the number represents the static table index; when S=0, the
-number is the relative index of the entry in the dynamic table.
+An encoder adds an entry to the dynamic table where the header field name
+matches the header field name of an entry stored in the static table or the

```suggestion
matches the header field name of an entry stored in the static or the
```

you omit this later 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/2759#pullrequestreview-252255145

---

From: Mike Bishop <[email-masked]>
Date: Thu, 20 Jun 2019 09:55:22 -0700

MikeBishop approved this pull request.

 ## Encoder

"encodes," "represents"?  But regardless, this isn't new text in this change.

 An encoder MUST NOT insert an entry into the dynamic table (or duplicate an
-existing entry) if doing so would evict a blocking entry.  In this case, the
-encoder can send literal representations of header fields.
+existing entry) if doing so would evict a blocking entry.  An encoder can always
+send literal representations of header fields when the dynamic table is full

Perhaps just omit "when the dynamic table..."?  It's true universally, regardless of the dynamic table state.

This instruction, you're 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/2759#pullrequestreview-252404774

---

From: Mike Bishop <[email-masked]>
Date: Thu, 20 Jun 2019 10:10:25 -0700

MikeBishop commented on this pull request.

@LPardue has noted something that's confusing here, albeit not really new text.

+### Blocked Decoding
+

The intent is to say that, when you actually process the block, if there was no reference to the entry the RIC said you needed, then the peer's encoder is broken.  Blow up.

Yes -- the general error case is that the RIC didn't match the actual contents of the header block; deviation in any direction is an error, even if you were in fact able to decode all the headers.

-- 
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/2759#pullrequestreview-252419606

---

From: Mike Bishop <[email-masked]>
Date: Thu, 20 Jun 2019 10:17:01 -0700

Minor quibble with @LPardue's text:  That seems to imply that header blocks on the stream might *not* be completely processed in order.  Now, that's certainly possible to do (albeit not useful in most circumstances and not directly discussed in this doc), but acknowledgements always have to be in order here.

+
+When an endpoint receives a stream reset before the end of a stream or before
+all header blocks are processed on that stream, or when it abandons reading of a
+stream, it generates a Stream Cancellation instruction (see
+{{stream-cancellation}}).  This signals to the encoder that all references to
+the dynamic table on that stream are no longer outstanding.  A decoder with a
+maximum dynamic table capacity equal to zero (see
+{{maximum-dynamic-table-capacity}}) MAY omit sending Stream Cancellations,
+because the encoder cannot have any dynamic table references.  An encoder cannot
+infer from this instruction that any updates to the dynamic table have been
+received.
+
+The Header Acknowledgement and Stream Cancellation instructions permit the
+encoder to evict entries to which no unacknowledged references remain (see
+{{blocked-insertion}}).

This wording feels weird.  Entries without unacknowledged references can always be evicted.  The instructions acknowledge the references themselves.

-- 
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/2759#pullrequestreview-252422522

---

From: Mike Bishop <[email-masked]>
Date: Thu, 20 Jun 2019 10:32:32 -0700

> @@ -795,8 +797,10 @@ already been acknowledged, that MUST be treated as a connection error of type
 
 ### Stream Cancellation
 
-The instruction begins with the '01' two-bit pattern. The instruction includes
-the stream ID of the affected stream encoded as a 6-bit prefix integer.
+When a stream is reset, the decoder emits a Stream Cancellation instruction. The

...or reading is abandoned (i.e. STOP_SENDING)

> @@ -1035,17 +1038,19 @@ values that are not to be put at risk by compressing them (see Section 7.1 of
 ~~~~~~~~~~
 {: title="Literal Header Field With Name Reference"}
 
-For entries in the static table or in the dynamic table with an absolute index
-less than the Base, the header field name is represented using the relative
-index of that entry, which is represented as an integer with a 4-bit prefix (see
-Section 5.1 of [RFC7541]). The `S` bit indicates whether the reference is to the
-static (S=1) or dynamic (S=0) table.
+The fourth (`S`) bit indicates whether the reference is to the static (S=1) or
+dynamic (S=0) table.  Finally, the header field name is represented using the
+static or relative index of that entry, which is represented as an integer with

We define relative, absolute, and post-base indices.  We talk about "static table index" and "index into the static table".  "Static table index" is used on line 656 of the current doc.  This use of "static index" is a new variation.  I suspect a reader could reasonably infer that they all mean the same thing -- obviously we all have -- but it might be nice to align the terminology and make sure it's defined somewhere.

-- 
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/2759#pullrequestreview-252428023

---

From: Mike Bishop <[email-masked]>
Date: Thu, 20 Jun 2019 10:34:02 -0700

> @@ -344,7 +337,7 @@ MUST treat this as a connection error of type HTTP_QPACK_DECOMPRESSION_FAILED.

-The decoder signals key events by emitting decoder instructions
+The decoder signals the following key events by emitting decoder instructions

You can probably just replace "key" instead of adding.

-- 
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/2759#pullrequestreview-252432670

---

From: afrind <[email-masked]>
Date: Mon, 24 Jun 2019 17:02:14 -0700

afrind commented on this pull request.

Thanks for the comments all!

There isn't a requirement in QPACK that the blocks are "processed" in the order they are received on the stream -- that's an HTTP requirement.  I think I should change this to something else, that indicates that the encoder must interpret the acknowledgements in the order the frames were sent.  I came up with this, but it feels like it could use a MUST or MUST NOT in there:

"The encoder interprets each Header Acknowledgement instruction as acknowledging the earliest unacknowledged header block containing dynamic table references sent on the
given stream."

This is old language (see line 825 in the old spec).  I'm not clear on what is unclear.

 ~~~~~~~~~~

I added one sentence in the static table section describing the index.  I also normalized the three instructions that have an S bit to all use similar language.  For header block instructions, do I need to specify that 'relative' indexes are relative to Base from the enclosing header block?  Is it sufficient to just say 'relative' or 'relative to Base'?

I'm going to try omitting the entire sentence.  People will figure out that they should send a literal?

-- 
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/2759#pullrequestreview-253702347

---

From: Mike Bishop <[email-masked]>
Date: Tue, 25 Jun 2019 09:29:48 -0700

Maybe, but giving them a hint seems reasonable and generous.

-- 
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/2759#discussion_r297281029

---

From: Mike Bishop <[email-masked]>
Date: Tue, 25 Jun 2019 09:30:48 -0700

~~~~~~~~~~

I think it's sufficient to just call them a relative index; the section defining that term describes relative to what in different contexts.

-- 
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/2759#discussion_r297281567

---

From: Mike Bishop <[email-masked]>
Date: Tue, 25 Jun 2019 09:34:10 -0700

While blocked, header block data SHOULD remain in the blocked stream's flow
 control window.  A stream becomes unblocked when the Insert Count becomes
 greater than or equal to the Required Insert Count for all header blocks the
 decoder has started reading from the stream.
 <!-- doesn't the stream become unblocked when the encoder receives the acks? -->
 
-If the decoder encounters a header block with a Required Insert Count value
-larger than defined in {{blocked-streams}}, it MAY treat this as a connection
-error of type HTTP_QPACK_DECOMPRESSION_FAILED.  If the decoder encounters a
-header block with a Required Insert Count value smaller than defined in
-{{blocked-streams}}, it MUST treat this as a connection error of type
-HTTP_QPACK_DECOMPRESSION_FAILED as prescribed in {{invalid-references}}.
+When processing header blocks, the decoder expects the Required Insert Count
+value to exactly match the value defined in {{blocked-streams}. If it encounters
+a smaller value, it MUST treat this as a connection error of type
+HTTP_QPACK_DECOMPRESSION_FAILED (see {{invalid-references}}). If it encounters a
+larger value, it MAY treat this as a connection error of type
+HTTP_QPACK_DECOMPRESSION_FAILED.

I worry slightly that people might get confused about which "value" is smaller or larger than which other "value", because that term is used to describe both.

-- 
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/2759#pullrequestreview-254129160

---

From: afrind <[email-masked]>
Date: Wed, 26 Jun 2019 10:51:10 -0700

This is covered in the "Encoder" section: 

> An encoder converts a header list into a header block by emitting either an indexed or a literal representation for each header field in the list
...
References to the static table and literal representations do not require any dynamic state and never risk head-of-line blocking

I guess it doesn't specifically state the encoder "can always send" these representations, but I'm going for less=more here.  Maybe we can add it later, if really needed.  I don't think it belongs in Avoiding Blocked Insertions, since optimistic insertions could also be blocked, and have nothing to do with literal representations.

-- 
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/2759#discussion_r297792420

---

From: afrind <[email-masked]>
Date: Wed, 26 Jun 2019 11:00:38 -0700

@afrind pushed 1 commit.

51aed833ad799b817ab2ca16a68fe2ad30b3e2f4  More feedback

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2759/files/8dfe13435f086a0c5197027e2f223c0f0122907f..51aed833ad799b817ab2ca16a68fe2ad30b3e2f4

---

From: afrind <[email-masked]>
Date: Wed, 26 Jun 2019 11:19:25 -0700

Merged #2759 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/2759#event-2441793456