cluster: return worker reference from disconnect() by stv8 · Pull Request #10019 · nodejs/node · GitHub

Checklist

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

Affected core subsystem(s)

cluster

Description of change

Changes disconnect() to return a refererence to the worker.
This will enable method chaining such as
worker.disconnect().once('disconnect', doThis);

This issue was pulled from nodejs/code-and-learn#60

@sam-github

I'm not sure this is needed here, I think it's best left to actual tests.

@mscdex are you suggesting that it doesn't need to be tested or to pull it out into its own test?

You've already made changes to the test file, that's good enough IMHO. I believe these changes here can be removed.

Oops, sorry, disregard my comments for this file. I did not see it is a test file.

No worries :) Do you think the assertion is only needed in one test file instead of two?

I don't think it matters either way, as long as there's at least one check for it.

Can you add backticks around worker (the second one).

EDIT: And add a period at the end.

LGTM with a nit. CI: https://ci.nodejs.org/job/node-test-pull-request/5141/

@cjihrig changes added

New CI https://ci.nodejs.org/job/node-test-pull-request/5222/

Its hard to tell from the diff, but you modified the return value of two functions in cluster.js, are you certain that both call's return values are covered by the tests?

@sam-github Nice catch... it looks like we are discarding the first one? Isn't it just overwritten? Looks like a bug to me.

One is the master implementation and one is the worker implementation, right?

But it does look like the tests only cover the master case.

Yes, looks to me like the tests only check the master's disconnect, and don't check the worker's.

Nice catch, I will see if I can add a test for the worker as well.

Actually on a second glance, doesn't the assert inside of the test/parallel/test-cluster-worker-init.js test take care of the worker case?

That instance of worker is a result from a call to cluster.fork(), and it looks to me that fork() returns a new worker instance. Is that right?

https://github.com/nodejs/node/blob/master/lib/cluster.js#L407

Let me know if I am overlooking something.

No. In order to cover the worker case, the assertion would need to come from the worker process. In that test, the assertion is in the cluster.isMaster branch.

Oh I see. Cool I'll see if I can add another for the non-master case. Thanks

Labeling semver-minor

ping @stv8

@italoacasas sorry got caught up in some things, should have the test today

Changes disconnect() to return a refererence to the worker. This will enable method chaining such as "worker.disconnect().once('disconnect', doThis);"

Add minor changes to docs

Add test for work in addition to tests for master

Thanks @stv8. Newly added check LGTM.

CI: https://ci.nodejs.org/job/node-test-pull-request/5455/console

Landed in 5d14602, thank you.

Changes disconnect() to return a refererence to the worker. This will enable method chaining such as worker.disconnect().once('disconnect', doThis); PR-URL: #10019 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

Changes disconnect() to return a refererence to the worker. This will enable method chaining such as worker.disconnect().once('disconnect', doThis); PR-URL: nodejs#10019 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

Notable changes: * buffer: - buffer.fill() now works properly for the UCS2 encoding on Big-Endian machines. (Anna Henningsen) nodejs#9837 * cluster: - disconnect() now returns a reference to the disconnected worker. (Sean Villars) nodejs#10019 * crypto: - The built-in list of Well-Known CAs (Certificate Authorities) can now be extended via a NODE_EXTRA_CA_CERTS environment variable. (Sam Roberts) nodejs#9139 * http: - Remove stale timeout listeners in order to prevent a memory leak when using keep alive. (Karl Böhlmark) nodejs#9440 * tls: - Allow obvious key/passphrase combinations. (Sam Roberts) nodejs#10294 * url: - Including base argument in URL.originFor() to meet specification compliance. (joyeecheung) nodejs#10021 - Improve URLSearchParams to meet specification compliance. (Timothy Gu) nodejs#9484 PR-URL: nodejs#10277

Changes disconnect() to return a refererence to the worker. This will enable method chaining such as worker.disconnect().once('disconnect', doThis); PR-URL: #10019 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

Notable changes: * buffer: - buffer.fill() now works properly for the UCS2 encoding on Big-Endian machines. (Anna Henningsen) #9837 * cluster: - disconnect() now returns a reference to the disconnected worker. (Sean Villars) #10019 * crypto: - The built-in list of Well-Known CAs (Certificate Authorities) can now be extended via a NODE_EXTRA_CA_CERTS environment variable. (Sam Roberts) #9139 * http: - Remove stale timeout listeners in order to prevent a memory leak when using keep alive. (Karl Böhlmark) #9440 * tls: - Allow obvious key/passphrase combinations. (Sam Roberts) #10294 * url: - Including base argument in URL.originFor() to meet specification compliance. (joyeecheung) #10021 - Improve URLSearchParams to meet specification compliance. (Timothy Gu) #9484 PR-URL: #10277

