net/http: fix Server blocking after a Handler's Write fails

If a Handle's Write to a ResponseWriter fails (e.g. via a
net.Conn WriteDeadline via WriteTimeout on the Server), the
Server was blocking forever waiting for reads on that
net.Conn, even after a Write failed.

Instead, once we see a Write fail, close the connection,
since it's then dead to us anyway.

Fixes #4741

R=golang-dev, adg
CC=golang-dev
https://golang.org/cl/7301043

diff --git a/src/pkg/net/http/serve_test.go b/src/pkg/net/http/serve_test.go
index 6c97eaf63707b1056ee167362b7f3b38cb84d26e..dc07a8969d8cbde5cbd535ab22565cee589b7b1a 100644 (file)
--- a/src/pkg/net/http/serve_test.go
+++ b/src/pkg/net/http/serve_test.go
@@ -326,6 +326,66 @@ func TestServerTimeouts(t *testing.T) {
        }
 }
 
+// golang.org/issue/4741 -- setting only a write timeout that triggers
+// shouldn't cause a handler to block forever on reads (next HTTP
+// request) that will never happen.
+func TestOnlyWriteTimeout(t *testing.T) {
+       var conn net.Conn
+       var afterTimeoutErrc = make(chan error, 1)
+       ts := httptest.NewUnstartedServer(HandlerFunc(func(w ResponseWriter, req *Request) {
+               buf := make([]byte, 512<<10)
+               _, err := w.Write(buf)
+               if err != nil {
+                       t.Errorf("handler Write error: %v", err)
+                       return
+               }
+               conn.SetWriteDeadline(time.Now().Add(-30 * time.Second))
+               _, err = w.Write(buf)
+               afterTimeoutErrc <- err
+       }))
+       ts.Listener = trackLastConnListener{ts.Listener, &conn}
+       ts.Start()
+       defer ts.Close()
+
+       tr := &Transport{DisableKeepAlives: false}
+       defer tr.CloseIdleConnections()
+       c := &Client{Transport: tr}
+
+       errc := make(chan error)
+       go func() {
+               res, err := c.Get(ts.URL)
+               if err != nil {
+                       errc <- err
+                       return
+               }
+               _, err = io.Copy(ioutil.Discard, res.Body)
+               errc <- err
+       }()
+       select {
+       case err := <-errc:
+               if err == nil {
+                       t.Errorf("expected an error from Get request")
+               }
+       case <-time.After(5 * time.Second):
+               t.Fatal("timeout waiting for Get error")
+       }
+       if err := <-afterTimeoutErrc; err == nil {
+               t.Error("expected write error after timeout")
+       }
+}
+
+// trackLastConnListener tracks the last net.Conn that was accepted.
+type trackLastConnListener struct {
+       net.Listener
+       last *net.Conn // destination
+}
+
+func (l trackLastConnListener) Accept() (c net.Conn, err error) {
+       c, err = l.Listener.Accept()
+       *l.last = c
+       return
+}
+
 // TestIdentityResponse verifies that a handler can unset
 func TestIdentityResponse(t *testing.T) {
        handler := HandlerFunc(func(rw ResponseWriter, req *Request) {

diff --git a/src/pkg/net/http/server.go b/src/pkg/net/http/server.go
index e70d129e7ee4ea3a70efdbf7ab3defb7b942ab23..a965a0e9f92868e4295d76ff7e9ed5163d64dc73 100644 (file)
--- a/src/pkg/net/http/server.go
+++ b/src/pkg/net/http/server.go
@@ -223,6 +223,7 @@ func (cw *chunkWriter) Write(p []byte) (n int, err error) {
        if cw.chunking {
                _, err = fmt.Fprintf(cw.res.conn.buf, "%x\r\n", len(p))
                if err != nil {
+                       cw.res.conn.rwc.Close()
                        return
                }
        }
@@ -230,6 +231,9 @@ func (cw *chunkWriter) Write(p []byte) (n int, err error) {
        if cw.chunking && err == nil {
                _, err = cw.res.conn.buf.Write(crlf)
        }
+       if err != nil {
+               cw.res.conn.rwc.Close()
+       }
        return
 }