From 979d71c883ee76c5e1246c5ad02f2791b745bef6 Mon Sep 17 00:00:00 2001
From: Bert Belder <bertbelder@gmail.com>
Date: Wed, 13 Jan 2021 20:52:12 -0800
Subject: [PATCH] refactor: make Process#kill() throw sensible errors on
 Windows (#9111)

Previously, calling `Process#kill()` after the process had exited would
sometimes throw a `TypeError` on Windows. After this patch, it will
throw `NotFound` instead, just like other platforms.

This patch also fixes flakiness of the `runKillAfterStatus` test on
Windows.
---
 cli/tests/unit/process_test.ts | 24 +++++++-----
 runtime/ops/process.rs         | 72 ++++++++++++++++------------------
 2 files changed, 48 insertions(+), 48 deletions(-)

diff --git a/cli/tests/unit/process_test.ts b/cli/tests/unit/process_test.ts
index 1eaa3c3b3e..9bb4d7fc20 100644
--- a/cli/tests/unit/process_test.ts
+++ b/cli/tests/unit/process_test.ts
@@ -434,16 +434,20 @@ unitTest(
     });
     await p.status();
 
-    // On Windows the underlying Rust API returns `ERROR_ACCESS_DENIED`,
-    // which serves kind of as a catch all error code. More specific
-    // error codes do exist, e.g. `ERROR_WAIT_NO_CHILDREN`; it's unclear
-    // why they're not returned.
-    const expectedErrorType = Deno.build.os === "windows"
-      ? Deno.errors.PermissionDenied
-      : Deno.errors.NotFound;
-    assertThrows(
-      () => p.kill(Deno.Signal.SIGTERM),
-      expectedErrorType,
+    let error = null;
+    try {
+      p.kill(Deno.Signal.SIGTERM);
+    } catch (e) {
+      error = e;
+    }
+
+    assert(
+      error instanceof Deno.errors.NotFound ||
+        // On Windows, the underlying Windows API may return
+        // `ERROR_ACCESS_DENIED` when the process has exited, but hasn't been
+        // completely cleaned up yet and its `pid` is still valid.
+        (Deno.build.os === "windows" &&
+          error instanceof Deno.errors.PermissionDenied),
     );
 
     p.close();
diff --git a/runtime/ops/process.rs b/runtime/ops/process.rs
index 63e22b6018..412d21ef2a 100644
--- a/runtime/ops/process.rs
+++ b/runtime/ops/process.rs
@@ -219,23 +219,6 @@ async fn op_run_status(
   }))
 }
 
-#[cfg(not(unix))]
-const SIGINT: i32 = 2;
-#[cfg(not(unix))]
-const SIGKILL: i32 = 9;
-#[cfg(not(unix))]
-const SIGTERM: i32 = 15;
-
-#[cfg(not(unix))]
-use winapi::{
-  shared::minwindef::DWORD,
-  um::{
-    handleapi::CloseHandle,
-    processthreadsapi::{OpenProcess, TerminateProcess},
-    winnt::PROCESS_TERMINATE,
-  },
-};
-
 #[cfg(unix)]
 pub fn kill(pid: i32, signo: i32) -> Result<(), AnyError> {
   use nix::sys::signal::{kill as unix_kill, Signal};
@@ -248,30 +231,43 @@ pub fn kill(pid: i32, signo: i32) -> Result<(), AnyError> {
 #[cfg(not(unix))]
 pub fn kill(pid: i32, signal: i32) -> Result<(), AnyError> {
   use std::io::Error;
-  match signal {
-    SIGINT | SIGKILL | SIGTERM => {
-      if pid <= 0 {
-        return Err(type_error("unsupported pid"));
+  use std::io::ErrorKind::NotFound;
+  use winapi::shared::minwindef::DWORD;
+  use winapi::shared::minwindef::FALSE;
+  use winapi::shared::minwindef::TRUE;
+  use winapi::shared::winerror::ERROR_INVALID_PARAMETER;
+  use winapi::um::errhandlingapi::GetLastError;
+  use winapi::um::handleapi::CloseHandle;
+  use winapi::um::processthreadsapi::OpenProcess;
+  use winapi::um::processthreadsapi::TerminateProcess;
+  use winapi::um::winnt::PROCESS_TERMINATE;
+
+  const SIGINT: i32 = 2;
+  const SIGKILL: i32 = 9;
+  const SIGTERM: i32 = 15;
+
+  if !matches!(signal, SIGINT | SIGKILL | SIGTERM) {
+    Err(type_error("unsupported signal"))
+  } else if pid <= 0 {
+    Err(type_error("unsupported pid"))
+  } else {
+    let handle = unsafe { OpenProcess(PROCESS_TERMINATE, FALSE, pid as DWORD) };
+    if handle.is_null() {
+      let err = match unsafe { GetLastError() } {
+        ERROR_INVALID_PARAMETER => Error::from(NotFound), // Invalid `pid`.
+        errno => Error::from_raw_os_error(errno as i32),
+      };
+      Err(err.into())
+    } else {
+      let r = unsafe { TerminateProcess(handle, 1) };
+      unsafe { CloseHandle(handle) };
+      match r {
+        FALSE => Err(Error::last_os_error().into()),
+        TRUE => Ok(()),
+        _ => unreachable!(),
       }
-      unsafe {
-        let handle = OpenProcess(PROCESS_TERMINATE, 0, pid as DWORD);
-        if handle.is_null() {
-          return Err(Error::last_os_error().into());
-        }
-        if TerminateProcess(handle, 1) == 0 {
-          CloseHandle(handle);
-          return Err(Error::last_os_error().into());
-        }
-        if CloseHandle(handle) == 0 {
-          return Err(Error::last_os_error().into());
-        }
-      }
-    }
-    _ => {
-      return Err(type_error("unsupported signal"));
     }
   }
-  Ok(())
 }
 
 #[derive(Deserialize)]