http: fix request with option timeout and agent

When request with both timeout and agent, timeout not
work. This patch will fix it, socket timeout will set
to request timeout before socket is connected, and
socket timeout will reset to agent timeout after
response end.

Update agent doc, add timeout option.

Fixes: #21185

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} A number specifying the 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 that it'll 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,43 @@
+'use strict';
+const common = require('../common');
+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 response
+});
+
+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 + 50));
+}

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

@@ -0,0 +1,48 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const http = require('http');
+
+const HTTP_AGENT_TIMEOUT = 1000;
+const HTTP_CLIENT_TIMEOUT = 2000;
+
+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 response
+});
+
+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;
+    assert((Math.abs(duration - HTTP_CLIENT_TIMEOUT) < 20));
+  }));
+  req.end();
+
+  setTimeout(function() {
+    req.destroy();
+    assert.strictEqual(timeout_events, 1);
+  }, common.platformTimeout(HTTP_CLIENT_TIMEOUT + 50));
+}