Split proc termination paths for UNIX vs. Windows

This allows us to rely on gentler termination semantics (os.Interrupt)
for UNIX-y systems, as such interrupts do not work on Windows.
Terminations are triggered by e.g. ctrl-C interrupts. Relying generally
on Process.Kill() for such purposes was causing git to often exit in
unclean ways, resulting in a dirty cache dir and putting users in an
awkward position.

We maintain the invariant that we ensure the process IS either exited,
or forcibly Process.Kill()ed, before exiting monitoredCmd.run(). This
allows us to ensure we do not have orphan subprocesses that exist beyond
the lifetime of the parent sourceMgr.

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
 }
 

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
+}

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()
+}