fix(ext/node): Implement detached option in `child_process` (#25218)

Fixes https://github.com/denoland/deno/issues/25193.

8 files changed, 136 insertions, 20 deletions

diff --git a/ext/node/polyfills/internal/child_process.ts b/ext/node/polyfills/internal/child_process.ts
index 30c249277..175119afa 100644
--- a/ext/node/polyfills/internal/child_process.ts
+++ b/ext/node/polyfills/internal/child_process.ts
@@ -56,6 +56,7 @@ import { StringPrototypeSlice } from "ext:deno_node/internal/primordials.mjs";
 import { StreamBase } from "ext:deno_node/internal_binding/stream_wrap.ts";
 import { Pipe, socketType } from "ext:deno_node/internal_binding/pipe_wrap.ts";
 import { Socket } from "node:net";
+import { kDetached, kExtraStdio, kIpc } from "ext:runtime/40_process.js";
 
 export function mapValues<T, O>(
   record: Readonly<Record<string, T>>,
@@ -109,6 +110,7 @@ export function stdioStringToArray(
 
 const kClosesNeeded = Symbol("_closesNeeded");
 const kClosesReceived = Symbol("_closesReceived");
+const kCanDisconnect = Symbol("_canDisconnect");
 
 // We only want to emit a close event for the child process when all of
 // the writable streams have closed. The value of `child[kClosesNeeded]` should be 1 +
@@ -222,7 +224,7 @@ export class ChildProcess extends EventEmitter {
   #spawned = Promise.withResolvers<void>();
   [kClosesNeeded] = 1;
   [kClosesReceived] = 0;
-  canDisconnect = false;
+  [kCanDisconnect] = false;
 
   constructor(
     command: string,
@@ -238,6 +240,7 @@ export class ChildProcess extends EventEmitter {
       shell = false,
       signal,
       windowsVerbatimArguments = false,
+      detached,
     } = options || {};
     const normalizedStdio = normalizeStdioOption(stdio);
     const [
@@ -275,8 +278,9 @@ export class ChildProcess extends EventEmitter {
         stdout: toDenoStdio(stdout),
         stderr: toDenoStdio(stderr),
         windowsRawArguments: windowsVerbatimArguments,
-        ipc, // internal
-        extraStdio: extraStdioNormalized,
+        [kIpc]: ipc, // internal
+        [kExtraStdio]: extraStdioNormalized,
+        [kDetached]: detached,
       }).spawn();
       this.pid = this.#process.pid;
 
@@ -387,8 +391,8 @@ export class ChildProcess extends EventEmitter {
           this.emit("exit", exitCode, signalCode);
           await this.#_waitForChildStreamsToClose();
           this.#closePipes();
-          maybeClose(this);
           nextTick(flushStdio, this);
+          maybeClose(this);
         });
       })();
     } catch (err) {
@@ -421,7 +425,7 @@ export class ChildProcess extends EventEmitter {
     }
 
     /* Cancel any pending IPC I/O */
-    if (this.canDisconnect) {
+    if (this[kCanDisconnect]) {
       this.disconnect?.();
     }
 
@@ -552,7 +556,7 @@ export interface ChildProcessOptions {
   stdio?: Array<NodeStdio | number | Stream | null | undefined> | NodeStdio;
 
   /**
-   * NOTE: This option is not yet implemented.
+   * Whether to spawn the process in a detached state.
    */
   detached?: boolean;
 
@@ -1416,7 +1420,7 @@ export function setupChannel(target: any, ipc: number) {
     }
 
     target.connected = false;
-    target.canDisconnect = false;
+    target[kCanDisconnect] = false;
     control[kControlDisconnect]();
     process.nextTick(() => {
       target.channel = null;
@@ -1424,7 +1428,7 @@ export function setupChannel(target: any, ipc: number) {
       target.emit("disconnect");
     });
   };
-  target.canDisconnect = true;
+  target[kCanDisconnect] = true;
 
   // Start reading messages from the channel.
   readLoop();
diff --git a/ext/node/polyfills/process.ts b/ext/node/polyfills/process.ts
index f84255814..d7e21b657 100644
--- a/ext/node/polyfills/process.ts
+++ b/ext/node/polyfills/process.ts
@@ -267,9 +267,11 @@ memoryUsage.rss = function (): number {
 
 // Returns a negative error code than can be recognized by errnoException
 function _kill(pid: number, sig: number): number {
+  const maybeMapErrno = (res: number) =>
+    res === 0 ? res : uv.mapSysErrnoToUvErrno(res);
   // signal 0 does not exist in constants.os.signals, thats why it have to be handled explicitly
   if (sig === 0) {
-    return op_node_process_kill(pid, 0);
+    return maybeMapErrno(op_node_process_kill(pid, 0));
   }
   const maybeSignal = Object.entries(constants.os.signals).find((
     [_, numericCode],
@@ -278,7 +280,7 @@ function _kill(pid: number, sig: number): number {
   if (!maybeSignal) {
     return uv.codeMap.get("EINVAL");
   }
-  return op_node_process_kill(pid, sig);
+  return maybeMapErrno(op_node_process_kill(pid, sig));
 }
 
 export function dlopen(module, filename, _flags) {
diff --git a/runtime/js/40_process.js b/runtime/js/40_process.js
index 358805180..ac9411916 100644
--- a/runtime/js/40_process.js
+++ b/runtime/js/40_process.js
@@ -157,6 +157,10 @@ function run({
   return new Process(res);
 }
 
+export const kExtraStdio = Symbol("extraStdio");
+export const kIpc = Symbol("ipc");
+export const kDetached = Symbol("detached");
+
 const illegalConstructorKey = Symbol("illegalConstructorKey");
 
 function spawnChildInner(command, apiName, {
@@ -166,13 +170,14 @@ function spawnChildInner(command, apiName, {
   env = { __proto__: null },
   uid = undefined,
   gid = undefined,
+  signal = undefined,
   stdin = "null",
   stdout = "piped",
   stderr = "piped",
-  signal = undefined,
   windowsRawArguments = false,
-  ipc = -1,
-  extraStdio = [],
+  [kDetached]: detached = false,
+  [kExtraStdio]: extraStdio = [],
+  [kIpc]: ipc = -1,
 } = { __proto__: null }) {
   const child = op_spawn_child({
     cmd: pathFromURL(command),
@@ -188,6 +193,7 @@ function spawnChildInner(command, apiName, {
     windowsRawArguments,
     ipc,
     extraStdio,
+    detached,
   }, apiName);
   return new ChildProcess(illegalConstructorKey, {
     ...child,
@@ -414,6 +420,7 @@ function spawnSync(command, {
     stderr,
     windowsRawArguments,
     extraStdio: [],
+    detached: false,
   });
   return {
     success: result.status.success,
diff --git a/runtime/ops/process.rs b/runtime/ops/process.rs
index d7058a053..b7242c07f 100644
--- a/runtime/ops/process.rs
+++ b/runtime/ops/process.rs
@@ -159,6 +159,7 @@ pub struct SpawnArgs {
   stdio: ChildStdio,
 
   extra_stdio: Vec<Stdio>,
+  detached: bool,
 }
 
 #[derive(Deserialize)]
@@ -243,12 +244,21 @@ fn create_command(
   let mut command = std::process::Command::new(cmd);
 
   #[cfg(windows)]
-  if args.windows_raw_arguments {
-    for arg in args.args.iter() {
-      command.raw_arg(arg);
+  {
+    if args.detached {
+      // TODO(nathanwhit): Currently this causes the process to hang
+      // until the detached process exits (so never). It repros with just the
+      // rust std library, so it's either a bug or requires more control than we have.
+      // To be resolved at the same time as additional stdio support.
+      log::warn!("detached processes are not currently supported on Windows");
+    }
+    if args.windows_raw_arguments {
+      for arg in args.args.iter() {
+        command.raw_arg(arg);
+      }
+    } else {
+      command.args(args.args);
     }
-  } else {
-    command.args(args.args);
   }
 
   #[cfg(not(windows))]
@@ -336,7 +346,11 @@ fn create_command(
       }
     }
 
+    let detached = args.detached;
     command.pre_exec(move || {
+      if detached {
+        libc::setsid();
+      }
       for &(src, dst) in &fds_to_dup {
         if src >= 0 && dst >= 0 {
           let _fd = libc::dup2(src, dst);
@@ -402,12 +416,15 @@ fn spawn_child(
   command: std::process::Command,
   ipc_pipe_rid: Option<ResourceId>,
   extra_pipe_rids: Vec<Option<ResourceId>>,
+  detached: bool,
 ) -> Result<Child, AnyError> {
   let mut command = tokio::process::Command::from(command);
   // TODO(@crowlkats): allow detaching processes.
   //  currently deno will orphan a process when exiting with an error or Deno.exit()
   // We want to kill child when it's closed
-  command.kill_on_drop(true);
+  if !detached {
+    command.kill_on_drop(true);
+  }
 
   let mut child = match command.spawn() {
     Ok(child) => child,
@@ -647,9 +664,10 @@ fn op_spawn_child(
   #[serde] args: SpawnArgs,
   #[string] api_name: String,
 ) -> Result<Child, AnyError> {
+  let detached = args.detached;
   let (command, pipe_rid, extra_pipe_rids, handles_to_close) =
     create_command(state, args, &api_name)?;
-  let child = spawn_child(state, command, pipe_rid, extra_pipe_rids);
+  let child = spawn_child(state, command, pipe_rid, extra_pipe_rids, detached);
   for handle in handles_to_close {
     close_raw_handle(handle);
   }
diff --git a/tests/node_compat/config.jsonc b/tests/node_compat/config.jsonc
index 8c8d503d5..01db5557d 100644
--- a/tests/node_compat/config.jsonc
+++ b/tests/node_compat/config.jsonc
@@ -8,6 +8,7 @@
       "elipses.txt",
       "empty.txt",
       "exit.js",
+      "parent-process-nonpersistent.js",
       "print-chars.js",
       "x.txt"
     ],
@@ -25,6 +26,7 @@
       "test-buffer-from.js",
       "test-buffer-includes.js",
       "test-buffer-indexof.js",
+      "test-child-process-detached.js",
       "test-child-process-exec-abortcontroller-promisified.js",
       "test-child-process-exec-encoding.js",
       "test-child-process-exec-kill-throws.js",
@@ -135,6 +137,7 @@
     "fixtures": [
       "a.js",
       "child_process_should_emit_error.js",
+      "child-process-persistent.js",
       "child-process-spawn-node.js",
       "echo.js",
       "elipses.txt",
@@ -766,6 +769,7 @@
   },
   "windowsIgnore": {
     "parallel": [
+      "test-child-process-detached.js",
       "test-child-process-exec-abortcontroller-promisified.js",
       "test-console-log-throw-primitive.js",
       "test-console-no-swallow-stack-overflow.js",
diff --git a/tests/node_compat/test/fixtures/child-process-persistent.js b/tests/node_compat/test/fixtures/child-process-persistent.js
new file mode 100644
index 000000000..520c4bebc
--- /dev/null
+++ b/tests/node_compat/test/fixtures/child-process-persistent.js
@@ -0,0 +1,8 @@
+// deno-fmt-ignore-file
+// deno-lint-ignore-file
+
+// Copyright Joyent and Node contributors. All rights reserved. MIT license.
+// Taken from Node 18.12.1
+// This file is automatically generated by `tests/node_compat/runner/setup.ts`. Do not modify this file manually.
+
+setInterval(function() {}, 9999);
diff --git a/tests/node_compat/test/fixtures/parent-process-nonpersistent.js b/tests/node_compat/test/fixtures/parent-process-nonpersistent.js
new file mode 100644
index 000000000..537478aac
--- /dev/null
+++ b/tests/node_compat/test/fixtures/parent-process-nonpersistent.js
@@ -0,0 +1,21 @@
+// deno-fmt-ignore-file
+// deno-lint-ignore-file
+
+// Copyright Joyent and Node contributors. All rights reserved. MIT license.
+// Taken from Node 18.12.1
+// This file is automatically generated by `tests/node_compat/runner/setup.ts`. Do not modify this file manually.
+
+// Modified to add `runner.ts` to inject `require` into subprocess
+
+const spawn = require('child_process').spawn,
+    path = require('path'),
+    childPath = path.join(__dirname, 'child-process-persistent.js');
+
+var child = spawn(process.execPath, [ childPath ], {
+  detached: true,
+  stdio: 'ignore'
+});
+
+console.log(child.pid);
+
+child.unref();
diff --git a/tests/node_compat/test/parallel/test-child-process-detached.js b/tests/node_compat/test/parallel/test-child-process-detached.js
new file mode 100644
index 000000000..65ec20753
--- /dev/null
+++ b/tests/node_compat/test/parallel/test-child-process-detached.js
@@ -0,0 +1,52 @@
+// deno-fmt-ignore-file
+// deno-lint-ignore-file
+
+// Copyright Joyent and Node contributors. All rights reserved. MIT license.
+// Taken from Node 18.12.1
+// This file is automatically generated by `tests/node_compat/runner/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.
+
+// Modified to add `runner.ts` to inject `require` into subprocess
+
+'use strict';
+require('../common');
+const assert = require('assert');
+const fixtures = require('../common/fixtures');
+
+const spawn = require('child_process').spawn;
+const childPath = fixtures.path('parent-process-nonpersistent.js');
+let persistentPid = -1;
+
+const child = spawn(process.execPath, [ "runner.ts", childPath ]);
+
+child.stdout.on('data', function(data) {
+  persistentPid = parseInt(data, 10);
+});
+
+process.on('exit', function() {
+  assert.notStrictEqual(persistentPid, -1);
+  assert.throws(function() {
+    process.kill(child.pid);
+  }, /^Error: kill ESRCH$/);
+  process.kill(persistentPid);
+});