dns.lookup blocks filesystem I/O

[ottobonn]

• Version: v4.5.0
• Platform: Linux jessie 3.16.0-4-amd64 # 1 SMP Debian 3.16.7-ckt25-2+deb8u3 (2016-07-02) x86_64 GNU/Linux
• Subsystem: dns

Hi all,

I'm posting this issue as a continuation of an issue originally opened against node-serialport: serialport/node-serialport#797

On networks with slow DNS response, or where DNS requests time out and fail, we observe that the blocking calls to getaddrinfo issued by dns.lookup saturate Node's libuv threadpool and delay our serialport or filesystem IO.

It looks like this issue has come up many times before. The most pertinent example I could find is nodejs/node-v0.x-archive#2868, which outlines exactly this issue. However, it was closed in favor of nodejs/node-v0.x-archive#8475, in which the API of net.connect was changed to allow a user-specified DNS resolution handler.

The original proposal of issue 2868, which included a change to dns.lookup, seems to have been lost in the consolidation to issue 8475. In our case, we'd like to be able to use the OS-level DNS facilities like a local DNS cache, so using dns.resolve does not seem to be an equivalent substitute to using dns.lookup. Furthermore, our application uses multiple high-level modules that wrap REST APIs with their own request modules, so changing every call to net.connect to use a custom DNS function is not feasible.

There seems to be a relevant open issue against libuv on the matter at libuv/libuv#203. Through various issue reports, I've seen proposals in libuv to use getaddrinfo_a, to segregate the threadpools, or to cap the number of threads DNS requests could use, to avoid DNS requests starving all other IO.

Unfortunately, although the synchronicity of dns.lookup is documented at https://nodejs.org/api/dns.html#dns_implementation_considerations, its behavior is not intuitive given that other network requests are truly asynchronous, and it has resulted in many downstream issues, including our seeming issue with serialport.

[jasnell]

Yes, unfortunately this has been a known issue for a while but there's not a clear solution just yet outside of a massive (and non-backwards compatible) refactoring of the dns module in core. It's something that needs to get done, for sure, but it's going to take a while. I'm going to mark this as a confirmed bug so that it stays on the radar.

[ottobonn]

Thanks for the prompt response.

Just to clarify, I'm referring to the fact that the underlying implementation of dns.lookup is based on a blocking call to getaddrinfo. Could you elaborate on why the dns module would need refactoring? Do you mean the JavaScript layer needs refactoring, or the implementation of the dns module's low-level interaction with libuv? When you say it wouldn't be backwards-compatible, are you referring to the JS API or the internals?

[jasnell]

Little of both, really. There may be ways of tweaking the current implementation to improve this particular issue without breaking things but doing so would largely just end up being a band aid. There are several issues with the current implementation that make it less than desirable.

[owenallenaz]

Upvoting this issue. We had this recently hit one of our applications in a very non-intuitive way. The node process we are running acts as a monitor for hundreds of host names. We are pinging the servers using request but the monitor process was running out of memory and crashing. From our tests the Node event loop wasn't blocked because setImmediate fires efficiently, yet fs calls would never complete.

After roughly 10 hours searching the internet on how outgoing requests could cause the fs to block I finally found this thread. Based on it, and the dns docs, it appears that what is occurring under the hood is that the dns requests are being delayed, causing the libuv threadpool to fill. We had a setInterval in place that was logging the state of the system, since the fs.writeFile was hanging forever, then the scope of that function were staying in memory forever (since the writeFile callback is never executed).

