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: remove deprecated API #13702

Closed
wants to merge 3 commits into from
Closed

cluster: remove deprecated API #13702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
14930b5
doc: EOL deprecated API and update notes
jasnell Jun 15, 2017
f1888bc
test: use worker.exitedAfterDisconnect consistently
jasnell Jun 15, 2017
6400a52
cluster: remove deprecated property
jasnell Jun 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 0 additions & 34 deletions doc/api/cluster.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,40 +451,6 @@ if (cluster.isMaster) {
}
```

### worker.suicide
<!-- YAML
added: v0.7.0
deprecated: v6.0.0
changes:
- version: v7.0.0
pr-url: https://github.com/nodejs/node/pull/3747
description: Accessing this property will now emit a deprecation warning.
-->

> Stability: 0 - Deprecated: Use [`worker.exitedAfterDisconnect`][] instead.

An alias to [`worker.exitedAfterDisconnect`][].

Set by calling `.kill()` or `.disconnect()`. Until then, it is `undefined`.

The boolean `worker.suicide` is used to distinguish between voluntary
and accidental exit, the master may choose not to respawn a worker based on
this value.

```js
cluster.on('exit', (worker, code, signal) => {
if (worker.suicide === true) {
console.log('Oh, it was just voluntary – no need to worry');
}
});

