http: destroy sockets after keepAliveTimeout

Implement server.keepAliveTimeout in addition to server.timeout to
prevent temporary socket/memory leaking in keep-alive mode.

PR-URL: #2534
Author: Timur Shemsedinov <timur.shemsedinov@gmail.com>
Author: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>

Author: tshemsedinov

doc/api/http.md

@@ -873,17 +873,33 @@ customised.
 added: v0.9.12
 -->
 
-* {Number} Default = 120000 (2 minutes)
+* {number} Timeout in milliseconds. Defaults to 120000 (2 minutes).
 
 The number of milliseconds of inactivity before a socket is presumed
 to have timed out.
 
-Note that the socket timeout logic is set up on connection, so
-changing this value only affects *new* connections to the server, not
-any existing connections.
+A value of 0 will disable the timeout behavior on incoming connections.
 
-Set to 0 to disable any kind of automatic timeout behavior on incoming
-connections.
+*Note*: The socket timeout logic is set up on connection, so changing this
+value only affects new connections to the server, not any existing connections.
+
+### server.keepAliveTimeout
+<!-- YAML
+added: REPLACEME
+-->
+
+* {number} Timeout in milliseconds. Defaults to 5000 (5 seconds).
+
+The number of milliseconds of inactivity a server needs to wait for additional
+incoming data, after it has finished writing the last response, before a socket
+will be destroyed. If the server receives new data before the keep-alive
+timeout has fired, it will reset the regular inactivity timeout, i.e.,
+[`server.timeout`][].
+
+A value of 0 will disable the keep-alive timeout behavior on incoming connections.
+
+*Note*: The socket timeout logic is set up on connection, so changing this
+value only affects new connections to the server, not any existing connections.
 
 ## Class: http.ServerResponse
 <!-- YAML
@@ -1739,6 +1755,7 @@ There are a few special headers that should be noted.
 [`response.write(data, encoding)`]: #http_response_write_chunk_encoding_callback
 [`response.writeContinue()`]: #http_response_writecontinue
 [`response.writeHead()`]: #http_response_writehead_statuscode_statusmessage_headers
+[`server.timeout`]: #http_server_timeout
 [`socket.setKeepAlive()`]: net.html#net_socket_setkeepalive_enable_initialdelay
 [`socket.setNoDelay()`]: net.html#net_socket_setnodelay_nodelay
 [`socket.setTimeout()`]: net.html#net_socket_settimeout_timeout_callback

doc/api/https.md

@@ -46,6 +46,14 @@ added: v0.11.2
 
 See [`http.Server#timeout`][].
 
+### server.keepAliveTimeout
+<!-- YAML
+added: REPLACEME
+-->
+- {number} Defaults to 5000 (5 seconds).
+
+See [`http.Server#keepAliveTimeout`][].
+
 ## https.createServer(options[, requestListener])
 <!-- YAML
 added: v0.3.4
