main: fix child process handling/cleanup (#108)

This change simplifies child process handling and cleanup, which should make tests and hot-reloading most robust.

go.mod

@@ -7,7 +7,7 @@ require (
 	github.com/google/go-cmp v0.5.8
 	golang.org/x/mod v0.7.0
 	golang.org/x/net v0.0.0-20220706163947-c90051bbdb60
-	golang.org/x/sync v0.0.0-20220601150217-0de741cfad7f
+	golang.org/x/sync v0.1.0
 )
 
 require golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a // indirect

go.sum

@@ -8,6 +8,8 @@ golang.org/x/net v0.0.0-20220706163947-c90051bbdb60 h1:8NSylCMxLW4JvserAndSgFL7a
 golang.org/x/net v0.0.0-20220706163947-c90051bbdb60/go.mod h1:XRhObCWvk6IyKnWLug+ECip1KBveYUHfp+8e9klMJ9c=
 golang.org/x/sync v0.0.0-20220601150217-0de741cfad7f h1:Ax0t5p6N38Ga0dThY21weqDEyz2oklo4IvDkpigvkD8=
 golang.org/x/sync v0.0.0-20220601150217-0de741cfad7f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
+golang.org/x/sync v0.1.0 h1:wsuoTGHzEhffawBOhz5CYhcrV4IdKZbEyZjBMuTp12o=
+golang.org/x/sync v0.1.0/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sys v0.0.0-20220412211240-33da011f77ad/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a h1:dGzPydgVsqGcTRVwiLJ1jVbufYwmzD3LfVPLKsKg+0k=
 golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=

main.go

@@ -407,6 +407,8 @@ func (r *runCmd) do() error {
 		return fmt.Errorf("build command: %w", err)
 	}
 
+	binExePath := filepath.Join(r.outDir, "bin", r.projectName.String())
+
 	if r.compileOnly {
 		return nil
 	}
@@ -481,7 +483,7 @@ func (r *runCmd) do() error {
 			}
 
 			watchForReload(ctx, r.appDir, reload)
-			if err := runProject(ctx, filepath.Join(r.outDir, "bin", r.projectName.String()), ln); err != nil {
+			if err := runProject(ctx, binExePath, ln); err != nil {
 				return fmt.Errorf("building and running generated Go code: %v", err)
 			}
 
@@ -506,7 +508,7 @@ func (r *runCmd) do() error {
 				return fmt.Errorf("listening on TCP socket: %v", err)
 			}
 		}
-		if err := runProject(context.Background(), filepath.Join(r.outDir, "bin", r.projectName.String()), ln); err != nil {
+		if err := runProject(context.Background(), binExePath, ln); err != nil {
 			return fmt.Errorf("building and running generated Go code: %v", err)
 		}
 	}
@@ -804,62 +806,36 @@ func runProject(ctx context.Context, exePath string, ln net.Listener) error {
 		return fmt.Errorf("getting file from Unix socket listener: %w", err)
 	}
 
-	cmd := exec.Command(exePath)
+	cmd := exec.CommandContext(ctx, exePath)
 	cmd.Stdout = os.Stdout
 	cmd.Stderr = os.Stderr
-	sysProcAttr(cmd)
 	cmd.ExtraFiles = []*os.File{file}
 	cmd.Env = append(os.Environ(), "PUSHUP_LISTENER_FD=3")
-	if err := cmd.Start(); err != nil {
-		return fmt.Errorf("starting project main executable: %w", err)
-	}
 
 	g := new(errgroup.Group)
-	done := make(chan struct{})
 
 	g.Go(func() error {
-		select {
-		case <-ctx.Done():
-		case <-done:
-			return nil
-		}
-		if err := context.Cause(ctx); errors.Is(err, errFileChanged) {
-			log.Printf("\x1b[35mFILE CHANGED\x1b[0m")
-		} else if errors.Is(err, errSignalCaught) {
-			log.Printf("\x1b[34mSIGNAL TRAPPED\x1b[0m")
-		} else {
-			log.Printf("context cancelled (unknown reason)")
-		}
-		if err := syscall.Kill(-cmd.Process.Pid, syscall.SIGINT); err != nil {
-			return fmt.Errorf("syscall kill: %w", err)
-		}
-		if !errors.Is(ctx.Err(), context.Canceled) {
-			return ctx.Err()
+		<-ctx.Done()
+		if errors.Is(context.Cause(ctx), errFileChanged) {
+			log.Printf("[PUSHUP RELOADER] file changed, reloading")
+		} else if errors.Is(context.Cause(ctx), errSignalCaught) {
+			log.Printf("[PUSHUP] got signal, shutting down")
 		}
 		return nil
 	})
 
 	g.Go(func() error {
-		defer close(done)
-		// NOTE(paulsmith): we have to wait() the child process(es) in any case,
-		// regardless of how they were exited. this is also why there is a
-		// `done' channel in this function, to signal to the other goroutine
-		// waiting for context cancellation.
-		if err := cmd.Wait(); err != nil {
-			if ee, ok := err.(*exec.ExitError); ok {
-				if err := context.Cause(ctx); errors.Is(err, errFileChanged) || errors.Is(err, errSignalCaught) {
-					return fmt.Errorf("wait: %w", ee)
-				}
-			} else {
-				return fmt.Errorf("wait: %w", ee)
-			}
-		}
+		// NOTE(paulsmith): intentionally ignoring *ExitError because the child
+		// process will be signal killed here as a matter of course
+		//nolint:errcheck
+		cmd.Run()
 		return nil
 	})
 
-	if err := g.Wait(); err != nil {
-		return fmt.Errorf("errgroup: %w", err)
-	}
+	// NOTE(paulsmith): intentionally ignoring *ExitError for same reason as
+	// above
+	//nolint:errcheck
+	g.Wait()
 
 	return nil
 }

main_test.go

@@ -11,7 +11,9 @@ import (
 	"net/http"
 	"os"
 	"os/exec"
+	"os/signal"
 	"path/filepath"
+	"runtime"
 	"strings"
 	"syscall"
 	"testing"
@@ -29,6 +31,19 @@ type testRequest struct {
 }
 
 func TestPushup(t *testing.T) {
+	// do a `kill -QUIT <pid>` to get a stack trace of all goroutines, useful
+	// if the test seems to hang
+	go func() {
+		sigs := make(chan os.Signal, 1)
+		signal.Notify(sigs, syscall.SIGQUIT)
+		buf := make([]byte, 1<<20)
+		for {
+			<-sigs
+			stacklen := runtime.Stack(buf, true)
+			log.Printf("=== received SIGQUIT ===\n*** goroutine dump...\n%s\n*** end\n", buf[:stacklen])
+		}
+	}()
+
 	if testing.Short() {
 		t.Skip("skipping test in short mode")
 	}
@@ -158,31 +173,29 @@ func TestPushup(t *testing.T) {
 						defer os.RemoveAll(tmpdir)
 						socketPath := filepath.Join(tmpdir, "sock")
 
-						var errb bytes.Buffer
-						var stdout io.ReadCloser
-						var allgood bool
-						var cmd *exec.Cmd
+						var (
+							errb    bytes.Buffer
+							allgood bool
+						)
 
-						g.Go(func() error {
-							cmd = exec.Command(pushup, "run", "-page", pushupFile, "-unix-socket", socketPath)
-							sysProcAttr(cmd)
+						cmd := exec.Command(pushup, "run", "-page", pushupFile, "-unix-socket", socketPath)
+						sysProcAttr(cmd)
 
-							var err error
-							stdout, err = cmd.StdoutPipe()
-							if err != nil {
-								return err
-							}
+						stdout, err := cmd.StdoutPipe()
+						if err != nil {
+							t.Fatal(err)
+						}
 
-							cmd.Stderr = &errb
+						cmd.Stderr = &errb
 
-							if err := cmd.Start(); err != nil {
-								return err
-							}
+						if err := cmd.Start(); err != nil {
+							t.Fatal(err)
+						}
 
+						g.Go(func() error {
 							if err := cmd.Wait(); err != nil {
 								return err
 							}
-
 							return nil
 						})
 
@@ -220,7 +233,7 @@ func TestPushup(t *testing.T) {
 							select {
 							case <-done:
 								if cmd != nil {
-									if err := syscall.Kill(-cmd.Process.Pid, syscall.SIGINT); err != nil {
+									if err := syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL); err != nil {
 										return err
 									}
 								}

reloader.go

@@ -42,7 +42,6 @@ func watchForReload(ctx context.Context, root string, reload chan struct{}) {
 			}
 			return
 		}
-		log.Printf("change detected in project directory, reloading")
 		reload <- struct{}{}
 		watcher.Close()
 	})