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

dns: fix `resolve` failed starts without network #13076

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@XadillaX
Member

XadillaX commented May 17, 2017

Fix the bug that you start process without network at first, but it connected lately, dns.resolve will stay failed with ECONNREFUSED because c-ares servers fallback to 127.0.0.1 at the very beginning.

If c-ares servers "127.0.0.1" is detected and its not set by user self, and last query is not OK, recreating ares_channel operation will be triggered to reload servers.

You can test with this script:

const dns = require("dns");

function test() {
    dns.resolve("google.com", function(err, ret) {
        console.log(err, ret, dns.getServers());
        setTimeout(test, 10000);
    });
}

test();

Turn off your network at first before starting this script. Then try to open the network and then turn off. Make the steps above in a loop and you will get the result.

Fixes: #1644

Solutaion

DNS server matches follow conditions is considered as a fallback server of c-ares:

  1. caused last query ECONNREFUSED; (the fallback server very likely to have this error)
  2. is exactly [ "127.0.0.1" ]; (the fallback server is exactly this value)
  3. haven't set by users or developers by dns.setServers.

Because users or developers may do dns.setServers([ "127.0.0.1" ]) or theirs /etc/resolv.conf is 127.0.0.1.

As a result, I gathered those 3 conditions.

I created two flags at the process start up.

  • cares_query_last_ok_ indicates that is last dns query succeeded or not;
  • cares_is_servers_use_default_ indicates that whether current dns servers is the fallback 127.0.0.1 that c-ares initialized at the beginning.

If cares_query_last_ok_ is true or cares_is_servers_use_default_ is false, everything will be OK.

If not, the code will check current dns servers to see if it's 127.0.0.1 (maybe c-ares' fallback server). When it's 127.0.0.1, old ares_channel will be released and a new one will be created.

Once one query succeeded or not ECONNREFUSED, cares_query_last_ok_ will be marked as true. And once user set dns servers via dns.setServers, cares_is_servers_use_default_ will be marked as false. And what's more, once the first check found the dns servers is not 127.0.0.1, cares_is_servers_use_default_ also will be marked as false.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

dns

@XadillaX

This comment has been minimized.

Show comment
Hide comment
@XadillaX

XadillaX May 17, 2017

Member

Anyone who has time to review this code?

/cc @trevnorris @indutny @addaleax @danbev

Member

XadillaX commented May 17, 2017

Anyone who has time to review this code?

/cc @trevnorris @indutny @addaleax @danbev

Show outdated Hide outdated src/env-inl.h
inline void Environment::cares_servers_use_default(bool is_default) {
cares_is_servers_use_default_ = is_default;
}

This comment has been minimized.

@addaleax

addaleax May 17, 2017

Member

Any chance you could name the setters set_FLAG() and the getters just FLAG() (compare with e.g. using_domains)?

@addaleax

addaleax May 17, 2017

Member

Any chance you could name the setters set_FLAG() and the getters just FLAG() (compare with e.g. using_domains)?

