From: Eliza Weisman <eliza@buoyant.io>
Date: Wed, 31 Aug 2022 19:16:18 +0000 (+0000)
Subject: net/http: don't time out idle server connections after ReadHeaderTimeout
X-Git-Tag: go1.20rc1~1221
X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=4e7e7ae1406c70d9cc0809ec11105a55a60a0b70;p=gostls13.git

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 d28bfba759..143874d70a 100644
--- 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 9aea1b8002..3d427e5ae4 100644
--- 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{})
 	}
 }