http: destroy timeout socket by Agent

fengmk2 · fengmk2 · commit be95679e3ad8 · 2019-12-21T11:58:25.000+08:00
Agent must destroy timeout socket when there is no any timeout
handler. Avoid socket hang on forever when the server don't send
any response back.
diff --git a/doc/api/errors.md b/doc/api/errors.md
@@ -1912,6 +1912,11 @@ removed: v10.0.0
 Used when an invalid character is found in an HTTP response status message
 (reason phrase).
 
+<a id="ERR_HTTP_SOCKET_TIMEOUT"></a>
+### ERR_HTTP_SOCKET_TIMEOUT
+
+A socket timed out, it's triggered by HTTP.
+
 <a id="ERR_INDEX_OUT_OF_RANGE"></a>
 ### ERR_INDEX_OUT_OF_RANGE
 <!-- YAML
diff --git a/doc/api/http.md b/doc/api/http.md
@@ -692,6 +692,9 @@ added: v0.5.9
 Once a socket is assigned to this request and is connected
 [`socket.setTimeout()`][] will be called.
 
+Emitted when the underlying socket times out from inactivity.
+This only notifies that the socket has been idle. The request must be aborted manually.
+
 ### request.socket
 <!-- YAML
 added: v0.3.0
diff --git a/lib/_http_agent.js b/lib/_http_agent.js
@@ -26,6 +26,10 @@ const util = require('util');
 const EventEmitter = require('events');
 const debug = util.debuglog('http');
 const { async_id_symbol } = require('internal/async_hooks').symbols;
+const errors = require('internal/errors');
+const {
+  ERR_HTTP_SOCKET_TIMEOUT,
+} = errors.codes;
 
 // New Agent code.
 
@@ -268,6 +272,24 @@ function installListeners(agent, s, options) {
   }
   s.on('close', onClose);
 
