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

win: gc/test-net-timeout.js failure in v6.2.1 #7291

Closed
gibfahn opened this issue Jun 13, 2016 · 10 comments
Closed

win: gc/test-net-timeout.js failure in v6.2.1 #7291

gibfahn opened this issue Jun 13, 2016 · 10 comments
Labels
net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.

Comments

@gibfahn
Copy link
Member

gibfahn commented Jun 13, 2016

  • Version:v6.2.1
  • Platform:win 7 (x86 & x64)
  • Subsystem:

The problem seems to be due to this change, specifically the use of server.address().address. Changing the address back to '127.0.0.1' makes the test pass.

From what I can work out, the address defaults to :: (the equivalent of 0.0.0.0) for IPv6 enabled machines. It seems that it never gets resolved to ::1 (localhost) on windows. The same test passes on Linux.

5 - gc/test-net-timeout

not ok 5 gc/test-net-timeout
# events.js:160
# throw er; // Unhandled 'error' event
# ˆ
#
# Error: connect EADDRNOTAVAIL :::61066
# at Object.exports._errnoException (util.js:1007:11)
# at exports._exceptionWithHostPort (util.js:1030:20)
# at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1080:14)
# We should do 500 requests
@mscdex mscdex added net Issues and PRs related to the net subsystem. windows Issues and PRs related to the Windows platform. test Issues and PRs related to the tests. labels Jun 13, 2016
@mscdex
Copy link
Contributor

mscdex commented Jun 13, 2016

/cc @Trott

@mhdawson
Copy link
Member

mhdawson commented Jun 13, 2016

Also seems to fail on one of our linux machines:

not ok 5 gc/test-net-timeout
# events.js:160
#       throw er; // Unhandled 'error' event
#       ^
# 
# Error: connect ENETUNREACH :::50340 - Local (:::0)
#     at Object.exports._errnoException (util.js:1007:11)
#     at exports._exceptionWithHostPort (util.js:1030:20)
#     at connect (net.js:874:16)
#     at net.js:964:9
#     at _combinedTickCallback (internal/process/next_tick.js:67:7)
#     at process._tickCallback (internal/process/next_tick.js:98:9)
#     at Module.runMain (module.js:577:11)
#     at run (node.js:340:7)
#     at startup (node.js:132:9)
#     at node.js:455:3
# We should do 500 requests
  ---
  duration: 0.213s

but that could be a different configuration issue although I think it started around the same timeframe

@Trott
Copy link
Member

Trott commented Jun 13, 2016

Not sure if this is a bug in Node.js or if it is a host configuration issue.

For what it's worth, I think it's been suggested that we just get rid of the gc tests, IIRC because they are low-to-zero value and sometimes problematic. Wish I could remember who proposed it so i could @-mention them to make sure I got that right. Maybe it was @bnoordhuis?

@Trott
Copy link
Member

Trott commented Jun 13, 2016

/cc @nodejs/build for comments on possibility of a configuration issue vs. bug in Node.js.

@gibfahn
Copy link
Member Author

gibfahn commented Jun 14, 2016

Passing 'localhost' to test/gc/test-net-timeout.js#L39 works everywhere, but passing '::' only works on Linux (not on the three windows 7 boxes I tested).

@Trott
Copy link
Member

Trott commented Jun 14, 2016

Feel free to submit a PR to change it to 'localhost' if you think that's a reasonable workaround.

I'm still not clear if this is a bug in Windows or a bug in Node.js. server.address().address should certainly return a usable value, so the test itself should be fine as is.

@gibfahn
Copy link
Member Author

gibfahn commented Jun 14, 2016

@Trott Exactly, I am happy to submit a change to 'localhost' if that makes sense, but I'd like someone who knows more about this to confirm that this isn't a bug in Windows, it certainly seems problematic.

@Trott
Copy link
Member

Trott commented Jun 14, 2016

@nodejs/platform-windows

@reshnm
Copy link
Contributor

reshnm commented Jun 23, 2016

