test: Avoid usage of mixed IPV6 addresses by gireeshpunathil · Pull Request #7702 · nodejs/node · GitHub

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test, cluster

Description of change

Please see #7563 for full details.

The test case test/parallel/test-cluster-disconnect-handles.js
fails in AIX due to the mixed-use of unspecified
and loopback addresses. This is not a problem in most platforms
but fails in AIX. (In Windows too, but does not manifest as the
test is omitted in Windows for a different reason).

There exists no documented evidence which supports the mixed use
of unspecified and loopback addresses.

While AIX strictly follows the IPV6 specification with respect to
unspecified address ('::') and loopback address ('::1'), the test
case latches on to the behavior exhibited by other platforms,
and hence it fails in AIX.

The proposed fix is to make it work in all platforms including
AIX by using the loopback address for the client to connect,
as that is the address at which the server listens.

@gireeshpunathil Do you want to make a similar change to gc/test-net-timeout.js to fix #7291? It seems to be the same problem, so I guess it would make sense to do it in the same PR.

@gibfahn - sure, will check.

@gireeshpunathil I've checked @quaidn 's suggested fix at #7291 (comment) and it seems to work.

diff --git a/test/gc/test-net-timeout.js b/test/gc/test-net-timeout.js
index ff1d565..0e68f78 100644
--- a/test/gc/test-net-timeout.js
+++ b/test/gc/test-net-timeout.js
@@ -36,7 +36,11 @@ function getall() {
     return;

   (function() {
-    var req = net.connect(server.address().port, server.address().address);
+       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');
+       }
     req.resume();
     req.setTimeout(10, function() {
       //console.log('timeout (expected)')

The problem is with the lack of cohesion between what is documented in the server.listen documentation and what is requested by the client in the proposed fix. For example, the API spec says 'If the hostname is omitted, the server will accept connections on any IPv6 address (::) when IPv6 is available'. When connecting to a server socket thus created, it becomes problematic if we use '::1' which is basically the loopback address, not 'any address'. While everything works in Linux magically, I see an issue in Mac.

For better traceability, I suggest we separate this (AIX) from #7291 (windows). I can work with you on that further.

@gireeshpunathil Okay, if they're different fixes I agree it makes sense to do different PRs.

@gibfahn - FYI, I have opened #7728 to cover the inconsistency in general, which includes the windows issue too.

Seeing that this test is not intended to test the edge case it is failing on I'm good with the fix along with the plan that we'd add a test case back in for the edge case under #7728.

LGTM

@Trott FYI

Might it be more elegant to move the const address = common.hasIPv6 ? '[::1]' : common.localhostIPv4; on line 28 up a few lines so it is assigned whether isMaster is true or not, and then get rid of the checking hasIPv6 around this change, just always running server.listen(0, address, cb)?

Oh, wait never mind my previous comment. That sets an array, not a single address value. This change LGTM.

/cc @bnoordhuis

s/Avoid/avoid/ in the commit status line, otherwise LGTM.

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

The test case fails in AIX due to the mixed-use of unspecified and loopback addresses. This is not a problem in most platforms but fails in AIX. (In Windows too, but does not manifest as the test is omitted in Windows for a different reason). There exists no documented evidence which supports the mixed use of unspecified and loopback addresses. While AIX strictly follows the IPV6 specification with respect to unspecified address ('::') and loopback address ('::1'), the test case latches on to the behavior exhibited by other platforms, and hence it fails in AIX. The proposed fix is to make it work in all platforms including AIX by using the loopback address for the client to connect, as that is the address at which the server listens.

Made the changes to commit message (s/Avoid/avoid/) as advised by @bnoordhuis

CI looks good landing

landed as 17883df

The test case fails in AIX due to the mixed-use of unspecified and loopback addresses. This is not a problem in most platforms but fails in AIX. (In Windows too, but does not manifest as the test is omitted in Windows for a different reason). There exists no documented evidence which supports the mixed use of unspecified and loopback addresses. While AIX strictly follows the IPV6 specification with respect to unspecified address ('::') and loopback address ('::1'), the test case latches on to the behavior exhibited by other platforms, and hence it fails in AIX. The proposed fix is to make it work in all platforms including AIX by using the loopback address for the client to connect, as that is the address at which the server listens. Fixes: #7563 PR-URL: #7702 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 13, 2016

mscdex added the cluster Issues and PRs related to the cluster subsystem. label Jul 13, 2016

mhdawson self-assigned this Jul 14, 2016

mhdawson closed this Jul 18, 2016

evanlucas mentioned this pull request Jul 19, 2016

v6.3.1 Release Proposal #7782

Merged

MylesBorins added the land-on-v4.x label Sep 30, 2016

MylesBorins mentioned this pull request Oct 26, 2016

V4.6.2 proposal #9298

Closed

gireeshpunathil mentioned this pull request Jan 4, 2017

Inconsistencies with IPv6 endpoint specification across platforms #7728

Closed

gibfahn mentioned this pull request Aug 17, 2017

net: node on Linux allows opening a connection to 0.0.0.0/:: while on Windows it doesn't #14900

Closed

test: Avoid usage of mixed IPV6 addresses by gireeshpunathil · Pull Request #7702 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: Avoid usage of mixed IPV6 addresses - #7702

test: Avoid usage of mixed IPV6 addresses#7702gireeshpunathil wants to merge 1 commit intonodejs:masternodejs/node:masterfrom gireeshpunathil:unicastipv6Copy head branch name to clipboard

Conversation

gireeshpunathil commented Jul 13, 2016 • edited by thefourtheye Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

gibfahn commented Jul 13, 2016

Uh oh!

gireeshpunathil commented Jul 13, 2016

Uh oh!

gibfahn commented Jul 13, 2016 • edited by Fishrock123 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gireeshpunathil commented Jul 13, 2016

Uh oh!

gibfahn commented Jul 13, 2016

Uh oh!

gireeshpunathil commented Jul 14, 2016

Uh oh!

mhdawson commented Jul 14, 2016

Uh oh!

Trott commented Jul 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Jul 15, 2016

Uh oh!

Trott commented Jul 15, 2016

Uh oh!

bnoordhuis commented Jul 16, 2016

Uh oh!

cjihrig commented Jul 16, 2016

Uh oh!

gireeshpunathil commented Jul 18, 2016

Uh oh!

mhdawson commented Jul 18, 2016

Uh oh!

mhdawson commented Jul 18, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

test: Avoid usage of mixed IPV6 addresses#7702
gireeshpunathil wants to merge 1 commit into
nodejs:masterfrom
gireeshpunathil:unicastipv6

gireeshpunathil commented Jul 13, 2016 •
edited by thefourtheye
Loading

gibfahn commented Jul 13, 2016 •
edited by Fishrock123
Loading

Trott commented Jul 15, 2016 •
edited
Loading