Skip to content

Commit 40a2a8e

Browse files
committed
http: reset parser.incoming when server request is finished
This resolves a memory leak for keep-alive connections and does not regress in the way that 779a05d did by waiting for the incoming request to be finished before releasing the `parser.incoming` object. Refs: nodejs#28646 Refs: nodejs#29263 Fixes: nodejs#9668
1 parent b3172f8 commit 40a2a8e

File tree

3 files changed

+66
-3
lines changed

3 files changed

+66
-3
lines changed

lib/_http_server.js

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -611,13 +611,25 @@ function onParserExecuteCommon(server, socket, parser, state, ret, d) {
611611
}
612612
}
613613

614+
function clearIncoming(parser, req) {
615+
// Reset the .incoming property so that the request object can be gc'ed.
616+
if (parser && parser.incoming === req) {
617+
if (req.complete) {
618+
parser.incoming = null;
619+
} else {
620+
req.on('end', clearIncoming.bind(null, parser, req));
621+
}
622+
}
623+
}
624+
614625
function resOnFinish(req, res, socket, state, server) {
615626
// Usually the first incoming element should be our request. it may
616627
// be that in the case abortIncoming() was called that the incoming
617628
// array will be empty.
618629
assert(state.incoming.length === 0 || state.incoming[0] === req);
619630

620631
state.incoming.shift();
632+
clearIncoming(socket.parser, req);
621633

622634
// If the user never called req.read(), and didn't pipe() or
623635
// .resume() or .on('data'), then we call req._dump() so that the

test/parallel/test-http-server-keepalive-end.js

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,27 @@
11
'use strict';
2-
32
const common = require('../common');
3+
const assert = require('assert');
44
const { createServer } = require('http');
55
const { connect } = require('net');
66

7+
// Make sure that for HTTP keepalive requests, the .on('end') event is emitted
8+
// on the incoming request object, and that the parser instance does not hold
9+
// on to that request object afterwards.
10+
711
const server = createServer(common.mustCall((req, res) => {
8-
req.on('end', common.mustCall());
12+
req.on('end', common.mustCall(() => {
13+
const parser = req.socket.parser;
14+
assert.strictEqual(parser.incoming, req);
15+
process.nextTick(() => {
16+
assert.strictEqual(parser.incoming, null);
17+
});
18+
}));
919
res.end('hello world');
1020
}));
1121

1222
server.unref();
1323

1424
server.listen(0, common.mustCall(() => {
15-
1625
const client = connect(server.address().port);
1726

1827
const req = [
Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
// Flags: --expose-gc
2+
'use strict';
3+
const common = require('../common');
4+
const onGC = require('../common/ongc');
5+
const { createServer } = require('http');
6+
const { connect } = require('net');
7+
8+
// Make sure that for HTTP keepalive requests, the req object can be
9+
// garbage collected once the request is finished.
10+
// Refs: https://github.com/nodejs/node/issues/9668
11+
12+
let client;
13+
const server = createServer(common.mustCall((req, res) => {
14+
onGC(req, { ongc: common.mustCall() });
15+
req.resume();
16+
req.on('end', common.mustCall(() => {
17+
setImmediate(() => {
18+
client.end();
19+
global.gc();
20+
});
21+
}));
22+
res.end('hello world');
23+
}));
24+
25+
server.unref();
26+
27+
server.listen(0, common.mustCall(() => {
28+
client = connect(server.address().port);
29+
30+
const req = [
31+
'POST / HTTP/1.1',
32+
`Host: localhost:${server.address().port}`,
33+
'Connection: keep-alive',
34+
'Content-Length: 11',
35+
'',
36+
'hello world',
37+
''
38+
].join('\r\n');
39+
40+
client.write(req);
41+
client.unref();
42+
}));

0 commit comments

Comments
 (0)