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.

Sign up for GitHub

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

Jump to bottom

test: improve cluster-disconnect-handles test #4084

Closed
wants to merge 1 commit into from
Closed

test: improve cluster-disconnect-handles test #4084

wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Dec 1, 2015

This commit fixes two issues in test-cluster-disconnect-handles:

  1. If the master's TCP connection to the worker fails, the worker process stays alive and causes many other tests that use the same common port number to also fail (with EADDRINUSE).
  2. One particular problem that can cause the master's TCP connection to fail is attempting an IPv6 connection to the worker when no IPv6 network interfaces are available (this was the case on my local Linux machine).

@mscdex mscdex added cluster Issues and PRs related to the cluster subsystem. test Issues and PRs related to the tests. labels Dec 1, 2015
@mscdex
Copy link
Contributor Author

mscdex commented Dec 1, 2015

CI is all green except unrelated errors on Windows.

cjihrig
cjihrig reviewed Dec 1, 2015
View reviewed changes
test/parallel/test-cluster-disconnect-handles.js
process.send(['listening', server.address()]);
debugger;
});
};
if (common.hasIPv6)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... is this the only test that suffers from this problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, but this was the only one I personally saw that was causing this problem.

@cjihrig
Copy link
Contributor

cjihrig commented Dec 1, 2015

LGTM with a question and comment.

@mscdex
test: improve cluster-disconnect-handles test
e22c756 
This commit fixes two issues in test-cluster-disconnect-handles:

1. If the master's TCP connection to the worker fails, the worker
process stays alive and causes many other tests that use the same
common port number to also fail (with EADDRINUSE).

2. One particular problem that can cause the master's TCP connection
to fail is attempting an IPv6 connection to the worker when no IPv6
network interfaces are available.
@mscdex mscdex force-pushed the fix-test-cluster-disconnect-handles branch from 73d2dd4 to e22c756 Compare December 1, 2015 18:34
@cjihrig
Copy link
Contributor

cjihrig commented Dec 1, 2015

Does this close #3907? There is also #4009 for that test.

@mscdex
Copy link
Contributor Author

mscdex commented Dec 1, 2015

@cjihrig I don't know, that might be a separate issue.

This is what I get when I don't have the IPv6 check in place:

Debugger listening on port 12946
Debugger listening on port 12346
/home/mscdex/git/node/test/parallel/test-cluster-disconnect-handles.js:72
    throw ex;
    ^

Error: connect EADDRNOTAVAIL :::12346 - Local (:::0)
    at Object.exports._errnoException (util.js:873:11)
    at exports._exceptionWithHostPort (util.js:896:20)
    at connect (net.js:844:14)
    at net.js:942:9
    at doNTCallback0 (node.js:430:9)
    at process._tickDomainCallback (node.js:400:13)

@cjihrig
Copy link
Contributor

cjihrig commented Dec 1, 2015

Does listening on 'localhost' work for you?

@mscdex
Copy link
Contributor Author

mscdex commented Dec 1, 2015

It would probably work for me because I only have localhost resolving to 127.0.0.1, but by default most modern Linux distros have localhost resolve to both IPv4 and IPv6 addresses (even if there are no IPv6 interfaces).

@cjihrig
Copy link
Contributor

cjihrig commented Dec 1, 2015

OK, I don't want to hold up this PR, I was just looking for a cleaner alternative.

mscdex added a commit that referenced this pull request Dec 2, 2015
@mscdex
test: improve cluster-disconnect-handles test
49b5445 
This commit fixes two issues in test-cluster-disconnect-handles:

1. If the master's TCP connection to the worker fails, the worker
process stays alive and causes many other tests that use the same
common port number to also fail (with EADDRINUSE).

2. One particular problem that can cause the master's TCP connection
to fail is attempting an IPv6 connection to the worker when no IPv6
network interfaces are available.

PR-URL: #4084
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@mscdex
Copy link
Contributor Author

mscdex commented Dec 2, 2015

Landed in 49b5445.

@mscdex mscdex closed this Dec 2, 2015
@imran-iq imran-iq mentioned this pull request Dec 4, 2015
Closed
mscdex added a commit that referenced this pull request Dec 5, 2015
@mscdex @rvagg
test: improve cluster-disconnect-handles test
cb55c67 
This commit fixes two issues in test-cluster-disconnect-handles:

1. If the master's TCP connection to the worker fails, the worker
process stays alive and causes many other tests that use the same
common port number to also fail (with EADDRINUSE).

2. One particular problem that can cause the master's TCP connection
to fail is attempting an IPv6 connection to the worker when no IPv6
network interfaces are available.

PR-URL: #4084
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This was referenced Dec 8, 2015
Closed
Merged
@rvagg rvagg mentioned this pull request Dec 17, 2015
Closed
@mscdex mscdex deleted the fix-test-cluster-disconnect-handles branch December 17, 2015 18:01
mscdex added a commit that referenced this pull request Dec 29, 2015
@mscdex
test: improve cluster-disconnect-handles test
ebfbf55 
This commit fixes two issues in test-cluster-disconnect-handles:

1. If the master's TCP connection to the worker fails, the worker
process stays alive and causes many other tests that use the same
common port number to also fail (with EADDRINUSE).

2. One particular problem that can cause the master's TCP connection
to fail is attempting an IPv6 connection to the worker when no IPv6
network interfaces are available.

PR-URL: #4084
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
@mscdex
test: improve cluster-disconnect-handles test
ac939d5 
This commit fixes two issues in test-cluster-disconnect-handles:

1. If the master's TCP connection to the worker fails, the worker
process stays alive and causes many other tests that use the same
common port number to also fail (with EADDRINUSE).

2. One particular problem that can cause the master's TCP connection
to fail is attempting an IPv6 connection to the worker when no IPv6
network interfaces are available.

PR-URL: #4084
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
Merged
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@mscdex
test: improve cluster-disconnect-handles test
cf5a3a6 
This commit fixes two issues in test-cluster-disconnect-handles:

1. If the master's TCP connection to the worker fails, the worker
process stays alive and causes many other tests that use the same
common port number to also fail (with EADDRINUSE).

2. One particular problem that can cause the master's TCP connection
to fail is attempting an IPv6 connection to the worker when no IPv6
network interfaces are available.

PR-URL: nodejs#4084
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cluster Issues and PRs related to the cluster subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mscdex @cjihrig @jasnell @MylesBorins