cluster: migrate from worker.suicide

[evanlucas]

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

Ref: https://github.com/nodejs/node/issues/3721

[MylesBorins]

LGTM

[MylesBorins]

Test suite passes locally fwiw

[JungMinu]

LGTM with one comment

[mikeal]

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

[mikeal]

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

[jasnell]

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]

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

[jasnell]

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]

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

[evanlucas]

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

[jasnell]

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

[evanlucas]

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

[Fishrock123]

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

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]

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

[Fishrock123]

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]

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

[mikeal]

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.

[evanlucas]

ok, opened https://github.com/nodejs/node/pull/3747 as the follow up

[ChALkeR]

Is soft-deprecation semver-minor?

See https://github.com/nodejs/node/issues/1854#issuecomment-107242008 (by @isaacs). What's the actual policy here?

[ChALkeR]

@Fishrock123

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

Will do when I'll get home (in a few hours), those stats are on my PC. I did a preliminary check that time and remember there being a little more than 100 modules using suicide, but I'm not sure if I remember things correctly.

[ChALkeR]

Also note that cluster API is marked as stable. That property is not only exposed, it's documented as being a part of a stable API.

Compatibility with the npm ecosystem is a high priority, and will not be broken unless absolutely necessary.

This PR even in its current state looks like a semver-major to me.

[evanlucas]

It is still an alias with a getter/setter, so it can still be used the same way it could before

[ChALkeR]

It changes stable api documentation, soft-deprecating things. I will get home in a few hours and will elaborate my reasoning.

[evanlucas]

Should I make the property enumerable?

[OrangeDog]

Using "self" instead of "voluntarily" would be shorter, for those worried about that sort of thing.

[Fishrock123]

I'm noting voluntarily isn't easy to type, would willingly be better? http://www.thesaurus.com/browse/voluntarily

[ChALkeR]

Ok, I'm at home now.

All of the below expresses my personal point of view (except for obvious facts and citations), please keep that in mind when reading. So, my reasoning:

1. This is a documented property in an API that is marked stable.

   «Stable» is documented as:

   Stability: 2 - Stable The API has proven satisfactory. Compatibility with the npm ecosystem is a high priority, and will not be broken unless absolutely necessary.

2. The reason for removing this is non-technical.

   While I agree that this shouldn't be originally commited under that name, it's already there, used, and documented, and the removal/deprecation is not forced upon us by a thirdparty (as with v8) or a security issue.

3. Even _prefixed API changes (those are not documented and are viewed as «internal») broke things, and people got quite upset about that. See #1854 (and issues linked there).
4. Documentation is primary, especially for things marked as being «Stable», if there are no obvious mistakes in it. Changing documentation for a stable API is an important change, especially when an property is renamed or removed. I consider fixing the API to conform with documentation or removing/deprecating an undocumented property as a less important change then changing the documentation if it matches the API.
5. Speaking of the ecosystem breakage: this commit breaks it. It doesn't matter that the code doesn't throw warnings. For some, using even softly deprecated API is not acceptable, because they are concerned about support being removed in the the following release.

6. This commit and the deprecation warning commit would make people write something like w.hasOwnProperty('willinglyExited') ? w.willinglyExited : w.suicide instead of w.suicide. If they don't do that — they will be hitting deprecated API.

   Actual code samples (one per line):

   if (worker && !worker.suicide) {
   suicide: !!w.suicide,
   logger.info("Worker " + worker.id + " exit: " + code + " " + signal + ". Suicide? " + worker.suicide);
   var connected = !worker.suicide; // suicide is set after .disconnect()
   var exit = worker.suicide ? 'expected' : 'accidental';

   Now imagine the above replacement for all of those cases. We would get:

   if (worker && !(`worker.hasOwnProperty('willinglyExited') ? worker.willinglyExited : worker.suicide`)) {
   suicide: !!(w.hasOwnProperty('willinglyExited') ? w.willinglyExited : w.suicide), // TODO: rename my property name?
   logger.info("Worker " + worker.id + " exit: " + code + " " + signal + ". Suicide? " + (worker.hasOwnProperty('willinglyExited') ? worker.willinglyExited : worker.suicide));
   var connected = !(worker.hasOwnProperty('willinglyExited') ? worker.willinglyExited : worker.suicide); // willinglyExited/suicide is set after .disconnect()
   var exit = (worker.hasOwnProperty('willinglyExited') ? worker.willinglyExited : worker.suicide) ? 'expected' : 'accidental';

   And you can't tell them not to — some people still support 0.8, 0.12 is going to be supported by a lot of modules for a long time. Even if you introduce your new willinglyExited (or whatever it would be called) support in 4.x LTS — people would be still forced to check for it and use suicide.

   Any decision to deprecate this would cause a huge amount of work and a huge amount of ugly, unnecessary code.

7. Also note how people are declaring suicide on their own properties to match with worker (second line in the code block above). The previous point covers only trivial cases, there are things that are going to break worse, and will require even more non-functional code to work around that.

8. The original arguments for this change look to me like overdoing things.

   This word isn't offensive by itself, because it's not directed onto anyone. Specific usage of words could be offensive, without that words are just words. Dictionaries, Wikipedia, books — all those use that word and it doesn't make them bad or inappropriate.

   I agree though that in the current situation this specific choice of wording wasn't perfect, but I don't see how this justifies breaking things.

   If we start treating plain words as being offensive, then we have things a lot worse then suicide in the ecosystem.

I'm against this change, and I'm strongly against pulling that in as a semver-minor.

@Fishrock123 A list will follow in a few minutes.

Ah, a note: an alias could work if some people feel very serious about that wording and if that justifies adding an alias, but without the deprecation. Even in the documentation.

[ChALkeR]

@Fishrock123

Grep for .suicide over npm modules: https://gist.github.com/ChALkeR/6b25950a20949195246d. There are, of course, both false negatives and false positives.

Also keep in mind that cluster module is probably likely to be used by the applications themselves (not by the libs), and a large amount of those are supposedly not packaged on npmjs.org.

[Fishrock123]

The API has proven satisfactory.

Saying we can't realize it wasn't satisfactory is a bad trap.

The reason for removing this is non-technical.

The same argument could probably be used for https://github.com/nodejs/node/commit/93e28306864d14aaeef14061990abb733bbe7105, @@objectTag or whatever the API called doesn't actually cause those to stop functioning.

Even _prefixed API changes (those are not documented and are viewed as «internal») broke things, and people got quite upset about that.

Which is why we need to be careful.

Documentation is primary, especially for things marked as being «Stable», if there are no obvious mistakes in it. Changing documentation for a stable API is an important change, especially when an property is renamed or removed. I consider fixing the API to conform with documentation or removing/deprecating an undocumented property as a less important change then changing the documentation if it matches the API.

That's not how we have worked. https://github.com/nodejs/node/pull/2447 was deemed minor.

Heck we've even considered issuing deprecations on Object.observe and smalloc in Stable and LTS branches.

Speaking of the ecosystem breakage: this commit breaks it.

Clearly why we are working on that. Thanks. :)

This commit and the deprecation warning commit would make people write something like w.hasOwnProperty('willinglyExited') ? w.willinglyExited : w.suicide instead of w.suicide. If they don't do that — they will be hitting deprecated API.

const exitedAPI = worker.hasOwnProperty('willinglyExited') ? 'willinglyExited' : 'suicide'
if (worker && !worker[exitedAPI])) {
suicide: !!worker[exitedAPI], // TODO: rename my property name?
logger.info("Worker " + worker.id + " exit: " + code + " " + signal + ". Suicide? " + worker[exitAPI]);
var connected = !worker[exitedAPI]; // willinglyExited/suicide is set after .disconnect()
var exit = worker[exitedAPI] ? 'expected' : 'accidental';

Also note how people are declaring suicide on their own properties to match with worker (second line in the code block above). The previous point covers only trivial cases, there are things that are going to break worse, and will require even more non-functional code to work around that.

that's horrible and we should work to clean up after ourselves.

The original arguments for this change look to me like overdoing things.

community members have indicated in the original thread that is is painful for some of them, even if not to us.

[jasnell]

Thank you for doing the analysis! On Nov 11, 2015 8:28 AM, "Сковорода Никита Андреевич" < notifications@github.com> wrote:

Grep for .suicide over npm modules: https://gist.github.com/ChALkeR/6b25950a20949195246d There are, of course, both false negatives and false positives. Also keep in mind that cluster module is probably likely to be used by the applications themselves (not by the libs), and a large amount of those are supposedly not packaged on npmjs.org.

— Reply to this email directly or view it on GitHub https://github.com/nodejs/node/pull/3743#issuecomment-155835295.

[Fishrock123]

We should smoke-test those modules if possible against this.

I don't see anything trying to delete or otherwise to fancy detection of the old property, so a getter/setter seems fine.

@evanlucas I think making it enumerable would be a good idea though.

[ChALkeR]

The same argument could probably be used for 93e2830, @@objectTag or whatever the API called doesn't actually cause those to stop functioning.

93e2830 was a soft-deprecation done in a semver-major (despite being labeled as a semver-minor) and those were not functioning correctly for some known cases. There was a reason for removing those.

That's not how we have worked. #2447 was deemed minor.

See above, despite being labeled as a semver-minor, it got only into 4.0. And yes, there is no strict policy on that, and I'm upset about it. See #1854, I guess.

community members have indicated in the original thread that is is painful for some of them, even if not to us.

I'm not strongly against adding an alias if that is the case. I'm not even against the alias being the main property (if the getter-setter will be identical to what there was before, keep in mind that those sometimes failed), so that the commit logs would get the new property name. I'm against the deprecation.

[Fishrock123]

See above, despite being labeled as a semver-minor, it got only into 4.0.

Only by virtue of timing.

I'm not strongly against adding an alias if that is the case. I'm not even against the alias being the main property (if the getter-setter will be identical to what there was before), so that the commit logs would get the new property name. I'm against the deprecation.

Would making the new one be the getter for a deprecation cycle help?

[ChALkeR]

Only by virtue of timing.

It doesn't matter. And yes, we need a policy on those things.

Would making the new one be the getter for a deprecation cycle help?

I doubt that. As I already said: even the documentation-only deprecation seems like a bad idea to me.

[mikeal]

If I remember correctly @chrisdickinson worked on the deprecation policy, maybe he can help out here.

[ChALkeR]

Ok, I have manually selected four packages from that list.

@Unitech for https://github.com/Unitech/pm2/blob/master/lib/God/ActionMethods.js#L297, @julianlam and @barisusakli for https://github.com/NodeBB/NodeBB/blob/master/loader.js, @hij1nx and @ralphtheninja for https://github.com/hij1nx/liveswap/blob/master/index.js#L52, @tshemsedinov for https://github.com/tshemsedinov/impress/blob/master/lib/impress.js (and another file in that repo).

What would your course of actions be if worker.suicide is soft-deprecated (doc-only)? What would it be if it's hard-deprecated (warning)? What would the code look like?

[ChALkeR]

@rvagg Woah, I was not aware that the meeting is planned today =). Could this issue be postponed till the next week meeting? I guess that not all have read it, and I guess that there should be a bit more prior discussion before the resolution.