Notable changes: * buffer: - buffer.fill() now works properly for the UCS2 encoding on Big-Endian machines. (Anna Henningsen) nodejs/node#9837 * cluster: - disconnect() now returns a reference to the disconnected worker. (Sean Villars) nodejs/node#10019 * crypto: - The built-in list of Well-Known CAs (Certificate Authorities) can now be extended via a NODE_EXTRA_CA_CERTS environment variable. (Sam Roberts) nodejs/node#9139 * http: - Remove stale timeout listeners in order to prevent a memory leak when using keep alive. (Karl Bohlmark) nodejs/node#9440 * tls: - Allow obvious key/passphrase combinations. (Sam Roberts) nodejs/node#10294 * url: - Including base argument in URL.originFor() to meet specification compliance. (joyeecheung) nodejs/node#10021 - Improve URLSearchParams to meet specification compliance. (Timothy Gu) nodejs/node#9484 PR-URL: nodejs/node#10277 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

Changes disconnect() to return a refererence to the worker. This will enable method chaining such as worker.disconnect().once('disconnect', doThis); PR-URL: #10019 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

backported in ae95a3b

edit: LOL I've been doing this for too long tonight... I shouldn't have even written this.

Changes disconnect() to return a refererence to the worker. This will enable method chaining such as worker.disconnect().once('disconnect', doThis); PR-URL: #10019 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

This LTS release comes with 126 commits. This includes 40 which are test related, 32 which are doc related, 12 which are build / tool related and 4 commits which are updates to dependencies. Notable Changes: * build: - support for building mips64el (nanxiongchao) #10991 * cluster: - disconnect() now returns a reference to the disconnected worker. (Sean Villars) #10019 * crypto: - ability to select cert store at runtime (Adam Majer) #8334 - Use system CAs instead of using bundled ones (Adam Majer) #8334 - The `Decipher` methods `setAuthTag()` and `setAAD` now return `this`. (Kirill Fomichev) #9398 - adding support for OPENSSL_CONF again (Sam Roberts) #11006 - make LazyTransform compabile with Streams1 (Matteo Collina) #12380 * deps: - upgrade libuv to 1.11.0 (cjihrig) #11094 - upgrade libuv to 1.10.2 (cjihrig) #10717 - upgrade libuv to 1.10.1 (cjihrig) #9647 - upgrade libuv to 1.10.0 (cjihrig) #9267 * dns: - Implemented `{ttl: true}` for `resolve4()` and `resolve6()` (Ben Noordhuis) #9296 * process: - add NODE_NO_WARNINGS environment variable (cjihrig) #10842 * readline: - add option to stop duplicates in history (Danny Nemer) #2982 * src: - support "--" after "-e" as end-of-options (John Barboza) #10651 * tls: - new tls.TLSSocket() supports sec ctx options (Sam Roberts) #11005 - Allow obvious key/passphrase combinations. (Sam Roberts) #10294 PR-URL: #13059

Changes disconnect() to return a refererence to the worker. This will enable method chaining such as worker.disconnect().once('disconnect', doThis); PR-URL: nodejs/node#10019 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

This LTS release comes with 126 commits. This includes 40 which are test related, 32 which are doc related, 12 which are build / tool related and 4 commits which are updates to dependencies. Notable Changes: * build: - support for building mips64el (nanxiongchao) nodejs/node#10991 * cluster: - disconnect() now returns a reference to the disconnected worker. (Sean Villars) nodejs/node#10019 * crypto: - ability to select cert store at runtime (Adam Majer) nodejs/node#8334 - Use system CAs instead of using bundled ones (Adam Majer) nodejs/node#8334 - The `Decipher` methods `setAuthTag()` and `setAAD` now return `this`. (Kirill Fomichev) nodejs/node#9398 - adding support for OPENSSL_CONF again (Sam Roberts) nodejs/node#11006 - make LazyTransform compabile with Streams1 (Matteo Collina) nodejs/node#12380 * deps: - upgrade libuv to 1.11.0 (cjihrig) nodejs/node#11094 - upgrade libuv to 1.10.2 (cjihrig) nodejs/node#10717 - upgrade libuv to 1.10.1 (cjihrig) nodejs/node#9647 - upgrade libuv to 1.10.0 (cjihrig) nodejs/node#9267 * dns: - Implemented `{ttl: true}` for `resolve4()` and `resolve6()` (Ben Noordhuis) nodejs/node#9296 * process: - add NODE_NO_WARNINGS environment variable (cjihrig) nodejs/node#10842 * readline: - add option to stop duplicates in history (Danny Nemer) nodejs/node#2982 * src: - support "--" after "-e" as end-of-options (John Barboza) nodejs/node#10651 * tls: - new tls.TLSSocket() supports sec ctx options (Sam Roberts) nodejs/node#11005 - Allow obvious key/passphrase combinations. (Sam Roberts) nodejs/node#10294 PR-URL: nodejs/node#13059

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

