http: fix timeout reset after keep-alive timeout

Fix the logic of resetting the socket timeout of keep-alive HTTP
connections and add two tests:

* `test-http-server-keep-alive-timeout-slow-server` is a regression test
  for GH-13391.  It ensures that the server-side keep-alive timeout will
  not fire during processing of a request.

* `test-http-server-keep-alive-timeout-slow-client-headers` ensures that
  the regular socket timeout is restored as soon as a client starts
  sending a new request, not as soon as the whole message is received,
  so that the keep-alive timeout will not fire while, e.g., the client
  is sending large cookies.

Refs: #2534
Fixes: #13391
PR-URL: #13549
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Brian White <[email protected]>

Author: aqrln

lib/_http_server.js

@@ -441,14 +441,6 @@ function socketOnData(server, socket, parser, state, d) {
   assert(!socket._paused);
   debug('SERVER socketOnData %d', d.length);
 
-  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);
 }
@@ -469,6 +461,8 @@ function socketOnError(e) {
 }
 
 function onParserExecuteCommon(server, socket, parser, state, ret, d) {
+  resetSocketTimeout(server, socket, state);
+
   if (ret instanceof Error) {
     debug('parse error', ret);
     socketOnError.call(socket, ret);
@@ -550,6 +544,8 @@ function resOnFinish(req, res, socket, state, server) {
 // new message. In this callback we setup the response object and pass it
 // to the user.
 function parserOnIncoming(server, socket, state, req, keepAlive) {
+  resetSocketTimeout(server, socket, state);
+
   state.incoming.push(req);
 
   // If the writable end isn't consuming, then stop reading
@@ -611,6 +607,14 @@ function parserOnIncoming(server, socket, state, req, keepAlive) {
   return false; // Not a HEAD response. (Not even a response!)
 }
 
+function resetSocketTimeout(server, socket, state) {
+  if (!state.keepAliveTimeoutSet)
+    return;
+
+  socket.setTimeout(server.timeout || 0);
+  state.keepAliveTimeoutSet = false;
+}
+
 function onSocketResume() {
   // It may seem that the socket is resumed, but this is an enemy's trick to
   // deceive us! `resume` is emitted asynchronously, and may be called from

test/parallel/test-http-server-keep-alive-timeout-slow-client-headers.js

@@ -0,0 +1,57 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const http = require('http');
+const net = require('net');
+
+const server = http.createServer(common.mustCall((req, res) => {
+  res.end();
+}, 2));
+
+server.keepAliveTimeout = common.platformTimeout(100);
+
+server.listen(0, common.mustCall(() => {
+  const port = server.address().port;
+  const socket = net.connect({ port }, common.mustCall(() => {
+    request(common.mustCall(() => {
+      // Make a second request on the same socket, after the keep-alive timeout
+      // has been set on the server side.
+      request(common.mustCall());
+    }));
+  }));
+
+  server.on('timeout', common.mustCall(() => {
+    socket.end();
+    server.close();
+  }));
+
+  function request(callback) {
+    socket.setEncoding('utf8');
+    socket.on('data', onData);
+    let response = '';
+
+    // Simulate a client that sends headers slowly (with a period of inactivity
+    // that is longer than the keep-alive timeout).
+    socket.write('GET / HTTP/1.1\r\n' +
+                 `Host: localhost:${port}\r\n`);
+    setTimeout(() => {
+      socket.write('Connection: keep-alive\r\n' +
+                   '\r\n');
+    }, common.platformTimeout(300));
+
+    function onData(chunk) {
+      response += chunk;
+      if (chunk.includes('\r\n')) {
+        socket.removeListener('data', onData);
+        onHeaders();
+      }
+    }
+
+    function onHeaders() {
+      assert.ok(response.includes('HTTP/1.1 200 OK\r\n'));
+      assert.ok(response.includes('Connection: keep-alive\r\n'));
+      callback();
+    }
+  }
+}));

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

@@ -0,0 +1,50 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const http = require('http');
+
+const server = http.createServer(common.mustCall((req, res) => {
+  if (req.url === '/first') {
+    res.end('ok');
+    return;
+  }
+  setTimeout(() => {
+    res.end('ok');
+  }, common.platformTimeout(500));
+}, 2));
+
+server.keepAliveTimeout = common.platformTimeout(200);
+
+const agent = new http.Agent({
+  keepAlive: true,
+  maxSockets: 1
+});
+
+function request(path, callback) {
+  const port = server.address().port;
+  const req = http.request({ agent, path, port }, common.mustCall((res) => {
+    assert.strictEqual(res.statusCode, 200);
+
+    res.setEncoding('utf8');
+
+    let result = '';
+    res.on('data', (chunk) => {
+      result += chunk;
+    });
+
+    res.on('end', common.mustCall(() => {
+      assert.strictEqual(result, 'ok');
+      callback();
+    }));
+  }));
+  req.end();
+}
+
+server.listen(0, common.mustCall(() => {
+  request('/first', () => {
+    request('/second', () => {
+      server.close();
+    });
+  });
+}));