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.

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

Bug: Missing stdio pipes with node:child_process on Windows #23524

Open
marvinhagemeister opened this issue Apr 24, 2024 · 10 comments
Open

Bug: Missing stdio pipes with node:child_process on Windows #23524

marvinhagemeister opened this issue Apr 24, 2024 · 10 comments
Assignees
Labels
bug Something isn't working correctly node compat

Comments

@marvinhagemeister
Copy link
Contributor

marvinhagemeister commented Apr 24, 2024

Update: This issue is now resolved on unix platforms (macOS and linux), but is not yet fixed on Windows.


Tried the current state of running playwright in Deno (see #16899) and ran into an issue regarding the pipes set up for node:child_process.spawn(). The launcher code can be found here https://github.com/microsoft/playwright/blob/main/packages/playwright-core/src/utils/processLauncher.ts#L133

They expect to receive 5 streams to be set up, but we only set up the first three. I created a minimal reproduction out of that:

Steps to reproduce

  1. Create file main.mjs with these contents
import cp from "node:child_process";
import process from "node:process";

if (import.meta.url.endsWith("main.mjs")) {
  const child = cp.spawn(process.execPath, [import.meta.url], {
    stdio: ["ignore", "pipe", "pipe", "pipe", "pipe"],
  });

  console.assert(
    child.stdio.length === 5,
    `Expected stdio length to equal 5 but got ${child.stdio.length}`
  );
}
  1. Run DENO_FUTURE=1 deno run -A main.mjs

Can also be run in Node for comparison node main.mjs

Version: Deno 1.42.4 (git 5294885 2024-04-24)

@marvinhagemeister marvinhagemeister added bug Something isn't working correctly node compat labels Apr 24, 2024
@cowboyd
Copy link

cowboyd commented Apr 26, 2024

@marvinhagemeister The docs only very briefly mention the ability to open other file descriptors https://nodejs.org/api/child_process.html#optionsstdio Is there any explainer anywhere on how they should behave?

@marvinhagemeister
Copy link
Contributor Author

I'm not aware of any explainer. I don't know how they're supposed to work either. One would probably look at the Node source code to get an answer to that.

@cowboyd
Copy link

cowboyd commented Jun 26, 2024

There isn't almost a day that goes by that I don't wish I had this working so that I could use Playwright from Deno. In fact, it is the only thing keeping my current work project on Node. If I wanted to take a stab at implementing this, do you have any pointers on where I should start?

@uasi
Copy link

uasi commented Jul 12, 2024

@cowboyd See https://github.com/denoland/deno/blob/v1.45.1/ext/node/polyfills/internal/child_process.ts#L178 . Here, the stream specifiers after the first three are discarded. We need to figure out how to handle this and pass it to new Deno.Command(). We also need to make Deno.Command 1 accept additional streams and pass them down to ops::process::op_spawn_child 2.

Footnotes

  1. https://github.com/denoland/deno/blob/v1.45.1/runtime/js/40_process.js#L440-L476

  2. https://github.com/denoland/deno/blob/v1.45.1/runtime/ops/process.rs#L592-L599

@uasi
Copy link

uasi commented Jul 12, 2024

#24106 introduces changes to enable Deno's node:child_process polyfill to support a new type of stream. This might be useful as a reference when working on Deno.Command.

@uasi
Copy link

uasi commented Jul 13, 2024

I've delved deeper into how we might implement this feature. Here's what I've found:

We can use a pair of pipes returned by deno_io::pipe::pipe()1 as a stream. The writable end can be passed to the child process, while the readable end can be used by the parent process.

Unfortunately, there doesn't seem to be a cross-platform way to pass "pipes" to child processes.

On Unix systems, pipes are just file descriptors. The command-fds2 crate allows passing additional file descriptors to child processes.

On Windows, pipes are HANDLEs of Windows' named pipes. To pass these to child processes, we need to set them in the STARTUPINFOW structure3 passed to the CreateProcessW function (which is internally called by std::process::Command::spawn()4). I couldn't find an existing crate to handle this. It seems we might need to reimplement a method equivalent to std::process::Command::spawn() ourselves to achieve this.

Footnotes

  1. https://github.com/denoland/deno/blob/v1.45.2/ext/io/pipe.rs#L270-L272

  2. https://github.com/google/command-fds

  3. This is undocumented but widely used Win32API feature; Node's cross platform layer, libuv, does it like this https://github.com/libuv/libuv/blob/v1.48.0/src/win/process.c#L1034-L1035

  4. https://github.com/rust-lang/rust/blob/1.79.0/library/std/src/sys/pal/windows/process.rs#L338

@cowboyd
Copy link

cowboyd commented Jul 16, 2024

@uasi This is fantastic research, and very enlightening about where exactly to look :)

It seems we might need to reimplement a method equivalent to std::process::Command::spawn() ourselves to achieve this.

This seems like a pretty heavy lift to not only implement but maintain a bespoke equivalent of Command. Do you think that the rust std lib might accept a change that would enable creating additional pipes between parent and child processes?

@uasi
Copy link

uasi commented Jul 16, 2024

@cowboyd I think the chances are low. The challenges have been discussed in a PR1 proposing an RFC about the exact API for that.

As said in rust-lang/rfcs#2939 (comment) , however, we could implement it on our side relatively easily if the std lib provides a Windows-only CommandExt2 method that allows us to modify STARTUPINFO.

Or we could set aside Windows support for now and make it work on Unix first.

Footnotes

  1. https://github.com/rust-lang/rfcs/pull/2939

  2. https://doc.rust-lang.org/std/os/windows/process/trait.CommandExt.html

@nathanwhit nathanwhit self-assigned this Jul 18, 2024
@nathanwhit
Copy link
Member

I'm looking into this currently

nathanwhit added a commit that referenced this issue Aug 15, 2024
Linux/macos only currently.

Part of #23524 (fixes it on
platforms other than windows).
Part of #16899  (fixes it on platforms other than windows).

After this PR, playwright is functional on mac/linux.
@nathanwhit
Copy link
Member

This is now fixed on macOS and linux.

As mentioned above windows is trickier, so that will come in a future PR.

@nathanwhit nathanwhit changed the title Bug: Missing stdio pipes with node:child_process Bug: Missing stdio pipes with node:child_process on Windows Aug 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctly node compat
Projects
None yet
Development

No branches or pull requests

4 participants