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 lookup failure dns.js:82 #4545

Closed
allthetime opened this issue Jan 6, 2016 · 13 comments
Closed

DNS lookup failure dns.js:82 #4545

allthetime opened this issue Jan 6, 2016 · 13 comments
Labels
dns Issues and PRs related to the dns subsystem.

Comments

@allthetime
Copy link

TypeError: Cannot read property 'indexOf' of undefined    
 at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:82:51)

here's the relevant line in node.js lib (https://github.com/nodejs/node/blob/master/lib/dns.js#L82)

I have a bot running that polls an sftp server for the existence of a file that is uploaded daily. It connects to socketio so that it can make posts to our main app.

It has suddenly not been working properly for the last 3 days. We have made no changes to the relevant code for a couple of weeks and the stack trace is incredibly unhelpful as it doesn't show what initiated it.

This seems to be happening before the request to the sftp server is made because I would be getting more feedback/logs. The bot successfully connects to our socket server as well, so I'm at a loss for where DNS is even being hit.

connect to socket io --> poll sftp server --> grab file when ready

is all that is happening, so it seems something is happening in the nodejs blackbox between step one and two...

any ideas?

@saghul saghul added the dns Issues and PRs related to the dns subsystem. label Jan 6, 2016
@saghul
Copy link
Member

saghul commented Jan 6, 2016

Looks like getaddrinfo returned an empty list of results somehow? I thought that would fail with EAI_NODATA. Do you have a small reproducible test case?

@silverwind
Copy link
Contributor

@allthetime can you post the hostname of that sftp server?

@evanlucas
Copy link
Contributor

@saghul it seems like https://github.com/nodejs/node/blob/master/src/cares_wrap.cc#L947 might be the cause of this. uv_inet_ntop could be returning UV_ENOSPC. So in that case, res exists, but the array is an empty array when we switch back to js. So perhaps we should add a check for an empty array and if it exists, callback with an EAI_NONAME err code?

@saghul
Copy link
Member

saghul commented Jan 15, 2016

@evanlucas it could be, yes. I'd say there is nothing to fix here other than documentation. It's possible the list comes back empty, so one should look out for that.

@evanlucas
Copy link
Contributor

Well we are assuming that the array has at least one element

@saghul
Copy link
Member

saghul commented Jan 15, 2016

Bad assumption, I guess 😄

cjihrig added a commit to cjihrig/node that referenced this issue Jan 15, 2016
AfterGetAddrInfo() can potentially return an empty array of
results without setting an error value. The JavaScript layer
expects the array to have at least one value if an error is
not returned. This commit sets a UV_EAI_NODATA error when an
empty result array is detected.

Fixes: nodejs#4545
@a1arias
Copy link

a1arias commented Jan 16, 2016

This seems to be the same as nodejs/node-v0.x-archive#8698

I too see this when hitting EMFILE errors, same on (all?) versions of node (testing on 4.2.3 and 5.4.0). Interestingly, when running with a version of node compiled with --debug it doesn't happen. Why is that?

@saghul
Copy link
Member

saghul commented Jan 16, 2016

If you are seeing EMFILE errors you should raise the file descriptor limits
with uname -n.
On Jan 16, 2016 03:29, "Adrian Arias" notifications@github.com wrote:

This seems to be the same as nodejs/node-v0.x-archive#8698
nodejs/node-v0.x-archive#8698

I too see this when hitting EMFILE errors, same on (all?) versions of node
(testing on 4.2.3 and 5.4.0). Interestingly, when running with a version of
node compiled with --debug it doesn't happen. Why is that?


Reply to this email directly or view it on GitHub
#4545 (comment).

evanlucas pushed a commit that referenced this issue Jan 18, 2016
AfterGetAddrInfo() can potentially return an empty array of
results without setting an error value. The JavaScript layer
expects the array to have at least one value if an error is
not returned. This commit sets a UV_EAI_NODATA error when an
empty result array is detected.

