Remove common.PORT usage from parallel tests

[thelostone-mc]

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]

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

Thanks for pushing me to get into this!
@thefourtheye

[vsemozhetbyt]

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

[thefourtheye]

Good job @adityaanandmc 👏

[thefourtheye]

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

[thelostone-mc]

@thefourtheye Do the changes look alright ?

[thelostone-mc]

@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]

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]

@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]

@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]

@adityaanandmc what about doing something like this:

... constserver=net.createServer();server.listen(0);constport=server.address().port;constc=net.createConnection(port);server.close(); ...

[thelostone-mc]

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

[santigimeno]

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

[jasnell]

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

[thelostone-mc]

@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]

@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]

@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]

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]

@adityaanandmc, what @addaleax said.

BTW, could you address this nit? Thanks!

[thelostone-mc]

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

[thelostone-mc]

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

[gibfahn]

CI: https://ci.nodejs.org/job/node-test-commit/9717/

[santigimeno]

@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]

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

Updated:

• test-net-better-error-messages-port-hostname
• test-net-options-lookup.js

Thanks @santigimeno@thefourtheye@Trott@cjihrig !!

[santigimeno]

Added comments ! Do they look alright ?

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

[thelostone-mc]

@santigimeno Done!

[thefourtheye]

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

[santigimeno]

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 ?

[thefourtheye]

@santigimenoclose() 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...

[santigimeno]

@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.

[thelostone-mc]

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

[thefourtheye]

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

[thelostone-mc]

@thefourtheye Done. CI Build trigger necessary ?

[thefourtheye]

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

[jasnell]

Landed in 94eed0f