]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/go/internal/script: use the Cancel and WaitDelay fields for subprocesses
authorBryan C. Mills <bcmills@google.com>
Tue, 25 Oct 2022 21:05:46 +0000 (17:05 -0400)
committerBryan Mills <bcmills@google.com>
Wed, 26 Oct 2022 14:08:16 +0000 (14:08 +0000)
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 <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

src/cmd/go/internal/script/cmds.go
src/cmd/go/internal/vcweb/script.go
src/cmd/go/scriptcmds_test.go

index 9fb092e0d8a1d00ffe68af1015e722d31b6fc110..393f565733d499d4d2ba19b63a847a91f9342c5a 100644 (file)
@@ -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)
                })
 }
 
index b0a4087661f0e2066d4c79cc9e55e06593a7e880..6e8f1589137f00e817b768cd8c8b9ed9805bf1ea 100644 (file)
@@ -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()
index 2a9900782ba5e74967a2c80e787ee0aa01bc8576..db5e6cafdafe51d5a94f97e0841f1c3e9fa62a27 100644 (file)
@@ -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.