FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

cluster: send suicide message on disconnect by cjihrig · Pull Request #3720 · nodejs/node · GitHub

/ node Public

cluster: send suicide message on disconnect - #3720

Closed
cjihrig wants to merge 1 commit into
nodejs:masterfrom
cjihrig:3238
ConversationCommits (1)ChecksFiles changed
Closed

cluster: send suicide message on disconnect#3720
cjihrig wants to merge 1 commit into
nodejs:masterfrom
cjihrig:3238

Conversation

cjihrig commented Nov 9, 2015

Copy link
Copy Markdown
Contributor

This commit causes Worker.prototype.disconnect() to send a suicide message to the cluster master. The function is also restructured to eliminate redundant code.

Fixes: #3238

jasnell commented Nov 9, 2015

Copy link
Copy Markdown
Member

Ok with the idea. Using the term suicide likely isn't the best choice, however.

jasnell added semver-minor PRs that contain new features and should be released in the next minor version. cluster Issues and PRs related to the cluster subsystem. labels Nov 9, 2015

cjihrig commented Nov 9, 2015

Copy link
Copy Markdown
Contributor Author

suicide is an existing term, used throughout the code. Changing it would be a semver-major bump.

jasnell commented Nov 9, 2015

Copy link
Copy Markdown
Member

Yeah, I was just looking at that. That's unfortunate but ok.

cjihrig commented Nov 9, 2015

Copy link
Copy Markdown
Contributor Author

Technically, this changes behavior, but it brings it back into line with what is documented (see #3238). I would say it is semver-patch.

MylesBorins commented Nov 9, 2015

Copy link
Copy Markdown
Contributor

@jasnell suicide is definitely problematic... so is Master / Worker and a handful of other Nouns / Verbs throughout the codebase.

As mentioned, changing any of those would be an obvious semver major... it would also require a lot of churn in the ecosystem.

That being said, I think that the benefits to our ethics / ethos / community outweighs the potential technical debt.

If people are open to it we can open an issue to discuss this further.

mikeal commented Nov 9, 2015

Copy link
Copy Markdown
Contributor

we could alias 'suicide' to another word in the codebase to maintain reverse compatibility while moving to the new word and updating the docs. then it's a feature rather than compat break and stays semver-minor.

jasnell commented Nov 9, 2015

Copy link
Copy Markdown
Member

Yep. I had just forgotten that the term was already in use. I'm painfully aware of the various unfortunate other uses I just didn't want us to add another. I'm +1 on @mikeal's proposal.

MylesBorins mentioned this pull request Nov 9, 2015
Closed
bnoordhuis reviewed Nov 9, 2015
View reviewed changes
Comment thread test/parallel/test-regress-GH-3238.js Outdated

bnoordhuis Nov 9, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you wrap the event listeners in common.mustCall(...)?

bnoordhuis Nov 9, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Aside: perhaps it's a good idea to check the order of events as well?

jasnell commented Nov 10, 2015

Copy link
Copy Markdown
Member

@cjihrig ... ah, ok, so this is a regression fix. Grr... ok. Agree with semver-patch then. I would recommend expanding the description in the commit message to describe the regression.

jasnell removed the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 10, 2015
cluster: send suicide message on disconnect
c0de81b 
This commit causes Worker.prototype.disconnect() to send a
suicide message to the cluster master. The function is also
restructured to eliminate redundant code.

Fixes: nodejs#3238

cjihrig commented Nov 10, 2015

Copy link
Copy Markdown
Contributor Author

Updated with @bnoordhuis comments.

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

jasnell commented Nov 10, 2015

Copy link
Copy Markdown
Member

LGTM. Applicable for LTS? (assuming yes)

cjihrig commented Nov 10, 2015

Copy link
Copy Markdown
Contributor Author

I'd say yes. The documentation is incorrect without this change.

cjihrig commented Nov 11, 2015

Copy link
Copy Markdown
Contributor Author

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

cjihrig added a commit that referenced this pull request Nov 11, 2015
cluster: send suicide message on disconnect
f299d87 
This commit causes Worker.prototype.disconnect() to send a
suicide message to the cluster master. The function is also
restructured to eliminate redundant code.

Fixes: #3238
PR-URL: #3720
Reviewed-By: James M Snell <jasnell@gmail.com>

cjihrig commented Nov 11, 2015

Copy link
Copy Markdown
Contributor Author

Landed in f299d87. @evanlucas you may need to update your suicide replacement PR accordingly.

cjihrig closed this Nov 11, 2015
cjihrig deleted the 3238 branch November 11, 2015 16:22
cjihrig added a commit that referenced this pull request Nov 11, 2015
cluster: send suicide message on disconnect
47f3735 
This commit causes Worker.prototype.disconnect() to send a
suicide message to the cluster master. The function is also
restructured to eliminate redundant code.

Fixes: #3238
PR-URL: #3720
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 mentioned this pull request Nov 11, 2015
Merged

evanlucas commented Nov 11, 2015

Copy link
Copy Markdown
Contributor

Will do. Thanks for the heads up

cjihrig added a commit that referenced this pull request Nov 17, 2015
cluster: send suicide message on disconnect
e86817c 
This commit causes Worker.prototype.disconnect() to send a
suicide message to the cluster master. The function is also
restructured to eliminate redundant code.

Fixes: #3238
PR-URL: #3720
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins commented Nov 17, 2015

Copy link
Copy Markdown
Contributor

landed in v4.x-staging in e86817c

rvagg mentioned this pull request Nov 17, 2015
Closed
cjihrig added a commit that referenced this pull request Dec 4, 2015
cluster: send suicide message on disconnect
bf41a97 
This commit causes Worker.prototype.disconnect() to send a
suicide message to the cluster master. The function is also
restructured to eliminate redundant code.

Fixes: #3238
PR-URL: #3720
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell mentioned this pull request Dec 17, 2015
Closed
cjihrig added a commit that referenced this pull request Dec 17, 2015
cluster: send suicide message on disconnect
f5c5e8b 
This commit causes Worker.prototype.disconnect() to send a
suicide message to the cluster master. The function is also
restructured to eliminate redundant code.

Fixes: #3238
PR-URL: #3720
Reviewed-By: James M Snell <jasnell@gmail.com>
cjihrig added a commit that referenced this pull request Dec 23, 2015
cluster: send suicide message on disconnect
160702c 
This commit causes Worker.prototype.disconnect() to send a
suicide message to the cluster master. The function is also
restructured to eliminate redundant code.

Fixes: #3238
PR-URL: #3720
Reviewed-By: James M Snell <jasnell@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

cluster Issues and PRs related to the cluster subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL