cluster,net: fix random port keying #7043

mscdex · 2016-05-29T07:14:49Z

Checklist

tests and code linting passes
a test and/or benchmark is included
the commit message follows commit guidelines

Affected core subsystem(s)

cluster
net

Description of change

This commit fixes an issue where sockets that are bound to a random port are keyed on port 0 instead of the actual port that was assigned to the socket.

mscdex · 2016-05-29T07:18:47Z

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

thefourtheye · 2016-05-29T07:28:40Z

lib/cluster.js

+        if (!errno)
+          handles[key] = handle;
+        data = handle.data;
+        handle.key = key;


Shouldn't this be within the !errno check?

It shouldn't really matter. I just did it there so that it's overwritten no matter if there should happen to be another reference to the handle now or in the future.

mscdex · 2016-05-29T09:13:35Z

Fixed Windows issue. CI again: https://ci.nodejs.org/job/node-test-pull-request/2845/

mscdex · 2016-05-29T09:45:59Z

I'm not sure how to label this one (e.g. semver-major or not). It could be breaking backwards compatibility if there are people relying on the previous behavior (for example the parallel/test-dgram-exclusive-implicit-bind.js test), but it could be seen as a bug fix to some degree.

What does everyone else think?

Trott · 2016-05-29T21:22:25Z

What does everyone else think?

Maybe run CITGM?

mscdex · 2016-05-29T22:02:24Z

citgm: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/284/

mscdex · 2016-05-30T10:52:40Z

citgm looks green.

cjihrig · 2016-06-02T16:46:56Z

test/parallel/test-net-server-bind.js

-  assert.equal(common.PORT + 2, address3.port);
-  assert.equal(common.PORT + 3, address4.port);
+
+  assert.strictEqual(typeof address2.port, 'number');


Could you DRY this by moving the checks into a function.

mscdex · 2016-06-07T03:21:26Z

@cjihrig Addressed your comments

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

mscdex · 2016-06-11T02:39:43Z

/cc @nodejs/collaborators Any thoughts?

ronkorving · 2016-06-13T03:50:34Z

This sounds like a very welcome change. I never understood why on a cluster, port 0 would just mean each worker would be assigned a random port. This solution works, although imho there is one concern:
If I run more than 1 server from a Node cluster, I can't use 2 random ports (1 for each service).

This commit fixes an issue where sockets that are bound to a random port are keyed on port 0 instead of the actual port that was assigned to the socket.

mscdex · 2016-06-13T04:53:54Z

@ronkorving I've fixed that now and included a new test. Please take a look and let me know if that was what you had in mind.

ronkorving · 2016-06-13T07:28:26Z

Conceptually, how would I create both an HTTP server and a separate TCP server that both listen on a random port, and are both clustered? I'm not cluster-savvy enough atm to have a very critical eye to your solution. But humor me please on the conceptual level :)

mscdex · 2016-06-13T07:54:38Z

@ronkorving So you're asking to put this new functionality behind a new listen() option so that you can still have the old behavior ? If not, I'm not sure what you mean by "clustered."

Without this PR, all servers bound in workers that listen on port "0" all refer to the exact same port (whatever it ultimately gets resolved to by the OS) because the master includes the port number in the key name in its handle table. So if you want to spin up another server on a random port within any of the workers, you're SOL as the master will see a server already listening on port "0" and you will either get unexpected behavior (if you have different types of servers sharing the port) or you will get an EADDRINUSE (in the exclusive: true case).

With this PR, workers can listen on as many random ports as they want. This means that port "0" handles are no longer permanently stored by the master (they get re-indexed using the real port after the port is bound) and thus are not automatically shared amongst all workers when they all listen on port "0." However, the port it ultimately resolves to will be shareable (assuming you don't set exclusive: true), so if one worker listens on a random port you would basically have to let the other workers know the real port to listen on. So it would require a little more work since you'd have to do some message passing to the other workers, but the upside is workers are not all locked to one, same, random port.

I hope that makes some sense.

ronkorving · 2016-06-13T08:23:40Z

I don't want the old behavior at all (I think it's useless). But there are 2 valid behaviors imho, that hopefully could co-exist.

Scenario 1:

I have an HTTP server on a random port
I run my Node.js setup in cluster mode
Every worker's HTTP server listens on the same random port that was determined only once

Scenario 2:

I have an HTTP service (a) on a random port
I have another HTTP service (b) on another random port
I run my Node.js setup in cluster mode
Every worker's HTTP server (a) listens on the same random port that was determined only once (eg: 1234)
Every worker's HTTP server (b) listens on the same random port that was determined only once (eg: 4321)

I understand from your comments that scenario 1 is possible, and scenario 2 requires some more userland intervention, but is possible?

mscdex · 2016-06-13T08:54:42Z

With this PR it's the same process no matter the scenario. If a worker listens on a random port, the real port needs to be broadcasted to the other workers in order for them to listen on that same port. This can actually be done a couple of different ways:

Worker 1 could do something like:

server.listen(0, function() {
  process.send({ type: 'rest', port: this.address().port });
});
server2.listen(0, function() {
  process.send({ type: 'healthcheck', port: this.address().port });
});

and in the master:

worker1.on('message', function(msg) {
  if (msg.type && msg.port) {
    Object.keys(cluster.workers).forEach(function(worker) {
      if (worker === worker1)
        return;
      worker.send(msg);
    });
  }
});

then the other workers watch for those messages and then listen on the port(s).

OR

Use the master's listening event to broadcast the port(s) to the other workers. This is probably less feasible since you won't have any context associated with the address/port passed to the event handler (e.g. which server is which).

sam-github · 2016-06-14T19:39:36Z

With this PR, workers can listen on as many random ports as they want. This means that port "0" handles are no longer permanently stored by the master (they get re-indexed using the real port after the port is bound) and thus are not automatically shared amongst all workers when they all listen on port "0."

First, this is absolutely semver-major, the fact that all cluster workers listen on the same port with an app that listens on port 0 is documented.

Second, I get that there are multiple use-cases, but I'd suggest that an API that expresses the intent would be better than breaking the current use case, particularly when it might enable other use cases, but not completely, you have to write a bunch more code to get anything useful.

Right now, the common case (one listen on 0 in a single workers .js file) works with no additional code.

This PR breaks that use, it would make every worker listen on a different port, which means its effectively not clustered.

It breaks it in favour of a model where if you listen multiple times in one worker every single listen port is ephemeral... which strikes me as wholly undesireable, so now you have to write complex cluster code to coordinate, code that isn't even possible to write (there is no worker to worker messaging supported directly by the cluster lib) if you don't control the implementation of the cluster master, which you don't with most supervisor-type code bases which auto-cluster applications.

If some way could be found so that the listens could be (random bad API example design) on some special token, maybe ephemeral:http and ephemeral:https, so that with no cluster, both of those would be the same as listen 0 (they would get unique ports), but with cluster, all the workers would get the same port for http, and a different but same across worker port for https, it would be better.

mscdex · 2016-06-14T19:42:33Z

@sam-github I've already proposed putting it behind an option if that's what everyone wants.

sam-github · 2016-06-14T21:48:52Z

@mscdex where? I'm not finding that proposal in the PR comments.

What kind of option? node CLI option? an option to cluster.setup()?

sam-github · 2016-06-14T21:51:15Z

@mscdex also, what about my suggestion of reworking the API so instead of some global switch, we allow the code doing the listen to say what it wants, true ephemeral, or shared-ephemeral? These global everything one way or everything the other switches I feel eventually run afoul of code that is combined, wherein some code wants one, some the other, and you are stuck....

Also, I'd be interested in what the use case is for having every worker listening on worker-specific/unclustered ports. This is the cluster module... not clustering seems odd.

mscdex · 2016-07-13T04:27:14Z

Is an additional boolean config option acceptable for this then? If so, does anyone have a good name they want to throw out there?

trevnorris · 2016-07-20T08:44:14Z

Sounds like a good option to me, but don't have any good ideas for the name.

jasnell · 2016-08-09T22:02:31Z

pulled the dont-land-on-v4.x label since it's a semver-major that wouldn't land there anyway

jasnell · 2016-08-09T22:03:08Z

@mscdex @Trott @trevnorris ... ping. what's next on this?

mscdex · 2016-08-09T22:27:45Z

@jasnell If most people are in favor of adding this behavior under a separate option, then I will go ahead and add one.

jasnell · 2016-08-09T22:28:51Z

seems like a good idea to me. /cc @nodejs/collaborators

imyller · 2016-09-24T19:55:43Z

CI with current master: https://ci.nodejs.org/job/node-test-pull-request/4261/

gibfahn · 2016-09-25T17:38:31Z

Something went wrong with that last CI run (node-test-pull-request never triggered node-test-commit), rerunning...

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

mscdex · 2016-09-25T19:48:59Z

Don't worry about CI right now, I need to change this PR still.

imyller · 2016-09-25T22:35:13Z

Added in-progress label

jasnell · 2017-03-01T01:29:40Z

@mscdex ... still want to pursue this?

mscdex · 2017-03-01T02:52:48Z

@jasnell At some point, yes.

BridgeAR · 2017-06-08T06:46:52Z

lib/cluster.js

