http: export Transport, add keep-alive support

This patch adds a connection cache and keep-alive
support to Transport, which is used by the
HTTP client.

It's also structured such that it's easy to add
HTTP pipelining in the future.

R=rsc, petar-m, bradfitzwork, r
CC=golang-dev
https://golang.org/cl/4272045

src/pkg/http/client.go

@@ -57,40 +57,6 @@ type readClose struct {
 	io.Closer
 }
 
-// matchNoProxy returns true if requests to addr should not use a proxy,
-// according to the NO_PROXY or no_proxy environment variable.
-func matchNoProxy(addr string) bool {
-	if len(addr) == 0 {
-		return false
-	}
-	no_proxy := os.Getenv("NO_PROXY")
-	if len(no_proxy) == 0 {
-		no_proxy = os.Getenv("no_proxy")
-	}
-	if no_proxy == "*" {
-		return true
-	}
-
-	addr = strings.ToLower(strings.TrimSpace(addr))
-	if hasPort(addr) {
-		addr = addr[:strings.LastIndex(addr, ":")]
-	}
-
-	for _, p := range strings.Split(no_proxy, ",", -1) {
-		p = strings.ToLower(strings.TrimSpace(p))
-		if len(p) == 0 {
-			continue
-		}
-		if hasPort(p) {
-			p = p[:strings.LastIndex(p, ":")]
-		}
-		if addr == p || (p[0] == '.' && (strings.HasSuffix(addr, p) || addr == p[1:])) {
-			return true
-		}
-	}
-	return false
-}
-
 // Do sends an HTTP request and returns an HTTP response, following
 // policy (e.g. redirects, cookies, auth) as configured on the client.
 //

src/pkg/http/export_test.go

+// Copyright 2011 The Go Authors.  All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Bridge package to expose http internals to tests in the http_test
+// package.
+
+package http
+
+func (t *Transport) IdleConnKeysForTesting() (keys []string) {
+	keys = make([]string, 0)
+	t.lk.Lock()
+	defer t.lk.Unlock()
+	if t.idleConn == nil {
+		return
+	}
+	for key, _ := range t.idleConn {
+		keys = append(keys, key)
+	}
+	return
+}
+
+func (t *Transport) IdleConnCountForTesting(cacheKey string) int {
+	t.lk.Lock()
+	defer t.lk.Unlock()
+	if t.idleConn == nil {
+		return 0
+	}
+	conns, ok := t.idleConn[cacheKey]
+	if !ok {
+		return 0
+	}
+	return len(conns)
+}

src/pkg/http/httptest/server.go

@@ -9,6 +9,7 @@ package httptest
 import (
 	"fmt"
 	"http"
+	"os"
 	"net"
 )
 
@@ -19,6 +20,21 @@ type Server struct {
 	Listener net.Listener
 }
 
+// historyListener keeps track of all connections that it's ever
+// accepted.
+type historyListener struct {
+	net.Listener
+	history []net.Conn
+}
+
+func (hs *historyListener) Accept() (c net.Conn, err os.Error) {
+	c, err = hs.Listener.Accept()
+	if err == nil {
+		hs.history = append(hs.history, c)
+	}
+	return
+}
+
 // NewServer starts and returns a new Server.
 // The caller should call Close when finished, to shut it down.
 func NewServer(handler http.Handler) *Server {
@@ -29,10 +45,10 @@ func NewServer(handler http.Handler) *Server {
 			panic(fmt.Sprintf("httptest: failed to listen on a port: %v", err))
 		}
 	}
-	ts.Listener = l
+	ts.Listener = &historyListener{l, make([]net.Conn, 0)}
 	ts.URL = "http://" + l.Addr().String()
 	server := &http.Server{Handler: handler}
-	go server.Serve(l)
+	go server.Serve(ts.Listener)
 	return ts
 }
 
