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: refine worker.destroy function #6502

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@yorkie
Member

yorkie commented May 1, 2016

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)
  • cluster
Description of change

This PR just refines a cluster function and make the duplicated exit functions to be shared, so there is no tests or benchmarks could be provided :-)

@addaleax addaleax added the cluster label May 1, 2016

Show outdated Hide outdated lib/cluster.js
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell May 1, 2016

Member

Generally LGTM with a nit and if CI is green: https://ci.nodejs.org/job/node-test-pull-request/2451/

Member

jasnell commented May 1, 2016

Generally LGTM with a nit and if CI is green: https://ci.nodejs.org/job/node-test-pull-request/2451/

@yorkie

This comment has been minimized.

Show comment
Hide comment
@yorkie

yorkie May 1, 2016

Member

@jasnell fixed the nit, could you help me run a new job, thank you :-)

Member

yorkie commented May 1, 2016

@jasnell fixed the nit, could you help me run a new job, thank you :-)

Show outdated Hide outdated lib/cluster.js
@jasnell

This comment has been minimized.

Show comment
Hide comment
Show outdated Hide outdated lib/cluster.js
@yorkie

This comment has been minimized.

Show comment
Hide comment
@yorkie

yorkie May 1, 2016

Member

Fixed up, thank you @cjihrig @jasnell I thought the default is 0.

Member

yorkie commented May 1, 2016

Fixed up, thank you @cjihrig @jasnell I thought the default is 0.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell May 1, 2016

Member

The default is 0 unless process.exitCode is set, if it is, then just calling process.exit() will use it's value instead of 0. in other words, process.exitCode = 4; process.exit() would exit with 4 and not 0. It's fairly non-obvious

Member

jasnell commented May 1, 2016

The default is 0 unless process.exitCode is set, if it is, then just calling process.exit() will use it's value instead of 0. in other words, process.exitCode = 4; process.exit() would exit with 4 and not 0. It's fairly non-obvious

@yorkie

This comment has been minimized.

Show comment
Hide comment
@yorkie

yorkie May 1, 2016

Member

So why did we introduce the exitCode as you described it as non-obvious culprit? Do we really need to set the default exit code for user-land?

Member

yorkie commented May 1, 2016

So why did we introduce the exitCode as you described it as non-obvious culprit? Do we really need to set the default exit code for user-land?

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig May 1, 2016

Contributor

LGTM

Contributor

cjihrig commented May 1, 2016

LGTM

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell
Member

jasnell commented May 1, 2016

@yorkie

This comment has been minimized.

Show comment
Hide comment
@yorkie

yorkie May 2, 2016

Member

CI has been green :-) /cc @jasnell @cjihrig

Member

yorkie commented May 2, 2016

CI has been green :-) /cc @jasnell @cjihrig

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig May 2, 2016

Contributor

Nice. Let's let this sit for another day or so in case anyone else wants to weigh in.

Contributor

cjihrig commented May 2, 2016

Nice. Let's let this sit for another day or so in case anyone else wants to weigh in.

@yorkie

This comment has been minimized.

Show comment
Hide comment
@yorkie
Member

yorkie commented May 3, 2016

Ping @cjihrig

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig May 3, 2016

Contributor

Thanks, landed in 4e905fa.

Contributor

cjihrig commented May 3, 2016

Thanks, landed in 4e905fa.

@cjihrig cjihrig closed this May 3, 2016

cjihrig added a commit that referenced this pull request May 3, 2016

cluster: remove use of bind() in destroy()
This commit replaces process.exit.bind() with an arrow function
in Worker.prototype.destroy().

PR-URL: #6502
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@yorkie yorkie deleted the yorkie:refine/cluster-worker branch May 3, 2016

@yorkie

This comment has been minimized.

Show comment
Hide comment
@yorkie

yorkie May 3, 2016

Member

:-) thanks for your advices too

Member

yorkie commented May 3, 2016

:-) thanks for your advices too

Fishrock123 added a commit that referenced this pull request May 4, 2016

cluster: remove use of bind() in destroy()
This commit replaces process.exit.bind() with an arrow function
in Worker.prototype.destroy().

PR-URL: #6502
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

joelostrowski added a commit to joelostrowski/node that referenced this pull request May 4, 2016

cluster: remove use of bind() in destroy()
This commit replaces process.exit.bind() with an arrow function
in Worker.prototype.destroy().

PR-URL: nodejs#6502
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Jun 2, 2016

Member

Added dont-land for lts. Please feel free to let me know if I am incorrect in making this assumption

Member

MylesBorins commented Jun 2, 2016

Added dont-land for lts. Please feel free to let me know if I am incorrect in making this assumption

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment