This repository has been archived by the owner. It is now read-only.

Implement dns.lookupAll() and use for net.connect() #4169

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
6 participants

In hope that this fixes #4168, this implements dns.lookupAll() to return all A/AAAA RRs for a hostname retrieved from getaddrinfo() and subsequently sets net.connect() to use this when connecting to remote hosts. This requires re-working of some of the other net connection code to retry with another host on failure and to setup a new TCP socket handle for each so hopefully it's all OK still.

This is my first node hacking; be gentle!

@bnoordhuis bnoordhuis commented on an outdated diff Oct 20, 2012

lib/dns.js
@@ -97,27 +107,45 @@ exports.lookup = function(domain, family, callback) {
return {};
}
+ var callbackAddr = function(address, fam){
@bnoordhuis

bnoordhuis Oct 20, 2012

Member

s/fam/family/

Member

bnoordhuis commented Oct 20, 2012

I see no fundamental flaws in your PR but I'm somewhat undecided if trying addresses sequentially is a good thing. In a worst case scenario none of the addresses are reachable/usable but all of a sudden your net.connect() calls takes ten times as long to finish.

In the past, we've always recommended that people (most likely library authors) handle this manually because there's no 'one size fits all' solution.

Member

bnoordhuis commented Oct 20, 2012

Also, this needs tests. :-)

Thanks Ben for the feedback.

I've updated my PR with a refactored dns.lookupAll(), changed lookup() to just wrap lookupAll() and pulled out the option parsing.

I'll take a look at some tests for lookupAll(), but the existing tests for lookup() will now apply here too.

Also, in response to the "...we've recommended that people handle this manually..." - the main issue I'm trying to resolve is with node's own net.connect() which does a dns.lookup() and only tries to connect to the first IP (no matter which address family it belongs to).

On a machine with only IPv4, this will fail if that first IP is an IPv6 address (even if that host does have an IPv4 address) and similarly for a machine with only IPv6 if the first IP is an IPv4 address; this is horribly broken, so the extra delay is a lesser of two evils.

net: with dns.lookupAll() don't die on first address failure.
This fixes errors from an IPv4-only or IPv6-only host connecting
to the opposite address family; this used to throw an EHOSTUNREACH
or ENETUNREACH because the host didn't have a route to that address
family.

Can one of the admins verify this patch?

I'm wondering what the status of this is? Issue #4168 is a blocker on my current project.

@cmawhorter cmawhorter referenced this pull request in request/request Apr 28, 2015

Open

Doesn't try next IP after ECONNREFUSED? #1551

Owner

jasnell commented Jun 1, 2015

At this point, it's likely best to close this. There is work underway to refactor dns support entirely within io.js. That work would be picked up in the converged stream and this would need to be updated significantly.

@tomlanyon ... I know it's been a while, but is this still something you want to pursue? If yes, the PR would need to be updated significantly. If not, we should likely go ahead and close.

Happy for this to be closed. I suggest that lookups in whatever DNS changes occur in io.js need to implement support for trying A and AAAA records, trying multiple records - potentially concurrently (happy eyeballs).

Owner

jasnell commented Jun 1, 2015

Noted! Closing

@jasnell jasnell closed this Jun 1, 2015

cjihrig commented Jun 2, 2015

I only read the title of this PR just now, so this could be something different, but dns.lookup({all: true}) landed in io.js a while back.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.