diff --git a/internal/gps/cmd.go b/internal/gps/cmd.go index 73d8702d..53999e88 100644 --- a/internal/gps/cmd.go +++ b/internal/gps/cmd.go @@ -45,15 +45,15 @@ func (c *monitoredCmd) run(ctx context.Context) error { return ctx.Err() } - ticker := time.NewTicker(c.timeout) - done := make(chan error, 1) - defer ticker.Stop() - err := c.cmd.Start() if err != nil { return err } + ticker := time.NewTicker(c.timeout) + defer ticker.Stop() + done := make(chan error, 1) + go func() { done <- c.cmd.Wait() }() @@ -62,14 +62,14 @@ func (c *monitoredCmd) run(ctx context.Context) error { select { case <-ticker.C: if c.hasTimedOut() { - if err := c.cmd.Process.Kill(); err != nil { + if err := killProcess(c.cmd); err != nil { return &killCmdError{err} } return &timeoutError{c.timeout} } case <-ctx.Done(): - if err := c.cmd.Process.Kill(); err != nil { + if err := killProcess(c.cmd); err != nil { return &killCmdError{err} } return ctx.Err() @@ -90,6 +90,7 @@ func (c *monitoredCmd) combinedOutput(ctx context.Context) ([]byte, error) { return c.stderr.buf.Bytes(), err } + // FIXME(sdboyer) this is not actually combined output return c.stdout.buf.Bytes(), nil } diff --git a/internal/gps/cmd_unix.go b/internal/gps/cmd_unix.go new file mode 100644 index 00000000..b885b0a8 --- /dev/null +++ b/internal/gps/cmd_unix.go @@ -0,0 +1,53 @@ +// Copyright 2017 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. + +// +build !windows + +package gps + +import ( + "os" + "os/exec" + "time" +) + +// killProcess manages the termination of subprocesses in a way that tries to be +// gentle (via os.Interrupt), but resorts to Kill if needed. +// +// +// TODO(sdboyer) should the return differentiate between whether gentle methods +// succeeded vs. falling back to a hard kill? +func killProcess(cmd *exec.Cmd) error { + if err := cmd.Process.Signal(os.Interrupt); err != nil { + // If an error comes back from attempting to signal, proceed immediately + // to hard kill. + return cmd.Process.Kill() + } + + // If the process doesn't exit immediately, check every 50ms, up to 3s, + // after which send a hard kill. + // + // Cannot rely on cmd.ProcessState.Exited() here, as that is not set + // correctly when the process exits due to a signal. See + // https://github.com/golang/go/issues/19798 + if cmd.ProcessState == nil || !cmd.ProcessState.Exited() { + to := time.NewTimer(3 * time.Second) + tick := time.NewTicker(50 * time.Millisecond) + + defer to.Stop() + defer tick.Stop() + + // Loop until the ProcessState shows up, indicating the proc has exited, + // or the timer expires and + for cmd.ProcessState != nil { + select { + case <-to.C: + return cmd.Process.Kill() + case <-tick.C: + } + } + } + + return nil +} diff --git a/internal/gps/cmd_windows.go b/internal/gps/cmd_windows.go new file mode 100644 index 00000000..168884cb --- /dev/null +++ b/internal/gps/cmd_windows.go @@ -0,0 +1,14 @@ +// Copyright 2017 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. + +// +build windows + +package gps + +import "os/exec" + +func killProcess(cmd *exec.Cmd) error { + // TODO it'd be great if this could be more sophisticated... + return cmd.Process.Kill() +}