diff --git a/.release-notes/3187.md b/.release-notes/3187.md new file mode 100644 index 0000000000..22a3624617 --- /dev/null +++ b/.release-notes/3187.md @@ -0,0 +1,3 @@ +## Clean up child process exits on Windows + +Fixed the `ProcessMonitor` class sometimes waiting on an invalid process handle on Windows. diff --git a/packages/process/_process.pony b/packages/process/_process.pony index 8b83639298..2984b6dbf7 100644 --- a/packages/process/_process.pony +++ b/packages/process/_process.pony @@ -347,6 +347,7 @@ primitive _WaitPidStatus class _ProcessWindows is _Process let hProcess: USize let processError: (ProcessError | None) + var finalWaitResult: (_WaitResult | None) = None new create( path: String, @@ -429,17 +430,27 @@ class _ProcessWindows is _Process end fun ref wait(): _WaitResult => - if hProcess != 0 then - var exit_code: I32 = 0 - match @ponyint_win_process_wait(hProcess, addressof exit_code) - | 0 => Exited(exit_code) - | 1 => _StillRunning - | let code: I32 => - // we might want to propagate that code to the user, but should it do - // for other errors too - WaitpidError - end + match finalWaitResult + | let wr: _WaitResult => + wr else - WaitpidError + var wr: _WaitResult = WaitpidError + if hProcess != 0 then + var exit_code: I32 = 0 + match @ponyint_win_process_wait(hProcess, addressof exit_code) + | 0 => + wr = Exited(exit_code) + finalWaitResult = wr + wr + | 1 => _StillRunning + | let code: I32 => + // we might want to propagate that code to the user, but should it do + // for other errors too + finalWaitResult = wr + wr + end + else + finalWaitResult = wr + wr + end end - diff --git a/packages/process/_test.pony b/packages/process/_test.pony index 8f53fbe372..85d629460f 100644 --- a/packages/process/_test.pony +++ b/packages/process/_test.pony @@ -25,6 +25,7 @@ actor Main is TestList test(_TestBadExec) test(_TestLongRunningChild) test(_TestKillLongRunningChild) + test(_TestWaitingOnClosedProcessTwice) class _PathResolver let _path: Array[FilePath] val @@ -677,6 +678,81 @@ class iso _TestKillLongRunningChild is UnitTest fun timed_out(h: TestHelper) => h.complete(false) +class iso _TestWaitingOnClosedProcessTwice is UnitTest + fun name(): String => "process/wait-on-closed-process-twice" + fun exclusion_group(): String => "process-monitor" + fun apply(h: TestHelper)? => + let auth = h.env.root as AmbientAuth + try + let path_resolver = _PathResolver(h.env.vars, auth) + let path = FilePath(auth, _SleepCommand.path(path_resolver)?)? + h.log(path.path) + let args = _SleepCommand.args(where seconds = 2) + let vars: Array[String] val = ["HOME=/"; "PATH=/bin"] + + // we want to make sure that ProcessMonitor is not checking the exit + // status of a process after it has already exited + // since it calls dispose() on its notifier after it has checked the + // exit status, we test whether or not we get more than 1 dispose() + + // we start a process that should take less than 3 seconds + // after 3 seconds we send 2 dispose() messages to it + // when we are notified of the first dispose() message, we check the exit + // status to make sure we've exited successfully, then we start a second + // timer that will check if we were notified of the 2nd dispose() + // we shouldn't have been + + let timers = Timers + + let pn = object iso is ProcessNotify + var n: USize = 0 + fun ref dispose(pm: ProcessMonitor ref, status: ProcessExitStatus) => + n = n + 1 + h.log("dispose() called " + n.string() + " time") + match status + | Exited(0) => + h.log("child exited") + else + h.fail("child did not exit") + h.complete(false) + return + end + + if n == 1 then + let timer2 = Timer( + object iso is TimerNotify + fun ref apply(timer: Timer, count: U64): Bool => + h.log("timer2 called; n == " + n.string()) + if n == 1 then + h.complete(true) + else + h.fail("notifier received dispose() more than once") + h.complete(false) + end + true + end, + 500_000_000, 0 + ) + timers(consume timer2) + end + end + let pm = ProcessMonitor(auth, auth, consume pn, path, args, vars) + + let timer1 = Timer( + object iso is TimerNotify + fun ref apply(timer: Timer, count: U64): Bool => + h.log("timer1 calling pm.dispose() twice") + pm.dispose() + pm.dispose() + true + end, + 3_000_000_000, 0) + timers(consume timer1) + + h.long_test(5_000_000_000) + else + h.fail("process monitor threw an error") + end class _ProcessClient is ProcessNotify """ diff --git a/packages/process/process_monitor.pony b/packages/process/process_monitor.pony index 011da802c3..96d91b281f 100644 --- a/packages/process/process_monitor.pony +++ b/packages/process/process_monitor.pony @@ -32,6 +32,9 @@ actor ProcessMonitor var _timers: (Timers tag | None) = None let _process_poll_interval: U64 + var _pollingChild: Bool = false + var _finalWaitResult: (_WaitResult | None) = None + new create( auth: ProcessMonitorAuth, backpressure_auth: BackpressureAuth, @@ -262,25 +265,35 @@ actor ProcessMonitor end be _wait_for_child() => - match _child.wait() - | _StillRunning => - let timers = _ensure_timers() - let pm: ProcessMonitor tag = this - let tn = - object iso is TimerNotify - fun ref apply(timer: Timer, count: U64): Bool => - pm._wait_for_child() - true + match _finalWaitResult + | let wr: _WaitResult => + None + else + match _child.wait() + | let sr: _StillRunning => + if not _pollingChild then + _pollingChild = true + let timers = _ensure_timers() + let pm: ProcessMonitor tag = this + let tn = + object iso is TimerNotify + fun ref apply(timer: Timer, count: U64): Bool => + pm._wait_for_child() + true + end + let timer = Timer(consume tn, _process_poll_interval, _process_poll_interval) + timers(consume timer) end - let timer = Timer(consume tn, _process_poll_interval, _process_poll_interval) - timers(consume timer) - | let exit_status: ProcessExitStatus => - // process child exit code or termination signal - _notifier.dispose(this, exit_status) - _dispose_timers() - | let wpe: WaitpidError => - _notifier.failed(this, ProcessError(WaitpidError)) - _dispose_timers() + | let exit_status: ProcessExitStatus => + // process child exit code or termination signal + _finalWaitResult = exit_status + _notifier.dispose(this, exit_status) + _dispose_timers() + | let wpe: WaitpidError => + _finalWaitResult = wpe + _notifier.failed(this, ProcessError(WaitpidError)) + _dispose_timers() + end end fun ref _ensure_timers(): Timers tag => diff --git a/src/libponyrt/lang/process.c b/src/libponyrt/lang/process.c index 20c05e52a3..4f6921b422 100644 --- a/src/libponyrt/lang/process.c +++ b/src/libponyrt/lang/process.c @@ -128,7 +128,8 @@ PONY_API int32_t ponyint_win_process_wait(size_t hProcess, int32_t* exit_code_pt int32_t retval = 0; // just poll - switch (WaitForSingleObject((HANDLE)hProcess, 0)) + DWORD result = WaitForSingleObject((HANDLE)hProcess, 0); + switch (result) { case WAIT_OBJECT_0: // process exited if (GetExitCodeProcess((HANDLE)hProcess, (DWORD*)exit_code_ptr) == 0) @@ -144,6 +145,8 @@ PONY_API int32_t ponyint_win_process_wait(size_t hProcess, int32_t* exit_code_pt retval = GetLastError(); if (retval == 0) retval = -1; break; + default: + break; } CloseHandle((HANDLE)hProcess);