Subject: Frame size errors and GOAWAY prose still ambiguous

From: Jonathan Thackray <[email-masked]>
Date: Mon, 29 Sep 2014 11:33:22 +0100

Hi,

I've been implementing the HTTP/2-draft14 specification for a couple of
months now, and have an client implementation that (so far) works with
nghttp2 and webtide.com.

Here are some of issues/ambiguities I found whilst following the -draft14
specification, which haven't yet been fixed in the editor's copy:

Frame size errors: I'm glad to see Martin updated the editor's copy on
Friday in this commit: "Tightening frame size restrictions efa9a6d" to
make errors more explicit, which I'd noticed weren't well defined.
However, I'm puzzled that all additions in Martin's commit are all
connection-level frame size errors. Section 4.2 says:

   "A frame size error in a frame that could alter the state of the
    entire connection MUST be treated as a connection error
    (Section 5.4.1); this includes any frame carrying a header block
    (Section 4.3) (that is, HEADERS, PUSH_PROMISE, and CONTINUATION),
    SETTINGS, and any WINDOW_UPDATE frame with a stream identifier of 0."

which I assume means any other frame size error should be a stream
error? For example, if a PRIORITY frame has length 7 octets (not the
legal 5 octets) shouldn't the connection continue, and just that one
stream be sent a RST_STREAM frame size error? The quoted text above
seems to imply this.

Likewise, if SETTINGS_MAX_FRAME_SIZE is still at the default value of 2^14
octets, and a DATA frame is received of 2^15 octets, that would be a frame
size stream error, I assume? Could we clarify this text to be more explicit?

The description of the GOAWAY frame is slightly inconsistent on first
reading, as its usage is mentioned in a few places. For example,
section 5.4.1 says:

  "A connection error is any error which prevents further processing of
  the framing layer, or which corrupts any connection state. After sending
  the GOAWAY frame, the endpoint MUST close the TCP connection."

which in my mind implies "close immediately", whereas section 6.8 says:

  "The purpose of this frame is to allow an endpoint to gracefully stop
  accepting new streams, while still finishing processing of previously
  established streams."

and then mentions that 1 round-trip time can be waited before sending
another GOAWAY, and then closing the TCP connection.

So on one hand it appears that GOAWAY is used if corruption is detected
and is immediately fatal, and the other sending multiple GOAWAY frames
as an administrative convenience, and then eventually closing the TLS
or TCP connection. Presumably it's too late to define a new "CLOSING"
frame type, which might be more appropriate?

It might be worth describing the both uses of GOAWAY in a single place
in the document, so that readers who read one then the other don't start
thinking that GOAWAY is inconsistently defined. Also, the text mentions
1 round-trip-time. Presumably my implementation should send a PING packet,
or obtain an RTT TCP metric from the kernel?

Other minor stylistic issues:

Most of the references to "byte" have now been changed to "octet",
but a couple of unchanged sentences remain:
   "use those bytes in any fashion"
   "All numeric values are in network byte order"

There are some inconsistencies in quoting defaults. I'd be happy with
either hex or decimal, but I think the spefication should be consistent
throughout:

   "The legal range for the increment to the flow control window
    is 1 to 2^31-1 (0x7fffffff) octets."

   "This setting can have any value between 214 (16,384) and 2^24-1
    (16,777,215) octets, inclusive."

Many thanks,
Jonathan.

---

From: Martin Thomson <[email-masked]>
Date: Mon, 29 Sep 2014 11:08:12 -0700

Thanks for the comments.  You can find the commits in the usual place.

> connection-level frame size errors. Section 4.2 says:

Most have to be.  Though I was a little overzealous regarding
PRIORITY. [1885874]

> size stream error, I assume? Could we clarify this text to be more explicit?

[cff888e]

>   the GOAWAY frame, the endpoint MUST close the TCP connection."

This section is all about errors. [44aff204]

> frame type, which might be more appropriate?

That's what NO_ERROR is.

> Other minor stylistic issues:

[8fe00fd]