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

test: dynamic port in dgram bind shared ports #12620

Closed
wants to merge 1 commit into from
Closed

test: dynamic port in dgram bind shared ports #12620

wants to merge 1 commit into from

Conversation

sebastianplesciuc
Copy link

Removed common.PORT from test-dgram-bind-shared-ports in order to
eliminate the possibility of port collision with other tests.

Refs: #12376

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Removed common.PORT from test-dgram-bind-shared-ports in order to
eliminate the possibility of port collision with other tests.

Refs: #12376
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 24, 2017
@sebastianplesciuc
Copy link
Author

I have a bit of trouble understanding why this works. If I were to set the socket1.bind() port to be the one that comes from worker1 explicitly, then I would get a EADDRINUSE from socket1 in worker2 even though exclusive is set to false.

While exclusive is false, socket1 gets the same port on both workers while using 0 and I can assert that to check it's true.

@santigimeno
Copy link
Member

santigimeno commented Apr 24, 2017

I have a bit of trouble understanding why this works. If I were to set the socket1.bind() port to be the one that comes from worker1 explicitly, then I would get a EADDRINUSE from socket1 in worker2 even though exclusive is set to false.

That's probably because the key by which the handle is identified in the cluster is different: the first worker would use 0 as port and the second the specific port.

@sebastianplesciuc
Copy link
Author

@santigimeno Thank you for the explanation! Now I get it :)

@santigimeno
Copy link
Member

Even if it's a corner case, I wonder if it should be handled correctly by the cluster module. /cc @bnoordhuis @cjihrig ?

@mscdex mscdex added the dgram Issues and PRs related to the dgram subsystem / UDP. label Apr 24, 2017
@mscdex
Copy link
Contributor

mscdex commented Apr 24, 2017

@santigimeno This is a long-standing issue and is why I did not bother "fixing" the cluster and some other tests when mass converting tests from common.PORT to 0.

See #7043, which was my attempt at trying to fix the cluster random port binding issue.

@santigimeno
Copy link
Member

@mscdex thanks for the info. I've been looking at #7043, and instead of redefining what binding to port 0 means, I was thinking more about solving the specific issue @sebastianplesciuc was referring to and storing the real port number that was assigned to the port 0 query and return the same handle whether a worker queries for port 0 or that specific port. Does it make sense to you?

@santigimeno santigimeno added the cluster Issues and PRs related to the cluster subsystem. label Apr 24, 2017
@sebastianplesciuc
Copy link
Author

sebastianplesciuc commented May 15, 2017

Closing this since it was moved to sequential by this PR

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. dgram Issues and PRs related to the dgram subsystem / UDP. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants