]> Cypherpunks repositories - gostls13.git/commitdiff
net/http/httputil: make ReverseProxy send nil Body requests when possible
authorBrad Fitzpatrick <bradfitz@golang.org>
Fri, 2 Sep 2016 05:00:05 +0000 (05:00 +0000)
committerBrad Fitzpatrick <bradfitz@golang.org>
Fri, 2 Sep 2016 23:57:58 +0000 (23:57 +0000)
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 <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
src/go/build/deps_test.go
src/net/http/httputil/reverseproxy.go
src/net/http/httputil/reverseproxy_test.go

index caacb14f7e1edc54b3a773b1201a53eb425c4914..cb8f95fb1deed5a3f64f6dc8df4f29e896b364ee 100644 (file)
@@ -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"},
index 79831b3a9766e1e25faf3afe50ef1d18da8ef68a..47cd0ae97d7877cac5d08f206a232a1111981966 100644 (file)
@@ -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.
index bfa13d9b6dc0a1ed584cbd674addc2a8f4da617e..8e181dbb04240690aa78a58f83a98c212e75316e 100644 (file)
@@ -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)
+       }
+}