From 81a6504e670d32bdc5e0a8328c328fdf8e208913 Mon Sep 17 00:00:00 2001 From: Divy Srivastava Date: Fri, 15 Dec 2023 16:20:05 +0530 Subject: [PATCH] refactor: setup child process pipe in Rust (#21579) Avoid passing the fd into JS and back into Rust. Instead we setup the child's end of the pipe directly using a special Rust op. --- ext/node/lib.rs | 2 ++ ext/node/ops/ipc.rs | 24 ++++++++++++++++++++++++ ext/node/polyfills/02_init.js | 3 +-- ext/node/polyfills/child_process.ts | 4 +++- runtime/js/99_main.js | 3 +-- runtime/worker.rs | 11 ++++++++++- runtime/worker_bootstrap.rs | 3 --- 7 files changed, 41 insertions(+), 9 deletions(-) diff --git a/ext/node/lib.rs b/ext/node/lib.rs index 77f01b3d38..dd3acd17cc 100644 --- a/ext/node/lib.rs +++ b/ext/node/lib.rs @@ -30,6 +30,7 @@ mod path; mod polyfill; mod resolution; +pub use ops::ipc::ChildPipeFd; pub use ops::v8::VM_CONTEXT_INDEX; pub use package_json::PackageJson; pub use path::PathClean; @@ -313,6 +314,7 @@ deno_core::extension!(deno_node, ops::util::op_node_guess_handle_type, ops::crypto::op_node_create_private_key, ops::ipc::op_node_ipc_pipe, + ops::ipc::op_node_child_ipc_pipe, ops::ipc::op_node_ipc_write, ops::ipc::op_node_ipc_read, ], diff --git a/ext/node/ops/ipc.rs b/ext/node/ops/ipc.rs index d1aeeb40c1..afaccc49f0 100644 --- a/ext/node/ops/ipc.rs +++ b/ext/node/ops/ipc.rs @@ -6,6 +6,8 @@ pub use unix::*; #[cfg(windows)] pub use windows::*; +pub struct ChildPipeFd(pub i32); + #[cfg(unix)] mod unix { use std::cell::RefCell; @@ -46,6 +48,22 @@ mod unix { Ok(state.resource_table.add(IpcJsonStreamResource::new(fd)?)) } + // Open IPC pipe from bootstrap options. + #[op2] + #[smi] + pub fn op_node_child_ipc_pipe( + state: &mut OpState, + ) -> Result, AnyError> { + let fd = match state.try_borrow_mut::() { + Some(child_pipe_fd) => child_pipe_fd.0, + None => return Ok(None), + }; + + Ok(Some( + state.resource_table.add(IpcJsonStreamResource::new(fd)?), + )) + } + #[op2(async)] pub async fn op_node_ipc_write( state: Rc>, @@ -492,6 +510,12 @@ mod windows { Err(deno_core::error::not_supported()) } + #[op2(fast)] + #[smi] + pub fn op_node_child_ipc_pipe() -> Result { + Ok(-1) + } + #[op2(async)] pub async fn op_node_ipc_write() -> Result<(), AnyError> { Err(deno_core::error::not_supported()) diff --git a/ext/node/polyfills/02_init.js b/ext/node/polyfills/02_init.js index e5a0279a57..8d9405c163 100644 --- a/ext/node/polyfills/02_init.js +++ b/ext/node/polyfills/02_init.js @@ -12,7 +12,6 @@ let initialized = false; function initialize( usesLocalNodeModulesDir, argv0, - ipcFd, ) { if (initialized) { throw Error("Node runtime already initialized"); @@ -38,7 +37,7 @@ function initialize( // but it's the only way to get `args` and `version` and this point. internals.__bootstrapNodeProcess(argv0, Deno.args, Deno.version); internals.__initWorkerThreads(); - internals.__setupChildProcessIpcChannel(ipcFd); + internals.__setupChildProcessIpcChannel(); // `Deno[Deno.internal].requireImpl` will be unreachable after this line. delete internals.requireImpl; } diff --git a/ext/node/polyfills/child_process.ts b/ext/node/polyfills/child_process.ts index c7d007f460..da82546f64 100644 --- a/ext/node/polyfills/child_process.ts +++ b/ext/node/polyfills/child_process.ts @@ -49,6 +49,7 @@ import { } from "ext:deno_node/internal/util.mjs"; const { core } = globalThis.__bootstrap; +const ops = core.ops; const MAX_BUFFER = 1024 * 1024; @@ -822,7 +823,8 @@ export function execFileSync( return ret.stdout as string | Buffer; } -function setupChildProcessIpcChannel(fd: number) { +function setupChildProcessIpcChannel() { + const fd = ops.op_node_child_ipc_pipe(); if (typeof fd != "number" || fd < 0) return; setupChannel(process, fd); } diff --git a/runtime/js/99_main.js b/runtime/js/99_main.js index 5b4b164a25..0469b38bfc 100644 --- a/runtime/js/99_main.js +++ b/runtime/js/99_main.js @@ -440,7 +440,6 @@ function bootstrapMainRuntime(runtimeOptions) { 3: inspectFlag, 5: hasNodeModulesDir, 6: maybeBinaryNpmCommandName, - 7: nodeIpcFd, } = runtimeOptions; performance.setTimeOrigin(DateNow()); @@ -546,7 +545,7 @@ function bootstrapMainRuntime(runtimeOptions) { ObjectDefineProperty(globalThis, "Deno", util.readOnly(finalDenoNs)); if (nodeBootstrap) { - nodeBootstrap(hasNodeModulesDir, maybeBinaryNpmCommandName, nodeIpcFd); + nodeBootstrap(hasNodeModulesDir, maybeBinaryNpmCommandName); } } diff --git a/runtime/worker.rs b/runtime/worker.rs index 94b0d9606a..549a6cdd69 100644 --- a/runtime/worker.rs +++ b/runtime/worker.rs @@ -487,7 +487,16 @@ impl MainWorker { } pub fn bootstrap(&mut self, options: BootstrapOptions) { - self.js_runtime.op_state().borrow_mut().put(options.clone()); + // Setup bootstrap options for ops. + { + let op_state = self.js_runtime.op_state(); + let mut state = op_state.borrow_mut(); + state.put(options.clone()); + if let Some(node_ipc_fd) = options.node_ipc_fd { + state.put(deno_node::ChildPipeFd(node_ipc_fd)); + } + } + let scope = &mut self.js_runtime.handle_scope(); let args = options.as_v8(scope); let bootstrap_fn = self.bootstrap_fn_global.take().unwrap(); diff --git a/runtime/worker_bootstrap.rs b/runtime/worker_bootstrap.rs index 8674190f3e..97d66158b6 100644 --- a/runtime/worker_bootstrap.rs +++ b/runtime/worker_bootstrap.rs @@ -117,8 +117,6 @@ struct BootstrapV8<'a>( bool, // maybe_binary_npm_command_name Option<&'a str>, - // node_ipc_fd - i32, ); impl BootstrapOptions { @@ -138,7 +136,6 @@ impl BootstrapOptions { self.enable_testing_features, self.has_node_modules_dir, self.maybe_binary_npm_command_name.as_deref(), - self.node_ipc_fd.unwrap_or(-1), ); bootstrap.serialize(ser).unwrap()