Skip to content

net: remove hot path comment from connect - #4648

Merged
evanlucas merged 1 commit into
nodejs:masterfrom
evanlucas:netrmcomment
Jan 12, 2016
Merged

net: remove hot path comment from connect#4648
evanlucas merged 1 commit into
nodejs:masterfrom
evanlucas:netrmcomment

Conversation

@evanlucas

@evanlucasevanlucas commented Jan 12, 2016

Copy link
Copy Markdown
Contributor

This comment was added with an assumption that we could determine the
IP address that localhost should resolve to without performing a
lookup. This was a false assumption and should be removed.

Ref: #4642

@evanlucasevanlucas added the net Issues and PRs related to the net subsystem. label Jan 12, 2016
@benjamingr

benjamingr commented Jan 12, 2016

Copy link
Copy Markdown
Member

Sorry for the silly question - why is the assumption false?

@evanlucas

evanlucas commented Jan 12, 2016

Copy link
Copy Markdown
ContributorAuthor

Not a silly question at all. I consider it to be false because we have to use the system resolver (which is what dns.lookup does) to determine what IP should be used for localhost. More information on that can be found at https://nodejs.org/dist/latest-v5.x/docs/api/dns.html#dns_implementation_considerations. As the entire point of the original commit was to bypass calling dns.lookup to prevent running in the threadpool, it makes trying to do anything with localhost specifically unnecessary (unless there is another way to determine the IP for localhost of which I am not aware).

@evanlucas

evanlucas commented Jan 12, 2016

Copy link
Copy Markdown
ContributorAuthor

For reference, the commit where this was added was 1bef717

@mscdex

mscdex commented Jan 12, 2016

Copy link
Copy Markdown
Contributor

LGTM

@benjamingr

benjamingr commented Jan 12, 2016

Copy link
Copy Markdown
Member

Hmm, I guess someone can override it to be something other than ::1 (v6) or 127.0.0.1 (v4) and Node cannot be the one to make that assumption. Users can always optimize it at their end.

LGTM.

@cjihrig

cjihrig commented Jan 12, 2016

Copy link
Copy Markdown
Contributor

LGTM

@evanlucas
net: remove hot path comment from connect
3a7f106
This comment was added with an assumption that we could determine the
IP address that localhost should resolve to without performing a
lookup. This was a false assumption and should be removed.
PR-URL: nodejs#4648
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@evanlucas
evanlucas deleted the netrmcomment branch January 12, 2016 15:58
@evanlucas
evanlucas merged commit 3a7f106 into nodejs:masterJan 12, 2016
@evanlucas

evanlucas commented Jan 12, 2016

Copy link
Copy Markdown
ContributorAuthor

Landed in 3a7f106. Thanks!

MylesBorins pushed a commit that referenced this pull request Jan 12, 2016
@evanlucasMyles Borins
net: remove hot path comment from connect
ffb7deb
This comment was added with an assumption that we could determine the
IP address that localhost should resolve to without performing a
lookup. This was a false assumption and should be removed.
PR-URL: #4648
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Jan 12, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Jan 28, 2016
@evanlucasMyles Borins
net: remove hot path comment from connect
c423924
This comment was added with an assumption that we could determine the
IP address that localhost should resolve to without performing a
lookup. This was a false assumption and should be removed.
PR-URL: #4648
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
@evanlucasMyles Borins
net: remove hot path comment from connect
9a02fb3
This comment was added with an assumption that we could determine the
IP address that localhost should resolve to without performing a
lookup. This was a false assumption and should be removed.
PR-URL: #4648
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 11, 2016
@evanlucasMyles Borins
net: remove hot path comment from connect
62f18c3
This comment was added with an assumption that we could determine the
IP address that localhost should resolve to without performing a
lookup. This was a false assumption and should be removed.
PR-URL: nodejs#4648
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Feb 11, 2016
Merged
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 15, 2016
@evanlucasMyles Borins
net: remove hot path comment from connect
40c8e6d
This comment was added with an assumption that we could determine the
IP address that localhost should resolve to without performing a
lookup. This was a false assumption and should be removed.
PR-URL: nodejs#4648
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@evanlucasMichael Scovetta
net: remove hot path comment from connect
5725f89
This comment was added with an assumption that we could determine the
IP address that localhost should resolve to without performing a
lookup. This was a false assumption and should be removed.
PR-URL: nodejs#4648
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

netIssues and PRs related to the net subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@evanlucas@benjamingr@mscdex@cjihrig@jasnell@MylesBorins