Replace deps/cares with pure-javascript alternative

[jbergstroem]

This has been brought up a time or two, so I just thought I'd get a discussion going whether we actually want to do this.

We've somewhat moved away from upstream c-ares. We're not abi compatible any longer which made us remove supporting building against a shared library. The way I see it; we could avoid complexity and c++ by replacing c-ares with a pure-javascript alternative.

The most likely contender for this spot would be node-dns by @tjfontaine which is meant to be a drop-in replacement. It has additional functionality such as a simple dns server which we may (or likely not) be interested in adopting. It has a few dependencies of it's own I'm unsure we'd want to have maintained externally. There's also been a few issues mentioned that needs to be resolved [todo: be more specific].

Before putting too much work in this, are there counter-arguments against such a transition -- or should we perhaps turn it around; why move at all? My main arguments would be that the benefits of c++ doesn't give us much here, as well as not having to stay close to c-ares (for good or bad). It would probably also allow more people to chip in in terms of maintenance.

[bnoordhuis]

I'm in favor of moving to (something like) node-dns.

It has a few dependencies of it's own I'm unsure we'd want to have maintained externally.

I'm guessing 'no'. The dependency chain is a few layers deep and some things seem to be more convenient than optimal.

There's also been a few issues mentioned that needs to be resolved [todo: be more specific].

node-dns doesn't support fallback to TCP, like c-ares does. Probably not an issue in everyday use for most.

It would probably also allow more people to chip in in terms of maintenance.

I'd say that's the best argument for moving away from c-ares.

[mscdex]

+1

[tellnes]

+1

[tjfontaine]

I will provide a better write up on if I think as it exists today if node-dns should be included. I do want to quickly clarify that it does do both tcp and udp lookups.

[silverwind]

I'd definitely be in favor of a javascript implementation that exposes more than resolve does right now. With node-dns, I'm able to access the socket through answer._socket which for example allows me to check the source address of the answer packet.

I have to caution you of dropping in node-dns as-is though. I encountered a throw in async code, which is a no-go, and I think there could be more surprises in there.

[rvagg]

Perf implications?

[tjfontaine]

For hysterical^Whistorical context, allow me to describe why I wrote node-dns/native-dns.

At the time I was fed up with a DNS server I was having to maintain that was written in twisted. Having recently fell in love with Node.js I embarked on writing a DNS application server in JavaScript. I started by re-using someone else's module, until I found a bug and realized that (despite what people were distributing it as on npm) the original source of the module was actually unlicensed. So I went about the work to write my own DNS parser and server, and it was evident the same code could be reusable as a client stack as well. At this point I decided I wanted to it to be a drop in replacement for the dns module found in Node.js, but written in pure JavaScript.

That's the lens you should look at native-dns with (now maintained by @taoeffect [though I owe him some code reviews]). A module that initially wanted to enable application server style workflows (like the http module, but for DNS), that grew a client implementation.

Now to ask the question if I, a maintainer of Node.js, would accept the module I wrote in core? As it stands today, no.

There are far too many places native-dns overreaches in scope for what I would consider to be included in core:

• native-dns.lookup
  • For a drop in replacement, it's important to have this -- but it does not actually implement the interface it claims to.
  • This should have fidelity with the actual system resolver, that is the one maintained by libc, and it's unlikely that you as a user program will be able to know enough about the plumbing of the system to be able to provide that interface. (i.e. it's harder than just reading /etc/nsswitch.conf)
  • It is attractive though to implement, because it takes pressure off the thread pool, but solving that concern should be left to non-core modules.
• DNS server
  • The way this works today in native-dns is too heavy handed for what I think is worthwhile for core.
  • The measuring stick for adding features/APIs to Node.js is often one that asks if there's another module that needs or would benefit from this API. And there's really not something that core needs for this.
  • Though enabling reuse of the code (like the parser etc) would be ideal such that it would be trivial for someone to write their own non-core module to do DNS serving.
• DNS caching
  • This is a trap, completely and utterly a trap, leave this to the forwarders they're going to be way better at it than you.
• Technical debt
  • Unfortunately for @taoeffect and as @silverwind mentioned, this module has warts that come from it growing with me as I grew in Node.js, much of the internals here should be redone.

Now on to the pieces of the API for native-dns that I think are worthwhile.

• native-dns.Request
  • A single shot API to a specific DNS server, you specify up front TCP/UDP (this may be what @bnoordhuis was referring to not having a fallback)
• native-dns.Resolve
  • Behaves like dns.resolve but you're able to pass in per question the list of DNS servers, timeouts, etc

Things that fall out of this implementation that I find neat.

