denoland / deno

A modern runtime for JavaScript and TypeScript.
https://deno.com
MIT License
97.28k stars 5.36k forks source link

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

Open marvinhagemeister opened 6 months ago

marvinhagemeister commented 6 months ago

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 https://github.com/denoland/deno/issues/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}`
    );
    }
  2. 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 https://github.com/denoland/deno/commit/5294885a5a411e6b2e9674ce9d8f951c9c011988 2024-04-24)

cowboyd commented 6 months ago

@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 commented 6 months ago

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 commented 4 months ago

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 commented 3 months ago

@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.

uasi commented 3 months ago

https://github.com/denoland/deno/pull/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 commented 3 months ago

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()^pipe 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-fds^command-fds 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 structure[^startupinfow] passed to the CreateProcessW function (which is internally called by std::process::Command::spawn()^win-spawn). 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.

[^startupinfow]: 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

cowboyd commented 3 months ago

@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 commented 3 months ago

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

As said in https://github.com/rust-lang/rfcs/pull/2939#issuecomment-640162653 , however, we could implement it on our side relatively easily if the std lib provides a Windows-only CommandExt^command-ext method that allows us to modify STARTUPINFO.

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

nathanwhit commented 3 months ago

I'm looking into this currently

nathanwhit commented 2 months ago

This is now fixed on macOS and linux.

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