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 <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>

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"},

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.

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)
+	}
+}