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.

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

cluster: send suicide message on disconnect #3720

Closed
wants to merge 1 commit into from
Closed

cluster: send suicide message on disconnect #3720

wants to merge 1 commit into from

Conversation

cjihrig
Copy link
Contributor

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

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

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
Contributor Author

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

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

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

worker.on('exit', function() {
assert.strictEqual(worker.suicide, true);
});
Copy link
Member

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

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.

@jasnell jasnell 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
Contributor Author

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
Contributor Author

cjihrig commented Nov 10, 2015

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

@cjihrig
Copy link
Contributor Author

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 <jasnell@gmail.com>
@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 11, 2015

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

@cjihrig cjihrig closed this Nov 11, 2015
@cjihrig cjihrig 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 <jasnell@gmail.com>
@Fishrock123 Fishrock123 mentioned this pull request Nov 11, 2015
Merged
@evanlucas
Copy link
Contributor

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 <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

landed in v4.x-staging in e86817c

@rvagg rvagg 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 <jasnell@gmail.com>
@jasnell jasnell 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 <jasnell@gmail.com>
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 <jasnell@gmail.com>
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
@cjihrig @jasnell @MylesBorins @mikeal @evanlucas @bnoordhuis