@@ -591,6 +633,16 @@ function workerInit() {
    send(message, function(reply, handle) {
      if (obj._setServerData) obj._setServerData(reply.data);

+      if (+options.port === 0 && reply.sockname) {


Nit: As the +options.port coercion is done twice you might just move that in a variable above?

bnoordhuis · 2017-06-08T09:23:56Z

I think this PR can be made more general. Proposal:

Add a shared: true/false message property to the 'queryServer' command.
In the child, do message.shared = (message.port !== 0)
In the master, only do handle.add(...) if message.shared === false (send but don't cache.)
If sending fails, close the handle, don't send it to another worker (implied by not caching it.)

That opens a path for supporting unshared sockets for more use cases than just the port 0 scenario.

EDIT: The unshared behavior should be opt-in for users, though. We can't and shouldn't break the current behavior.

BridgeAR · 2017-08-27T03:06:09Z

Ping @mscdex

mscdex · 2017-08-27T05:33:47Z

@BridgeAR I'm not going to have the time to fix this up unfortunately. If someone else wants to work on it or do it from scratch in the way that @bnoordhuis suggested, that's fine by me.

BridgeAR · 2017-12-05T02:59:53Z

Closing due to long inactivity. If someone wants to pick it up, please feel free to open a new PR with the suggested code change.

mscdex added cluster Issues and PRs related to the cluster subsystem. net Issues and PRs related to the net subsystem. labels May 29, 2016

thefourtheye reviewed May 29, 2016
View reviewed changes

mscdex force-pushed the cluster-fix-random-port-handling branch from 9baa080 to dc76278 Compare May 29, 2016 09:11

mscdex added the dont-land-on-v4.x label May 29, 2016

mscdex mentioned this pull request May 29, 2016

test: use random ports where possible #7045

Merged

2 tasks

cjihrig reviewed Jun 2, 2016
View reviewed changes

mscdex force-pushed the cluster-fix-random-port-handling branch from dc76278 to 21090a0 Compare June 7, 2016 03:20

cluster,net: fix random port keying

d87bc5f

This commit fixes an issue where sockets that are bound to a random port are keyed on port 0 instead of the actual port that was assigned to the socket.

mscdex force-pushed the cluster-fix-random-port-handling branch from 21090a0 to d87bc5f Compare June 13, 2016 04:53

Trott mentioned this pull request Jun 29, 2016

test: remove common.PORT from http tests #7467

Closed

3 tasks

jasnell removed the dont-land-on-v4.x label Aug 9, 2016

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:09

imyller added the wip Issues and PRs that are still a work in progress. label Sep 25, 2016

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:01

MylesBorins force-pushed the master branch from 8df7ee0 to 54fef67 Compare February 1, 2017 01:00

jasnell added the stalled Issues and PRs that are stalled. label Mar 1, 2017

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

mscdex mentioned this pull request Apr 24, 2017

test: dynamic port in dgram bind shared ports #12620

Closed

3 tasks

mscdex mentioned this pull request Jun 7, 2017

cluster: workers can't share socket if created with port 0 then recreated with assigned port number #13526

Closed

refack mentioned this pull request Jun 7, 2017

test,dgram: harden test-dgram-bind-shared-ports.js #13100

Merged

3 tasks

BridgeAR reviewed Jun 8, 2017

View reviewed changes

BridgeAR closed this Dec 5, 2017

cluster,net: fix random port keying #7043

cluster,net: fix random port keying #7043

Conversation

mscdex commented May 29, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

mscdex commented May 29, 2016

thefourtheye May 29, 2016

Choose a reason for hiding this comment

mscdex May 29, 2016 • edited Loading

Choose a reason for hiding this comment

mscdex commented May 29, 2016

mscdex commented May 29, 2016 • edited Loading

Trott commented May 29, 2016

mscdex commented May 29, 2016 • edited Loading

mscdex commented May 30, 2016

cjihrig Jun 2, 2016

Choose a reason for hiding this comment

mscdex commented Jun 7, 2016 • edited Loading

mscdex commented Jun 11, 2016

ronkorving commented Jun 13, 2016

mscdex commented Jun 13, 2016

ronkorving commented Jun 13, 2016

mscdex commented Jun 13, 2016 • edited Loading

ronkorving commented Jun 13, 2016 • edited Loading

mscdex commented Jun 13, 2016 • edited Loading

sam-github commented Jun 14, 2016

mscdex commented Jun 14, 2016

sam-github commented Jun 14, 2016

sam-github commented Jun 14, 2016

mscdex commented Jul 13, 2016

trevnorris commented Jul 20, 2016

jasnell commented Aug 9, 2016

jasnell commented Aug 9, 2016

mscdex commented Aug 9, 2016

jasnell commented Aug 9, 2016

imyller commented Sep 24, 2016

gibfahn commented Sep 25, 2016

mscdex commented Sep 25, 2016 • edited Loading

imyller commented Sep 25, 2016

jasnell commented Mar 1, 2017

mscdex commented Mar 1, 2017

BridgeAR Jun 8, 2017

Choose a reason for hiding this comment

bnoordhuis commented Jun 8, 2017 • edited Loading

BridgeAR commented Aug 27, 2017

mscdex commented Aug 27, 2017

BridgeAR commented Dec 5, 2017

mscdex commented May 29, 2016 •

edited

Loading

mscdex May 29, 2016 •

edited

Loading

mscdex commented May 29, 2016 •

edited

Loading

mscdex commented May 29, 2016 •

edited

Loading

mscdex commented Jun 7, 2016 •

edited

Loading

mscdex commented Jun 13, 2016 •

edited

Loading

ronkorving commented Jun 13, 2016 •

edited

Loading

mscdex commented Jun 13, 2016 •

edited

Loading

mscdex commented Sep 25, 2016 •

edited

Loading

bnoordhuis commented Jun 8, 2017 •

edited

Loading