From: Bryan C. Mills Date: Tue, 25 Oct 2022 21:05:46 +0000 (-0400) Subject: cmd/go/internal/script: use the Cancel and WaitDelay fields for subprocesses X-Git-Tag: go1.20rc1~530 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=49abdbccde5de042997d6aabe7819212b88f2ef5;p=gostls13.git 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 --- diff --git a/src/cmd/go/internal/script/cmds.go b/src/cmd/go/internal/script/cmds.go index 9fb092e0d8..393f565733 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 b0a4087661..6e8f158913 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 2a9900782b..db5e6cafda 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.