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

How to add max timeout on DNS query? #7231

Closed
yhojann-cl opened this issue Jun 8, 2016 · 18 comments
Closed

How to add max timeout on DNS query? #7231

yhojann-cl opened this issue Jun 8, 2016 · 18 comments
Assignees
Labels
dns Issues and PRs related to the dns subsystem. feature request Issues that request new features to be added to Node.js.

Comments

@yhojann-cl
Copy link

For example, the request a MX query is a very very long response, how to set by example 8 seconds to stop connection and launch the error callback?

@mscdex mscdex added question Issues that look for answers. dns Issues and PRs related to the dns subsystem. labels Jun 8, 2016
@silverwind
Copy link
Contributor

Good question. I don't think there's any timeout control exposed in dns yet, which looks like an oversight. We should evaluate adding it.

As for a possible implementation, instead of adding another static method (like the ill-conceived dns.setServers), I'd like to see a timeout added to option objects of lookup and resolve* (need to add object parameter support).

@silverwind silverwind added feature request Issues that request new features to be added to Node.js. and removed question Issues that look for answers. labels Jun 9, 2016
@bnoordhuis
Copy link
Member

dns.lookup() calls getaddrinfo(3) under the hood, there is no real way to cancel that once started.

The other dns functions use c-ares. It has a configurable timeout but unfortunately that is effectively a per-process setting. It can be configured on a per-channel basis but node uses only one channel for efficiency reasons.

@yhojann-cl
Copy link
Author

http://stackoverflow.com/questions/10777657/node-js-dns-lookup-how-to-set-timeout

The stop async thread realy stop the parent process?

fengmk2 added a commit to alibaba-archive/failover-dns that referenced this issue Sep 26, 2016
fengmk2 added a commit to alibaba-archive/failover-dns that referenced this issue Sep 26, 2016
dead-horse pushed a commit to alibaba-archive/failover-dns that referenced this issue Sep 27, 2016
* feat: support lookup with timeout

nodejs/node#7231

* refactor: add meaningful error name

* feat: allow to set default query timeout
@geoffreak
Copy link

Any chance of this getting added? It seems like such a simple feature to wait this long.

@silverwind
Copy link
Contributor

@geoffreak try https://github.com/mafintosh/dns-socket.

@Trott
Copy link
Member

Trott commented Jul 26, 2017

It seems like this is a feature request that is unlikely to be implemented, at least not without changes in upstream c-ares. I'm going to close this, but please comment (or re-open) if you think that it should stay open.

@Trott Trott closed this as completed Jul 26, 2017
@realJoshByrnes
Copy link

It's certainly something that should be in any DNS lib.

I'm getting requests to implement a timeout as some lookups are taking 30s+
https://www.npmjs.com/package/fcrdns

@bnoordhuis
Copy link
Member

As a workaround, start a timer in parallel. If the timer expires before you get an answer back, tell the caller it timed out and ignore the answer once it does come in.

@realJoshByrnes
Copy link

Agreed that it seems this is the only other option, but I'm under the impression it will still hold up any future lookups as it's not actually stopping.

eg. #8436

@bnoordhuis
Copy link
Member

Yes, that's true for dns.lookup(); it calls into the C runtime library from the thread pool. dns.resolve() and friends use minimal resources though; only a few bytes of memory.

@ikokostya
Copy link

If the timer expires before you get an answer back, tell the caller it timed out and ignore the answer once it does come in.

As I can understand с-ares allows cancel requests: https://c-ares.haxx.se/ares_cancel.html. But this API isn't available in Node.js.

Have you any plans for support cancellation for dns.resolve* functions?

@XadillaX
Copy link
Contributor

@ikokostya I think I can try to work for this feature

@Trott Trott reopened this Jul 26, 2017
@bnoordhuis
Copy link
Member

ares_cancel() cancels all outstanding requests, there is no way to cancel a single one.

Giving a request its own channel with ares_dup() is an option, but probably not a good option. It has high overhead but what's more, since it rescans /etc/resolv.conf etc., it adds a whole new set of failure modes if the configuration changed since the application was started.

@addaleax
Copy link
Member

@bnoordhuis @XadillaX I’ve been thinking it might make sense to expose ways to manage c-ares channels manually from JS, basically just to reduce the global state that’s inherent in having a single one per environment (then making dns.setServers just replace the default one). And now it sounds like this issue is something where that would actually help solve a problem; what do you think?

@bnoordhuis
Copy link
Member

Not opposed but depends on what the API would look like. Too much direct exposure of implementation details would make it hard to move away from c-ares if we ever needed to do that.

@XadillaX
Copy link
Contributor

@addaleax I wonder how the performance it will be if we use a single channel (it means we ares_init for each) for each request?

@bnoordhuis
Copy link
Member

@XadillaX Bad, most likely. It does a lot of things: parsing config files, parsing environment variables, and so on.

It would be a very nice solution if it weren't for the overhead. Maybe someone can look into adding a less expensive version of ares_dup() upstream. ares_clone(), anyone?

@addaleax
Copy link
Member

PR for my suggestion: #14518

addaleax added a commit to addaleax/node that referenced this issue Aug 1, 2017
addaleax added a commit to addaleax/node that referenced this issue Aug 1, 2017
This can be used to implement custom timeouts.

Fixes: nodejs#7231
addaleax added a commit that referenced this issue Aug 1, 2017
Ref: #7231
PR-URL: #14518
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
addaleax added a commit that referenced this issue Aug 1, 2017
Ref: #7231
PR-URL: #14518
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
addaleax added a commit that referenced this issue Aug 1, 2017
This can be used to implement custom timeouts.

Fixes: #7231
PR-URL: #14518
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dns Issues and PRs related to the dns subsystem. feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

No branches or pull requests

10 participants