Fixes: #4545
PR-URL: #4715
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
@a1arias
Copy link

a1arias commented Jan 19, 2016

@saghul that is a good for dev only work-around, not a solution, and will only differ the problem. What do you do when the security limits have already been increased to the max the machine can physically handle?

In my case, a more robust solution would be to refactor the app so that a) we use connection pooling and b) we don't keep the connections open while we process the request. However, that is a architecture flaw and besides the point: this is a confirmed bug and EMFILE helps reproduce it reliably.

A node process should not crash, as described in this issue, when EMFILE gets hit.

To easily reproduce this error, these conditions need to exists.

  1. An external domain name must be used. That is, a domain that is not resolvable locally and for which the resolver must send an actual DNS query. This is so the flow control routes to the correct code path in the core dns.js module where the issue exists.

  2. EMFILE error must be hit. To simulate an EMFILE error, you can do the following:

    • set the max open file limit really low
    ulimit -n 1024
    
    • setup a test http server route which keeps the connections open by not sending a reply.

With these conditions met, run the app and wait for the error.

Strangely, this does not happen when running with the debug version of node, node_g, compiled with the --debug and --gdb option. Maybe this is because the debug version introduces some delay that mitigates an underlying race condition.

@saghul
Copy link
Member

saghul commented Jan 20, 2016

We try hard to make EMFILE errors propagate as far up as possible (IIRC), so Node doesn't crash and your application handles those.

AFAIS the patch @cjihrig made does not alter that behavior, because it only applies if status == 0, that is, if getaddrinfo succeeded.

set the max open file limit really low

ulimit -n 1024

It defaults to 256 on OSX, BTW.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 20, 2016

AFAIS the patch @cjihrig made does not alter that behavior, because it only applies if status == 0, that is, if getaddrinfo succeeded.

Yes, the crash in JavaScript was the result of getaddrinfo() succeeding, but passing back an empty array. In JS, we were assuming that the array had at least one element. My patch only ensured that that situation didn't happen.

MylesBorins pushed a commit that referenced this issue Mar 17, 2016
AfterGetAddrInfo() can potentially return an empty array of
results without setting an error value. The JavaScript layer
expects the array to have at least one value if an error is
not returned. This commit sets a UV_EAI_NODATA error when an
empty result array is detected.

Fixes: #4545
PR-URL: #4715
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 21, 2016
AfterGetAddrInfo() can potentially return an empty array of
results without setting an error value. The JavaScript layer
expects the array to have at least one value if an error is
not returned. This commit sets a UV_EAI_NODATA error when an
empty result array is detected.

Fixes: #4545
PR-URL: #4715
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
scovetta pushed a commit to scovetta/node that referenced this issue Apr 2, 2016
AfterGetAddrInfo() can potentially return an empty array of
results without setting an error value. The JavaScript layer
expects the array to have at least one value if an error is
not returned. This commit sets a UV_EAI_NODATA error when an
empty result array is detected.

Fixes: nodejs#4545
PR-URL: nodejs#4715
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
@stanislav-kozyrev
Copy link

Hi.

TL;DR Any chance the fix gets back-ported to 4.x?

I am on Ubuntu 16.04.1 LTS which is stuck with 4.2.6 at the moment. npm install worked like a charm until recently. I read that NodeJS is using libuv for DNS resolution so it probably has something to do with my network configuration (which is strange since I haven't changed/rebooted anything in months). Either way, I tried the fix on Ubuntu's version of NodeJS and npm is working again. Should I talk to Ubuntu/Debian developers about that? Thank you in advance.

@bnoordhuis
Copy link
Member

@stakoz It was released in v4.4.1, commit 7c3b844. Getting in touch with the maintainer of the distro package is your best course of action, yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dns Issues and PRs related to the dns subsystem.
Projects
None yet
Development

No branches or pull requests

8 participants