Subject: [quicwg/base-drafts] MAX_PUSH_ID: lower-than-previous value conn error seems to severe (#2412)

From: Lucas Pardue <[email-masked]>
Date: Mon, 04 Feb 2019 13:55:01 -0800

The 18 spec says:

> A MAX_PUSH_ID frame cannot reduce the maximum Push ID; receipt of a MAX_PUSH_ID that contains a smaller value than previously received MUST be treated as a connection error of type HTTP_MALFORMED_FRAME.

This feels like another case of an error that isn't strictly a frame error. Its the data inside that is semantically wrong. The connection error severity also seems pretty extreme, and I question if its really needed. I think the appropriate fix is to borrow logic from the Transport's similar MAX_STREAMS frame:

> Loss or reordering can cause a MAX_STREAMS frame to be received which
   states a lower stream limit than an endpoint has previously received.
   MAX_STREAMS frames which do not increase the stream limit MUST be
   ignored.

The proposal therefore is to say that MAX_PUSH_ID can only increase the value, anything else gets ignored.

-- 
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/issues/2412

---

From: Martin Thomson <[email-masked]>
Date: Mon, 04 Feb 2019 14:53:39 -0800

A connection error is entirely appropriate.  The awkward compromise we have in the transport docs exists because there is at least a plausible argument in support of ignoring regressions in limits.  There, reordering and retransmission schemes might produce the situation (even if the latter might be somewhere between inadvisable and malicious).  Here, the frames appear on a strictly ordered stream with no chance that it is going to be missed.  I see no reason to treat this leniently.

The error code question I have no opinion on, other than to suggest the application of principles already articulated.

-- 
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/issues/2412#issuecomment-460445798

---

From: Lucas Pardue <[email-masked]>
Date: Mon, 04 Feb 2019 23:07:08 +0000 (UTC)

I see your point of view. The downside is that the client application layer needs to coordinate its sending of this frame to ensure the sequence is strictly growing - this constrains some of the design choices an implementation might choose to make. 

I question the severity because I don't know why a server would get so upset by seeing a value that is smaller than it had before. If the server is at the current max streams amount, it is unable to generate pushes until something increases the max. What could possibly go wrong? ™️ I don't want to fall into the trap of justifying ever decision but this one is not resonating with me probably because I am missing something.

-- 
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/issues/2412#issuecomment-460449754

---

From: ianswett <[email-masked]>
Date: Mon, 04 Feb 2019 16:55:46 -0800

Given this is delivered in order on the control stream, requiring it never decrease seems sensible.

-- 
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/issues/2412#issuecomment-460473677

---

From: Lucas Pardue <[email-masked]>
Date: Mon, 04 Feb 2019 17:40:26 -0800

So the hypothetical implementation I have in mind is a client that receives pushes that can be processed independently. In some edge cases, pushes may have mixed use of static and dynamic tables. Static-only pushes can make progress without reliance on QPACK stream(s) progressing. I might set up a handling chain for a single push that does something like "when complete, send MAX_PUSH_ID with value current push ID + 1". There may be cases where this leads to a lower limit being sent.

Another edge case is naive handling of CANCEL_PUSH when it is sent by the server. A client that wants to maintain a number of concurrent pushes needs to update MAX_PUSH_ID pretty quick, and a simple way to do this is to read the Push ID out of CANCEL_PUSH and increment by 1. Again this may cause a lower limit being sent.

One solution to these problems is, on the client side, to track the high water mark of Push ID and only emit a MAX_PUSH_ID frame that increases the high water mark. 

-- 
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/issues/2412#issuecomment-460482554

---

From: Martin Thomson <[email-masked]>
Date: Mon, 04 Feb 2019 17:42:46 -0800

Don't you need a synchronization point when it comes to putting the bytes in the stream buffer 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/issues/2412#issuecomment-460483030

---

From: Lucas Pardue <[email-masked]>
Date: Mon, 04 Feb 2019 17:54:39 -0800

Yes, but whether that synchronisation point coordinates based on H3 frames or simply STREAM data seems like an implementation detail. I.e. I call frame.to_bytes() and then feed that to my transport library API.

The upside to doing limit increase cleverly on the client, is that you open the opportunity to aggregate push limit increments. I.e. send one frame that increments by a single larger value.

If we were to bring the discussion  back to principles, is the intent of the connection error here because sending a lower limit is meaningless for both sides?

-- 
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/issues/2412#issuecomment-460485181

---

From: Martin Thomson <[email-masked]>
Date: Tue, 05 Feb 2019 03:16:52 -0800

Oh, principles.  The principle at play is that if a peer is demonstrably not participating in the same protocol, stop communicating with them as soon as you detect that so they can sort things out.  There are cases where meaningless messages can be ignored as harmless, but it's best not to ignore them unless ignoring is difficult.  I still think we made the wrong choice with MAX_DATA and friends, but at least there the extenuating circumstances are such that the choice is defensible.

-- 
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/issues/2412#issuecomment-460600967

---

From: Lucas Pardue <[email-masked]>
Date: Tue, 05 Feb 2019 03:19:29 -0800

Ok, I can get behind that. I have an alternate proposal at hand on #2418. Depending on how that goes I'll file a separate issue about adjusting some of the wording about the error type.

-- 
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/issues/2412#issuecomment-460601627

---

From: Mike Bishop <[email-masked]>
Date: Tue, 05 Feb 2019 13:59:12 -0800

And more pragmatically, you can't do a non-fatal stream error on the control stream.  If these moved to a separate unidirectional stream, I'd be open to the possibility of making an error kill push rather than the whole 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/issues/2412#issuecomment-460820506

---

From: Lucas Pardue <[email-masked]>
Date: Tue, 05 Feb 2019 14:03:09 -0800

That's what got me thinking about the unidirectional stream. Killing the stream is a very explicit signal, but also not dissimilar to running out of credit permanently. A stream error could be upgraded to connection error if a server really wanted...

-- 
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/issues/2412#issuecomment-460821802