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

Remove common.PORT usage from `parallel` tests #12473

Closed
wants to merge 1 commit into from

Conversation

@thelostone-mc
Copy link
Contributor

commented Apr 17, 2017

Refs: #12376

Tests Updated:

  • test/parallel/test-net-better-error-messages-port-hostname.js
  • test/parallel/test-net-socket-destroy-twice.js
  • test/parallel/test-net-options-lookup.js
  • test/parallel/test-net-connect-immediate-destroy.js
  • test/parallel/test-net-localerror.js
  • test/parallel/test-net-connect-handle-econnrefused.js

Tests ported from test/parallel to test/sequential:

  • test/sequential/test-net-listen-shared-ports.js
  • test/sequential/test-net-better-error-messages-port.js

Analyzed and continuing to use common.PORT

  • test/parallel/test-net-connect-immediate-finish.js
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

@thelostone-mc

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2017

First-time contributor. (Noob level)
Anything I'm missing out on?

Thanks for pushing me to get into this!
@thefourtheye

@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2017

@mscdex mscdex added the net label Apr 17, 2017

@thefourtheye

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2017

Good job @adityaanandmc 👏

@Trott Trott referenced this pull request Apr 18, 2017
55 of 64 tasks complete
@thefourtheye

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2017

@adityaanandmc Changes are all good. If you could make sure if the commits follow these guidelines it would be great?

@thelostone-mc thelostone-mc force-pushed the thelostone-mc:master branch Apr 18, 2017

@thelostone-mc thelostone-mc changed the title Removing common.PORT usage from `parallel` tests Remove common.PORT usage from `parallel` tests Apr 18, 2017

@thelostone-mc thelostone-mc force-pushed the thelostone-mc:master branch Apr 18, 2017

@thelostone-mc

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2017

@thefourtheye Do the changes look alright ?

@thelostone-mc

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2017

@thefourtheye @cjihrig
Are we allowed to do this? Create a server and kill it simply to get a free port.
Is there a way to just get an unused port ?

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2017

Are we allowed to do this? Create a server and kill it simply to get a free port.

That would probably introduce a race condition. Once you close the server to release the port, something else can claim that port.

@thelostone-mc

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2017

@cjihrig What would be the ideal way to get this done? I was aware of the race condition and hence a little stuck. My commit ( code below ) will fail if the race condition is met.
How do I secure a free port and listen to it ensuring that a server hasn't been created in the gap?

const net = require('net');
const assert = require('assert');

//const c = net.createConnection(common.PORT);
const server = net.createServer();
server.listen(0);
const port = server.address().port;
server.close();

const c = net.createConnection(port);

c.on('connect', common.mustNotCall());

c.on('error', common.mustCall(function(e) {
    assert.strictEqual(e.code, 'ECONNREFUSED');
    //assert.strictEqual(e.port, common.PORT);
    assert.strictEqual(e.port, port);
    assert.strictEqual(e.address, '127.0.0.1');
}));
@thefourtheye

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2017

@adityaanandmc wrote

How do I secure a free port and listen to it ensuring that a server hasn't been created in the gap?

Although the possibility of this happening is less, it is still possible and we cannot avoid it.

@santigimeno

This comment has been minimized.

Copy link
Member

commented Apr 19, 2017

@adityaanandmc what about doing something like this:

...
const server = net.createServer();
server.listen(0);
const port = server.address().port;
const c = net.createConnection(port);
server.close();
...

@thelostone-mc thelostone-mc force-pushed the thelostone-mc:master branch Apr 19, 2017

@thelostone-mc

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2017

@santigimeno Now i feel like a idiot :P Thanks for that!! :D

@santigimeno

This comment has been minimized.

Copy link
Member

commented Apr 19, 2017

@adityaanandmc could you squash the commits into one? Thanks!

@jasnell

This comment has been minimized.

@thelostone-mc

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2017

@santigimeno will squash them!!
Looks like there are a few failures test-net-connect-immediate-finish

I can't seem to understand the failures. I thought about doing this, but it didn't seem like the right thing to do.

const server = net.createServer();
server.listen(0);
const port = server.address().port;
server.close();

const client = net.connect({host: '***', port: port});
@santigimeno

This comment has been minimized.

Copy link
Member

commented Apr 19, 2017

@adityaanandmc looking at the changes again it seems that we're trying to connect() to the 0 port. I don't think that makes sense: the port 0 makes sense when calling bind() or listen() not connect(). What I think it could be done is create a helper function in common.js (connectToRandomPort for example) to reserve a port and connect, similar to the code you were proposing.

@thelostone-mc thelostone-mc force-pushed the thelostone-mc:master branch 2 times, most recently Apr 20, 2017