• Full access to the resource records for every result, maybe you want to know TTLs or whatever else, you finally have access to that, it's not locked away in c-ares.
• Observable/Cancelable requests, the handle that's returned by dns.resolve lets you debug how far along you were with a request, and even "cancel" it (where cancel in DNS land mostly involves removing it from the hash table of valid response IDs)

Random thoughts I have on this:

• Moving to a pure JavaScript implementation means that the core can be reused and embedded in other platforms, and the DNS resolution behaves the same across implementations.
• Other platforms who embed the core JavaScript would probably prefer to have their own DNS stacks manage resolution so its consistent on their platform
• Does the core really need to be in the business of maintaining a DNS resolver?
• native-dns doesn't currently do DNSSec, if you're going to be maintaining DNS in core do you want to have to do that work as well?
• Aside from ABI compatibility concerns, what do you want to gain/enable by replacing the DNS implementation (regardless of who/what implements it).

Many of the things I like about native-dns could probably find ways of being achieved with other implementations, or even the current c-ares implementation. It's just a matter of understanding what it is you're trying to solve for by doing the work.

If you decide you want core to have a pure JavaScript implementation, I would advocate for working from an existing code base that can be morphed into the thing you're looking for. I do believe that native-dns with @taoeffect is a good place to coordinate that effort. That being said, I also know @indutny has a DNS parser that he wrote, and there are a myriad of others that can be found in our ecosystem.

Food for thought.

[piscisaureus]

In general, replacing cares by an all-js implementation is is an idea worth exploring IMO.

My preference would be to keep only GetAddrInfo/GetNameInfo (the APIs that net and http rely on) in core, and move the more advanced dns methods to userland. What we need is a transition path.

DNS caching
This is a trap, completely and utterly a trap, leave this to the forwarders they're going to be way better at it than you.

c-ares afaik doesn't do any caching. I've always seen this as problematic; using node-dns might be a way to resolve this long-standing issue.
But I will take @tjfontaine's word for it that it's challenging to get it right.

[tjfontaine]

I think the desire for caching mostly stems from use of dns.lookup and not dns.resolve. And the desire for caching that is to avoid saturating the thread pool with unnecessary work and reducing the amount of C/C++ roundtrips that core is doing.

If we contain the conversation to only dns.resolve the issue is pretty much moot, as most resolvers don't do client side caching. Indeed, c-ares does not do caching -- this is mostly left to the forwarders to handle.

If caching were to be considered in scope, there's almost certainly no way for the opinion of core to get it right for what the users needs are. You could do traditional TTL based caching, but the consumer may need more nuanced semantics.

All in all, caching is effectively wrapping http.request or net.connect with your own thing to cache:

// error handling, typos, etc all reserved -- this is a paraphrase.
var cache = {};
function lookup(name, cb) {
  if (name in cache)
    setImmediate(cb, cache[name]);
  else
    dns.lookup(name, function store(err, ip) {
      cache[name] = ip;
      cb(err, ip);
    });
}

function myconnect(port, name, cb) {
  lookup(name, function(err, ip) {
    net.connect(port, ip, cb);
  });
}

Where you can handle your own invalidation, on whatever semantics you want to achieve.

native-dns grew a concept about caching because it's related to wanting to provide the semantics of holding records that need to be resolved on the server side, which in some ways behaves similarly to what you might need for forwarder caching. But all in all, out of scope for what a pure resolver needs to function.

Splitting DNS resolution out of core is an interesting idea, and indeed fits more in line with the needs of core -- but in my opinion that ship has sailed, the API was included and for many people expected to be there and it (currently) is not too much of a burden to continue to maintain. However, shifting to something that core would be seeking to maintain, that burden could indeed feel too great and decide to not do that at all.

[piscisaureus]

If we contain the conversation to only dns.resolve the issue is pretty much moot, as most resolvers don't do client side caching. Indeed, c-ares does not do caching -- this is mostly left to the forwarders to handle.

But c-ares doesn't even expose the TTL that's reported by the DNS server, so implementing sensible custom caching is almost impossible.

Splitting DNS resolution out of core is an interesting idea, and indeed fits more in line with the needs of core -- but in my opinion that ship has sailed, the API was included and for many people expected to be there

That's true, we can't remove the dns module and break people's code. We need a creative solution for the core-to-userland problem however.
I noticed that recently people have begun to suggest semantic changes to many core modules. The most popular targets are util, assert, url and path.
I am reluctant to support modifications to these modules - even if the proposed changes are reasonable - because it'll break applications. I'd me much more comfortable if these modules were versioned independently.
If we were to find a good solution, changes to the dns module could use the same infrastructure.

[silverwind]

What other modules would be worth considering? I only know of @indutny's dns.js which could be a workable base, but it's far from being done last I tried it.

