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 #12376

Closed
55 of 64 tasks
Trott opened this issue Apr 12, 2017 · 22 comments
Closed
55 of 64 tasks

Remove common.PORT usage from parallel tests #12376

Trott opened this issue Apr 12, 2017 · 22 comments
Labels
test Issues and PRs related to the tests.

Comments

@Trott
Copy link
Member

Trott commented Apr 12, 2017

  • 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 #12363 (comment).)

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:

@Trott Trott added the test Issues and PRs related to the tests. label Apr 12, 2017
@richardlau
Copy link
Member

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
Copy link
Contributor

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 evanlucas added the good first issue Issues that are suitable for first-time contributors. label Apr 13, 2017
@orafaelfragoso
Copy link
Contributor

@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
Copy link
Contributor

So we are slowly removing common.PORT?

orafaelfragoso added a commit to orafaelfragoso/node that referenced this issue Apr 15, 2017
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
@gibfahn
Copy link
Member

gibfahn commented Apr 15, 2017

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
Copy link
Contributor

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

@Trott
Copy link
Member Author

Trott commented Apr 16, 2017

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
Copy link
Contributor

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

@gibfahn
Copy link
Member

gibfahn commented Apr 16, 2017

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
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Member Author

Trott commented Apr 16, 2017

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
Copy link
Member

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
Copy link
Member

gibfahn commented Apr 16, 2017

@rafaelfragosom I would suggest doing something like #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
Copy link
Member Author

Trott commented Apr 16, 2017

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.

MylesBorins pushed a commit that referenced this issue Jul 17, 2017
The usage of common.PORT could cause undesired port collisions when run
in parallel. The following test was moved to sequential.
test-net-reconnect-error.js

PR-URL: #13033
Refs: #12376
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit that referenced this issue Jul 17, 2017
Replace common.PORT with available port assigned by the operating
system in test-dgram-send-callback-buffer.

PR-URL: #12942
Refs: #12376
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this issue Jul 17, 2017
Remove common.PORT from test-dgram-send-callback-buffer-length to
eliminate possibility that a dynamic port used in another test will
collide with common.PORT.

PR-URL: #12943
Refs: #12376
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
Remove common.PORT from test-cluster-worker-disconnect,
test-cluster-worker-exit and test-cluster-worker-kill to
eliminate the possibility that a dynamic port used in
another test will collide with common.PORT.

PR-URL: nodejs/node#12443
Ref: nodejs/node#12376
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
PR-URL: nodejs/node#12441
Ref: nodejs/node#12376
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
Remove common.PORT from test-cluster-bind-twice to eliminate possibility
that a dynamic port used in another test will collide with common.PORT.

PR-URL: nodejs/node#12418
Ref: nodejs/node#12376
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
Remove common.PORT from test-cluster-send-deadlock and
test-cluster-send-handle-twice to reduce possibility that
a dynamic port used in another test will collide with common.PORT.

PR-URL: nodejs/node#12472
Ref: nodejs/node#12376
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
Change common.PORT to '0' to avoid the possibility of getting EADDRINUSE error
if another test in 'parallel' uses port '0' at the same time.

PR-URL: nodejs/node#12461
Ref: nodejs/node#12376
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
Remove common.PORT from test-cluster-worker-disconnect-on-error
possibility that a dynamic port used in another test will collide
with common.PORT.

PR-URL: nodejs/node#12457
Ref: nodejs/node#12376
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
Remove common.PORT from test-cluster-dgram-1 and
test-cluster-dgram-2, in order to eliminate the
possibility of port collision.

PR-URL: nodejs/node#12487
Ref: nodejs/node#12376
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
Removed common.PORT from test-cluster-message,
test-cluster-server-restart-none, test-cluster-server-restart-rr
and test-cluster-shared-handle-bind-error to eliminate the
possibility that a dynamic port used in another test will collide
with common.PORT.

PR-URL: nodejs/node#12584
Ref: nodejs/node#12376
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
Removed common.PORT from test-cluster-ipc-throw to eliminate the
possibility that a dynamic port used in another test will collide
with common.PORT.

PR-URL: nodejs/node#12571
Ref: nodejs/node#12376
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
Removed common.PORT from test-cluster-eaddrinuse to eliminate the
possibility that a dynamic port used in another test will collide
with common.PORT.

PR-URL: nodejs/node#12547
Ref: nodejs/node#12376
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
oantoro added a commit to oantoro/node that referenced this issue Aug 8, 2017
Change common.PORT to dynamic port to prevent collision
with other parallel test which use common.PORT

Refs: [nodejs#12376](nodejs#12376)
@maclover7
Copy link
Contributor

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 Trott removed the good first issue Issues that are suitable for first-time contributors. label Aug 18, 2017
@Trott
Copy link
Member Author

Trott commented Aug 18, 2017

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

maclover7 added a commit to maclover7/node that referenced this issue Aug 24, 2017
jasnell pushed a commit that referenced this issue Aug 24, 2017
PR-URL: #14928
Ref: #12376
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
addaleax pushed a commit to addaleax/ayo that referenced this issue Aug 25, 2017
PR-URL: nodejs/node#14928
Ref: nodejs/node#12376
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this issue Aug 28, 2017
PR-URL: nodejs/node#14928
Ref: nodejs/node#12376
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this issue Sep 10, 2017
PR-URL: #14928
Ref: #12376
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this issue Sep 12, 2017
PR-URL: #14928
Ref: #12376
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this issue Sep 20, 2017
PR-URL: #14928
Ref: #12376
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@Trott Trott closed this as completed Jan 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

No branches or pull requests

10 participants