http: set default timeout in agent keepSocketAlive

Previous location of setting the timeout would override
behaviour of custom HttpAgents' keepSocketAlive. Moving
it into the default keepSocketAlive allows it to
interoperate with custom agents.

Fixes: nodejs#33111

PR-URL: nodejs#33127
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>

Author: omsmith

doc/api/errors.md

@@ -2274,6 +2274,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

doc/api/http.md

@@ -860,6 +860,9 @@ changes:
 Once a socket is assigned to this request and is connected
 [`socket.setTimeout()`][] will be called.
 
+If no `'timeout'` listener is registered the request will error
+with `ERR_HTTP_SOCKET_TIMEOUT`.
+
 ### `request.socket`
 <!-- YAML
 added: v0.3.0

lib/_http_agent.js

@@ -34,6 +34,7 @@ const debug = require('internal/util/debuglog').debuglog('http');
 const { async_id_symbol } = require('internal/async_hooks').symbols;
 const {
   codes: {
+    ERR_HTTP_SOCKET_TIMEOUT,
     ERR_INVALID_ARG_TYPE,
   },
 } = require('internal/errors');
@@ -339,8 +340,13 @@ function installListeners(agent, s, options) {
   function onTimeout() {
     debug('CLIENT socket onTimeout');
 
+    if (s.listenerCount('timeout') === 1) {
+      // No req timeout handler, agent must destroy the socket.
+      s.destroy(new ERR_HTTP_SOCKET_TIMEOUT());
+      return;
+    }
+
     // Destroy if in free list.
-    // TODO(ronag): Always destroy, even if not in free list.
     const sockets = agent.freeSockets;
     for (const name of ObjectKeys(sockets)) {
       if (sockets[name].includes(s)) {

lib/internal/errors.js

@@ -915,6 +915,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_INCOMPATIBLE_OPTION_PAIR',

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 });

test/parallel/test-http-agent-timeout.js

@@ -132,3 +132,43 @@ const http = require('http');
       }));
   }));
 }
+
+{
+  // Ensure custom keepSocketAlive timeout is respected
+
+  const CUSTOM_TIMEOUT = 60;
+  const AGENT_TIMEOUT = 50;
+
+  class CustomAgent extends http.Agent {
+    keepSocketAlive(socket) {
+      if (!super.keepSocketAlive(socket)) {
+        return false;
+      }
+
+      socket.setTimeout(CUSTOM_TIMEOUT);
+      return true;
+    }
+  }
+
+  const agent = new CustomAgent({ keepAlive: true, timeout: AGENT_TIMEOUT });
+
+  const server = http.createServer((req, res) => {
+    res.end();
+  });
+
+  server.listen(0, common.mustCall(() => {
+    http.get({ port: server.address().port, agent })
+      .on('response', common.mustCall((res) => {
+        const socket = res.socket;
+        assert(socket);
+        res.resume();
+        socket.on('free', common.mustCall(() => {
+          socket.on('timeout', common.mustCall(() => {
+            assert.strictEqual(socket.timeout, CUSTOM_TIMEOUT);
+            agent.destroy();
+            server.close();
+          }));
+        }));
+      }));
+  }));
+}

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();
+  }));
+}));

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');
+  }));
+}));

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()));
+      }));
+    }));
+  }));
+}));