From 59d45305629e54b8cc9a1f2e0991609363d792a4 Mon Sep 17 00:00:00 2001 From: cheatfate Date: Mon, 11 Dec 2017 21:12:07 +0200 Subject: [PATCH 1/4] Remove `-3` as marker of exited process. Cache exiting process for Windows to omit unnecessary syscalls. Fix closing hThread for Windows. Fix for pause/resume on Windows. Fix process handle leak on Windows. Change behavior for waitForExit on Windows. --- lib/pure/osproc.nim | 116 ++++++++++++++++++++++++++-------------- lib/windows/winlean.nim | 1 + 2 files changed, 77 insertions(+), 40 deletions(-) diff --git a/lib/pure/osproc.nim b/lib/pure/osproc.nim index 2a1ce0c58..f762a713b 100644 --- a/lib/pure/osproc.nim +++ b/lib/pure/osproc.nim @@ -47,6 +47,7 @@ type ProcessObj = object of RootObj when defined(windows): fProcessHandle: Handle + fThreadHandle: Handle inHandle, outHandle, errHandle: FileHandle id: Handle else: @@ -54,6 +55,7 @@ type inStream, outStream, errStream: Stream id: Pid exitStatus: cint + exitFlag: bool options: set[ProcessOption] Process* = ref ProcessObj ## represents an operating system process @@ -237,11 +239,13 @@ proc execProcesses*(cmds: openArray[string], if n > 1: var i = 0 var q = newSeq[Process](n) - var m = min(n, cmds.len) when defined(windows): var w: WOHandleArray + var m = min(min(n, MAXIMUM_WAIT_OBJECTS), cmds.len) var wcount = m + else: + var m = min(n, cmds.len) while i < m: if beforeRunEvent != nil: @@ -262,8 +266,17 @@ proc execProcesses*(cmds: openArray[string], discard elif ret == WAIT_FAILED: raiseOSError(osLastError()) + else: + var status: int32 + for r in 0..m-1: + if not isNil(q[r]) and q[r].fProcessHandle == w[ret]: + discard getExitCodeProcess(q[r].fProcessHandle, status) + q[r].exitFlag = true + q[r].exitStatus = status + discard closeHandle(q[r].fProcessHandle) + break else: - var status : cint = 1 + var status: cint = 1 # waiting for all children, get result if any child exits let res = waitpid(-1, status, 0) if res > 0: @@ -271,6 +284,7 @@ proc execProcesses*(cmds: openArray[string], if not isNil(q[r]) and q[r].id == res: # we updating `exitStatus` manually, so `running()` can work. if WIFEXITED(status) or WIFSIGNALED(status): + q[r].exitFlag = true q[r].exitStatus = status break else: @@ -491,6 +505,7 @@ when defined(Windows) and not defined(useNimRtl): hi, ho, he: Handle new(result) result.options = options + result.exitFlag = true si.cb = sizeof(si).cint if poParentStreams notin options: si.dwFlags = STARTF_USESTDHANDLES # STARTF_USESHOWWINDOW or @@ -559,28 +574,31 @@ when defined(Windows) and not defined(useNimRtl): "Requested command not found: '$1'. OS error:" % command) else: raiseOSError(lastError, command) - # Close the handle now so anyone waiting is woken: - discard closeHandle(procInfo.hThread) + result.fProcessHandle = procInfo.hProcess + result.fThreadHandle = procInfo.hThread result.id = procInfo.dwProcessId + result.exitFlag = false proc close(p: Process) = if poInteractive in p.options: - # somehow this is not always required on Windows: discard closeHandle(p.inHandle) discard closeHandle(p.outHandle) discard closeHandle(p.errHandle) - #discard closeHandle(p.FProcessHandle) + discard closeHandle(p.fProcessHandle) proc suspend(p: Process) = - discard suspendThread(p.fProcessHandle) + discard suspendThread(p.fThreadHandle) proc resume(p: Process) = - discard resumeThread(p.fProcessHandle) + discard resumeThread(p.fThreadHandle) proc running(p: Process): bool = - var x = waitForSingleObject(p.fProcessHandle, 50) - return x == WAIT_TIMEOUT + if p.exitFlag: + return false + else: + var x = waitForSingleObject(p.fProcessHandle, 0) + return x == WAIT_TIMEOUT proc terminate(p: Process) = if running(p): @@ -590,22 +608,35 @@ when defined(Windows) and not defined(useNimRtl): terminate(p) proc waitForExit(p: Process, timeout: int = -1): int = - discard waitForSingleObject(p.fProcessHandle, timeout.int32) + if p.exitFlag: + return p.exitStatus - var res: int32 - discard getExitCodeProcess(p.fProcessHandle, res) - result = res - p.exitStatus = res - discard closeHandle(p.fProcessHandle) + let res = waitForSingleObject(p.fProcessHandle, timeout.int32) + if res == WAIT_TIMEOUT: + terminate(p) + var status: int32 + discard getExitCodeProcess(p.fProcessHandle, status) + if status != STILL_ACTIVE: + p.exitFlag = true + p.exitStatus = status + discard closeHandle(p.fProcessHandle) + result = status + else: + result = -1 proc peekExitCode(p: Process): int = - var b = waitForSingleObject(p.fProcessHandle, 50) == WAIT_TIMEOUT - if b: result = -1 - else: - var res: int32 - discard getExitCodeProcess(p.fProcessHandle, res) - if res == 0: return p.exitStatus - return res + if p.exitFlag: + return p.exitStatus + + result = -1 + var b = waitForSingleObject(p.fProcessHandle, 0) == WAIT_TIMEOUT + if not b: + var status: int32 + discard getExitCodeProcess(p.fProcessHandle, status) + p.exitFlag = true + p.exitStatus = status + discard closeHandle(p.fProcessHandle) + result = status proc inputStream(p: Process): Stream = streamAccess(p) @@ -737,7 +768,8 @@ elif not defined(useNimRtl): pStdin, pStdout, pStderr: array[0..1, cint] new(result) result.options = options - result.exitStatus = -3 # for ``waitForExit`` + result.exitFlag = true + if poParentStreams notin options: if pipe(pStdin) != 0'i32 or pipe(pStdout) != 0'i32 or pipe(pStderr) != 0'i32: @@ -792,6 +824,7 @@ elif not defined(useNimRtl): if poEchoCmd in options: echo(command, " ", join(args, " ")) result.id = pid + result.exitFlag = false if poParentStreams in options: # does not make much sense, but better than nothing: @@ -968,14 +1001,14 @@ elif not defined(useNimRtl): if kill(p.id, SIGCONT) != 0'i32: raiseOsError(osLastError()) proc running(p: Process): bool = - if p.exitStatus != -3: + if p.exitFlag: return false else: - var ret : int - var status : cint = 1 - ret = waitpid(p.id, status, WNOHANG) + var status: cint = 1 + let ret = waitpid(p.id, status, WNOHANG) if ret == int(p.id): if isExitStatus(status): + p.exitFlag = true p.exitStatus = status return false else: @@ -998,13 +1031,14 @@ elif not defined(useNimRtl): import kqueue, times proc waitForExit(p: Process, timeout: int = -1): int = - if p.exitStatus != -3: + if p.exitFlag: return exitStatus(p.exitStatus) if timeout == -1: - var status : cint = 1 + var status: cint = 1 if waitpid(p.id, status, 0) < 0: raiseOSError(osLastError()) + p.exitFlag = true p.exitStatus = status else: var kqFD = kqueue() @@ -1025,7 +1059,7 @@ elif not defined(useNimRtl): try: while true: - var status : cint = 1 + var status: cint = 1 var count = kevent(kqFD, addr(kevIn), 1, addr(kevOut), 1, addr(tmspec)) if count < 0: @@ -1038,12 +1072,14 @@ elif not defined(useNimRtl): raiseOSError(osLastError()) if waitpid(p.id, status, 0) < 0: raiseOSError(osLastError()) + p.exitFlag = true p.exitStatus = status break else: if kevOut.ident == p.id.uint and kevOut.filter == EVFILT_PROC: if waitpid(p.id, status, 0) < 0: raiseOSError(osLastError()) + p.exitFlag = true p.exitStatus = status break else: @@ -1083,17 +1119,14 @@ elif not defined(useNimRtl): s.tv_sec = b.tv_sec s.tv_nsec = b.tv_nsec - #if waitPid(p.id, p.exitStatus, 0) == int(p.id): - # ``waitPid`` fails if the process is not running anymore. But then - # ``running`` probably set ``p.exitStatus`` for us. Since ``p.exitStatus`` is - # initialized with -3, wrong success exit codes are prevented. - if p.exitStatus != -3: + if p.exitFlag: return exitStatus(p.exitStatus) if timeout == -1: - var status : cint = 1 + var status: cint = 1 if waitpid(p.id, status, 0) < 0: raiseOSError(osLastError()) + p.exitFlag = true p.exitStatus = status else: var nmask, omask: Sigset @@ -1125,9 +1158,10 @@ elif not defined(useNimRtl): let res = sigtimedwait(nmask, sinfo, tmspec) if res == SIGCHLD: if sinfo.si_pid == p.id: - var status : cint = 1 + var status: cint = 1 if waitpid(p.id, status, 0) < 0: raiseOSError(osLastError()) + p.exitFlag = true p.exitStatus = status break else: @@ -1148,9 +1182,10 @@ elif not defined(useNimRtl): # timeout expired, so we trying to kill process if posix.kill(p.id, SIGKILL) == -1: raiseOSError(osLastError()) - var status : cint = 1 + var status: cint = 1 if waitpid(p.id, status, 0) < 0: raiseOSError(osLastError()) + p.exitFlag = true p.exitStatus = status break else: @@ -1168,12 +1203,13 @@ elif not defined(useNimRtl): proc peekExitCode(p: Process): int = var status = cint(0) result = -1 - if p.exitStatus != -3: + if p.exitFlag: return exitStatus(p.exitStatus) var ret = waitpid(p.id, status, WNOHANG) if ret > 0: if isExitStatus(status): + p.exitFlag = true p.exitStatus = status result = exitStatus(status) diff --git a/lib/windows/winlean.nim b/lib/windows/winlean.nim index 7eb268a9a..a833377e5 100644 --- a/lib/windows/winlean.nim +++ b/lib/windows/winlean.nim @@ -111,6 +111,7 @@ const WAIT_TIMEOUT* = 0x00000102'i32 WAIT_FAILED* = 0xFFFFFFFF'i32 INFINITE* = -1'i32 + STILL_ACTIVE* = 0x00000103'i32 STD_INPUT_HANDLE* = -10'i32 STD_OUTPUT_HANDLE* = -11'i32 From e6722498595eabf6ef0ae314ad099c4108fde346 Mon Sep 17 00:00:00 2001 From: cheatfate Date: Tue, 12 Dec 2017 16:53:09 +0200 Subject: [PATCH 2/4] Windows: Fix invalid handle value for `execProcesses`. Windows. Fix named pipes leak. --- lib/pure/osproc.nim | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/lib/pure/osproc.nim b/lib/pure/osproc.nim index f762a713b..f61045663 100644 --- a/lib/pure/osproc.nim +++ b/lib/pure/osproc.nim @@ -273,7 +273,6 @@ proc execProcesses*(cmds: openArray[string], discard getExitCodeProcess(q[r].fProcessHandle, status) q[r].exitFlag = true q[r].exitStatus = status - discard closeHandle(q[r].fProcessHandle) break else: var status: cint = 1 @@ -313,11 +312,14 @@ proc execProcesses*(cmds: openArray[string], w[r] = q[r].fProcessHandle inc(i) else: - q[r] = nil when defined(windows): - for c in r..MAXIMUM_WAIT_OBJECTS - 2: - w[c] = w[c + 1] - dec(wcount) + for k in 0..wcount - 1: + if w[k] == q[r].fProcessHandle: + w[k] = w[wcount - 1] + w[wcount - 1] = 0 + dec(wcount) + break + q[r] = nil dec(ecount) else: for i in 0..high(cmds): @@ -574,17 +576,17 @@ when defined(Windows) and not defined(useNimRtl): "Requested command not found: '$1'. OS error:" % command) else: raiseOSError(lastError, command) - result.fProcessHandle = procInfo.hProcess result.fThreadHandle = procInfo.hThread result.id = procInfo.dwProcessId result.exitFlag = false proc close(p: Process) = - if poInteractive in p.options: + if poParentStreams notin p.options: discard closeHandle(p.inHandle) discard closeHandle(p.outHandle) discard closeHandle(p.errHandle) + discard closeHandle(p.fThreadHandle) discard closeHandle(p.fProcessHandle) proc suspend(p: Process) = @@ -619,6 +621,7 @@ when defined(Windows) and not defined(useNimRtl): if status != STILL_ACTIVE: p.exitFlag = true p.exitStatus = status + discard closeHandle(p.fThreadHandle) discard closeHandle(p.fProcessHandle) result = status else: @@ -635,6 +638,7 @@ when defined(Windows) and not defined(useNimRtl): discard getExitCodeProcess(p.fProcessHandle, status) p.exitFlag = true p.exitStatus = status + discard closeHandle(p.fThreadHandle) discard closeHandle(p.fProcessHandle) result = status From 0429f41e9862f7b6c1570ccbcd4cce541767fdad Mon Sep 17 00:00:00 2001 From: cheatfate Date: Tue, 12 Dec 2017 20:00:14 +0200 Subject: [PATCH 3/4] execProcesses optimization. --- lib/pure/osproc.nim | 53 +++++++++++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 24 deletions(-) diff --git a/lib/pure/osproc.nim b/lib/pure/osproc.nim index f61045663..d72ed1772 100644 --- a/lib/pure/osproc.nim +++ b/lib/pure/osproc.nim @@ -257,6 +257,7 @@ proc execProcesses*(cmds: openArray[string], var ecount = len(cmds) while ecount > 0: + var rexit = -1 when defined(windows): # waiting for all children, get result if any child exits var ret = waitForMultipleObjects(int32(wcount), addr(w), 0'i32, @@ -273,6 +274,7 @@ proc execProcesses*(cmds: openArray[string], discard getExitCodeProcess(q[r].fProcessHandle, status) q[r].exitFlag = true q[r].exitStatus = status + rexit = r break else: var status: cint = 1 @@ -281,16 +283,21 @@ proc execProcesses*(cmds: openArray[string], if res > 0: for r in 0..m-1: if not isNil(q[r]) and q[r].id == res: - # we updating `exitStatus` manually, so `running()` can work. if WIFEXITED(status) or WIFSIGNALED(status): q[r].exitFlag = true q[r].exitStatus = status + rexit = r break else: let err = osLastError() if err == OSErrorCode(ECHILD): # some child exits, we need to check our childs exit codes - discard + for r in 0..m-1: + if (not isNil(q[r])) and (not running(q[r])): + q[r].exitFlag = true + q[r].exitStatus = status + rexit = r + break elif err == OSErrorCode(EINTR): # signal interrupted our syscall, lets repeat it continue @@ -298,29 +305,27 @@ proc execProcesses*(cmds: openArray[string], # all other errors are exceptions raiseOSError(err) - for r in 0..m-1: - if not isNil(q[r]): - if not running(q[r]): - result = max(result, q[r].peekExitCode()) - if afterRunEvent != nil: afterRunEvent(r, q[r]) - close(q[r]) - if i < len(cmds): - if beforeRunEvent != nil: beforeRunEvent(i) - q[r] = startProcess(cmds[i], + if rexit >= 0: + result = max(result, q[rexit].peekExitCode()) + if afterRunEvent != nil: afterRunEvent(rexit, q[rexit]) + close(q[rexit]) + if i < len(cmds): + if beforeRunEvent != nil: beforeRunEvent(i) + q[rexit] = startProcess(cmds[i], options = options + {poEvalCommand}) - when defined(windows): - w[r] = q[r].fProcessHandle - inc(i) - else: - when defined(windows): - for k in 0..wcount - 1: - if w[k] == q[r].fProcessHandle: - w[k] = w[wcount - 1] - w[wcount - 1] = 0 - dec(wcount) - break - q[r] = nil - dec(ecount) + when defined(windows): + w[rexit] = q[rexit].fProcessHandle + inc(i) + else: + when defined(windows): + for k in 0..wcount - 1: + if w[k] == q[rexit].fProcessHandle: + w[k] = w[wcount - 1] + w[wcount - 1] = 0 + dec(wcount) + break + q[rexit] = nil + dec(ecount) else: for i in 0..high(cmds): if beforeRunEvent != nil: From e952ada1ba81675b0f6d22e76012afe290b4356c Mon Sep 17 00:00:00 2001 From: cheatfate Date: Wed, 13 Dec 2017 00:36:14 +0200 Subject: [PATCH 4/4] Fix --- lib/pure/osproc.nim | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pure/osproc.nim b/lib/pure/osproc.nim index d72ed1772..f0542ea98 100644 --- a/lib/pure/osproc.nim +++ b/lib/pure/osproc.nim @@ -315,7 +315,7 @@ proc execProcesses*(cmds: openArray[string], options = options + {poEvalCommand}) when defined(windows): w[rexit] = q[rexit].fProcessHandle - inc(i) + inc(i) else: when defined(windows): for k in 0..wcount - 1: