fix(node): Rework node:child_process IPC #24763
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bartlomieju
reviewed
Jul 29, 2024
| @@ -1050,10 +1050,10 @@ impl CliOptions { | |||
| } | |||
|
|
|||
| pub fn node_ipc_fd(&self) -> Option<i64> { | |||
| let maybe_node_channel_fd = std::env::var("DENO_CHANNEL_FD").ok(); | |||
| let maybe_node_channel_fd = std::env::var("NODE_CHANNEL_FD").ok(); | |||
There was a problem hiding this comment.
@littledivy just to be sure, we went with DENO_CHANNEL_FD because we couldn't communicate between Node.js and Deno subprocesses, right?
There was a problem hiding this comment.
yup, this PR seems to enable Deno processes spawned by Node processes to communicate. @nathanwhit is it also working the other way around?
There was a problem hiding this comment.
Yup, at least on unix (haven't tested windows). Though sending handles won't work (bc we don't support it yet).
| @@ -53,25 +163,66 @@ mod impl_ { | |||
| Some(child_pipe_fd) => child_pipe_fd.0, | |||
| None => return Ok(None), | |||
| }; | |||
|
|
|||
| let ref_tracker = IpcRefTracker::new(state.external_ops_tracker.clone()); | |||
There was a problem hiding this comment.
Not for this PR, but I think we should move most of this module to runtime/ops or even create a separate deno_ipc crate. Besides the serialization code it seems abstract enough that we could expose an IPC API for Deno.Command API.
nathanwhit
force-pushed
the
child-process-rework
branch
from
c08627b to
34b4115
Compare
crowlKats
pushed a commit
that referenced
this pull request
Jul 31, 2024
Fixes #24756. Fixes #24796. This also gets vitest working when using [`--pool=forks`](https://vitest.dev/guide/improving-performance#pool) (which is the default as of vitest 2.0). Ref #23882. --- This PR resolves a handful of issues with child_process IPC. In particular: - We didn't support sending typed array views over IPC - Opening an IPC channel resulted in the event loop never exiting - Sending a `null` over IPC would terminate the channel - There was some UB in the read implementation (transmuting an `&[u8]` to `&mut [u8]`) - The `send` method wasn't returning anything, so there was no way to signal backpressure (this also resulted in the benchmark `child_process_ipc.mjs` being misleading, as it tried to respect backpressure. That gave node much worse results at larger message sizes, and gave us much worse results at smaller message sizes). - We weren't setting up the `channel` property on the `process` global (or on the `ChildProcess` object), and also didn't have a way to ref/unref the channel - Calling `kill` multiple times (or disconnecting the channel, then calling kill) would throw an error - Node couldn't spawn a deno subprocess and communicate with it over IPC (cherry picked from commit cd59fc5)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #24756. Fixes #24796.
This also gets vitest working when using
--pool=forks(which is the default as of vitest 2.0). Ref #23882.This PR resolves a handful of issues with child_process IPC. In particular:
nullover IPC would terminate the channel&[u8]to&mut [u8])sendmethod wasn't returning anything, so there was no way to signal backpressure (this also resulted in the benchmarkchild_process_ipc.mjsbeing misleading, as it tried to respect backpressure. That gave node much worse results at larger message sizes, and gave us much worse results at smaller message sizes).channelproperty on theprocessglobal (or on theChildProcessobject), and also didn't have a way to ref/unref the channelkillmultiple times (or disconnecting the channel, then calling kill) would throw an error