From 3c4f12a7d6bc6839b3dd2f4b04aeca962745afb3 Mon Sep 17 00:00:00 2001
From: "Bryan C. Mills" <bcmills@google.com>
Date: Tue, 19 Sep 2023 10:37:28 -0400
Subject: [PATCH] net/http: buffer the testConn close channel in
 TestHandlerFinishSkipBigContentLengthRead

Previously the test used an unbuffered channel, but testConn.Close
sends to it with a select-with-default, so the send would be dropped
if the test goroutine happened not to have parked on the receive yet.

To make this kind of bug less likely in future tests, use a
newTestConn helper function instead of constructing testConn channel
literals in each test individually.

Fixes #62622.

Change-Id: I016cd0a89cf8a2a748ed57a4cdbd01a178f04dab
Reviewed-on: https://go-review.googlesource.com/c/go/+/529475
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
---
 src/net/http/serve_test.go | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go
index cadadf48bcd..8fa40e61ff1 100644
--- a/src/net/http/serve_test.go
+++ b/src/net/http/serve_test.go
@@ -107,10 +107,14 @@ type testConn struct {
 	readMu   sync.Mutex // for TestHandlerBodyClose
 	readBuf  bytes.Buffer
 	writeBuf bytes.Buffer
-	closec   chan bool // if non-nil, send value to it on close
+	closec   chan bool // 1-buffered; receives true when Close is called
 	noopConn
 }
 
+func newTestConn() *testConn {
+	return &testConn{closec: make(chan bool, 1)}
+}
+
 func (c *testConn) Read(b []byte) (int, error) {
 	c.readMu.Lock()
 	defer c.readMu.Unlock()
@@ -4589,10 +4593,10 @@ 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.
+// verify the server implicitly tries to do a read on it before replying.
 func TestHandlerFinishSkipBigContentLengthRead(t *testing.T) {
 	setParallel(t)
-	conn := &testConn{closec: make(chan bool)}
+	conn := newTestConn()
 	conn.readBuf.Write([]byte(fmt.Sprintf(
 		"POST / HTTP/1.1\r\n" +
 			"Host: test\r\n" +
@@ -4682,7 +4686,7 @@ func TestServerValidatesHostHeader(t *testing.T) {
 		{"GET / HTTP/3.0", "", 505},
 	}
 	for _, tt := range tests {
-		conn := &testConn{closec: make(chan bool, 1)}
+		conn := newTestConn()
 		methodTarget := "GET / "
 		if !strings.HasPrefix(tt.proto, "HTTP/") {
 			methodTarget = ""
@@ -4780,7 +4784,7 @@ func TestServerValidatesHeaders(t *testing.T) {
 		{"foo: foo\xfffoo\r\n", 200}, // non-ASCII high octets in value are fine
 	}
 	for _, tt := range tests {
-		conn := &testConn{closec: make(chan bool, 1)}
+		conn := newTestConn()
 		io.WriteString(&conn.readBuf, "GET / HTTP/1.1\r\nHost: foo\r\n"+tt.header+"\r\n")
 
 		ln := &oneConnListener{conn}
@@ -5166,11 +5170,7 @@ Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.3
 `)
 	res := []byte("Hello world!\n")
 
-	conn := &testConn{
-		// testConn.Close will not push into the channel
-		// if it's full.
-		closec: make(chan bool, 1),
-	}
+	conn := newTestConn()
 	handler := HandlerFunc(func(rw ResponseWriter, r *Request) {
 		rw.Header().Set("Content-Type", "text/html; charset=utf-8")
 		rw.Write(res)
@@ -5991,7 +5991,7 @@ func TestServerValidatesMethod(t *testing.T) {
 		{"GE(T", 400},
 	}
 	for _, tt := range tests {
-		conn := &testConn{closec: make(chan bool, 1)}
+		conn := newTestConn()
 		io.WriteString(&conn.readBuf, tt.method+" / HTTP/1.1\r\nHost: foo.example\r\n\r\n")
 
 		ln := &oneConnListener{conn}