request.setTimeout should be used for inactivity

[WestonThayer]

follow-redirects/index.js

Line 207 in 6f5029a

this.on("response", clearTimer);

In the standard http module, ClientRequest.setTimeout() is passed down to net.Socket.setTimeout(), which fires the timeout event if the socket is idle long enough. timeout could fire if the server doesn't respond fast enough, but could also fire for a multi-part response with a big delay between data packets. setTimeout does not look at the whole request lifecycle from start to finish (i.e. you could have a timeout of 1000ms with a server sending data every 900ms — the entire request could take > 1000ms, but since the socket is still receiving data < 1000ms between packets, timeout will not fire).

If the goal is to mimic http's API, we need to continue monitoring for a timeout, even after the very first response.

To reproduce:

const http = require("http");
const followRedirects = require("follow-redirects");

const server = http.createServer((req, res) => {
  res.write("partial response");
  setTimeout(() => res.end(), 2000);
});
server.listen(3000);

const req = followRedirects.http.request({
  port: 3000,
  // Specifying timeout here causes a different result than setTimeout (but it shouldn't)
});
req.setTimeout(1000, () => {
  console.log("timed out"); // Will not be logged
  req.end();
  server.close();
});
req.end();

Maybe a fix would pass the timeout down to each underlying socket in the redirect chain and emit RedirectableRequest's timeout event if any underlying socket emitted?

[RubenVerborgh]

Super, that's very helpful. The current timeout semantics are indeed not the ones you mention; I wonder if you have an official source for this desired behavior? The Node.js documentation is vague on it.

[WestonThayer]

request.setTimeout refers to socket.setTimeout, which says, emphasis mine:

Sets the socket to timeout after timeout milliseconds of inactivity on the socket.

You can also simply change my sample code about to use native http.request and observe how timeout is fired.