fix: properly kill child processes on Windows (#68)

Also add a test that ensures process groups are killed, and create a new unit test action that runs the tests on Ubuntu, MacOS, and Windows.

Author: mtraver

.github/workflows/build.yml

@@ -1,9 +1,12 @@
-on: [push, pull_request]
-
+name: Check compiled action script CI
+on:
+  push:
+    branches:
+    - master
+  pull_request:
 jobs:
   compile:
     runs-on: ubuntu-latest
-    name: Check compiled script
     steps:
     - uses: actions/checkout@v2
     - name: Setup Node.js

.github/workflows/main.yml

@@ -1,15 +1,18 @@
-on: [push, pull_request]
-
+name: Conformance test framework validation CI
+on:
+  push:
+    branches:
+    - master
+  pull_request:
 jobs:
   run-conformance:
     runs-on: ubuntu-latest
-    name: Validate conformance test framework
     steps:
     - uses: actions/checkout@v2
 
     - uses: actions/setup-go@v2
       with:
-        go-version: '^1.13.1'
+        go-version: '^1.16'
 
     - name: Vendor dependencies
       run: "go mod vendor"

.github/workflows/unit.yml

@@ -0,0 +1,24 @@
+name: Unit test CI
+on:
+  push:
+    branches:
+    - master
+  pull_request:
+jobs:
+  test:
+    strategy:
+      matrix:
+        os: [ubuntu-latest, macos-latest, windows-latest]
+    runs-on: ${{ matrix.os }}
+
+    steps:
+      - name: Check out repo
+        uses: actions/checkout@v2
+
+      - name: Set up Go
+        uses: actions/setup-go@v2
+        with:
+          go-version: '^1.16'
+
+      - name: Build and test
+        run: "go test -v -cover ./..."

client/local.go

@@ -18,9 +18,7 @@ import (
 	"io/ioutil"
 	"log"
 	"os"
-	"os/exec"
 	"strings"
-	"syscall"
 	"time"
 )
 
@@ -31,14 +29,7 @@ type localFunctionServer struct {
 
 func (l *localFunctionServer) Start() (func(), error) {
 	args := strings.Fields(l.cmd)
-	cmd := exec.Command(args[0], args[1:]...)
-
-	// Set a process group ID so that later we can kill child processes too. As an
-	// example, if the command is `go run main.go`, Go will build a binary in a
-	// temp dir and then execute it. If we simply cmd.Process.Kill() the exec.Command
-	// then the running binary will not be killed. Only if we make a group and then
-	// kill the group will child processes be killed.
-	cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
+	cmd := newCmd(args)
 
 	stdout, err := os.Create(stdoutFile)
 	if err != nil {
@@ -64,19 +55,8 @@ func (l *localFunctionServer) Start() (func(), error) {
 		stdout.Close()
 		stderr.Close()
 
-		pgid, err := syscall.Getpgid(cmd.Process.Pid)
-		if err != nil {
-			log.Printf("Failed to get pgid: %v", err)
-
-			// Kill just the parent process since we failed to get the process group ID.
-			if err := cmd.Process.Kill(); err != nil {
-				log.Fatalf("Failed to kill process: %v", err)
-			}
-		} else {
-			// Kill the whole process group.
-			if err := syscall.Kill(-pgid, syscall.SIGKILL); err != nil {
-				log.Fatalf("Failed to kill process group: %v", err)
-			}
+		if err := stopCmd(cmd); err != nil {
+			log.Fatalf("Failed to shut down framework server: %v", err)
 		}
 
 		log.Printf("Framework server shut down. Wrote logs to %v and %v.", stdoutFile, stderrFile)

client/local_notwindows.go

@@ -0,0 +1,55 @@
+// Copyright 2021 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// +build !windows
+
+package main
+
+import (
+	"fmt"
+	"os/exec"
+	"syscall"
+)
+
+func newCmd(args []string) *exec.Cmd {
+	cmd := exec.Command(args[0], args[1:]...)
+
+	// Set a process group ID so that later we can kill child processes too. As an
+	// example, if the command is `go run main.go`, Go will build a binary in a
+	// temp dir and then execute it. If we simply cmd.Process.Kill() the exec.Command
+	// then the running binary will not be killed. Only if we make a group and then
+	// kill the group will child processes be killed.
+	cmd.SysProcAttr = &syscall.SysProcAttr{
+		Setpgid: true,
+	}
+
+	return cmd
+}
+
+func stopCmd(cmd *exec.Cmd) error {
+	pgid, err := syscall.Getpgid(cmd.Process.Pid)
+	if err != nil {
+		// Kill just the parent process since we failed to get the process group ID.
+		if err := cmd.Process.Kill(); err != nil {
+			return fmt.Errorf("failed to kill process: %v", err)
+		}
+	} else {
+		// Kill the whole process group.
+		if err := syscall.Kill(-pgid, syscall.SIGKILL); err != nil {
+			return fmt.Errorf("failed to kill process group: %v", err)
+		}
+	}
+
+	return nil
+}

client/local_test.go

@@ -0,0 +1,60 @@
+// Copyright 2021 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package main
+
+import (
+	"fmt"
+	"io/ioutil"
+	"path/filepath"
+	"testing"
+)
+
+const testProgram = `package main
+
+import (
+	"fmt"
+	"time"
+)
+
+func main() {
+	sleepDuration := time.Second * 90
+	fmt.Printf("Hello from test program. Sleeping for %v.\n", sleepDuration)
+	time.Sleep(sleepDuration)
+}
+`
+
+func TestStartAndShutdown(t *testing.T) {
+	dir := t.TempDir()
+	f := filepath.Join(dir, "main.go")
+	if err := ioutil.WriteFile(f, []byte(testProgram), 0644); err != nil {
+		t.Fatalf("Failed to write file: %v", err)
+	}
+
+	server := localFunctionServer{
+		// `go run` compiles the program and then execs it, which allows us to test that
+		// the whole process group is killed. It's done this way instead of something
+		// simpler like "/bin/sh -c 'exec sleep 90'" so that it's cross-platform compatible.
+		cmd: fmt.Sprintf("go run %s", f),
+	}
+
+	shutdown, err := server.Start()
+	if shutdown != nil {
+		defer shutdown()
+	}
+
+	if err != nil {
+		t.Errorf("unable to start localFunctionServer: %v", err)
+	}
+}

client/local_windows.go

@@ -0,0 +1,43 @@
+// Copyright 2021 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package main
+
+import (
+	"fmt"
+	"os/exec"
+	"syscall"
+)
+
+func newCmd(args []string) *exec.Cmd {
+	cmd := exec.Command(args[0], args[1:]...)
+
+	// Make a process group so that later we can kill child processes too. As an
+	// example, if the command is `go run main.go`, Go will build a binary in a
+	// temp dir and then execute it. If we simply cmd.Process.Kill() the exec.Command
+	// then the running binary will not be killed. Only if we make a group will
+	// child processes be killed.
+	cmd.SysProcAttr = &syscall.SysProcAttr{
+		CreationFlags: syscall.CREATE_NEW_PROCESS_GROUP,
+	}
+
+	return cmd
+}
+
+func stopCmd(cmd *exec.Cmd) error {
+	if err := cmd.Process.Kill(); err != nil {
+		return fmt.Errorf("failed to kill process: %v", err)
+	}
+	return nil
+}