Skip to content

cluster: send suicide message on disconnect#3720

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.

Sign up for GitHub

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

Jump to bottom
Closed
cjihrig wants to merge 1 commit into from
Closed

cluster: send suicide message on disconnect #3720

cjihrig wants to merge 1 commit into from
+31 −12

Conversation

@cjihrig
Copy link
Contributor

@cjihrigcjihrig commented Nov 9, 2015

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
Copy link
Member

jasnell commented Nov 9, 2015

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

@jasnelljasnell 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
Copy link
ContributorAuthor

cjihrig commented Nov 9, 2015

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

@jasnell
Copy link
Member

jasnell commented Nov 9, 2015

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

@cjihrig
Copy link
ContributorAuthor

cjihrig commented Nov 9, 2015

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

MylesBorins commented Nov 9, 2015

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

mikeal commented Nov 9, 2015

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
Copy link
Member

jasnell commented Nov 9, 2015

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.

@MylesBorinsMylesBorins mentioned this pull request Nov 9, 2015
Closed
bnoordhuis
bnoordhuis reviewed Nov 9, 2015
View reviewed changes
test/parallel/test-regress-GH-3238.js Outdated
Copy link
Member

@bnoordhuisbnoordhuisNov 9, 2015

Choose a reason for hiding this comment

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

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

Copy link
Member

@bnoordhuisbnoordhuisNov 9, 2015

Choose a reason for hiding this comment

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

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

@jasnell
Copy link
Member

jasnell commented Nov 10, 2015

@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.

@jasnelljasnell removed the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 10, 2015
@cjihrig
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
Copy link
ContributorAuthor

cjihrig commented Nov 10, 2015

Updated with @bnoordhuis comments.

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

@jasnell
Copy link
Member

jasnell commented Nov 10, 2015

LGTM. Applicable for LTS? (assuming yes)

@cjihrig
Copy link
ContributorAuthor

cjihrig commented Nov 10, 2015

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

@cjihrig
Copy link
ContributorAuthor

cjihrig commented Nov 11, 2015

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

cjihrig added a commit that referenced this pull request Nov 11, 2015
@cjihrig
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 <[email protected]>
@cjihrig
Copy link
ContributorAuthor

cjihrig commented Nov 11, 2015

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

@cjihrigcjihrig closed this Nov 11, 2015
@cjihrigcjihrig deleted the 3238 branch November 11, 2015 16:22
cjihrig added a commit that referenced this pull request Nov 11, 2015
@cjihrig@Fishrock123
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 <[email protected]>
@Fishrock123Fishrock123 mentioned this pull request Nov 11, 2015
Merged
@evanlucas
Copy link
Contributor

evanlucas commented Nov 11, 2015

Will do. Thanks for the heads up

cjihrig added a commit that referenced this pull request Nov 17, 2015
@cjihrig
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 <[email protected]>
@MylesBorins
Copy link
Contributor

MylesBorins commented Nov 17, 2015

landed in v4.x-staging in e86817c

@rvaggrvagg mentioned this pull request Nov 17, 2015
Closed
cjihrig added a commit that referenced this pull request Dec 4, 2015
@cjihrig@rvagg
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 <[email protected]>
@jasnelljasnell mentioned this pull request Dec 17, 2015
Closed
cjihrig added a commit that referenced this pull request Dec 17, 2015
@cjihrig@jasnell
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 <[email protected]>
cjihrig added a commit that referenced this pull request Dec 23, 2015
@cjihrig@jasnell
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 <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

clusterIssues 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

@cjihrig@jasnell@MylesBorins@mikeal@evanlucas@bnoordhuis