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.

Sign up for GitHub

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
Closed

dns: refactor dns to more modern JavaScript #5762

Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
8d6c22e
initial work - normalize if/else style
benjamingr Mar 17, 2016
f5c4f55
more template strings and less abused returns
benjamingr Mar 17, 2016
2784847
block over for loop, spread arguments from apply
benjamingr Mar 17, 2016
48fde13
address comments
benjamingr Mar 17, 2016
bfe0f40
address comments from mscdex
benjamingr Mar 17, 2016
2d54ae9
refactor forEach to map
benjamingr Mar 17, 2016
85f9b87
var to const
benjamingr Mar 17, 2016
457de39
removed references to net module
benjamingr Mar 17, 2016
4f124f7
changed {} to Object.create(null) to prevent __proto__ meddling
benjamingr Mar 17, 2016
6fe93ed
remove isValidIp and check for 0 instead
benjamingr Mar 17, 2016
b3ced41
silly mistake missing $
benjamingr Mar 17, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 67 additions & 51 deletions lib/dns.js
View file
Open in desktop
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 @@ -25,7 +24,7 @@ function errnoException(err, syscall, hostname) {
}
var ex = null;
if (typeof err === 'string') { // c-ares error code.
ex = new Error(syscall + ' ' + err + (hostname ? ' ' + hostname : ''));
ex = new Error(`${syscall} ${err} ${(hostname ? ' ' + hostname : '')}`);
ex.code = err;
ex.errno = err;
ex.syscall = syscall;
Expand All @@ -38,7 +37,6 @@ function errnoException(err, syscall, hostname) {
return ex;
}


// c-ares invokes a callback either synchronously or asynchronously,
// but the dns API should always invoke a callback asynchronously.
//
Expand Down Expand Up @@ -66,8 +64,9 @@ function makeAsync(callback) {
} else {
var args = new Array(arguments.length + 1);
args[0] = callback;
for (var i = 1, a = 0; a < arguments.length; ++i, ++a)
for (var i = 1, a = 0; a < arguments.length; ++i, ++a) {
args[i] = arguments[a];
}
Copy link
Member

Choose a reason for hiding this comment

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

nit: not in love with the style changes

process.nextTick.apply(null, args);
}
};
Expand All @@ -76,8 +75,10 @@ function makeAsync(callback) {

function onlookup(err, addresses) {
if (err) {
return this.callback(errnoException(err, 'getaddrinfo', this.hostname));
this.callback(errnoException(err, 'getaddrinfo', this.hostname));
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

why change this? the return callback on errors is a commonly used pattern.


if (this.family) {
this.callback(null, addresses[0], this.family);
} else {
Expand All @@ -87,16 +88,18 @@ function onlookup(err, addresses) {


function onlookupall(err, addresses) {
var results = [];
if (err) {
return this.callback(errnoException(err, 'getaddrinfo', this.hostname));
this.callback(errnoException(err, 'getaddrinfo', this.hostname));
return;
}

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 @@ -134,8 +137,10 @@ exports.lookup = function lookup(hostname, options, callback) {
family = options >>> 0;
}

if (family !== 0 && family !== 4 && family !== 6)
throw new TypeError('Invalid argument: family must be 4 or 6');
if (family !== 0 && family !== 4 && family !== 6) {
const msg = `Invalid argument: family must be 4 or 6, got {family}`;
throw new TypeError(msg);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the $ for the template string went missing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks :)


callback = makeAsync(callback);

Expand All @@ -148,7 +153,7 @@ exports.lookup = function lookup(hostname, options, callback) {
return {};
}

var matchedFamily = net.isIP(hostname);
var matchedFamily = isIP(hostname);
if (matchedFamily) {
if (all) {
callback(null, [{address: hostname, family: matchedFamily}]);
Expand Down Expand Up @@ -176,25 +181,30 @@ exports.lookup = function lookup(hostname, options, callback) {


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

this.callback(null, host, service);
}


// lookupService(address, port, callback)
exports.lookupService = function(host, port, callback) {
if (arguments.length !== 3)
throw new Error('Invalid arguments');
if (arguments.length !== 3) {
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');
}

if (port == null || !isLegalPort(port))
if (port == null || !isLegalPort(port)) {
throw new TypeError(`"port" should be >= 0 and < 65536, got "${port}"`);
}

port = +port;
port = Number(port);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

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, unary + and Number do the same thing but Number is more explicit and obvious. Not everyone is familiar with things like this so when there is no doubt about performance issues I'd rather refactor to cleaner code.

I'm also not sure about the >>>s to convert to integer numbers. We use them for the options object in this file and I wonder if I shouldn't convert them to Math.floor since it's more obvious and it's not a performance sensitive area.

callback = makeAsync(callback);

var req = new GetNameInfoReqWrap();
Expand All @@ -204,18 +214,21 @@ exports.lookupService = function(host, port, callback) {
req.oncomplete = onlookupservice;

var err = cares.getnameinfo(req, host, port);
if (err) throw errnoException(err, 'getnameinfo', host);
if (err) {
throw errnoException(err, 'getnameinfo', host);
}

callback.immediately = true;
return req;
};


function onresolve(err, result) {
if (err)
if (err) {
this.callback(errnoException(err, this.bindingName, this.hostname));
else
} else {
this.callback(null, result);
}
}


Expand All @@ -224,9 +237,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 All @@ -236,14 +250,17 @@ function resolver(bindingName) {
req.hostname = name;
req.oncomplete = onresolve;
var err = binding(req, name);
if (err) throw errnoException(err, bindingName);
if (err) {
throw errnoException(err, bindingName);
}

callback.immediately = true;
return req;
};
}


var resolveMap = {};
var resolveMap = Object.create(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question, we're using this object for a map, I thought about converting it to an ES2015 map but wasn't 100% sure about the implications or performance. Object.create(null) means that it can't be used as something other than a map - for example this guards against __proto__ being passed as the second argument to dns.resolve which currently I assume is pretty harmless - but I'm not sure.

With your blessing I'll refactor this to a Map gladly. Although that would make the above export lines double in size since it can be one-lined.

exports.resolve4 = resolveMap.A = resolver('queryA');
exports.resolve6 = resolveMap.AAAA = resolver('queryAaaa');
exports.resolveCname = resolveMap.CNAME = resolver('queryCname');
Expand All @@ -257,7 +274,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 @@ -272,56 +289,55 @@ exports.resolve = function(hostname, type_, callback_) {
if (typeof resolver === 'function') {
return resolver(hostname, callback);
} else {
throw new Error('Unknown type "' + type_ + '"');
throw new Error(`Unknown type ${type_}`);
}
};


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 originalServers = cares.getServers();

var newSet = [];
const newSet = servers.map((serv) => {
var ipVersion = isIp(serv);

servers.forEach(function(serv) {
var ver = isIp(serv);

if (ver)
return newSet.push([ver, serv]);

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

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

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

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

throw new Error('IP address is not properly formatted: ' + serv);
throw new Error(`IP address is not properly formatted: ${serv}`);
});

var r = cares.setServers(newSet);
const errorNumber = cares.setServers(newSet);

if (r) {
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 + ']');
throw new Error(`c-ares failed to set servers: ${err} [${servers}]`);
}
};

Expand Down