child_process: add windowsHide option #15380

cjihrig · 2017-09-13T04:28:39Z

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 · 2017-09-13T06:37:21Z

src/process_wrap.cc

+    // options.windowsHide
+    Local<String> windows_hide_key = env->windows_hide_string();
+
+    if (js_options->Get(windows_hide_key)->IsTrue()) {


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

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

bnoordhuis · 2017-09-13T06:37:44Z

src/spawn_sync.cc

@@ -777,6 +777,11 @@ int SyncProcessRunner::ParseOptions(Local<Value> js_value) {
  if (js_options->Get(env()->detached_string())->BooleanValue())
    uv_process_options_.flags |= UV_PROCESS_DETACHED;

+  Local<String> win_hide = env()->windows_hide_string();
+
+  if (js_options->Get(win_hide)->BooleanValue())


jasnell

LGTM with docs

bzoz · 2017-09-13T16:33:22Z

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 · 2017-09-13T18:08:07Z

@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 · 2017-09-13T19:44:02Z

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

bzoz · 2017-09-14T08:57:36Z

@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 · 2017-09-21T22:14:52Z

@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 · 2017-09-22T07:13:00Z

There is discussion going on in #15217 about this

BridgeAR · 2017-09-22T20:35:16Z

@bzoz thanks for pointing this out.

sam-github · 2017-09-24T04:34:08Z

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.

The existing UV_PROCESS_WINDOWS_HIDE flag only applies to executables linked to the WINDOWS subsystem. This allows CONSOLE subsystem applications to pop up a console window. This commit sets the CREATE_NO_WINDOW process flag when UV_PROCESS_WINDOWS_HIDE to prevent this behavior. Refs: nodejs/node#15380 Refs: joyent/libuv#627 Refs: libuv#965 PR-URL: libuv#1558 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>

cjihrig · 2017-09-26T00:12:30Z

This should be fixed by the next libuv update.

refack · 2017-09-26T02:16:57Z

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)

const assert = require('assert');
const cp = require('child_process');

{
  const bat = 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 · 2017-09-26T02:43:09Z

@refack it seems like there are two problems:

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

refack · 2017-09-26T21:18:14Z

@cjihrig

🤞
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 · 2017-09-26T22:52:54Z

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

sam-github · 2017-09-27T03:11:15Z

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

refack · 2017-09-27T13:00:12Z

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 · 2017-09-27T13:02:18Z

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

Refs: nodejs#15380 Refs: nodejs#15683 Fixes: nodejs#15394 Fixes: nodejs#15770 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

cjihrig · 2017-10-24T01:07:14Z

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

Refs: nodejs#15380 PR-URL: nodejs#16247 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

In spawn_sync.cc, two consecutive loops are used to convert data to strings, and compute the size of the data. This commit merges the two independent loops into one. Refs: nodejs#15380 PR-URL: nodejs#16247 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

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. Backport-PR-URL: #16425 Fixes: #15217 PR-URL: #15380 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

Backport-PR-URL: #16426 Refs: #15380 PR-URL: #16247 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

In spawn_sync.cc, two consecutive loops are used to convert data to strings, and compute the size of the data. This commit merges the two independent loops into one. Backport-PR-URL: #16426 Refs: #15380 PR-URL: #16247 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #15745 Refs: #15380 Refs: #15683 Fixes: #15394 Fixes: #15770 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Refs: nodejs/node#15380 PR-URL: nodejs/node#16247 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

In spawn_sync.cc, two consecutive loops are used to convert data to strings, and compute the size of the data. This commit merges the two independent loops into one. Refs: nodejs/node#15380 PR-URL: nodejs/node#16247 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2017-11-16T21:04:22Z

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 · 2017-11-16T21:19:05Z

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 · 2017-11-16T21:20:56Z

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 · 2017-11-17T09:14:34Z

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

Refs: nodejs/node#15380 PR-URL: nodejs/node#16247 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

In spawn_sync.cc, two consecutive loops are used to convert data to strings, and compute the size of the data. This commit merges the two independent loops into one. Refs: nodejs/node#15380 PR-URL: nodejs/node#16247 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Sep 13, 2017

cjihrig mentioned this pull request Sep 13, 2017

Fix spawn of cmd and bat files on Windows #15217

Closed

mscdex added child_process Issues and PRs related to the child_process subsystem. windows Issues and PRs related to the Windows platform. labels Sep 13, 2017

bnoordhuis approved these changes Sep 13, 2017

View reviewed changes

jasnell approved these changes Sep 13, 2017

View reviewed changes

BridgeAR added blocked PRs that are blocked by other issues or PRs. wip Issues and PRs that are still a work in progress. labels Sep 19, 2017

sindresorhus mentioned this pull request Sep 22, 2017

windows shell with forever sindresorhus/wallpaper#25

Closed

cjihrig mentioned this pull request Sep 24, 2017

win: let UV_PROCESS_WINDOWS_HIDE hide consoles libuv/libuv#1558

Merged

cjihrig mentioned this pull request Oct 3, 2017

deps: upgrade libuv to 1.15.0 #15745

Merged

2 tasks

cjihrig mentioned this pull request Oct 24, 2017

[v8.x backport] child_process: add windowsHide option #16425

Closed

4 tasks

cjihrig added a commit to cjihrig/node that referenced this pull request Oct 24, 2017

src: increase usage of context in process_wrap.cc

e8cbacd

Refs: nodejs#15380 PR-URL: nodejs#16247 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

cjihrig added a commit to cjihrig/node that referenced this pull request Oct 24, 2017

src: increase usage of context in spawn_sync.cc

4af1bec

Refs: nodejs#15380 PR-URL: nodejs#16247 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Oct 24, 2017

src: increase usage of context in process_wrap.cc

91dc6c7

Backport-PR-URL: #16426 Refs: #15380 PR-URL: #16247 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Oct 24, 2017

src: increase usage of context in spawn_sync.cc

5dbefe1

Backport-PR-URL: #16426 Refs: #15380 PR-URL: #16247 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Oct 25, 2017

deps: upgrade libuv to 1.15.0

02e4303

PR-URL: #15745 Refs: #15380 Refs: #15683 Fixes: #15394 Fixes: #15770 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017

src: increase usage of context in process_wrap.cc

db1b343

Refs: nodejs/node#15380 PR-URL: nodejs/node#16247 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017

src: increase usage of context in spawn_sync.cc

9934df1

Refs: nodejs/node#15380 PR-URL: nodejs/node#16247 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

lance added backported-to-v8.x and removed backport-requested-v8.x labels Oct 28, 2017

MylesBorins added the backport-requested-v6.x label Nov 16, 2017

addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017

src: increase usage of context in process_wrap.cc

8efc0f9

Refs: nodejs/node#15380 PR-URL: nodejs/node#16247 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017

src: increase usage of context in spawn_sync.cc

f895e1c

Refs: nodejs/node#15380 PR-URL: nodejs/node#16247 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

skyfore mentioned this pull request Jun 27, 2018

How can i run server in daemon mode in Windows ? eggjs/egg#2738

Closed

jasnell removed wip Issues and PRs that are still a work in progress. blocked PRs that are blocked by other issues or PRs. labels Aug 17, 2018

bzoz mentioned this pull request Nov 9, 2018

Revert "child_process: change windowsHide default to true" #24034

Closed

4 tasks

neel1996 mentioned this pull request Jul 8, 2020

Fix terminal spawn bug with pm2 and child_process neel1996/gitconvex-node-server#8

Closed

ehmicky mentioned this pull request Aug 18, 2020

Windows: execa leaving processes around when running npm scripts in a sh shell sindresorhus/execa#433

Open

Gresliebear mentioned this pull request Jul 13, 2022

[Tech debt]: Re-enable telemetry for windows redwoodjs/redwood#5901

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

child_process: add windowsHide option #15380

child_process: add windowsHide option #15380

cjihrig commented Sep 13, 2017 •

edited

Loading

bnoordhuis Sep 13, 2017

cjihrig Oct 16, 2017

bnoordhuis Sep 13, 2017

jasnell left a comment

bzoz commented Sep 13, 2017

vmarchaud commented Sep 13, 2017

cjihrig commented Sep 13, 2017

bzoz commented Sep 14, 2017

BridgeAR commented Sep 21, 2017

bzoz commented Sep 22, 2017 •

edited

Loading

BridgeAR commented Sep 22, 2017

sam-github commented Sep 24, 2017

cjihrig commented Sep 26, 2017

refack commented Sep 26, 2017

cjihrig commented Sep 26, 2017

refack commented Sep 26, 2017 •

edited

Loading

tniessen commented Sep 26, 2017

sam-github commented Sep 27, 2017

refack commented Sep 27, 2017

cjihrig commented Sep 27, 2017

cjihrig commented Oct 24, 2017

MylesBorins commented Nov 16, 2017

bnoordhuis commented Nov 16, 2017

bnoordhuis commented Nov 16, 2017

vmarchaud commented Nov 17, 2017 •

edited

Loading

child_process: add windowsHide option #15380

child_process: add windowsHide option #15380

Conversation

cjihrig commented Sep 13, 2017 • edited Loading

Checklist

Affected core subsystem(s)

bnoordhuis Sep 13, 2017

Choose a reason for hiding this comment

cjihrig Oct 16, 2017

Choose a reason for hiding this comment

bnoordhuis Sep 13, 2017

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

bzoz commented Sep 13, 2017

vmarchaud commented Sep 13, 2017

cjihrig commented Sep 13, 2017

bzoz commented Sep 14, 2017

BridgeAR commented Sep 21, 2017

bzoz commented Sep 22, 2017 • edited Loading

BridgeAR commented Sep 22, 2017

sam-github commented Sep 24, 2017

cjihrig commented Sep 26, 2017

refack commented Sep 26, 2017

cjihrig commented Sep 26, 2017

refack commented Sep 26, 2017 • edited Loading

tniessen commented Sep 26, 2017

sam-github commented Sep 27, 2017

refack commented Sep 27, 2017

cjihrig commented Sep 27, 2017

cjihrig commented Oct 24, 2017

MylesBorins commented Nov 16, 2017

bnoordhuis commented Nov 16, 2017

bnoordhuis commented Nov 16, 2017

vmarchaud commented Nov 17, 2017 • edited Loading

cjihrig commented Sep 13, 2017 •

edited

Loading

bzoz commented Sep 22, 2017 •

edited

Loading

refack commented Sep 26, 2017 •

edited

Loading

vmarchaud commented Nov 17, 2017 •

edited

Loading