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: workers can't share socket if created with port 0 then recreated with assigned port number #13526

Closed
refack opened this issue Jun 7, 2017 · 10 comments
Labels
cluster Issues and PRs related to the cluster subsystem. known limitation Issues that are identified as known limitations.

Comments

@refack
Copy link
Contributor

refack commented Jun 7, 2017

  • Version: master
  • Platform: *
  • Subsystem: cluster

Found while working on #13100
Failing code can be found here

@refack refack self-assigned this Jun 7, 2017
@refack refack added cluster Issues and PRs related to the cluster subsystem. confirmed-bug Issues with confirmed bugs. labels Jun 7, 2017
@sam-github
Copy link
Contributor

server.listen(0) Normally, this will cause servers to listen on a random port. However, in a cluster, each worker will receive the same "random" port each time they do listen(0). In essence, the port is random the first time, but predictable thereafter. To listen on a unique port, generate a port number based on the cluster worker ID.

Cluster does not work for all use cases. Various suggestions have been made to improve this, they generally make one previously unsupported use-case work, and make a curretly supported one stop working.

@refack
Copy link
Contributor Author

refack commented Jun 7, 2017

Ohh that makes sense.
The literal way doesn't work (bind in one worker, then communicate port number to other workers).

@refack refack removed the confirmed-bug Issues with confirmed bugs. label Jun 7, 2017
@refack refack changed the title cluster: workers can't share socket if created with port 0 cluster: workers can't share socket if created with port 0 then recreated with assigned port number Jun 7, 2017
@sam-github
Copy link
Contributor

You don't have to communicate to others. If you bind(0) in two different workers, you will get the same port in both workers, cluster will arrange this, no communication needed (by you). Unfortunately, as you report, if you bind(0) twice in the same worker, you will also get the same port.... which is almost certainly not what you wanted.

@sam-github
Copy link
Contributor

and in case its not clear: if you bind(0) twice in one worker, and they got two different ports, as you would likely want, then when the same code runs in another workern and binds twice to 0... which of the binds gets which one of the two different ports from the other workere? Its impossible for node to know :-(, so it doesn't support this.

@refack
Copy link
Contributor Author

refack commented Jun 7, 2017

and in case its not clear: if you bind(0) twice in one worker, and they got two different ports, as you would likely want, then when the same code runs in another workern and binds twice to 0... which of the binds gets which one of the two different ports from the other workere? Its impossible for node to know :-(, so it doesn't support this.

Yeah I saw the code, master indexes it under port 0 for sharing, so it works if and only if workers do one bind(0) (and only port 0, not the actual assigned port).
I agree it's not a bug, but a known limitation (we should have a known limitation tag)

@refack refack added the known limitation Issues that are identified as known limitations. label Jun 7, 2017
@sam-github
Copy link
Contributor

The docs could be expanded to not just describe how it works, but the kinds of thing that won't work.

@mscdex
Copy link
Contributor

mscdex commented Jun 7, 2017

FWIW: #7043

@refack
Copy link
Contributor Author

refack commented Jun 7, 2017

@mscdex #7043 looks promising, you plan to follow up? I could continue your work...

@mscdex
Copy link
Contributor

mscdex commented Jun 7, 2017

@refack I think we still need to get some more input from other collaborators about whether it should be a behavior change or opt-in change via config option or something else.

@bnoordhuis
Copy link
Member

Let's move the discussion to #7043. I'll close this out but FWIW, I'm supportive of making it configurable.

refack added a commit to refack/node that referenced this issue Jun 9, 2017
* add `mustCall` and `mustNotCall` to all callbacks
* added `known_issue` for port binding

PR-URL: nodejs#13100
Refs: nodejs#13055
Refs: nodejs#12999
Refs: nodejs#13526
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
addaleax pushed a commit that referenced this issue Jun 10, 2017
* add `mustCall` and `mustNotCall` to all callbacks
* added `known_issue` for port binding

PR-URL: #13100
Refs: #13055
Refs: #12999
Refs: #13526
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
refack added a commit to refack/node that referenced this issue Jul 17, 2017
* add `mustCall` and `mustNotCall` to all callbacks
* added `known_issue` for port binding

PR-URL: nodejs#13100
Refs: nodejs#13055
Refs: nodejs#12999
Refs: nodejs#13526
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 14, 2017
* add `mustCall` and `mustNotCall` to all callbacks
* added `known_issue` for port binding

PR-URL: #13100
Refs: #13055
Refs: #12999
Refs: #13526
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 14, 2017
* add `mustCall` and `mustNotCall` to all callbacks
* added `known_issue` for port binding

Backport-PR-URL: #14327
PR-URL: #13100
Refs: #13055
Refs: #12999
Refs: #13526
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 14, 2017
* add `mustCall` and `mustNotCall` to all callbacks
* added `known_issue` for port binding

Backport-PR-URL: #14327
PR-URL: #13100
Refs: #13055
Refs: #12999
Refs: #13526
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 16, 2017
* add `mustCall` and `mustNotCall` to all callbacks
* added `known_issue` for port binding

Backport-PR-URL: #14327
PR-URL: #13100
Refs: #13055
Refs: #12999
Refs: #13526
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@refack refack removed their assignment Oct 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cluster Issues and PRs related to the cluster subsystem. known limitation Issues that are identified as known limitations.
Projects
None yet
Development

No branches or pull requests

4 participants