From d5b9f47765ec8acc26bbe8e68732ec70cc117934 Mon Sep 17 00:00:00 2001 From: Lachlan Donald Date: Thu, 3 Jan 2019 11:38:15 +1100 Subject: [PATCH 1/2] Use windows console groups and ctrl-c for signal handling --- process/process_test.go | 5 ---- process/utils_windows.go | 51 +++++++++++++++++++++++++++------------- 2 files changed, 35 insertions(+), 21 deletions(-) diff --git a/process/process_test.go b/process/process_test.go index 546008ac87..88586dcded 100644 --- a/process/process_test.go +++ b/process/process_test.go @@ -5,7 +5,6 @@ import ( "os" "os/signal" "reflect" - "runtime" "strings" "sync" "sync/atomic" @@ -93,10 +92,6 @@ func TestProcessCapturesOutputLineByLine(t *testing.T) { } func TestProcessInterrupts(t *testing.T) { - if runtime.GOOS == `windows` { - t.Skip("Not supported on windows") - } - var lines []string var mu sync.Mutex diff --git a/process/utils_windows.go b/process/utils_windows.go index 16484eee65..bb41e40c9a 100644 --- a/process/utils_windows.go +++ b/process/utils_windows.go @@ -5,39 +5,58 @@ import ( "os" "os/exec" "strconv" + "syscall" "github.com/buildkite/agent/logger" ) -// Windows signal handling isn't a thing unfortunately, so we are limited in what we -// can do with gracefully terminating. Windows also doesn't have process hierarchies -// in the same way that unix does, so killing a process doesn't have any effect on -// the processes it spawned. We use TASKKILL.EXE with the /T flag to traverse processes -// that have a matching ParentProcessID, but even then if a process in the middle -// of the chain has died, we leave a heap of processes hanging around. Future improvements -// might include using terminal groups or job groups, which are a modern windows thing -// that might work for us. +// Windows has no concept of parent/child processes or signals. The best we can do +// is create processes inside a "console group" and then send break / ctrl-c events +// to that group. This is superior to walking a process tree to kill each process +// because that relies on each process in that chain still being active. + +// See https://docs.microsoft.com/en-us/windows/console/generateconsolectrlevent + +var ( + libkernel32 = syscall.MustLoadDLL("kernel32") + procSetConsoleCtrlHandler = libkernel32.MustFindProc("SetConsoleCtrlHandler") + procGenerateConsoleCtrlEvent = libkernel32.MustFindProc("GenerateConsoleCtrlEvent") +) + +const ( + createNewProcessGroupFlag = 0x00000200 +) func StartPTY(c *exec.Cmd) (*os.File, error) { return nil, errors.New("PTY is not supported on Windows") } -func createCommand(name string, arg ...string) *exec.Cmd { - return exec.Command(name, arg...) +func createCommand(cmd string, args ...string) *exec.Cmd { + execCmd := exec.Command(cmd, args...) + execCmd.SysProcAttr = &syscall.SysProcAttr{ + CreationFlags: syscall.CREATE_UNICODE_ENVIRONMENT | createNewProcessGroupFlag, + } + return execCmd } func terminateProcess(p *os.Process, l *logger.Logger) error { l.Debug("[Process] Terminating process tree with TASKKILL.EXE PID: %d", p.Pid) // taskkill.exe with /F will call TerminateProcess and hard-kill the process and - // anything in it's process tree. + // anything left in it's process tree. return exec.Command("CMD", "/C", "TASKKILL.EXE", "/F", "/T", "/PID", strconv.Itoa(p.Pid)).Run() } func interruptProcess(p *os.Process, l *logger.Logger) error { - l.Debug("[Process] Interrupting process tree with TASKKILL.EXE PID: %d", p.Pid) - - // taskkill.exe without the /F will use window signalling (WM_STOP, WM_QUIT) to kind - // of gracefull shutdown windows things that support it. - return exec.Command("CMD", "/C", "TASKKILL.EXE", "/T", "/PID", strconv.Itoa(p.Pid)).Run() + procSetConsoleCtrlHandler.Call(0, 1) + defer procSetConsoleCtrlHandler.Call(0, 0) + r1, _, err := procGenerateConsoleCtrlEvent.Call(syscall.CTRL_BREAK_EVENT, uintptr(p.Pid)) + if r1 == 0 { + return err + } + r1, _, err = procGenerateConsoleCtrlEvent.Call(syscall.CTRL_C_EVENT, uintptr(p.Pid)) + if r1 == 0 { + return err + } + return nil } From a6eb9db621488bacc4d6ca73eae72ee538b750f5 Mon Sep 17 00:00:00 2001 From: Lachlan Donald Date: Thu, 3 Jan 2019 15:40:55 +1100 Subject: [PATCH 2/2] Skip test that fails in docker --- process/process_test.go | 5 +++++ process/utils_windows.go | 9 ++------- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/process/process_test.go b/process/process_test.go index 88586dcded..dc1782a66e 100644 --- a/process/process_test.go +++ b/process/process_test.go @@ -5,6 +5,7 @@ import ( "os" "os/signal" "reflect" + "runtime" "strings" "sync" "sync/atomic" @@ -92,6 +93,10 @@ func TestProcessCapturesOutputLineByLine(t *testing.T) { } func TestProcessInterrupts(t *testing.T) { + if runtime.GOOS == `windows` { + t.Skip("Works in windows, but not in docker") + } + var lines []string var mu sync.Mutex diff --git a/process/utils_windows.go b/process/utils_windows.go index bb41e40c9a..050f5d435a 100644 --- a/process/utils_windows.go +++ b/process/utils_windows.go @@ -19,7 +19,6 @@ import ( var ( libkernel32 = syscall.MustLoadDLL("kernel32") - procSetConsoleCtrlHandler = libkernel32.MustFindProc("SetConsoleCtrlHandler") procGenerateConsoleCtrlEvent = libkernel32.MustFindProc("GenerateConsoleCtrlEvent") ) @@ -48,15 +47,11 @@ func terminateProcess(p *os.Process, l *logger.Logger) error { } func interruptProcess(p *os.Process, l *logger.Logger) error { - procSetConsoleCtrlHandler.Call(0, 1) - defer procSetConsoleCtrlHandler.Call(0, 0) + // Sends a CTRL-BREAK signal to the process group id, which is the same as the process PID + // For some reason I cannot fathom, this returns "Incorrect function" in docker for windows r1, _, err := procGenerateConsoleCtrlEvent.Call(syscall.CTRL_BREAK_EVENT, uintptr(p.Pid)) if r1 == 0 { return err } - r1, _, err = procGenerateConsoleCtrlEvent.Call(syscall.CTRL_C_EVENT, uintptr(p.Pid)) - if r1 == 0 { - return err - } return nil }