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

dns: refactor dns to more modern JavaScript #5762

Closed
wants to merge 11 commits into from
63 changes: 31 additions & 32 deletions lib/dns.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
'use strict';

const net = require('net');
const util = require('util');

const cares = process.binding('cares_wrap');
Expand All @@ -11,7 +10,7 @@ const GetAddrInfoReqWrap = cares.GetAddrInfoReqWrap;
const GetNameInfoReqWrap = cares.GetNameInfoReqWrap;
const QueryReqWrap = cares.QueryReqWrap;

const isIp = net.isIP;
const isIp = cares.isIP;
const isLegalPort = internalNet.isLegalPort;


Expand All @@ -38,6 +37,9 @@ function errnoException(err, syscall, hostname) {
return ex;
}

function validIpVersion(isIpResult) {
return isIpResult !== 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO I don't think it's necessary to extract a conditional like this into its own function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mscdex I would agree- but then it would certainly need a comment to explain it. I would not think "validIpVersion" from isIpResult !== 0

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to suggestions here - it wasn't clear to me when I read this file what isIp returns and I had to read the C++ to realize that it returns 4 for ipv4 6 for ipv6 and 0 for "parsing failed".

The goal was to make the fact that 0 is used for "false" here explicit. I don't mind inlining those checked if you'd like but I think that this helps clarify things - I'm open to other approaches too - what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation for net.isIP() describes the valid return values (including 0), so it shouldn't be private knowledge.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point, I still think it helps the code quality but if there is no consensus on that fact I'll do !== 0 since it's less objectionable and more like the old behavior.

}

// c-ares invokes a callback either synchronously or asynchronously,
// but the dns API should always invoke a callback asynchronously.
Expand All @@ -62,7 +64,7 @@ function makeAsync(callback) {
return function asyncCallback() {
if (asyncCallback.immediately) {
// The API already returned, we can invoke the callback immediately.
callback(...arguments);
callback.apply(null, arguments);
} else {
var args = new Array(arguments.length + 1);
args[0] = callback;
Expand Down Expand Up @@ -95,13 +97,13 @@ function onlookupall(err, addresses) {
return;
}

const results = Array(addresses.length);
const results = new Array(addresses.length);

for (var i = 0; i < addresses.length; i++) {
results.push({
results[i] = {
address: addresses[i],
family: this.family || (addresses[i].indexOf(':') >= 0 ? 6 : 4)
});
};
}

this.callback(null, results);
Expand Down Expand Up @@ -182,7 +184,8 @@ exports.lookup = function lookup(hostname, options, callback) {

function onlookupservice(err, host, service) {
if (err) {
return this.callback(errnoException(err, 'getnameinfo', this.host));
this.callback(errnoException(err, 'getnameinfo', this.host));
return;
}

this.callback(null, host, service);
Expand All @@ -192,10 +195,10 @@ function onlookupservice(err, host, service) {
// lookupService(address, port, callback)
exports.lookupService = function(host, port, callback) {
if (arguments.length !== 3) {
throw new Error('Invalid arguments');
throw new TypeError(`Expected 3 arguments, got ${arguments.length}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't really a TypeError per se, since it is more to do with argument count rather than argument type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the type of lookupService is a function that takes three arguments and returns its results - if you pass an incorrect number of arguments to it then you're misusing the type.

For example - when you call setTimeout with 0 arguments in browsers you get a TypeError. When you call Array#reduce with 1 argument on an empty array you get a TypeError about the missing argument, and so on.

I don't feel very strongly about this - but the language and most APIs I know throw type errors in this case.

}

if (cares.isIP(host) === 0) {
if (isIP(host) === 0) {
throw new TypeError('"host" argument needs to be a valid IP address');
}

Expand Down Expand Up @@ -236,9 +239,10 @@ function resolver(bindingName) {

return function query(name, callback) {
if (typeof name !== 'string') {
throw new Error('"name" argument must be a string');
const msg = `"name" argument must be a string, got ${typeof name}`;
throw new TypeError(msg);
} else if (typeof callback !== 'function') {
throw new Error('"callback" argument must be a function');
throw new TypeError('"callback" argument must be a function');
}

callback = makeAsync(callback);
Expand Down Expand Up @@ -272,7 +276,7 @@ exports.resolveSoa = resolveMap.SOA = resolver('querySoa');
exports.reverse = resolver('getHostByAddr');


exports.resolve = function(hostname, type_, callback_) {
exports.resolve = (hostname, type_, callback_) => {
var resolver, callback;
if (typeof type_ === 'string') {
resolver = resolveMap[type_];
Expand All @@ -292,42 +296,37 @@ exports.resolve = function(hostname, type_, callback_) {
};


exports.getServers = function() {
exports.getServers = () => {
return cares.getServers();
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exports.getServers = () => cares.getServers();

Are the curlys required by coding convention here? ::shrugs::

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they're required in multi-line functions. I recall talking about it but I'm not sure what conclusion was reached and I'm fine with either - @thefourtheye ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember the discussion regarding the parens in arrow function parameters, but not sure about the body. I'll have to look. But I feel that having curly braces for even a single expression kinda beats the purpose of arrow functions.



exports.setServers = function(servers) {
// cache the original servers because in the event of an error setting the
// servers cares won't have any servers available for resolution
var orig = cares.getServers();

var newSet = [];
var originalServers = cares.getServers();

servers.forEach(function(serv) {
var ver = isIp(serv);
const newSet = servers.map((serv) => {
var ipVersion = isIp(serv);

if (ver) {
newSet.push([ver, serv]);
return;
if (validIpVersion(ipVersion)) {
return [ver, serv];
}
var match = serv.match(/\[(.*)\](:\d+)?/);
const match = serv.match(/\[(.*)\](:\d+)?/);

// we have an IPv6 in brackets
if (match) {
ver = isIp(match[1]);
if (ver) {
newSet.push([ver, match[1]]);
return;
ipVersion = isIp(match[1]);
if (validIpVersion(ipVersion)) {
return [ver, match[1]];
}
}

var s = serv.split(/:\d+$/)[0];
ver = isIp(s);
ipVersion = isIp(s);

if (ver) {
newSet.push([ver, s]);
return;
if (validIpVersion(ipVersion)) {
return [ver, s];
}

throw new Error(`IP address is not properly formatted: ${serv}`);
Expand All @@ -337,7 +336,7 @@ exports.setServers = function(servers) {

if (errorNumber !== 0) {
// reset the servers to the old servers, because ares probably unset them
cares.setServers(orig.join(','));
cares.setServers(originalServers.join(','));

var err = cares.strerror(r);
throw new Error(`c-ares failed to set servers: ${err} [${servers}]`);
Expand Down