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: moved dgram bind shared ports to sequential #12999

Closed
wants to merge 1 commit into from
Closed

test: moved dgram bind shared ports to sequential #12999

wants to merge 1 commit into from

Conversation

arturgvieira-zz
Copy link

@arturgvieira-zz arturgvieira-zz commented May 12, 2017

The usage of common.PORT could cause undesired port collisions when run
in parallel. The following test was moved to sequential.
test-dgram-bind-shared-ports.js

Ref: #12376

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • 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 May 12, 2017
@arturgvieira-zz
Copy link
Author

I'm not 100% on this one, just because of the necessity for the +1 mechanism, any advice would be appreciated.

@@ -64,10 +64,10 @@ if (cluster.isMaster) {

socket1.bind({
address: 'localhost',
port: common.PORT,
port: 0,
Copy link
Member

Choose a reason for hiding this comment

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

If I'm reading the test correctly this can't be a dynamic port as the same port should be used by the two workers.

@lpinca
Copy link
Member

lpinca commented May 12, 2017

I think it's better to move this test in sequential but wait for other feedback.

@gibfahn
Copy link
Member

gibfahn commented May 12, 2017

This patch does seem to work on my machine, not sure how.

EDIT: That is to say, the test does pass.

@lpinca
Copy link
Member

lpinca commented May 12, 2017

@gibfahn yes but I think it is not doing what the test is supposed to do, which is testing the exclusive option.

@mscdex mscdex added dgram Issues and PRs related to the dgram subsystem / UDP. cluster Issues and PRs related to the cluster subsystem. labels May 12, 2017
@arturgvieira-zz
Copy link
Author

Got it, I'll await feedback and go from there.

@arturgvieira-zz
Copy link
Author

@lpinca I just noticed that this PR is a duplicate. I apologize. #12452

@lpinca
Copy link
Member

lpinca commented May 12, 2017

No problem.

@arturgvieira-zz
Copy link
Author

In the other PR, is it not landing because it would be preferred to use process.send . I can make that edit if it solves this issue.

@lpinca
Copy link
Member

lpinca commented May 12, 2017

#12452 seems ready to go apart from the merge conflicts.

@arturgvieira-zz
Copy link
Author

arturgvieira-zz commented May 12, 2017

Yeah, it looks like the PR may have gone inactive. I'll wait for a little more info.

@arturgvieira-zz
Copy link
Author

arturgvieira-zz commented May 12, 2017

@lpinca Ok, so I'm starting to understand the exclusive property better. I understand what you mean by the exclusive property not being utilized. So in my tests second round, it picks a new port and it does not conflict as it should. Is that deduction correct?

@lpinca
Copy link
Member

lpinca commented May 12, 2017

Yup.

@arturgvieira-zz
Copy link
Author

Ok thank you

@arturgvieira-zz
Copy link
Author

I'm checking and I am getting that it is picking the same port, both times. @lpinca could you confirm?

nestroia1:~/workspace (commonPort08-branch) $ ./node ./test/parallel/test-dgram-bind-shared-ports.js
49049
49049

by adding:

}, () => {
    console.log(socket1.address().port); // This
    socket2.bind({ port: socket1.address().port + 1, exclusive: true }, () => {

@arturgvieira-zz
Copy link
Author

arturgvieira-zz commented May 12, 2017

I started to reverse the changes in my local branch and debugged first with common.PORT then with an initial dynamic port. Results are similar.

@arturgvieira-zz
Copy link
Author

arturgvieira-zz commented May 12, 2017

Would that behavior scale? I'm not sure, technically speaking. So I will stand by for further review and feedback. It's all the same to me.

@lpinca
Copy link
Member

lpinca commented May 13, 2017

@arturgvieira you are right, it seems to pick up the same port also when using 0 and this explains why the test passes with the changes.
socket1.address().port + 1 is still an issue though as discussed in #12452.

@arturgvieira-zz
Copy link
Author

arturgvieira-zz commented May 13, 2017

@lpinca I made some changes. This will get assigned a random port then maintain that port for the collision testing. Let me know what you think. I'm not sure how to remove the + 1 though. I tried and it lost the port collision issue.

// the first worker should succeed
port = socket1.address().port;
socket2.bind({
port: 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.

There is no guarantee that this port is not being used by another process.

Copy link
Author

Choose a reason for hiding this comment

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

Very true, I am trying to find some way to remove this type of syntax from the test.

socket1.bind({
address: 'localhost',
port: common.PORT,
port: port,
Copy link
Member

Choose a reason for hiding this comment

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

This is still 0 for the second worker.

exclusive: false
}, () => {
socket2.bind({ port: common.PORT + 1, exclusive: true }, () => {
// the first worker should succeed
port = socket1.address().port;
Copy link
Author

Choose a reason for hiding this comment

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

Please let me know if I am correct to say that the first worker reassigns port1 to the bound port in this line, so the second worker is using that port, not zero.

Copy link
Member

Choose a reason for hiding this comment

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

No, this is not correct, each worker has its own context.

Copy link
Author

@arturgvieira-zz arturgvieira-zz May 13, 2017

Choose a reason for hiding this comment

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

I understand, that is why I am having problems with the port assignments. Thank you

@arturgvieira-zz
Copy link
Author

arturgvieira-zz commented May 13, 2017

I was able to reduce the usage of common.PORT, is this better?

@lpinca
Copy link
Member

lpinca commented May 13, 2017

I still think it's better to just move this test to sequential, no added complexity, no changes.

@arturgvieira-zz
Copy link
Author

arturgvieira-zz commented May 13, 2017

I agree, there are a couple other tests that might be the same.

@arturgvieira-zz arturgvieira-zz changed the title test: replace port in dgram shared ports test test: moved dgram bind shared ports to sequential May 13, 2017
The usage of common.PORT could cause undesired port collisions when run
in parallel. The following test was moved to sequential.
test-dgram-bind-shared-ports.js

Ref: #12376
@lpinca
Copy link
Member

lpinca commented May 13, 2017

@arturgvieira I merged #12452 because

  • It was opened first.
  • It was already reviewed.
  • It was @rafaelfragosom's first contribution.

Hope you don't mind.

@arturgvieira-zz
Copy link
Author

I don't mind.

@arturgvieira-zz arturgvieira-zz deleted the commonPort08-branch branch May 13, 2017 22:19
refack added a commit to refack/node that referenced this pull request 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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
addaleax pushed a commit that referenced this pull request 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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
refack added a commit to refack/node that referenced this pull request 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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request 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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request 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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request 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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request 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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
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.

6 participants