Make it not fail if it can't find /etc/resolv.conf

[greggman]

On OSX /etc/resolv.conf is generated by the OS anytime the network
changes. If I create a network "By picking 'Create Network...'" from
the wifi menu then there is no /etc/resolv.conf and the throw
that was here would .. uh, .. throw.

Making it not throw makes the dns-server work for my purposes.

I don't know if this is how you like to deal with that or if you'd like some flag or check or OSX only or what but I thought I'd at least pass on what I needed to do to get it to work for me.

[jesusprubio]

Please merge it, I also need this fix. :)

[taoeffect]

Hmm, this fix could be done better. Instead of ignoring the error, the error can be checked to see if it's because the file wasn't found, and in that case return (to not re-indent all the lines below it; re-indenting = all lines must be reviewed by someone to make sure nothing else changed), otherwise, throw the error. @greggman, could you re-submit such a PR?

[greggman]

something like this? (see new patch)

[taoeffect]

@greggman Yep, that's perfect, thanks! Hope to merge this soon along with #71 and maybe some others.

[sindresorhus]

👍

[silverwind]

Another approach for OS X could be to use /private/etc/resolv.conf instead of /etc/resolv.conf, which seems to always exist, even if no network interface is up.

[silverwind]

@taoeffect Any time frame on when you could merge this? It seems you have commit access and could publish on npm too. sindresorhus/is-online#8 depends on this issue and we're pondering whether to fork this module with this fix included.

[taoeffect]

@silverwind Thanks for the poke.

I just pushed latest changes to my PR for native-dns-packet.

I bumped its version to 0.1.0, and am now waiting for @tjfontaine to review and OK the PR to be merged into master. Once he does that, I'll be able to update (or create) our PR for this repo and bump the version (and reference the changes in native-dns-packet).

After that (or at the same time) we can merge this PR. Ball is in @tjfontaine's court.

Sorry for taking so long on this!

[taoeffect]

BTW, if any of you want to review that PR, it would be super appreciated! I am currently using it in DNSChain.

[silverwind]

@taoeffect sounds good, hopefully this project will see a bit activity again soon! If time permits, I'll have a look at your changes.

[tjfontaine]

lgtm, @taoeffect merge it?

[tjfontaine]

actually let me correct that, we need to also need to set nsReady probably because otherwise we won't make forward progress?

[silverwind]

From my testing with this patch, it looks to be working. I set off a request with all interfaces down on OS X and got a timeout event, as expected.

[silverwind]

While I haven't yet understood the intricacies of how OS X manages /etc/hosts, I think the proper fix here would probably be to use /private/etc/resolv.conf as a fallback when /etc/hosts doesn't exist (on my machine, it contains a local nameserver I statically configured on my WiFi interface).

I can do PR for this fallback, but I'd have to study how OS X manages these files first.

[tjfontaine]

Ok thinking about this more, here are my full thoughts:

We should probably have a way to instantiate a platform that represents native-dns, which has some default behaviors.

require('native-dns') for strawman purposes createNativeDNS({ hostsFile: platformHostsFile, resolvConf: platformResolvConf })

but you could later, recreate another (and even potentially replace the existing one [may be bad hygeine]) with other places where you expect to find those files.

var dns = require('native-dns')
newdns = dns.createNativeDNS({
  hostsFiles: '/private/etc/hosts',
  resolvConf: '/private/etc/resolv.conf',
});
// maybe?
dns.setDefaultImplementation(newdns);

Also the interface for setting the "platform" (i.e. default nameservers) is wrong, it shouldn't be platform.name_servers that should be considered private, it instead should be nativedns.setServers or calling that on what the result from createNativeDNS returns. [This interface has been defined in node 0.12].

A further improvement would be if there is nothing in platform.name_servers and we've some how signaled that we're ok with trying to make forward progress on requests, we should fail fast on the specific request by saying there are no available name servers [libc resolver probably has a default error for this that we should piggy back on].

So in short:

• an ENOENT for the default files we're parsing should allow the state machine to progress
  • we may want to raise platformError with the ENOENT if people want to handle it, or any error for that matter
• we should have a cleaner interface [that matches what's coming in node v0.12] for setting the default name servers
• when there are no name servers defined we should flush the request queue with an error not a timeout
  • TODO find out what libc normally uses for an error in this case and use that
• optionally in the future we should have a way to create and maybe replace the default implementation

[silverwind]

Thanks for the detailed writeup, I think this merits an issue of its own.

But could we get this PR (after review from @taoeffect) merged and pushed to npm? Right now we're relying on a fork with this fix in is-online, as this throw inside async code cannot be caught except with uncaughtException, which is obviously not what we wanna do.

[taoeffect]

#71 has been merged! 0.7.0 has been published! Thanks @tjfontaine! (Hope it doesn't break things for anyone! crosses fingers).

I've gotta run for now, but I will read the comments above and merge this if all is good. (@tjfontaine: TLDR, does it lgty? Or are you requesting something else?)

[silverwind]

@taoeffect from above:

lgtm, @taoeffect merge it?

I'd say we merge it and open an own issue for the platform rework described above.

[sindresorhus]

I'd say we merge it and open an own issue for the platform rework described above.

👍

[silverwind]

@taoeffect nudge nudge

(we still depend on this PR's branch, would love seeing this merged and released)

[taoeffect]

@silverwind OK, this looks harmless enough, I'll add merging this to my TODO for tomorrow. POKE ME PLZ if I fail to do that!! I even give you permission to badger me on twitter (if I forget to do it, I will be grateful for any reminders!): @taoeffect

[taoeffect]

OK, merged!

Now, is this little change worth making a release for? I am of no opinion either way, so you folks tell me what to do! (Esp. @tjfontaine if you've a mind on it.)

[silverwind]

Thank you ❤️

To help you on that, I'll look into the issues and PRs for any critical stuff on which I can help. #87 sounds pretty serious if true. I should be able to finish this on the weekend hopefully.

[taoeffect]

Awesome! Thanks much @silverwind! 😄

[silverwind]

Took a shot at a few issues, but couldn't reproduce any of them. There's a few low-risk PRs which I'd likely merge before a release:

#82
#83
#84
#90

[taoeffect]

Thanks @silverwind! Merged #84 and #90, but the others I have some questions/concerns that need to be addressed before I feel OK merging them.

I know most everyone who's working on this is doing it in their volunteer time (including myself), and my time's up. Would be oh so awesome if I could get some help reviewing #82 and #83! (See my comments there.)

[silverwind]

@taoeffect thanks for your effort. I'm not sure I'll be motivated enough to dive deeper into this module right now, as the addition of dns.setServers in node 0.12 is enough for my use case to switch back to core dns.

There's a lot of low-level stuff this module provides to which there's really no alternative in node-land right now, so I hope this module will be in a better shape soon.

[sindresorhus]

@taoeffect Any chance you could do a new release?

[taoeffect]

@sindresorhus I am swamped at the moment with other commitments, I posted a call for help here.