@thelostone-mc

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2017

@santigimeno @thefourtheye I did that and it passed locally but at times it fails with the following error!
It started only after i had touched this file test-net-connect-immediate-finish.
Not able to figure out why it's happening.

Path: parallel/test-domain-no-error-handler-abort-on-uncaught-0
Command: out/Release/node /Users/adianand/Labs/node/test/parallel/test-domain-no-error-handler-abort-on-uncaught-0.js
--- TIMEOUT ---
=== release test-domain-no-error-handler-abort-on-uncaught-1 ===
Path: parallel/test-domain-no-error-handler-abort-on-uncaught-1
Command: out/Release/node /Users/adianand/Labs/node/test/parallel/test-domain-no-error-handler-abort-on-uncaught-1.js
--- TIMEOUT ---
=== release test-domain-no-error-handler-abort-on-uncaught-2 ===
Path: parallel/test-domain-no-error-handler-abort-on-uncaught-2
Command: out/Release/node /Users/adianand/Labs/node/test/parallel/test-domain-no-error-handler-abort-on-uncaught-2.js
--- TIMEOUT ---
=== release test-domain-no-error-handler-abort-on-uncaught-3 ===
Path: parallel/test-domain-no-error-handler-abort-on-uncaught-3
Command: out/Release/node /Users/adianand/Labs/node/test/parallel/test-domain-no-error-handler-abort-on-uncaught-3.js
--- TIMEOUT ---
=== release test-domain-no-error-handler-abort-on-uncaught-4 ===
Path: parallel/test-domain-no-error-handler-abort-on-uncaught-4
Command: out/Release/node /Users/adianand/Labs/node/test/parallel/test-domain-no-error-handler-abort-on-uncaught-4.js
--- TIMEOUT ---

Sorry for flooding you with doubts. Just a lil confused for a first-time contributor on a project of this scale.

@addaleax

This comment has been minimized.

Copy link
Member

commented May 6, 2017

I think the actual error in the output you linked to is this:

not ok 357 sequential/test-benchmark-child-process
  ---
  duration_ms: 60.190
  severity: fail
  stack: |-
    timeout

That is unrelated, and was tracked at #12817 (but should be fixed now).

Either way, it looks like this PR has a merge conflict now so it might need to rebase – is that something you can do?

@santigimeno

This comment has been minimized.

Copy link
Member

commented May 6, 2017

@adityaanandmc, what @addaleax said.

BTW, could you address this nit? Thanks!

@thelostone-mc

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2017

Sure thing !! Will do that in a bit though. ( heading out to watch Guardians of the Galaxy ) :D

@thelostone-mc thelostone-mc force-pushed the thelostone-mc:master branch May 7, 2017

@thelostone-mc

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2017

@addaleax @santigimeno Rebased!! Thanks for that. Can a CI job be triggered?

@gibfahn

This comment has been minimized.

Copy link
Member

commented May 7, 2017

@santigimeno

This comment has been minimized.

Copy link
Member

commented May 7, 2017

@adityaanandmc sorry to bring this up again, but it would be great if you could address this. Anyway, the PR as it is LGTM. Thanks!

@thelostone-mc thelostone-mc force-pushed the thelostone-mc:master branch May 7, 2017

@thelostone-mc

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2017

@santigimeno That's alright. I did not see that comment until now. Sorry about that!
Added comments ! Do they look alright ?

Updated:

Thanks @santigimeno @thefourtheye @Trott @cjihrig !!

@santigimeno

This comment has been minimized.

Copy link
Member

commented May 7, 2017

Added comments ! Do they look alright ?

Yes, LGTM. I think another in test-net-localerror.js could be useful. Thanks!

@thelostone-mc thelostone-mc force-pushed the thelostone-mc:master branch May 8, 2017

@thelostone-mc

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2017

@santigimeno Done!

test/parallel/test-net-better-error-messages-port.js Outdated
assert.strictEqual(e.address, '127.0.0.1');
}));
server.close();

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye May 8, 2017

Contributor

Okay, server.close is actually asynchronous. Perhaps we should run this test in the close event of server.

This comment has been minimized.

Copy link
@santigimeno

santigimeno May 8, 2017

Member

Okay, server.close is actually asynchronous

You're right, though the close() is executed syncronously at least on Unixes, but that's an implementation detail.

Perhaps we should run this test in the close event of server.

I think that's not going to work either, because there's going to be a window when the port is not assigned and could be grabbed by some other process/test.

So probably the only way to make this work without race conditions is moving it to sequential. What do you think @thefourtheye ?

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye May 8, 2017

Contributor

