cluster: support windowsHide option for workers #17412

toddwong · 2017-12-01T03:36:26Z

Fixes: #17370

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)

cluster

bnoordhuis

LGTM with a couple of comments about the test.

bnoordhuis · 2017-12-01T10:18:15Z

test/parallel/test-cluster-fork-windowsHide.js

+    assert.strictEqual(signal, null);
+  }));
+
+  cstdout.on('data', (data) => {


Call cstdout.setEncoding('utf8'), then you don't have to .toString('utf8') below.

Yeah, you are right, cstdout.setEncoding('utf8') make more sense here. Just deal too much binary data these days :D .

bnoordhuis · 2017-12-01T10:19:00Z

test/parallel/test-cluster-fork-windowsHide.js

+      worker.disconnect();
+    }
+  });
+  cstdin.write(msg, 'utf8');


You don't have to specify 'utf8' if you're writing a string, that's the default encoding.

I wasn't aware of that...

bnoordhuis · 2017-12-01T10:19:20Z

test/parallel/test-cluster-fork-windowsHide.js

+    process.stdout.write(data);
+  });
+  process.on('disconnect', function() {
+    process.stdin.pause();


Can you explain why you call .pause() here?

The stdin keeps the worker from exiting. That's the only way I know to get rid of it.

mscdex · 2017-12-01T12:47:45Z

Shouldn't the test actually check that the console window doesn't exist somehow (e.g. using Powershell)?

mscdex · 2017-12-01T12:48:25Z

Also, the test could probably be skipped on non-Windows platforms.

bnoordhuis · 2017-12-01T13:13:04Z

I'd say it's good to test that windowsHide doesn't have an adverse effect on other platforms.

cjihrig · 2017-12-01T13:32:53Z

test/parallel/test-cluster-fork-windowsHide.js

+  process.stdin.on('data', (data) => {
+    process.stdout.write(data);
+  });
+  process.on('disconnect', function() {


It doesn't really matter, just curious why you used a function here but arrow functions everywhere else.

My bad. A little careless. I wasn't expecting the stdin would block the process from exiting at the beginning, and put the disconnect handler a little later, and not notice the coding style difference.

toddwong · 2017-12-01T13:34:19Z

@mscdex I must confess that this test is just for "formally"(sorry about my English, can't find a proper word...) completeness according to the PR requirements (tests and/or benchmarks are included).

Actually, I don't know how to automatically test it at all, and very interested in advises.

@bnoordhuis So my test actually useful I think.

PS: Thanks for reviewing.

toddwong · 2017-12-01T16:11:50Z

Just polished the test code a little bit according to the above reviews

bnoordhuis

LGTM, thanks. CI: https://ci.nodejs.org/job/node-test-pull-request/11845/

If there is a way to test with PS that the windows aren't actually created that would be even more awesome, but I have no idea how you'd test that.

toddwong · 2017-12-02T08:44:09Z

@bnoordhuis Just checked "test/parallel/test-child-process-windows-hide.js" and got some inspiration. See the commit 242d80c .

The idea is that, we just make sure the windowsHide option is passed to the underlying child_process.fork function correctly. And child_process.fork will make sure it passed to libuv finally. And libuv may do the real test for us (Not sure if/how it does, though).

bnoordhuis · 2017-12-02T12:34:52Z

What @mscdex was getting at is using some powershell-fu to check if the worker has a console window associated with it. I'm by no means a PS guru but I expect it would look something like this:

const output = child_process.execSync(
    `powershell -ExecutionPolicy Unrestricted -NoProfile  -c ` +
    `"Get-Process -Id ${worker.process.pid} | where ..."`)
    .toString('utf8');

Where ... checks if the Process object has a window associated with it or not. I don't know what the right property is but, given the plethora of Process properties, there must be one.

bnoordhuis · 2017-12-02T12:36:40Z

And libuv may do the real test for us (Not sure if/how it does, though).

No, libuv just passes it on to the OS.

toddwong · 2017-12-02T13:17:14Z

@bnoordhuis Just checked, there is a MainWindowHandle property. But there is another problem, the console windows won't be there for service processes even if they are spawned with windowsHide: false. Not sure if the CI runs as a service or not.

bnoordhuis · 2017-12-02T13:41:31Z

I'm reasonably sure the CI runs tests as a regular process and user.

Fixes: nodejs#17370

toddwong · 2017-12-02T16:09:46Z

@bnoordhuis @mscdex Added the fancy PowerShell thing! 😃

mscdex · 2017-12-02T17:09:35Z

test/parallel/test-cluster-fork-windowsHide.js

+    process.send({ type: 'workerOnline' });
+
+    let output = '0';
+    if (process.platform === 'win32')


Braces please for multi-line bodies.

bnoordhuis

One suggestion, otherwise nice work.

bnoordhuis · 2017-12-03T10:31:10Z

test/parallel/test-cluster-fork-windowsHide.js

+
+let patchedFork = child_process.fork;
+child_process.fork = (...args) => patchedFork.apply(this, args);
+const cluster = require('cluster');


You can get rid of the monkey-patch now because ultimately it doesn't how matter how the option is passed down, as long as the observable effect is the same.

Presumably the child_process.spawn() call can go too? I admit I don't quite understand why you run the tests in a child process.

I keep the monkey-patch just in case some one run the tests on other platforms or in a windows service session. Not sure if this is necessary or practically useful.

It seems Windows only allocate a new console window for an attaching process spawned by a detached process.

If process D is spawned by process C with detached: true , and process W is spawned by process D with detached: false, W will get a new black console window popped up.

if D is spawned by C with detached: false or W is spawned by D with detached: true, no console window will pop up for W.

C = Command, D = Daemon, W = worker, practically.

That's why the test need to be run in a (detached) child process.

@bnoordhuis Is this OK?

I keep the monkey-patch just in case some one run the tests on other platforms or in a windows service session. Not sure if this is necessary or practically useful.

Neither, I'd say. :-)

Can you add a comment to the test that explains why it spawns a child process first? I get it now from your explanation but it's not obvious from just looking at the code.

Sure, i'll add the comment, and remove the monkey-patch thing.

BridgeAR · 2017-12-12T22:26:49Z

@mscdex @bnoordhuis @cjihrig would you mind to take another look? It seems like all comments got addressed.

toddwong · 2017-12-14T03:50:46Z

Thanks, guys. Sorry I forgot leave a message for the last commit.

toddwong · 2017-12-29T02:01:54Z

Hi, guys,

It's been a while. Would this be landed any soon?

bnoordhuis · 2017-12-29T12:03:17Z

Sorry for the delay. New CI because the old one is inaccessible and I've added the 'ready' label.

https://ci.nodejs.org/job/node-test-commit/15130/

toddwong · 2018-01-05T11:15:41Z

Thanks!

Fixes: #17370 PR-URL: #17412 Fixes: #17370 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins · 2018-01-24T09:22:03Z

Should this have been semver minor?

Refs: nodejs#17412

PR-URL: #19578 Refs: #17412 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

Fixes: #17370 PR-URL: #17412 Fixes: #17370 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: #19578 Refs: #17412 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

Fixes: #17370 PR-URL: #17412 Fixes: #17370 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: #19578 Refs: #17412 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) #21592 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) nodejs#18633 - remove runtime deprecation (Ali Ijaz Sheikh) nodejs#19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) nodejs#18513 * cluster: - add cwd to cluster.settings (cjihrig) nodejs#18399 - support windowsHide option for workers (Todd Wong) nodejs#17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) nodejs#18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) nodejs#21592 - upgrade libuv to 1.19.2 (cjihrig) nodejs#18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) nodejs#21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) nodejs#18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) nodejs#19408 * http, http2: - add options to http.createServer() (Peter Marton) nodejs#15752 - add 103 Early Hints status code (Yosuke Furukawa) nodejs#16644 - add http fallback options to .createServer (Peter Marton) nodejs#15752 * n-api: - take n-api out of experimental (Michael Dawson) nodejs#19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) nodejs#18087 * src: - add public API for managing NodePlatform (Cheng Zhao) nodejs#16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) nodejs#17600 - node internals' postmortem metadata (Matheus Marchini) nodejs#14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) nodejs#19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) nodejs#18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) nodejs#18186 PR-URL: nodejs#21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.4.1 (Kat Marchán) #22591 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) #21592 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

nodejs-github-bot added the cluster Issues and PRs related to the cluster subsystem. label Dec 1, 2017

bnoordhuis approved these changes Dec 1, 2017

View reviewed changes

cjihrig approved these changes Dec 1, 2017

View reviewed changes

toddwong force-pushed the hwcicf branch from 4e07b0e to 0b9373a Compare December 1, 2017 16:09

bnoordhuis approved these changes Dec 1, 2017

View reviewed changes

cluster: support windowsHide option for workers

45134fb

Fixes: nodejs#17370

toddwong force-pushed the hwcicf branch from d4e3f43 to 45134fb Compare December 2, 2017 16:06

mscdex reviewed Dec 2, 2017

View reviewed changes

fixup! cluster: support windowsHide option for workers

e8d752e

bnoordhuis reviewed Dec 3, 2017

View reviewed changes

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

fixup! cluster: support windowsHide option for workers

cc4081a

BridgeAR approved these changes Dec 12, 2017

View reviewed changes

jasnell approved these changes Dec 12, 2017

View reviewed changes

bnoordhuis added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 29, 2017

BridgeAR closed this Jan 5, 2018

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

TimothyGu removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 13, 2018

MylesBorins added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 24, 2018

tniessen added a commit to tniessen/node that referenced this pull request Mar 24, 2018

doc: add missing metadata for settings.windowsHide

82dcee3

Refs: nodejs#17412

tniessen mentioned this pull request Mar 24, 2018

doc: add missing metadata for cluster.settings.windowsHide #19578

Closed

2 tasks

MylesBorins mentioned this pull request Aug 16, 2018

v8.12.0 proposal #21593

Merged

cluster: support windowsHide option for workers #17412

cluster: support windowsHide option for workers #17412

Conversation

toddwong commented Dec 1, 2017

Checklist

Affected core subsystem(s)

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Dec 1, 2017

mscdex commented Dec 1, 2017

bnoordhuis commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toddwong commented Dec 1, 2017

toddwong commented Dec 1, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

toddwong commented Dec 2, 2017

bnoordhuis commented Dec 2, 2017

bnoordhuis commented Dec 2, 2017

toddwong commented Dec 2, 2017 • edited Loading

bnoordhuis commented Dec 2, 2017

toddwong commented Dec 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toddwong Dec 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Dec 12, 2017

toddwong commented Dec 14, 2017

toddwong commented Dec 29, 2017

bnoordhuis commented Dec 29, 2017 • edited Loading

toddwong commented Jan 5, 2018

MylesBorins commented Jan 24, 2018

toddwong commented Dec 2, 2017 •

edited

Loading

toddwong Dec 3, 2017 •

edited

Loading

bnoordhuis commented Dec 29, 2017 •

edited

Loading