Subject: Re: [quicwg/base-drafts] Avoid data corruption with wrapped Largest Reference. (#2261)

From: Mike Bishop <[email-masked]>
Date: Tue, 08 Jan 2019 18:51:03 +0000 (UTC)

MikeBishop approved this pull request.

This looks reasonable.

>  An encoder MUST NOT insert an entry into the dynamic table (or duplicate an
-existing entry) if doing so would evict an entry with unacknowledged references.
-For header blocks that might rely on the newly added entry, the encoder can use
-a literal representation and maybe insert the entry later.
+existing entry) if doing so would evict a blocking entry.  For header blocks
+that would rely on the newly added entry, the encoder can instead use a literal
+representation and maybe insert the entry later.

With the added language, this line feels more ambiguous about which "the entry" is being referenced.  (Of course, it was somewhat ambiguous before.)  Maybe just drop the "and maybe insert the entry later"?

-- 
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/2261#pullrequestreview-190388996

---

From: Bence Béky <[email-masked]>
Date: Wed, 23 Jan 2019 08:18:24 -0800

Rebased on #2111.  Please let me know if anybody can come up with a term better than "blocking" which is confusing given that streams can be "blocked".

-- 
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/2261#issuecomment-456865100

---

From: Bence Béky <[email-masked]>
Date: Wed, 23 Jan 2019 16:19:14 +0000 (UTC)

bencebeky commented on this pull request.

> @@ -324,10 +332,9 @@ permit the encoder to track the decoder's state.  These events are:
 
 Knowledge that a header block with references to the dynamic table has been
 processed permits the encoder to evict entries to which no unacknowledged
-references remain, regardless of whether those references were potentially
-blocking (see {{blocked-insertion}}).  When a stream is reset or abandoned, the

This makes no sense: every unacknowledged reference is blocking eviction.

-- 
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/2261#pullrequestreview-195611281

---

From: Martin Thomson <[email-masked]>
Date: Wed, 23 Jan 2019 14:10:15 -0800

martinthomson approved this pull request.

Some stylistic changes as suggestions (damn I wish GitHub supported multi-line suggestions), but this is generally much better.

> @@ -210,10 +210,18 @@ acknowledged by the decoder.
 
 ### Blocked Dynamic Table Insertions {#blocked-insertion}
 
+The encoder considers a dynamic table entry to be blocking if it has not yet
+received acknowledgement of insertion of the entry, or if it has not yet
+received acknowledgement of all references to the entry.  Note that a dynamic
+table entry that has never been referenced can still be blocking.  Also note

This "Note that" sentence is redundant.

> @@ -210,10 +210,18 @@ acknowledged by the decoder.
 
 ### Blocked Dynamic Table Insertions {#blocked-insertion}
 
+The encoder considers a dynamic table entry to be blocking if it has not yet
+received acknowledgement of insertion of the entry, or if it has not yet
+received acknowledgement of all references to the entry.  Note that a dynamic
+table entry that has never been referenced can still be blocking.  Also note
+that a blocking entry cannot be evicted, which is unrelated to a blocked stream,

Suggest: 
```
A blocking entry cannot be evicted until its insertion has been acknowledged and there are no unacknowledged references to the entry outstanding.
--new paragraph
Note:
: A blocking entry is unrelated to a blocked stream, where a decoder cannot decode a stream as a result of references to entries that are not yet available.  An encoder that uses the dynamic table has to manage the possibility of blocked entries, whereas a decoder can forbid an encoder from making references to unacknowledged entries.
```

-a literal representation.

Not your text, but if we're talking about not being able to insert, then this really needs to say something like: An encoder that cannot insert entries in the dynamic table sends literal representations of header fields.

 Knowledge that a header block with references to the dynamic table has been
 processed permits the encoder to evict entries to which no unacknowledged
-references remain, regardless of whether those references were potentially
-blocking (see {{blocked-insertion}}).  When a stream is reset or abandoned, the
-indication that these header blocks will never be processed serves a similar
-function; see {{stream-cancellation}}.
+references remain; see {{blocked-insertion}}.  When a stream is reset or

Style nit: I think that we've been using the parenthetical form for this sort of reference.  I think that this is right, even the semi-colon, which I initially stumbled over, but consistency is somewhat important.

-- 
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/2261#pullrequestreview-195765011

---

From: Bence Béky <[email-masked]>
Date: Thu, 24 Jan 2019 17:00:13 -0800

Indeed it is.  However, I think it is worth keeping, especially because at two places in the spec this aspect of blocking has been overlooked.

-- 
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/2261#discussion_r250834601

---

From: Bence Béky <[email-masked]>
Date: Thu, 24 Jan 2019 17:05:50 -0800

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/2261#discussion_r250835619

---

From: Bence Béky <[email-masked]>
Date: Thu, 24 Jan 2019 17:07:55 -0800

I agree with you.  Sorry, I got inspired by the initial semicolon in this section.

-- 
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/2261#discussion_r250835992

---

From: Bence Béky <[email-masked]>
Date: Thu, 24 Jan 2019 17:14:00 -0800

I have a slight preference to emphasize that "blocking" is a concept from the encoder's point of view, for the sake of clarity, but I do not mind dropping that.

However, let me rephase that as "an entry is considered blocking and cannot be evicted" so that this sentence acts an a definition of blocking.  Otherwise the property "blocking" is not defined, and it is not clear if after the insertion is acknowledged and so are all references, the entry will become a kind of blocking entry that can be evicted, or an entry that is not blocking any longer.

-- 
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/2261#discussion_r250836998

---

From: Bence Béky <[email-masked]>
Date: Thu, 24 Jan 2019 17:14:46 -0800

@bencebeky pushed 1 commit.

3701ac562a6e53cd83f10177736c0d4a460d44e1  Incorporate some of @martinthomson's comments.

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2261/files/dd8ad0fe537d3b7556b04f7b1f152dda9afd1b05..3701ac562a6e53cd83f10177736c0d4a460d44e1

---

From: Bence Béky <[email-masked]>
Date: Thu, 24 Jan 2019 17:16:14 -0800

@martinthomson:  Thank you for your feedback.

-- 
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/2261#issuecomment-457419807

---

From: Martin Thomson <[email-masked]>
Date: Thu, 24 Jan 2019 19:59:19 -0800

-- 
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/2261#pullrequestreview-196351282