@santigimeno close() is actually executed synchronously, but by the time the connection attempt is made (which happens asynchronously), the port is still up for grab, right? Perhaps I am overthinking 🤔

I am afraid sequential also will not solve this. common.PORT could still have been assigned to other processes.

Perhaps we can live with the current change...

This comment has been minimized.

Copy link
@santigimeno

santigimeno May 8, 2017

Member

@santigimeno close() is actually executed synchronously, but by the time the connection attempt is made (which happens asynchronously), the port is still up for grab, right? Perhaps I am overthinking 🤔

Yes, I thought of that too. And I think I'm also overthinking 😆 .

I am afraid sequential also will not solve this. common.PORT could still have been assigned to other processes.

Right, but not to other tests in the same run though.

Perhaps we can live with the current change...

Yeah, probably they'll never fail.

This comment has been minimized.

Copy link
@thelostone-mc

thelostone-mc May 8, 2017

Author Contributor

Since we've reached a moo point. Would it be alright if I followed @thefourtheye approach and change the code accordingly ?

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye May 8, 2017

Contributor

@adityaanandmc No, move this to sequential, without the changes.

@thelostone-mc thelostone-mc force-pushed the thelostone-mc:master branch May 8, 2017

test: use dynamic port instead of common.PORT
Remove common.PORT from, test-net-connect-immediate-destroy,
test-net-options-lookup, test-net-connect-local-error,
test-net-connect-handle-econnrefused, test-net-socket-destroy-twice,
test-net-better-error-messages-port-hostname, test-net-localerror,
to reduce possibility that a dynamic port used in another test will
collide with common.PORT.

Moved test-net-listen-shared-ports, test-net-better-error-messages-port
from tests/parallel to test/sequential

Refs: #12376

@thelostone-mc thelostone-mc force-pushed the thelostone-mc:master branch to f5aead0 May 8, 2017

@thelostone-mc

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2017

@thefourtheye Done. CI Build trigger necessary ?

@thefourtheye

This comment has been minimized.

Copy link
Contributor

commented May 9, 2017

jasnell added a commit that referenced this pull request May 9, 2017
test: use dynamic port instead of common.PORT
Remove common.PORT from, test-net-connect-immediate-destroy,
test-net-options-lookup, test-net-connect-local-error,
test-net-connect-handle-econnrefused, test-net-socket-destroy-twice,
test-net-better-error-messages-port-hostname, test-net-localerror,
to reduce possibility that a dynamic port used in another test will
collide with common.PORT.

Moved test-net-listen-shared-ports, test-net-better-error-messages-port
from tests/parallel to test/sequential

Refs: #12376
PR-URL: #12473
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jasnell

This comment has been minimized.

Copy link
Member

commented May 9, 2017

Landed in 94eed0f

@jasnell jasnell closed this May 9, 2017

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
test: use dynamic port instead of common.PORT
Remove common.PORT from, test-net-connect-immediate-destroy,
test-net-options-lookup, test-net-connect-local-error,
test-net-connect-handle-econnrefused, test-net-socket-destroy-twice,
test-net-better-error-messages-port-hostname, test-net-localerror,
to reduce possibility that a dynamic port used in another test will
collide with common.PORT.

Moved test-net-listen-shared-ports, test-net-better-error-messages-port
from tests/parallel to test/sequential

Refs: nodejs#12376
PR-URL: nodejs#12473
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jasnell jasnell referenced this pull request May 11, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
gibfahn added a commit that referenced this pull request Jun 20, 2017
test: use dynamic port instead of common.PORT
Remove common.PORT from, test-net-connect-immediate-destroy,
test-net-options-lookup, test-net-connect-local-error,
test-net-connect-handle-econnrefused, test-net-socket-destroy-twice,
test-net-better-error-messages-port-hostname, test-net-localerror,
to reduce possibility that a dynamic port used in another test will
collide with common.PORT.

Moved test-net-listen-shared-ports, test-net-better-error-messages-port
from tests/parallel to test/sequential

Refs: #12376
PR-URL: #12473
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins added a commit that referenced this pull request Jul 11, 2017
test: use dynamic port instead of common.PORT
Remove common.PORT from, test-net-connect-immediate-destroy,
test-net-options-lookup, test-net-connect-local-error,
test-net-connect-handle-econnrefused, test-net-socket-destroy-twice,
test-net-better-error-messages-port-hostname, test-net-localerror,
to reduce possibility that a dynamic port used in another test will
collide with common.PORT.

Moved test-net-listen-shared-ports, test-net-better-error-messages-port
from tests/parallel to test/sequential

Refs: #12376
PR-URL: #12473
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@MylesBorins MylesBorins referenced this pull request Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.