Skip to content

dns: default to verbatim=true in dns.lookup() #37681

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

Closed
treysis wants to merge 27 commits into from
Closed

dns: default to verbatim=true in dns.lookup() #37681

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
5bef0de
dns: default to verbatim=true in dns.lookup()
bnoordhuis Jan 29, 2020
4be87a0
Fixes for IPv6 compatibility; new const 'common.localhostIP'
treysis Mar 10, 2021
43d1492
Update test-net-dns-lookup.js
treysis Mar 13, 2021
beb6420
Update test-net-dns-lookup.js
treysis Mar 13, 2021
604366c
Fix test-net-dns-lookup.js
treysis Mar 13, 2021
1bb0e55
Apply suggestions from code review
treysis Mar 13, 2021
16a1682
IPv6 fixes
treysis Mar 14, 2021
178683c
Update inspector-helper.js
treysis Mar 14, 2021
2b67c17
Revert "Fixes for IPv6 compatibility; new const 'common.localhostIP'"
treysis Mar 16, 2021
4b06383
Update test-net-better-error-messages-port.js
treysis Mar 16, 2021
6340f13
Test suite fixes. Hardcode localhost IPv4.
treysis Mar 16, 2021
2f7edf0
More localhost/127.0.0.1 fixes.
treysis Mar 16, 2021
75bd959
Update test-net-pingpong.js
treysis Mar 16, 2021
a19f8ec
Update test/parallel/test-net-writable.js
treysis Mar 20, 2021
0232a4a
Update test-net-writable.js
treysis Mar 24, 2021
2ef88c7
Update test-net-writable.js
treysis Mar 24, 2021
98b3ffb
Update test-net-writable.js
treysis Mar 24, 2021
7a0e447
Update test-net-writable.js
treysis Mar 24, 2021
5037efc
Update parallel.status
treysis Mar 24, 2021
787dc69
Update test-net-connect-options-port.js
treysis Mar 24, 2021
26f7f45
Update parallel.status
treysis Mar 24, 2021
269eda7
Update test-net-connect-options-port.js
treysis Mar 24, 2021
1928b44
Update parallel.status
treysis Mar 24, 2021
5557b85
Updated from review
treysis Mar 25, 2021
bccf676
Update test-net-remote-address-port.js
treysis Mar 26, 2021
0c8be88
Update test/parallel/test-net-remote-address-port.js
treysis Mar 26, 2021
7cae409
Update test-net-better-error-messages-port.js
treysis Mar 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions doc/api/dns.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,9 @@ section if a custom port is used.
<!-- YAML
added: v0.1.90
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/37681
description: The `verbatim` options defaults to `true` now.
- version: v8.5.0
pr-url: https://github.com/nodejs/node/pull/14731
description: The `verbatim` option is supported now.
Expand All @@ -183,9 +186,7 @@ changes:
* `verbatim` {boolean} When `true`, the callback receives IPv4 and IPv6
addresses in the order the DNS resolver returned them. When `false`,
IPv4 addresses are placed before IPv6 addresses.
**Default:** currently `false` (addresses are reordered) but this is
expected to change in the not too distant future.
New code should use `{ verbatim: true }`.
**Default:** `true`.
* `callback` {Function}
* `err` {Error}
* `address` {string} A string representation of an IPv4 or IPv6 address.
Expand Down
4 changes: 2 additions & 2 deletions lib/dns.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ function lookup(hostname, options, callback) {
let hints = 0;
let family = -1;
let all = false;
let verbatim = false;
let verbatim = true;

// Parse arguments
if (hostname) {
Expand All @@ -113,7 +113,7 @@ function lookup(hostname, options, callback) {
hints = options.hints >>> 0;
family = options.family >>> 0;
all = options.all === true;
verbatim = options.verbatim === true;
verbatim = options.verbatim !== false;

validateHints(hints);
} else {
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/dns/promises.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ function lookup(hostname, options) {
var hints = 0;
var family = -1;
var all = false;
var verbatim = false;
var verbatim = true;

// Parse arguments
if (hostname && typeof hostname !== 'string') {
Expand All @@ -112,7 +112,7 @@ function lookup(hostname, options) {
hints = options.hints >>> 0;
family = options.family >>> 0;
all = options.all === true;
verbatim = options.verbatim === true;
verbatim = options.verbatim !== false;

validateHints(hints);
} else {
Expand Down
3 changes: 2 additions & 1 deletion test/common/inspector-helper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@ class NodeInstance extends EventEmitter {
console.log('[test]', `Testing ${path}`);
const headers = hostHeaderValue ? { 'Host': hostHeaderValue } : null;
return this.portPromise.then((port) => new Promise((resolve, reject) => {
const req = http.get({ host, port, path, headers }, (res) => {
const req = http.get({ host, family: 4, port, path, headers }, (res) => {
let response = '';
res.setEncoding('utf8');
res
Expand All @@ -418,6 +418,7 @@ class NodeInstance extends EventEmitter {
const devtoolsUrl = response[0].webSocketDebuggerUrl;
const port = await this.portPromise;
return http.get({
family: 4,
port,
path: parseURL(devtoolsUrl).path,
headers: {
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-cluster-message.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ if (cluster.isWorker) {
// When a TCP server is listening in the worker connect to it
worker.on('listening', function(address) {

client = net.connect(address.port, function() {
client = net.connect(address.port, '127.0.0.1', function() {
Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Mar 17, 2021

Choose a reason for hiding this comment

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

Instead of doing this, I suggest changing line 62 to

server.listen(0, common.hasIPv6 ? '::1' : '127.0.0.1');

That way, we are still testing the default value.

Copy link
Copy Markdown
Contributor Author

@treysis treysis Mar 17, 2021

Choose a reason for hiding this comment

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

I don't think that works. Or we have to change the connect-line as follows as well:
client = net.connect(address.port, common.hasIPv6 ? '::1' : '127.0.0.1', function() {

Otherwise it will listen on ::1 on an IPv6-enabled system, but will try to connect to 127.0.0.1 and will fail with ECONNREFUSED.

Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Mar 17, 2021

Choose a reason for hiding this comment

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

Otherwise it will listen on ::1 on an IPv6-enabled system, but will try to connect to 127.0.0.1 and will fail with ECONNREFUSED.

Hum that's not what I see on my machine… It tries to connect to ::1 as expected.
If you don't specify a host in net.connect, it will use localhost, which always resolves to 127.0.0.1 with verbatim set to false (that's the current test). With verbatim set to true, it will resolve to ::1 on IPv6 enabled system, and to 127.0.0.1 otherwise.

Anyway, I don't feel strongly about this, if you think it's simpler let's keep IPv4 for this test.

Copy link
Copy Markdown
Contributor Author

@treysis treysis Mar 17, 2021

Choose a reason for hiding this comment

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

It did almost everywhere. But on SmartOS it would still resolve to 127.0.0.1, probably because ::1 localhost is not set in /etc/hosts. I seem to remember that it also resolved to 127.0.0.1 on my machine until I set that line in /etc/hosts. This wasn't a problem because the Linux kernel always adds 127.0.0.1 to the lo-interface, so even on an otherwise IPv6-only system it would still have that 127.0.0.1 address available and most software will not only listen on ::1 but also 127.0.0.1. Until we can be sure that every system will also have ::1 localhost I think the safe solution is to hardcode the address here.

Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Mar 17, 2021

Choose a reason for hiding this comment

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

Until we can be sure that every system will also have ::1 localhost I think the safe solution is to hardcode the address here.

One could argue that the safe thing to do is to keep verbatim to false… If it's not easy to fix our CI machines, I don't think we can expect our users to fix their machines when updating Node.js.

Copy link
Copy Markdown
Contributor Author

@treysis treysis Mar 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'm sure if we add ::1 localhost to /etc/hosts/ will solve this. Unfortunately I don't have access to SmartOS to examine this. As mentioned already, IPv6 is here to stay and should be accounted for. Having systems with faulty IPv6 will fail eventually. This is a bit ideologic, though, I have to admit. But landing this in the next LTS would give people both the time and the incentive to fix some fundamental issues. Otherwise people that need IPv6 will suffer. Unless we rewrite a lot of the logic, to catch all all cases, and/or implement dualstack listeners (for localhost at least) this could cause some tiny problems. And those will only affect localhost.

// Send message to worker.
worker.send('message from primary');
});
Expand Down
1 change: 1 addition & 0 deletions test/parallel/test-http-localaddress.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ const server = http.createServer((req, res) => {

server.listen(0, '127.0.0.1', () => {
const options = { host: 'localhost',
family: 4,
port: server.address().port,
path: '/',
method: 'GET',
Expand Down
1 change: 1 addition & 0 deletions test/parallel/test-http-upgrade-client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ server.listen(0, '127.0.0.1', common.mustCall(function() {

headers.forEach(function(h) {
const req = http.get({
host: '127.0.0.1',
Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Mar 17, 2021

Choose a reason for hiding this comment

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

Instead of doing this, I suggest changing line 52 to

server.listen(0, common.hasIPv6 ? '::1' : '127.0.0.1', common.mustCall(function() {

That way, we are still testing the default value.

Copy link
Copy Markdown
Contributor Author

@treysis treysis Mar 17, 2021

Choose a reason for hiding this comment

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

That is problematic because we don't know if localhost resolves to ::1 or 127.0.0.1. And that will be the default as it seems if we don't specify the host for http.get. If we do as per your suggestion, we should also do

host: common.hasIPv6 ? '::1' :  '127.0.0.1'

I think it's better to stick to pure IPv4 for now.

port: port,
headers: h
});
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-http2-connect-options.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ server.listen(0, '127.0.0.1', common.mustCall(() => {
const options = { localAddress: '127.0.0.2' };

const client = http2.connect(
'http://localhost:' + server.address().port,
'http://127.0.0.1:' + server.address().port,
options
);
const req = client.request({
Expand Down
1 change: 1 addition & 0 deletions test/parallel/test-https-localaddress.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ const server = https.createServer(options, function(req, res) {
server.listen(0, '127.0.0.1', function() {
const options = {
host: 'localhost',
family: 4,
port: this.address().port,
path: '/',
method: 'GET',
Expand Down
14 changes: 7 additions & 7 deletions test/parallel/test-net-connect-options-port.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ const net = require('net');
}
}, expectedConnections));

server.listen(0, 'localhost', common.mustCall(() => {
server.listen(0, '127.0.0.1', common.mustCall(() => {
Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Mar 17, 2021

Choose a reason for hiding this comment

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

I don't think we should change anything in this test, it should work just fine with IPv6 as with IPv4 – at least it does on my machine.

Suggested change
server.listen(0, '127.0.0.1', common.mustCall(() => {
server.listen(0, 'localhost', common.mustCall(() => {

Copy link
Copy Markdown
Contributor Author

@treysis treysis Mar 17, 2021

Choose a reason for hiding this comment

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

Well, the problem is that listen and connect seem to do DNS lookups differently. So listening on localhost isn't necessarily the same as connecting to localhost. This depends highly on the system configuration and the local resolver. E.g. on SmartOS it seems listening on localhost means listening on ::1, while connecting to localhost means connecting to 127.0.0.1. And on my personal Linux Mint installation it was the other way round.
So this only works reliantly if we do common.hasIPv6 ? '::1' : '127.0.0.1'.

Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Mar 17, 2021

Choose a reason for hiding this comment

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

That's interesting, I missed that information. It makes more sense now. It's a bit concerning that this change may break code using localhost to refer to the loopback address, we should probably document somewhere how to fix this issue (by updating the /etc/hosts file I guess?).

Copy link
Copy Markdown
Contributor Author

@treysis treysis Mar 17, 2021

Choose a reason for hiding this comment

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

That's a good question and I am not sure how this will play out in production outside of the test suite. If everyone would have the hosts-file ready for IPv6 (i.e. including ::1) that should be okay. I seem to understand this is why this PR should land in a major version. Maybe even the DNS lib needs updating? I don't understand why it uses different methods.

See https://github.com/nodejs/node/blob/master/lib/net.js and look for dns.lookup. The code around it for lookupAndListen and lookupAndConnect is different, though I haven't really understood what the differences do.

On the other hand, not setting verbatim=true by default seriously hurts IPv6 adoption. Currently, as long as there's an A record available, NodeJS will always default to the IPv4 address. Unless the dev using the lib manually sets it to true (which almost nobody does). Of course the best would be if listening on localhost would open a dualstack socket on both 127.0.0.1 and ::1. At the moment, this only works for 0.0.0.0.

const port = server.address().port;

// Total connections = 3 * 4(canConnect) * 6(doConnect) = 72
Expand Down Expand Up @@ -167,25 +167,25 @@ function canConnect(port) {
const noop = () => common.mustCall();

// connect(port, cb) and connect(port)
const portArgFunctions = doConnect([port], noop);
const portArgFunctions = doConnect([port, '127.0.0.1'], noop);
for (const fn of portArgFunctions) {
fn();
}

// connect(port, host, cb) and connect(port, host)
const portHostArgFunctions = doConnect([port, 'localhost'], noop);
const portHostArgFunctions = doConnect([port, '127.0.0.1'], noop);
for (const fn of portHostArgFunctions) {
fn();
}

// connect({port}, cb) and connect({port})
const portOptFunctions = doConnect([{ port }], noop);
const portOptFunctions = doConnect([{ port, family: 4 }], noop);
for (const fn of portOptFunctions) {
fn();
}

// connect({port, host}, cb) and connect({port, host})
const portHostOptFns = doConnect([{ port, host: 'localhost' }], noop);
const portHostOptFns = doConnect([{ port, host: '127.0.0.1' }], noop);
for (const fn of portHostOptFns) {
fn();
}
Expand All @@ -205,13 +205,13 @@ function asyncFailToConnect(port) {
}

// connect({port}, cb) and connect({port})
const portOptFunctions = doConnect([{ port }], dont);
const portOptFunctions = doConnect([{ port, family: 4 }], dont);
for (const fn of portOptFunctions) {
fn().on('error', onError());
}

// connect({port, host}, cb) and connect({port, host})
const portHostOptFns = doConnect([{ port, host: 'localhost' }], dont);
const portHostOptFns = doConnect([{ port, host: '127.0.0.1' }], dont);
for (const fn of portHostOptFns) {
fn().on('error', onError());
}
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-net-dns-lookup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ const server = net.createServer(function(client) {
});

server.listen(0, '127.0.0.1', common.mustCall(function() {
net.connect(this.address().port, 'localhost')
net.connect({ port: this.address().port, host: 'localhost', family: 4 })
.on('lookup', common.mustCall(function(err, ip, type, host) {
assert.strictEqual(err, null);
assert.strictEqual(ip, '127.0.0.1');
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-net-pingpong.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,6 @@ const tmpdir = require('../common/tmpdir');
tmpdir.refresh();
pingPongTest(common.PIPE);
pingPongTest(0);
pingPongTest(0, 'localhost');
pingPongTest(0, '127.0.0.1');
Copy link
Copy Markdown
Contributor Author

@treysis treysis Mar 16, 2021

Choose a reason for hiding this comment

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

Suggested change
pingPongTest(0, '127.0.0.1');
pingPongTest(0, '127.0.0.1');
if (common.hasIPv6)
pingPongTest(0, '::1');

Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Mar 17, 2021

Choose a reason for hiding this comment

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

Let's keep localhost.

Suggested change
pingPongTest(0, '127.0.0.1');
pingPongTest(0, 'localhost');
pingPongTest(0, '127.0.0.1');

Copy link
Copy Markdown
Contributor Author

@treysis treysis Mar 17, 2021

Choose a reason for hiding this comment

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

Will fail at least on SmartOS because listening on localhost resolves to ::1, while connecting to localhost resolves to 127.0.0.1.

Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Mar 25, 2021

Choose a reason for hiding this comment

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

PIng, can we add localhost back? :)

Copy link
Copy Markdown
Contributor Author

@treysis treysis Mar 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hm, that doesn't make much sense to me as this test is clearly aimed at IPv4, because there's the second test that explicitly checks for IPv6. Furthermore, localhost here will resolve to two different addresses for listen and connect. But I'll do it just to see the result.
Should I mark it as flaky as well?

Copy link
Copy Markdown
Contributor Author

@treysis treysis Mar 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, so with localhost it fails. I'd really change it to IPv4 literal here because that's clearly what is intended. I've also marked it flaky on SmartOS.

if (common.hasIPv6)
pingPongTest(0, '::1');
6 changes: 3 additions & 3 deletions test/parallel/test-net-remote-address-port.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,9 @@ const server = net.createServer(common.mustCall(function(socket) {
socket.resume();
}, 2));

server.listen(0, 'localhost', function() {
const client = net.createConnection(this.address().port, 'localhost');
const client2 = net.createConnection(this.address().port);
server.listen(0, '127.0.0.1', function() {
const client = net.createConnection(this.address().port, '127.0.0.1');
const client2 = net.createConnection(this.address().port, '127.0.0.1');
Comment thread
treysis marked this conversation as resolved.
Outdated
client.on('connect', function() {
assert.ok(remoteAddrCandidates.includes(client.remoteAddress));
assert.ok(remoteFamilyCandidates.includes(client.remoteFamily));
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-net-writable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ const net = require('net');
const server = net.createServer(common.mustCall(function(s) {
server.close();
s.end();
})).listen(0, 'localhost', common.mustCall(function() {
const socket = net.connect(this.address().port, 'localhost');
})).listen(0, '127.0.0.1', common.mustCall(function() {
const socket = net.connect(this.address().port, '127.0.0.1');
Comment thread
treysis marked this conversation as resolved.
Outdated
socket.on('end', common.mustCall(() => {
assert.strictEqual(socket.writable, true);
socket.write('hello world');
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-tcp-wrap-listen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ server.onconnection = (err, client) => {

const net = require('net');

const c = net.createConnection(port);
const c = net.createConnection(port, '127.0.0.1');

c.on('connect', common.mustCall(() => { c.end('hello world'); }));

Expand Down
6 changes: 4 additions & 2 deletions test/parallel/test-timers-socket-timeout-removes-other-socket-unref-timer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,14 @@ server.listen(0, common.localhostIPv4, common.mustCall(() => {
const countdown = new Countdown(2, () => server.close());

{
const client = net.connect({ port: server.address().port });
const client = net.connect({ port: server.address().port,
host: '127.0.0.1' });
client.on('end', () => countdown.dec());
}

{
const client = net.connect({ port: server.address().port });
const client = net.connect({ port: server.address().port,
host: '127.0.0.1' });
client.on('end', () => countdown.dec());
}
}));
1 change: 1 addition & 0 deletions test/parallel/test-tls-client-getephemeralkeyinfo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ function test(size, type, name, cipher) {
server.listen(0, '127.0.0.1', common.mustCall(() => {
const client = tls.connect({
port: server.address().port,
host: '127.0.0.1',
rejectUnauthorized: false
}, common.mustCall(function() {
const ekeyinfo = client.getEphemeralKeyInfo();
Expand Down
1 change: 1 addition & 0 deletions test/parallel/test-tls-client-mindhsize.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ function test(size, err, next) {
const client = tls.connect({
minDHSize: 2048,
port: this.address().port,
host: '127.0.0.1',
Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Mar 17, 2021

Choose a reason for hiding this comment

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

Suggested change
host: '127.0.0.1',
family: 4,

Copy link
Copy Markdown
Contributor Author

@treysis treysis Mar 17, 2021

Choose a reason for hiding this comment

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

We can try this but I think it's safer to use hardcoded IPv4 because not specifying the host would mean it defaults to localhost anyways which was always expected to be 127.0.0.1 by the test suite.

rejectUnauthorized: false
}, function() {
nsuccess++;
Expand Down
1 change: 1 addition & 0 deletions test/parallel/test-tls-wrap-econnreset-localaddress.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const server = net.createServer((c) => {
let errored = false;
tls.connect({
port: port,
family: 4,
localAddress: common.localhostIPv4
}, common.localhostIPv4)
.once('error', common.mustCall((e) => {
Expand Down
2 changes: 1 addition & 1 deletion test/sequential/test-https-connect-localport.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ const assert = require('assert');
res.end();
}));

server.listen(0, 'localhost', common.mustCall(() => {
server.listen(0, '127.0.0.1', common.mustCall(() => {
const port = server.address().port;
const req = https.get({
host: 'localhost',
Expand Down
2 changes: 1 addition & 1 deletion test/sequential/test-inspector-open.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ function reopenAfterClose(msg) {
}

function ping(port, callback) {
net.connect(port)
net.connect(port, '127.0.0.1')
Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Mar 17, 2021

Choose a reason for hiding this comment

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

The inspector should "just work" with the default IMHO, the fact that it defaults to IPv4 goes a bit against that. I think it comes from here:

node/src/node_options.h

Line 85 in f392ac0

HostPort host_port{"127.0.0.1", kDefaultInspectorPort};

Not that it has anything to do with this PR, I just wanted to point it out.

Copy link
Copy Markdown
Contributor Author

@treysis treysis Mar 17, 2021

Choose a reason for hiding this comment

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

It should, but it doesn't. On some systems it will listen on 127.0.0.1, and on others it will listen on ::1.

.on('connect', function() { close(this); })
.on('error', function(err) { close(this, err); });

Expand Down
2 changes: 1 addition & 1 deletion test/sequential/test-net-better-error-messages-port.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ const common = require('../common');
const net = require('net');
const assert = require('assert');

const c = net.createConnection(common.PORT);
const c = net.createConnection(common.PORT, common.localhostIPv4);
Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Mar 17, 2021

Choose a reason for hiding this comment

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

Suggested change
const c = net.createConnection(common.PORT, common.localhostIPv4);
const c = net.createConnection(common.PORT);

Shouldn't you instead change the assertion line 13:

assert.strictEqual(e.address, common.hasIPv6 ? '::1' : '127.0.0.1');

Copy link
Copy Markdown
Contributor Author

@treysis treysis Mar 17, 2021

Choose a reason for hiding this comment

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

No, because we can't predict to which IP address net.createConnection will try to connect to. This is independent of the system having IPv6 or not. Leaving out the address in net.createConnection just means it tries to connect to localhost, which is not reliably resolved on dualstack systems. So if we leave out the common.localhostIPv4 we have to use RegEx again:

assert.match(e.address, /^(127\.0\.0\.1|::1)$/);

Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Mar 26, 2021

Choose a reason for hiding this comment

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

FWIW I think assert.match would make more sense for this test.

Copy link
Copy Markdown
Contributor Author

@treysis treysis Mar 26, 2021

Choose a reason for hiding this comment

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

And also remove , common.localhostIPv4?


c.on('connect', common.mustNotCall());

Expand Down
4 changes: 3 additions & 1 deletion test/sequential/test-net-connect-local-error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,16 @@ const expectedErrorCodes = ['ECONNREFUSED', 'EADDRINUSE'];
const optionsIPv4 = {
port: common.PORT,
localPort: common.PORT + 1,
localAddress: common.localhostIPv4
localAddress: common.localhostIPv4,
family: 4
Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Mar 13, 2021

Choose a reason for hiding this comment

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

nit

Suggested change
family: 4
family: 4,

Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Mar 17, 2021

Choose a reason for hiding this comment

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

I would still like to keep the trailing comma.

Copy link
Copy Markdown
Contributor Author

@treysis treysis Mar 17, 2021

Choose a reason for hiding this comment

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

I haven't seen the trailing comma being used in other files, but I'm okay with either way.

Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Mar 16, 2021

Choose a reason for hiding this comment

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

I noticed you added the family option in a few tests where it was using the default value before. Is it because this PR removes/breaks the default value somehow? If that's the case, it would be worth mentioning it in the docs I think.

Copy link
Copy Markdown

@richfelker richfelker Mar 16, 2021

Choose a reason for hiding this comment

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

I would assume it's because, being options for IPv4, this configuration is seeking to get a v4 connection. Previously it would have due to reordering to always put v4 first. Now, it has to explicitly request that it wants v4.

Copy link
Copy Markdown
Contributor Author

@treysis treysis Mar 16, 2021

Choose a reason for hiding this comment

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

Exactly. Sometimes it would also try to bind to the wrong address family, causing EINVAL. Chosing family was a way to force IPv4 without messing with the defaults.

PS: I am not happy with test-net-pingping.js. I should adjust it and readd the IPv6 test part.

Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Mar 17, 2021

Choose a reason for hiding this comment

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

family : Version of IP stack. Must be 4, 6, or 0. The value 0 indicates that both IPv4 and IPv6 addresses are allowed. Default: 0.

I don't think we should need to specify it though, when using the default value both IPv4 and IPv6 should be allowed.

Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Mar 17, 2021

Choose a reason for hiding this comment

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

Suggested change
family: 4
host: common.localhostIPv4,

Copy link
Copy Markdown
Contributor Author

@treysis treysis Mar 17, 2021

Choose a reason for hiding this comment

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

It should. But since we can't reliably predict whether localhost resolves to IPv6 or to IPv4, and sometimes this would throw EINVAL because for some reason NodeJS would try to bind IPv4 address on IPv6 socket and vice versa. We can solve this by forcing the address family.

Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Mar 17, 2021

Choose a reason for hiding this comment

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

Or you can force the IP address, as per my suggestion

Suggested change
family: 4
host: common.localhostIPv4,

Previously, host was defined for optionsIPv6 and not for optionsIPv4. I think it makes more sense to just switch the two (have a host in optionsIPv4, and not in optionsIPv6). Or we could have the family option set in optionsIPv4 and not optionsIPv6, but it needs to be consistent IMHO.

Copy link
Copy Markdown
Contributor Author

@treysis treysis Mar 17, 2021

Choose a reason for hiding this comment

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

Why not have host and family in both?

Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Mar 25, 2021

Choose a reason for hiding this comment

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

Sure, can you make this change please?

Copy link
Copy Markdown
Contributor Author

@treysis treysis Mar 25, 2021

Choose a reason for hiding this comment

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

This one passed well.

};

const optionsIPv6 = {
host: '::1',
Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Mar 17, 2021

Choose a reason for hiding this comment

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

Suggested change
host: '::1',

Copy link
Copy Markdown
Contributor Author

@treysis treysis Mar 17, 2021

Choose a reason for hiding this comment

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

I wouldn't remove it. I feel this might break stuff.

port: common.PORT + 2,
localPort: common.PORT + 3,
localAddress: '::1',
family: 6
Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Mar 13, 2021

Choose a reason for hiding this comment

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

nit

Suggested change
family: 6
family: 6,

Copy link
Copy Markdown
Contributor Author

@treysis treysis Mar 17, 2021

Choose a reason for hiding this comment

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

Again, I haven't seen trailing comma elsewhere in the test suite, but no objection from my side.

Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Mar 17, 2021

Choose a reason for hiding this comment

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

Suggested change
family: 6

Copy link
Copy Markdown
Contributor Author

@treysis treysis Mar 17, 2021

Choose a reason for hiding this comment

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

I think this was necessary here.

Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Mar 17, 2021

Choose a reason for hiding this comment

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

It's not, the host and localAddress are both IPv6, adding a family: 6 here is redundant. It certainly doesn't harm to have it though, but I'm confident this is not necessary.

Copy link
Copy Markdown
Contributor Author

@treysis treysis Mar 17, 2021

Choose a reason for hiding this comment

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

Yeah, unfortunately I lost a bit track during the extensive test runs where this failed and if this specifically fixed it or not. I would prefer to have family included.

};

function onError(err, options) {
Expand Down