feat(node/child_process): support signal parameter for ChildProcess.kill

node/_tools/config.json

@@ -134,6 +134,7 @@
       "test-buffer-zero-fill.js",
       "test-child-process-execfile.js",
       "test-child-process-fork.js",
+      "test-child-process-kill.js",
       "test-client-request-destroy.js",
       "test-console-async-write-error.js",
       "test-console-group.js",
@@ -473,6 +474,7 @@
     "parallel": [
       "test-child-process-execfile.js",
       "test-child-process-fork.js",
+      "test-child-process-kill.js",
       "test-console-log-throw-primitive.js",
       "test-console-no-swallow-stack-overflow.js",
       "test-console-sync-write-error.js",

node/_tools/test/parallel/test-child-process-kill.js

@@ -0,0 +1,48 @@
+// deno-fmt-ignore-file
+// deno-lint-ignore-file
+
+// Copyright Joyent and Node contributors. All rights reserved. MIT license.
+// Taken from Node 16.13.0
+// This file is automatically generated by "node/_tools/setup.ts". Do not modify this file manually
+
+// Copyright Joyent, Inc. and other Node contributors.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to permit
+// persons to whom the Software is furnished to do so, subject to the
+// following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const spawn = require('child_process').spawn;
+const cat = spawn(common.isWindows ? 'cmd' : 'cat');
+
+cat.stdout.on('end', common.mustCall());
+cat.stderr.on('data', common.mustNotCall());
+cat.stderr.on('end', common.mustCall());
+
+cat.on('exit', common.mustCall((code, signal) => {
+  assert.strictEqual(code, null);
+  assert.strictEqual(signal, 'SIGTERM');
+  assert.strictEqual(cat.signalCode, 'SIGTERM');
+}));
+
+assert.strictEqual(cat.signalCode, null);
+assert.strictEqual(cat.killed, false);
+cat.kill();
+assert.strictEqual(cat.killed, true);

node/child_process_test.ts

@@ -3,6 +3,7 @@
 import {
   assert,
   assertEquals,
+  assertExists,
   assertNotStrictEquals,
   assertStrictEquals,
 } from "../testing/asserts.ts";
@@ -464,3 +465,21 @@ Deno.test({
     }
   },
 });
+
+Deno.test({
+  name: "[node/child_process] ChildProcess.kill()",
+  async fn() {
+    const script = path.join(
+      path.dirname(path.fromFileUrl(import.meta.url)),
+      "./testdata/infinite_loop.js",
+    );
+    const childProcess = spawn(Deno.execPath(), ["run", script]);
+    const p = withTimeout(3000);
+    childProcess.on("exit", () => p.resolve());
+    childProcess.kill("SIGKILL");
+    await p;
+    assert(childProcess.killed);
+    assertEquals(childProcess.signalCode, "SIGKILL");
+    assertExists(childProcess.exitCode);
+  },
+});

node/internal/child_process.ts

@@ -4,14 +4,19 @@
 // ref: https://nodejs.org/api/child_process.html
 import { assert } from "../../_util/assert.ts";
 import { EventEmitter } from "../events.ts";
+import { os } from "../internal_binding/constants.ts";
 import { notImplemented } from "../_utils.ts";
 import { Readable, Stream, Writable } from "../stream.ts";
 import { deferred } from "../../async/deferred.ts";
 import { iterateReader, writeAll } from "../../streams/conversion.ts";
 import { isWindows } from "../../_util/os.ts";
 import { Buffer } from "../buffer.ts";
 import { nextTick } from "../_next_tick.ts";
-import { AbortError, ERR_INVALID_ARG_VALUE } from "./errors.ts";
+import {
+  AbortError,
+  ERR_INVALID_ARG_VALUE,
+  ERR_UNKNOWN_SIGNAL,
+} from "./errors.ts";
 import { mapValues } from "../../collections/map_values.ts";
 
 type NodeStdio = "pipe" | "overlapped" | "ignore" | "inherit" | "ipc";