@@ -40,3 +56,15 @@ func NewServer(handler http.Handler) *Server {
 func (s *Server) Close() {
 	s.Listener.Close()
 }
+
+// CloseClientConnections closes any currently open HTTP connections
+// to the test Server.
+func (s *Server) CloseClientConnections() {
+	hl, ok := s.Listener.(*historyListener)
+	if !ok {
+		return
+	}
+	for _, conn := range hl.history {
+		conn.Close()
+	}
+}

src/pkg/http/persist.go

@@ -213,6 +213,7 @@ type ClientConn struct {
 
 	pipe     textproto.Pipeline
 	writeReq func(*Request, io.Writer) os.Error
+	readRes  func(buf *bufio.Reader, method string) (*Response, os.Error)
 }
 
 // NewClientConn returns a new ClientConn reading and writing c.  If r is not
@@ -226,6 +227,7 @@ func NewClientConn(c net.Conn, r *bufio.Reader) *ClientConn {
 		r:        r,
 		pipereq:  make(map[*Request]uint),
 		writeReq: (*Request).Write,
+		readRes:  ReadResponse,
 	}
 }
 
@@ -363,7 +365,7 @@ func (cc *ClientConn) Read(req *Request) (resp *Response, err os.Error) {
 		}
 	}
 
-	resp, err = ReadResponse(r, req.Method)
+	resp, err = cc.readRes(r, req.Method)
 	cc.lk.Lock()
 	defer cc.lk.Unlock()
 	if err != nil {

src/pkg/http/proxy_test.go

@@ -12,31 +12,33 @@ import (
 // TODO(mattn):
 //	test ProxyAuth
 
-var MatchNoProxyTests = []struct {
+var UseProxyTests = []struct {
 	host  string
 	match bool
 }{
-	{"localhost", true},        // match completely
-	{"barbaz.net", true},       // match as .barbaz.net
-	{"foobar.com:443", true},   // have a port but match 
-	{"foofoobar.com", false},   // not match as a part of foobar.com
-	{"baz.com", false},         // not match as a part of barbaz.com
-	{"localhost.net", false},   // not match as suffix of address
-	{"local.localhost", false}, // not match as prefix as address
-	{"barbarbaz.net", false},   // not match because NO_PROXY have a '.'
-	{"www.foobar.com", false},  // not match because NO_PROXY is not .foobar.com
+	{"localhost", false},      // match completely
+	{"barbaz.net", false},     // match as .barbaz.net
+	{"foobar.com:443", false}, // have a port but match 
+	{"foofoobar.com", true},   // not match as a part of foobar.com
+	{"baz.com", true},         // not match as a part of barbaz.com
+	{"localhost.net", true},   // not match as suffix of address
+	{"local.localhost", true}, // not match as prefix as address
+	{"barbarbaz.net", true},   // not match because NO_PROXY have a '.'
+	{"www.foobar.com", true},  // not match because NO_PROXY is not .foobar.com
 }
 
-func TestMatchNoProxy(t *testing.T) {
+func TestUseProxy(t *testing.T) {
 	oldenv := os.Getenv("NO_PROXY")
 	no_proxy := "foobar.com, .barbaz.net   , localhost"
 	os.Setenv("NO_PROXY", no_proxy)
 	defer os.Setenv("NO_PROXY", oldenv)
 
-	for _, test := range MatchNoProxyTests {
-		if matchNoProxy(test.host) != test.match {
+	tr := &Transport{}
+
+	for _, test := range UseProxyTests {
+		if tr.useProxy(test.host) != test.match {
 			if test.match {
-				t.Errorf("matchNoProxy(%v) = %v, want %v", test.host, !test.match, test.match)
+				t.Errorf("useProxy(%v) = %v, want %v", test.host, !test.match, test.match)
 			} else {
 				t.Errorf("not expected: '%s' shouldn't match as '%s'", test.host, no_proxy)
 			}

src/pkg/http/serve_test.go

@@ -250,7 +250,9 @@ func TestServerTimeouts(t *testing.T) {
 	url := fmt.Sprintf("http://localhost:%d/", addr.Port)
 
 	// Hit the HTTP server successfully.
-	r, _, err := Get(url)
+	tr := &Transport{DisableKeepAlives: true} // they interfere with this test
+	c := &Client{Transport: tr}
+	r, _, err := c.Get(url)
 	if err != nil {
 		t.Fatalf("http Get #1: %v", err)
 	}
@@ -335,6 +337,7 @@ func TestIdentityResponse(t *testing.T) {
 			t.Errorf("for %s expected len(res.TransferEncoding) of %d; got %d (%v)",
 				url, expected, tl, res.TransferEncoding)
 		}
+		res.Body.Close()
 	}
 
 	// Verify that ErrContentLength is returned
@@ -343,7 +346,6 @@ func TestIdentityResponse(t *testing.T) {
 	if err != nil {
 		t.Fatalf("error with Get of %s: %v", url, err)
 	}
-
 	// Verify that the connection is closed when the declared Content-Length
 	// is larger than what the handler wrote.
 	conn, err := net.Dial("tcp", "", ts.Listener.Addr().String())

src/pkg/http/transport_test.go

@@ -11,9 +11,204 @@ import (
 	. "http"
 	"http/httptest"
 	"io/ioutil"
+	"os"
 	"testing"
+	"time"
 )
 
+// TODO: test 5 pipelined requests with responses: 1) OK, 2) OK, Connection: Close
+//       and then verify that the final 2 responses get errors back.
+
+// hostPortHandler writes back the client's "host:port".
+var hostPortHandler = HandlerFunc(func(w ResponseWriter, r *Request) {
+	if r.FormValue("close") == "true" {
+		w.Header().Set("Connection", "close")
+	}
+	fmt.Fprintf(w, "%s", r.RemoteAddr)
+})
+
+// Two subsequent requests and verify their response is the same.
+// The response from the server is our own IP:port
+func TestTransportKeepAlives(t *testing.T) {
+	ts := httptest.NewServer(hostPortHandler)
+	defer ts.Close()
+
+	for _, disableKeepAlive := range []bool{false, true} {
+		tr := &Transport{DisableKeepAlives: disableKeepAlive}
+		c := &Client{Transport: tr}
+
+		fetch := func(n int) string {
+			res, _, err := c.Get(ts.URL)
+			if err != nil {
+				t.Fatalf("error in disableKeepAlive=%v, req #%d, GET: %v", disableKeepAlive, n, err)
+			}
+			body, err := ioutil.ReadAll(res.Body)
+			if err != nil {
+				t.Fatalf("error in disableKeepAlive=%v, req #%d, ReadAll: %v", disableKeepAlive, n, err)
+			}
+			return string(body)
+		}
+
+		body1 := fetch(1)
+		body2 := fetch(2)
+
+		bodiesDiffer := body1 != body2
+		if bodiesDiffer != disableKeepAlive {
+			t.Errorf("error in disableKeepAlive=%v. unexpected bodiesDiffer=%v; body1=%q; body2=%q",
+				disableKeepAlive, bodiesDiffer, body1, body2)
+		}
+	}
+}
+
+func TestTransportConnectionCloseOnResponse(t *testing.T) {
+	ts := httptest.NewServer(hostPortHandler)
+	defer ts.Close()
+
+	for _, connectionClose := range []bool{false, true} {
+		tr := &Transport{}
+		c := &Client{Transport: tr}
+
+		fetch := func(n int) string {
+			req := new(Request)
+			var err os.Error
+			req.URL, err = ParseURL(ts.URL + fmt.Sprintf("?close=%v", connectionClose))
+			if err != nil {
+				t.Fatalf("URL parse error: %v", err)
+			}
+			req.Method = "GET"
+			req.Proto = "HTTP/1.1"
+			req.ProtoMajor = 1
+			req.ProtoMinor = 1
+
+			res, err := c.Do(req)
+			if err != nil {
+				t.Fatalf("error in connectionClose=%v, req #%d, Do: %v", connectionClose, n, err)
+			}
+			body, err := ioutil.ReadAll(res.Body)
+			if err != nil {
+				t.Fatalf("error in connectionClose=%v, req #%d, ReadAll: %v", connectionClose, n, err)
+			}
+			return string(body)
+		}
+
+		body1 := fetch(1)
+		body2 := fetch(2)
+		bodiesDiffer := body1 != body2
+		if bodiesDiffer != connectionClose {
+			t.Errorf("error in connectionClose=%v. unexpected bodiesDiffer=%v; body1=%q; body2=%q",
+				connectionClose, bodiesDiffer, body1, body2)
+		}
+	}
+}
+
+func TestTransportConnectionCloseOnRequest(t *testing.T) {
+	ts := httptest.NewServer(hostPortHandler)
+	defer ts.Close()
+
+	for _, connectionClose := range []bool{false, true} {
+		tr := &Transport{}
+		c := &Client{Transport: tr}
+
+		fetch := func(n int) string {
+			req := new(Request)
+			var err os.Error
+			req.URL, err = ParseURL(ts.URL)
+			if err != nil {
+				t.Fatalf("URL parse error: %v", err)
+			}
+			req.Method = "GET"
+			req.Proto = "HTTP/1.1"
+			req.ProtoMajor = 1
+			req.ProtoMinor = 1
+			req.Close = connectionClose
+
+			res, err := c.Do(req)
+			if err != nil {
+				t.Fatalf("error in connectionClose=%v, req #%d, Do: %v", connectionClose, n, err)
+			}
+			body, err := ioutil.ReadAll(res.Body)
+			if err != nil {
+				t.Fatalf("error in connectionClose=%v, req #%d, ReadAll: %v", connectionClose, n, err)
+			}
+			return string(body)
+		}
+
+		body1 := fetch(1)
+		body2 := fetch(2)
+		bodiesDiffer := body1 != body2
+		if bodiesDiffer != connectionClose {
+			t.Errorf("error in connectionClose=%v. unexpected bodiesDiffer=%v; body1=%q; body2=%q",
+				connectionClose, bodiesDiffer, body1, body2)
+		}
+	}
+}
+
+func TestTransportIdleCacheKeys(t *testing.T) {
+	ts := httptest.NewServer(hostPortHandler)
+	defer ts.Close()
+
+	tr := &Transport{DisableKeepAlives: false}
+	c := &Client{Transport: tr}
+
+	if e, g := 0, len(tr.IdleConnKeysForTesting()); e != g {
+		t.Errorf("After CloseIdleConnections expected %d idle conn cache keys; got %d", e, g)
+	}
+
+	if _, _, err := c.Get(ts.URL); err != nil {
+		t.Error(err)
+	}
+
+	keys := tr.IdleConnKeysForTesting()
+	if e, g := 1, len(keys); e != g {
+		t.Fatalf("After Get expected %d idle conn cache keys; got %d", e, g)
+	}
+
+	if e := "|http|" + ts.Listener.Addr().String(); keys[0] != e {
+		t.Logf("Expected idle cache key %q; got %q", e, keys[0])
+	}
+
+	tr.CloseIdleConnections()
+	if e, g := 0, len(tr.IdleConnKeysForTesting()); e != g {
+		t.Errorf("After CloseIdleConnections expected %d idle conn cache keys; got %d", e, g)
+	}
+}
+
+func TestTransportServerClosingUnexpectedly(t *testing.T) {
+	ts := httptest.NewServer(hostPortHandler)
+	defer ts.Close()
+
+	tr := &Transport{}
+	c := &Client{Transport: tr}
+
+	fetch := func(n int) string {
+		res, _, err := c.Get(ts.URL)
+		if err != nil {
+			t.Fatalf("error in req #%d, GET: %v", n, err)
+		}
+		body, err := ioutil.ReadAll(res.Body)
+		if err != nil {
+			t.Fatalf("error in req #%d, ReadAll: %v", n, err)
+		}
+		res.Body.Close()
+		return string(body)
+	}
+
+	body1 := fetch(1)
+	body2 := fetch(2)
+
+	ts.CloseClientConnections() // surprise!
+	time.Sleep(25e6)            // idle for a bit (test is inherently racey, but expectedly)
+
+	body3 := fetch(3)
+
+	if body1 != body2 {
+		t.Errorf("expected body1 and body2 to be equal")
+	}
+	if body2 == body3 {
+		t.Errorf("expected body2 and body3 to be different")
+	}
+}
+
 func TestTransportNilURL(t *testing.T) {
 	ts := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) {
 		fmt.Fprintf(w, "Hi")
@@ -28,9 +223,8 @@ func TestTransportNilURL(t *testing.T) {
 	req.ProtoMajor = 1
 	req.ProtoMinor = 1
 
-	// TODO(bradfitz): test &transport{} and not DefaultTransport
-	// once Transport is exported.
-	res, err := DefaultTransport.RoundTrip(req)
+	tr := &Transport{}
+	res, err := tr.RoundTrip(req)
 	if err != nil {
 		t.Fatalf("unexpected RoundTrip error: %v", err)
 	}