Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cluster: return worker reference from disconnect() #10019

Closed
wants to merge 3 commits into from

Conversation

stv8
Copy link
Contributor

@stv8 stv8 commented Dec 1, 2016

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);

@nodejs-github-bot nodejs-github-bot added the cluster Issues and PRs related to the cluster subsystem. label Dec 1, 2016
@stv8
Copy link
Contributor Author

stv8 commented Dec 1, 2016

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

@sam-github

@@ -40,7 +40,8 @@ if (cluster.isWorker) {

// Disconnect worker when it is ready
worker.once('listening', common.mustCall(() => {
worker.disconnect();
const w = worker.disconnect();
assert.strictEqual(worker, w, 'did not return a reference');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@mscdex mscdex Dec 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@imyller imyller added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Dec 1, 2016
@@ -257,6 +257,8 @@ It is not emitted in the worker.
added: v0.7.7
-->

* Returns: {Worker} A reference to worker
Copy link
Contributor

@cjihrig cjihrig Dec 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

EDIT: And add a period at the end.

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stv8
Copy link
Contributor Author

stv8 commented Dec 5, 2016

@cjihrig changes added

@jasnell
Copy link
Member

jasnell commented Dec 5, 2016

@sam-github
Copy link
Contributor

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?

@Fishrock123
Copy link
Contributor

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

@cjihrig
Copy link
Contributor

cjihrig commented Dec 6, 2016

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

@cjihrig
Copy link
Contributor

cjihrig commented Dec 6, 2016

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

@sam-github
Copy link
Contributor

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

@stv8
Copy link
Contributor Author

stv8 commented Dec 6, 2016

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

@stv8
Copy link
Contributor Author

stv8 commented Dec 6, 2016

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.

@cjihrig
Copy link
Contributor

cjihrig commented Dec 6, 2016

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.

@stv8
Copy link
Contributor Author

stv8 commented Dec 6, 2016

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

@Trott
Copy link
Member

Trott commented Dec 7, 2016

Labeling semver-minor

@Trott Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 7, 2016
@italoacasas italoacasas added the wip Issues and PRs that are still a work in progress. label Dec 7, 2016
@italoacasas
Copy link
Contributor

ping @stv8

@stv8
Copy link
Contributor Author

stv8 commented Dec 15, 2016

@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
@cjihrig
Copy link
Contributor

cjihrig commented Dec 16, 2016

Thanks @stv8. Newly added check LGTM.

@italoacasas
Copy link
Contributor

@sam-github
Copy link
Contributor

Landed in 5d14602, thank you.

@sam-github sam-github closed this Dec 19, 2016
sam-github pushed a commit that referenced this pull request Dec 19, 2016
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 <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
cjihrig pushed a commit to cjihrig/node that referenced this pull request Dec 20, 2016
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 <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
@italoacasas italoacasas mentioned this pull request Dec 20, 2016
cjihrig added a commit to cjihrig/node that referenced this pull request Dec 20, 2016
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
cjihrig added a commit to cjihrig/node that referenced this pull request Dec 20, 2016
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
cjihrig pushed a commit that referenced this pull request Dec 20, 2016
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 <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
cjihrig added a commit that referenced this pull request Dec 20, 2016
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
imyller added a commit to imyller/meta-nodejs that referenced this pull request Dec 21, 2016
    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 <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 16, 2017
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 <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
@MylesBorins
Copy link
Contributor

MylesBorins commented May 16, 2017

backported in ae95a3b

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

@MylesBorins MylesBorins reopened this May 16, 2017
MylesBorins pushed a commit that referenced this pull request May 18, 2017
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 <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 23, 2017
MylesBorins added a commit that referenced this pull request Jun 6, 2017
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
MylesBorins added a commit that referenced this pull request Jun 6, 2017
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
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
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 <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cluster Issues and PRs related to the cluster subsystem. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. semver-minor PRs that contain new features and should be released in the next minor version. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.