-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
improve error messge in lib/dns #3891
Comments
I'm all for removing these types of things from the codebase. My only reservation is that we don't know how much code would break for relatively little gain. |
I'm torn. Like @cjihrig I don't see a lot of value but eliminating technical debt is generally a good thing. Smoke testing this would be worthwhile. |
We discussed DNS errors in #1843, and ideally we'd like to return real DNS error codes ( |
@jasnell is this something I should try and get together for v6? Will it just create too much churn? |
We should likely hold off and do this post v6. It's still not yet clear how much this will impact. |
These are some exotic errors, I guess it would be pretty low impact, except maybe UV_EAI_NONAME. I'd say we should take the chance and fix this now. |
@jasnell with that in mind it makes sense to aim to get this in for v7, that way if it was a terrible idea we can back it out in v8 and not be stuck with it for 3 years! |
What's the supposed fix going to be, by the way? Remove the conditional and progagate libuv error codes the user? |
@silverwind indeed. It would be propogated proper error codes |
Hmm, I'm just worried about our |
I've been wresting with an ENOTFOUND error (in Node.js
This happens within about 30 seconds after service startup. Any idea how to get more info on what's really happening here? |
@atomantic I speculate that if you attach |
@bnoordhuis thanks. Regardless of why it's happening, I'm going to bypass this by setting up a child process DNS watchdog and simply continue to reference the target by IP. One less thing for the http request to have to do so it should be better performance anyway and removes the potential for a cache lookup or DNS resolution mishap. |
ping @MylesBorins ... what do you want to do with this one? |
I don't think this is going to happen any time soon, although a clean CITGM run would go a long way towards making it seem more possible. I'm going to close this for now, but definitely re-open it if you're working on it. |
Due to backwards compatibility dns.js is reporting UV_EAI_MEMORY, UV_EAI_NODATA, and UV_EAI_NONAME errors as ENOTFOUND
This is currently labelled FIXME --> https://github.com/nodejs/node/blob/master/lib/dns.js#L17-L23
The change breaks a few tests
interestingly enough every testc ase we had was for EAI_NONAME, we have no tests for EAI_MEMORY or EAI_NODATA
If people are interested I'll get started on this change, including writing extra tests and smoke testing. This should be a great candidate for citgm
The text was updated successfully, but these errors were encountered: