-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
…ost name is "localhost"
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -945,7 +945,12 @@ function lookupAndConnect(self, options) { | |
}; | ||
|
||
if (dnsopts.family !== 4 && dnsopts.family !== 6) { | ||
dnsopts.hints = dns.ADDRCONFIG; | ||
// DO NOT USE AI_ADDRCONFIG ON WINDOWS. | ||
// see http://src.chromium.org/viewvc/chrome/trunk/src/net/dns/host_resolver_proc.cc for details. | ||
if (process.platform !== 'win32' || host != "localhost") { | ||
This comment has been minimized.
Sorry, something went wrong.
This comment has been minimized.
Sorry, something went wrong.
joaocgreis
|
||
dnsopts.hints = dns.ADDRCONFIG; | ||
} | ||
|
||
// The AI_V4MAPPED hint is not supported on FreeBSD, and getaddrinfo | ||
// returns EAI_BADFLAGS. However, it seems to be supported on most other | ||
// systems. See | ||
|
4 comments
on commit 8e0f244
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your change seems effective in making Windows behave like Linux. However something about this still doesn't add up. @joaocgreis and I both reviewed this and came to the same conclusion. But maybe we are missing something...
The Node.js documentation (https://nodejs.org/docs/v0.12.0/api/dns.html#dns_supported_getaddrinfo_flags for v0.12 and https://nodejs.org/api/dns.html#dns_dns_lookup_hostname_options_callback for 4.0) states that the loopback address is not considered when dns.ADDRCONFIG is passed to dns.lookup.
Since 4306786, net.connect is passing dns.ADDRCONFIG to dns.lookup when no specific address family is requested, so I would expect (whether that's desirable or not), for the lookback address not to be resolved according to the documentation.
The documentation is consistent with the man page for getaddrinfo, which says:
The loopback address is not considered for this case as valid as a configured address.
So when misterdjules says at nodejs/node-v0.x-archive#25489 (comment):
This seems to be a Windows specific issue. I haven't been able to reproduce it on Linux.
... it seems to me that Linux is the one that is not behaving according to documentation.
Does this analysis make sense to you? If so, I would probably continue the discussion on the github issue, to get to the bottom of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit message needs to adhere to the following guidelines (as seen here):
- The first line should have a maximum of 50 columns.
- The second line should be blank.
- Any additional lines should not exceed 72 columns.
- The author field should be properly filled out with your full name and email address.
You may also want to check for linting errors with vcbuild jslint nobuild nosign
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@orangemocha, In fact, I fully agree with your statement:
... it seems to me that Linux is the one that is not behaving according to documentation.
I would love to share our discussion with the community. What is the best way to do that? I assume that we need first to copy the issue to the new repo and second - push the PR. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@v-vlshch : certainly, we can and should have this discussion openly in the community. No need to open a new issue, we can comment on the one that's already open. Feel free to report your findings, or let me know if you prefer me to initiate the conversation.
Since at the moment we are not convinced that Windows is at fault / we came to a different conclusion with others on the issue, I think we should hold off on opening the PR. You could however post an updated commit in this repo (fixing the style issues) and reference it from your comments, if it helps the conversation.
use !== instead of !=