Show outdated Hide outdated src/cares_wrap.cc
@@ -1306,6 +1373,8 @@ void SetServers(const FunctionCallbackInfo<Value>& args) {
delete[] servers;
if (err == ARES_SUCCESS) env->cares_servers_use_default(false);

This comment has been minimized.

@addaleax

addaleax May 17, 2017

Member

Extremely tiny nit: I’d put these on separate lines

@addaleax

addaleax May 17, 2017

Member

Extremely tiny nit: I’d put these on separate lines

Show outdated Hide outdated src/cares_wrap.cc
@@ -328,10 +386,18 @@ class QueryWrap : public AsyncWrap {
return static_cast<void*>(this);
}
static void AresQuery(Environment* env, const char* name,
int dnsclass, int type, ares_callback callback,

This comment has been minimized.

@addaleax

addaleax May 17, 2017

Member

I realize the code around this doesn’t do it either, but once more, would you mind aligning the arguments vertically? 😄

@addaleax

addaleax May 17, 2017

Member

I realize the code around this doesn’t do it either, but once more, would you mind aligning the arguments vertically? 😄

Show outdated Hide outdated src/cares_wrap.cc
* The fallback servers of cares is [ "127.0.0.1" ] with no user additional
* setting.
*/
void AresCheckServers(Environment* env) {

This comment has been minimized.

@addaleax

addaleax May 17, 2017

Member

tiny nit: Check sounds a bit like it’s just testing things, maybe Ensure is better?

@addaleax

addaleax May 17, 2017

Member

tiny nit: Check sounds a bit like it’s just testing things, maybe Ensure is better?

Show outdated Hide outdated src/cares_wrap.cc
/* We do the call to ares_init_option for caller. */
int r = ares_init_options(env->cares_channel_ptr(),
&options,
ARES_OPT_FLAGS | ARES_OPT_SOCK_STATE_CB);

This comment has been minimized.

@addaleax

addaleax May 17, 2017

Member

(ditto for aligning)

@addaleax

addaleax May 17, 2017

Member

(ditto for aligning)

memset(&options, 0, sizeof(options));
options.flags = ARES_FLAG_NOCHECKRESP;
options.sock_state_cb = ares_sockstate_cb;
options.sock_state_cb_data = env;

This comment has been minimized.

@addaleax

addaleax May 17, 2017

Member

Can we somehow share this code with Initialize, to make it more obviously correct? It looks like there’s quite a bit that could be shared

@addaleax

addaleax May 17, 2017

Member

Can we somehow share this code with Initialize, to make it more obviously correct? It looks like there’s quite a bit that could be shared

This comment has been minimized.

@XadillaX

XadillaX May 18, 2017

Member

I'v extracted these code to SetupCaresChannel().

@XadillaX

XadillaX May 18, 2017

Member

I'v extracted these code to SetupCaresChannel().

@addaleax

This comment has been minimized.

Show comment
Hide comment
@pmq20

This comment has been minimized.

Show comment
Hide comment
@pmq20

pmq20 May 18, 2017

Contributor

One of Windows CI Machines failed. I wonder why.

not ok 2 parallel/test-async-wrap-getasyncid
  ---
  duration_ms: 0.248
  severity: fail
  stack: |-
    Mismatched <anonymous> function calls. Expected 1, actual 0.
        at Object.<anonymous> (c:\workspace\node-test-binary-windows\RUN_SUBSET\0\VS_VERSION\vs2015-x86\label\win2008r2\test\parallel\test-async-wrap-getasyncid.js:155:42)
        at Module._compile (module.js:569:30)
        at Object.Module._extensions..js (module.js:580:10)
        at Module.load (module.js:503:32)
        at tryModuleLoad (module.js:466:12)
        at Function.Module._load (module.js:458:3)
        at Function.Module.runMain (module.js:605:10)
        at startup (bootstrap_node.js:144:16)
        at bootstrap_node.js:561:3
    (node:5140) [DEP0064] DeprecationWarning: tls.createSecurePair() is deprecated. Please use tls.Socket instead.
  ...
Contributor

pmq20 commented May 18, 2017

One of Windows CI Machines failed. I wonder why.

not ok 2 parallel/test-async-wrap-getasyncid
  ---
  duration_ms: 0.248
  severity: fail
  stack: |-
    Mismatched <anonymous> function calls. Expected 1, actual 0.
        at Object.<anonymous> (c:\workspace\node-test-binary-windows\RUN_SUBSET\0\VS_VERSION\vs2015-x86\label\win2008r2\test\parallel\test-async-wrap-getasyncid.js:155:42)
        at Module._compile (module.js:569:30)
        at Object.Module._extensions..js (module.js:580:10)
        at Module.load (module.js:503:32)
        at tryModuleLoad (module.js:466:12)
        at Function.Module._load (module.js:458:3)
        at Function.Module.runMain (module.js:605:10)
        at startup (bootstrap_node.js:144:16)
        at bootstrap_node.js:561:3
    (node:5140) [DEP0064] DeprecationWarning: tls.createSecurePair() is deprecated. Please use tls.Socket instead.
  ...
@XadillaX

This comment has been minimized.

Show comment
Hide comment
@XadillaX

XadillaX May 18, 2017

Member

I don't know why, maybe it's the bug of CI? I'm not sure. @addaleax

Member

XadillaX commented May 18, 2017

I don't know why, maybe it's the bug of CI? I'm not sure. @addaleax

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax May 18, 2017

Member

Yes, the CI failure is nothing to worry about. 👍 (It’s not a bug in the CI but it’s a known failure that’s not related to this PR.)

Member

addaleax commented May 18, 2017

Yes, the CI failure is nothing to worry about. 👍 (It’s not a bug in the CI but it’s a known failure that’s not related to this PR.)

@XadillaX

This comment has been minimized.

Show comment
Hide comment
@XadillaX

XadillaX May 19, 2017

Member

Hey @addaleax I've already rebased for some conflicts with master.

Member

XadillaX commented May 19, 2017

Hey @addaleax I've already rebased for some conflicts with master.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax May 19, 2017

Member

CI is green. Maybe @bnoordhuis or @silverwind could give this a look, too?

Member

addaleax commented May 19, 2017

CI is green. Maybe @bnoordhuis or @silverwind could give this a look, too?

@bnoordhuis

LGTM modulo comments.

Show outdated Hide outdated src/cares_wrap.cc
ns_c_in,
ns_t_a,
Callback,
GetQueryArg());

This comment has been minimized.

@bnoordhuis

bnoordhuis May 19, 2017

Member

This fits on one line now.

@bnoordhuis

bnoordhuis May 19, 2017

Member

This fits on one line now.

Show outdated Hide outdated src/cares_wrap.cc
ns_c_in,
ns_t_aaaa,
Callback,
GetQueryArg());

This comment has been minimized.

@bnoordhuis

bnoordhuis May 19, 2017

Member

Likewise. I'll stop pointing it out from here on.

@bnoordhuis

bnoordhuis May 19, 2017

Member

Likewise. I'll stop pointing it out from here on.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax May 19, 2017

Member

@XadillaX Actually, one more thing: your author name in this commit is given as “XadillaX”. Is that intended or do you prefer to be listed (changelog, git log, AUTHORS file) with some other name? People typically prefer their full name, but ultimately it’s up to you. (Edit: Just to be clear, that name doesn’t have to be ASCII-only – whatever you prefer, really.)

Member

addaleax commented May 19, 2017

@XadillaX Actually, one more thing: your author name in this commit is given as “XadillaX”. Is that intended or do you prefer to be listed (changelog, git log, AUTHORS file) with some other name? People typically prefer their full name, but ultimately it’s up to you. (Edit: Just to be clear, that name doesn’t have to be ASCII-only – whatever you prefer, really.)

dns: fix `resolve` failed starts without network
Fix the bug that you start process without network at first, but it
connected lately, `dns.resolve` will stay failed with ECONNREFUSED
because c-ares servers fallback to 127.0.0.1 at the very beginning.

If c-ares servers "127.0.0.1" is detected and its not set by user self,
and last query is not OK, recreating `ares_channel` operation will be
triggered to reload servers.

Fixes: #1644
@XadillaX

This comment has been minimized.

Show comment
Hide comment
@XadillaX

XadillaX May 20, 2017

Member

@addaleax I think 'XadillaX - Khaidi Chu' would be fine to me.

Member

XadillaX commented May 20, 2017

@addaleax I think 'XadillaX - Khaidi Chu' would be fine to me.

@XadillaX

This comment has been minimized.

Show comment
Hide comment
@XadillaX

XadillaX May 21, 2017

Member

@addaleax I mean I'd like to use XadillaX when in git log, changelog and authors file. And if someday I have the honor of being a collaborator, I'd like to use 'XadillaX - Khaidi Chu' in the list.

Member

XadillaX commented May 21, 2017

@addaleax I mean I'd like to use XadillaX when in git log, changelog and authors file. And if someday I have the honor of being a collaborator, I'd like to use 'XadillaX - Khaidi Chu' in the list.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax May 21, 2017

Member

Landed in 2b54147

@XadillaX I think you’ve already figured out that you can set this information using git config [--global] user.name and … user.email, but if you need any help, just let us know. :)