[mikeal]

Quick question, why did we already add the ctc-agenda tag?

Usually we try to reach a consensus through review before escalating to the CTC to make a decision. As it stands the only objections are from @ChALkeR and each of them are actively under discussion and could potentially be resolved without escalating to the CTC. It's only if some objections reach an impasse that we would escalate, which could happen here but hasn't happened yet.

[ChALkeR]

@mikeal I added the ctc-agenda tag, because from my point of view this issue is controversal enough, and the objections from my side could not be resolved without removing the deprecation completely or without a CTC resolution. Also note that this PR already received three (or two) LGTMs, and there are enough people who support this change.

Tell me if I was not the using the label correctly. I consulted with GOVERNANCE.md prior to adding the label:

Collaborators may opt to elevate significant or controversial modifications, or modifications that have not found consensus to the TC for discussion by assigning the tc-agenda tag to a pull request or issue. The TC should serve as the final arbiter where required.

…

Any community member or contributor can ask that something be added to the next meeting's agenda by logging a GitHub Issue. Any Collaborator, TC member or the moderator can add the item to the agenda by adding the tc-agenda tag to the issue.

[ChALkeR]

If the desired effect is deprecation and removal of .suicide — an alias has to be present for several major releases prior to the soft deprecation. Or else, people would be forced to write code like w.hasOwnProperty('willinglyExited') ? w.willinglyExited : w.suicide each time to access that property in order not to hit a deprecated method.

[MylesBorins]

So it would seem that there are a number of tools that are built on top of cluster for managing workers.

For example forky and throng

While forky does reference suicide on one line, throng does not.

