cluster: alias suicide to exitedAfterDisconnect

[evanlucas]

Checklist

• make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

cluster

Description of change

This is a backport of 4f619bd which
migrates from worker.suicide to worker.exitedAfterDisconnect.

I did not add documentation as I wasn't sure if we wanted to document it or not.

Related: #3743

[ChALkeR]

Actual code changes LGTM, but I don't think that there is any value in adding this without the documentation — that would still mean that users wouldn't be able rely on it in 4.x.

I think that documentation should also be added, mentioning that this is an alias.

[evanlucas]

Thanks @ChALkeR. Updated.

[evanlucas]

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

[ChALkeR]

This link doesn't seem to work. Perhaps the definition is missing (at the end of file)?

[evanlucas]

yep, fixed, sorry about that

[ChALkeR]

LGTM.

[addaleax]

LGTM, CI is green

[jasnell]

LGTM

[jasnell]

@nodejs/lts WG ... any objections to this?

[MylesBorins]

lgtm

On Tue, Aug 9, 2016, 3:25 PM James M Snell notifications@github.com wrote:

@nodejs/lts https://github.com/orgs/nodejs/teams/lts WG ... any
objections to this?

—
You are receiving this because you are on a team that was mentioned.

Reply to this email directly, view it on GitHub
#7998 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAecV69eMcp-QQIstBsK74oth1Nxdkppks5qeNQYgaJpZM4JeVZh
.

[jasnell]

Landed in v4.x-staging in 810f29f

[evanlucas]

Thanks @jasnell!

[sam-github]

@nodejs/lts this says it landed in v4.x-staging, but maybe it later got rebased out again? I can't find any sign of it, and I can't checkout 810f29f on my local repo, which makes it not reachable from any branches.

[MylesBorins]

@sam-github that seems like what happened

We could modify it to be semver patch (reverse the alias)

[bnoordhuis]

For v4.x? That's rather stretching the credibility of semver and maintenance mode...

[gibfahn]

As I said in nodejs/Release#231 (comment), I don't see how this lands as a patch, as it's a new property.

However, I don't see the harm in including this in an upcoming 4.x release, it has the clear benefit of allowing people to write code that works on all supported releases (without deprecation warnings). The point of maintenance is that we don't land things unless we have a good reason, and I think we do here.

I also don't understand the reasoning behind "no semver-minors in maintenance mode". Why is a semver-minor worse than a semver-patch? I'd say that a small, low-risk minor like this is less of an issue than a large, more risky patch like #13233.

[bnoordhuis]

I also don't understand the reasoning behind "no semver-minors in maintenance mode". Why is a semver-minor worse than a semver-patch?

Because it's a slippery slope. If you accept this semver-minor change, how can you reasonably reject others? The point of maintenance mode is that you say: this is it, no more changes except critical bug fixes.

[gibfahn]

Because it's a slippery slope. If you accept this semver-minor change, how can you reasonably reject others?

But what does that have to do with the semver-ness? We only include things that people request, and that the LTS team (and de facto collaborators in general) think are worth making an exception for. We reject most changes because there is no obvious need, whether it's a minor or a patch. It's not as if we accept all patches because we're accepting one.

To be clear, I'm happy to debate whether this change should land, but I don't see why we (collaborators) or end users of node would care about whether it's a minor or a patch.

[bnoordhuis]

Are we using the same definition of maintenance mode? To me, it means this:

no more changes except critical bug fixes.

The goal is to reduce the overall work load. If people want new features, they can upgrade.

A semver-minor change is by definition not a bug fix, it's a new feature. If you want to land those in v4.x, fine, but you can't call it maintenance mode anymore - it's just another release line, with all the extra work and agony that entails.

[MylesBorins]

I've opened nodejs/Release#232 to move the v4.x conversation