net/http: pause briefly after closing Server connection when body remains

From https://github.com/golang/go/issues/11745#issuecomment-123555313
this implements option (b), having the server pause slightly after
sending the final response on a TCP connection when we're about to close
it when we know there's a request body outstanding. This biases the
client (which might not be Go) to prefer our response header over the
request body write error.

Updates #11745

Change-Id: I07cb0b74519d266c8049d9e0eb23a61304eedbf8
Reviewed-on: https://go-review.googlesource.com/12658
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>

diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go
index 345bac5608a1c5ccfb8a8a672527630e009e3249..61bbeb8f5397409aa47dfc6115c5da60f2f4bda0 100644 (file)
--- a/src/net/http/serve_test.go
+++ b/src/net/http/serve_test.go
@@ -3034,6 +3034,30 @@ Host: foo
        }
 }
 
+// If a Handler finishes and there's an unread request body,
+// verify the server try to do implicit read on it before replying.
+func TestHandlerFinishSkipBigContentLengthRead(t *testing.T) {
+       conn := &testConn{closec: make(chan bool)}
+       conn.readBuf.Write([]byte(fmt.Sprintf(
+               "POST / HTTP/1.1\r\n" +
+                       "Host: test\r\n" +
+                       "Content-Length: 9999999999\r\n" +
+                       "\r\n" + strings.Repeat("a", 1<<20))))
+
+       ls := &oneConnListener{conn}
+       var inHandlerLen int
+       go Serve(ls, HandlerFunc(func(rw ResponseWriter, req *Request) {
+               inHandlerLen = conn.readBuf.Len()
+               rw.WriteHeader(404)
+       }))
+       <-conn.closec
+       afterHandlerLen := conn.readBuf.Len()
+
+       if afterHandlerLen != inHandlerLen {
+               t.Errorf("unexpected implicit read. Read buffer went from %d -> %d", inHandlerLen, afterHandlerLen)
+       }
+}
+
 func BenchmarkClientServer(b *testing.B) {
        b.ReportAllocs()
        b.StopTimer()

diff --git a/src/net/http/server.go b/src/net/http/server.go
index aad55d0838501a480c1005529bf39934d1b37dde..8c204fb648773ead7d62d18ba034a387b5b24a11 100644 (file)
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -874,8 +874,14 @@ func (cw *chunkWriter) writeHeader(p []byte) {
        if w.req.ContentLength != 0 && !w.closeAfterReply {
                ecr, isExpecter := w.req.Body.(*expectContinueReader)
                if !isExpecter || ecr.resp.wroteContinue {
-                       n, _ := io.CopyN(ioutil.Discard, w.req.Body, maxPostHandlerReadBytes+1)
-                       if n >= maxPostHandlerReadBytes {
+                       var tooBig bool
+                       if reqBody, ok := w.req.Body.(*body); ok && reqBody.unreadDataSize() >= maxPostHandlerReadBytes {
+                               tooBig = true
+                       } else {
+                               n, _ := io.CopyN(ioutil.Discard, w.req.Body, maxPostHandlerReadBytes+1)
+                               tooBig = n >= maxPostHandlerReadBytes
+                       }
+                       if tooBig {
                                w.requestTooLarge()
                                delHeader("Connection")
                                setHeader.connection = "close"
@@ -1144,13 +1150,18 @@ func (w *response) shouldReuseConnection() bool {
                return false
        }
 
-       if body, ok := w.req.Body.(*body); ok && body.didEarlyClose() {
+       if w.closedRequestBodyEarly() {
                return false
        }
 
        return true
 }
 
+func (w *response) closedRequestBodyEarly() bool {
+       body, ok := w.req.Body.(*body)
+       return ok && body.didEarlyClose()
+}
+
 func (w *response) Flush() {
        if !w.wroteHeader {
                w.WriteHeader(StatusOK)
@@ -1318,7 +1329,7 @@ func (c *conn) serve() {
                }
                w.finishRequest()
                if !w.shouldReuseConnection() {
-                       if w.requestBodyLimitHit {
+                       if w.requestBodyLimitHit || w.closedRequestBodyEarly() {
                                c.closeWriteAndWait()
                        }
                        break

diff --git a/src/net/http/transfer.go b/src/net/http/transfer.go
index fbbbf2417ace3a88245601ca61034f7f956d1671..d1762ebbd24cf766f5f94d3451834ffb137b22c8 100644 (file)
--- a/src/net/http/transfer.go
+++ b/src/net/http/transfer.go
@@ -737,6 +737,17 @@ func mergeSetHeader(dst *Header, src Header) {
        }
 }
 
+// unreadDataSize returns the number of bytes of unread input.
+// It returns -1 if unknown.
+func (b *body) unreadDataSize() int64 {
+       b.mu.Lock()
+       defer b.mu.Unlock()
+       if lr, ok := b.src.(*io.LimitedReader); ok {
+               return lr.N
+       }
+       return -1
+}
+
 func (b *body) Close() error {
        b.mu.Lock()
        defer b.mu.Unlock()

diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index e7ee5c2825974b7da02791f0ed247f5008b826db..b0773f163926cad658301ec1c6caff7341887af5 100644 (file)
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -1164,7 +1164,7 @@ WaitResponse:
        for {
                select {
                case err := <-writeErrCh:
-                       if isSyscallWriteError(err) {
+                       if isNetWriteError(err) {
                                // Issue 11745. If we failed to write the request
                                // body, it's possible the server just heard enough
                                // and already wrote to us. Prioritize the server's
@@ -1383,14 +1383,12 @@ type fakeLocker struct{}
 func (fakeLocker) Lock()   {}
 func (fakeLocker) Unlock() {}
 
-func isSyscallWriteError(err error) bool {
+func isNetWriteError(err error) bool {
        switch e := err.(type) {
        case *url.Error:
-               return isSyscallWriteError(e.Err)
+               return isNetWriteError(e.Err)
        case *net.OpError:
-               return e.Op == "write" && isSyscallWriteError(e.Err)
-       case *os.SyscallError:
-               return e.Syscall == "write"
+               return e.Op == "write"
        default:
                return false
        }