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

src: cares_wrap: only loop through once on lookup #4693

Closed
wants to merge 1 commit into from

Conversation

evanlucas
Copy link
Contributor

The results will already be sorted based on the implementation of
getaddrinfo(). There is no need to loop through the returned addresses
twice (once for IPv4, once for IPv6). We should return them in the
order in which they already are.

References:

This may need to be marked as semver-major

The results will already be sorted based on the implementation of
getaddrinfo(). There is no need to loop through the returned addresses
twice (once for IPv4, once for IPv6). We should return them in the
order in which they already are.
@evanlucas evanlucas added the dns Issues and PRs related to the dns subsystem. label Jan 14, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Jan 14, 2016

Anyone know the original reason for doing this? Looks like it happened way back in 194511f.

@mscdex mscdex added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 14, 2016
@indutny
Copy link
Member

indutny commented Jan 19, 2016

The original reason was to make sure that it won't break on IPv4-only machines. We don't do happy eyeballs in node, so it was pretty important at that time, I guess? cc @bnoordhuis

@indutny
Copy link
Member

indutny commented Jan 19, 2016

As of it, I am not sure if we should fix it, or leave it as it is.

@bnoordhuis
Copy link
Member

What Fedor said, node prefers IPv4 over IPv6 to avoid breakage when there is no IPv6 connectivity.

We don't do happy eyeballs. If you change the order, that would have to change. That's a big can of worms.

@evanlucas
Copy link
Contributor Author

Ok, thanks for the explanation. Is us implementing happy eyeballs something we should explore? Or is that too big of a breaking change? Or is it just not worth it?

@bnoordhuis
Copy link
Member

I don't know how others feel but I'm of the opinion that happy eyeballs is a shameful hack with too much potential for confusing, seemingly random behavior.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 19, 2016

Seems like it would make bugs more difficult to reproduce and fix.

@evanlucas
Copy link
Contributor Author

Ok, that works for me.

@igalic
Copy link

igalic commented Dec 28, 2016

@indutny The original reason was to make sure that it won't break on IPv4-only machines. We don't do happy eyeballs in node, so it was pretty important at that time, I guess? cc @bnoordhuis

unfortunately, the consequence is that it breaks IPv6 only machines.

@mdavids
Copy link

mdavids commented Dec 13, 2019

Any news?

@schleiftier
Copy link

Referring to the comment of @igalic : Yes, exactly that is happening for me now. I have a nodejs program on a ipv6 only machine and it fails when trying to connect to outside servers because ipv4 is given preference, but no connection can be established.

Is there any way to work around that problem?

@bnoordhuis
Copy link
Member

You can pass { verbatim: true } to dns.lookup(), that returns the results in the order they were received from the DNS server.

@schleiftier
Copy link

In theory I could, but the problem is actually hidden a few layers deep within the software I am trying to run.

The actual problem seems to be caused within the package nodemailer when trying to send an email using SMTP. Nodemailer is used by the mailing list tool mailtrain, which is running inside a docker container.

I could of course now dive into the source code of nodemailer, try to isolate the line which is causing the problem, then extract & fix the corresponding file and mount it as an overlay into the docker container, but that really seems like an overly complex solution :/

@treysis
Copy link
Contributor

treysis commented Jan 21, 2021

Maybe the problem is even higher? I don't recall IPv4-only machines even doing AAAA-lookups. So why is an IPv6-only machine doing A-lookups? Should this be addressed in the Linux kernel?

And for short term solution until NodeJS introduces happy eyeballs: there should be two packages made available, one with IPv4-preference, and one with IPv6-preference. This way it satisfies both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. dns Issues and PRs related to the dns subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.