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 <[email protected]> Author: Alexey Orlenko <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
nodejs · May 28, 2017 · a7f5c9c · a7f5c9c
1 parent bb77cce
commit a7f5c9c
Show file tree

Hide file tree

Showing 6 changed files with 252 additions and 11 deletions.
diff --git a/doc/api/http.md b/doc/api/http.md
@@ -832,17 +832,33 @@ Returns `server`.
 added: v0.9.12
 -->
 
-* {number} Defaults to 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
@@ -1764,6 +1780,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

diff --git a/doc/api/https.md b/doc/api/https.md
@@ -38,6 +38,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
@@ -229,6 +237,7 @@ const req = https.request(options, (res) => {
 
 [`Agent`]: #https_class_https_agent
 [`http.Agent`]: http.html#http_class_http_agent
+[`http.Server#keepAliveTimeout`]: http.html#http_server_keepalivetimeout
 [`http.Server#setTimeout()`]: http.html#http_server_settimeout_msecs_callback
 [`http.Server#timeout`]: http.html#http_server_timeout
 [`http.Server`]: http.html#http_class_http_server

diff --git a/lib/_http_server.js b/lib/_http_server.js
@@ -271,7 +271,7 @@ function Server(requestListener) {
   this.on('connection', connectionListener);
 
   this.timeout = 2 * 60 * 1000;
-
+  this.keepAliveTimeout = 5000;
   this._pendingResponseData = 0;
   this.maxHeadersCount = null;
 }
@@ -323,7 +323,8 @@ function connectionListener(socket) {
     // inactive responses. If more data than the high watermark is queued - we
     // need to pause TCP socket/HTTP parser, and wait until the data will be
     // sent to the client.
-    outgoingData: 0
+    outgoingData: 0,
+    keepAliveTimeoutSet: false
   };
   state.onData = socketOnData.bind(undefined, this, socket, parser, state);
   state.onEnd = socketOnEnd.bind(undefined, this, socket, parser, state);
@@ -431,8 +432,16 @@ function socketOnEnd(server, socket, parser, state) {
 function socketOnData(server, socket, parser, state, d) {
   assert(!socket._paused);
   debug('SERVER socketOnData %d', d.length);
-  var ret = parser.execute(d);
 
+  if (state.keepAliveTimeoutSet) {
+    socket.setTimeout(0);
+    if (server.timeout) {
+      socket.setTimeout(server.timeout);
+    }
+    state.keepAliveTimeoutSet = false;
+  }
+
+  var ret = parser.execute(d);
   onParserExecuteCommon(server, socket, parser, state, ret, d);
 }
 
@@ -496,7 +505,7 @@ function onParserExecuteCommon(server, socket, parser, state, ret, d) {
   }
 }
 
-function resOnFinish(req, res, socket, state) {
+function resOnFinish(req, res, socket, state, server) {
   // Usually the first incoming element should be our request.  it may
   // be that in the case abortIncoming() was called that the incoming
   // array will be empty.
@@ -514,6 +523,12 @@ function resOnFinish(req, res, socket, state) {
 
   if (res._last) {
     socket.destroySoon();
+  } else if (state.outgoing.length === 0) {
+    if (server.keepAliveTimeout) {
+      socket.setTimeout(0);
+      socket.setTimeout(server.keepAliveTimeout);
+      state.keepAliveTimeoutSet = true;
+    }
   } else {
     // start sending the next message
     var m = state.outgoing.shift();
@@ -560,7 +575,8 @@ function parserOnIncoming(server, socket, state, req, keepAlive) {
 
   // When we're finished writing the response, check if this is the last
   // response, if so destroy the socket.
-  res.on('finish', resOnFinish.bind(undefined, req, res, socket, state));
+  res.on('finish',
+         resOnFinish.bind(undefined, req, res, socket, state, server));
 
   if (req.headers.expect !== undefined &&
       (req.httpVersionMajor === 1 && req.httpVersionMinor === 1)) {

diff --git a/lib/https.js b/lib/https.js
@@ -59,6 +59,7 @@ function Server(opts, requestListener) {
   });
 
   this.timeout = 2 * 60 * 1000;
+  this.keepAliveTimeout = 5000;
 }
 inherits(Server, tls.Server);
 exports.Server = Server;

diff --git a/test/parallel/test-http-server-keep-alive-timeout.js b/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);
+  }));
+});
diff --git a/test/parallel/test-https-server-keep-alive-timeout.js b/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);
+  }));
+});