// kill worker
worker.kill();
```

This API only exists for backwards compatibility and will be removed in the
future.

## Event: 'disconnect'
<!-- YAML
added: v0.7.9
Expand Down
13 changes: 8 additions & 5 deletions doc/api/deprecations.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,12 +95,16 @@ methods, the `options.customFds` option is deprecated. The `options.stdio`
option should be used instead.

<a id="DEP0007"></a>
### DEP0007: cluster worker.suicide
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this word continues to appear in this file, it should have a content warning at the top, and/or a content warning and a click-through to view this section.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with a content warning within this line item (where it's more likely to be seen when someone clicks through to find the information for the specific deprecation code. However, it would be helpful if you had a specific suggestion for the wording of such a warning.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to not include worker.suicide inside the error message but rather a reference to the documentation?

### DEP0007:  The api used is deprecated, please replace with worker.exitedAfterDisconnect. Please refer to documentation for more information.

In the extended documentation we could start with

Content warning: Self Harm.

Please click here to skip to the next entry.

We can then make "click here" an href to the next header

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the name of the property is not included in the warning message that is printed to the console., it is included only in this one deprecations.md document. I'd rather not make it any more complicated than that.

Copy link
Contributor

@isaacs isaacs Jun 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be an improvement:

 <a id="DEP0007"></a>
 **Content Warning**: self harm.  [Click here to skip to the next entry.](#DEP00008)

-----

### DEP0007: Replace cluster worker.suicide with worker.exitedAfterDisconnect

A much better approach would add a CSS rule like this:

#DEP0007Content { visibility: hidden }
#DEP0007Content:target { visibility: visible }

and then

 <a id="DEP0007"></a>
 **Content Warning**: self harm.  [Click to show](#DEP0007Content)

<div id="DEP0007Content">
...

I realize this adds some complexity, but it's not like it's something we'll have to do very often, and it seems like a very reasonable cost for the benefit it provides.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shoot. I just now saw this comment come through @isaacs .... of course it was immediately after landing. This specific additional change can be handled and discussed via a separate PR. Sorry about missing it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell No problem. Improvements can come iteratively :) Do you want me to take a crack at writing that update, or do you want to tackle it?

### DEP0007: Replace cluster worker.suicide with worker.exitedAfterDisconnect

Type: Runtime
Type: End-of-Life
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather unfortunate terminology here, in context. I don't have a better suggestion though :\

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could replace all instances of End-of-Life with Broken and/or Removed, that might be more accurate terminology anyway (but we can also keep that for a follow-up, that might be easier).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a fixed term as defined in deprecations.md, so I don't think we want to change this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noted the irony also. To change this we would need a change to the deprecation policy as a whole, and the usage here is consistent with general usage in the industry. That's not to say it's particularly good, just that for now there isn't a better option.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tniessen I know, as I said I’m suggesting that we change the terminology, not this single instance.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax We both wrote our comments within a minute, I did not see your comment until I had written mine, I was not responding to you ;)


Within the `cluster` module, the [`worker.suicide`][] property has been
deprecated. Please use [`worker.exitedAfterDisconnect`][] instead.
In an earlier version of the Node.js `cluster`, a boolean property with the name
`suicide` was added to the `Worker` object. The intent of this property was to
provide an indication of how and why the `Worker` instance exited. In Node.js
6.0.0, the old property was deprecated and replaced with a new
[worker.exitedAfterDisconnect][] property. The old property name did not
precisely describe the actual semantics and was unnecessarily emotion-laden.

<a id="DEP0008"></a>
### DEP0008: require('constants')
Expand Down Expand Up @@ -689,7 +693,6 @@ Type: Runtime
[`util.puts()`]: util.html#util_util_puts_strings
[`util`]: util.html
[`worker.exitedAfterDisconnect`]: cluster.html#cluster_worker_exitedafterdisconnect
[`worker.suicide`]: cluster.html#cluster_worker_suicide
[alloc]: buffer.html#buffer_class_method_buffer_alloc_size_fill_encoding
[alloc_unsafe_size]: buffer.html#buffer_class_method_buffer_allocunsafe_size
[from_arraybuffer]: buffer.html#buffer_class_method_buffer_from_arraybuffer_byteoffset_length
Expand Down
14 changes: 0 additions & 14 deletions lib/internal/cluster/worker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,6 @@
'use strict';
const EventEmitter = require('events');
const internalUtil = require('internal/util');
const util = require('util');
const defineProperty = Object.defineProperty;
const suicideDeprecationMessage =
'worker.suicide is deprecated. Please use worker.exitedAfterDisconnect.';

module.exports = Worker;

Expand All @@ -20,16 +16,6 @@ function Worker(options) {

this.exitedAfterDisconnect = undefined;

defineProperty(this, 'suicide', {
get: internalUtil.deprecate(
() => this.exitedAfterDisconnect,
suicideDeprecationMessage, 'DEP0007'),
set: internalUtil.deprecate(
(val) => { this.exitedAfterDisconnect = val; },
suicideDeprecationMessage, 'DEP0007'),
enumerable: true
});

this.state = options.state || 'none';
this.id = options.id | 0;

Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-cluster-worker-constructor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ const cluster = require('cluster');
let worker;

worker = new cluster.Worker();
assert.strictEqual(worker.suicide, undefined);
assert.strictEqual(worker.exitedAfterDisconnect, undefined);
assert.strictEqual(worker.state, 'none');
assert.strictEqual(worker.id, 0);
assert.strictEqual(worker.process, undefined);
Expand All @@ -39,7 +39,7 @@ worker = new cluster.Worker({
state: 'online',
process: process
});
assert.strictEqual(worker.suicide, undefined);
assert.strictEqual(worker.exitedAfterDisconnect, undefined);
assert.strictEqual(worker.state, 'online');
assert.strictEqual(worker.id, 3);
assert.strictEqual(worker.process, process);
Expand Down
18 changes: 0 additions & 18 deletions test/parallel/test-cluster-worker-deprecated.js
View file
Open in desktop

This file was deleted.

4 changes: 0 additions & 4 deletions test/parallel/test-cluster-worker-disconnect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,8 @@ if (cluster.isWorker) {
http.Server(() => {

}).listen(0, '127.0.0.1');
const worker = cluster.worker;
assert.strictEqual(worker.exitedAfterDisconnect, worker.suicide);

cluster.worker.on('disconnect', common.mustCall(() => {
assert.strictEqual(cluster.worker.exitedAfterDisconnect,
cluster.worker.suicide);
process.exit(42);
}));

Expand Down
2 changes: 0 additions & 2 deletions test/parallel/test-cluster-worker-exit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'],
worker_exitedAfterDisconnect: [
false, 'the .exitedAfterDisconnect flag is incorrect'
],
Expand Down Expand Up @@ -84,7 +83,6 @@ if (cluster.isWorker) {
// Check worker events and properties
worker.on('disconnect', common.mustCall(() => {
results.worker_emitDisconnect += 1;
results.worker_suicideMode = worker.suicide;
results.worker_exitedAfterDisconnect = worker.exitedAfterDisconnect;
results.worker_state = worker.state;
if (results.worker_emitExit > 0) {
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-regress-GH-3238.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ if (cluster.isMaster) {
function forkWorker(action) {
const worker = cluster.fork({ action });
worker.on('disconnect', common.mustCall(() => {
assert.strictEqual(worker.suicide, true);
assert.strictEqual(worker.exitedAfterDisconnect, true);
}));

worker.on('exit', common.mustCall(() => {
assert.strictEqual(worker.suicide, true);
assert.strictEqual(worker.exitedAfterDisconnect, true);
}));
}

Expand Down