child_process: add windowsHide option

[cjihrig]

This commit exposes the UV_PROCESS_WINDOWS_HIDE flag in Node as a windowsHide option to the child_process methods. The option is only applicable to Windows, and is ignored elsewhere.

This still needs docs, which I'll write if:

• Someone verifies that this actually works on Windows.
• We agree that we want this documented. windowsVerbatimArguments, for example, is not.

Fixes: #15217

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

child_process

[bnoordhuis]

This should ideally use the overload that takes a Local<Context> but OTOH, all the other options don't.

[cjihrig]

@bnoordhuis I'll submit a follow up PR to switch all of the options to use the context.

[bnoordhuis]

Likewise.

[jasnell]

LGTM with docs

[bzoz]

It looks like the original issue is Unitech/pm2#2182 - console window pops-out when starting Node.js through PM2. Based on #7653 I added windowsHide: true to the spawn call that is used there.

This PR does not fix that issue, the console window still pops up with pm2 start something.js.

/cc @atrauzzi@vmarchaud

[vmarchaud]

@bzoz From what i remember while working on this issue (one year ago already), the "fork mode" using spawn wasn't popping up console windows.
However the cluster mode was the problem, i believe that the problem came from the fact that the cluster mode is using the fork method, is this possible to pass windowsHide to the cluster implementation ?
Sadly i cant help much since i gave up my windows workstation also one year ago :(

[cjihrig]

@bzoz is there any value in me keeping this open then?

[bzoz]

@vmarchaud hm, I observed the window popping out for a split-second when using pm2 start script.js. I think that uses the fork method. Now I'm confused and I don't know if I tested the right thing.

@cjihrig I think we should keep this open. At least until we verify what is the exact issue here.

[BridgeAR]

@bzoz would you be so kind look into this issue further and assign this to yourself? It seems like you know what you have to look for and keeping a issue open to maybe look into something later on is not the best way to handle it ;-)

[bzoz]

There is discussion going on in #15217 about this

[BridgeAR]

@bzoz thanks for pointing this out.

[sam-github]

I completely failed to figure out the specific conditions under which console windows pop up, much less that this flag prevents it, when I tried (see #2908 (comment)), but people do report this issue, its annoying, and would be great to have a fix for.

[cjihrig]

This should be fixed by the next libuv update.

[refack]

As @bzoz writes in #15217 (comment)
{detached: true } can cause a new console window to pop (but AFAICT it's a speed thing, so on my old machine it's 100% reproducible with the current code is master and with #15217)

constassert=require('assert');constcp=require('child_process');{constbat=cp.spawn('cmd.exe',['/k','test.cmd'],{stdio: 'ignore',detached: true,windowsHide: true});bat.on('exit',(code)=>{console.log(`Child exited with code ${code}`);});}

[cjihrig]

@refack it seems like there are two problems:

1. Hiding windows when the detached flag is not used. I think this should be fixed in libuv/libuv@b21c1f9.
2. Hiding windows when detached is true. Is there a straightforward way to fix this? If not, we may have to just document it.

[refack]

@cjihrig

1. 🤞
2. Might be covered by libuv/libuv@b21c1f9 as well (at least that's what the docs hint to), I'm building and testing now.

[tniessen]

I would prefer to prefix Windows-specific options with win32 instead of windows.

[sam-github]

Fwiw, I'm OK with just hide as the option name, or even hideWindow.

[refack]

I would prefer to prefix Windows-specific options with win32 instead of windows.

Fwiw, I'm OK with just hide as the option name, or even hideWindow.

If this will forever be a WIN32 only flag, we already have windowsVerbatimArguments as legacy, so IMHO better to keep that "convention".
If there might be use for this on other platforms I'd go with hidden

[cjihrig]

I agree with @refack. In fact, I picked the name windowsHide because of windowsVerbatimArguments.

[cjihrig]

Backport in #16425. The issue was that a change to the internals of spawnSync() had not been backported.

[MylesBorins]

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

[bnoordhuis]

In light of libuv/libuv#1625, currently not well-understood, it's probably better to hold off a little longer. I'll add labels.

[bnoordhuis]

Hm, hadn't noticed this has already been merged into v8.x. If no bugs have been reported, then I guess there is no reason to keep it out of v6.x.

[vmarchaud]

In the case of PM2, we added the windowsHide options to our fork an spawn calls and will soon publish it under the version 2.8.0. If there is any problem we should have people coming back to us about it, i will ping the libuv/libuv#1625 and here if that happen