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.

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

cluster: overriding inspector port #14140

Merged
merged 3 commits into from
Jul 14, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
31 changes: 14 additions & 17 deletions doc/api/cluster.md
Original file line number Diff line number Diff line change
Expand Up @@ -700,18 +700,21 @@ changes:
-->

* {Object}
* `execArgv` {Array} list of string arguments passed to the Node.js
* `execArgv` {Array} List of string arguments passed to the Node.js
executable. (Default=`process.execArgv`)
* `exec` {string} file path to worker file. (Default=`process.argv[1]`)
* `args` {Array} string arguments passed to worker.
* `exec` {string} File path to worker file. (Default=`process.argv[1]`)
* `args` {Array} String arguments passed to worker.
(Default=`process.argv.slice(2)`)
* `silent` {boolean} whether or not to send output to parent's stdio.
* `silent` {boolean} Whether or not to send output to parent's stdio.
(Default=`false`)
* `stdio` {Array} Configures the stdio of forked processes. Because the
cluster module relies on IPC to function, this configuration must contain an
`'ipc'` entry. When this option is provided, it overrides `silent`.
* `uid` {number} Sets the user identity of the process. (See setuid(2).)
* `gid` {number} Sets the group identity of the process. (See setgid(2).)
* `inspectPort` {number|function} Sets inspector port of worker.
Accepts number, or function that evaluates to number. By default
each worker gets port, incremented from master's `process.debugPort`.

After calling `.setupMaster()` (or `.fork()`) this settings object will contain
the settings, including the default values.
Expand All @@ -727,26 +730,19 @@ changes:
description: The `stdio` option is supported now.
-->

* `settings` {Object}
* `exec` {string} file path to worker file. (Default=`process.argv[1]`)
* `args` {Array} string arguments passed to worker.
(Default=`process.argv.slice(2)`)
* `silent` {boolean} whether or not to send output to parent's stdio.
(Default=`false`)
* `stdio` {Array} Configures the stdio of forked processes. When this option
is provided, it overrides `silent`.
* `settings` {Object} see [`cluster.settings`][]

`setupMaster` is used to change the default 'fork' behavior. Once called,
the settings will be present in `cluster.settings`.

Note that:

* any settings changes only affect future calls to `.fork()` and have no
effect on workers that are already running
* Any settings changes only affect future calls to `.fork()` and have no
effect on workers that are already running.
* The *only* attribute of a worker that cannot be set via `.setupMaster()` is
the `env` passed to `.fork()`
* the defaults above apply to the first call only, the defaults for later
calls is the current value at the time of `cluster.setupMaster()` is called
the `env` passed to `.fork()`.
* The defaults above apply to the first call only, the defaults for later
calls is the current value at the time of `cluster.setupMaster()` is called.

Example:

Expand Down Expand Up @@ -834,3 +830,4 @@ socket.on('data', (id) => {
[Child Process module]: child_process.html#child_process_child_process_fork_modulepath_args_options
[child_process event: 'exit']: child_process.html#child_process_event_exit
[child_process event: 'message']: child_process.html#child_process_event_message
[`cluster.settings`]: #clustersettings
20 changes: 18 additions & 2 deletions lib/internal/cluster/master.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const cluster = new EventEmitter();
const intercom = new EventEmitter();
const SCHED_NONE = 1;
const SCHED_RR = 2;
const {isLegalPort} = require('internal/net');

module.exports = cluster;

Expand Down Expand Up @@ -104,8 +105,23 @@ function createWorkerProcess(id, env) {
workerEnv.NODE_UNIQUE_ID = '' + id;

if (execArgv.some((arg) => arg.match(debugArgRegex))) {
execArgv.push(`--inspect-port=${process.debugPort + debugPortOffset}`);
debugPortOffset++;
let inspectPort;
if ('inspectPort' in cluster.settings) {
if (typeof cluster.settings.inspectPort === 'function')
inspectPort = cluster.settings.inspectPort();
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't you check this is a positive integer less than 16K? otherwise the cmd line passed will be invalid. same for the else below

Copy link
Contributor

Choose a reason for hiding this comment

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

I was going to open a follow up PR, but it could be done here.

Check if cluster.settings.inspectPort is a function. If so, call it and assign to inspectPort. If it's not a function, assign it directly to inspectPort. Then, call require('internal/net').isValidPort(inspectPort), and throw an error if it's not valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Test file became less readable, is it okay? I think I can refactor it, but is it really worth it?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO Readability is not that bad. Just a tiny-bit surprising to find the validation assertions so deep. There are much worse tests.

else
inspectPort = cluster.settings.inspectPort;

if (!isLegalPort(inspectPort)) {
throw new TypeError('cluster.settings.inspectPort' +
' is invalid');
}
} else {
inspectPort = process.debugPort + debugPortOffset;
debugPortOffset++;
}

execArgv.push(`--inspect-port=${inspectPort}`);
}

return fork(cluster.settings.exec, cluster.settings.args, {
Expand Down
Loading