cluster: only assign debug port if in valid range.

[mjseidel]

See: #8159

Per above discussion, cluster.fork() currently sends --debug-port to
forked processes regardless of whether debug is enabled, and more
importantly, without any bounds checking. This will rather unexpectedly
crash any Node process that forks enough times.

This patch simply bounds checks --debug-port and doesn't set arg if out
of bounds (V8 requires 1024 < debug-port < 65535). This will prevent
surprises to callers of cluster.fork() while waiting for the debug part
of node to be rewritten as mentioned in issue discussion above.

[bahamas10]

I don't know about this syntax...

$ node > 1 < 2 < 3 true > 1 < 5 < 3 true > 1 < 0 < 3 true 

you probably need

if (debugPort > 1024 && debugPort < 65535) ... 

[sam-github]

Don't those numbers depend on your system configuration?

[mjseidel]

@sam-github I don't believe so; these numbers are hardcoded in node.cc:

https://github.com/joyent/node/blob/4d9c81b7e2522c5d5d9d35058cbb0bce1228d360/src/node.cc#L2903

[mjseidel]

@bahamas10, changing to your syntax and re-committing, thanks.

[cjihrig]

Instead of hard coding port numbers throughout the code, maybe they should be defined as constants. This approach also prevents workers from being debugged after they reach the upper limit. Perhaps the cluster master should track which ports have been allocated to workers.

[cjihrig]

if (debug_port >= 1024 && debug_port <= 65536){

[sam-github]

I think workers should have a "secondary" ID. One only present in the environment, but maintained by cluster, and ID that has the property that new workers always get the lowest unallocated ID > 0.

There are a number of use-cases for such an ID. One is to do things open ports at semi-stable locations, it would all the debug port to be: DEBUG_BASE + ID. I would have other uses for it as well, there are various things I've worked on that would benefit from such an ID.

Of course, ID is a terrible name, workers already have a worker ID, perhaps "index": worker.index(in master)/cluster.index (in workers).

Thoughts, @cjhrig?

[cjihrig]

Is there any reason that the existing id property couldn't be modified to take the lowest available value? If there is a good reason, then such an index might not be a bad idea. This came in over the weekend on io.js and appears to supersede this PR. nodejs/node#1524

[mjseidel]

@cjihrig, This definitely retains some undesirable behavior, like workers not being debuggable above the max port value. I just think it's strictly an improvement over what currently happens, which is a hard crash :)

To your other point about lowest-available id value: forking in Node is already pretty costly timewise, and adding overhead to debug port assignment (even when debug will not be used) could make it costlier. That's just my two cents and I definitely have not deep-dived on this codebase.

[jasnell]

@mjseidel@cjihrig ... how would you like to proceed on this one? If it's a bug in v0.12 that needs to be fixed, then retargeting the PR there is fine. Otherwise, this may need to be reopened against master on http://github.com/nodejs/node

[cjihrig]

This is mostly mitigated in 3.0.0. Now, a debug port is only assigned if the master is started in debug mode. Technically it's still possible to encounter, but you'd have to have a very long running process running in debug mode, which is a bit counter intuitive.

[jasnell]

Ok. then I'd be inclined to close. A new issue/PR can be opened in nodejs/node if it continues to be a problem

[jasnell]

Going ahead and closing here. We can backport the change from 3.x if necessary but pretty sure it's safe to leave this here.