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>

diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go
index 784074200c16578dabb3bb0844f7a4d43cb73cf6..11a0a9e1209cc22ed113ffade8ead6c85a3893a4 100644 (file)
--- 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 0bdc9b685c7db51f3b1dcd522e04646d9cac0d23..ae62e076ddffb9644064795edf6c2966045cd00e 100644 (file)
--- 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 {