From 49abdbccde5de042997d6aabe7819212b88f2ef5 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Tue, 25 Oct 2022 17:05:46 -0400 Subject: [PATCH] cmd/go/internal/script: use the Cancel and WaitDelay fields for subprocesses The Cancel and WaitDelay fields recently added to exec.Cmd are intended to support exactly the sort of cancellation behavior that we need for script tests. Use them, and simplify the cmd/go tests accordingly. The more robust implementation may also help to diagose recurring test hangs (#50187). For #50187. Updates #27494. Change-Id: I7817fca0dd9a18e18984a252d3116f6a5275a401 Reviewed-on: https://go-review.googlesource.com/c/go/+/445357 Run-TryBot: Bryan Mills Reviewed-by: Ian Lance Taylor TryBot-Result: Gopher Robot --- src/cmd/go/internal/script/cmds.go | 93 +++++------------------------ src/cmd/go/internal/vcweb/script.go | 3 +- src/cmd/go/scriptcmds_test.go | 18 ++++-- 3 files changed, 30 insertions(+), 84 deletions(-) diff --git a/src/cmd/go/internal/script/cmds.go b/src/cmd/go/internal/script/cmds.go index 9fb092e0d8a1d0..393f565733d499 100644 --- a/src/cmd/go/internal/script/cmds.go +++ b/src/cmd/go/internal/script/cmds.go @@ -6,7 +6,6 @@ package script import ( "cmd/go/internal/robustio" - "context" "errors" "fmt" "internal/diff" @@ -36,7 +35,7 @@ func DefaultCmds() map[string]Cmd { "cp": Cp(), "echo": Echo(), "env": Env(), - "exec": Exec(os.Interrupt, 100*time.Millisecond), // arbitrary grace period + "exec": Exec(func(cmd *exec.Cmd) error { return cmd.Process.Signal(os.Interrupt) }, 100*time.Millisecond), // arbitrary grace period "exists": Exists(), "grep": Grep(), "help": Help(), @@ -400,7 +399,7 @@ func Env() Cmd { // When the Script's context is canceled, Exec sends the interrupt signal, then // waits for up to the given delay for the subprocess to flush output before // terminating it with os.Kill. -func Exec(interrupt os.Signal, delay time.Duration) Cmd { +func Exec(cancel func(*exec.Cmd) error, waitDelay time.Duration) Cmd { return Command( CmdUsage{ Summary: "run an executable program with arguments", @@ -428,13 +427,19 @@ func Exec(interrupt os.Signal, delay time.Duration) Cmd { } } - return startCommand(s, name, path, args[1:], interrupt, delay) + return startCommand(s, name, path, args[1:], cancel, waitDelay) }) } -func startCommand(s *State, name, path string, args []string, interrupt os.Signal, gracePeriod time.Duration) (WaitFunc, error) { +func startCommand(s *State, name, path string, args []string, cancel func(*exec.Cmd) error, waitDelay time.Duration) (WaitFunc, error) { var stdoutBuf, stderrBuf strings.Builder - cmd := exec.Command(path, args...) + cmd := exec.CommandContext(s.Context(), path, args...) + if cancel == nil { + cmd.Cancel = nil + } else { + cmd.Cancel = func() error { return cancel(cmd) } + } + cmd.WaitDelay = waitDelay cmd.Args[0] = name cmd.Dir = s.Getwd() cmd.Env = s.env @@ -444,16 +449,9 @@ func startCommand(s *State, name, path string, args []string, interrupt os.Signa return nil, err } - var waitErr error - done := make(chan struct{}) - go func() { - waitErr = waitOrStop(s.Context(), cmd, interrupt, gracePeriod) - close(done) - }() - wait := func(s *State) (stdout, stderr string, err error) { - <-done - return stdoutBuf.String(), stderrBuf.String(), waitErr + err = cmd.Wait() + return stdoutBuf.String(), stderrBuf.String(), err } return wait, nil } @@ -535,67 +533,6 @@ func pathEnvName() string { } } -// waitOrStop waits for the already-started command cmd by calling its Wait method. -// -// If cmd does not return before ctx is done, waitOrStop sends it the given interrupt signal. -// If killDelay is positive, waitOrStop waits that additional period for Wait to return before sending os.Kill. -// -// This function is copied from the one added to x/playground/internal in -// http://golang.org/cl/228438. -func waitOrStop(ctx context.Context, cmd *exec.Cmd, interrupt os.Signal, killDelay time.Duration) error { - if cmd.Process == nil { - panic("waitOrStop called with a nil cmd.Process — missing Start call?") - } - if interrupt == nil { - panic("waitOrStop requires a non-nil interrupt signal") - } - - errc := make(chan error) - go func() { - select { - case errc <- nil: - return - case <-ctx.Done(): - } - - err := cmd.Process.Signal(interrupt) - if err == nil { - err = ctx.Err() // Report ctx.Err() as the reason we interrupted. - } else if err == os.ErrProcessDone { - errc <- nil - return - } - - if killDelay > 0 { - timer := time.NewTimer(killDelay) - select { - // Report ctx.Err() as the reason we interrupted the process... - case errc <- ctx.Err(): - timer.Stop() - return - // ...but after killDelay has elapsed, fall back to a stronger signal. - case <-timer.C: - } - - // Wait still hasn't returned. - // Kill the process harder to make sure that it exits. - // - // Ignore any error: if cmd.Process has already terminated, we still - // want to send ctx.Err() (or the error from the Interrupt call) - // to properly attribute the signal that may have terminated it. - _ = cmd.Process.Kill() - } - - errc <- err - }() - - waitErr := cmd.Wait() - if interruptErr := <-errc; interruptErr != nil { - return interruptErr - } - return waitErr -} - // Exists checks that the named file(s) exist. func Exists() Cmd { return Command( @@ -834,7 +771,7 @@ func Mv() Cmd { // Program returns a new command that runs the named program, found from the // host process's PATH (not looked up in the script's PATH). -func Program(name string, interrupt os.Signal, gracePeriod time.Duration) Cmd { +func Program(name string, cancel func(*exec.Cmd) error, waitDelay time.Duration) Cmd { var ( shortName string summary string @@ -864,7 +801,7 @@ func Program(name string, interrupt os.Signal, gracePeriod time.Duration) Cmd { if pathErr != nil { return nil, pathErr } - return startCommand(s, shortName, path, args, interrupt, gracePeriod) + return startCommand(s, shortName, path, args, cancel, waitDelay) }) } diff --git a/src/cmd/go/internal/vcweb/script.go b/src/cmd/go/internal/vcweb/script.go index b0a4087661f0e2..6e8f1589137f00 100644 --- a/src/cmd/go/internal/vcweb/script.go +++ b/src/cmd/go/internal/vcweb/script.go @@ -16,6 +16,7 @@ import ( "log" "net/http" "os" + "os/exec" "path/filepath" "runtime" "strconv" @@ -31,7 +32,7 @@ import ( func newScriptEngine() *script.Engine { conds := script.DefaultConds() - interrupt := os.Interrupt + interrupt := func(cmd *exec.Cmd) error { return cmd.Process.Signal(os.Interrupt) } gracePeriod := 1 * time.Second // arbitrary cmds := script.DefaultCmds() diff --git a/src/cmd/go/scriptcmds_test.go b/src/cmd/go/scriptcmds_test.go index 2a9900782ba5e7..db5e6cafdafe51 100644 --- a/src/cmd/go/scriptcmds_test.go +++ b/src/cmd/go/scriptcmds_test.go @@ -11,15 +11,23 @@ import ( "errors" "fmt" "os" + "os/exec" "strings" "time" ) -func scriptCommands(interrupt os.Signal, gracePeriod time.Duration) map[string]script.Cmd { +func scriptCommands(interrupt os.Signal, waitDelay time.Duration) map[string]script.Cmd { cmds := scripttest.DefaultCmds() // Customize the "exec" interrupt signal and grace period. - cmdExec := script.Exec(quitSignal(), gracePeriod) + var cancel func(cmd *exec.Cmd) error + if interrupt != nil { + cancel = func(cmd *exec.Cmd) error { + return cmd.Process.Signal(interrupt) + } + } + + cmdExec := script.Exec(cancel, waitDelay) cmds["exec"] = cmdExec add := func(name string, cmd script.Cmd) { @@ -30,7 +38,7 @@ func scriptCommands(interrupt os.Signal, gracePeriod time.Duration) map[string]s } add("cc", scriptCC(cmdExec)) - cmdGo := scriptGo(interrupt, gracePeriod) + cmdGo := scriptGo(cancel, waitDelay) add("go", cmdGo) add("stale", scriptStale(cmdGo)) @@ -62,8 +70,8 @@ func scriptCC(cmdExec script.Cmd) script.Cmd { } // scriptGo runs the go command. -func scriptGo(interrupt os.Signal, gracePeriod time.Duration) script.Cmd { - return script.Program(testGo, interrupt, gracePeriod) +func scriptGo(cancel func(*exec.Cmd) error, waitDelay time.Duration) script.Cmd { + return script.Program(testGo, cancel, waitDelay) } // scriptStale checks that the named build targets are stale.