@@ -57,6 +62,11 @@ export class ChildProcess extends EventEmitter {
    */
   pid!: number;
 
+  /**
+   * The signal received by this child process.
+   */
+  signalCode: string | null = null;
+
   /**
    * Command line arguments given to this child process.
    */
@@ -160,7 +170,7 @@ export class ChildProcess extends EventEmitter {
       if (signal) {
         const onAbortListener = () => {
           try {
-            if (this.kill(/** TODO Pass killSignal (not implemented) */)) {
+            if (this.kill("SIGKILL")) {
               this.emit("error", new AbortError());
             }
           } catch (err) {
@@ -182,11 +192,13 @@ export class ChildProcess extends EventEmitter {
         const status = await this.#process.status();
         this.exitCode = status.code;
         this.#spawned.then(async () => {
+          const exitCode = this.signalCode == null ? this.exitCode : null;
+          const signalCode = this.signalCode == null ? null : this.signalCode;
           // The 'exit' and 'close' events must be emitted after the 'spawn' event.
-          this.emit("exit", this.exitCode, status.signal ?? null);
+          this.emit("exit", exitCode, signalCode);
           await this._waitForChildStreamsToClose();
-          this.kill();
-          this.emit("close", this.exitCode, status.signal ?? null);
+          this.#close();
+          this.emit("close", exitCode, signalCode);
         });
       })();
     } catch (err) {
@@ -198,26 +210,23 @@ export class ChildProcess extends EventEmitter {
    * @param signal NOTE: this parameter is not yet implemented.
    */
   kill(signal?: number | string): boolean {
-    if (signal !== undefined) {
-      notImplemented("`ChildProcess.kill()` with the `signal` parameter");
-    }
-
     if (this.killed) {
       return this.killed;
     }
 
-    if (this.#process.stdin) {
-      assert(this.stdin);
-      ensureClosed(this.#process.stdin);
-    }
-    if (this.#process.stdout) {
-      ensureClosed(this.#process.stdout);
-    }
-    if (this.#process.stderr) {
-      ensureClosed(this.#process.stderr);
+    const denoSignal = signal == null ? "SIGTERM" : toDenoSignal(signal);
+    this.#closePipes();
+    try {
+      this.#process.kill(denoSignal);
+    } catch (err) {
+      const alreadyClosed = err instanceof Deno.errors.NotFound ||
+        err instanceof Deno.errors.PermissionDenied;
+      if (!alreadyClosed) {
+        throw err;
+      }
     }
-    ensureClosed(this.#process); // TODO Use `Deno.Process.kill` instead when it becomes stable.
     this.killed = true;
+    this.signalCode = denoSignal;
     return this.killed;
   }
 
@@ -250,6 +259,24 @@ export class ChildProcess extends EventEmitter {
       this.emit("error", err); // TODO(uki00a) Convert `err` into nodejs's `SystemError` class.
     });
   }
+
+  #close(): void {
+    this.#closePipes();
+    ensureClosed(this.#process);
+  }
+
+  #closePipes(): void {
+    if (this.#process.stdin) {
+      assert(this.stdin);
+      ensureClosed(this.#process.stdin);
+    }
+    if (this.#process.stdout) {
+      ensureClosed(this.#process.stdout);
+    }
+    if (this.#process.stderr) {
+      ensureClosed(this.#process.stderr);
+    }
+  }
 }
 
 const supportedNodeStdioTypes: NodeStdio[] = ["pipe", "ignore", "inherit"];
@@ -276,6 +303,27 @@ function toDenoStdio(
   }
 }
 
+function toDenoSignal(signal: number | string): Deno.Signal {
+  if (typeof signal === "number") {
+    for (const name of keys(os.signals)) {
+      if (os.signals[name] === signal) {
+        return name as Deno.Signal;
+      }
+    }
+    throw new ERR_UNKNOWN_SIGNAL(String(signal));
+  }
+
+  const denoSignal = signal as Deno.Signal;
+  if (os.signals[denoSignal] != null) {
+    return denoSignal;
+  }
+  throw new ERR_UNKNOWN_SIGNAL(signal);
+}
+
+function keys<T extends Record<string, unknown>>(object: T): Array<keyof T> {
+  return Object.keys(object);
+}
+
 export interface ChildProcessOptions {
   /**
    * Current working directory of the child process.

node/internal_binding/constants.ts

@@ -120,6 +120,9 @@ export const os = {
     SIGIO: 23,
     SIGINFO: 29,
     SIGSYS: 12,
+    SIGEMT: 7,
+    SIGPWR: 30, // TODO(uki00a): In Solaris, this should be `19`. Is there any way to address this difference?
+    SIGSTKFLT: 16,
   },
   priority: {
     PRIORITY_LOW: 19,

node/testdata/infinite_loop.js

@@ -0,0 +1,3 @@
+while (true) {
+  await new Promise((resolve) => setTimeout(resolve, 1000));
+}