Member

addaleax commented May 21, 2017

Landed in 2b54147

@XadillaX I think you’ve already figured out that you can set this information using git config [--global] user.name and … user.email, but if you need any help, just let us know. :)

@addaleax addaleax closed this May 21, 2017

addaleax added a commit that referenced this pull request May 21, 2017

dns: fix `resolve` failed starts without network
Fix the bug that you start process without network at first, but it
connected lately, `dns.resolve` will stay failed with ECONNREFUSED
because c-ares servers fallback to 127.0.0.1 at the very beginning.

If c-ares servers "127.0.0.1" is detected and its not set by user self,
and last query is not OK, recreating `ares_channel` operation will be
triggered to reload servers.

Fixes: #1644
PR-URL: #13076
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@XadillaX

This comment has been minimized.

Show comment
Hide comment
@XadillaX

XadillaX May 21, 2017

Member

@addaleax Thanks. I wondered if this PR passed the coverity? Shall I trigger that by myself?

Member

XadillaX commented May 21, 2017

@addaleax Thanks. I wondered if this PR passed the coverity? Shall I trigger that by myself?

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax May 21, 2017

Member

@XadillaX I am not sure how to do that. I think the tool gets run periodically without being triggered, but really, I’m not sure.

Member

addaleax commented May 21, 2017

