fix: subprocess kill support on windows (#12134)

cli/tests/unit/process_test.ts

@@ -114,8 +114,6 @@ unitTest(
 
 unitTest(
   {
-    // No signals on windows.
-    ignore: Deno.build.os === "windows",
     permissions: { run: true, read: true },
   },
   async function runCommandFailedWithSignal() {
@@ -129,8 +127,13 @@ unitTest(
     });
     const status = await p.status();
     assertEquals(status.success, false);
-    assertEquals(status.code, 128 + 9);
-    assertEquals(status.signal, 9);
+    if (Deno.build.os === "windows") {
+      assertEquals(status.code, 1);
+      assertEquals(status.signal, undefined);
+    } else {
+      assertEquals(status.code, 128 + 9);
+      assertEquals(status.signal, 9);
+    }
     p.close();
   },
 );
@@ -448,9 +451,11 @@ unitTest(
 
     assert(
       error instanceof Deno.errors.NotFound ||
-        // This is not yet implemented on Windows
+        // 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 Error && error.message === "not implemented"),
+          error instanceof Deno.errors.PermissionDenied),
     );
 
     p.close();
@@ -467,6 +472,24 @@ unitTest(function killPermissions() {
   }, Deno.errors.PermissionDenied);
 });
 
+unitTest(
+  { ignore: Deno.build.os !== "windows", permissions: { run: true } },
+  function negativePidInvalidWindows() {
+    assertThrows(() => {
+      Deno.kill(-1, "SIGINT");
+    }, TypeError);
+  },
+);
+
+unitTest(
+  { ignore: Deno.build.os !== "windows", permissions: { run: true } },
+  function invalidSignalNameWindows() {
+    assertThrows(() => {
+      Deno.kill(Deno.pid, "SIGUSR1");
+    }, TypeError);
+  },
+);
+
 unitTest(
   { permissions: { run: true, read: true } },
   async function killSuccess() {
@@ -475,16 +498,14 @@ unitTest(
     });
 
     try {
-      if (Deno.build.os === "windows") {
-        // currently not implemented
-        assertThrows(() => {
-          Deno.kill(p.pid, "SIGINT");
-        }, Error);
-      } else {
-        Deno.kill(p.pid, "SIGINT");
-        const status = await p.status();
+      Deno.kill(p.pid, "SIGINT");
+      const status = await p.status();
 
-        assertEquals(status.success, false);
+      assertEquals(status.success, false);
+      if (Deno.build.os === "windows") {
+        assertEquals(status.code, 1);
+        assertEquals(status.signal, undefined);
+      } else {
         assertEquals(status.code, 130);
         assertEquals(status.signal, 2);
       }
@@ -501,11 +522,10 @@ unitTest({ permissions: { run: true, read: true } }, function killFailed() {
   assert(!p.stdin);
   assert(!p.stdout);
 
-  // windows is currently not implemented so it throws a regular Error saying so
   assertThrows(() => {
     // @ts-expect-error testing runtime error of bad signal
     Deno.kill(p.pid, "foobar");
-  }, Deno.build.os === "windows" ? Error : TypeError);
+  }, TypeError);
 
   p.close();
 });

runtime/ops/process.rs

@@ -257,7 +257,8 @@ async fn op_run_status(
 }
 
 #[cfg(unix)]
-pub fn kill(pid: i32, signo: i32) -> Result<(), AnyError> {
+pub fn kill(pid: i32, signal: &str) -> Result<(), AnyError> {
+  let signo = super::signal::signal_str_to_int(signal)?;
   use nix::sys::signal::{kill as unix_kill, Signal};
   use nix::unistd::Pid;
   use std::convert::TryFrom;
@@ -266,7 +267,7 @@ pub fn kill(pid: i32, signo: i32) -> Result<(), AnyError> {
 }
 
 #[cfg(not(unix))]
-pub fn kill(pid: i32, signal: i32) -> Result<(), AnyError> {
+pub fn kill(pid: i32, signal: &str) -> Result<(), AnyError> {
   use std::io::Error;
   use std::io::ErrorKind::NotFound;
   use winapi::shared::minwindef::DWORD;
@@ -279,14 +280,10 @@ pub fn kill(pid: i32, signal: i32) -> Result<(), AnyError> {
   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"))
+  if !matches!(signal, "SIGINT" | "SIGKILL" | "SIGTERM") {
+    Err(type_error(format!("Invalid signal: {}", signal)))
   } else if pid <= 0 {
-    Err(type_error("unsupported pid"))
+    Err(type_error("Invalid pid"))
   } else {
     let handle = unsafe { OpenProcess(PROCESS_TERMINATE, FALSE, pid as DWORD) };
     if handle.is_null() {
@@ -310,12 +307,10 @@ pub fn kill(pid: i32, signal: i32) -> Result<(), AnyError> {
 fn op_kill(
   state: &mut OpState,
   pid: i32,
-  signo: String,
+  signal: String,
 ) -> Result<(), AnyError> {
   super::check_unstable(state, "Deno.kill");
   state.borrow_mut::<Permissions>().run.check_all()?;
-
-  let signo = super::signal::signal_str_to_int(&signo)?;
-  kill(pid, signo)?;
+  kill(pid, &signal)?;
   Ok(())
 }

runtime/ops/signal.rs

@@ -169,15 +169,10 @@ pub fn signal_str_to_int(s: &str) -> Result<libc::c_int, AnyError> {
     "SIGINFO" => Ok(29),
     "SIGUSR1" => Ok(30),
     "SIGUSR2" => Ok(31),
-    _ => Err(type_error(format!("Invalid signal : {}", s))),
+    _ => Err(type_error(format!("Invalid signal: {}", s))),
   }
 }
 
-#[cfg(target_os = "windows")]
-pub fn signal_str_to_int(_s: &str) -> Result<libc::c_int, AnyError> {
-  Err(generic_error("not implemented"))
-}
-
 #[cfg(unix)]
 fn op_signal_bind(
   state: &mut OpState,