http2: move merging of HEADERS and CONTINUATION into Framer
HEADERS and CONTINUATION frames are special in that they must appear
contiguous on the wire and there are lots of annoying details to
verify while working through its state machine, including the handling
of hpack header list size limits and DoS vectors.
We now have three implementations of this merging (Server, Transport,
and grpc), and grpc's is not complete. The Transport's was also
partially incomplete.
Move this up to the Framer (opt-in, for compatibility) and remove the
support from the Server and Transport. I can fix grpc later to use
this.
Recommended reviewing order:
* hpack.go exports the HeaderField.Size method and adds an IsPseudo
method.
* errors.go adds some new unexported error types, for testing.
* frame.go adds the new type MetaHeadersFrame.
* frame.go adds new fields on Framer for controlling how ReadFrame
behaves
* frame.go Framer.ReadFrame now calls the new Framer.readMetaFrame
method
* frame_test.go adds a bunch of tests. these are largely redundant
with the existing tests which were in server and transport
before. They really belong with frame_test.go, but I also don't want
to delete tests in a CL like this. I probably won't remove them
later either.
* server.go and transport.go can be reviewed in either order at this
point. Both are the fun part of this change: deleting lots of hairy
state machine code (which was redundant in at least 6 ways: server
headers, server trailers, client headers, client trailers, grpc
headers, grpc trailers...). Both server and transport.go have the
general following form:
- set Framer.ReadMetaHeaders
- stop handling *HeadersFrame and *ContinuationFrame; handle
*MetaHeadersFrame instead.
- delete all the state machine + hpack parsing callback hell
The diffstat numbers look like a wash once you exclude the new tests,
but this pays for itself by far when you consider the grpc savings as
well, and the increased simplicity.
Change-Id: If348cf585165b528b7d3ab2e5f86b49a03fbb0d2
Reviewed-on: https://go-review.googlesource.com/19726
Reviewed-by: Blake Mizerany <blake.mizerany@gmail.com>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
diff --git a/http2/transport_test.go b/http2/transport_test.go
index 3942873..e812011 100644
--- a/http2/transport_test.go
+++ b/http2/transport_test.go
@@ -1104,7 +1104,7 @@
testTransportInvalidTrailer_Pseudo(t, splitHeader)
}
func testTransportInvalidTrailer_Pseudo(t *testing.T, trailers headerType) {
- testInvalidTrailer(t, trailers, errPseudoTrailers, func(enc *hpack.Encoder) {
+ testInvalidTrailer(t, trailers, pseudoHeaderError(":colon"), func(enc *hpack.Encoder) {
enc.WriteField(hpack.HeaderField{Name: ":colon", Value: "foo"})
enc.WriteField(hpack.HeaderField{Name: "foo", Value: "bar"})
})
@@ -1117,19 +1117,19 @@
testTransportInvalidTrailer_Capital(t, splitHeader)
}
func testTransportInvalidTrailer_Capital(t *testing.T, trailers headerType) {
- testInvalidTrailer(t, trailers, errInvalidHeaderFieldName, func(enc *hpack.Encoder) {
+ testInvalidTrailer(t, trailers, headerFieldNameError("Capital"), func(enc *hpack.Encoder) {
enc.WriteField(hpack.HeaderField{Name: "foo", Value: "bar"})
enc.WriteField(hpack.HeaderField{Name: "Capital", Value: "bad"})
})
}
func TestTransportInvalidTrailer_EmptyFieldName(t *testing.T) {
- testInvalidTrailer(t, oneHeader, errInvalidHeaderFieldName, func(enc *hpack.Encoder) {
+ testInvalidTrailer(t, oneHeader, headerFieldNameError(""), func(enc *hpack.Encoder) {
enc.WriteField(hpack.HeaderField{Name: "", Value: "bad"})
})
}
func TestTransportInvalidTrailer_BinaryFieldValue(t *testing.T) {
- testInvalidTrailer(t, oneHeader, errInvalidHeaderFieldValue, func(enc *hpack.Encoder) {
- enc.WriteField(hpack.HeaderField{Name: "", Value: "has\nnewline"})
+ testInvalidTrailer(t, oneHeader, headerFieldValueError("has\nnewline"), func(enc *hpack.Encoder) {
+ enc.WriteField(hpack.HeaderField{Name: "x", Value: "has\nnewline"})
})
}
@@ -1147,7 +1147,7 @@
}
slurp, err := ioutil.ReadAll(res.Body)
if err != wantErr {
- return fmt.Errorf("res.Body ReadAll error = %q, %v; want %v", slurp, err, wantErr)
+ return fmt.Errorf("res.Body ReadAll error = %q, %#v; want %T of %#v", slurp, err, wantErr, wantErr)
}
if len(slurp) > 0 {
return fmt.Errorf("body = %q; want nothing", slurp)