@XadillaX I am not sure how to do that. I think the tool gets run periodically without being triggered, but really, I’m not sure.

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn May 22, 2017

Member

@addaleax Thanks. I wondered if this PR passed the coverity? Shall I trigger that by myself?

see #13117 (comment)

Member

gibfahn commented May 22, 2017

@addaleax Thanks. I wondered if this PR passed the coverity? Shall I trigger that by myself?

see #13117 (comment)

@jasnell jasnell referenced this pull request May 28, 2017

Closed

8.0.0 Release Proposal #12220

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Jul 17, 2017

Member

Should this be backported to v6.x? If so it will require to be done manually

Member

MylesBorins commented Jul 17, 2017

Should this be backported to v6.x? If so it will require to be done manually

XadillaX added a commit to XadillaX/node that referenced this pull request Jul 23, 2017

dns: fix `resolve` failed starts without network
Fix the bug that you start process without network at first, but it
connected lately, `dns.resolve` will stay failed with ECONNREFUSED
because c-ares servers fallback to 127.0.0.1 at the very beginning.

If c-ares servers "127.0.0.1" is detected and its not set by user self,
and last query is not OK, recreating `ares_channel` operation will be
triggered to reload servers.

Fixes: nodejs#1644
PR-URL: nodejs#13076
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@XadillaX

This comment has been minimized.

Show comment
Hide comment
@XadillaX

XadillaX Jul 23, 2017

Member

Should this be backported to v6.x? If so it will require to be done manually

#14434

Member

XadillaX commented Jul 23, 2017

Should this be backported to v6.x? If so it will require to be done manually

#14434

MylesBorins added a commit that referenced this pull request Aug 14, 2017

dns: fix `resolve` failed starts without network
Fix the bug that you start process without network at first, but it
connected lately, `dns.resolve` will stay failed with ECONNREFUSED
because c-ares servers fallback to 127.0.0.1 at the very beginning.

If c-ares servers "127.0.0.1" is detected and its not set by user self,
and last query is not OK, recreating `ares_channel` operation will be
triggered to reload servers.

Fixes: #1644
Backport-PR-URL: #14434
PR-URL: #13076
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins added a commit that referenced this pull request Aug 14, 2017

dns: fix `resolve` failed starts without network
Fix the bug that you start process without network at first, but it
connected lately, `dns.resolve` will stay failed with ECONNREFUSED
because c-ares servers fallback to 127.0.0.1 at the very beginning.

If c-ares servers "127.0.0.1" is detected and its not set by user self,
and last query is not OK, recreating `ares_channel` operation will be
triggered to reload servers.

Fixes: #1644
Backport-PR-URL: #14434
PR-URL: #13076
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins added a commit that referenced this pull request Aug 16, 2017

dns: fix `resolve` failed starts without network
Fix the bug that you start process without network at first, but it
connected lately, `dns.resolve` will stay failed with ECONNREFUSED
because c-ares servers fallback to 127.0.0.1 at the very beginning.

If c-ares servers "127.0.0.1" is detected and its not set by user self,
and last query is not OK, recreating `ares_channel` operation will be
triggered to reload servers.

Fixes: #1644
Backport-PR-URL: #14434
PR-URL: #13076
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

@MylesBorins MylesBorins referenced this pull request Aug 16, 2017

Merged

v6.11.3-proposal #14852

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