Node: Remove `common.PORT` usage from `parallel` tests

[Trott]

• Version: master
• Platform: all
• Subsystem: test

Tests in parallel that use common.PORT risk getting EADDRINUSE if another test in parallel uses port 0 (to get an open port assigned by the operating system) at the same time the test runs. This appears to have happened recently. (See https://github.com/nodejs/node/pull/12363#issuecomment-293721140.)

IMO, all instances of common.PORT in parallel should either be changed to use port 0 (if possible in the context of the test) or else moved to sequential (if using port 0 is not possible).

Here are the current tests that use common.PORT:

• [x] test/parallel/test-cluster-basic.js (https://github.com/nodejs/node/pull/12377)
• [x] test/parallel/test-cluster-bind-twice.js (https://github.com/nodejs/node/pull/12418)
• [x] test/parallel/test-cluster-dgram-1.js (https://github.com/nodejs/node/pull/12487)
• [x] test/parallel/test-cluster-dgram-2.js (https://github.com/nodejs/node/pull/12487) (https://github.com/nodejs/node/pull/12492)
• [x] test/parallel/test-cluster-dgram-reuse.js (https://github.com/nodejs/node/pull/12901)
• [x] test/parallel/test-cluster-disconnect-leak.js (https://github.com/nodejs/node/pull/12441)
• [x] test/parallel/test-cluster-disconnect-race.js (https://github.com/nodejs/node/pull/12441)
• [x] test/parallel/test-cluster-disconnect.js
• [x] test/parallel/test-cluster-eaddrinuse.js
• [x] test/parallel/test-cluster-inspector-debug-port.js
• [ ] test/parallel/test-cluster-ipc-throw.js
• [x] test/parallel/test-cluster-master-error.js (https://github.com/nodejs/node/pull/12451)
• [x] test/parallel/test-cluster-master-kill.js (https://github.com/nodejs/node/pull/12451)
• [ ] test/parallel/test-cluster-message.js
• [x] test/parallel/test-cluster-net-send.js (https://github.com/nodejs/node/pull/12451)
• [x] test/parallel/test-cluster-process-disconnect.js (https://github.com/nodejs/node/pull/12441)
• [x] test/parallel/test-cluster-rr-domain-listen.js (https://github.com/nodejs/node/pull/12451)
• [x] test/parallel/test-cluster-rr-ref.js (https://github.com/nodejs/node/pull/12451)
• [x] test/parallel/test-cluster-send-deadlock.js (https://github.com/nodejs/node/pull/12472)
• [x] test/parallel/test-cluster-send-handle-twice.js (https://github.com/nodejs/node/pull/12472)
• [ ] test/parallel/test-cluster-server-restart-none.js
• [ ] test/parallel/test-cluster-server-restart-rr.js
• [ ] test/parallel/test-cluster-shared-handle-bind-error.js
• [x] test/parallel/test-cluster-shared-leak.js (https://github.com/nodejs/node/pull/12451)
• [x] test/parallel/test-cluster-worker-disconnect-on-error.js (https://github.com/nodejs/node/pull/12457)
• [x] test/parallel/test-cluster-worker-disconnect.js (#12443)
• [x] test/parallel/test-cluster-worker-exit.js (#12443)
• [x] test/parallel/test-cluster-worker-kill.js (#12443)
• [x] test/parallel/test-cluster-worker-no-exit.js (https://github.com/nodejs/node/pull/12451)
• [x] test/parallel/test-cluster-worker-wait-server-close.js (https://github.com/nodejs/node/pull/12466)
• [x] test/parallel/test-debugger-repeat-last.js (#12470)
• [x] test/parallel/test-dgram-bind-shared-ports.js (#12452)
• [x] test/parallel/test-dgram-close-in-listening.js
• [x] test/parallel/test-dgram-close-is-not-callback.js
• [x] test/parallel/test-dgram-close.js
• [x] test/parallel/test-dgram-exclusive-implicit-bind.js
• [ ] test/parallel/test-dgram-implicit-bind-failure.js
• [x] test/parallel/test-dgram-oob-buffer.js
• [x] test/parallel/test-dgram-send-address-types.js (https://github.com/nodejs/node/pull/13007)
• [x] test/parallel/test-dgram-send-callback-buffer-empty-address.js (https://github.com/nodejs/node/pull/12929)
• [x] test/parallel/test-dgram-send-callback-buffer-length-empty-address.js (https://github.com/nodejs/node/pull/12944)
• [x] test/parallel/test-dgram-send-callback-buffer-length.js (https://github.com/nodejs/node/pull/12943)
• [x] test/parallel/test-dgram-send-callback-buffer.js (https://github.com/nodejs/node/pull/12942)
• [x] test/parallel/test-https-connect-address-family.js (#12915)
• [x] test/parallel/test-net-better-error-messages-port-hostname.js (https://github.com/nodejs/node/pull/12473)
• [x] test/parallel/test-net-better-error-messages-port.js
• [x] test/parallel/test-net-connect-handle-econnrefused.js
• [x] test/parallel/test-net-connect-immediate-destroy.js
• [ ] test/parallel/test-net-connect-immediate-finish.js
• [x] test/parallel/test-net-connect-local-error.js
• [x] test/parallel/test-net-listen-shared-ports.js
• [x] test/parallel/test-net-localerror.js
• [ ] test/parallel/test-net-localport.js
• [x] test/parallel/test-net-options-lookup.js
• [x] test/parallel/test-net-reconnect-error.js
• [x] test/parallel/test-net-server-bind.js (https://github.com/nodejs/node/pull/13273)
• [x] test/parallel/test-net-socket-destroy-twice.js(https://github.com/nodejs/node/pull/12473)
• [x] test/parallel/test-regress-GH-5051.js
• [x] test/parallel/test-regress-GH-5727.js
• [x] test/parallel/test-tls-client-abort.js (being worked on by @ahmed-taj) (https://github.com/nodejs/node/pull/12461)
• [x] test/parallel/test-tls-client-abort2.js (being worked on by @ahmed-taj) (https://github.com/nodejs/node/pull/12461)
• [x] test/parallel/test-tls-client-default-ciphers.js
• [ ] test/parallel/test-tls-connect.js
• [x] test/parallel/test-tls-ticket-cluster.js

[richardlau]

IMO, all instances of common.PORT in parallel should either be changed to use port 0 (if possible in the context of the test) or else moved to sequential (if using port 0 is not possible).

Maybe update the description of common.PORT in test/README.md with this recommendation and/or (if possible) introduce a linting rule after the list is resolved?

[evanlucas]

For a few of these, (test-net-better-error-messages-*) they don't actually have a server, they try to connect to a server that is not running. Are those ones that you think we should still change @Trott?

[orafaelfragoso]

@evanlucas @richardlau I would love to contribute to this one, but I don't feel comfortable doing so. Can I count on you to guide me on my first contributions? Anything I should read on OS port attribution?

[thefourtheye]

So we are slowly removing common.PORT?

[gibfahn]

I would love to contribute to this one, but I don't feel comfortable doing so. Can I count on you to guide me on my first contributions?

@rafaelfragosom go for it! You should have plenty of people willing to help you if you have any issues (count me in).

[orafaelfragoso]

@gibfahn I'm making progress so far. Thank you!

[Trott]

So we are slowly removing common.PORT?

@thefourtheye From tests in parallel only. It's fine in sequential. And if we find tests in parallel that cannot be re-written to avoid common.PORT, then moving those tests to sequential is probably the way we'll go.

Once we get common.PORT out of parallel, we can probably alter common.js and maybe test.py to simply hardcode common.PORT to a single value. (In other words, there's special logic in there to make sure that parallel tests have different common.PORT values. That won't be necessary anymore.)

[orafaelfragoso]

@Trott @gibfahn Is it really necessary to make 1 PR for each altered file on this issue? Seems exhausting.

[gibfahn]

Is it really necessary to make 1 PR for each altered file on this issue?

@rafaelfragosom no not at all. The general rule (see CONTRIBUTING.md) is one commit per logical change. If you're doing basically the same thing to 20 files, that's one commit. You can also have multiple commits per PR. My general rule is one PR per easily reviewed thing.

_EDIT:_ In fact loads of very similar PRs can get pretty exhausting for reviewers as well.

[orafaelfragoso]

@gibfahn Awesome!

I'm trying to wrap my head inside the tests, specially from clusters, and what's bothering me right now is how common.PORT is being used, like the issue says.

I wasn't here when these tests were written so I don't know what I'm saying but, isn't it better to write a getAvailablePortSync() helper function to retrieve an available port? Generators would be awesome for that.

What are your thoughts on this?

I'm currently looking at parallel/test-cluster-disconnect.js and I'm faced with this:

// ...
if (cluster.isWorker) {
  net.createServer((socket) => {
    socket.end('echo');
  }).listen(common.PORT, '127.0.0.1');

  net.createServer((socket) => {
    socket.end('echo');
  }).listen(common.PORT + 1, '127.0.0.1');
// ...

And could be probably written like this:

// ...
if (cluster.isWorker) {
  var port = common.getAvailablePortSync();

  net.createServer((socket) => {
    socket.end('echo');
  }).listen(port, '127.0.0.1');

  net.createServer((socket) => {
    socket.end('echo');
  }).listen(port + 1, '127.0.0.1');
// ...

I'm really excited for this next PR, this is the last test on test-cluster-disconnect-* and I can't see another way around it. I tried saving the ports in an array but since it's Async, it's not available when the master is called.

Thank you.

[thefourtheye]

@thefourtheye From tests in parallel only. It's fine in sequential.

Just playing devil's advocate. There is a remote possibility that the port is already chosen by the OS.

[Trott]

Just playing devil's advocate. There is a remote possibility that the port is already chosen by the OS.

Yes, that's always a possibility. I agree it's best to avoid common.PORT altogether where possible.

[santigimeno]

I imagine it was taken into account but we have to be sure that we're not removing coverage for the case when a method is called with a specific port.

[gibfahn]

@rafaelfragosom I would suggest doing something like https://github.com/nodejs/node/pull/12418 (using process.send to send the ports back to the parent).

The problem with getting a free port through something like common.getAvailablePortSync() is that the port could be used by something else between calling that function and actually starting the server on the port. You also still have the problem that the child processes have to pass their port numbers to the parent process.

cc/ @Trott as there might be a better solution.

[Trott]

I imagine it was taken into account but we have to be sure that we're not removing coverage for the case when a method is called with a specific port.

Yup, absolutely, good to make that explicit for sure.

[fhalde]

Rather than porting tests which cannot be run in parallel to sequential, isn't there a way to get the port number assigned by the OS for you and reuse that port number wherever required ?

[Trott]

For a few of these, (test-net-better-error-messages-*) they don't actually have a server, they try to connect to a server that is not running. Are those ones that you think we should still change @Trott?

@evanlucas IMO, it's probably better if they don't use common.PORT so we can have a lint rule to catch common.PORT in parallel.

If we don't want to use 0 for them, we can hard code 12346 or any other non-privileged port number and put in a comment indicating that it's an unused value because the function call triggers an error and never opens a port.

Or we can just use 0.

[thefourtheye]

@fhalde Using zero as port number means the Operating system is free to choose any available port number at that point in time.

[thelostone-mc]

@Trott Lint Question :
Why do we enforce common module to be imported for all tests?

  ## common module is mandatory in tests
  required-modules: [2, common]

Reason : net-localerror.js
If i remove common.PORT -> common module doesn't need to be imported -> removing it -> lint error -> frustrating day :P

[santigimeno]

@adityaanandmc from https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#lines-1-2

Even if a test uses no functions or other properties exported by common, the test should still include the common module before any other modules. This is because the common module includes code that will cause a test to fail if the test leaks variables into the global space. In situations where a test uses no functions or other properties exported by common, include it without assigning it to an identifier:

require('../common');

[maclover7]

A bunch more of the test files from the big list above have been completed (purposefully not putting # before pull request number to avoid creating tons of notification noise):

test/parallel/test-cluster-disconnect.js, 12545
test/parallel/test-cluster-eaddrinuse.js, 12547
test/parallel/test-cluster-inspector-debug-port.js moved to test/inspector/test-inspector-port-cluster.js
test/parallel/test-cluster-ipc-throw.js, 12571
test-cluster-message.js, 12584
test-cluster-server-restart-none.js, 12584
test-cluster-server-restart-rr.js, 12584
test-cluster-shared-handle-bind-error.js, 12584
test-dgram-close-in-listening.js, 12376
test-dgram-close-is-not-callback.js, 12376
test-dgram-close.js, 12376
test-dgram-exclusive-implicit-bind.js, 12376
test-dgram-oob-buffer.js, 12376
test-net-better-error-messages-port.js, 12473
test-net-connect-handle-econnrefused.js, 12473
test-net-connect-immediate-destroy.js, 12473
test-net-connect-local-error.js, 12473
test-net-listen-shared-ports.js, 12473
test-net-localerror.js, 12473
test-net-options-lookup.js, 12473
test-net-reconnect-error.js, 13033
test/parallel/test-regress-GH-5051.js, 12639
test/parallel/test-regress-GH-5727.js, 12639
test-tls-ticket-cluster.js, 12715

[Trott]

Thanks, @maclover7! I've updated the list.