From 7cbc1058ea9240d9df92a339932d5c6dce694e7a Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Fri, 2 Sep 2016 05:00:05 +0000 Subject: [PATCH] net/http/httputil: make ReverseProxy send nil Body requests when possible The http.Transport's retry can't retry requests with non-nil bodies. When cloning an incoming server request into an outgoing client request, nil out the Body field if the ContentLength is 0. (For server requests, Body is always non-nil, even for GET, HEAD, etc.) Also, don't use the deprecated CancelRequest and use Context instead. And don't set Proto, ProtoMajor, ProtoMinor. Those are ignored in client requests, which was probably a later documentation clarification. Fixes #16036 Updates #16696 (remove useless Proto lines) Change-Id: I70a869e9bd4bf240c5838e82fb5aa695a539b343 Reviewed-on: https://go-review.googlesource.com/28412 Run-TryBot: Brad Fitzpatrick TryBot-Result: Gobot Gobot Reviewed-by: Joe Tsai --- src/go/build/deps_test.go | 2 +- src/net/http/httputil/reverseproxy.go | 70 ++++++---------------- src/net/http/httputil/reverseproxy_test.go | 41 +++++++++++-- 3 files changed, 56 insertions(+), 57 deletions(-) diff --git a/src/go/build/deps_test.go b/src/go/build/deps_test.go index caacb14f7e..cb8f95fb1d 100644 --- a/src/go/build/deps_test.go +++ b/src/go/build/deps_test.go @@ -392,7 +392,7 @@ var pkgDeps = map[string][]string{ "net/http/cookiejar": {"L4", "NET", "net/http"}, "net/http/fcgi": {"L4", "NET", "OS", "net/http", "net/http/cgi"}, "net/http/httptest": {"L4", "NET", "OS", "crypto/tls", "flag", "net/http", "net/http/internal"}, - "net/http/httputil": {"L4", "NET", "OS", "net/http", "net/http/internal"}, + "net/http/httputil": {"L4", "NET", "OS", "context", "net/http", "net/http/internal"}, "net/http/pprof": {"L4", "OS", "html/template", "net/http", "runtime/pprof", "runtime/trace"}, "net/rpc": {"L4", "NET", "encoding/gob", "html/template", "net/http"}, "net/rpc/jsonrpc": {"L4", "NET", "encoding/json", "net/rpc"}, diff --git a/src/net/http/httputil/reverseproxy.go b/src/net/http/httputil/reverseproxy.go index 79831b3a97..47cd0ae97d 100644 --- a/src/net/http/httputil/reverseproxy.go +++ b/src/net/http/httputil/reverseproxy.go @@ -7,6 +7,7 @@ package httputil import ( + "context" "io" "log" "net" @@ -120,68 +121,35 @@ var hopHeaders = []string{ "Upgrade", } -type requestCanceler interface { - CancelRequest(*http.Request) -} - -type runOnFirstRead struct { - io.Reader // optional; nil means empty body - - fn func() // Run before first Read, then set to nil -} - -func (c *runOnFirstRead) Read(bs []byte) (int, error) { - if c.fn != nil { - c.fn() - c.fn = nil - } - if c.Reader == nil { - return 0, io.EOF - } - return c.Reader.Read(bs) -} - func (p *ReverseProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) { transport := p.Transport if transport == nil { transport = http.DefaultTransport } + ctx := req.Context() + if cn, ok := rw.(http.CloseNotifier); ok { + var cancel context.CancelFunc + ctx, cancel = context.WithCancel(ctx) + defer cancel() + notifyChan := cn.CloseNotify() + go func() { + select { + case <-notifyChan: + cancel() + case <-ctx.Done(): + } + }() + } + outreq := new(http.Request) *outreq = *req // includes shallow copies of maps, but okay - - if closeNotifier, ok := rw.(http.CloseNotifier); ok { - if requestCanceler, ok := transport.(requestCanceler); ok { - reqDone := make(chan struct{}) - defer close(reqDone) - - clientGone := closeNotifier.CloseNotify() - - outreq.Body = struct { - io.Reader - io.Closer - }{ - Reader: &runOnFirstRead{ - Reader: outreq.Body, - fn: func() { - go func() { - select { - case <-clientGone: - requestCanceler.CancelRequest(outreq) - case <-reqDone: - } - }() - }, - }, - Closer: outreq.Body, - } - } + if req.ContentLength == 0 { + outreq.Body = nil // Issue 16036: nil Body for http.Transport retries } + outreq = outreq.WithContext(ctx) p.Director(outreq) - outreq.Proto = "HTTP/1.1" - outreq.ProtoMajor = 1 - outreq.ProtoMinor = 1 outreq.Close = false // Remove headers with the same name as the connection-tokens. diff --git a/src/net/http/httputil/reverseproxy_test.go b/src/net/http/httputil/reverseproxy_test.go index bfa13d9b6d..8e181dbb04 100644 --- a/src/net/http/httputil/reverseproxy_test.go +++ b/src/net/http/httputil/reverseproxy_test.go @@ -9,6 +9,7 @@ package httputil import ( "bufio" "bytes" + "errors" "io" "io/ioutil" "log" @@ -301,14 +302,14 @@ func TestReverseProxyCancelation(t *testing.T) { reqInFlight := make(chan struct{}) backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - close(reqInFlight) + close(reqInFlight) // cause the client to cancel its request select { case <-time.After(10 * time.Second): // Note: this should only happen in broken implementations, and the // closenotify case should be instantaneous. - t.Log("Failed to close backend connection") - t.Fail() + t.Error("Handler never saw CloseNotify") + return case <-w.(http.CloseNotifier).CloseNotify(): } @@ -341,13 +342,13 @@ func TestReverseProxyCancelation(t *testing.T) { }() res, err := http.DefaultClient.Do(getReq) if res != nil { - t.Fatal("Non-nil response") + t.Error("got response %v; want nil", res.Status) } if err == nil { // This should be an error like: // Get http://127.0.0.1:58079: read tcp 127.0.0.1:58079: // use of closed network connection - t.Fatal("DefaultClient.Do() returned nil error") + t.Error("DefaultClient.Do() returned nil error; want non-nil error") } } @@ -536,3 +537,33 @@ func TestReverseProxy_Post(t *testing.T) { t.Errorf("got body %q; expected %q", g, e) } } + +type RoundTripperFunc func(*http.Request) (*http.Response, error) + +func (fn RoundTripperFunc) RoundTrip(req *http.Request) (*http.Response, error) { + return fn(req) +} + +// Issue 16036: send a Request with a nil Body when possible +func TestReverseProxy_NilBody(t *testing.T) { + backendURL, _ := url.Parse("http://fake.tld/") + proxyHandler := NewSingleHostReverseProxy(backendURL) + proxyHandler.ErrorLog = log.New(ioutil.Discard, "", 0) // quiet for tests + proxyHandler.Transport = RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + if req.Body != nil { + t.Error("Body != nil; want a nil Body") + } + return nil, errors.New("done testing the interesting part; so force a 502 Gateway error") + }) + frontend := httptest.NewServer(proxyHandler) + defer frontend.Close() + + res, err := http.DefaultClient.Get(frontend.URL) + if err != nil { + t.Fatal(err) + } + defer res.Body.Close() + if res.StatusCode != 502 { + t.Errorf("status code = %v; want 502 (Gateway Error)", res.Status) + } +} -- 2.48.1