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: use dynamic port in test-dgram-bind-shared-ports #12452

Closed
wants to merge 4 commits into from

Conversation

orafaelfragoso
Copy link
Contributor

Use of common.PORT in parallel tests is not completely safe (because
the same port can be previously assigned to another test running in
parallel if that test uses port 0 to get an arbitrary available port).

Remove common.PORT from test-dgram-bind-shared-ports.

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 16, 2017
socket2.bind({ port: common.PORT + 1, exclusive: true }, () => {
}, common.mustCall(() => {
socket2.bind({
port: socket1.address().port + 1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is correct. There's no guarantee that this port is in use by a different process. TBH I'm not sure there's a better solution than using common.PORT, maybe someone has a better idea.

Copy link
Member

@gibfahn gibfahn Apr 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason that this port has to be the first port + 1? Couldn't it just be port: 0 as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That won't work as it would use a different port on every worker.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, both workers need to use the same two port numbers for both sockets. (In other words, each worker has two sockets. And those sockets should have duplicate ports across workers.)

I think the choices are:

  • introduce code to communicate ports between the two workers, or
  • move the test as-is to sequential

Copy link
Member

@Trott Trott Apr 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, the change already in place in this test is incorrect because it guarantees that socket1 doesn't use the same port in worker1 and worker2. Both workers should use the same port. (Nothing to feel bad about with missing that. It's not a completely intuitive test. It certainly took me a while to figure out what was going on.)

I think the way to go is to move the test as it currently exists on the master branch from the parallel directory to the sequential directory.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the change already in place in this test is incorrect because it guarantees that socket1 doesn't use the same port in worker1 and worker2.

I'm not sure about this. I think that socket1 being a non-exclusive socket will use the same port in both workers. That's not the case with socket2 though.

I think the way to go is to move the test as it currently exists on the master branch from the parallel directory to the sequential directory.

I agree.

@santigimeno santigimeno added cluster Issues and PRs related to the cluster subsystem. dgram Issues and PRs related to the dgram subsystem / UDP. labels Apr 16, 2017
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using socket1.address().port +1 is still a problem. See comments. This one might be do-able using operating system-assigned ports, but the easiest thing to do might be to move it from parallel to sequential and don't make any changes to the code. If someone wants to refactor it to use dynamic ports at a later date, then great. If not, that's fine too.

@orafaelfragoso
Copy link
Contributor Author

I'm lost. The original test was using common.PORT + 1. So that means it was wrong from the start?

@Trott
Copy link
Member

Trott commented Apr 18, 2017

I'm lost. The original test was using common.PORT + 1. So that means it was wrong from the start?

No, the original test was fine.

The test creates two workers. Each worker creates two dgram sockets. One is on common.PORT and one is on common.PORT + 1. The whole point of the test is that both workers each try to use the same ports. The first dgram socket is configured in such a way that it is not a problem that they are using the same port. The second dgram socket is configured in such a way that this results in an error. The test was checking to see if that behavior was being seen or not.

By changing common.PORT to 0, the port collision (which is an essential part of the test) is removed.

@orafaelfragoso
Copy link
Contributor Author

I see. Thanks for clearing that up.

What would you recommend me doing?

@Trott
Copy link
Member

Trott commented Apr 18, 2017

What would you recommend me doing?

I would recommend rewriting the test so that the ports selected by the first worker can be communicated to the second worker so it can use the same ports. Probably use process.send() in worker1 to send the port information to the cluster master process, which can then send it to worker2.

@orafaelfragoso
Copy link
Contributor Author

@Trott is there a place where we can chat about node? I would love to get some more insights from you.

I'll try to make these changes.

@Trott
Copy link
Member

Trott commented Apr 18, 2017

@rafaelfragosom A large portion of the regular contributors to Node.js (including me) are often in the #node-dev channel on Freenode IRC, so that might be a place to get insights from a bunch of people.

@orafaelfragoso
Copy link
Contributor Author

@Trott I moved the test to sequential and restored the use of common.PORT(). I'll be glad to understand and help with the other approach you mentioned (dynamic ports).

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Trott
Copy link
Member

Trott commented Apr 27, 2017

@lpinca
Copy link
Member

lpinca commented May 13, 2017

@rafaelfragosom can you please rebase against master?

Use of common.PORT in parallel tests is not completely safe (because
the same port can be previously assigned to another test running in
parallel if that test uses port 0 to get an arbitrary available port).

Remove common.PORT from test-dgram-bind-shared-ports.
Adding a common.mustCall() to socket1 callback on
test-dgram-bind-shared-ports to make sure the callback
is being called.
As was pointed out on [this
review](nodejs#12452 (review))
I moved the test to the sequential folder.

Refs: nodejs#12376
Restoring the contents of the test to it's original with common.PORT()

Refs: nodejs#12376
@orafaelfragoso
Copy link
Contributor Author

@lpinca I think I just did it, let me know.

@lpinca
Copy link
Member

lpinca commented May 13, 2017

@rafaelfragosom thank you.

@lpinca
Copy link
Member

lpinca commented May 13, 2017

lpinca pushed a commit to lpinca/node that referenced this pull request May 13, 2017
PR-URL: nodejs#12452
Ref: nodejs#12376
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@lpinca
Copy link
Member

lpinca commented May 13, 2017

Landed in 84fc069.

@lpinca lpinca closed this May 13, 2017
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
PR-URL: nodejs#12452
Ref: nodejs#12376
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@jasnell jasnell mentioned this pull request May 28, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
MylesBorins pushed a commit that referenced this pull request Jun 22, 2017
PR-URL: #12452
Ref: #12376
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
PR-URL: #12452
Ref: #12376
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
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.

9 participants