cluster: remove deprecated problematic API #13684
Conversation
Content Warning: mental health and self-harm triggers. --- The word "suicide" was used in the cluster API 4 years ago. This was a serious mistake for several reasons. It was deprecated in version 6, and long past time for full removal now. First, and most important, casual use of words referring to self-harm serve to stigmatize mental illness and make it harder for people in our community to literally stay alive. It is impossible to claim that we intend to be inclusive and welcoming, and also casually show such disrespect for the lives of those in our community. Some of us have lost friends or struggled with mental illness. Leaving this lying around tells us that we are not welcome. Second, in addition to being just wildly inappropriate on ethical and community grounds, the term "suicide" was never even appropriate technically! In context, it refers to a case where a child process exits without receiving a fatal signal from the parent process. The "cute" unix joke here is that the process "killed itself". However, the "suicide" flag is set regardless of how the process exited, so in addition to being offensive and harmful, it is also misleading and incorrect, and has always been so. (It could've thrown an error, called `process.exit()` or received a fatal signal from some other process.) An open source project is not the place for emotional landmines. It's distracting and misleading at best, and actively harmful at worst. There is no justification for it. The API has already been renamed, and it's an obscure surface that is unlikely to ever be used outside of core.
tniessen
added
the
semver-major
|
This is semver-major, right? |
|
@tniessen yes |
|
I applaud the concern for others, but I doubt that this is really helping anyone. We only have so many words to play with and blacklisting one because it could potentially trigger a negative emotional response seems counterproductive. This is a striking example of the Voldemort Effect in the Harry Potter books. Suicide is equivalent with "he who must not be named". Instead of talking about the problem openly you shut down conversation in hopes of it going away. |
|
@michaelmcmillan the argument about getting rid of |
|
@cjihrig I was not aware of that, sorry for resurrecting that whole debate. |
|
So this was doc deprecated in Node 6 and emitted a warning in Node 7? Finally dropping it in Node 9 seems reasonable. CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/877/ |
|
Perhaps this (and all other deprecated APIs) could be routinely removed once an alternative is available to them across all LTS branches. This particular property has a replacement in 6.x and later, but not in 4.x. We can backport the replacement property to 4.x, which should be quite low risk, or we can wait until 4.x goes out of maintenance and do a house-cleaning then. |
|
To be clear, I'm not opposed to doing this now, but I hope its deprecation cycle is the same as every other deprecated property. We should find all the candidates for deprecation, and make sure they all go away on the same schedule. |
[UPDATED: The
Definitely -1 on removing stuff without a reason.
|
@Trott If I understand you correctly you're replying to @sam-github's general point about removing APIs, not expressing a particular issue with this PR. If so can we move that to a new Issue (I think/hope @sam-github was going to raise one)? |
| @@ -52,7 +52,6 @@ if (cluster.isWorker) { | |||
| worker_emitDisconnect: [1, "the worker did not emit 'disconnect'"], | |||
| worker_emitExit: [1, "the worker did not emit 'exit'"], | |||
| worker_state: ['disconnected', 'the worker state is incorrect'], | |||
| worker_suicideMode: [false, 'the worker.suicide flag is incorrect'], | |||
There was a problem hiding this comment.
just to confirm, this mode was only entered when using the deprecated API correct?
| @@ -7,12 +7,12 @@ const cluster = require('cluster'); | |||
| const worker = new cluster.Worker(); | |||
|
|
|||
| assert.strictEqual(worker.exitedAfterDisconnect, undefined); | |||
| assert.strictEqual(worker.suicide, undefined); | |||
| assert.strictEqual(worker.exitedAfterDisconnect, undefined); | |||
There was a problem hiding this comment.
@gibfahn Yes, I agree, it seems like this test file can be removed rather than modified.
@gibfahn Correct, sorry about the confusing wording. I'm A-OK with this change pending a CITGM run and maybe a few other checks to estimate ecosystem impact. |
No, I thought it was the intention that deprecation was a step on the road to deletion, but apparently that's not the case. |
|
@sam-github @Trott can you please move conversation about deprecation to a meta issue. Thanks! |
|
Just noticed that @nodejs/lts is there a chance that we could backport that? We've got a couple of fixes that we're planning to backport to 4.x anyway, and it's in maintenance till March 2018 . |
|
we could backport to v4.x if it was done as a patch |
|
I'm having trouble following the history. @evanlucas said he would backport, #3743 (comment), and he did, #7998, and it got closed, but I can't find any evidence of it in v4.x-staging or v4.x. Maybe its the remnants of my cold? What's up? |
#7998 was a |
|
@gibfahn want to open an issue over on Lts and we can discuss? |
|
FYI... I am currently in the process of preparing an alternate semver-major PR targeted for 9.x, and a separate PR that should be less disruptive for 8.x that we ought to be able to backport to 6.x. There is a separate conversation on going about whether to backport the |
|
Maybe I'm missing something, so let me know if this was already covered or suggested.
|
|
@mikeal .. A runtime deprecation was added in 6.0.0, not a docs-only deprecation |
|
@jasnell ah, thanks for the clarification. |
|
Any investigation into the scope of breakage that would be caused by this should limit to programs that currently pass their tests on node 8 without any code changes. If they're broken already, it doesn't matter if they also have to change this, as long as the change is made as soon as possible. If we delay, then there's a chance that they'll upgrade, and not know to upgrade this (though that is unlikely, given the preexisting deprecation.) |
|
@isaacs Just a note (as I am feeling that you might have misunderstood me): I understand your concern and I am not blindly opposed to making changes there or even bypassing the deprecation procedure if that would be the least bad thing. I am opposed to not having a discussion here and not doing the estimations, and to rushing this. If you truly wish the best for everyone out there, you shouldn't be opposed to factorizing the concern and investigating which way would be the least harmful. |
I agree in principle, but consider what you're suggesting. What is the chance that you will find a scope of program breakage anywhere near the harm of triggering surprise PTSD flashbacks in our users?
I don't understand how that's possible. They won't get "is not a function" errors, because it never was. Unless a userland module is doing something very peculiar, it seems unlikely that this will result in a triggering log spam. Can you demonstrate how that would work? "Making the existing problem worse" is the only reasonable argument against this imo, but I am very skeptical. |
Again — that's not what I am suggesting. We should take all the consequences into the account that we could, of course, but that's not even the main point. You are implying that this change will reduce the chance of triggering those flashbacks, but I am afraid that the opposite could be true. I am not even sure that landing this PR would have prevented those logs that you are talking about. See below, though.
Would it be possible to get those logs to better understand what exactly caused them? |
|
Even though I am normally +1 for backwards compat/safety. This has encountered a deprecation cycle/warnings and has harm by staying. I would want a serious argument for why keeping deprecated things with better replacement APIs should stay supported if support could cause harm to users. With any usage data I would like to see ones that would continue to function silently in this scenario. |
|
@isaacs To elaborate: the runtime deprecation caused this: Unitech/pm2#2780, they have that as an open issue at this moment — users are seeing that deprecation warning in their logs and have no idea why (the issue is not resolved yet). And the fact that that is being actively hit means that by removing that we will not only break things, but force some percentage of people to dig into the code that calls that. Which would expose them more. |
|
@ChALkeR If anything, it shows that noisy deprecation here is almost certainly worse than outright removal. Since PM2 doesn't use that field anywhere, it's really weird that it's showing up at all. I've asked some users in that thread to turn on trace-deprecation. Probably we can fix this at the root. |
|
@isaacs PM2 1.x, 2.0.x, 2.1.0 and 2.1.1 use it at this line: https://github.com/Unitech/pm2/blob/v2.0.18/lib/God/ActionMethods.js#L303 |
|
@ChALkeR ... do you know if a PR has been opened in PM2 to remove/replace usage of the deprecated API? |
|
@jasnell It is fixed in the current version for several releases (2.1.2 and later), but it is strange, as people are still getting the warnings from somewhere. I can't locate that in the code. Could be some PM2-related module or local configuration, or incomplete update. |
|
@ChALkeR I believe it is ending up in logs when doing introspection on the worker object. Making the property unenumerable would stop that from happening. |
|
cc @Unitech - Alexandre can you have a look? |
|
#13704 makes the property non-enumerable and can be backported to the 6.x, and 7.x release lines. |
|
@MylesBorins introspection could surely make it appear in the logs, but don't think that could have caused any of the specific mentioned log issues ( |
|
@isaacs Thanks for taking a look at that pm2 issue. Re: removal — the problem that I see here is that deprecation messages being fired (and people are still reporting it being fired) means that the code path is actually being used, meaning that once that is removed completely, that code path is going to be silently broken. And one of the implications of silently breaking it would be that users/developer would probably have to spend some visible amount of people-hours in total, trying to nail it down, getting more emotionally tied to that than to regular logs, and during that they will be closer dealing with (or suddenly discovering) the exact same api/wording the intention was to make less visible. Because of that, I am not convinced that removing will make the flashbacks less harmful ( = frequent * severe). |
|
At this point, removal in 9.x is the right thing to do. For users who may be impacted by that, a trivial mitigation is possible by monkey-patching the cluster API. A small module could easily be published to npm to accomplish that goal. The API was runtime deprecated in 6.x and, per the deprecation policy, is ready to reach the end-of-life stage. My concern is only that this specific PR is not the way to accomplish it and I would recommend closing this PR in favor of #13702 and #13704. |
@ChALkeR As far as you can tell, is there anyone in this conversation with insights that would make their estimates of these numbers significantly better than your own estimates? If you believe these should be measured/estimated to the extent possible and there's no reason to think that your estimates are going to be significantly inferior to anyone else's, might you be able to create your own estimates and explanations? If others do have insights into ways premises can be improved, etc., they might more readily respond to a concrete example. |
|
@Trott Based on preliminary estimations, I believe that this is an order of magnitude of 10-100 users getting those flashbacks per month (where flashback is being defined as «noticing and reminded of their previous attempt», not something more severe). I have not completed the estimations, but those are the numbers that I expect to get based on what I have now. I don't have the knowledge to estimate how bad those are, what portion of those are severe, and how likely are they to harm people. That said, I do not know how would that be changed once we remove that property. |
|
Preliminary data and assumptions used, if anyone wants to check my result:
|
|
If immediate removal is not on the table, then I'd recommend removing the deprecation warning to stderr and making the property non-enumerable, removing from documentation (or hiding by default with a warning), and backporting these changes as far back as possible. The deprecation in 6.x made the problem worse, as @ChALkeR has described above. |
|
Sure, removing the runtime deprecation will prevent people from seeing the word in their console outputs or logs. But it won't help people seeing it in their codebase, or when debugging dependencies, or when looking at Node.js's code. I think eventual removal is a better solution in the long term. |
|
A bit late, since this is already landed, but just to close the loop on the unanswered question. The log spam was a result of a module used by PM2 that walks an object's members mutating functions. Since it has to check for each member's functionness, it was calling So, removing this flag will not break PM2. They don't use it for anything. I still maintain that the best course of action is full immediate removal, but #13704 and #13702 do make the situation a lot better. Thank you for taking this seriously. |
Content Warning: mental health and self-harm triggers.
The word "suicide" was used in the cluster API 4 years ago. This was a
serious mistake for several reasons. It was deprecated in version 6,
and long past time for full removal now.
First, and most important, casual use of words referring to self-harm
serve to stigmatize mental illness and make it harder for people in our
community to literally stay alive. It is impossible to claim that we
intend to be inclusive and welcoming, and also casually show such
disrespect for the lives of those in our community. Some of us have
lost friends or struggled with mental illness. Leaving this lying
around tells us that we are not welcome.
Second, in addition to being just wildly inappropriate on ethical and
community grounds, the term "suicide" was never even appropriate
technically! In context, it refers to a case where a child process
exits without receiving a fatal signal from the parent process. The
"cute" unix joke here is that the process "killed itself". However, the
"suicide" flag is set regardless of how the process exited, so in
addition to being offensive and harmful, it is also misleading and
incorrect, and has always been so. (It could've thrown an error, called
process.exit()or received a fatal signal from some other process.)An open source project is not the place for emotional landmines. It's
distracting and misleading at best, and actively harmful at worst.
There is no justification for it. The API has already been renamed, and
it's an obscure surface that is unlikely to ever be used outside of
core.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)