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 <refack@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

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