+  function onTimeout() {
+    debug('CLIENT socket onTimeout after', s.timeout, 'ms');
+    const name = agent.getName(options);
+    // If free socket timeout, must remove it from freeSockets list immediately
+    // to prevent new requests from being sent through this timeout socket.
+    if (agent.freeSockets[name] && agent.freeSockets[name].indexOf(s) !== -1) {
+      s.destroy();
+      agent.removeSocket(s, options);
+      debug('CLIENT free socket destroy');
+    } else if (s.listeners('timeout').length === 1) {
+      // No req timeout handler, agent must destroy the socket.
+      s.destroy(new ERR_HTTP_SOCKET_TIMEOUT());
+      agent.removeSocket(s, options);
+      debug('CLIENT active socket destroy');
+    }
+  }
+  s.on('timeout', onTimeout);
+
   function onRemove() {
     // We need this function for cases like HTTP 'upgrade'
     // (defined by WebSockets) where we need to remove a socket from the
@@ -276,6 +298,7 @@ function installListeners(agent, s, options) {
     agent.removeSocket(s, options);
     s.removeListener('close', onClose);
     s.removeListener('free', onFree);
+    s.removeListener('timeout', onTimeout);
     s.removeListener('agentRemove', onRemove);
   }
   s.on('agentRemove', onRemove);
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
@@ -664,6 +664,7 @@ E('ERR_HTTP_HEADERS_SENT',
 E('ERR_HTTP_INVALID_HEADER_VALUE',
   'Invalid value "%s" for header "%s"', TypeError);
 E('ERR_HTTP_INVALID_STATUS_CODE', 'Invalid status code: %s', RangeError);
+E('ERR_HTTP_SOCKET_TIMEOUT', 'Socket timeout', Error);
 E('ERR_HTTP_TRAILER_INVALID',
   'Trailers are invalid with this transfer encoding', Error);
 E('ERR_INSPECTOR_ALREADY_CONNECTED', '%s is already connected', Error);
diff --git a/test/parallel/test-gc-http-client-timeout.js b/test/parallel/test-gc-http-client-timeout.js
@@ -5,6 +5,7 @@
 
 const common = require('../common');
 const onGC = require('../common/ongc');
+const assert = require('assert');
 
 function serverHandler(req, res) {
   setTimeout(function() {
@@ -38,6 +39,11 @@ function getall() {
   req.setTimeout(10, function() {
     console.log('timeout (expected)');
   });
+  req.on('error', (err) => {
+    // only allow Socket timeout error
+    assert.strictEqual(err.code, 'ERR_SOCKET_TIMEOUT');
+    assert.strictEqual(err.message, 'Socket timeout');
+  });
 
   count++;
   onGC(req, { ongc });
diff --git a/test/parallel/test-http-client-timeout-option-listeners.js b/test/parallel/test-http-client-timeout-option-listeners.js
@@ -24,9 +24,10 @@ const options = {
 server.listen(0, options.host, common.mustCall(() => {
   options.port = server.address().port;
   doRequest(common.mustCall((numListeners) => {
-    assert.strictEqual(numListeners, 1);
+    // including the default onTimeout on Agent
+    assert.strictEqual(numListeners, 2);
     doRequest(common.mustCall((numListeners) => {
-      assert.strictEqual(numListeners, 1);
+      assert.strictEqual(numListeners, 2);
       server.close();
       agent.destroy();
     }));
diff --git a/test/sequential/test-http-custom-timeout-handler.js b/test/sequential/test-http-custom-timeout-handler.js
@@ -0,0 +1,26 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const http = require('http');
+
+const server = http.createServer(common.mustCall((req, res) => {
+  server.close();
+  // do nothing, wait client socket timeout and close
+}));
+
+server.listen(0, common.mustCall(() => {
+  const agent = new http.Agent({ timeout: 100 });
+  const req = http.get({
+    path: '/',
+    port: server.address().port,
+    timeout: 50,
+    agent
+  }, common.mustNotCall())
+  .on('error', common.mustCall((err) => {
+    assert.strictEqual(err.message, 'socket hang up');
+    assert.strictEqual(err.code, 'ECONNRESET');
+  }));
+  req.on('timeout', common.mustCall(() => {
+    req.abort();
+  }));
+}));
diff --git a/test/sequential/test-http-default-timeout-handler.js b/test/sequential/test-http-default-timeout-handler.js
@@ -0,0 +1,29 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const http = require('http');
+
+const server = http.createServer(common.mustCall((req, res) => {
+  server.close();
+  // do nothing, wait client socket timeout and close
+}));
+
+server.listen(0, common.mustCall(() => {
+  let req;
+  const timer = setTimeout(() => {
+    req.abort();
+    assert.fail('should not timeout here');
+  }, 100);
+
+  const agent = new http.Agent({ timeout: 50 });
+  req = http.get({
+    path: '/',
+    port: server.address().port,
+    agent
+  }, common.mustNotCall())
+  .on('error', common.mustCall((err) => {
+    clearTimeout(timer);
+    assert.strictEqual(err.message, 'Socket timeout');
+    assert.strictEqual(err.code, 'ERR_HTTP_SOCKET_TIMEOUT');
+  }));
+}));
diff --git a/test/sequential/test-http-request-on-free-socket-timeout.js b/test/sequential/test-http-request-on-free-socket-timeout.js
@@ -0,0 +1,52 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const http = require('http');
+
+const server = http.createServer(common.mustCallAtLeast((req, res) => {
+  const content = 'Hello Agent';
+  res.writeHead(200, {
+    'Content-Length': content.length.toString(),
+  });
+  res.write(content);
+  res.end();
+}, 2));
+
+server.listen(0, common.mustCall(() => {
+  const agent = new http.Agent({ timeout: 100, keepAlive: true });
+  const req = http.get({
+    path: '/',
+    port: server.address().port,
+    agent
+  }, common.mustCall((res) => {
+    assert.strictEqual(res.statusCode, 200);
+    res.resume();
+    res.on('end', common.mustCall());
+  }));
+
+  const timer = setTimeout(() => {
+    assert.fail('should not timeout here');
+    req.abort();
+  }, 1000);
+
+  req.on('socket', common.mustCall((socket) => {
+    // wait free socket become free and timeout
+    socket.on('timeout', common.mustCall(() => {
+      // free socket should be destroyed
+      assert.strictEqual(socket.writable, false);
+      // send new request will be fails
+      clearTimeout(timer);
+      const newReq = http.get({
+        path: '/',
+        port: server.address().port,
+        agent
+      }, common.mustCall((res) => {
+        // agent must create a new socket to handle request
+        assert.notStrictEqual(newReq.socket, socket);
+        assert.strictEqual(res.statusCode, 200);
+        res.resume();
+        res.on('end', common.mustCall(() => server.close()));
+      }));
+    }));
+  }));
+}));
