Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(node): Rework node:child_process IPC #24763

Merged
merged 43 commits into from
Jul 30, 2024
Merged

fix(node): Rework node:child_process IPC #24763

Show file tree
Hide file tree
Changes from 40 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
ab62f0e
Skip conditions arg in fork
nathanwhit Jul 10, 2024
4818ecf
Only serialize once for ipc_write
nathanwhit Jul 10, 2024
28fb48c
Rework ipc serialization
nathanwhit Jul 16, 2024
057aeec
Add unit test for ipc serialization
nathanwhit Jul 16, 2024
f548aa1
Use memchr crate instead of handrolling
nathanwhit Jul 16, 2024
94a97de
Don't use null as sentinel for closing ipc
nathanwhit Jul 17, 2024
7957505
Fix test
nathanwhit Jul 17, 2024
4ffc92a
Fix inability to exit due to unresolved async op
nathanwhit Jul 17, 2024
f7d1739
Fix test
nathanwhit Jul 17, 2024
24e91ea
Don't throw if you call kill multiple times
nathanwhit Jul 17, 2024
fab2dad
Do buffering ourselves to avoid UB
nathanwhit Jul 17, 2024
8c2910b
Return value from send for backpressure
nathanwhit Jul 17, 2024
9ddf6e3
Fix benchmark to properly listen to backpressure
nathanwhit Jul 17, 2024
c58c3aa
Add comment
nathanwhit Jul 17, 2024
c19e2e6
Fix off by one bug
nathanwhit Jul 17, 2024
adfb302
Fix ref unref and attach channel property
nathanwhit Jul 18, 2024
5057197
patch argv in runner script
nathanwhit Jul 18, 2024
dd679bc
Fix refing after channel disconnect
nathanwhit Jul 18, 2024
849036c
Use `NODE_CHANNEL_FD` env var
nathanwhit Jul 24, 2024
88731ff
Remove argv patch because I keep forkbombing myself
nathanwhit Jul 25, 2024
d090bf0
Rebase fixup
nathanwhit Jul 26, 2024
852ec72
Use setimmediate
nathanwhit Jul 27, 2024
137db96
Some small tweaks
nathanwhit Jul 27, 2024
a6272e9
Fmt + lint
nathanwhit Jul 27, 2024
a87bac7
Use setimmediate in bench
nathanwhit Jul 27, 2024
c43141c
Windows
nathanwhit Jul 27, 2024
36b6a00
Windows
nathanwhit Jul 27, 2024
4a70d50
Emit child process close event once streams close
nathanwhit Jul 29, 2024
13d98fb
Lints
nathanwhit Jul 29, 2024
ab6eda8
Disconnect only defined if using ipc
nathanwhit Jul 29, 2024
ef37d3e
Fix inverted condition
nathanwhit Jul 29, 2024
0e4933a
Buffer messages received before listener, proper error
nathanwhit Jul 30, 2024
e758030
Add tests + cleanup
nathanwhit Jul 30, 2024
145c205
Some fixes + unit tests
nathanwhit Jul 30, 2024
238745a
Wait for close event in test
nathanwhit Jul 30, 2024
b3f3f0d
Add comment + cleanup
nathanwhit Jul 30, 2024
faec9d2
make sure to init platform
nathanwhit Jul 30, 2024
e863b27
Fix
nathanwhit Jul 30, 2024
61c0f6a
Filter out internal node messages
nathanwhit Jul 30, 2024
34b4115
Windows
nathanwhit Jul 30, 2024
eb0ff1b
Windows again
nathanwhit Jul 30, 2024
c0ea53d
Disconnect on windows to fix hang
nathanwhit Jul 30, 2024
da62777
Fix flipped condition
nathanwhit Jul 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions cli/args/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@littledivy just to be sure, we went with DENO_CHANNEL_FD because we couldn't communicate between Node.js and Deno subprocesses, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, this PR seems to enable Deno processes spawned by Node processes to communicate. @nathanwhit is it also working the other way around?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, at least on unix (haven't tested windows). Though sending handles won't work (bc we don't support it yet).

if let Some(node_channel_fd) = maybe_node_channel_fd {
// Remove so that child processes don't inherit this environment variable.
std::env::remove_var("DENO_CHANNEL_FD");
std::env::remove_var("NODE_CHANNEL_FD");
node_channel_fd.parse::<i64>().ok()
} else {
None
Expand Down
1 change: 1 addition & 0 deletions ext/node/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ libc.workspace = true
libz-sys.workspace = true
md-5 = { version = "0.10.5", features = ["oid"] }
md4 = "0.10.2"
memchr = "2.7.4"
node_resolver.workspace = true
num-bigint.workspace = true
num-bigint-dig = "0.8.2"
Expand Down
14 changes: 12 additions & 2 deletions ext/node/benchmarks/child_process_ipc.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,20 @@ import { setImmediate } from "node:timers";
if (process.env.CHILD) {
const len = +process.env.CHILD;
const msg = ".".repeat(len);
let waiting = false;
const send = () => {
while (process.send(msg));
while (
process.send(msg, undefined, undefined, (_e) => {
if (waiting) {
waiting = false;
setImmediate(send);
}
})
);
// Wait: backlog of unsent messages exceeds threshold
setImmediate(send);
// once the message is sent, the callback will be called
// and we'll resume
waiting = true;
};
send();
} else {
Expand Down
3 changes: 3 additions & 0 deletions ext/node/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ pub use deno_package_json::PackageJson;
pub use node_resolver::PathClean;
pub use ops::ipc::ChildPipeFd;
pub use ops::ipc::IpcJsonStreamResource;
pub use ops::ipc::IpcRefTracker;
use ops::vm;
pub use ops::vm::create_v8_context;
pub use ops::vm::init_global_template;
Expand Down Expand Up @@ -378,6 +379,8 @@ deno_core::extension!(deno_node,
ops::ipc::op_node_child_ipc_pipe,
ops::ipc::op_node_ipc_write,
ops::ipc::op_node_ipc_read,
ops::ipc::op_node_ipc_ref,
ops::ipc::op_node_ipc_unref,
ops::process::op_node_process_kill,
ops::process::op_process_abort,
],
Expand Down
Loading
Loading