Skip to content

Commit

Permalink
net/http: update bundled http2
Browse files Browse the repository at this point in the history
If a user starts two HTTP requests when no http2 connection is
available, both end up creating new TCP connections, since the
server's protocol (h1 or h2) isn't yet known. Once it turns out that
the server supports h2, one of the connections is useless. Previously
we kept upgrading both TLS connections to h2 (SETTINGS frame exchange,
etc).  Now the unnecessary connections are closed instead, before the
h2 preface/SETTINGS.

Updates x/net/http2 to git rev a8e212f3d for https://golang.org/cl/18675

This CL contains the tests for https://golang.org/cl/18675

Semi-related change noticed while writing the tests: now that we have
TLSNextProto in Go 1.6, which consults the TLS
ConnectionState.NegotiatedProtocol, we have to gurantee that the TLS
handshake has been done before we look at the ConnectionState. So add
that check after the DialTLS hook. (we never documented that users
have to call Handshake, so do it for them, now that it matters)

Updates #13957

Change-Id: I9a70e9d1282fe937ea654d9b1269c984c4e366c0
Reviewed-on: https://go-review.googlesource.com/18676
Reviewed-by: Andrew Gerrand <adg@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
  • Loading branch information
bradfitz committed Jan 15, 2016
1 parent 1556c31 commit 91b910b
Show file tree
Hide file tree
Showing 3 changed files with 179 additions and 14 deletions.
93 changes: 93 additions & 0 deletions src/net/http/clientserver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"io"
"io/ioutil"
"log"
"net"
. "net/http"
"net/http/httptest"
"net/url"
Expand All @@ -22,7 +23,9 @@ import (
"sort"
"strings"
"sync"
"sync/atomic"
"testing"
"time"
)

type clientServerTest struct {
Expand Down Expand Up @@ -861,3 +864,93 @@ func testStarRequest(t *testing.T, method string, h2 bool) {
t.Errorf("RequestURI = %q; want *", req.RequestURI)
}
}

// Issue 13957
func TestTransportDiscardsUnneededConns(t *testing.T) {
defer afterTest(t)
cst := newClientServerTest(t, h2Mode, HandlerFunc(func(w ResponseWriter, r *Request) {
fmt.Fprintf(w, "Hello, %v", r.RemoteAddr)
}))
defer cst.close()

var numOpen, numClose int32 // atomic

tlsConfig := &tls.Config{InsecureSkipVerify: true}
tr := &Transport{
TLSClientConfig: tlsConfig,
DialTLS: func(_, addr string) (net.Conn, error) {
time.Sleep(10 * time.Millisecond)
rc, err := net.Dial("tcp", addr)
if err != nil {
return nil, err
}
atomic.AddInt32(&numOpen, 1)
c := noteCloseConn{rc, func() { atomic.AddInt32(&numClose, 1) }}
return tls.Client(c, tlsConfig), nil
},
}
if err := ExportHttp2ConfigureTransport(tr); err != nil {
t.Fatal(err)
}
defer tr.CloseIdleConnections()

c := &Client{Transport: tr}

const N = 10
gotBody := make(chan string, N)
var wg sync.WaitGroup
for i := 0; i < N; i++ {
wg.Add(1)
go func() {
defer wg.Done()
resp, err := c.Get(cst.ts.URL)
if err != nil {
t.Errorf("Get: %v", err)
return
}
defer resp.Body.Close()
slurp, err := ioutil.ReadAll(resp.Body)
if err != nil {
t.Error(err)
}
gotBody <- string(slurp)
}()
}
wg.Wait()
close(gotBody)

var last string
for got := range gotBody {
if last == "" {
last = got
continue
}
if got != last {
t.Errorf("Response body changed: %q -> %q", last, got)
}
}

var open, close int32
for i := 0; i < 150; i++ {
open, close = atomic.LoadInt32(&numOpen), atomic.LoadInt32(&numClose)
if open < 1 {
t.Fatalf("open = %d; want at least", open)
}
if close == open-1 {
// Success
return
}
time.Sleep(10 * time.Millisecond)
}
t.Errorf("%d connections opened, %d closed; want %d to close", open, close, open-1)
}

type noteCloseConn struct {
net.Conn
closeFunc func()
}

func (x noteCloseConn) Close() error {
x.closeFunc()
return x.Conn.Close()
}
94 changes: 80 additions & 14 deletions src/net/http/h2_bundle.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions src/net/http/transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -713,6 +713,12 @@ func (t *Transport) dialConn(cm connectMethod) (*persistConn, error) {
return nil, errors.New("net/http: Transport.DialTLS returned (nil, nil)")
}
if tc, ok := pconn.conn.(*tls.Conn); ok {
// Handshake here, in case DialTLS didn't. TLSNextProto below
// depends on it for knowing the connection state.
if err := tc.Handshake(); err != nil {
go pconn.conn.Close()
return nil, err
}
cs := tc.ConnectionState()
pconn.tlsState = &cs
}
Expand Down

0 comments on commit 91b910b

Please sign in to comment.