net/http: prevent transport sends two "Connection: close" headers

There are three functions that do Connection header write:
1. transport.go/ persistConn.roundTrip
2. transfer.go/ transferWriter.writeHeader
3. request.go/ Request.write

The root cause is roundTrip didn't lookup into request.Close and
transferWriter
didn't take care of extraHeaders.

Fixes #28886

Change-Id: I1d131019c7cd42eb1bcc972c631b7df7511c1f39
Reviewed-on: https://go-review.googlesource.com/c/150722
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

diff --git a/src/net/http/request.go b/src/net/http/request.go
index 5b7e6564ae5044aa345dc45bdeb982d526a4068f..d994e81d237713f3a9e4ac7a91b465f4fa91f69f 100644 (file)
--- a/src/net/http/request.go
+++ b/src/net/http/request.go
@@ -1330,6 +1330,9 @@ func (r *Request) wantsHttp10KeepAlive() bool {
 }
 
 func (r *Request) wantsClose() bool {
+       if r.Close {
+               return true
+       }
        return hasToken(r.Header.get("Connection"), "close")
 }
 

diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index e1cfc668ea84f19e51c3f87680f41c86e59b1c84..ad0201d554b75763cdaadf19bae7cb0df1d183e2 100644 (file)
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -2135,7 +2135,7 @@ func (pc *persistConn) roundTrip(req *transportRequest) (resp *Response, err err
                continueCh = make(chan struct{}, 1)
        }
 
-       if pc.t.DisableKeepAlives {
+       if pc.t.DisableKeepAlives && !req.wantsClose() {
                req.extraHeaders().Set("Connection", "close")
        }
 

diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go
index 22ca3f9550e40d15f2b4444178830ff8d0a9a067..1021ce5aa20a349d40dbde4b4a56a2de9a8d8e95 100644 (file)
--- a/src/net/http/transport_test.go
+++ b/src/net/http/transport_test.go
@@ -41,6 +41,8 @@ import (
        "sync/atomic"
        "testing"
        "time"
+
+       "golang_org/x/net/http/httpguts"
 )
 
 // TODO: test 5 pipelined requests with responses: 1) OK, 2) OK, Connection: Close
@@ -310,6 +312,58 @@ func TestTransportConnectionCloseOnRequestDisableKeepAlive(t *testing.T) {
        }
 }
 
+// Test that Transport only sends one "Connection: close", regardless of
+// how "close" was indicated.
+func TestTransportRespectRequestWantsClose(t *testing.T) {
+       tests := []struct {
+               disableKeepAlives bool
+               close             bool
+       }{
+               {disableKeepAlives: false, close: false},
+               {disableKeepAlives: false, close: true},
+               {disableKeepAlives: true, close: false},
+               {disableKeepAlives: true, close: true},
+       }
+
+       for _, tc := range tests {
+               t.Run(fmt.Sprintf("DisableKeepAlive=%v,RequestClose=%v", tc.disableKeepAlives, tc.close),
+                       func(t *testing.T) {
+                               defer afterTest(t)
+                               ts := httptest.NewServer(hostPortHandler)
+                               defer ts.Close()
+
+                               c := ts.Client()
+                               c.Transport.(*Transport).DisableKeepAlives = tc.disableKeepAlives
+                               req, err := NewRequest("GET", ts.URL, nil)
+                               if err != nil {
+                                       t.Fatal(err)
+                               }
+                               count := 0
+                               trace := &httptrace.ClientTrace{
+                                       WroteHeaderField: func(key string, field []string) {
+                                               if key != "Connection" {
+                                                       return
+                                               }
+                                               if httpguts.HeaderValuesContainsToken(field, "close") {
+                                                       count += 1
+                                               }
+                                       },
+                               }
+                               req = req.WithContext(httptrace.WithClientTrace(req.Context(), trace))
+                               req.Close = tc.close
+                               res, err := c.Do(req)
+                               if err != nil {
+                                       t.Fatal(err)
+                               }
+                               defer res.Body.Close()
+                               if want := tc.disableKeepAlives || tc.close; count > 1 || (count == 1) != want {
+                                       t.Errorf("expecting want:%v, got 'Connection: close':%d", want, count)
+                               }
+                       })
+       }
+
+}
+
 func TestTransportIdleCacheKeys(t *testing.T) {
        defer afterTest(t)
        ts := httptest.NewServer(hostPortHandler)