[tellnes]

But c-ares doesn't even expose the TTL that's reported by the DNS server, so implementing sensible custom caching is almost impossible.

Exposing the TTL in the api would be nice. That is something I've needed in the past.

[taoeffect]

On the topic of a DNS implementation, have any of you had a chance to evaluate/look at/consider getdns-node? It's node bindings for getdns.

Re: node-dns, it needs a good amount of simplification and cleanup IMO. I especially want to see fast packet relay that avoids parsing packets unless necessary (see issues tjfontaine/node-dns#67 and tjfontaine/node-dns#69).

If the C++ from getdns can be integrated into node, then it might be a reasonable alternative (I still need to evaluate it more though to be sure). While I don't like C++, the advantage is that it seems to be a fairly mature library that's being actively maintained by another group.

cc @silverwind @tjfontaine

[taoeffect]

(Note to email subscribers: I edited the comment I just posted. So visit GitHub to see the changes.)

[silverwind]

@taoeffect I think a C++ implementation is out of question for io.js at this point. There's a huge risk of it ending up in a stale state like c-ares is right now, where only a couple people are able/willing to dive into it to fix issues like #894.

Having a clean and well-tested DNS implementation in JS would probably encourage a lot more people to maintain it, I'm just afraid that it may not yet be written.

[sam-github]

Hoping that there is a pool of people who know DNS and DNSSEC well enough to maintain a pure JS implementation, but don't know C, seems optimistic.

Also, a pure-js DNS library would be used by nothing but iojs, so would be entirely supported by iojs, and be unlikely to have DNSSEC support.

getdns looks like an interesting alternative to c-ares, and it is built to work off an event loop (it even has libuv support), so it might be a more modern and better supported alternative to a pure-js implementation, though I don't know how much pain C-ARES is actually causing. It seems pretty stable.

[silverwind]

Just adding this to the discussion here: dns.setServers's synchronous and global fashion isn't really a good fit for our async resolve methods, but it seems this is how c-ares rolls (See #1071).

Setting per-query options like servers (and possible other future options) should be something to keep in mind if we choose another implementation.

[saghul]

Throwing my 2 cents here :-)

@piscisaureus

But c-ares doesn't even expose the TTL that's reported by the DNS server, so implementing sensible custom caching is almost impossible.

AFAIS, it does report the TTL, but just for A and AAAA. Not ideal, but it's something. https://github.com/piscisaureus/cares/blob/master/src/ares_parse_a_reply.c#L49

@sam-github

I've been following getdns for a while and indeed it does have a nicer API, more features (DNSSEC for one) and it's actively developed. c-ares OTOH: http://c-ares.haxx.se/mail/c-ares-archive-2014-05/0000.shtml

FWIW, one thing that could benefit some people would be the ability to create multiple resolvers (aka ares_channel things) since all options apply to those:

var resolver = new dns.Resolver();
// resolver has the same API as dns, execpt the Resolver constructor
resolver.setServers(...);
// doesn't change the servers in 'dns', just in 'resolver'
resolver.resolve(...)

In addition, lookup / lookupService could be deprecated in favor of getaddrinfo / getnameinfo, since they use the system resolver, and that's what they really are inside. Of course, any instance of Resolver wouldn't have those, they'd be available just in the global dns module.

The Resolver API could be designed in such a way that replacing the guts with getdns or something else doesn't change the user facing API, hopefully.

[silverwind]

The Resolver API could be designed in such a way that replacing the guts with getdns or something else doesn't change the user facing API, hopefully.

Yeah, I'd prefer not to introduce the complexity of having to instantiate a resolver like in your example. I'd prefer adding an option argument to dns.resolve, so it's more in line with the current async API.

TTLs would be a great addition, but I think it's not worth adding the feature when it won't work for every rrtype.

[mscdex]

FWIW I have a pure js dns resolver in the works that is more or less done now (still missing tests and backwards-compatible error objects though). It handles configuration file parsing, dns.ADDRCONFIG behavior, etc. so that no c-ares or getaddrinfo/getnameinfo is needed.

Look for a PR to add this resolver in the near future....

[silverwind]

@mscdex awesome, very eager to review that ;)

[darionyaphet]

+1

[mscdex]

@nodejs/collaborators While working on error compatibility, I saw a comment in dns.js by @bnoordhuis that mentioned wanting to return the actual DNS error code/name instead of some of c-ares' custom error codes/names. For example, if you query a nameserver for a non-existent domain, you will get back NXDOMAIN, but c-ares returns their own ENOTFOUND instead (actually, c-ares uses ENOTFOUND for other situations too, like a successful query that resulted in no answers).

