test: use dynamic port in test-cluster-dgram-reuse #12426

orafaelfragoso · 2017-04-15T13:39:50Z

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-cluster-dgram-reuse.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test cluster

benjamingr · 2017-04-15T13:57:12Z

Thanks

Refs: #12376

I'm wondering, are both sockets guaranteed to be created on the same port in this case?

orafaelfragoso · 2017-04-15T14:05:06Z

@benjamingr Great question.

I wonder if there's a way to dynamically get a port from the OS and assign it to a const for every test?

addaleax · 2017-04-15T14:07:14Z

What you can do and what should work is creating the first socket, then using .address().port to get its port and use that for the second one. :)

benjamingr

I think we should do what @addaleax suggested then.

orafaelfragoso · 2017-04-15T14:27:42Z

@addaleax What do you think of this?

// Creates the first socket
const socket = dgram.createSocket({ type: 'udp4', reuseAddr: true });
// Let the OS assign a random port using '0'
socket.bind(0, next);
// Creates the second socket using the same port from the previous one
dgram.createSocket({ type: 'udp4', reuseAddr: true }).bind(socket.address().port, next);

I'm sure I am missing something here. Could you shed me some light? The test is failing with:

=== release test-cluster-dgram-reuse ===                   
Path: parallel/test-cluster-dgram-reuse
dgram.js:489
    throw errnoException(err, 'getsockname');
    ^

Error: getsockname EINVAL
    at exports._errnoException (util.js:1042:11)
    at Socket.address (dgram.js:489:11)
    at Object.<anonymous> (/home/rafael/Development/node/test/parallel/test-cluster-dgram-reuse.js:43:40)
    at Module._compile (module.js:607:30)
    at Object.Module._extensions..js (module.js:618:10)
    at Module.load (module.js:516:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)
    at Module.runMain (module.js:643:10)
    at run (bootstrap_node.js:443:7)

assert.js:86
  throw new assert.AssertionError({
  ^
AssertionError: 1 === 0
    at Worker.cluster.fork.on.common.mustCall (/home/rafael/Development/node/test/parallel/test-cluster-dgram-reuse.js:15:12)
    at Worker.<anonymous> (/home/rafael/Development/node/test/common.js:461:15)
    at emitTwo (events.js:125:13)
    at Worker.emit (events.js:213:7)
    at ChildProcess.worker.process.once (internal/cluster/master.js:188:12)
    at Object.onceWrapper (events.js:312:19)
    at emitTwo (events.js:125:13)
    at ChildProcess.emit (events.js:213:7)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:208:12)
Command: out/Release/node /home/rafael/Development/node/test/parallel/test-cluster-dgram-reuse.js

benjamingr · 2017-04-15T14:32:29Z

@rafaelfragosom I'm not sure port is available immediately, you might need to change it to something like:

// Creates the first socket
const socket = dgram.createSocket({ type: 'udp4', reuseAddr: true });
// Let the OS assign a random port using '0'
socket.bind(0, function() {
  // Creates the second socket using the same port from the previous one
  dgram.createSocket({ type: 'udp4', reuseAddr: true }).bind(socket.address().port, next);
  next.call(this);
});

Although I admit I never tested this.

Edit: tested, this appears to be the case, you can create a third socket if you'd like.

Here's a utility:

function getPort(cb) {
  const s = dgram.createSocket({ type: 'udp4', reuseAddr: true }).bind(0, () => {
    cb(null, s.address().port);
  });
}

You can do that to get the port and then keep the test (inside the callback) as it was just changing 0 to the port.

orafaelfragoso · 2017-04-15T14:37:16Z

@benjamingr It does work. I'm sending the commit.

benjamingr · 2017-04-15T14:48:16Z

technically the test is now slightly different, but it tests the same thing so I'm going to LGTM. We need to give it 72 hours anyway so if other people feel differently they can speak up :)

orafaelfragoso · 2017-04-15T14:50:33Z

@benjamingr Thank you!

This is my first contribution to a big project like node and it's been good so far. I'll get better at it.

benjamingr · 2017-04-15T15:00:36Z

You're doing great :) Thanks for taking the time to contribute.

jkrems · 2017-04-15T15:27:03Z

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

cjihrig · 2017-04-15T19:08:42Z

test/parallel/test-cluster-dgram-reuse.js

+// Creates the first socket
+const socket = dgram.createSocket({ type: 'udp4', reuseAddr: true });
+// Let the OS assign a random port to the first socket
+socket.bind(0, function() {


Can you add common.mustCall() to the callback here.

Yes. I'm still getting used to the tests.

@cjihrig done.

Remove common.PORT from test-cluster-dgram-reuse to eliminate possibility that a dynamic port used in another test will collide with common.PORT. Refs: nodejs#12376

…euse Using the bind() callback to get the dynamically assigned port from the first socket and assigning it to the next cluster.

Fixes the wordwrap error of my last commit based on the guidelines

Adding the common.mustCall() to the callback as the test documentation explains it.

santigimeno · 2017-04-16T19:20:16Z

I wonder if after the proposed changes, the next() machinery is needed at all. I think the sockets could be safely closed in the second socket bind callback.

orafaelfragoso · 2017-04-21T14:02:52Z

@cjihrig Any updates on your request?

orafaelfragoso · 2017-04-21T14:05:05Z

@benjamingr I'm gonna update this test with your suggestion. I was affraid of doing something like this, but since you suggested, I'm gonna do it.

cjihrig

LGTM. I'm not sure if the indentation on the second bind() is right or not though.

lpinca · 2017-05-19T13:49:53Z

Ops, I just landed #12901 which was a dupe of this.

lpinca · 2017-05-19T13:55:40Z

Closing this, sorry @rafaelfragosom.

nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 15, 2017

benjamingr requested changes Apr 15, 2017

View reviewed changes

benjamingr approved these changes Apr 15, 2017

View reviewed changes

mscdex added cluster Issues and PRs related to the cluster subsystem. dgram Issues and PRs related to the dgram subsystem / UDP. labels Apr 15, 2017

orafaelfragoso force-pushed the issue-12376 branch from d083e8e to 627fd01 Compare April 15, 2017 17:03

lpinca approved these changes Apr 15, 2017

View reviewed changes

cjihrig requested changes Apr 15, 2017

View reviewed changes

orafaelfragoso added 4 commits April 15, 2017 17:04

test: use dynamic port in test-cluster-dgram-reuse.js

d5af03c

Remove common.PORT from test-cluster-dgram-reuse to eliminate possibility that a dynamic port used in another test will collide with common.PORT. Refs: nodejs#12376

test: using the dynamic port for both sockets in test-cluster-dgram-r…

43bce0a

…euse Using the bind() callback to get the dynamically assigned port from the first socket and assigning it to the next cluster.

test: fixing the wordwrap in test-cluster-dgram-reuse

7ddb093

Fixes the wordwrap error of my last commit based on the guidelines

test: adding common.mustCall() to callback

bab6ccc

Adding the common.mustCall() to the callback as the test documentation explains it.

orafaelfragoso force-pushed the issue-12376 branch from 627fd01 to bab6ccc Compare April 15, 2017 20:04

Trott mentioned this pull request Apr 16, 2017

Remove common.PORT usage from parallel tests #12376

Closed

64 tasks

zivkaziv mentioned this pull request Apr 19, 2017

test: fix global.PORT in dgram-cluster-test-1 #12491

Closed

4 tasks

cjihrig approved these changes Apr 21, 2017

View reviewed changes

lpinca closed this May 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: use dynamic port in test-cluster-dgram-reuse #12426

test: use dynamic port in test-cluster-dgram-reuse #12426

orafaelfragoso commented Apr 15, 2017

benjamingr commented Apr 15, 2017

orafaelfragoso commented Apr 15, 2017

addaleax commented Apr 15, 2017 •

edited

Loading

benjamingr left a comment

orafaelfragoso commented Apr 15, 2017

benjamingr commented Apr 15, 2017 •

edited

Loading

orafaelfragoso commented Apr 15, 2017

benjamingr commented Apr 15, 2017

orafaelfragoso commented Apr 15, 2017 •

edited

Loading

benjamingr commented Apr 15, 2017

jkrems commented Apr 15, 2017 •

edited

Loading

cjihrig Apr 15, 2017

orafaelfragoso Apr 15, 2017

orafaelfragoso Apr 15, 2017

santigimeno commented Apr 16, 2017 •

edited

Loading

orafaelfragoso commented Apr 21, 2017

orafaelfragoso commented Apr 21, 2017

cjihrig left a comment

lpinca commented May 19, 2017

lpinca commented May 19, 2017

test: use dynamic port in test-cluster-dgram-reuse #12426

test: use dynamic port in test-cluster-dgram-reuse #12426

Conversation

orafaelfragoso commented Apr 15, 2017

Checklist

Affected core subsystem(s)

benjamingr commented Apr 15, 2017

orafaelfragoso commented Apr 15, 2017

addaleax commented Apr 15, 2017 • edited Loading

benjamingr left a comment

Choose a reason for hiding this comment

orafaelfragoso commented Apr 15, 2017

benjamingr commented Apr 15, 2017 • edited Loading

orafaelfragoso commented Apr 15, 2017

benjamingr commented Apr 15, 2017

orafaelfragoso commented Apr 15, 2017 • edited Loading

benjamingr commented Apr 15, 2017

jkrems commented Apr 15, 2017 • edited Loading

cjihrig Apr 15, 2017

Choose a reason for hiding this comment

orafaelfragoso Apr 15, 2017

Choose a reason for hiding this comment

orafaelfragoso Apr 15, 2017

Choose a reason for hiding this comment

santigimeno commented Apr 16, 2017 • edited Loading

orafaelfragoso commented Apr 21, 2017

orafaelfragoso commented Apr 21, 2017

cjihrig left a comment

Choose a reason for hiding this comment

lpinca commented May 19, 2017

lpinca commented May 19, 2017

addaleax commented Apr 15, 2017 •

edited

Loading

benjamingr commented Apr 15, 2017 •

edited

Loading

orafaelfragoso commented Apr 15, 2017 •

edited

Loading

jkrems commented Apr 15, 2017 •

edited

Loading

santigimeno commented Apr 16, 2017 •

edited

Loading