From d71718db6aa4feb8dc10edbad1134472468e971a Mon Sep 17 00:00:00 2001 From: Alexey Orlenko Date: Thu, 8 Jun 2017 17:20:24 +0300 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/2534 Fixes: https://github.com/nodejs/node/issues/13391 PR-URL: https://github.com/nodejs/node/pull/13549 Reviewed-By: Refael Ackermann Reviewed-By: Matteo Collina Reviewed-By: Brian White --- lib/_http_server.js | 20 ++++--- ...-keep-alive-timeout-slow-client-headers.js | 57 +++++++++++++++++++ ...p-server-keep-alive-timeout-slow-server.js | 50 ++++++++++++++++ 3 files changed, 119 insertions(+), 8 deletions(-) create mode 100644 test/parallel/test-http-server-keep-alive-timeout-slow-client-headers.js create mode 100644 test/parallel/test-http-server-keep-alive-timeout-slow-server.js diff --git a/lib/_http_server.js b/lib/_http_server.js index 6d5dbf4584a7ef..854ae56387e8cf 100644 --- a/lib/_http_server.js +++ b/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 diff --git a/test/parallel/test-http-server-keep-alive-timeout-slow-client-headers.js b/test/parallel/test-http-server-keep-alive-timeout-slow-client-headers.js new file mode 100644 index 00000000000000..453831ecba88a2 --- /dev/null +++ b/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(); + } + } +})); diff --git a/test/parallel/test-http-server-keep-alive-timeout-slow-server.js b/test/parallel/test-http-server-keep-alive-timeout-slow-server.js new file mode 100644 index 00000000000000..1543c1415fa5e9 --- /dev/null +++ b/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(); + }); + }); +}));