GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||
GeezerVison is showing github.com — cluster: return worker reference from disconnect() by stv8 · Pull Request #10019 · nodejs/node Conversation
Contributor
Checklist
Affected core subsystem(s)cluster Description of changeChanges disconnect() to return a refererence to the worker.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
Author
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
mscdex
reviewed
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
|
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a nit. CI: https://ci.nodejs.org/job/node-test-pull-request/5141/
Sorry, something went wrong.
There was an error while loading. Please reload this page.
1913e7f to
7c5d36a
Compare
|
@cjihrig changes added |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
New CI https://ci.nodejs.org/job/node-test-pull-request/5222/ |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Its hard to tell from the diff, but you modified the return value of two functions in |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@sam-github Nice catch... it looks like we are discarding the first one? Isn't it just overwritten? Looks like a bug to me. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
One is the master implementation and one is the worker implementation, right? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
But it does look like the tests only cover the master case. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Yes, looks to me like the tests only check the master's disconnect, and don't check the worker's. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Nice catch, I will see if I can add a test for the worker as well. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Actually on a second glance, doesn't the That instance of https://github.com/nodejs/node/blob/master/lib/cluster.js#L407 Let me know if I am overlooking something. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Oh I see. Cool I'll see if I can add another for the non-master case. Thanks |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Labeling |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
ping @stv8 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@italoacasas sorry got caught up in some things, should have the test today |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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
7c5d36a to
14f58fd
Compare
|
Thanks @stv8. Newly added check LGTM. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
CI: https://ci.nodejs.org/job/node-test-pull-request/5455/console |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Landed in 5d14602, thank you. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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]>
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]>
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
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 <[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]>
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 <[email protected]>
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]>
There was an error while loading. Please reload this page.
|
backported in ae95a3b edit: LOL I've been doing this for too long tonight... I shouldn't have even written this. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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]>
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
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 <[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]>
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
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.