dns - add 'all' option to dns.lookup by silverwind · Pull Request #744 · nodejs/node

silverwind · 2015-02-06T16:46:30Z

Issue: #736

Open questions:

Should dns.lookup(null, {all: true}) return an empty array (returns null right now)?
I assume c-ares can't return mixed 4/6 results, as the callback just gets addresses. Correct?
Test: Is it possible to reliably test the !this.family code path?

/cc @bnoordhuis

cjihrig · 2015-02-06T16:51:52Z

lib/dns.js

Can you do this with a for loop instead of map() for performance reasons.

Sure, if the difference is worthwile.

bnoordhuis · 2015-02-06T16:54:53Z

Should dns.lookup(null, {all: true}) return an empty array (returns null right now) ?

That seems reasonable.

I assume C-Ares can't return mixed 4/6 results, as the callback just gets addresses. Correct?

Wrong. :-)

The logic is in AfterGetAddrInfo() in src/cares_wrap.cc and probably needs to be adjusted in this PR. In a nutshell, you can tell c-ares to query for IPv4 and IPv6 by setting the address family to AF_UNSPEC (family=0 in JS.)

Test: Is it possible to reliably test the !this.family code path?

Not easily, I think.

bnoordhuis · 2015-02-06T16:56:46Z

lib/dns.js

addr.includes(':')?

EDIT: Never mind, just noticed that onlookup() does the same.

cjihrig · 2015-02-06T16:59:18Z

@bnoordhuis why does the C++ logic need to change? It's returning the correct array based on the desired family. The JS logic is just giving all the results back instead of the first result.

bnoordhuis · 2015-02-06T17:02:51Z

@cjihrig Please disregard that remark. I wrote that before I realized that lib/dns.js already does an ersatz IPv6 check.

cjihrig · 2015-02-06T17:04:48Z

test/internet/test-dns.js

Can you add a test that doesn't specify the family and check for mixed results.

You mean fail the test if mixed results are obtained?

No. If you don't specify the family, you should be able to get all IPv4 and IPv6 addresses.

Ah, I see what you mean:

> require("dns").lookup("www.google.com", {all: true}, console.log) null [ { address: '188.21.9.123', family: 4 }, { address: '188.21.9.117', family: 4 }, { address: '188.21.9.121', family: 4 }, { address: '188.21.9.122', family: 4 }, { address: '188.21.9.118', family: 4 }, { address: '188.21.9.119', family: 4 }, { address: '188.21.9.116', family: 4 }, { address: '188.21.9.120', family: 4 }, { address: '2a00:1450:4014:80a::1010', family: 6 } ]

silverwind · 2015-02-06T17:05:00Z

So, no change need there? I see that family = 0 can reach cares.getaddrinfo.

cjihrig · 2015-02-06T17:07:10Z

doc/api/dns.markdown

Can you add Defaults to false.

This describes how the callback arguments are changed, but doesn't really describe the purpose of all. Can you add something like "Return all matching addresses when true"

bnoordhuis · 2015-02-06T17:08:15Z

So, no change need there? I see that family = 0 can reach cares.getaddrinfo.

Yep, it's fine.

silverwind · 2015-02-06T17:51:32Z

Mixed test done. Had to fight a few other test failing, but both appear to be because of my specific dns setup. Just to note, the failing tests on OS X were:

test_resolve_failure: returned ENODATA instead of ENOTFOUND / ESERVFAIL
test_reverse_failure: my router did reverse-resolve 0.0.0.0 because I have intercepting adblocking through DNS in place.

cjihrig · 2015-02-06T18:35:39Z

doc/api/dns.markdown

This should go before the With the all option set,... sentence.

The whole paragraph?

Yes. I think it's helpful to define the default behavior before going into what it means for all to be set.

silverwind · 2015-02-06T19:21:43Z

All done. I restructured the docs to make more sense in the last commit.

Let me know when to squash, as I'd like to fix the commit message.

cjihrig · 2015-02-06T19:27:55Z

lib/dns.js

Tiny style nit, but can you put the var here.

Sure. Generally, I try to put var at the start of the scope, but i guess I'll follow the style of that file.

cjihrig · 2015-02-06T19:29:57Z

There is one case that hasn't been covered - https://github.com/iojs/io.js/blob/v1.x/lib/dns.js#L130. Other than that, and the style nit, LGTM. You can squash, the commits and follow the instructions in https://github.com/iojs/io.js/blob/v1.x/CONTRIBUTING.md#step-3-commit

silverwind · 2015-02-06T20:09:21Z

