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>

diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go
index cadadf48bcd0aceb529913a78111a0f8bb9418f5..8fa40e61ff1e11e0b90e008caaf5f7cbc34c8f01 100644 (file)
--- 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}