Skip to content

Commit

Permalink
http: reset parser.incoming when server request is finished
Browse files Browse the repository at this point in the history
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: #28646
Refs: #29263
Fixes: #9668

PR-URL: #29297
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
addaleax authored and targos committed Aug 26, 2019
1 parent a6abfcb commit ae0a0e9
Showing 3 changed files with 73 additions and 3 deletions.
14 changes: 14 additions & 0 deletions lib/_http_server.js
Original file line number Diff line number Diff line change
@@ -614,13 +614,27 @@ function onParserExecuteCommon(server, socket, parser, state, ret, d) {
}
}

function clearIncoming(req) {
req = req || this;
const parser = req.socket && req.socket.parser;
// Reset the .incoming property so that the request object can be gc'ed.
if (parser && parser.incoming === req) {
if (req.readableEnded) {
parser.incoming = null;
} else {
req.on('end', clearIncoming);
}
}
}

function resOnFinish(req, res, socket, state, server) {
// Usually the first incoming element should be our request. it may
// be that in the case abortIncoming() was called that the incoming
// array will be empty.
assert(state.incoming.length === 0 || state.incoming[0] === req);

state.incoming.shift();
clearIncoming(req);

// If the user never called req.read(), and didn't pipe() or
// .resume() or .on('data'), then we call req._dump() so that the
15 changes: 12 additions & 3 deletions test/parallel/test-http-server-keepalive-end.js
Original file line number Diff line number Diff line change
@@ -1,18 +1,27 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const { createServer } = require('http');
const { connect } = require('net');

// Make sure that for HTTP keepalive requests, the .on('end') event is emitted
// on the incoming request object, and that the parser instance does not hold
// on to that request object afterwards.

const server = createServer(common.mustCall((req, res) => {
req.on('end', common.mustCall());
req.on('end', common.mustCall(() => {
const parser = req.socket.parser;
assert.strictEqual(parser.incoming, req);
process.nextTick(() => {
assert.strictEqual(parser.incoming, null);
});
}));
res.end('hello world');
}));

server.unref();

server.listen(0, common.mustCall(() => {

const client = connect(server.address().port);

const req = [
47 changes: 47 additions & 0 deletions test/parallel/test-http-server-keepalive-req-gc.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// Flags: --expose-gc
'use strict';
const common = require('../common');
const onGC = require('../common/ongc');
const { createServer } = require('http');
const { connect } = require('net');

if (common.isWindows) {
// TODO(addaleax): Investigate why and remove the skip.
common.skip('This test is flaky on Windows.');
}

// Make sure that for HTTP keepalive requests, the req object can be
// garbage collected once the request is finished.
// Refs: https://github.com/nodejs/node/issues/9668

let client;
const server = createServer(common.mustCall((req, res) => {
onGC(req, { ongc: common.mustCall() });
req.resume();
req.on('end', common.mustCall(() => {
setImmediate(() => {
client.end();
global.gc();
});
}));
res.end('hello world');
}));

server.unref();

server.listen(0, common.mustCall(() => {
client = connect(server.address().port);

const req = [
'POST / HTTP/1.1',
`Host: localhost:${server.address().port}`,
'Connection: keep-alive',
'Content-Length: 11',
'',
'hello world',
''
].join('\r\n');

client.write(req);
client.unref();
}));

0 comments on commit ae0a0e9

Please sign in to comment.