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

net: wrap connect in nextTick #2054

Merged
merged 1 commit into from
Jul 1, 2015
Merged

Conversation

evanlucas
Copy link
Contributor

Fixes an edge case regression introduced in
1bef717.

With the lookup being skipped, an error could be emitted before and
error listener has been added.

An example of this was presented by changing the server’s IP address
and then immediately making a request to the old address.

Related: #1823

@evanlucas evanlucas added the net Issues and PRs related to the net subsystem. label Jun 25, 2015
@evanlucas
Copy link
Contributor Author

The test that is removed in this PR was added specifically to test the functionality introduced in the commit that caused the issue.

EDIT: No longer applicable.

@trevnorris
Copy link
Contributor

This is more costly than simply putting the connect in a nextTick.

@evanlucas evanlucas changed the title net: don't skip lookup on IP addresses net: wrap connect in nextTick Jun 25, 2015
@evanlucas
Copy link
Contributor Author

@trevnorris yea, good point. Updated to just wrap connect in process.nextTick

@evanlucas
Copy link
Contributor Author

@cjihrig
Copy link
Contributor

cjihrig commented Jun 25, 2015

LGTM

@trevnorris
Copy link
Contributor

LGTM

For reference it could have also been done as

process.nextTick(connect, self, host, port, addressType, localAddress, localPort);

But I'm fairly certain it would be slower this way. So don't change how you currently have it.

@trevnorris
Copy link
Contributor

Oh, is there a test we could add for the edge case regression?

@evanlucas
Copy link
Contributor Author

I was trying to think of a way to test that... The only case I've seen has been changing the server's IP address and then continuing to attempt to make a request to the old one.

I was able to reproduce fairly consistently using https://gist.github.com/m0ppers/3709f7d87b73be9aca7b and changing my IP

@evanlucas
Copy link
Contributor Author

I haven't been able to write a test that exhibits the exact same behavior each time for my local machine. It happens when a EHOSTDOWN or EHOSTUNREACH error is emitted and it takes a few requests to actually make the situation happen...

@trevnorris
Copy link
Contributor

@evanlucas I've tried myself. Can't figure out a way to do it from a simple test script. So we won't worry about it.

Fixes an edge case regression introduced in
1bef717.

With the lookup being skipped, an error could be emitted before an
error listener has been added.

An example of this was presented by changing the server’s IP address
and then immediately making a request to the old address.

Related: nodejs#1823
PR-URL: nodejs#2054
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-by: Trevor Norris <trev.norris@gmail.com>
@evanlucas evanlucas closed this Jul 1, 2015
@evanlucas evanlucas deleted the fixlookup branch July 1, 2015 04:20
@evanlucas evanlucas merged commit af249fa into nodejs:master Jul 1, 2015
@evanlucas
Copy link
Contributor Author

Thanks, landed in af249fa

@rvagg rvagg mentioned this pull request Jul 2, 2015
mscdex pushed a commit to mscdex/io.js that referenced this pull request Jul 9, 2015
Fixes an edge case regression introduced in
1bef717.

With the lookup being skipped, an error could be emitted before an
error listener has been added.

An example of this was presented by changing the server’s IP address
and then immediately making a request to the old address.

Related: nodejs#1823
PR-URL: nodejs#2054
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-by: Trevor Norris <trev.norris@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants