- From: Mike Belshe <mike@belshe.com>
- Date: Fri, 5 Jul 2013 08:37:04 -0700
- To: httpbis mailing list <ietf-http-wg@w3.org>
- Message-ID: <CABaLYCsgQ2uP7VXE=KaHeMye0QFeFtTsBBgSfBSF6CgLfmjkbQ@mail.gmail.com>
Here are a bunch of comments on the latest spec. Some of these are really minor. Let me know the best way to provide this feedback. 2 high-level bits about the layering: a) State diagram for streams. This is a good diagram. Suggestion: Remove the "idle" state. The diagram (and text) states that all states that could ever exist are "idle", which seems like they exist somehow. So instead of starting in idle, why not just depict 4 entry points into the state machine? a) PP send -> takes you to reserved (local) b) PP receive -> takes you to reserved (remote) c) H send -> takes you to open d) H receive -> takes you to open b) Now that we're getting the stream lifecycle clean again, it will soon be a good time to rethink the naming. When you're new to the protocol, its not clear that "HEADERS" is a way to start a stream. START_STREAM would be cleaner. Similarly, PUSH_PROMISE requires a lot of knowledge and doesn't sound stream-ish. PROMISE_STREAM would be better. Sorry to bring up naming - but I do think this is extremely important for making the protocol easy to understand when folks read it for the first time. Section by section: 2. I would strike all this text- its redundant with text in the subsections and not really clarifying. 3 - 3.5. The term "Starting HTTP/2.0" is understandable, but the document lacks a basic "connection establishment" section. I would probably rework this to be "3. Connection management", and reword everything from that viewpoint. 4.1. In an effort to modernize protocol docs, I'd define in the definitions a "byte" as a "sequence of 8 bits", and then strike the word "octets" everywhere from the doc in favor of "bytes". 4.2. * Strike the note about PING, or move it to the section that deals with PING. * I didn't understand when this happens: "If a frame size exceeds any defined limit, or is too small to contain mandatory frame data" I'd strike this. 4.3 When I got to this section, it was very hard to find where we define what a header block looks like! We link to PP frames, error states, compression, etc. Section 6.2 refers back to 4.3 to define a header block, but neither section actually defines a header block! :-) 5.1 * We might reword the way we introduce the concept of 'associated stream' here. Example "Sending a PUSH_PROMISE frame marks the associated stream..." . Perhaps change to: "Sending a PP frame creates a stream promised for future use. Promised streams ar always associated with an existing, open stream." Editorial: it saddens me that so much specification complexity is added to the protocol for push when push is so secondary to the core protocol. * Closed stream: This text: "An endpoint MUST NOT send frames on a closed stream. An endpoint that receives a frame after receiving a RST_STREAM or a frame containing a END_STREAM flag on that stream MUST treat that as a stream error (Section 5.4.2) of type STREAM_CLOSED." If we move it down one paragraph (after the description of how we transition into it) can be simplified to: "An endpoint MUST NOT send frames on a closed stream. An endpoint that receives a frame while in the closed state MUST treat that as a stream error of type STREAM_CLOSED." 5.4 I wondered if this section (Error handling, both stream & connection) shouldn't be in section 4 (http framing), rather than section 5 (streams). Nothing seems to fit that well. I stopped here for time.... Thanks everyone for the work on this. I hope these comments are useful. Mike
Received on Friday, 5 July 2013 15:37:32 UTC