Subject: [quicwg/base-drafts] PUSH_ID as a frame (#2526)

From: Luca Niccolini <[email-masked]>
Date: Sun, 17 Mar 2019 12:34:11 -0700

the push id that links a push stream to a push promise is now an unframed varint that comes right after the stream preface. 

this is right now the only unframed data (except made for the unidirectional stream prefaces) in the specs and I am proposing to make that a frame as well for consistency and to simplify implementation and allow code reuse. 

motivation: the specs already define frames that only carry a single varint (mostly push related)
  * max push id
  * duplicate push 
  * cancel push
  * and goaway!
here instead we decided to leave it unframed just because we can since it is at the beginning of the stream

cost: two extra bytes per push stream

the text will need to specify that on a push stream this new frame (PUSH_ID ?) MUST be the first one, otherwise it is a protocol error. 

I am happy to put up a PR, in case we reach consensus

-- 
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/2526

---

From: Martin Thomson <[email-masked]>
Date: Sun, 17 Mar 2019 13:38:40 -0700

The varint type and length of a frame is also unframed.  So you are asking to trade one unframed varint for two.  Is your assertion that this is unique and difficult code to write, or that the unframed content is somehow difficult to manage?

-- 
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/2526#issuecomment-473712393

---

From: Lucas Pardue <[email-masked]>
Date: Sun, 17 Mar 2019 13:59:03 -0700

This seems to be going back to some of the early discussion we had on a thread I can't find. I'm not against discussing things again because we have more experience. However, I don't see a compelling reason for the change. My implementation needs to parse unframed data because of the stream header and for QPACK streams. 

I think this change might make my implementation more complex. Reading multiple varints requires more work: more checks to see if the stream has enough data, more checks to ensure only 1 frame sent at a specific time (this is the same as SETTINGS) 

-- 
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/2526#issuecomment-473714174

---

From: Luca Niccolini <[email-masked]>
Date: Sun, 17 Mar 2019 15:44:37 -0700

> The varint type and length of a frame is also unframed. 

@martinthomson those are in the frame header -- so they are part of the frame. I am not sure what your point is here then.

Here I am assuming that an implementation has generic code to parse frames, and that parsing anything that is not part of a frame requires special handling. 
While this is implementation specific and there can definitely be counter examples, from an abstraction and consistency point of view I see no reason for doing it the way we are doing it now. 

I believe there should be two type of streams, framed (control, request, and push streams) and unframed (QPACK, and potentially extension streams). The push stream right now is a hybrid, that only becomes a framed stream after receiving that first PUSH_ID. If I am recalling correctly when we discussed potentially having DATA frames with zero-length to transition to unframed data, it was also considered a bit unusual. Here I am arguing that we are sort of doing the reverse here where we have an unframed stream that then transitions to framed. I think this generates complexity. And really the benefit we get is saving two bytes and one frame type.

@lpardue I do vaguely remember that discussion and not 'wasting' frame types may have been one of the arguments when we made that decision, but that is not a problem now that we have a 62bit space for frame types. 

-- 
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/2526#issuecomment-473722917

---

From: Martin Thomson <[email-masked]>
Date: Sun, 17 Mar 2019 16:10:31 -0700

I don't care about the cost, I care about things like having to deal with PUSH_ID (or whatever) frames in places other than the start of a push stream.  And I don't believe that it is difficult to take the special-case code for pulling a varint off a partially-available stream and use that for push ID.  We've not had too much trouble with that so far in our implementation.

-- 
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/2526#issuecomment-473725100

---

From: Luca Niccolini <[email-masked]>
Date: Sun, 17 Mar 2019 16:36:30 -0700

it is definitely not difficult, but it is a special case and as any special case forces an increased complexity in the possible codepaths, even if not that big in this case. 
I am making my argument for the sake of consistency mostly, and for the fact that I believe it would also help to better define error handling.  
With the current text, if I receive something bogus on a push stream (after the preface) I'll have to keep the stream around (because the push stream may come out of order) waiting for a PUSH_PROMISE with that ID that may never come. 
So I believe that framing could help error handling as well. 

I do not have super strong feelings about this proposal but at the same time I'd like us to try and recall what the motivations are for the current text and for making this special since it was a while back and personally I can't recall what they were.

-- 
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/2526#issuecomment-473727176

---

From: Lucas Pardue <[email-masked]>
Date: Sun, 17 Mar 2019 16:50:25 -0700

> With the current text, if I receive something bogus on a push stream (after the preface) I'll have to keep the stream around (because the push stream may come out of order) waiting for a PUSH_PROMI…

This sounds like the case @rmarx is looking at. I'd argue this is a good reason for the current design, frame processing should only take place once the PUSH_PROMISE is received. 

But I'd like to understand how framing could help avoid errors. 

-- 
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/2526#issuecomment-473728271

---

From: Luca Niccolini <[email-masked]>
Date: Mon, 18 Mar 2019 06:45:22 -0700

I was not trying to solve the same problem as #2527 here, but I was mentioning the fact that I believe framing would slightly help in a subset of cases. 
Take a server with a buggy push implementation, rather than a malicious one. Something that doesn't send the PUSH_ID at all. On the client you'll start seeing a flow of data and you can't do anything better than parsing the first few bytes of that as a PUSH_ID, and basically see random IDs. With Framing, if a server forgets to send the PUSH_ID, it is clear at the receiver that it is a protocol error.
That being said there are many other classes of bugs that may trigger the same behavior even with frames, so this is minor. 

> frame processing should only take place once the PUSH_PROMISE is received. 
@LPardue now, that seems like an implementation dependent claim :)
 
