http.ClientRequest.setTimeout does not timeout if dns.lookup exceeds the timeout

[owenallenaz]

• Version:v6.9.2
• Platform:Linux nodeServer 2.6.32-573.el6.x86_64 deps: update openssl to 1.0.1j #1 SMP Thu Jul 23 15:44:03 UTC 2015 x86_64 x86_64 x86_64 GNU/Linux
• Subsystem:http

This issue arises from a separate issue regarding dns.lookup #8436 potentially blocking the libuv threadpool. Right now if you do a setTimeout on a ClientRequest it does not include the dns resolution in that period. In example if you have a req.setTimeout(100) and dns resolution takes 5000, it will not timeout. This is explained in the docs because req.setTimeout() is simply a pass-through to socket.setTimeout().

That being said, if a user does http.request().setTimeout(100) the user-intent in my opinion is that if that request, in it's entirety, is not complete in 100ms, then it should pass a timeout error, irregardless of what part of the dns/tcp/http phase the failure occurs in.

Replication gist - https://gist.github.com/owenallenaz/b98ce2b9b491243e76dd517e0fdc46ca

[cjihrig]

Are you sure this is a bug, and not just a missing 'timeout' handler? For example, the following code emits the 'timeout' event when DNS lookup takes longer than the socket's timeout (I left the ability to experiment with the duration of dns.lookup() and the server response):

'use strict';
const dns = require('dns');
const http = require('http');
const lookup = dns.lookup;

dns.lookup = function patchedLookup(...args) {
  setTimeout(() => { lookup.apply(this, args); }, 1000);
  // return lookup.apply(this, args);
};

const server = http.createServer((req, res) => {
  // setTimeout(() => { res.end('ok'); }, 1000);
  res.end('ok');
});

server.listen(0, () => {
  const req = http.request({
    port: server.address().port
  }, function(res) {
    let data = "";

    res.on('data', (chunk) => { data += chunk; });
    res.on('end', () => {
      console.log(data);
      server.close();
    });
  });

  req.on('timeout', () => {
    console.error('timed out');
  });

  req.on('error', (err) => {
    console.error(err);
  });

  req.setTimeout(500);
  req.end();
});

[owenallenaz]

Looks like you are correct. I experimented with your mock as well as running a local dns server with low responses and both behaved the same way. I believe I was mistakenly thinking that timeout events would propagate to the error handler. The oddity is that I was using request at the time, and simplified it down to this case. With request it does pass timeouts on to the err handler. So it appears I mistaken. Thanks for looking in to it. Closing.