mscdex reviewed Dec 1, 2016

View reviewed changes

imyller added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Dec 1, 2016

cjihrig reviewed Dec 3, 2016

View reviewed changes

cjihrig approved these changes Dec 3, 2016

View reviewed changes

stv8 force-pushed the return_worker_reference branch from 1913e7f to 7c5d36a Compare December 5, 2016 19:45

santigimeno approved these changes Dec 5, 2016

View reviewed changes

Fishrock123 approved these changes Dec 6, 2016

View reviewed changes

Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 7, 2016

italoacasas added the wip Issues and PRs that are still a work in progress. label Dec 7, 2016

stv8 added 3 commits December 15, 2016 13:28

cluster: return worker reference from disconnect()

7ef5107

Changes disconnect() to return a refererence to the worker. This will enable method chaining such as "worker.disconnect().once('disconnect', doThis);"

cluster: Update markdown

a0af33c

Add minor changes to docs

cluster: add test for worker

14f58fd

Add test for work in addition to tests for master

stv8 force-pushed the return_worker_reference branch from 7c5d36a to 14f58fd Compare December 15, 2016 19:30

nodejs-github-bot added the lts-watch-v6.x label Dec 15, 2016

silverwind approved these changes Dec 16, 2016

View reviewed changes

sam-github approved these changes Dec 19, 2016

View reviewed changes

sam-github closed this Dec 19, 2016

italoacasas mentioned this pull request Dec 20, 2016

v7.3.0 proposal #10277

Merged

gibfahn mentioned this pull request Apr 24, 2017

Potential Semver Minor Backports nodejs/Release#188

Closed

31 tasks

MylesBorins reopened this May 16, 2017

MylesBorins closed this May 16, 2017

MylesBorins mentioned this pull request May 23, 2017

v6.11.0 proposal #13059

Merged

MylesBorins added land-on-v6.x and removed lts-watch-v6.x labels Sep 19, 2017

cluster: return worker reference from disconnect() by stv8 · Pull Request #10019 · nodejs/node · GitHub

Uh oh!

Uh oh!

cluster: return worker reference from disconnect() - #10019

cluster: return worker reference from disconnect()#10019stv8 wants to merge 3 commits intonodejs:masternodejs/node:masterfrom stv8:return_worker_referencestv8/node:return_worker_referenceCopy head branch name to clipboard

Conversation

stv8 commented Dec 1, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

stv8 commented Dec 1, 2016

Uh oh!

mscdex Dec 1, 2016

Choose a reason for hiding this comment

Uh oh!

stv8 Dec 1, 2016

Choose a reason for hiding this comment

Uh oh!

mscdex Dec 1, 2016

Choose a reason for hiding this comment

Uh oh!

mscdex Dec 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stv8 Dec 1, 2016

Choose a reason for hiding this comment

Uh oh!

mscdex Dec 1, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrig Dec 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

stv8 commented Dec 5, 2016

Uh oh!

jasnell commented Dec 5, 2016

Uh oh!

sam-github commented Dec 6, 2016

Uh oh!

Fishrock123 commented Dec 6, 2016

Uh oh!

cjihrig commented Dec 6, 2016

Uh oh!

cjihrig commented Dec 6, 2016

Uh oh!

sam-github commented Dec 6, 2016

Uh oh!

stv8 commented Dec 6, 2016

Uh oh!

stv8 commented Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjihrig commented Dec 6, 2016

Uh oh!

stv8 commented Dec 6, 2016

Uh oh!

Trott commented Dec 7, 2016

Uh oh!

italoacasas commented Dec 15, 2016

Uh oh!

stv8 commented Dec 15, 2016

Uh oh!

cjihrig commented Dec 16, 2016

Uh oh!

italoacasas commented Dec 17, 2016

Uh oh!

sam-github commented Dec 19, 2016

Uh oh!

MylesBorins commented May 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

cluster: return worker reference from disconnect()#10019
stv8 wants to merge 3 commits into
nodejs:masterfrom
stv8:return_worker_reference

mscdex Dec 1, 2016 •
edited
Loading

cjihrig Dec 3, 2016 •
edited
Loading

stv8 commented Dec 6, 2016 •
edited
Loading

MylesBorins commented May 16, 2017 •
edited
Loading