I'm going to try and get a feature into citgm that can quickly smoke test an array of modules against a build of node. If you want to make a definitive list I can do a pass at smoketesting

[mikeal]

the objections from my side could not be resolved without removing the deprecation completely or without a CTC resolution

Ah, see, I don't know that the deprecation is off the table except for maybe a "docs only" deprecation (note that it would still say something like "formerly worker.suicide").

But you're using the label correctly if you believed that point could not be resolved.

[ChALkeR]

Ah, see, I don't know that the deprecation is off the table except for maybe a "docs only" deprecation (note that it would still say something like "formerly worker.suicide").

Sorry, I am not sure that I understand that phrase correctly. A «doc only» deprecation is still a deprecation, see my point 4 in https://github.com/nodejs/node/pull/3743#issuecomment-155826183 and https://github.com/nodejs/node/pull/3743#issuecomment-155867463.

I consider removing or adding a deprecation label to https://nodejs.org/api/cluster.html#cluster_worker_suicide as deprecation, and

the objections from my side could not be resolved without removing the deprecation completely

applies to that.

[mikeal]

@ChALkeR in terms of our current policies I don't think "doc only" would be considered a semver-major deprecation. I doubt it's explicitly covered but we have talked about changes that break "tests but not actual code" as not being semver-major, so I would say that the bar for semver-major would be "does this break any in-use code." Although I think the CTC is talking about this again today in regards to changes in error message text.

That doesn't address all issues you may have with the doc-only deprecation but I thought it might add a little context.

[ChALkeR]

@Fishrock123

const exitedAPI = worker.hasOwnProperty('willinglyExited') ? 'willinglyExited' : 'suicide'
if (worker && !worker[exitedAPI])) {
suicide: !!worker[exitedAPI], // TODO: rename my property name?
logger.info("Worker " + worker.id + " exit: " + code + " " + signal + ". Suicide? " + worker[exitAPI]);
var connected = !worker[exitedAPI]; // willinglyExited/suicide is set after .disconnect()
var exit = worker[exitedAPI] ? 'expected' : 'accidental';

That's not making it smaller, that's making it bigger:

const exitedAPI = worker.hasOwnProperty('willinglyExited') ? 'willinglyExited' : 'suicide'
if (worker && !worker[exitedAPI])) {

const exitedAPI = worker.hasOwnProperty('willinglyExited') ? 'willinglyExited' : 'suicide'
suicide: !!worker[exitedAPI], // TODO: rename my property name?

const exitedAPI = worker.hasOwnProperty('willinglyExited') ? 'willinglyExited' : 'suicide'
logger.info("Worker " + worker.id + " exit: " + code + " " + signal + ". Suicide? " + worker[exitAPI]);

const exitedAPI = worker.hasOwnProperty('willinglyExited') ? 'willinglyExited' : 'suicide'
var connected = !worker[exitedAPI]; // willinglyExited/suicide is set after .disconnect()

const exitedAPI = worker.hasOwnProperty('willinglyExited') ? 'willinglyExited' : 'suicide'
var exit = worker[exitedAPI] ? 'expected' : 'accidental';

Those lines were from different files, as I already stated above. Those can't be covered by a single const exitedAPI for obvious reasons =).

[Fishrock123]

I doubt that. As I already said: even the documentation-only deprecation seems like a bad idea to me.

Well, that's how we've been doing things. ¯\_(ツ)_/¯

I think it's a good idea to let users known in the docs that something may not work in the future.

[jasnell]

This is going the right direction but I'm kinda losing the thread on which parts are being done here vs. the other PR. I don't see why this should be on the CTC agenda but not objecting to it either. I think it's something we can discuss and resolve quickly.

[mikeal]

@jasnell it's off the CTC agenda for this week.

[jwueller]

Not a collaborator (though apparently nominated).

I've had someone close to me attempt suicide, and the function name is obviously not a brilliant choice, but I nonetheless agree that a non-technical reason does not justify a deprecation. It appears a little selfish of me to expect potentially several thousands of developers having to change their programs in the future because of something I am dealing with. We are working in a technical context here, and I agree that it should be treated as such. Words themselves are not bad, as previously noted.

I would like to explicitly express support for @ChALkeR's points, and reiterating that this PR would change a public, documented and stable API. There seem to be some inconsistencies here.