http: fix request with option timeout and agent

doc/api/http.md

@@ -126,6 +126,8 @@ added: v0.3.4
   * `maxFreeSockets` {number} Maximum number of sockets to leave open
     in a free state. Only relevant if `keepAlive` is set to `true`.
     **Default:** `256`.
+  * `timeout` {number} Socket timeout in milliseconds.
+    This will set the timeout after the socket is connected.
 
 The default [`http.globalAgent`][] that is used by [`http.request()`][] has all
 of these values set to their respective defaults.

lib/_http_agent.js

@@ -66,7 +66,8 @@ function Agent(options) {
 
     if (socket.writable &&
         this.requests[name] && this.requests[name].length) {
-      this.requests[name].shift().onSocket(socket);
+      const req = this.requests[name].shift();
+      setRequestSocket(this, req, socket);
       if (this.requests[name].length === 0) {
         // don't leak
         delete this.requests[name];
@@ -176,12 +177,12 @@ Agent.prototype.addRequest = function addRequest(req, options, port/* legacy */,
       delete this.freeSockets[name];
 
     this.reuseSocket(socket, req);
-    req.onSocket(socket);
+    setRequestSocket(this, req, socket);
     this.sockets[name].push(socket);
   } else if (sockLen < this.maxSockets) {
     debug('call onSocket', sockLen, freeLen);
     // If we are under maxSockets create a new one.
-    this.createSocket(req, options, handleSocketCreation(req, true));
+    this.createSocket(req, options, handleSocketCreation(this, req, true));
   } else {
     debug('wait for socket');
     // We are over limit so we'll add it to the queue.
@@ -305,9 +306,10 @@ Agent.prototype.removeSocket = function removeSocket(s, options) {
 
   if (this.requests[name] && this.requests[name].length) {
     debug('removeSocket, have a request, make a socket');
-    var req = this.requests[name][0];
+    const req = this.requests[name][0];
     // If we have pending requests and a socket gets closed make a new one
-    this.createSocket(req, options, handleSocketCreation(req, false));
+    const socketCreationHandler = handleSocketCreation(this, req, false);
+    this.createSocket(req, options, socketCreationHandler);
   }
 };
 
@@ -337,19 +339,36 @@ Agent.prototype.destroy = function destroy() {
   }
 };
 
-function handleSocketCreation(request, informRequest) {
+function handleSocketCreation(agent, request, informRequest) {
   return function handleSocketCreation_Inner(err, socket) {
     if (err) {
       process.nextTick(emitErrorNT, request, err);
       return;
     }
     if (informRequest)
-      request.onSocket(socket);
+      setRequestSocket(agent, request, socket);
     else
       socket.emit('free');
   };
 }
 
+function setRequestSocket(agent, req, socket) {
+  req.onSocket(socket);
+  const agentTimeout = agent.options.timeout || 0;
+  if (req.timeout === undefined || req.timeout === agentTimeout) {
+    return;
+  }
+  socket.setTimeout(req.timeout);
+  // reset timeout after response end
+  req.once('response', (res) => {
+    res.once('end', () => {
+      if (socket.timeout !== agentTimeout) {
+        socket.setTimeout(agentTimeout);
+      }
+    });
+  });
+}
+
 function emitErrorNT(emitter, err) {
   emitter.emit('error', err);
 }

lib/_http_client.js

@@ -647,18 +647,35 @@ function tickOnSocket(req, socket) {
   socket.on('end', socketOnEnd);
   socket.on('close', socketCloseListener);
 
-  if (req.timeout) {
-    const emitRequestTimeout = () => req.emit('timeout');
-    socket.once('timeout', emitRequestTimeout);
-    req.once('response', (res) => {
-      res.once('end', () => {
-        socket.removeListener('timeout', emitRequestTimeout);
-      });
-    });
+  if (req.timeout !== undefined) {
+    listenSocketTimeout(req);
   }
   req.emit('socket', socket);
 }
 
+function listenSocketTimeout(req) {
+  if (req.timeoutCb) {
+    return;
+  }
+  const emitRequestTimeout = () => req.emit('timeout');
+  // Set timeoutCb so it will get cleaned up on request end.
+  req.timeoutCb = emitRequestTimeout;
+  // Delegate socket timeout event.
+  if (req.socket) {
+    req.socket.once('timeout', emitRequestTimeout);
+  } else {
+    req.on('socket', (socket) => {
+      socket.once('timeout', emitRequestTimeout);
+    });
+  }
+  // Remove socket timeout listener after response end.
+  req.once('response', (res) => {
+    res.once('end', () => {
+      req.socket.removeListener('timeout', emitRequestTimeout);
+    });
+  });
+}
+
 ClientRequest.prototype.onSocket = function onSocket(socket) {
   process.nextTick(onSocketNT, this, socket);
 };
@@ -708,42 +725,29 @@ function _deferToConnect(method, arguments_, cb) {
 }
 
 ClientRequest.prototype.setTimeout = function setTimeout(msecs, callback) {
+  listenSocketTimeout(this);
   msecs = validateTimerDuration(msecs);
   if (callback) this.once('timeout', callback);
 
-  const emitTimeout = () => this.emit('timeout');
-
-  if (this.socket && this.socket.writable) {
-    if (this.timeoutCb)
-      this.socket.setTimeout(0, this.timeoutCb);
-    this.timeoutCb = emitTimeout;
-    this.socket.setTimeout(msecs, emitTimeout);
-    return this;
-  }
-
-  // Set timeoutCb so that it'll get cleaned up on request end
-  this.timeoutCb = emitTimeout;
   if (this.socket) {
-    var sock = this.socket;
-    this.socket.once('connect', function() {
-      sock.setTimeout(msecs, emitTimeout);
-    });
-    return this;
+    setSocketTimeout(this.socket, msecs);
+  } else {
+    this.once('socket', (sock) => setSocketTimeout(sock, msecs));
   }
 
-  this.once('socket', function(sock) {
-    if (sock.connecting) {
-      sock.once('connect', function() {
-        sock.setTimeout(msecs, emitTimeout);
-      });
-    } else {
-      sock.setTimeout(msecs, emitTimeout);
-    }
-  });
-
   return this;
 };
 
+function setSocketTimeout(sock, msecs) {
+  if (sock.connecting) {
+    sock.once('connect', function() {
+      sock.setTimeout(msecs);
+    });
+  } else {
+    sock.setTimeout(msecs);
+  }
+}
+
 ClientRequest.prototype.setNoDelay = function setNoDelay(noDelay) {
   this._deferToConnect('setNoDelay', [noDelay]);
 };

lib/net.js

@@ -405,6 +405,7 @@ function writeAfterFIN(chunk, encoding, cb) {
 }
 
 Socket.prototype.setTimeout = function(msecs, callback) {
+  this.timeout = msecs;
   // Type checking identical to timers.enroll()
   msecs = validateTimerDuration(msecs);
 

test/parallel/test-http-client-set-timeout.js

@@ -0,0 +1,46 @@
+'use strict';
+const common = require('../common');
+
+// Test that `req.setTimeout` will fired exactly once.
+
+const assert = require('assert');
+const http = require('http');
+
+const HTTP_CLIENT_TIMEOUT = 2000;
+
+const options = {
+  method: 'GET',
+  port: undefined,
+  host: '127.0.0.1',
+  path: '/',
+  timeout: HTTP_CLIENT_TIMEOUT,
+};
+
+const server = http.createServer(() => {
+  // Never respond.
+});
+
+server.listen(0, options.host, function() {
+  doRequest();
+});
+
+function doRequest() {
+  options.port = server.address().port;
+  const req = http.request(options);
+  req.setTimeout(HTTP_CLIENT_TIMEOUT / 2);
+  req.on('error', () => {
+    // This space is intentionally left blank.
+  });
+  req.on('close', common.mustCall(() => server.close()));
+
+  let timeout_events = 0;
+  req.on('timeout', common.mustCall(() => {
+    timeout_events += 1;
+  }));
+  req.end();
+
+  setTimeout(function() {
+    req.destroy();
+    assert.strictEqual(timeout_events, 1);
+  }, common.platformTimeout(HTTP_CLIENT_TIMEOUT));
+}

test/parallel/test-http-client-timeout-option-with-agent.js

@@ -0,0 +1,55 @@
+'use strict';
+const common = require('../common');
+
+// Test that when http request uses both timeout and agent,
+// timeout will work as expected.
+
+const assert = require('assert');
+const http = require('http');
+
+const HTTP_AGENT_TIMEOUT = 1000;
+const HTTP_CLIENT_TIMEOUT = 3000;
+
+const agent = new http.Agent({ timeout: HTTP_AGENT_TIMEOUT });
+const options = {
+  method: 'GET',
+  port: undefined,
+  host: '127.0.0.1',
+  path: '/',
+  timeout: HTTP_CLIENT_TIMEOUT,
+  agent,
+};
+
+const server = http.createServer(() => {
+  // Never respond.
+});
+
+server.listen(0, options.host, function() {
+  doRequest();
+});
+
+function doRequest() {
+  options.port = server.address().port;
+  const req = http.request(options);
+  const start = Date.now();
+  req.on('error', () => {
+    // This space is intentionally left blank.
+  });
+  req.on('close', common.mustCall(() => server.close()));
+
+  let timeout_events = 0;
+  req.on('timeout', common.mustCall(() => {
+    timeout_events += 1;
+    const duration = Date.now() - start;
+    // The timeout event cannot be precisely timed. It will delay
+    // some number of milliseconds, so test it in second units.
+    assert.strictEqual(duration / 1000 | 0, HTTP_CLIENT_TIMEOUT / 1000);
+  }));
+  req.end();
+
+  setTimeout(function() {
+    req.destroy();
+    assert.strictEqual(timeout_events, 1);
+    // Ensure the `timeout` event fired only once.
+  }, common.platformTimeout(HTTP_CLIENT_TIMEOUT * 2));
+}