net/http: don't time out idle server connections after ReadHeaderTimeout

Consistently wait for idle connections to become readable before
starting the ReadHeaderTimeout timer. Previously, connections with no
idle timeout skipped directly to reading headers, so the
ReadHeaderTimeout also included time spent idle.

Fixes #54784

Change-Id: Iff1a876f00311d03dfa0fbef5b577506c62f7c41
GitHub-Last-Rev: 09332743ad6d5a9eb1137adaade2810c583d38ca
GitHub-Pull-Request: golang/go#54785
Reviewed-on: https://go-review.googlesource.com/c/go/+/426895
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Benny Siegert <bsiegert@gmail.com>

diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go
index d28bfba7592585e796aabe8f668118924a9ca7e4..143874d70ad42b7fc91fac0d0903c37f90a08334 100644 (file)
--- a/src/net/http/serve_test.go
+++ b/src/net/http/serve_test.go
@@ -5843,6 +5843,58 @@ func TestServerCancelsReadTimeoutWhenIdle(t *testing.T) {
        })
 }
 
+// Issue 54784: test that the Server's ReadHeaderTimeout only starts once the
+// beginning of a request has been received, rather than including time the
+// connection spent idle.
+func TestServerCancelsReadHeaderTimeoutWhenIdle(t *testing.T) {
+       setParallel(t)
+       defer afterTest(t)
+       runTimeSensitiveTest(t, []time.Duration{
+               10 * time.Millisecond,
+               50 * time.Millisecond,
+               250 * time.Millisecond,
+               time.Second,
+               2 * time.Second,
+       }, func(t *testing.T, timeout time.Duration) error {
+               ts := httptest.NewUnstartedServer(serve(200))
+               ts.Config.ReadHeaderTimeout = timeout
+               ts.Config.IdleTimeout = 0 // disable idle timeout
+               ts.Start()
+               defer ts.Close()
+
+               // rather than using an http.Client, create a single connection, so that
+               // we can ensure this connection is not closed.
+               conn, err := net.Dial("tcp", ts.Listener.Addr().String())
+               if err != nil {
+                       t.Fatalf("dial failed: %v", err)
+               }
+               br := bufio.NewReader(conn)
+               defer conn.Close()
+
+               if _, err := conn.Write([]byte("GET / HTTP/1.1\r\nHost: e.com\r\n\r\n")); err != nil {
+                       t.Fatalf("writing first request failed: %v", err)
+               }
+
+               if _, err := ReadResponse(br, nil); err != nil {
+                       t.Fatalf("first response (before timeout) failed: %v", err)
+               }
+
+               // wait for longer than the server's ReadHeaderTimeout, and then send
+               // another request
+               time.Sleep(timeout + 10*time.Millisecond)
+
+               if _, err := conn.Write([]byte("GET / HTTP/1.1\r\nHost: e.com\r\n\r\n")); err != nil {
+                       t.Fatalf("writing second request failed: %v", err)
+               }
+
+               if _, err := ReadResponse(br, nil); err != nil {
+                       t.Fatalf("second response (after timeout) failed: %v", err)
+               }
+
+               return nil
+       })
+}
+
 // runTimeSensitiveTest runs test with the provided durations until one passes.
 // If they all fail, t.Fatal is called with the last one's duration and error value.
 func runTimeSensitiveTest(t *testing.T, durations []time.Duration, test func(t *testing.T, d time.Duration) error) {

diff --git a/src/net/http/server.go b/src/net/http/server.go
index 9aea1b800248153435d5d52297f7ceff71b5f509..3d427e5ae4bab6e836bbe7c6bf8bc548a367c6de 100644 (file)
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -2002,10 +2002,18 @@ func (c *conn) serve(ctx context.Context) {
 
                if d := c.server.idleTimeout(); d != 0 {
                        c.rwc.SetReadDeadline(time.Now().Add(d))
-                       if _, err := c.bufr.Peek(4); err != nil {
-                               return
-                       }
+               } else {
+                       c.rwc.SetReadDeadline(time.Time{})
                }
+
+               // Wait for the connection to become readable again before trying to
+               // read the next request. This prevents a ReadHeaderTimeout or
+               // ReadTimeout from starting until the first bytes of the next request
+               // have been received.
+               if _, err := c.bufr.Peek(4); err != nil {
+                       return
+               }
+
                c.rwc.SetReadDeadline(time.Time{})
        }
 }