@@ -240,6 +248,7 @@ const req = https.request(options, (res) => {
 [`globalAgent`]: #https_https_globalagent
 [`http.Agent`]: http.html#http_class_http_agent
 [`http.Server#headersTimeout`]: http.html#http_server_headerstimeout
+[`http.Server#keepAliveTimeout`]: http.html#http_server_keepalivetimeout
 [`http.close()`]: http.html#http_server_close_callback
 [`http.get()`]: http.html#http_http_get_options_callback
 [`http.listen()`]: http.html#http_server_listen_port_hostname_backlog_callback

lib/_http_server.js

@@ -243,6 +243,7 @@ function Server(requestListener) {
   this.addListener('connection', connectionListener);
 
   this.timeout = 2 * 60 * 1000;
+  this.keepAliveTimeout = 5000;
 
   this._pendingResponseData = 0;
   this.headersTimeout = 40 * 1000; // 40 seconds
@@ -316,6 +317,8 @@ function connectionListener(socket) {
       socket.destroy();
   });
 
+  socket._keepAliveTimeoutSet = false;
+
   var parser = parsers.alloc();
   parser.reinitialize(HTTPParser.REQUEST);
   parser.socket = socket;
@@ -371,6 +374,15 @@ function connectionListener(socket) {
   function socketOnData(d) {
     assert(!socket._paused);
     debug('SERVER socketOnData %d', d.length);
+
+    if (socket._keepAliveTimeoutSet) {
+      socket.setTimeout(0);
+      if (self.timeout) {
+        socket.setTimeout(self.timeout);
+      }
+      socket._keepAliveTimeoutSet = false;
+    }
+
     var ret = parser.execute(d);
 
     onParserExecuteCommon(ret, d);
@@ -535,6 +547,12 @@ function connectionListener(socket) {
 
       if (res._last) {
         socket.destroySoon();
+      } else if (outgoing.length === 0) {
+        if (self.keepAliveTimeout) {
+          socket.setTimeout(0);
+          socket.setTimeout(self.keepAliveTimeout);
+          socket._keepAliveTimeoutSet = true;
+        }
       } else {
         // start sending the next message
         var m = outgoing.shift();

lib/https.js

@@ -37,7 +37,7 @@ function Server(opts, requestListener) {
   });
 
   this.timeout = 2 * 60 * 1000;
-
+  this.keepAliveTimeout = 5000;
   this.headersTimeout = 40 * 1000; // 40 seconds
 }
 inherits(Server, tls.Server);

test/parallel/test-http-server-keep-alive-timeout.js

@@ -0,0 +1,95 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const http = require('http');
+const net = require('net');
+
+const tests = [];
+
+function test(fn) {
+  if (!tests.length) {
+    process.nextTick(run);
+  }
+  tests.push(fn);
+}
+
+function run() {
+  const fn = tests.shift();
+  if (fn) fn(run);
+}
+
+test(function serverEndKeepAliveTimeoutWithPipeline(cb) {
+  let socket;
+  let destroyedSockets = 0;
+  let timeoutCount = 0;
+  let requestCount = 0;
+  process.on('exit', () => {
+    assert.strictEqual(timeoutCount, 1);
+    assert.strictEqual(requestCount, 3);
+    assert.strictEqual(destroyedSockets, 1);
+  });
+  const server = http.createServer((req, res) => {
+    socket = req.socket;
+    requestCount++;
+    res.end();
+  });
+  server.setTimeout(200, (socket) => {
+    timeoutCount++;
+    socket.destroy();
+  });
+  server.keepAliveTimeout = 50;
+  server.listen(0, common.mustCall(() => {
+    const options = {
+      port: server.address().port,
+      allowHalfOpen: true
+    };
+    const c = net.connect(options, () => {
+      c.write('GET /1 HTTP/1.1\r\nHost: localhost\r\n\r\n');
+      c.write('GET /2 HTTP/1.1\r\nHost: localhost\r\n\r\n');
+      c.write('GET /3 HTTP/1.1\r\nHost: localhost\r\n\r\n');
+    });
+    setTimeout(() => {
+      server.close();
+      if (socket.destroyed) destroyedSockets++;
+      cb();
+    }, 1000);
+  }));
+});
+
+test(function serverNoEndKeepAliveTimeoutWithPipeline(cb) {
+  let socket;
+  let destroyedSockets = 0;
+  let timeoutCount = 0;
+  let requestCount = 0;
+  process.on('exit', () => {
+    assert.strictEqual(timeoutCount, 1);
+    assert.strictEqual(requestCount, 3);
+    assert.strictEqual(destroyedSockets, 1);
+  });
+  const server = http.createServer((req, res) => {
+    socket = req.socket;
+    requestCount++;
+  });
+  server.setTimeout(200, (socket) => {
+    timeoutCount++;
+    socket.destroy();
+  });
+  server.keepAliveTimeout = 50;
+  server.listen(0, common.mustCall(() => {
+    const options = {
+      port: server.address().port,
+      allowHalfOpen: true
+    };
+    const c = net.connect(options, () => {
+      c.write('GET /1 HTTP/1.1\r\nHost: localhost\r\n\r\n');
+      c.write('GET /2 HTTP/1.1\r\nHost: localhost\r\n\r\n');
+      c.write('GET /3 HTTP/1.1\r\nHost: localhost\r\n\r\n');
+    });
+    setTimeout(() => {
+      server.close();
+      if (socket.destroyed) destroyedSockets++;
+      cb();
+    }, 1000);
+  }));
+});

test/parallel/test-https-server-keep-alive-timeout.js

@@ -0,0 +1,103 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const https = require('https');
+const tls = require('tls');
+const fs = require('fs');
+
+const tests = [];
+
+const serverOptions = {
+  key: fs.readFileSync(common.fixturesDir + '/keys/agent1-key.pem'),
+  cert: fs.readFileSync(common.fixturesDir + '/keys/agent1-cert.pem')
+};
+
+function test(fn) {
+  if (!tests.length) {
+    process.nextTick(run);
+  }
+  tests.push(fn);
+}
+
+function run() {
+  const fn = tests.shift();
+  if (fn) fn(run);
+}
+
+test(function serverKeepAliveTimeoutWithPipeline(cb) {
+  let socket;
+  let destroyedSockets = 0;
+  let timeoutCount = 0;
+  let requestCount = 0;
+  process.on('exit', function() {
+    assert.strictEqual(timeoutCount, 1);
+    assert.strictEqual(requestCount, 3);
+    assert.strictEqual(destroyedSockets, 1);
+  });
+  const server = https.createServer(serverOptions, (req, res) => {
+    socket = req.socket;
+    requestCount++;
+    res.end();
+  });
+  server.setTimeout(200, (socket) => {
+    timeoutCount++;
+    socket.destroy();
+  });
+  server.keepAliveTimeout = 50;
+  server.listen(0, common.mustCall(() => {
+    const options = {
+      port: server.address().port,
+      allowHalfOpen: true,
+      rejectUnauthorized: false
+    };
+    const c = tls.connect(options, () => {
+      c.write('GET /1 HTTP/1.1\r\nHost: localhost\r\n\r\n');
+      c.write('GET /2 HTTP/1.1\r\nHost: localhost\r\n\r\n');
+      c.write('GET /3 HTTP/1.1\r\nHost: localhost\r\n\r\n');
+    });
+    setTimeout(() => {
+      server.close();
+      if (socket.destroyed) destroyedSockets++;
+      cb();
+    }, 1000);
+  }));
+});
+
+test(function serverNoEndKeepAliveTimeoutWithPipeline(cb) {
+  let socket;
+  let destroyedSockets = 0;
+  let timeoutCount = 0;
+  let requestCount = 0;
+  process.on('exit', () => {
+    assert.strictEqual(timeoutCount, 1);
+    assert.strictEqual(requestCount, 3);
+    assert.strictEqual(destroyedSockets, 1);
+  });
+  const server = https.createServer(serverOptions, (req, res) => {
+    socket = req.socket;
+    requestCount++;
+  });
+  server.setTimeout(200, (socket) => {
+    timeoutCount++;
+    socket.destroy();
+  });
+  server.keepAliveTimeout = 50;
+  server.listen(0, common.mustCall(() => {
+    const options = {
+      port: server.address().port,
+      allowHalfOpen: true,
+      rejectUnauthorized: false
+    };
+    const c = tls.connect(options, () => {
+      c.write('GET /1 HTTP/1.1\r\nHost: localhost\r\n\r\n');
+      c.write('GET /2 HTTP/1.1\r\nHost: localhost\r\n\r\n');
+      c.write('GET /3 HTTP/1.1\r\nHost: localhost\r\n\r\n');
+    });
+    setTimeout(() => {
+      server.close();
+      if (socket && socket.destroyed) destroyedSockets++;
+      cb();
+    }, 1000);
+  }));
+});