@cjihrig honored your last suggestions and also added two more tests. do these look good to you so I can squash?

cjihrig · 2015-02-06T20:19:45Z

test/internet/test-dns.js

It doesn't matter a ton since this is a test, but I think you'd want to check for Array.isArray(ips) before checking the ips.length.

Right, I'll reorder the type checks to be on top.

The 'all' option allows to retrieve all addresses returned by `getaddrinfo` using the system resolver. While `dns.resolve` already does return multiple results, it's often preferable to use the system resolver, like for mDNS.

silverwind · 2015-02-06T20:57:57Z

And it's squashed.

cjihrig · 2015-02-06T21:27:43Z

Let's see what the CI thinks. https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/154/

I'll do some testing locally too. I don't think the CI runs the internet tests.

cjihrig · 2015-02-06T22:11:24Z

CI seems happy - the FreeBSD and Arm failures are pretty standard. I also ran the Internet tests locally, and they passed. This is a semver minor bump, but that window is already open due to #697.

cjihrig · 2015-02-06T22:14:46Z

Thanks! Landed in eb30009de65a9432b59ae77a425b623df28dbd27 with a tweaked commit message.

EDIT: Someone performed a force push after I landed this. So the mentioned commit was overwritten. The actual commit is now 633a990

This commit adds the 'all' option to dns.lookup(), allowing all lookup results to be returned. Semver: Minor Fixes: #736 PR-URL: #744 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

micnic · 2015-02-06T22:32:49Z

@cjihrig, oops, it was me, sorry for that, I edited a wrong commit message

cjihrig · 2015-02-06T22:37:53Z

@micnic no big deal. It's taken care of.

@sonewman

Notable changes: * stream: - Simpler stream construction, see nodejs/readable-stream#102 for details. This extends the streams base objects to make their constructors accept default implementation methods, reducing the boilerplate required to implement custom streams. An updated version of readable-stream will eventually be released to match this change in core. (@sonewman) * dns: - `lookup()` now supports an `'all'` boolean option, default to `false` but when turned on will cause the method to return an array of *all* resolved names for an address, see, #744 (@silverwind) * assert: - Remove `prototype` property comparison in `deepEqual()`, considered a bugfix, see #636 (@vkurchatkin) - Introduce a `deepStrictEqual()` method to mirror `deepEqual()` but performs strict equality checks on primitives, see #639 (@vkurchatkin) * **tracing**: - Add LTTng (Linux Trace Toolkit Next Generation) when compiled with the `--with-lttng` option. Trace points match those available for DTrace and ETW. #702 (@thekemkid) * npm upgrade to 2.5.1 * **libuv** upgrade to 1.4.0 * Add new collaborators: - Aleksey Smolenchuk (@lxe) - Shigeki Ohtsu (@shigeki)

cjihrig reviewed Feb 6, 2015
View reviewed changes

bnoordhuis reviewed Feb 6, 2015
View reviewed changes

cjihrig reviewed Feb 6, 2015
View reviewed changes

dns: add the 'all' option to dns.lookup

0a914bf

The 'all' option allows to retrieve all addresses returned by `getaddrinfo` using the system resolver. While `dns.resolve` already does return multiple results, it's often preferable to use the system resolver, like for mDNS.

silverwind force-pushed the lookup-multiple branch from f27e8af to 0a914bf Compare February 6, 2015 20:57

cjihrig added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 6, 2015

cjihrig closed this Feb 6, 2015

silverwind mentioned this pull request Feb 8, 2015

doc: fix dns.lookup options example #757

Closed

rvagg mentioned this pull request Feb 9, 2015

Release proposal: v1.2.0 #768

Closed

bnb mentioned this pull request Feb 12, 2015

Evangelism: Weekly Update Feb 13th nodejs/iojs.org#122

Closed

Schmoopiie mentioned this pull request Feb 15, 2015

Traduction de IO.js week of February 13th 2015 nodejs/nodejs-fr#24

Closed

Uh oh!

Conversation

silverwind commented Feb 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Feb 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Feb 6, 2015

Uh oh!

bnoordhuis commented Feb 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverwind commented Feb 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Feb 6, 2015

Uh oh!

silverwind commented Feb 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverwind commented Feb 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Feb 6, 2015

Uh oh!

silverwind commented Feb 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverwind commented Feb 6, 2015

Uh oh!

cjihrig commented Feb 6, 2015

Uh oh!

cjihrig commented Feb 6, 2015

Uh oh!

cjihrig commented Feb 6, 2015

Uh oh!

micnic commented Feb 6, 2015

Uh oh!

cjihrig commented Feb 6, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants