net/http: don't hang if RemoteAddr() blocks

The PROXY protocol is supported by several proxy servers such as haproxy
and Amazon ELB.  This protocol allows services running behind a proxy to
learn the remote address of the actual client connecting to the proxy,
by including a single textual line at the beginning of the TCP
connection.
http://www.haproxy.org/download/1.5/doc/proxy-protocol.txt

There are several Go libraries for this protocol (such as
https://github.com/armon/go-proxyproto), which operate by wrapping a
net.Conn with an implementation whose RemoteAddr method reads the
protocol line before returning. This means that RemoteAddr is a blocking
call.

Before this change, http.Serve called RemoteAddr from the main Accepting
goroutine, not from the per-connection goroutine. This meant that it
would not Accept another connection until RemoteAddr returned, which is
not appropriate if RemoteAddr needs to do a blocking read from the
socket first.

Fixes #12943.

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

2 files changed, 102 insertions, 1 deletions

diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go
index 784074200c..11a0a9e120 100644
--- a/src/net/http/serve_test.go
+++ b/src/net/http/serve_test.go
@@ -755,6 +755,107 @@ func TestSetsRemoteAddr(t *testing.T) {
 	}
 }
 
+type blockingRemoteAddrListener struct {
+	net.Listener
+	conns chan<- net.Conn
+}
+
+func (l *blockingRemoteAddrListener) Accept() (net.Conn, error) {
+	c, err := l.Listener.Accept()
+	if err != nil {
+		return nil, err
+	}
+	brac := &blockingRemoteAddrConn{
+		Conn:  c,
+		addrs: make(chan net.Addr, 1),
+	}
+	l.conns <- brac
+	return brac, nil
+}
+
+type blockingRemoteAddrConn struct {
+	net.Conn
+	addrs chan net.Addr
+}
+
+func (c *blockingRemoteAddrConn) RemoteAddr() net.Addr {
+	return <-c.addrs
+}
+
+// Issue 12943
+func TestServerAllowsBlockingRemoteAddr(t *testing.T) {
+	defer afterTest(t)
+	ts := httptest.NewUnstartedServer(HandlerFunc(func(w ResponseWriter, r *Request) {
+		fmt.Fprintf(w, "RA:%s", r.RemoteAddr)
+	}))
+	conns := make(chan net.Conn)
+	ts.Listener = &blockingRemoteAddrListener{
+		Listener: ts.Listener,
+		conns:    conns,
+	}
+	ts.Start()
+	defer ts.Close()
+
+	tr := &Transport{DisableKeepAlives: true}
+	defer tr.CloseIdleConnections()
+	c := &Client{Transport: tr, Timeout: time.Second}
+
+	fetch := func(response chan string) {
+		resp, err := c.Get(ts.URL)
+		if err != nil {
+			t.Error(err)
+			response <- ""
+			return
+		}
+		defer resp.Body.Close()
+		body, err := ioutil.ReadAll(resp.Body)
+		if err != nil {
+			t.Error(err)
+			response <- ""
+			return
+		}
+		response <- string(body)
+	}
+
+	// Start a request. The server will block on getting conn.RemoteAddr.
+	response1c := make(chan string, 1)
+	go fetch(response1c)
+
+	// Wait for the server to accept it; grab the connection.
+	conn1 := <-conns
+
+	// Start another request and grab its connection
+	response2c := make(chan string, 1)
+	go fetch(response2c)
+	var conn2 net.Conn
+
+	select {
+	case conn2 = <-conns:
+	case <-time.After(time.Second):
+		t.Fatal("Second Accept didn't happen")
+	}
+
+	// Send a response on connection 2.
+	conn2.(*blockingRemoteAddrConn).addrs <- &net.TCPAddr{
+		IP: net.ParseIP("12.12.12.12"), Port: 12}
+
+	// ... and see it
+	response2 := <-response2c
+	if g, e := response2, "RA:12.12.12.12:12"; g != e {
+		t.Fatalf("response 2 addr = %q; want %q", g, e)
+	}
+
+	// Finish the first response.
+	conn1.(*blockingRemoteAddrConn).addrs <- &net.TCPAddr{
+		IP: net.ParseIP("21.21.21.21"), Port: 21}
+
+	// ... and see it
+	response1 := <-response1c
+	if g, e := response1, "RA:21.21.21.21:21"; g != e {
+		t.Fatalf("response 1 addr = %q; want %q", g, e)
+	}
+}
+
 func TestChunkedResponseHeaders(t *testing.T) {
 	defer afterTest(t)
 	log.SetOutput(ioutil.Discard) // is noisy otherwise
diff --git a/src/net/http/server.go b/src/net/http/server.go
index 0bdc9b685c..ae62e076dd 100644
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -470,7 +470,6 @@ const debugServerConnections = false
 // Create new connection from rwc.
 func (srv *Server) newConn(rwc net.Conn) (c *conn, err error) {
 	c = new(conn)
-	c.remoteAddr = rwc.RemoteAddr().String()
 	c.server = srv
 	c.rwc = rwc
 	c.w = rwc
@@ -1290,6 +1289,7 @@ func (c *conn) setState(nc net.Conn, state ConnState) {
 
 // Serve a new connection.
 func (c *conn) serve() {
+	c.remoteAddr = c.rwc.RemoteAddr().String()
 	origConn := c.rwc // copy it before it's set nil on Close or Hijack
 	defer func() {
 		if err := recover(); err != nil {