-
Notifications
You must be signed in to change notification settings - Fork 30.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Making several http requests using keep-alive mode generates this trace: Warning: Possible EventEmitter memory leak detected. #9268
Comments
I joined httpRequests.js that shows the warning when executed with node v6.9.1 on my platform. |
Can you trim down your test case so it doesn't depend on a third-party module? It's allowed (and encouraged) to paste it inline. |
// make several http requests to reproduce the warning with node v6:
//(node:19604) Warning: Possible EventEmitter memory leak detected. 11 timeout listeners added. Use emitter.setMaxListeners() to increase limit
var asyncMapSeries = require('async/mapSeries');
var url = require('url');
var http = require('http');
var https = require('https');
var httpAgent = new http.Agent({ keepAlive: true });
var httpsAgent = new https.Agent({ keepAlive: true });
function request(requestUrl, requestOptions, data, callback) {
var options = url.parse(requestUrl);
options.agent = ((options.protocol === 'https:') ? httpsAgent : httpAgent);
// Add supp. options
if (requestOptions) {
var suppOptions = ((typeof requestOptions === 'string') ? JSON.parse(requestOptions) : requestOptions);
if (suppOptions.query) {
options.path += ((options.path.indexOf('?') > 0) ? '&' : '?') + suppOptions.query;
}
Object.assign(options, suppOptions);
}
// Add content-length header
if (!options.headers) {
options.headers = {};
}
if (data) {
options.headers['Content-Length'] = data.length;
}
// Send request
var t0 = Date.now();
var httpRequest = ((options.protocol === 'https:') ? https.request : http.request);
var req = httpRequest(options, function(res) {
var chunks = [];
res.setEncoding('utf8');
res.on('data', function (chunk) {
chunks.push(chunk);
});
res.on('end', function() {
callback(null, {method:options.method, url:requestUrl, status:res.statusCode, timeTaken:Date.now()-t0, response:chunks.join('')});
});
}).on('error', function(err) {
callback(requestUrl + ': ' + err.message);
});
if (options.timeout) {
req.setTimeout(options.timeout, function() {
console.error('HTTP Request timeout ('+options.timeout+' ms): '+requestUrl);
req.abort();
});
}
if (data) {
req.write(data);
}
req.end();
}
var requestUrl = 'http://example.com/foo.html';
var requestOptions = {
method: 'GET',
timeout: 10000,
headers: {
'User-Agent': 'Mozilla/5.0'
}
};
var requestData = undefined;
var array = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18];
//var array = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19];
asyncMapSeries(array,
function(arrayEntry, asyncCallback) {
request(requestUrl, requestOptions, requestData,
function(err, result) {
asyncCallback(err, result);
}
);
},
function(err, results) {
if (err) {
console.log('request error:', err);
process.exit(1);
} else {
console.log('all requests done');
process.exit(0);
}
}
); |
@arnaud-soulard I edited your comment to format it as js, hope that's ok! |
Thank you very much! |
@arnaud-soulard When I said 'third-party module', I was referring to async/mapSeries. Can you remove that from your snippet? |
@bnoordhuis I've removed the third-party module in the example and added some memory usage information, because we think that we experience the same problem in our setup. This is the code we ran: var url = require('url');
var http = require('http');
var httpAgent = new http.Agent({ keepAlive: true });
function request(callback) {
var options = url.parse('http://example.com/foo.html');
options.agent = httpAgent;
options.method = 'GET';
options.timeout = 10000;
var req = http.request(options, function (res) {
var chunks = [];
res.setEncoding('utf8');
res.on('data', function (chunk) {
chunks.push(chunk);
});
res.on('end', function () {
callback(null, res.statusCode);
});
}).on('error', function (err) {
callback(err.message);
});
req.end();
}
function asyncMapSeries(times, fn) {
let p = Promise.resolve();
for (let i = 0; i < times; i++) {
p = p.then(fn);
}
return p;
}
asyncMapSeries(100000,
function () {
return new Promise((resolve, reject) => {
request(function (err, result) {
if (err) {
return reject(err)
}
return resolve(result)
}
);
})
}
).then((result) => {
console.log('All done.')
}).catch((err) => {
console.log("err", err)
});
setInterval(() => {
gc();
console.log(process.memoryUsage());
}, 5000); And just as @arnaud-soulard reported we get a memory leak warning and an actual memory leak in 6.9.1 when using timeouts and keepAlive. This is the output from 6.9.1:
As you can see we get the warning and then the heapUsed is increasing. When running with only keepAlive but no timeout we get no warning and no leak:
We tried pinpointing which Node version introduced the leak and found that it's present in 6.8.0 but not in 6.7.0.
Could it have been introduced in c9b59e8? |
I'm seeing this too - we moved from 4.x to 6.9 the other day and have been seeing crashing due to memory. I spent today chasing it down and found that simply turning off What's interesting is I still get the |
The problem is due to this commit: ee7af01 Specifically these three lines of code Line 565 in ee7af01
In the case of keepalive, the socket is reused -- so each time a new listener is added This has caused our application major memory leak problems, and appears to have been there since 6.8.1. https://github.com/nodejs/node/tree/v6.8.1 |
@evantorrie Yup, that's what my pull request addresses. I'm a little bit surprised that a memory leak in the LTS is not given more priority. We had a bunch of services crash in production after upgrading to 6.9.1. |
@evantorrie Same problem, right now I bypass it by not using setTimeout on the request and implementing my own timer that resolves without waiting for the request. I definitely think it should be resolved in LTS as this kind of bug is prone to show up only in production and can be quite the headache to debug |
Closing as the fix in #9440 landed. |
@targos @italoacasas I don't know the process, does landing #9440 also get it into LTS? @arnaud-soulard v6.7.0 did not have this bug, you could try that. We rolled back to 6.2.2 but |
The PR is labeled |
Thanks @targos! |
still not cherry-picked in 6.9.5 for those interested, maybe if somebody could speed this up, bad bug for lts really @targos |
It will be in the next release. See #10974 |
Is it possible this is fixed in 6.10 and not 7.5? If this really solves it in 6.10 I can run my application on node 6.10. |
@5an1ty This is merged in 6.10 |
When making 20 http requests using keep-alive mode with node v6.9.1, I get the following trace:
Warning: Possible EventEmitter memory leak detected. 11 timeout listeners added. Use emitter.setMaxListeners() to increase limit
With node v4.4.7, this trace is not displayed.
It could be (from http://www.jongleberry.com/understanding-possible-eventemitter-leaks.html) that a listener has not been removed
The text was updated successfully, but these errors were encountered: