net/http/fcgi: eliminate goroutine leaks in tests

Also fix a (minor) double-Close error in Serve that was exposed by the
test fix.

Serve accepts a net.Listener, which produces net.Conn instances.
The documentation for net.Conn requires its methods to be safe for
concurrent use, so most implementations likely allow Close to be
called multiple times as a side effect of making it safe to call
concurrently with other methods. However, the net.Conn interface is a
superset of the io.Closer interface, io.Closer explicitly leaves the
behavior of multiple Close calls undefined, and net.Conn does not
explicitly document a stricter requirement.

Perhaps more importantly, the test for the fcgi package calls
unexported functions that accept an io.ReadWriteCloser (not a
net.Conn), and at least one of the test-helper ReadWriteCloser
implementations expects Close to be called only once.

The goroutine leaks were exposed by a racy arbitrary timeout reported
in #61271. Fixing the goroutine leak exposed the double-Close error:
one of the leaked goroutines was blocked on reading from an unclosed
pipe. Closing the pipe (to unblock the goroutine) triggered the second
Close call.

Fixes #61271.

Change-Id: I5cfac8870e4bb4f13adeee48910d165dbd4b76fe
Reviewed-on: https://go-review.googlesource.com/c/go/+/508815
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

src/net/http/fcgi/fcgi.go

@@ -99,8 +99,10 @@ func (h *header) init(recType recType, reqId uint16, contentLength int) {
 
 // conn sends records over rwc
 type conn struct {
-	mutex sync.Mutex
-	rwc   io.ReadWriteCloser
+	mutex    sync.Mutex
+	rwc      io.ReadWriteCloser
+	closeErr error
+	closed   bool
 
 	// to avoid allocations
 	buf bytes.Buffer
@@ -111,10 +113,15 @@ func newConn(rwc io.ReadWriteCloser) *conn {
 	return &conn{rwc: rwc}
 }
 
+// Close closes the conn if it is not already closed.
 func (c *conn) Close() error {
 	c.mutex.Lock()
 	defer c.mutex.Unlock()
-	return c.rwc.Close()
+	if !c.closed {
+		c.closeErr = c.rwc.Close()
+		c.closed = true
+	}
+	return c.closeErr
 }
 
 type record struct {

src/net/http/fcgi/fcgi_test.go

@@ -241,7 +241,7 @@ func TestChildServeCleansUp(t *testing.T) {
 		input := make([]byte, len(tt.input))
 		copy(input, tt.input)
 		rc := nopWriteCloser{bytes.NewReader(input)}
-		done := make(chan bool)
+		done := make(chan struct{})
 		c := newChild(rc, http.HandlerFunc(func(
 			w http.ResponseWriter,
 			r *http.Request,
@@ -252,9 +252,9 @@ func TestChildServeCleansUp(t *testing.T) {
 				t.Errorf("Expected %#v, got %#v", tt.err, err)
 			}
 			// not reached if body of request isn't closed
-			done <- true
+			close(done)
 		}))
-		go c.serve()
+		c.serve()
 		// wait for body of request to be closed or all goroutines to block
 		<-done
 	}
@@ -331,7 +331,7 @@ func TestChildServeReadsEnvVars(t *testing.T) {
 		input := make([]byte, len(tt.input))
 		copy(input, tt.input)
 		rc := nopWriteCloser{bytes.NewReader(input)}
-		done := make(chan bool)
+		done := make(chan struct{})
 		c := newChild(rc, http.HandlerFunc(func(
 			w http.ResponseWriter,
 			r *http.Request,
@@ -343,9 +343,9 @@ func TestChildServeReadsEnvVars(t *testing.T) {
 			} else if env[tt.envVar] != tt.expectedVal {
 				t.Errorf("Expected %s, got %s", tt.expectedVal, env[tt.envVar])
 			}
-			done <- true
+			close(done)
 		}))
-		go c.serve()
+		c.serve()
 		<-done
 	}
 }
@@ -381,7 +381,7 @@ func TestResponseWriterSniffsContentType(t *testing.T) {
 			input := make([]byte, len(streamFullRequestStdin))
 			copy(input, streamFullRequestStdin)
 			rc := nopWriteCloser{bytes.NewReader(input)}
-			done := make(chan bool)
+			done := make(chan struct{})
 			var resp *response
 			c := newChild(rc, http.HandlerFunc(func(
 				w http.ResponseWriter,
@@ -389,10 +389,9 @@ func TestResponseWriterSniffsContentType(t *testing.T) {
 			) {
 				io.WriteString(w, tt.body)
 				resp = w.(*response)
-				done <- true
+				close(done)
 			}))
-			defer c.cleanUp()
-			go c.serve()
+			c.serve()
 			<-done
 			if got := resp.Header().Get("Content-Type"); got != tt.wantCT {
 				t.Errorf("got a Content-Type of %q; expected it to start with %q", got, tt.wantCT)
@@ -401,25 +400,27 @@ func TestResponseWriterSniffsContentType(t *testing.T) {
 	}
 }
 
-type signalingNopCloser struct {
-	io.Reader
+type signalingNopWriteCloser struct {
+	io.ReadCloser
 	closed chan bool
 }
 
-func (*signalingNopCloser) Write(buf []byte) (int, error) {
+func (*signalingNopWriteCloser) Write(buf []byte) (int, error) {
 	return len(buf), nil
 }
 
-func (rc *signalingNopCloser) Close() error {
+func (rc *signalingNopWriteCloser) Close() error {
 	close(rc.closed)
-	return nil
+	return rc.ReadCloser.Close()
 }
 
 // Test whether server properly closes connection when processing slow
 // requests
 func TestSlowRequest(t *testing.T) {
 	pr, pw := io.Pipe()
-	go func(w io.Writer) {
+
+	writerDone := make(chan struct{})
+	go func() {
 		for _, buf := range [][]byte{
 			streamBeginTypeStdin,
 			makeRecord(typeStdin, 1, nil),
@@ -427,9 +428,14 @@ func TestSlowRequest(t *testing.T) {
 			pw.Write(buf)
 			time.Sleep(100 * time.Millisecond)
 		}
-	}(pw)
+		close(writerDone)
+	}()
+	defer func() {
+		<-writerDone
+		pw.Close()
+	}()
 
-	rc := &signalingNopCloser{pr, make(chan bool)}
+	rc := &signalingNopWriteCloser{pr, make(chan bool)}
 	handlerDone := make(chan bool)
 
 	c := newChild(rc, http.HandlerFunc(func(
@@ -439,16 +445,9 @@ func TestSlowRequest(t *testing.T) {
 		w.WriteHeader(200)
 		close(handlerDone)
 	}))
-	go c.serve()
-	defer c.cleanUp()
-
-	timeout := time.After(2 * time.Second)
+	c.serve()
 
 	<-handlerDone
-	select {
-	case <-rc.closed:
-		t.Log("FastCGI child closed connection")
-	case <-timeout:
-		t.Error("FastCGI child did not close socket after handling request")
-	}
+	<-rc.closed
+	t.Log("FastCGI child closed connection")
 }