HTTP and HTTPS attempt only single IP, causing intermittent failures

[jsha]

Steps to reproduce:

Find a host that answers a given port only on some of the returned IP addresses for its name. For instance, www.itunes.com has this problem. Then run this code:

require('https').get('https://www.itunes.com', function(res) {
  console.log('statusCode:', res.statusCode);
}).on('error', function(err) {
  console.log('Error:', err);
});

Since www.itunes.com resolves to three different IP addresses (see dig www.itunes.com) in random order, io.js will connect to a random host each time, sometimes failing and sometimes succeeding.

Arguably, this is broken behavior on the part of the host. However, it's reasonably common on the web. Firefox and Chrome each attempt parallel connections to all IP addresses provided in the DNS response, using the first connection to succeed. Curl attempts each IP address in sequence until it gets a successful connection or runs out of IP addresses: https://github.com/bagder/curl/blob/master/lib/connect.c#L1156

It looks like the io.js http and https libraries, through Agent, call into Socket.connect, which uses dns.lookup (returns just one IP address) instead of dns.resolve: https://github.com/iojs/io.js/blob/v1.x/lib/net.js#L900.

Is it practical to change the Socket.connect behavior to use dns.resolve and try multiple IP addresses? I'm guessing that would be a fairly API-incompatible change. Another possibility: Agent could implement failure handling code that specifically catched connection refused and timeout errors, calls dns.resolve itself, and calls Socket.connect again with IP address arguments. This would be less performant but also a smaller API change.

For the real-world motivating example, I ran into this when writing automated tests for HTTPS Everywhere's rulesets: https://github.com/jsha/https-everywhere/blob/rules-tester/rewriter/tester.js. As-is, I get a number of false positives from hosts that appear to fail but actual work fine in a browser.

[Fishrock123]

Slightly better test code:

var https = require('https')
function get() {
  https.get('https://www.itunes.com', function(res) {
    console.log('statusCode: %s & ip: %s', res.statusCode, res.connection.remoteAddress);
  }).on('error', function(err) {
    console.log('Error:', err);
  });
}

[bnoordhuis]

It looks like the io.js http and https libraries, through Agent, call into Socket.connect, which uses dns.lookup (returns just one IP address) instead of dns.resolve

The reason for dns.lookup() is that it uses the system resolver, which may have hooks for things like mDNS. dns.resolve() is a pure DNS resolver that just queries the servers in /etc/resolv.conf.

Is it practical to change the Socket.connect behavior to use dns.resolve and try multiple IP addresses?

That would be a backwards incompatible change and it would be detrimental to what is probably the common case: that the server is really down.

I'm leaning towards that this is working as intended and that more advanced error handling is best left to the user. I am sympathetic to the 'it should just work' argument but I don't see a way of doing it that doesn't have downsides.

[jsha]

It's challenging to do more sophisticated error handling above the https library level because if you pass an IP address to https.get(), certificate checking will fail.

I would argue that libcurl is a good reference implementation for non-interactive HTTP fetchers, and io.js fetching should aim to work in the same way.

What about changing only the http and https libraries to implement retry logic? Since the https library interacts directly with the tls library, it can pass an IP address and servername option to tls.connect, which I think fixes the certificate validation issues.

For mDNS and other things that don't show up in dns.resolve: What about calling both dns.lookup first, for backwards compatibility, then dns.resolve after a connection failure to see if there are other addresses available?

Alternately, I think getaddrinfo, on which dns.lookup is based, can return multiple addresses from the system resolver, right? Why not specify an alternate version of dns.lookup that returns all the addresses?

[meandmycode]

getaddrinfo does indeed appear to return multiple addresses so this seems like a good addition on it's own anyway.

In terms of HTTP and HTTPS having retry built in, I'm personally not so sure, it may improve a sense of stability in a lot of uses, but it also may be unexpected behaviour, this seems more like a layered behaviour.

That said, the way .NET solves this is with a delegate which could translate roughly to something like this:

var http = require('http');

var req = https.get('https://www.itunes.com', function(res) {
   // ...
});

// addresses is an array of IPs from dns.lookupAll
// retries represents how many times the handler has been called in order to make a connection
req.setBindingHandler(function(addresses, retries) {
    if (retries > 3 || retries > addresses.length) return null; // returning null would emit an error from req
    return addresses[retries]; // basic sequential
});

This allows developers to control the behaviour a little more, you could introduce some standard behaviours too such as:

var http = require('http');

var req = https.get('https://www.itunes.com', function(res) {
   // ...
});

req.setBindingHandler(http.roundRobinBinding({ maxRetries: 3 }));
// -or-
req.setBindingHandler(http.randomBinding({ maxRetries: 3 }));

The only thing I'm not sure about is race condition on the handler being set before the request starts and DNS lookup begins, but this seems like it would also cause issues with methods like req.setTimeout also, so perhaps node/iojs does an equivalent of setImmediate here?

Thanks,

[jsha]

I don't think the behavior I suggested is unexpected. In addition to the browser examples, curl, and wget, I tested Ruby's Net::HTTP and Python's Requests with Wireshark, and they also attempt parallel simultaneous connections and use the first to complete. The list so far is:

Firefox: parallel
Chrome: parallel
Ruby: parallel
Python: parallel
curl: sequential retries
wget: sequential retries

There is also RFC 6555, "Happy Eyeballs," describing the parallel connection algorithm in the context of mixed IPv4 and IPv6 connectivity. So I think this is generally accepted correct behavior for HTTP and HTTPS.

I filed a separate issue, #736, suggesting a new method dns.lookupAll.

[meandmycode]

I'm +1 on implementing this, I think it makes sense and seems mature for node to at least offer this capability instead of just selecting the first IP or nothing.

I see things like curl/wget/ruby as layered frameworks/apps, which is why I compared instead to the .NET core NET framework or Java (which I think node core correlates to better), their design is to default to the first IP but easily allow user code to customize this behaviour (as with the example API I gave above).

That said, these frameworks were designed long before the web of today, and I think it could make sense to have this behaviour as default.

[silverwind]

That would be a backwards incompatible change and it would be detrimental to what is probably the common case: that the server is really down.

I wouldn't say it's a incompatible change per se, as it usually benefits the application. The masking of a potential server outage is a bit unfortunate, but I'd say the benefits outweight the drawbacks here. I think it only makes sense to implement this in net.

[silverwind]

Any more +1/-1 from the TC if this should be added? I might give it a shot, if there's enough support.

[rvagg]

I think like @meandmycode's suggestions here. I don't think this is something that belongs in core but the fact that we can't easily support userland code to do something similar is a problem which we should address. request, hyperquest, superagent, etc. are all client libraries that people tend to reach for and they should be the ones that implement the specific behaviour desired by their creators and the communities using them, core should just be an enabler for this and nothing more.

[domenic]

Is this a regression or has this been the behavior since the early days of Node?