@gibm Maybe do not use localhost, but instead use the corresponding IPv4/IPv6 address.
This may help in rare cases where localhost cannot be resolved.
I also don't think this is a bug in Windows. From a network perspective it doesn't make sense to connect to "all" interfaces. What if the specified port is available on multiple interfaces? Connect to any interface?

if (server.address().family === 'IPv4') {
  var req = net.connect(server.address().port, '127.0.0.1');
} else {
   var req = net.connect(server.address().port, '::1');
}

@gibfahn
Copy link
Member Author

gibfahn commented Jun 24, 2016

@quaidn that does make sense, and I'll try it to make sure that works, but I wonder whether this code should be (or already is) somewhere else in node (for example in common.js).

gibfahn added a commit to gibfahn/node that referenced this issue Jan 17, 2017
If a test does http.listen(0) or net.listen(0),
http.listen(0).address().address returns '::'. Some machines will
resolve this to localhost, but not all. Every machine should have
localhost defined in /etc/hosts (or equivalent), so it should always
resolve.

Fixes: nodejs#7291
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 25, 2017
If a test does http.listen(0) or net.listen(0),
http.listen(0).address().address returns '::'. Some machines will
resolve this to localhost, but not all. Every machine should have
localhost defined in /etc/hosts (or equivalent), so it should always
resolve.

Fixes: nodejs#7291
PR-URL: nodejs#10854
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michal Zasso <targos@protonmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 27, 2017
If a test does http.listen(0) or net.listen(0),
http.listen(0).address().address returns '::'. Some machines will
resolve this to localhost, but not all. Every machine should have
localhost defined in /etc/hosts (or equivalent), so it should always
resolve.

Fixes: nodejs#7291
PR-URL: nodejs#10854
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michal Zasso <targos@protonmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 30, 2017
If a test does http.listen(0) or net.listen(0),
http.listen(0).address().address returns '::'. Some machines will
resolve this to localhost, but not all. Every machine should have
localhost defined in /etc/hosts (or equivalent), so it should always
resolve.

Fixes: nodejs#7291
PR-URL: nodejs#10854
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michal Zasso <targos@protonmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 30, 2017
If a test does http.listen(0) or net.listen(0),
http.listen(0).address().address returns '::'. Some machines will
resolve this to localhost, but not all. Every machine should have
localhost defined in /etc/hosts (or equivalent), so it should always
resolve.

Fixes: nodejs#7291
PR-URL: nodejs#10854
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michal Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this issue Mar 8, 2017
If a test does http.listen(0) or net.listen(0),
http.listen(0).address().address returns '::'. Some machines will
resolve this to localhost, but not all. Every machine should have
localhost defined in /etc/hosts (or equivalent), so it should always
resolve.

Fixes: #7291
PR-URL: #10854
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michal Zasso <targos@protonmail.com>
gibfahn added a commit to gibfahn/node that referenced this issue Mar 8, 2017
If a test does http.listen(0) or net.listen(0),
http.listen(0).address().address returns '::'. Some machines will
resolve this to localhost, but not all. Every machine should have
localhost defined in /etc/hosts (or equivalent), so it should always
resolve.

Fixes: nodejs#7291
MylesBorins pushed a commit that referenced this issue Mar 8, 2017
If a test does http.listen(0) or net.listen(0),
http.listen(0).address().address returns '::'. Some machines will
resolve this to localhost, but not all. Every machine should have
localhost defined in /etc/hosts (or equivalent), so it should always
resolve.

Fixes: #7291
MylesBorins pushed a commit that referenced this issue Mar 9, 2017
If a test does http.listen(0) or net.listen(0),
http.listen(0).address().address returns '::'. Some machines will
resolve this to localhost, but not all. Every machine should have
localhost defined in /etc/hosts (or equivalent), so it should always
resolve.

Fixes: #7291
PR-URL: #10854
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michal Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this issue Mar 9, 2017
If a test does http.listen(0) or net.listen(0),
http.listen(0).address().address returns '::'. Some machines will
resolve this to localhost, but not all. Every machine should have
localhost defined in /etc/hosts (or equivalent), so it should always
resolve.

Fixes: #7291
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.
Projects
None yet
5 participants