when stopping hooks, stop all their subprocesses too (#3004) (#3087)

go.mod

@@ -26,6 +26,7 @@ require (
 	github.com/pion/webrtc/v3 v3.2.22
 	github.com/stretchr/testify v1.8.4
 	golang.org/x/crypto v0.20.0
+	golang.org/x/sys v0.17.0
 	golang.org/x/term v0.17.0
 	gopkg.in/yaml.v2 v2.4.0
 )
@@ -65,7 +66,6 @@ require (
 	github.com/xo/terminfo v0.0.0-20210125001918-ca9a967f8778 // indirect
 	golang.org/x/arch v0.3.0 // indirect
 	golang.org/x/net v0.21.0 // indirect
-	golang.org/x/sys v0.17.0 // indirect
 	golang.org/x/text v0.14.0 // indirect
 	google.golang.org/protobuf v1.30.0 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect

internal/externalcmd/cmd.go

@@ -42,14 +42,12 @@ func NewCmd(
 ) *Cmd {
 	// replace variables in both Linux and Windows, in order to allow using the
 	// same commands on both of them.
-	expandEnv := func(variable string) string {
+	cmdstr = os.Expand(cmdstr, func(variable string) string {
 		if value, ok := env[variable]; ok {
 			return value
 		}
 		return os.Getenv(variable)
-	}
+	})
-
-	cmdstr = os.Expand(cmdstr, expandEnv)
 
 	if onExit == nil {
 		onExit = func(_ error) {}
@@ -79,8 +77,13 @@ func (e *Cmd) Close() {
 func (e *Cmd) run() {
 	defer e.pool.wg.Done()
 
+	env := append([]string(nil), os.Environ()...)
+	for key, val := range e.env {
+		env = append(env, key+"="+val)
+	}
+
 	for {
-		err := e.runOSSpecific()
+		err := e.runOSSpecific(env)
 		if errors.Is(err, errTerminated) {
 			return
 		}

internal/externalcmd/cmd_unix.go

@@ -13,7 +13,7 @@ import (
 	"github.com/kballard/go-shellquote"
 )
 
-func (e *Cmd) runOSSpecific() error {
+func (e *Cmd) runOSSpecific(env []string) error {
 	cmdParts, err := shellquote.Split(e.cmdstr)
 	if err != nil {
 		return err
@@ -21,14 +21,13 @@ func (e *Cmd) runOSSpecific() error {
 
 	cmd := exec.Command(cmdParts[0], cmdParts[1:]...)
 
-	cmd.Env = append([]string(nil), os.Environ()...)
+	cmd.Env = env
-	for key, val := range e.env {
-		cmd.Env = append(cmd.Env, key+"="+val)
-	}
-
 	cmd.Stdout = os.Stdout
 	cmd.Stderr = os.Stderr
 
+	// set process group in order to allow killing subprocesses
+	cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
+
 	err = cmd.Start()
 	if err != nil {
 		return err
@@ -51,7 +50,8 @@ func (e *Cmd) runOSSpecific() error {
 
 	select {
 	case <-e.terminate:
-		syscall.Kill(cmd.Process.Pid, syscall.SIGINT) //nolint:errcheck
+		// the minus is needed to kill all subprocesses
+		syscall.Kill(-cmd.Process.Pid, syscall.SIGINT) //nolint:errcheck
 		<-cmdDone
 		return errTerminated
 

internal/externalcmd/cmd_win.go

@@ -9,11 +9,52 @@ import (
 	"os/exec"
 	"strings"
 	"syscall"
+	"unsafe"
 
 	"github.com/kballard/go-shellquote"
+	"golang.org/x/sys/windows"
 )
 
-func (e *Cmd) runOSSpecific() error {
+// taken from
+// https://gist.github.com/hallazzang/76f3970bfc949831808bbebc8ca15209
+func createProcessGroup() (windows.Handle, error) {
+	h, err := windows.CreateJobObject(nil, nil)
+	if err != nil {
+		return 0, err
+	}
+
+	info := windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION{
+		BasicLimitInformation: windows.JOBOBJECT_BASIC_LIMIT_INFORMATION{
+			LimitFlags: windows.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE,
+		},
+	}
+	_, err = windows.SetInformationJobObject(
+		h,
+		windows.JobObjectExtendedLimitInformation,
+		uintptr(unsafe.Pointer(&info)),
+		uint32(unsafe.Sizeof(info)))
+	if err != nil {
+		return 0, err
+	}
+
+	return h, nil
+}
+
+func closeProcessGroup(h windows.Handle) error {
+	return windows.CloseHandle(h)
+}
+
+func addProcessToGroup(h windows.Handle, p *os.Process) error {
+	type process struct {
+		Pid    int
+		Handle uintptr
+	}
+
+	return windows.AssignProcessToJobObject(h,
+		windows.Handle((*process)(unsafe.Pointer(p)).Handle))
+}
+
+func (e *Cmd) runOSSpecific(env []string) error {
 	var cmd *exec.Cmd
 
 	// from Golang documentation:
@@ -39,15 +80,22 @@ func (e *Cmd) runOSSpecific() error {
 		cmd = exec.Command(cmdParts[0], cmdParts[1:]...)
 	}
 
-	cmd.Env = append([]string(nil), os.Environ()...)
+	cmd.Env = env
-	for key, val := range e.env {
-		cmd.Env = append(cmd.Env, key+"="+val)
-	}
-
 	cmd.Stdout = os.Stdout
 	cmd.Stderr = os.Stderr
 
-	err := cmd.Start()
+	// create a process group to kill all subprocesses
+	g, err := createProcessGroup()
+	if err != nil {
+		return err
+	}
+
+	err = cmd.Start()
+	if err != nil {
+		return err
+	}
+
+	err = addProcessToGroup(g, cmd.Process)
 	if err != nil {
 		return err
 	}
@@ -69,13 +117,12 @@ func (e *Cmd) runOSSpecific() error {
 
 	select {
 	case <-e.terminate:
-		// on Windows, it's not possible to send os.Interrupt to a process.
+		closeProcessGroup(g)
-		// Kill() is the only supported way.
-		cmd.Process.Kill()
 		<-cmdDone
 		return errTerminated
 
 	case c := <-cmdDone:
+		closeProcessGroup(g)
 		if c != 0 {
 			return fmt.Errorf("command exited with code %d", c)
 		}