summerwind/h2spec

3.5 & 5.4.1 should allow GOAWAY

apapirovski opened this issue · 2 comments

It's possible I'm missing something here but it seems like GOAWAY should be allowed in these two tests. The only other tests where VerifyConnectionClose seems to be used are ones where GOAWAY is first sent by h2spec.

3.5:

Clients and servers MUST treat an invalid connection preface as a connection error (Section 5.4.1) of type PROTOCOL_ERROR. A GOAWAY frame (Section 6.8) MAY be omitted in this case, since an invalid preface indicates that the peer is not using HTTP/2.

and

5.4.1:

An endpoint that encounters a connection error SHOULD first send a GOAWAY frame (Section 6.8) with the stream identifier of the last stream that it successfully received from its peer. The GOAWAY frame includes an error code that indicates why the connection is terminating. After sending the GOAWAY frame for an error condition, the endpoint MUST close the TCP connection.

Thank you for the comment!
I agree with 3.5. I fixed it to handle GOAWAY in 672e145. As for 5.4.1, I think there is no problem with the current test case. Can you explain more detail please?

The change in 672e145 requires a GOAWAY frame, where as the spec mentions:

A GOAWAY frame (Section 6.8) MAY be omitted in this case, since an invalid preface indicates that the peer is not using HTTP/2.