-
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
fs.readFile possible memory leak #21266
Comments
I'm not convinced there is a leak. Since you're kicking off a large number of requests in a tight loop, there are no available threads to service the requests, so they are placed into a queue. The queued tasks themselves consume memory. That is what you're seeing. You should instead measure memory usage after all of the requests have completed. The synchronous methods perform the work immediately on the main thread and thus do not consume any extra resources, which is why you see stable memory usage there. |
FYI, neither Node 4.8.6 nor 9.5.0 are supported version of Node, and you should report issue that affect up-to-date versions of Node. You can have a look on the LTS Release Schedule to see what versions are supported by the Node community, and on the CHANGELOG table to have the list of the last version released for each semver-major (at the time of writing, supported versions are v6.14.2 (maintenance only), v8.11.2, v9.11.1 (maintenance only) and v10.4.0). |
@ip413 could you please check if this affects |
@aduh95 - yep, sorry for that. |
I believe @mscdex's diagnosis is accurate. But we would need the "after requests are completed" data to prove it. I'm guessing this will be an easy-to-clean-up issue if someone wants to collect that data. |
@ip413 - is this still an issue? |
@ryzokuken - I haven't tested master, but tested last version of node 12.14.1, so probably master is affected I'm personally no longer interested in this issue, but...
Conclusions:
Personally I don't believe is such a big leak... but I don't have explanation of this behaviour. Maybe with normal usage of file everything is fine. Probably @mscdex is right... but I can't prove it. |
I am able to see the issue.
sections such as this, never gets unmapped. strace also shows a similar story:
there is no matching |
|
but this one looks like |
expanded v8 heaps are never shrunk, even if the usage is long ceased? |
also, is there a race condition by which multiple threads believe (together) that they need to expand? |
I have been working with @gireeshpunathil on this. After some investigation, I have found out that this is not an issue at all, but just the way Here is the proof: var fs = require(‘fs’)
function report() {
global.gc()
console.log(‘after gc:’)
setInterval(() => {
console.log(Math.round(process.memoryUsage()[‘rss’] / (1024 * 1024)))
}, 10000)
}
function readFile() {
fs.readFile(‘./foo.txt’, (e, d) => {
if(e) console.log(e)
})
}
console.log(`process id: ${process.pid}`)
fs.readFile(‘./foo.txt’, (e, d) => {
console.log(‘baseline:’)
console.log(Math.round(process.memoryUsage()[‘rss’] / (1024 * 1024)))
setTimeout(() => {
for (var i = 0; i < 1000; i++) {
readFile()
}
report()
}, 10000)
}) $ cat gc.js let list2 = []
setInterval(() => {
let list1 = []
// make 4K data, aligning to linux page size
for(var i=0; i< 4096; i++)
list1.push(‘x’.repeat(4096))
list2.push(list1)
list2.forEach((list) => {
list.forEach((item) => {
// make sure we ‘touch’ every pages of our memory
item.indexOf(‘x’)
})
})
}, 10)
setInterval(() => {
console.log(Math.round(process.memoryUsage()[‘rss’] / (1024 * 1024)))
}, 1000) check the free memory in your system. If it is huge, the above program may need to be modified to create larger arrays / strings; or else we need to wait for a longer period of time. My system has only ~2GB, so not very bad.
run the first program in a terminal, and wait for the
then run the second program to eat up all the free memory in the system.
When the consumption approaches the free memory limit, check the status of the first program. It would have come down!
inference: your program causes active memory to shoot up, but the system was relatively too free to reclaim those memory. There is no bug / misbehavior observed in node. |
Yep, the behavior you're seeing here is absolutely normal. While seeing that RSS rise and hold for so long is surprising and concerning for some, it's not the memory metric you need to watch with regards to memory leaks. |
i have same with fs.writeFileSync |
Looks like this was forgotten to be closed out? #11289 is a similar issue, FWIW. @machinetherapist If you think you've found a legitimate bug, please open a new issue. |
fs.readFile seems to have memory leak while reading asynchronously the same file many times
fs.readFileSync seems to have very different behaviour and clean up memory efficiently
Image showing issue https://github.com/ip413/node-read-file/blob/master/docs/data.png
Code is here https://github.com/ip413/node-read-file/blob/master/index.js
Excel is here https://github.com/ip413/node-read-file/blob/master/docs/data.ods
The same problem occurs on 9.5.0 node version.
The text was updated successfully, but these errors were encountered: