http.get() raises uncatchable EBADNAME exception

[bnoordhuis]

Test case:

var http = require('http');
var host = '********';

host += host;
host += host;
host += host;
host += host;
host += host;

try {
  // raises uncatchable EBADNAME
  http.get({host:host, port:80}, console.error);
}
catch (e) {
  console.error('Caught exception', e);
}

As a gist: https://gist.github.com/1033563

[landeiro]

Originally reported here http://groups.google.com/group/nodejs/browse_thread/thread/8d23735bb6d2bae0?hl=en
:)

[dhruvbird]

This may be related to this one: #1164

[landeiro]

It seems that you're right, Dhruv.
Thanks.

[dhruvbird]

You're welcome :-) It would be great if you could:

1. confirm that by retrying the same HTTP request with http.request instead of http.get and seeing if the problem persists and
2. commenting on my ticket, requesting that the API semantics be changed to async uniformly.

Thanks!

[ry]

Yes, this is a problem... the http request is not yet attached to the connecting socket.

[landeiro]

Thanks a lot for the response, Ryan :)

So it will be an issue resolved in future versions?

I'm an using node.js for requesting multiple pages per second, and using
first the DNS validating is slowing down the amazing performance of the
original request :)

On Mon, Jun 20, 2011 at 11:58 AM, ry <
reply@reply.github.com>wrote:

Yes, this is a problem... the http request is not yet attached to the
connecting socket.

Reply to this email directly or view it on GitHub:
#1202 (comment)

Pedro Landeiro
http://www.linkedin.com/in/pedrolandeiro

[dhruvbird]

You can try using var req = http.request(blah) instead of var req = http.get(blah);

And then do:
req.on('error', blah_dash);
req.end();

This should solve your problem (at least for now).

[landeiro]

That was my original example, with http.request, as you can see here : http://groups.google.com/group/nodejs/browse_thread/thread/8d23735bb6d2bae0?hl=en

The problem remains :(

[bnoordhuis]

get() is a thin wrapper around request() so that's not surprising.

[dhruvbird]

yes, you are right. It goes through the same code path. I was hoping that the http module would create a net.Stream, attach error handlers and then try to connect(). Instead, it does a createConnection(), which doesn't give it enough time to attach the 'error' handler (or so I think).

[SaltwaterC]

The DNS workaround, as posted into the Google group thread may avoid the double resolving like this:

dns.lookup(opt.host, function (err, addr) {
  if (err) {
    // err code
  } else {
    // patches in the host header in order to please the HTTP/1.1 servers aka most of them
    opt.headers.host = opt.host;
    // makes the connection to the resolved address itself
    opt.host = addr;
    var req = http.request(opt);
    // [...]
  }
});

So far I found this to be the only reliable way around this bug. Tried the legacy interface (http.createClient()) but it doesn't support HTTPS anymore. It also lacks the abort() call.

It doesn't look very elegant, but it gets the job done.

[bnoordhuis]

This issue is still under investigation. Something that complicates the matter is that the test behaves differently with the legacy and libuv backends: legacy raises an uncatchable exception, libuv raises a catchable exception but doesn't seem to kill the event loop (so the script hangs). Related to test/simple/test-http-dns-fail.js failing with libuv.

[bnoordhuis]

@koichik: can you have a look at 8feb7d4 and 5fa8d0d? Solves the uncatchable exception problem in a different way by deferring the connect() call in net.createConnection() so the caller has a chance to register an error event listener.

Benefit of your approach: works for both HTTP and HTTPS (mine doesn't because of how the tls module works)
Benefit of my approach: also handles other connect-time errors

Applying both patches would defer the actual connection for two ticks, that seems like too much of a performance drain. Maybe we should catch exceptions and emit them as error events on the next tick?

Want to talk it over on IRC? Our patches are probably two sides of the same coin.

[koichik]

@bnoordhuis:

Benefit of my approach: also handles other connect-time errors

Does Error occur synchronously (immediately) other than DNS?
I think other connect-time errors that emit 'error' event are asynchronously via Socket.destroy().

Our patches are probably two sides of the same coin.

Yeah, the big difference is that my patch only invokes the callback asynchronously, whereas your patch invokes whole API (Socket.connect()) asynchronously.
If some error occurred synchronously, my patch does not solve the problem.
But if all errors occurred asynchronously, your patch is not needed.

Want to talk it over on IRC?

Oops, it is too hard to me. I can write only three statements per an hour!

[bnoordhuis]

@koichik: 60fd7e6 seems like the optimal solution (performance-wise, for this particular instance) where only the error event is deferred. Like / don't like?

[koichik]

Is there a performance problem?
If a host was a domain name, dns.lookup() invokes callback asynchronously in any patches (in this case, my patch does not use process.nextTick()).
Therefore I do not think that my patch makes a performance problem.

But if a host was an IP address, hmm... we do not need dns.lookup().
How about like this? (with my patch)

    // TCP
    var addrType = binding.isIP(host);
    if (addrType === 4 || addrType === 6) {
      timers.active(self);
      self.type = addressType == 4 ? 'tcp4' : 'tcp6';
      self.fd = socket(self.type);
      self.remoteAddress = host;
      self.remotePort = port;
      doConnect(self, port, host);
    } else {
      require('dns').lookup(host, function(err, ip, addressType) {
        if (err) {
          self.emit('error', err);
        } else {
          timers.active(self);
          self.type = addressType == 4 ? 'tcp4' : 'tcp6';
          self.fd = socket(self.type);
          self.remoteAddress = ip;
          self.remotePort = port;
          doConnect(self, port, ip);
        }
      });
    }

[bnoordhuis]

Way too much special casing there, koichik. And it's replicating DNS logic in the TCP module, do not want.

The thing I like about 60fd7e6 is that it's minimally intrusive - it fixes the bug and touches nothing else.

I'm not against making DNS fully async by the way - on the contrary! - but that's something for issue #1164.

[koichik]

@bnoordhuis

I'm not against making DNS fully async by the way

Good news! :)

but that's something for issue #1164.

I think #1164 is cause of #1202.
If #1164 was fixed, #1202 would also be fixed (Is it wrong?).
So... I do not know the reason why you want to apply both patches.

[koichik]

@bnoordhuis - I have changed my mind.
I am +1 to 60fd7e6. It should be landed into both master and v0.4.
After that, I will fix #1164 and #1456 on the master (not v0.4).
I think it is steady (defensive) way.

[bnoordhuis]

Sweet, landed in master and v0.4.