-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add option for DNS resolving #306
Comments
@bminer any reason not to simply adopt the improved mechanisms by default? Do they require additional dependencies? What exactly are the tangible benefits (ie compared benchmarks) of adopting dns.resolve? It begs the question: If there really are substantial improvements would node not adopt the new functionality eventually? Raising the final and ultimate question: Is this not out of scope for superagent? |
@nickL - Good points. I think you're right -- this is outside of the scope for the Superagent project and is solely a Node.js issue. If you don't mind, I'd like to close this issue. Thanks! EDIT: Just to add on... Should Node provide a nice API in the future to change which resolver is used for an HTTP request, perhaps it would be appropriate to expose that option via Superagent. Until then, I vote to close this issue. |
@bminer Just a thought... would something like this not accomplish the same. dns = require('dns');
dns.lookup = function (domain, family, callback) {
if (family = 6)
return dns.resolve(domain, 'AAAA', callback);
else
return dns.resolve(domain, 'A', callback);
} After doing some quick tests against the above it appears that
|
Benchmarking the Environment: results looking up github.com for 100 000 iterations:
Conclusion: It is clear that Changing the superagent implementation to Node console output by which results were retrieved if you would like to reproduce or audit these findings. > dns = require('dns');
> function resolve_bench(d) {
... console.time('resolve-lock');
... console.time('resolve');
... a=0;
... for (i=0;i<=100000;i++)
... dns.resolve(d,'A',function () {if (++a == 100000) console.timeEnd('resolve'); });
... console.timeEnd('resolve-lock');
... }
> resolve_bench('github.com')
resolve-lock: 3946ms
resolve: 349471ms
> function lookup_bench(d) {
... console.time('lookup-lock');
... console.time('lookup');
... a=0;
... for (i=0;i<=100000;i++)
... dns.lookup(d,4,function () {if (++a == 100000) console.timeEnd('lookup'); });
... console.timeEnd('lookup-lock');
... }
> lookup_bench('github.com')
lookup-lock: 619ms
lookup: 18256ms
> function resolve4_bench(d) {
... console.time('resolve4-lock');
... console.time('resolve4');
... a=0;
... for (i=0;i<=100000;i++)
... dns.resolve4(d,function () {if (++a == 100000) console.timeEnd('resolve4'); });
... console.timeEnd('resolve4');
... }
> resolve4_bench('github.com');
resolve4: 3688ms
resolve4: 357464ms |
@nickl- Superagent is already using |
@bminer ahh yes I was looking at the link you posted for the fetch library I see =) |
There should be an option to change how DNS resolution is performed when issuing an HTTP request. By default, Node uses
dns.lookup
, which usesgetaddrinfo(3)
in a thread pool. On the other hand,dns.resolve
uses C-Ares, which is MUCH faster and does not use the thread pool.For reference, here is the patch for the fetch library: https://github.com/andris9/fetch/blob/2c10a54ccc749453f5446cf07bfbb45a5d1856cd/lib/fetch.js#L257-L274
There is currently no Node API for this, but it may be coming soon. See nodejs/node-v0.x-archive#2868 for details.
The text was updated successfully, but these errors were encountered: