Merge pull request #31 from arduino/kill_improvements

Improvements to Process.Kill() method.

Author: cmaglie

process.go

@@ -55,7 +55,8 @@ func NewProcess(extraEnv []string, args ...string) (*Process, error) {
 		cmd: exec.Command(args[0], args[1:]...),
 	}
 	p.cmd.Env = append(os.Environ(), extraEnv...)
-	p.TellCommandNotToSpawnShell()
+	tellCommandNotToSpawnShell(p.cmd)          // windows specific
+	tellCommandToStartOnNewProcessGroup(p.cmd) // linux specific
 
 	// This is required because some tools detects if the program is running
 	// from terminal by looking at the stdin/out bindings.
@@ -146,7 +147,7 @@ func (p *Process) Signal(sig os.Signal) error {
 // actually exited. This only kills the Process itself, not any other processes it may
 // have started.
 func (p *Process) Kill() error {
-	return p.cmd.Process.Kill()
+	return kill(p.cmd)
 }
 
 // SetDir sets the working directory of the command. If Dir is the empty string, Run

process_linux.go

@@ -0,0 +1,64 @@
+//
+// This file is part of PathsHelper library.
+//
+// Copyright 2023 Arduino AG (http://www.arduino.cc/)
+//
+// PathsHelper library is free software; you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation; either version 2 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program; if not, write to the Free Software
+// Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+//
+// As a special exception, you may use this file as part of a free software
+// library without restriction.  Specifically, if other files instantiate
+// templates or use macros or inline functions from this file, or you compile
+// this file and link it with other files to produce an executable, this
+// file does not by itself cause the resulting executable to be covered by
+// the GNU General Public License.  This exception does not however
+// invalidate any other reasons why the executable file might be covered by
+// the GNU General Public License.
+//
+
+//go:build !windows
+
+package paths
+
+import (
+	"os/exec"
+	"syscall"
+)
+
+func tellCommandNotToSpawnShell(_ *exec.Cmd) {
+	// no op
+}
+
+func tellCommandToStartOnNewProcessGroup(oscmd *exec.Cmd) {
+	// https://groups.google.com/g/golang-nuts/c/XoQ3RhFBJl8
+
+	// Start the process in a new process group.
+	// This is needed to kill the process and its children
+	// if we need to kill the process.
+	if oscmd.SysProcAttr == nil {
+		oscmd.SysProcAttr = &syscall.SysProcAttr{}
+	}
+	oscmd.SysProcAttr.Setpgid = true
+}
+
+func kill(oscmd *exec.Cmd) error {
+	// https://groups.google.com/g/golang-nuts/c/XoQ3RhFBJl8
+
+	// Kill the process group
+	pgid, err := syscall.Getpgid(oscmd.Process.Pid)
+	if err != nil {
+		return err
+	}
+	return syscall.Kill(-pgid, syscall.SIGKILL)
+}

process_others.go

@@ -27,12 +27,22 @@
 // the GNU General Public License.
 //
 
-//go:build !windows
+//go:build !windows && !linux
 
 package paths
 
-import "os/exec"
+import (
+	"os/exec"
+)
 
 func tellCommandNotToSpawnShell(_ *exec.Cmd) {
 	// no op
 }
+
+func tellCommandToStartOnNewProcessGroup(_ *exec.Cmd) {
+	// no op
+}
+
+func kill(oscmd *exec.Cmd) error {
+	return oscmd.Process.Kill()
+}

process_test.go

@@ -31,6 +31,7 @@ package paths
 
 import (
 	"context"
+	"runtime"
 	"testing"
 	"time"
 
@@ -54,3 +55,20 @@ func TestProcessWithinContext(t *testing.T) {
 	require.Less(t, time.Since(start), 500*time.Millisecond)
 	cancel()
 }
+
+func TestKillProcessGroupOnLinux(t *testing.T) {
+	if runtime.GOOS != "linux" {
+		t.Skip("skipping test on non-linux system")
+	}
+
+	p, err := NewProcess(nil, "bash", "-c", "sleep 5 ; echo -n 5")
+	require.NoError(t, err)
+	start := time.Now()
+	ctx, cancel := context.WithTimeout(context.Background(), time.Second)
+	defer cancel()
+
+	_, _, err = p.RunAndCaptureOutput(ctx)
+	require.EqualError(t, err, "signal: killed")
+	// Assert that the process was killed within the timeout
+	require.Less(t, time.Since(start), 2*time.Second)
+}

process_windows.go

@@ -35,5 +35,16 @@ import (
 )
 
 func tellCommandNotToSpawnShell(oscmd *exec.Cmd) {
-	oscmd.SysProcAttr = &syscall.SysProcAttr{HideWindow: true}
+	if oscmd.SysProcAttr == nil {
+		oscmd.SysProcAttr = &syscall.SysProcAttr{}
+	}
+	oscmd.SysProcAttr.HideWindow = true
+}
+
+func tellCommandToStartOnNewProcessGroup(_ *exec.Cmd) {
+	// no op
+}
+
+func kill(oscmd *exec.Cmd) error {
+	return oscmd.Process.Kill()
 }