The other complicating factor is that in the http package there is a setTimeout call but it doesn't affect the timeout on the dns.lookup. So if I put a setTimeout of 10ms on an http.request(), it will not timeout even if the dns.lookup takes 5000ms. This makes this bug very unintuitive for a user as they can have an http request with a timeout, that won't timeout, even if the request takes longer than the timeout (since it doesn't include dns.lookup in that timeout).

So while this behavior might be known, the results of it are very, very unintuitive and difficult to debug. The idea that a dns failure can block the file system, even though the event loop is clear is pretty arcane. That being said, what are the ways to avoid dns delays from causing widespread failures? Pre-lookup the ip based on the host header using dns.resolve and pass ip with host header to things like http.request()? Alterations to the local dns setup to timeout faster or cache better? Pray that dns never goes down? Speak to package developers for tools like request to see if they will bake this system into theirs?

Is there a list of specific configuration files that differ between dns.lookup and dns.resolve in the docs they say e.g. /etc/hosts that makes mention of other files, but what are they? From my tests our dnsmasq running locally is still hit even when I utilize dns.resolve.

Any guidance is appreciated.

[sam-github]

The other complicating factor is that in the http package there is a setTimeout call but it doesn't affect the timeout on the dns.lookup.

Can you report that as a bug, please? That should be fixable.

Pre-lookup the ip based on the host header using dns.resolve and pass ip with host header to things like http.request()?

Yes, that will keep node out of the thread pool and on the main thread, solving your contention problem.

Local DNS setup can help with the problem of just getting faster DNS responses.

Increasing the threadpool size from 4 to a higher number can help, UV_THREADPOOL_SIZE=X with the environment.

[sam-github]

Btw, no need to increase threadpool size if you can avoid using the threadpool, but fs is using the pool, so its still worth looking at.

[owenallenaz]

Is there an objection to exposing a setting on http and https that would allow someone to specify whether to utilize lookup or resolve? Currently there is a family argument where we can specify whether we want ipv4 or ipv6 results. My request feels like it is in the same vein. Is there a harm to allowing something like lookupMethod : "lookup" || "resolve" and if resolve is passed it will utilize the proper resolve4 or resolve6 according to the family argument. This alleviates backward compatibility issues and allows user-land modules like request to piggyback and expose that setting to their users.

Yes, the behavior I seek can be accomplish by doing things like createConnection argument on the http. request, but it's a fair amount of work for something which feels pretty simple and pretty ubiquitous. I think if most people knew that their dns requests were being processed synchronously, it would be a bigger issue than it is and it really only rears it's head when a major dns provider starts timing out and suddenly your node app comes to a screeching halt because of synchronous calls (which is why we avoid sync calls in the first place!)

[sam-github]

dns calls are no more synchronous than fs calls, they aren't. They do contend with the thread pool.

What is happening with DNS provider timeouts? Is the thread in the (small) pool taking so long that new lookups are blocked, waiting for a free thread?

Has pre-resolving the hostnames not worked for you?

There has been some interest in having a pluggable resolver, but no-one has stepped up to do the work, yet. Note that user-land modules like request can already implement options to resolve with dns.resolve instead of dns.lookup.

[owenallenaz]

@sam-github The problem arose for us that our DNS server was taking 5s+ to respond, due to issues with the server itself. The node process was responsible for heartbeating thousands of different host names via http requests every 5 minutes. When the dns server slowed, then the dns.lookup calls quickly took up all 4 members of the thread pool. Now since the node event loop was free, our setInterval based file logging, would fire, never reach the fs callback (due to full thread pool), causing a process crashing memory leak. The idea that DNS lookups can cause a memory leak due to file system backups is really, really unintuitive. Generally dns is instant, and we've updated our local dns configuration to further prevent this problem by querying multiple servers in parallel via dnsmasq.

I have reproduction of the fs lock-up below:

https://gist.github.com/owenallenaz/fc3d8b24cdaa4ec04cc9dd0bf8ab485f

In that case, by utilizing dns.lookup (the default for http.request) I can see that the fs.readFile stalls for an entire 4s. If I change this to dns.resolve this behavior does not occur.

The main reason I bring this up here if someone does not care about host file interactions, it would seem to me that the default behavior really should be dns.resolve rather than dns.lookup. Obviously we can't change that because of backwards compat reasons, but simplifying that switch to a setting seems worthwhile and then could be leveraged downstream by user-land packages. Pre-resolving did work for us, it's just a fair amount of code for something that feels as if it should almost be default behavior. If we were to submit a setting branch for this, would it be considered or is it still something you'd want in user-land?

[sam-github]

Thanks for the war-story, its good for them to be heard. I'm a bit surprised you don't also have a mem leak in the dns.resolve() case, don't you end up with the dns.resolve() calls taking much longer than necessary, and requests piling up? Or do they just pile up to a higher than expected but still tolerable steady-state memory usage, as opposed to the fs log output, which piled up until out-of-memory?

Btw, arguably, async itself is unintuitive (at least, lots of people have had difficulty wrapping their head around it), so general statements about how some node API was found unintuitive to some person aren't really compelling reasons to make the API more complex.

I find the docs are deficient, they have no description of what is async-on-epoll, and what is async-in-the-thread-pool, and I intend to fix that. I think that clearly documenting that, as well as UV_THREADPOOL_SIZE is a better fix of the "unintuitive" than adding yet another knob to an already complex HTTP API (and the same knob would need to be added to the net and tls APIs, too, of course, they need to resolve host names).

All PRs are considered, of course. But you want a predictor of how positive people would feel about it before wasting your time, understandable.

Personally, I'd be not inclined towards it, here is why.

function get(host, ...) {
  dns.resolve(host, function(ip) {
    http.get(ip, ...)
  }
}

vs

function get(host, ...) {
    http.get(ip, {
      hostLookup: 'resolve'
    }, ...)
}

The first looks like functional composition to me, nice APIs, that work well together, the second just looks like an ugly hack.

IMHO, of course.

Also, the options knob to change the underlying function in dns.* to use doesn't allow more interesting usage, like making the dns implementation completely user-selectable.

This particular suggestion is probably awful, but as a straw-man, I'd be more inclined to something like

function get(host, ...) {
   http.get(host, {dns: myDns}, ....);
}

Where myDns (and built-in dns) was required to have a host2addr() method. Then, you could completely replace dns with your own implementation, or just do myDns = {host2addr: dns.resolve}, or even dns.host2addr = dns.resolve; to change it globally.

Basically, I'd prefer to see an approach to a more pluggable DNS, that does more than what can be already achieved by functional composition, and otherwise, just document that node APIs can be used together to get specific results.

[owenallenaz]

Sounds good. I'll move my nagging up the chain to request to see if they are interested at that level. If this thread sees more traffic we can re-evaluate. As it stands right now I think it's fair to say the current system is entirely pluggable, due to the ability to pass http.request({ createConnection : customFn }) which then can pass through to net.createConnection({ lookup : customLookupFn }). That pathway does allow a custom dns lookup method to be used as you describe (not very well documented, but existing). It's just a fair amount of code per call, so putting it in a library somewhere is wise.

To your question, I end up with dns.resolve calls piling up, but only briefly, as the whole load of 1000 queries clears in 5s. What I don't end up with is fs.writeFile calls piling up and holding their string data in memory forever because the application setInterval logging mechanics still work since the thread pool still has room, even if the resolve calls are timing out..

With dns.lookup it can only run 4 requests in parallel, so 1000 calls means 1000 calls / 4 in parallel * 5 seconds per batch === 20 minutes of complete thread pool lock-out. Then with the thread pool locked up, the setInterval triggered fs.writeFile holds it's string forever, or at least for 20 minutes, by then it's attempted to queue up 4 writes before even one of them flushes. It would require a massive increase in the thread pool count to make a dent in that because of the problematic synchronicity.