Memory leak due to process.fork()? #15651
Comments
|
Do you have code that reproduces the memory leak? |
|
I tried to make a simplified setup of my app: server.js const http = require('http');
const { fork } = require('child_process');
const server = http.createServer((req, res) => {
const workerProcess = fork('worker.js', [], {execArgv: []});
workerProcess.on('message', (status) => {
switch (status) {
case 'ready':
workerProcess.send({
method: req.method,
headers: req.headers,
path: req.path,
httpVersionMajor: req.httpVersionMajor,
query: req.query,
}, res.socket);
break;
case 'done':
res.socket.end();
}
});
});
server.listen(8080);
setInterval(() => {
console.log(process.memoryUsage());
}, 10000);worker.js const http = require('http');
process.on('message', (req, socket) => {
const res = new http.ServerResponse(req);
res._finish = function _finish() {
res.emit('prefinish');
socket.end();
};
res.assignSocket(socket);
socket.once('finish', () => {
process.send('done', () => {
process.exit(0);
});
});
res.end(process.pid.toString());
});
process.send('ready');When simulating load on this server using ApacheBench, memory usage of the server-process slowly rises. You'll have to be patient: it does take a few hours on my machine to become critical... |
|
Node 8.6.0 has the same issue. |
|
When limiting the available memory for the server to let's say 100MB, it dies within a few minutes at a certain point: |
Entries in the `net.Server#_workers` array that is used to track handles sent from the master to workers were not deleted when a worker exited, resulting in a slow but inexorable memory leak. Fixes: nodejs#15651
|
@bnoordhuis Thanks! While i do think your change is part of the solution , this issue still isn't completely resolved: even now the server-process keeps leaking memory up untill it crashes... |
|
Do you mean with the example you posted or with a larger application? |
|
The example... |
|
The issue still exists... |
|
I was going to remove the |
|
Just checked out and built 2e59ec0 again and ran the test on my Linux 64bit environment.. Output of server.js: @bnoordhuis What exact commit did you run the test with? Or could it be platform-related? |
|
Hmmm... tried again with 4f339b5 but no cigar: i get the same output. Not sure what do to next... Should we close this issue since it isn't reproducible? |
|
@fruitl00p @AubreyHewes @mishavantol any luck in reproducing this issue? |
|
@Reggino what version of Linux are you using? |
|
Tested both Ubuntu 16.04 and 17.04, both with the same issue. @bnoordhuis Did you see the worker PID when requesting http://localhost:8080 ? Did |
Entries in the `net.Server#_slaves` array that is used to track handles sent from the master to workers were not deleted when a worker exited, resulting in a slow but inexorable memory leak. PR-URL: nodejs#15679 Fixes: nodejs#15651 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Entries in the `net.Server#_slaves` array that is used to track handles sent from the master to workers were not deleted when a worker exited, resulting in a slow but inexorable memory leak. PR-URL: #15679 Backport-PR-URL: #16586 Fixes: #15651 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Entries in the `net.Server#_slaves` array that is used to track handles sent from the master to workers were not deleted when a worker exited, resulting in a slow but inexorable memory leak. PR-URL: #15679 Backport-PR-URL: #16586 Fixes: #15651 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
|
I would like to bump this again as I am now seeing this in production environments. Will post reproduction code once available |
|
I am also experiencing memory leak with child_process.fork in one of my apps. There is a MQTT client connected to a MQTT broker in index.js and on receiving a message it forwards it to a child_process for processing using child_process.send(). In the child process I catch this message using process.on() and then do the relevant processing. There are 5 sensors sending data every 5s to the app and every 5 min or so the child process is exiting with SIGABRT and I see the heapdump going to 1GB+ just around that instant. node version 8.9.4 https://stackoverflow.com/questions/48801587/nodejs-child-process-ipc-communication-issue-on-server |
|
I think the problem with the original code was that after passing the socket to the worker, the underlying handle is automatically closed but the http (HTTPParser, etc) objects associated to the socket don't notice about it so they are never freed. Is this something the To avoid this you can pass the option |
|
@santigimeno Thank you for your suggestion. I tried it and indeed the server stays alive a bit longer, doesn't seem to run out of memory, but stops responding after about 2 minutes... weird? |
|
@Reggino can you try to send the |
|
@santigimeno same result, the server always crashes, with or without |
|
Weird, I ran the code several hours and the heap size seemed pretty stable. Is it the same crash? Did it took longer to crash? Can you try with this patch without the diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js
index 58bb46e8a2..91e4cbc978 100644
--- a/lib/internal/child_process.js
+++ b/lib/internal/child_process.js
@@ -18,6 +18,8 @@ const SocketList = require('internal/socket_list');
const { convertToValidSignal } = require('internal/util');
const { isUint8Array } = require('internal/util/types');
const spawn_sync = process.binding('spawn_sync');
+const { HTTPParser } = process.binding('http_parser');
+const { freeParser } = require('_http_common');
const {
UV_EAGAIN,
@@ -94,6 +96,14 @@ const handleConversion = {
if (!options.keepOpen) {
handle.onread = nop;
socket._handle = null;
+ socket.setTimeout(0);
+ // In case of an HTTP connection socket, release the associated resources
+ if (socket.parser && socket.parser instanceof HTTPParser) {
+ freeParser(socket.parser, null, socket);
+ if (socket._httpMessage) {
+ socket._httpMessage.detachSocket(socket);
+ }
+ }
}
return handle; |
|
@santigimeno Thanks for the patch. I tried it, but now, when i try some requests on the server i get an error like: To make sure you (and @bnoordhuis ) should be able to run the test i set up a repo containing the test here: https://github.com/Reggino/node-issue-15651-test . It should make sure the failing test is reproducible and we all use the same node flags and load-testing / benchmarking tool (this example uses https://github.com/carlos8f/slam ) |
|
@Reggino yeah with the patch the socket is detached from the response so it's set to |
|
@santigimeno awesome! Indeed, this seems to fix the issue! 🙇♂️ |
|
Hi, Can someone help me with my issue. nodejs/help#1123 |
|
Is it a close candidate? fwiw, I am able to reproduce the leak consistently in my MAC, with the latest. |
|
@gireeshpunathil I forgot about this one. I'll try to prepare a PR and see how it goes. |
|
thanks, you may |
After passing an HTTP socket, release its associated resources. Fixes: nodejs#15651
BridgeAR
added
confirmed-bug
After passing an HTTP socket, release its associated resources. PR-URL: #20305 Fixes: #15651 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]>
|
Hello, I updated my application to node.js 10.15 version however, the memory leak is still present. Regards |
|
@ellisium can you post code that reproduces the leak? Thanks |
|
it's a boilerplate used on my work so I'm not allowed to share it. I will confirm it with another test using only native node.js function and will back with result for end of this week. |
|
@ellisium could you try https://github.com/Reggino/node-issue-15651-test ? I was able to reproduce the issue and validate the actual fix with it... |
|
Ok I will try but I m focusing on testing cluster.fork first. Sorry maybe it's a different use case, if yes you can ignore my last messages so |
|
Hello, after few days testing, the curve stabilized and it seems ok. I guess it's depending on req frequencies which impact the memory allocation ramp up. |
I am experiencing memory leakage in an app that uses
process.fork()a lot. These child processes get sent messages viaprocess.send()with asendHandleand are terminated later on.I did run into issues with memory management here. Some heap dumps show that even after the child-processes exited, the
ChildProcess-instances are retained in the master process. I learned that usingsubprocess.disconnect()partly fixes that issue, but one more retainer can be found here:node/lib/net.js
Line 1665 in 20259f9
How, where and when should this
socketListbe removed from the_workers-array?The text was updated successfully, but these errors were encountered: