Clean up child process exits on Windows

.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.

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
-

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
   """

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 =>

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);