From 654daac3bce1314d5568b46bfa48ec4c546d7395 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Wed, 16 Dec 2015 19:47:42 +0000 Subject: [PATCH] net/http: split Trailers tests into two halves The old test was in client_test.go but was a mix of four things: - clients writing trailers - servers reading trailers - servers writing trailers - clients reading trailers It definitely wasn't just about clients. This moves it into clientserver_test.go and separates it into two halves: - servers writing trailers + clients reading trailers - clients writing trailers + servers reading trailers Which still isn't ideal, but is much better, and easier to read. Updates #13557 Change-Id: I8c3e58a1f974c1b10bb11ef9b588cfa0f73ff5d9 Reviewed-on: https://go-review.googlesource.com/17895 Run-TryBot: Brad Fitzpatrick Reviewed-by: Blake Mizerany TryBot-Result: Gobot Gobot --- src/net/http/client_test.go | 77 ------------------ src/net/http/clientserver_test.go | 126 ++++++++++++++++++++++++++++++ src/net/http/transfer.go | 30 ++++--- 3 files changed, 145 insertions(+), 88 deletions(-) diff --git a/src/net/http/client_test.go b/src/net/http/client_test.go index 3aa5b5d3ef..9d3444c89a 100644 --- a/src/net/http/client_test.go +++ b/src/net/http/client_test.go @@ -20,8 +20,6 @@ import ( . "net/http" "net/http/httptest" "net/url" - "reflect" - "sort" "strconv" "strings" "sync" @@ -1096,81 +1094,6 @@ func (f eofReaderFunc) Read(p []byte) (n int, err error) { return 0, io.EOF } -func TestClientTrailers_h1(t *testing.T) { testClientTrailers(t, h1Mode) } -func TestClientTrailers_h2(t *testing.T) { - t.Skip("skipping in http2 mode; golang.org/issue/13557") - testClientTrailers(t, h2Mode) -} -func testClientTrailers(t *testing.T, h2 bool) { - defer afterTest(t) - cst := newClientServerTest(t, h2, HandlerFunc(func(w ResponseWriter, r *Request) { - w.Header().Set("Connection", "close") - w.Header().Set("Trailer", "Server-Trailer-A, Server-Trailer-B") - w.Header().Add("Trailer", "Server-Trailer-C") - - var decl []string - for k := range r.Trailer { - decl = append(decl, k) - } - sort.Strings(decl) - - slurp, err := ioutil.ReadAll(r.Body) - if err != nil { - t.Errorf("Server reading request body: %v", err) - } - if string(slurp) != "foo" { - t.Errorf("Server read request body %q; want foo", slurp) - } - if r.Trailer == nil { - io.WriteString(w, "nil Trailer") - } else { - fmt.Fprintf(w, "decl: %v, vals: %s, %s", - decl, - r.Trailer.Get("Client-Trailer-A"), - r.Trailer.Get("Client-Trailer-B")) - } - - // How handlers set Trailers: declare it ahead of time - // with the Trailer header, and then mutate the - // Header() of those values later, after the response - // has been written (we wrote to w above). - w.Header().Set("Server-Trailer-A", "valuea") - w.Header().Set("Server-Trailer-C", "valuec") // skipping B - })) - defer cst.close() - - var req *Request - req, _ = NewRequest("POST", cst.ts.URL, io.MultiReader( - eofReaderFunc(func() { - req.Trailer["Client-Trailer-A"] = []string{"valuea"} - }), - strings.NewReader("foo"), - eofReaderFunc(func() { - req.Trailer["Client-Trailer-B"] = []string{"valueb"} - }), - )) - req.Trailer = Header{ - "Client-Trailer-A": nil, // to be set later - "Client-Trailer-B": nil, // to be set later - } - req.ContentLength = -1 - res, err := cst.c.Do(req) - if err != nil { - t.Fatal(err) - } - if err := wantBody(res, err, "decl: [Client-Trailer-A Client-Trailer-B], vals: valuea, valueb"); err != nil { - t.Error(err) - } - want := Header{ - "Server-Trailer-A": []string{"valuea"}, - "Server-Trailer-B": nil, - "Server-Trailer-C": []string{"valuec"}, - } - if !reflect.DeepEqual(res.Trailer, want) { - t.Errorf("Response trailers = %#v; want %#v", res.Trailer, want) - } -} - func TestReferer(t *testing.T) { tests := []struct { lastReq, newReq string // from -> to URLs diff --git a/src/net/http/clientserver_test.go b/src/net/http/clientserver_test.go index e54091f3b8..09dbceb99d 100644 --- a/src/net/http/clientserver_test.go +++ b/src/net/http/clientserver_test.go @@ -18,6 +18,7 @@ import ( "net/http/httptest" "os" "reflect" + "sort" "strings" "sync" "testing" @@ -465,3 +466,128 @@ func testCancelRequestMidBody(t *testing.T, h2 bool) { t.Errorf("ReadAll error = %v; want %v", err, ExportErrRequestCanceled) } } + +// Tests that clients can send trailers to a server and that the server can read them. +func TestTrailersClientToServer_h1(t *testing.T) { testTrailersClientToServer(t, h1Mode) } +func TestTrailersClientToServer_h2(t *testing.T) { + t.Skip("skipping in http2 mode; golang.org/issue/13557") + testTrailersClientToServer(t, h2Mode) +} + +func testTrailersClientToServer(t *testing.T, h2 bool) { + defer afterTest(t) + cst := newClientServerTest(t, h2, HandlerFunc(func(w ResponseWriter, r *Request) { + var decl []string + for k := range r.Trailer { + decl = append(decl, k) + } + sort.Strings(decl) + + slurp, err := ioutil.ReadAll(r.Body) + if err != nil { + t.Errorf("Server reading request body: %v", err) + } + if string(slurp) != "foo" { + t.Errorf("Server read request body %q; want foo", slurp) + } + if r.Trailer == nil { + io.WriteString(w, "nil Trailer") + } else { + fmt.Fprintf(w, "decl: %v, vals: %s, %s", + decl, + r.Trailer.Get("Client-Trailer-A"), + r.Trailer.Get("Client-Trailer-B")) + } + })) + defer cst.close() + + var req *Request + req, _ = NewRequest("POST", cst.ts.URL, io.MultiReader( + eofReaderFunc(func() { + req.Trailer["Client-Trailer-A"] = []string{"valuea"} + }), + strings.NewReader("foo"), + eofReaderFunc(func() { + req.Trailer["Client-Trailer-B"] = []string{"valueb"} + }), + )) + req.Trailer = Header{ + "Client-Trailer-A": nil, // to be set later + "Client-Trailer-B": nil, // to be set later + } + req.ContentLength = -1 + res, err := cst.c.Do(req) + if err != nil { + t.Fatal(err) + } + if err := wantBody(res, err, "decl: [Client-Trailer-A Client-Trailer-B], vals: valuea, valueb"); err != nil { + t.Error(err) + } +} + +// Tests that servers send trailers to a client and that the client can read them. +func TestTrailersServerToClient_h1(t *testing.T) { testTrailersServerToClient(t, h1Mode, false) } +func TestTrailersServerToClient_h2(t *testing.T) { + t.Skip("skipping in http2 mode; golang.org/issue/13557") + testTrailersServerToClient(t, h2Mode, false) +} +func TestTrailersServerToClient_Flush_h1(t *testing.T) { testTrailersServerToClient(t, h1Mode, true) } +func TestTrailersServerToClient_Flush_h2(t *testing.T) { + t.Skip("skipping in http2 mode; golang.org/issue/13557") + testTrailersServerToClient(t, h2Mode, true) +} + +func testTrailersServerToClient(t *testing.T, h2, flush bool) { + defer afterTest(t) + const body = "Some body" + cst := newClientServerTest(t, h2, HandlerFunc(func(w ResponseWriter, r *Request) { + w.Header().Set("Trailer", "Server-Trailer-A, Server-Trailer-B") + w.Header().Add("Trailer", "Server-Trailer-C") + + io.WriteString(w, body) + if flush { + w.(Flusher).Flush() + } + + // How handlers set Trailers: declare it ahead of time + // with the Trailer header, and then mutate the + // Header() of those values later, after the response + // has been written (we wrote to w above). + w.Header().Set("Server-Trailer-A", "valuea") + w.Header().Set("Server-Trailer-C", "valuec") // skipping B + w.Header().Set("Server-Trailer-NotDeclared", "should be omitted") + })) + defer cst.close() + + res, err := cst.c.Get(cst.ts.URL) + if err != nil { + t.Fatal(err) + } + + delete(res.Header, "Date") // irrelevant for test + if got, want := res.Header, (Header{ + "Content-Type": {"text/plain; charset=utf-8"}, + }); !reflect.DeepEqual(got, want) { + t.Errorf("Header = %v; want %v", got, want) + } + + if got, want := res.Trailer, (Header{ + "Server-Trailer-A": nil, + "Server-Trailer-B": nil, + "Server-Trailer-C": nil, + }); !reflect.DeepEqual(got, want) { + t.Errorf("Trailer before body read = %v; want %v", got, want) + } + + if err := wantBody(res, nil, body); err != nil { + t.Fatal(err) + } + + if got, want := res.Trailer, (Header{ + "Server-Trailer-A": {"valuea"}, + "Server-Trailer-B": nil, + "Server-Trailer-C": {"valuec"}, + }); !reflect.DeepEqual(got, want) { + t.Errorf("Trailer after body read = %v; want %v", got, want) + } +} diff --git a/src/net/http/transfer.go b/src/net/http/transfer.go index 38b6f67c42..b452f33ad6 100644 --- a/src/net/http/transfer.go +++ b/src/net/http/transfer.go @@ -577,21 +577,29 @@ func shouldClose(major, minor int, header Header, removeCloseHeader bool) bool { // Parse the trailer header func fixTrailer(header Header, te []string) (Header, error) { - raw := header.get("Trailer") - if raw == "" { + vv, ok := header["Trailer"] + if !ok { return nil, nil } - header.Del("Trailer") + trailer := make(Header) - keys := strings.Split(raw, ",") - for _, key := range keys { - key = CanonicalHeaderKey(strings.TrimSpace(key)) - switch key { - case "Transfer-Encoding", "Trailer", "Content-Length": - return nil, &badStringError{"bad trailer key", key} - } - trailer[key] = nil + var err error + for _, v := range vv { + foreachHeaderElement(v, func(key string) { + key = CanonicalHeaderKey(key) + switch key { + case "Transfer-Encoding", "Trailer", "Content-Length": + if err == nil { + err = &badStringError{"bad trailer key", key} + return + } + } + trailer[key] = nil + }) + } + if err != nil { + return nil, err } if len(trailer) == 0 { return nil, nil