Misc Comments on Layering layering work and sections 1-5.

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