My question is should I be making that switch to real DNS errors (e.g. NXDOMAIN, etc.) for the purposes of my upcoming dns resolver PR? Or should I continue to stick with c-ares' custom error codes/names?

[cjihrig]

Would it be possible to stick with what c-ares does in the short term, to reduce the perceived changes and then possibly transition to "real" DNS over a few major version bumps?

[Fishrock123]

@cjihrig as with the http parser, I think behind a flag and phased in was discussed, maybe that could be done ontop of what you suggested?

[cjihrig]

Sounds good to me 👍

[silverwind]

+1 for real DNS errors. I'd prefer the dns implementation landing in a semver-minor, and then switch directly to the new errors and possible other additions (TTL) in a semver-major.

I don't see much use in a transition period for errors, either we break them or we don't.

[mscdex]

Regarding flags, I currently have it so that the old DNS functionality is behind a flag (--old-dns) as suggested by some on IRC.

I think I agree with @silverwind about the errors though. If they need the old errors they could just use the old DNS functionality? I dunno...

The exposure of TTL and other data is a separate issue and I'm not sure of the best way to make that information (optionally) available.

[silverwind]

--old-dns sounds good, in that case I suggest landing it all along with new errors in a semver-major, given some major testing of course.

The exposure of TTL and other data is a separate issue and I'm not sure of the best way to make that information (optionally) available.

Yeah, that's a bit tricky given the current API. We could possibly return an array of TTLs in a third callback parameter, or go the route of adding something like .resolveTtl().

[jbergstroem]

Very positive towards exposing the real DNS error.

[silverwind]

Most of the official error codes aren't all-caps like our errors, should we still caps them like dig?

[jbergstroem]

@silverwind to avoid case comparison issues wouldn't the error code (rcode) be more beneficial to use [as a user]? That way we can at least say we follow rfc.

[mscdex]

@silverwind The error field in a DNS packet is just a number field, so technically you could use any string name you want. However our DNS module currently uses all caps for its error names. I would probably keep it that same style like dig and many others do as well.

[jasnell]

+1 to continuing to push in this direction. Great to see additional work moving forward

[silverwind]

ref: #1843

[snsparrish]

+1 to get this out asap. This is a big barrier for us using io.js currently.

[Fishrock123]

@snsparrish shouldn't be more of a barrier than any other node version though?

[rvagg]

@snsparrish can you clarify the barrier for us? This is basically about improving on very long-term Node technical debt, not fixing anything specific to io.js. Or are you talking about a barrier to Node in general?

[snsparrish]

We are currently using node-dns on older versions of node.js but that module doesn't appear to function in io.js 2.2.1. We need the ability to query multiple nameservers without the crash mentioned in #894. But as I understand it, even if setServers didn't crash, the resolver "stored in a per-app global state" keeps me from being able to query different nameservers at the same time in the same app.

We can't upgrade to io.js from our current outdated node.js versions without that ability.

[silverwind]

This is correct, right now you must do such queries to multiple servers serially because of the crashing issue. I plan on introducing a async-friendly option to resolve once #1843 has landed, but the PR isn't nearly ready, it has a few unresolved behaviour issues and needs a proper code review.

[Trott]

Applying stalled label to this. Feel free to remove if that's a misuse of the label.

[jasnell]

Given the lack of further activity and discussion, and the lack of any response to adding the stalled label, going to go ahead and close this. It's possible that we'd still want to pursue this but there's no reason to keep this particular issue open.

[royalpinto]

There is an alternate library node-cares available (created by me :-p) which can be used to customize the resolver behavior (like TCP/UDP ports, dns servers, timeouts etc.) link

[foxx]

@piscisaureus

But c-ares doesn't even expose the TTL that's reported by the DNS server, so implementing sensible custom caching is almost impossible.

Uh, yes it does.

[saghul]

Tat function just does some prints, that's not usable. The TTL is only
exposed for a few record types. Someone sent a patch to c-ares at some
point, but it breaks the ABI so alas it was not accepted.

On Oct 15, 2016 20:43, "Cal Leeming" notifications@github.com wrote:

@piscisaureus https://github.com/piscisaureus

But c-ares doesn't even expose the TTL that's reported by the DNS server,
so implementing sensible custom caching is almost impossible.

Uh, yes it does https://github.com/c-ares/c-ares/blob/master/adig.c#L557
.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1013 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AATYGAF50ZRz2ep3iHN8kL8TSw39YZC7ks5q0R7lgaJpZM4Dnpa5
.

[foxx]

Ohh, damn they should make that more clear in the README. You might have just saved me a lot of headache, thank you.

[Luluno01]

It has been 2 or 3 years and I am now experiencing a horrible time compiling/cross compiling node.js for my android device, with bunches of undefined reference errors probably brought by cares : (.