http/net: fix catching immediate connect errors #1823

m0ppers · 2015-05-28T14:29:54Z

When creating a standard http client request the agent is calling
net.createConnection. The agent will then defer installing error
handling to nextTick(). If there is any immediate error in
createConnection iojs will bail with an uncaught error. There is already
one error situation which is handled in a special way (i.e. throwing
error is deferred so the standard error handler may catch it). However
there are more situations where this may happen: Namely in the connect()
function.

Due to a locally changed ip address I was getting the following error and the iojs bailed out (which should not happen as error management was in place):

events.js:141
      throw er; // Unhandled 'error' event
            ^
Error: connect EHOSTDOWN 192.168.1.11:80 - Local (192.168.1.10:54125)
    at Object.exports._errnoException (util.js:846:11)
    at exports._exceptionWithHostPort (util.js:869:20)
    at connect (net.js:840:14)
    at lookupAndConnect (net.js:933:5)
    at Socket.connect (net.js:902:5)
    at Agent.exports.connect.exports.createConnection (net.js:61:35)
    at Agent.createSocket (_http_agent.js:177:16)
    at Agent.addRequest (_http_agent.js:147:23)
    at new ClientRequest (_http_client.js:132:16)
    at Object.exports.request (http.js:30:10)

I am still unsure why I am the only one facing this problem and maybe the pull request is entirely bogus. The error situation seems pretty common to me (and that's why I am surprised): My dhcp assigned my dev machine a new address and connecting to the old, hardcoded ip address is now failing with an ETIMEOUT (as expected). However another, slightly (few seconds) deferred request (while the first one is still working) will trigger an immediate EHOSTDOWN/EHOSTUNREACH error for me which cannot be caught using request.on("error", fn).

I can 100% reproduce this with iojs 2.1 and master using the following gist:

https://gist.github.com/m0ppers/3709f7d87b73be9aca7b

I am on osx yosemite.

The wrapped nextTick, nextTick thing is of course totally evil. However I cannot seem to fix it otherwise. The root cause IMHO is the deferred error management in the http client. I don't know the reason for it and left it untouched but IMHO this should be the real fix.

It has been introduced with this commit (which makes me even more unsure as this is years ago):

nodejs/node-v0.x-archive@7c87e09

Can somebody reproduce the problem? Am missing something? Does somebody have a better fix (nextTick(nextTick()))? :S

When creating a standard http client request the agent is calling net.createConnection. The agent will then defer installing error handling to nextTick(). If there is any immediate error in createConnection iojs will bail with an uncaught error. There is already one error situation which is handled in a special way (i.e. throwing error is deferred so the standard error handler may catch it). However there are more situations where this may happen: Namely in the connect() function.

Fishrock123 · 2015-05-28T14:32:31Z

@m0ppers does this happen on node 0.12 too? (I'm assuming it does given the commit in question)

m0ppers · 2015-05-28T14:41:43Z

Interesting :O works as expected with 0.12:

hans-guenther:io.js mop$ node -v; node broken.js 
v0.12.4
Server running at http://<ip-address>:8001/
GOT REQUEST
GOT REQUEST
ERR { [Error: connect EHOSTUNREACH]
  code: 'EHOSTUNREACH',
  errno: 'EHOSTUNREACH',
  syscall: 'connect' }

m0ppers · 2015-05-28T14:43:33Z

Just reverified with io.js 2.1.0:

hans-guenther:io.js mop$ node -v; node broken.js 
v2.1.0
Server running at http://<ip-address>:8001/
GOT REQUEST
GOT REQUEST
events.js:141
      throw er; // Unhandled 'error' event
            ^
Error: connect EHOSTDOWN 192.168.1.11:80 - Local (192.168.1.10:55714)
    at Object.exports._errnoException (util.js:846:11)
    at exports._exceptionWithHostPort (util.js:869:20)
    at connect (net.js:840:14)
    at lookupAndConnect (net.js:933:5)
    at Socket.connect (net.js:902:5)
    at Agent.exports.connect.exports.createConnection (net.js:61:35)
    at Agent.createSocket (_http_agent.js:177:16)
    at Agent.addRequest (_http_agent.js:147:23)
    at new ClientRequest (_http_client.js:132:16)
    at Object.exports.request (http.js:30:10)

So definately some regression. Interesting :O Didn't expect that. However that explains why so far nobody got that error

evanlucas · 2015-05-28T22:54:53Z

I'm thinking this may a regression introduced in #1505. If the host is an IP address, it skips calling the lookup function which could be causing the error to be on the same tick. Looking into it.

Ref: https://github.com/nodejs/io.js/blob/master/lib/net.js#L927

evanlucas · 2015-05-28T23:10:42Z

Can you try to reproduce after apply this patch?

commit f31295c0925464621073d2d635d9060284d4f62d
Author: Evan Lucas <evanlucas@me.com>
Date:   Thu May 28 18:08:06 2015 -0500

    defer

diff --git a/lib/net.js b/lib/net.js
index 5309521..d74f001 100644
--- a/lib/net.js
+++ b/lib/net.js
@@ -924,7 +924,9 @@ function lookupAndConnect(self, options) {
   // TODO(evanlucas) should we hot path this for localhost?
   var addressType = exports.isIP(host);
   if (addressType) {
-    connect(self, host, port, addressType, localAddress, localPort);
+    process.nextTick(function() {
+      connect(self, host, port, addressType, localAddress, localPort);
+    })
     return;
   }

m0ppers · 2015-05-29T07:28:44Z

Doesn't help :S it DOES however help when wrapped in another (evil) nextTick(). Would you agree that the rootcause for all the nextTick hacks in connect() is the http client stuff? Maybe it is time to fix the rootcause? The sole reason the error management has to be wrapped into nextTick(s) is that the http client stuff installs error management only on nextTick() isn't it?

evanlucas · 2015-06-02T19:58:08Z

I am not able to reproduce with the above patch. I am thinking that removing the hot path of skipping lookup should be added back though. Mind trying this patch?

commit 9daac3964dd0db81564759375b0f18a201d88306
Author: Evan Lucas <evanlucas@me.com>
Date:   Tue Jun 2 14:55:39 2015 -0500

    Don't skip lookup

    If lookup is skipped, an error can be emitted on the current tick
    (before an error listener has been added)

diff --git a/lib/net.js b/lib/net.js
index 5309521..06f38fa 100644
--- a/lib/net.js
+++ b/lib/net.js
@@ -920,14 +920,6 @@ function lookupAndConnect(self, options) {
   }
   port |= 0;

-  // If host is an IP, skip performing a lookup
-  // TODO(evanlucas) should we hot path this for localhost?
-  var addressType = exports.isIP(host);
-  if (addressType) {
-    connect(self, host, port, addressType, localAddress, localPort);
-    return;
-  }
-
   if (options.lookup && typeof options.lookup !== 'function')
     throw new TypeError('options.lookup should be a function.');

m0ppers · 2015-06-08T06:29:06Z

that works...

m0ppers · 2015-06-08T06:32:44Z

i just retried the nextTick around connect patch and it now (?) works for me too. Maybe i made a mistake while applying the patch. You can reproduce my original problem, can't you? If so i would assume that i made an error testing that patch.

m0ppers · 2015-06-24T19:25:26Z

any update?

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 · 2015-07-01T04:21:00Z

Fixed in af249fa.

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>

Fishrock123 added http Issues or PRs related to the http subsystem. net Issues and PRs related to the net subsystem. labels May 28, 2015

bnoordhuis force-pushed the master branch from 311c3e4 to b926718 Compare June 2, 2015 17:25

evanlucas mentioned this pull request Jun 25, 2015

net: wrap connect in nextTick #2054

Merged

rvagg force-pushed the master branch from 9d21135 to 628a3ab Compare June 25, 2015 09:18

evanlucas closed this Jul 1, 2015

This was referenced Jun 22, 2023

[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#506

Open

[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#508

Open

[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#543

Open

[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#574

Open

This was referenced Jul 7, 2023

[Snyk] Fix for 4 vulnerabilities leonardoadame/node#63

Open

[Snyk] Fix for 9 vulnerabilities leonardoadame/node#128

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http/net: fix catching immediate connect errors #1823

http/net: fix catching immediate connect errors #1823

m0ppers commented May 28, 2015

Fishrock123 commented May 28, 2015

m0ppers commented May 28, 2015

m0ppers commented May 28, 2015

evanlucas commented May 28, 2015

evanlucas commented May 28, 2015

m0ppers commented May 29, 2015

evanlucas commented Jun 2, 2015

m0ppers commented Jun 8, 2015

m0ppers commented Jun 8, 2015

m0ppers commented Jun 24, 2015

evanlucas commented Jul 1, 2015

http/net: fix catching immediate connect errors #1823

http/net: fix catching immediate connect errors #1823

Conversation

m0ppers commented May 28, 2015

Fishrock123 commented May 28, 2015

m0ppers commented May 28, 2015

m0ppers commented May 28, 2015

evanlucas commented May 28, 2015

evanlucas commented May 28, 2015

m0ppers commented May 29, 2015

evanlucas commented Jun 2, 2015

m0ppers commented Jun 8, 2015

m0ppers commented Jun 8, 2015

m0ppers commented Jun 24, 2015

evanlucas commented Jul 1, 2015