net/http/cgi: in TestCopyError, check for a Handler.ServeHTTP goroutine instead of...

Previously, the test could fail spuriously if the CGI process's PID
happened to be reused in between checks. That sort of reuse is highly
unlikely on platforms that cycle through the PID space sequentially
(such as Linux), but plausible on platforms that use randomized PIDs
(such as OpenBSD).

Also unskip the test on Windows, since it no longer relies on being
able to send signal 0 to an arbitrary PID.

Also change the expected failure mode of the test to a timeout instead
of a call to t.Fatalf, so that on failure we get a useful goroutine
dump for debugging instead of a non-actionable failure message.

Fixes #57369 (maybe).

Change-Id: Ib7e3fff556450b48cb5e6ea120fdf4d53547479b
Reviewed-on: https://go-review.googlesource.com/c/go/+/554075
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/cgi/host_test.go b/src/net/http/cgi/host_test.go
index 78e05d592a40deac1d48b91bb82bd130b4931087..f29395fe8493f02a85308db46ab847d253539a5c 100644 (file)
--- a/src/net/http/cgi/host_test.go
+++ b/src/net/http/cgi/host_test.go
@@ -17,8 +17,8 @@ import (
        "os"
        "path/filepath"
        "reflect"
+       "regexp"
        "runtime"
-       "strconv"
        "strings"
        "testing"
        "time"
@@ -363,11 +363,12 @@ func TestInternalRedirect(t *testing.T) {
 
 // TestCopyError tests that we kill the process if there's an error copying
 // its output. (for example, from the client having gone away)
+//
+// If we fail to do so, the test will time out (and dump its goroutines) with a
+// call to [Handler.ServeHTTP] blocked on a deferred call to [exec.Cmd.Wait].
 func TestCopyError(t *testing.T) {
        testenv.MustHaveExec(t)
-       if runtime.GOOS == "windows" {
-               t.Skipf("skipping test on %q", runtime.GOOS)
-       }
+
        h := &Handler{
                Path: os.Args[0],
                Root: "/test.cgi",
@@ -390,37 +391,42 @@ func TestCopyError(t *testing.T) {
                t.Fatalf("ReadResponse: %v", err)
        }
 
-       pidstr := res.Header.Get("X-CGI-Pid")
-       if pidstr == "" {
-               t.Fatalf("expected an X-CGI-Pid header in response")
-       }
-       pid, err := strconv.Atoi(pidstr)
-       if err != nil {
-               t.Fatalf("invalid X-CGI-Pid value")
-       }
-
        var buf [5000]byte
        n, err := io.ReadFull(res.Body, buf[:])
        if err != nil {
                t.Fatalf("ReadFull: %d bytes, %v", n, err)
        }
 
-       childRunning := func() bool {
-               return isProcessRunning(pid)
-       }
-
-       if !childRunning() {
-               t.Fatalf("pre-conn.Close, expected child to be running")
+       if !handlerRunning() {
+               t.Fatalf("pre-conn.Close, expected handler to still be running")
        }
        conn.Close()
+       closed := time.Now()
 
-       tries := 0
-       for tries < 25 && childRunning() {
-               time.Sleep(50 * time.Millisecond * time.Duration(tries))
-               tries++
+       nextSleep := 1 * time.Millisecond
+       for {
+               time.Sleep(nextSleep)
+               nextSleep *= 2
+               if !handlerRunning() {
+                       break
+               }
+               t.Logf("handler still running %v after conn.Close", time.Since(closed))
        }
-       if childRunning() {
-               t.Fatalf("post-conn.Close, expected child to be gone")
+}
+
+// handlerRunning reports whether any goroutine is currently running
+// [Handler.ServeHTTP].
+func handlerRunning() bool {
+       r := regexp.MustCompile(`net/http/cgi\.\(\*Handler\)\.ServeHTTP`)
+       buf := make([]byte, 64<<10)
+       for {
+               n := runtime.Stack(buf, true)
+               if n < len(buf) {
+                       return r.Match(buf[:n])
+               }
+               // Buffer wasn't large enough for a full goroutine dump.
+               // Resize it and try again.
+               buf = make([]byte, 2*len(buf))
        }
 }
 

diff --git a/src/net/http/cgi/plan9_test.go b/src/net/http/cgi/plan9_test.go
deleted file mode 100644 (file)
index b7ace3f..0000000
--- a/src/net/http/cgi/plan9_test.go
+++ /dev/null
@@ -1,17 +0,0 @@
-// Copyright 2013 The Go Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style
-// license that can be found in the LICENSE file.
-
-//go:build plan9
-
-package cgi
-
-import (
-       "os"
-       "strconv"
-)
-
-func isProcessRunning(pid int) bool {
-       _, err := os.Stat("/proc/" + strconv.Itoa(pid))
-       return err == nil
-}

diff --git a/src/net/http/cgi/posix_test.go b/src/net/http/cgi/posix_test.go
deleted file mode 100644 (file)
index 49b9470..0000000
--- a/src/net/http/cgi/posix_test.go
+++ /dev/null
@@ -1,20 +0,0 @@
-// Copyright 2013 The Go Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style
-// license that can be found in the LICENSE file.
-
-//go:build !plan9
-
-package cgi
-
-import (
-       "os"
-       "syscall"
-)
-
-func isProcessRunning(pid int) bool {
-       p, err := os.FindProcess(pid)
-       if err != nil {
-               return false
-       }
-       return p.Signal(syscall.Signal(0)) == nil
-}