From 498ee73a4b9f48c0916bb5a2bdd22ddf6aca79c6 Mon Sep 17 00:00:00 2001
From: "Bryan C. Mills" <bcmills@google.com>
Date: Wed, 12 Oct 2022 15:11:16 -0400
Subject: [PATCH] os/exec: reduce arbitrary sleeps in TestWaitid

If we use the "pipetest" helper command instead of "sleep",
we can use its stdout pipe to determine when the process
is ready to handle a SIGSTOP, and we can additionally check
that sending a SIGCONT actually causes the process to continue.

This also allows us to remove the "sleep" helper command,
making the test file somewhat more concise.

Noticed while looking into #50138.

Change-Id: If4fdee4b1ddf28c6ed07ec3268c81b73c2600238
Reviewed-on: https://go-review.googlesource.com/c/go/+/442576
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
---
 src/os/exec/exec_posix_test.go | 57 ++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 19 deletions(-)

diff --git a/src/os/exec/exec_posix_test.go b/src/os/exec/exec_posix_test.go
index d366840bb10..5d828b3475a 100644
--- a/src/os/exec/exec_posix_test.go
+++ b/src/os/exec/exec_posix_test.go
@@ -9,6 +9,7 @@ package exec_test
 import (
 	"fmt"
 	"internal/testenv"
+	"io"
 	"os"
 	"os/user"
 	"path/filepath"
@@ -23,7 +24,6 @@ import (
 
 func init() {
 	registerHelperCommand("pwd", cmdPwd)
-	registerHelperCommand("sleep", cmdSleep)
 }
 
 func cmdPwd(...string) {
@@ -35,15 +35,6 @@ func cmdPwd(...string) {
 	fmt.Println(pwd)
 }
 
-func cmdSleep(args ...string) {
-	n, err := strconv.Atoi(args[0])
-	if err != nil {
-		fmt.Println(err)
-		os.Exit(1)
-	}
-	time.Sleep(time.Duration(n) * time.Second)
-}
-
 func TestCredentialNoSetGroups(t *testing.T) {
 	if runtime.GOOS == "android" {
 		maySkipHelperCommand("echo")
@@ -86,15 +77,29 @@ func TestCredentialNoSetGroups(t *testing.T) {
 func TestWaitid(t *testing.T) {
 	t.Parallel()
 
-	cmd := helperCommand(t, "sleep", "3")
+	cmd := helperCommand(t, "pipetest")
+	stdin, err := cmd.StdinPipe()
+	if err != nil {
+		t.Fatal(err)
+	}
+	stdout, err := cmd.StdoutPipe()
+	if err != nil {
+		t.Fatal(err)
+	}
 	if err := cmd.Start(); err != nil {
 		t.Fatal(err)
 	}
 
-	// The sleeps here are unnecessary in the sense that the test
-	// should still pass, but they are useful to make it more
-	// likely that we are testing the expected state of the child.
-	time.Sleep(100 * time.Millisecond)
+	// Wait for the child process to come up and register any signal handlers.
+	const msg = "O:ping\n"
+	if _, err := io.WriteString(stdin, msg); err != nil {
+		t.Fatal(err)
+	}
+	buf := make([]byte, len(msg))
+	if _, err := io.ReadFull(stdout, buf); err != nil {
+		t.Fatal(err)
+	}
+	// Now leave the pipes open so that the process will hang until we close stdin.
 
 	if err := cmd.Process.Signal(syscall.SIGSTOP); err != nil {
 		cmd.Process.Kill()
@@ -106,16 +111,30 @@ func TestWaitid(t *testing.T) {
 		ch <- cmd.Wait()
 	}()
 
-	time.Sleep(100 * time.Millisecond)
+	// Give a little time for Wait to block on waiting for the process.
+	// (This is just to give some time to trigger the bug; it should not be
+	// necessary for the test to pass.)
+	if testing.Short() {
+		time.Sleep(1 * time.Millisecond)
+	} else {
+		time.Sleep(10 * time.Millisecond)
+	}
 
+	// This call to Signal should succeed because the process still exists.
+	// (Prior to the fix for #19314, this would fail with os.ErrProcessDone
+	// or an equivalent error.)
 	if err := cmd.Process.Signal(syscall.SIGCONT); err != nil {
 		t.Error(err)
 		syscall.Kill(cmd.Process.Pid, syscall.SIGCONT)
 	}
 
-	cmd.Process.Kill()
-
-	<-ch
+	// The SIGCONT should allow the process to wake up, notice that stdin
+	// is closed, and exit successfully.
+	stdin.Close()
+	err = <-ch
+	if err != nil {
+		t.Fatal(err)
+	}
 }
 
 // https://go.dev/issue/50599: if Env is not set explicitly, setting Dir should