cluster: migrate from worker.suicide #3743

evanlucas · 2015-11-10T19:15:06Z

Replace it with worker.exitedAfterDisconnect. Print deprecation message when
getting or setting until it is removed.

Ref: #3721

MylesBorins · 2015-11-10T19:18:26Z

LGTM

MylesBorins · 2015-11-10T19:23:00Z

Test suite passes locally fwiw

JungMinu · 2015-11-10T19:25:59Z

doc/api/cluster.markdown

-      if (worker.suicide === true) {
-        console.log('Oh, it was just suicide\' – no need to worry').
+      if (worker.voluntaryExit === true) {
+        console.log('Oh, it was just voluntary\' – no need to worry').


please add exit, it was just voluntary -> it was just voluntary exit

JungMinu · 2015-11-10T19:26:17Z

LGTM with one comment

mikeal · 2015-11-10T19:29:33Z

"deprecate" is the wrong term here since you are ensuring exisiting compatibility. maybe "migrating" instead?

mikeal · 2015-11-10T19:30:25Z

also, maybe not print the deprecation warning until the next major release.

MylesBorins · 2015-11-10T19:30:46Z

lib/cluster.js

@@ -427,8 +442,8 @@ function masterInit() {
      queryServer(worker, message);
    else if (message.act === 'listening')
      listening(worker, message);
-    else if (message.act === 'suicide')


do you need to watch for the 'suicide' action here to guarantee compatibility?

since the message should only be internal, I don't think so. Unless you or someone else feel differently about it

makes sense to me 😄

jasnell · 2015-11-10T19:53:57Z

LGTM but we need to be clear about the impact of this, however. It is quite likely a semver-major change (any time we talk about changing event names and deprecating existing terms we likely have to consider it a semver-major). That means getting a deprecation notice into v5 first, then not landing the actual change until v6.

mikeal · 2015-11-10T19:55:00Z

@jasnell if it's backwards compatible why would it be a semver-major change?

jasnell · 2015-11-10T20:00:09Z

I said "likely" ;-). The one part of this change that is potentially not backwards compatible is the event that's being triggered (https://github.com/nodejs/node/pull/3743/files#diff-0faa53fc02580d5de2ebb484c41d691cR696). Specifically, are we reasonably certain that this isn't breaking?

Qard · 2015-11-10T20:04:02Z

A more boolean terminology like exitedVoluntarily might be a bit better. I could see someone thinking voluntaryExit is a function name.

evanlucas · 2015-11-10T20:05:43Z

@jasnell I believe that those messages are only sent to/from a cluster by the cluster and workers.

jasnell · 2015-11-10T20:06:50Z

Ok. If we're reasonably sure that it's not breaking, I'm good with semver-minor.

evanlucas · 2015-11-10T20:12:52Z

@Qard I'm fine with either. If you feel strongly about it, I will change it

dandean · 2015-11-10T20:45:02Z

lib/cluster.js

+      return this.voluntaryExit;
+    },
+    set: function(val) {
+      // TODO: Print deprecation message.


Should this message include information about when (date or version) to expect deprecation to become removal?

Should be slated for a major version. Honestly, we should just merge this and then open another PR right away with the deprecation warning since master is v6.

Should I just add a second commit? That way we can cherry-pick the first over to v5 and not the actual deprecation?

Fishrock123 · 2015-11-10T20:53:42Z

@ChALkeR I know you didn't feel for it, but could you run some npm analysis on the usage of this API if possible?

(Also cc @chrisdickinson)

Fishrock123 · 2015-11-10T20:54:52Z

As others have said I also think it may be worthwhile to think of a version of voluntaryExit that makes a little more sense as a boolean, if that's the kind of thing we want.

evanlucas · 2015-11-10T20:58:01Z

Although long, if that is the way we want to go, I think exitedVoluntarily is the best

Fishrock123 · 2015-11-10T21:03:26Z

So long as we can avoid naming like exitedVoluntarilyFromWorkerProcess ala what a certain other programming language is often blamed for I think we are mostly fine.

evanlucas · 2015-11-10T21:12:05Z

Updated to change to exitedVoluntarily. Also includes a commit that prints a deprecation warning.

mikeal · 2015-11-10T21:18:44Z

hrm... I may have misspoke. since we tag the PR to automatically do the version updates we may need two PRs for the two commits since one is semver-minor and one is semver-major.

ChALkeR · 2016-08-05T13:35:58Z

Btw, this wasn't backported to 4.x, was it? Perhaps it is too late now as 4.5 already got rcs (cc @thealphanerd), but if there would be 4.6 sometime, it could make sense to backport it the other way around and without the deprecation — i.e. make .exitedAfterDisconnect a getter/setter alias to .suicide in a semver-minor version of 4.x.

jasnell · 2016-08-05T13:37:11Z

+1 SGTM

On Friday, August 5, 2016, Сковорода Никита Андреевич <
notifications@github.com> wrote:

Btw, this wasn't backported to 4.x, was it? Perhaps it is too late now as
4.5 already got rcs, but if there would be 4.6 sometime, it could make
sense to backport it the other way around and without the deprecation —
i.e. make .exitedAfterDisconnect a getter/setter alias to .suicide.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#3743 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2eSlRqcZPIm26xbWiN2nT7qPSqJATks5qczxGgaJpZM4Gfr24
.

ChALkeR · 2016-08-05T13:43:54Z

Because given our LTS timeline, if we don't make 4.x support .exitedAfterDisconnect, then hard-deprecating .suicide until at least 10.x won't be reasonable, because that would force libraries to resort to detection mechanisms.

evanlucas · 2016-08-05T13:44:52Z

I'll submit a backport PR today

This is a backport of 4f619bd which migrates from worker.suicide to worker.exitedAfterDisconnect. Related: nodejs#3743

This is a backport of 4f619bd which migrates from worker.suicide to worker.exitedAfterDisconnect. Refs: #3743 PR-URL: #7998 Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

The defaul value of worker.exitedAfterDisconnect is undefined. If the worker dies on its own the value is changed to false and doesn't remains undefined. While the documentation just mentions 'Set by calling .kill() or .disconnect(). Until then, it is undefined.'. Making the default value as false in worker.js Fixes: nodejs#28837 Refs: nodejs#3743

Fixed the documentaion to reflect the changes in the default value of worker.exitedAfterDisconnect Fixes: nodejs#28837 Refs: nodejs#3743

The test checka for the exit code of the worker. cluster on exit must be called. And the value for exitedAfterDisconnect should be false Fixes: nodejs#28837 Refs: nodejs#3743

fixed the test that checked for default value undefined to false and renamed the test that checks for the value exitedAfterDisconnect if the worker exits on its own. Fixes: nodejs#28837 Refs: nodejs#3743

Fixed the documentation to reflect the changes in the default value of worker.exitedAfterDisconnect Fixes: nodejs#28837 Refs: nodejs#3743

Fixed the documentation to reflect the changes in the default value of worker.exitedAfterDisconnect PR-URL: #29404 Fixes: #28837 Refs: #3743 Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>

evanlucas mentioned this pull request Nov 10, 2015

replace suicide as a verb #3721

Closed

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

JungMinu reviewed Nov 10, 2015
View reviewed changes

MylesBorins reviewed Nov 10, 2015
View reviewed changes

evanlucas force-pushed the voluntary branch from 63a0c29 to 7975198 Compare November 10, 2015 19:36

dandean reviewed Nov 10, 2015
View reviewed changes

evanlucas force-pushed the voluntary branch from 7975198 to a45c4e2 Compare November 10, 2015 21:11

evanlucas changed the title ~~cluster: deprecate worker.suicide~~ cluster: migrate from worker.suicide Nov 10, 2015

evanlucas force-pushed the voluntary branch from a45c4e2 to 00ff007 Compare November 10, 2015 21:20

evanlucas mentioned this pull request Aug 6, 2016

cluster: alias suicide to exitedAfterDisconnect #7998

Closed

4 tasks

evanlucas added a commit to evanlucas/node that referenced this pull request Aug 7, 2016

cluster: alias suicide to exitedAfterDisconnect

d101576

This is a backport of 4f619bd which migrates from worker.suicide to worker.exitedAfterDisconnect. Related: nodejs#3743

sam-github mentioned this pull request Jun 14, 2017

cluster: remove deprecated problematic API #13684

Closed

4 tasks

nimit95 mentioned this pull request Jul 24, 2019

worker.exitedAfterDisconnect is false, after the worker process has exited using process.exit() #28837

Closed

nimit95 added a commit to nimit95/node that referenced this pull request Jul 26, 2019

doc: changing the documentation to reflect the change

830e98a

Fixed the documentaion to reflect the changes in the default value of worker.exitedAfterDisconnect Fixes: nodejs#28837 Refs: nodejs#3743

nimit95 added a commit to nimit95/node that referenced this pull request Sep 2, 2019

doc: exitedAfterDisconnect value can be false

4ae3d75

Fixed the documentation to reflect the changes in the default value of worker.exitedAfterDisconnect Fixes: nodejs#28837 Refs: nodejs#3743

nimit95 mentioned this pull request Sep 2, 2019

doc: exitedAfterDisconnect value can be false #29404

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cluster: migrate from worker.suicide #3743

cluster: migrate from worker.suicide #3743

evanlucas commented Nov 10, 2015 •

edited by tniessen

Loading

MylesBorins commented Nov 10, 2015

MylesBorins commented Nov 10, 2015

JungMinu Nov 10, 2015

JungMinu commented Nov 10, 2015

mikeal commented Nov 10, 2015

mikeal commented Nov 10, 2015

MylesBorins Nov 10, 2015

evanlucas Nov 10, 2015

MylesBorins Nov 10, 2015

jasnell commented Nov 10, 2015

mikeal commented Nov 10, 2015

jasnell commented Nov 10, 2015

Qard commented Nov 10, 2015

evanlucas commented Nov 10, 2015

jasnell commented Nov 10, 2015

evanlucas commented Nov 10, 2015

dandean Nov 10, 2015

mikeal Nov 10, 2015

evanlucas Nov 10, 2015

mikeal Nov 10, 2015

Fishrock123 commented Nov 10, 2015

Fishrock123 commented Nov 10, 2015

evanlucas commented Nov 10, 2015

Fishrock123 commented Nov 10, 2015

evanlucas commented Nov 10, 2015

mikeal commented Nov 10, 2015

ChALkeR commented Aug 5, 2016 •

edited

Loading

jasnell commented Aug 5, 2016

ChALkeR commented Aug 5, 2016

evanlucas commented Aug 5, 2016

cluster: migrate from worker.suicide #3743

cluster: migrate from worker.suicide #3743

Conversation

evanlucas commented Nov 10, 2015 • edited by tniessen Loading

MylesBorins commented Nov 10, 2015

MylesBorins commented Nov 10, 2015

Choose a reason for hiding this comment

JungMinu commented Nov 10, 2015

mikeal commented Nov 10, 2015

mikeal commented Nov 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Nov 10, 2015

mikeal commented Nov 10, 2015

jasnell commented Nov 10, 2015

Qard commented Nov 10, 2015

evanlucas commented Nov 10, 2015

jasnell commented Nov 10, 2015

evanlucas commented Nov 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Nov 10, 2015

Fishrock123 commented Nov 10, 2015

evanlucas commented Nov 10, 2015

Fishrock123 commented Nov 10, 2015

evanlucas commented Nov 10, 2015

mikeal commented Nov 10, 2015

ChALkeR commented Aug 5, 2016 • edited Loading

jasnell commented Aug 5, 2016

ChALkeR commented Aug 5, 2016

evanlucas commented Aug 5, 2016

evanlucas commented Nov 10, 2015 •

edited by tniessen

Loading

ChALkeR commented Aug 5, 2016 •

edited

Loading