From 1e07c144c3e43d95b0c21fdc73c520fe809d7f51 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Thu, 4 Jan 2024 11:41:54 -0500 Subject: [PATCH] net/http/cgi: in TestCopyError, check for a Handler.ServeHTTP goroutine instead of a running PID 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 Auto-Submit: Bryan Mills Reviewed-by: Damien Neil --- src/net/http/cgi/host_test.go | 56 +++++++++++++++++++--------------- src/net/http/cgi/plan9_test.go | 17 ----------- src/net/http/cgi/posix_test.go | 20 ------------ 3 files changed, 31 insertions(+), 62 deletions(-) delete mode 100644 src/net/http/cgi/plan9_test.go delete mode 100644 src/net/http/cgi/posix_test.go diff --git a/src/net/http/cgi/host_test.go b/src/net/http/cgi/host_test.go index 78e05d592a..f29395fe84 100644 --- 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 index b7ace3f81c..0000000000 --- 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 index 49b9470d4a..0000000000 --- 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 -} -- 2.48.1