cluster: forked children eagerly ignore/replace inspect port from cluster settings

[nazrhyn]

• Version: 8.11.3
• Platform: Linux 4.13.0-46-generic #51-Ubuntu SMP Tue Jun 12 12:36:29 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
• Subsystem: cluster

The Setup

Here's a small example that can be used to show this behavior:

const cluster = require('cluster');

const args = [
		process.argv[0],
		...process.execArgv,
		...process.argv.slice(1)
	],
	prefix = cluster.isMaster ? '[MASTER]' : '[WORKER]';

console.log(prefix, ...args);

if (cluster.isMaster) {
	const options = {
		execArgv: [],
		args: ['worker']
	};

	if (process.execArgv.some(arg => arg.startsWith('--inspect'))) {
		options.execArgv.push('--inspect=20002')
	}

	cluster.setupMaster(options);

	cluster.fork();
}

In this example, when the master process has been started with any of the --inspect arguments, we want to start the worker with the inspector active on a specific port. Here's some example output:

$ node --inspect=10001 cluster.js master
Debugger listening on ws://127.0.0.1:10001/cb5774d6-7c8b-4a91-93fd-4c700a5349f4
For help see https://nodejs.org/en/docs/inspector
[MASTER] /usr/bin/node --inspect=10001 cluster.js master
Debugger listening on ws://127.0.0.1:10002/ffb4f474-3e4d-42c7-a8d7-45f0b19d9924
For help see https://nodejs.org/en/docs/inspector
[WORKER] /usr/bin/node --inspect=20002 --inspect-port=10002 cluster.js worker

What can be seen, here, is that the worker is forked with the configured argument from the cluster options' execArgv; but, the --inspect-port=10002 argument is also present, though it wasn't configured.

The References

The cluster Module Code

The code that does this appears to be the createWorkerProcess(id, env) function at /lib/internal/cluster/master.js:101. From this, we can see that this behavior appears to be intentional, at least as far as what the code says. Here's what I read from there:

1. If any of the cluster's configured execArgv arguments look like --inspect, --inspect-brk, --inspect-port or --debug-port...
   1. If an inspectPort is defined in the cluster settings...
      1. Get its value or run it as a getter function, saving the value as inspectPort.
   2. ...else...
      1. increment the master's debug port and save that value as inspectPort.
   3. Push `--inspect-port=${inspectPort}` into the worker's execArgv array.

The Documentation

In the CLI docs, all three of --inspect, --inspect-brk and --inspect-port are indicated to support the [=[host:]port] value. However, conceptually, based on what they each do, it seems like they would never be used together.

In the cluster docs, settings.inspectPort is indicated to set the inspect port of the worker.

The Problems

So, here are the problems, as I see them:

• Initially, without reading the code, it would seem that setting settings.inspectPort would allow one to configure the inspect port; however, that setting is completely ignored unless execArgv already has one of the inspect arguments present (which could be --inspect-port, resulting in two of the same argument).
• When there is an inspect argument present in execArgv, the selected inspectPort value ultimately results in the addition of the --inspect-port option; however, in the documentation, that argument is indicated to only configure what the inspect port will be when/if inspection is latently activated.
• While it seems, conceptually, that each of the inspect arguments would be mutually exclusive in practice, understandably there must be an order of precedence when they are combined; however, the cluster code explicitly combines them by intentionally adding --inspect-port when it knows there's already an inspect argument present.

Ultimately, the only option one has to be able to configure the inspect port for a worker is to both (1) add one of the inspect arguments and (2) set the settings.inspectPort. This will result in execArgv having something like --inspect --inspect-port=####. In that case, one might as well leave off the port from the original argument as it will be overridden by the added one.

All of this is fairly confusing to me. Why would the cluster code intentionally combine inspect arguments? If there is a precedence, it's opaque (i.e. it's not indicated in the documentation). Is this operating as expected? Am I just missing something and thinking about this "wrong"?

---

P.S. The code for this in 10.x, while it has received some changes, does still appear to exhibit this behavior -- though I have not tested it myself.

[nazrhyn]

While circling back on this, I was looking into the history on that file in the cluster module, and it looks like this behavior was added in 05707a9 (from #14140) by @mutantcornholio and committed by @Fishrock123 back on July 10, 2017, which means it's new for 8.x. I wonder if either of them have any thoughts on my thoughts, above?

As a quick TL;DR, since this will go with the email notification:

Even if a worker process is configured with a port already, as --inspect=<port>, the regex ignores that and only detects that we're in "debugging mode", then adds a port anyway. IMO, the detection should be smarter, realizing that the --inspect option has a value, and not add the --inspect-port option at all.

Also, the documentation is unclear on the precedence of the various --inspect* options (it turns out that the port of --inspect-port=<port> overrides the port of an --inspect=<port> option).

[mutantcornholio]

Also, the documentation is unclear on the precedence of the various --inspect* options (it turns out that the port of --inspect-port= overrides the port of an --inspect= option).

Seems like the rule is "last one wins". Maybe we should clear that out in docs, yeah.

node --inspect-port=4567 --inspect=3456
Debugger listening on ws://127.0.0.1:3456/7484ab84-7d40-4e2f-8f2b-b17426ba40b9
For help see https://nodejs.org/en/docs/inspector
>

node --inspect=3456 --inspect-port=4567
Debugger listening on ws://127.0.0.1:4567/f8394137-2bc0-4cc5-88f4-4849669e4c4a
For help see https://nodejs.org/en/docs/inspector
>

IMO, the detection should be smarter, realizing that the --inspect option has a value, and not add the --inspect-port option at all.

--inspect-port overrides port part from --inspect; and adding and current logic covers all cases of combining those in execArgv. I've tried implementing altering port part in --inspect, it was too complex with IMHO no profit whatsoever.

Am I getting your problem right? You want to set inspectPort in cluster settings and later activate inspect mode by using SIGUSR1?

[nazrhyn]

@mutantcornholio Thanks for the reply.

If you check out the code example at the top of the issue, that should show what the intention is. I want to propagate the "debug state" from the master to the worker with a port of my choosing. I'm doing that by doing (fairly similar to your code) detection on the args of the master, then adding --inspect=<port> to the args of the worker.

The problem is that that also triggers the detection you added, which doesn't consider the port I've already set.

All of that is described in much more detail in the main, issue, though; so, I don't want to keep talking in case I contradict myself in some later restating 😁.