as #2527 says (and I like it) a client should buffer any data after receiving the PUSH_ID. and that is sufficient. no mention of frames there.

-- 
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/2526#issuecomment-473915586

---

From: Lucas Pardue <[email-masked]>
Date: Mon, 18 Mar 2019 11:59:42 -0700

> Take a server with a buggy push implementation, rather than a malicious one. Something that doesn't send the PUSH_ID at all. On the client you'll start seeing a flow of data and you can't do anythi…

Right now in draft 19 we have:

1. Parse varint  - stream type
2. Parse varint - push ID
3. Parse varint - frame type (HEADERS). 

The assertion then is, if step 2 gets skipped by the sender, the receiver will parse the HEADERS frame type as a varint expecting it to be the push ID (value 0x1, with no restriction on min or max encoding). It will then parse the HEADERS frame length as a varint expecting it to be the HEADERS frame type (length is unlikely to be 1). Doesn't the client just blow up at that point?

-- 
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/2526#issuecomment-474056430

---

From: Luca Niccolini <[email-masked]>
Date: Mon, 18 Mar 2019 12:35:49 -0700

It was just an example. And as I said I don't feel strongly about the error handling nit. 
But if you must, yes if you assume headers is the first frame after the push_id then yes you would read 0x1 which is conveniently a server initiated bidirectional stream which is not allowed anyway, so you could reject it immediately. But I find this just fortuitous. 

For example, one could make a counter argument: why are we framing SETTINGS at all given that it is the first message on the control stream ? 
We also defined it quite clearly in this table what to do with it [Certain frames can only occur as the first frame of a particular stream type; these are indicated in Table 1 with a (1).] 
https://tools.ietf.org/html/draft-ietf-quic-http-19#section-4
Is there zero value in doing that for the PUSH_ID as well? 
I personally think there is.

Anyway, I think we are diverting this the wrong route. 
All I am looking for here is a good explanation for why it is defined like it is now, and I am failing to find it. 
As far as the implementation goes it makes little difference, and I guess it is more of a consistency problem for me. That naked varint on the wire triggers my OCD :) 

So instead of going back and forth like this, perhaps @MikeBishop can shed some light on the good reasons why it is this way.

-- 
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/2526#issuecomment-474068879

---

From: Lucas Pardue <[email-masked]>
Date: Mon, 18 Mar 2019 13:29:20 -0700

+1 to levelling up, I'll take acknowledge my guilt here. But before I move on:

> push_id then yes you would read 0x1 which is conveniently a server initiated bidirectional stream which is not allowed anyway, so you could reject it immediately

If your at the stage of parsing QUIC STREAM frame payload, you should already know exactly what the stream ID is.

Some historic points I remember:

In Paris MT presented some ideas for unidirecitonal streams. This relates to issue https://github.com/quicwg/base-drafts/issues/281 and PR https://github.com/quicwg/base-drafts/pull/701.

Push and control streams were identified then. And we were happy with reserving specific stream IDs for certain things. All non-reserved streams were server push streams. Subsequent data on the stream only makes logical sense to handle if you know what push it relates to, so those bytes go first. 

Roll forward some, QPACK design requires additional streams so those get reserved to. Around Kista, the topic of unidirectional stream types comes up. Without types, we make uncoordinated unidirecitonal stream extensions quite a difficult thing. So add more bytes at the start of stream and remove reservations.

> For example, one could make a counter argument: why are we framing SETTINGS at all given that it is the first message on the control stream ?

The SETTINGS frame can contain a set of parameters, indeed it SHOULD contain at least one GREASE setting. A single SETTINGS frame that describes the length of all parameters is quite useful, you know when the peer has finished sending all of them. I don't see as much value for PUSH_PROMISE because you're just framing a single integer that describes its own length already. 

-- 
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/2526#issuecomment-474088075

---

From: Luca Niccolini <[email-masked]>
Date: Mon, 18 Mar 2019 13:30:18 -0700

> But if you must, yes if you assume headers is the first frame after the push_id then yes you would read 0x1 which is conveniently a server initiated bidirectional stream which is not allowed anyway…

oh gosh yeah, I confused PUSH_ID with a stream id. scratch that completely

-- 
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/2526#issuecomment-474088408