Skip to content

Commit 2cb6f2b

Browse files
aqrlnMylesBorins
authored andcommitted
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>
1 parent a0f8faa commit 2cb6f2b

File tree

3 files changed

+119
-8
lines changed

3 files changed

+119
-8
lines changed

lib/_http_server.js

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -438,14 +438,6 @@ function socketOnData(server, socket, parser, state, d) {
438438
assert(!socket._paused);
439439
debug('SERVER socketOnData %d', d.length);
440440

441-
if (state.keepAliveTimeoutSet) {
442-
socket.setTimeout(0);
443-
if (server.timeout) {
444-
socket.setTimeout(server.timeout);
445-
}
446-
state.keepAliveTimeoutSet = false;
447-
}
448-
449441
var ret = parser.execute(d);
450442
onParserExecuteCommon(server, socket, parser, state, ret, d);
451443
}
@@ -466,6 +458,8 @@ function socketOnError(e) {
466458
}
467459

468460
function onParserExecuteCommon(server, socket, parser, state, ret, d) {
461+
resetSocketTimeout(server, socket, state);
462+
469463
if (ret instanceof Error) {
470464
debug('parse error', ret);
471465
socketOnError.call(socket, ret);
@@ -547,6 +541,8 @@ function resOnFinish(req, res, socket, state, server) {
547541
// new message. In this callback we setup the response object and pass it
548542
// to the user.
549543
function parserOnIncoming(server, socket, state, req, keepAlive) {
544+
resetSocketTimeout(server, socket, state);
545+
550546
state.incoming.push(req);
551547

552548
// If the writable end isn't consuming, then stop reading
@@ -608,6 +604,14 @@ function parserOnIncoming(server, socket, state, req, keepAlive) {
608604
return false; // Not a HEAD response. (Not even a response!)
609605
}
610606

607+
function resetSocketTimeout(server, socket, state) {
608+
if (!state.keepAliveTimeoutSet)
609+
return;
610+
611+
socket.setTimeout(server.timeout || 0);
612+
state.keepAliveTimeoutSet = false;
613+
}
614+
611615
function onSocketResume() {
612616
// It may seem that the socket is resumed, but this is an enemy's trick to
613617
// deceive us! `resume` is emitted asynchronously, and may be called from
Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const assert = require('assert');
5+
const http = require('http');
6+
const net = require('net');
7+
8+
const server = http.createServer(common.mustCall((req, res) => {
9+
res.end();
10+
}, 2));
11+
12+
server.keepAliveTimeout = common.platformTimeout(100);
13+
14+
server.listen(0, common.mustCall(() => {
15+
const port = server.address().port;
16+
const socket = net.connect({ port }, common.mustCall(() => {
17+
request(common.mustCall(() => {
18+
// Make a second request on the same socket, after the keep-alive timeout
19+
// has been set on the server side.
20+
request(common.mustCall());
21+
}));
22+
}));
23+
24+
server.on('timeout', common.mustCall(() => {
25+
socket.end();
26+
server.close();
27+
}));
28+
29+
function request(callback) {
30+
socket.setEncoding('utf8');
31+
socket.on('data', onData);
32+
let response = '';
33+
34+
// Simulate a client that sends headers slowly (with a period of inactivity
35+
// that is longer than the keep-alive timeout).
36+
socket.write('GET / HTTP/1.1\r\n' +
37+
`Host: localhost:${port}\r\n`);
38+
setTimeout(() => {
39+
socket.write('Connection: keep-alive\r\n' +
40+
'\r\n');
41+
}, common.platformTimeout(300));
42+
43+
function onData(chunk) {
44+
response += chunk;
45+
if (chunk.includes('\r\n')) {
46+
socket.removeListener('data', onData);
47+
onHeaders();
48+
}
49+
}
50+
51+
function onHeaders() {
52+
assert.ok(response.includes('HTTP/1.1 200 OK\r\n'));
53+
assert.ok(response.includes('Connection: keep-alive\r\n'));
54+
callback();
55+
}
56+
}
57+
}));
Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const assert = require('assert');
5+
const http = require('http');
6+
7+
const server = http.createServer(common.mustCall((req, res) => {
8+
if (req.url === '/first') {
9+
res.end('ok');
10+
return;
11+
}
12+
setTimeout(() => {
13+
res.end('ok');
14+
}, common.platformTimeout(500));
15+
}, 2));
16+
17+
server.keepAliveTimeout = common.platformTimeout(200);
18+
19+
const agent = new http.Agent({
20+
keepAlive: true,
21+
maxSockets: 1
22+
});
23+
24+
function request(path, callback) {
25+
const port = server.address().port;
26+
const req = http.request({ agent, path, port }, common.mustCall((res) => {
27+
assert.strictEqual(res.statusCode, 200);
28+
29+
res.setEncoding('utf8');
30+
31+
let result = '';
32+
res.on('data', (chunk) => {
33+
result += chunk;
34+
});
35+
36+
res.on('end', common.mustCall(() => {
37+
assert.strictEqual(result, 'ok');
38+
callback();
39+
}));
40+
}));
41+
req.end();
42+
}
43+
44+
server.listen(0, common.mustCall(() => {
45+
request('/first', () => {
46+
request('/second', () => {
47+
server.close();
48+
});
49+
});
50+
}));

0 commit comments

Comments
 (0)