Memory leaks in loops with Promise #6673
Comments
mscdex
added
the
memory
|
/cc @nodejs/v8 |
|
Is this because each promise keeps a reference to the next? |
|
@Fishrock123
|
|
filed https://bugs.chromium.org/p/v8/issues/detail?id=5002 Firefox also runs out of memory, so maybe it's a spec thing according to the crash, MS is executed. |
|
@jeisinger Update: I made a mistake in the test, shame on me =). |
|
@jeisinger If it's a spec thing it's an issue because I don't know a better way to do a loop with promises :/ @petkaantonov What's your opinion on this? |
|
|
|
@Fishrock123 @jeisinger I made a mistake in the test, ignore my previous comments. Sorry for the inconvenience. |
Unbounded recursive loops are typically not safe in JavaScript at the current time, promise or not apparently. |
|
Yes, it looks like all Promise objects are retained in the heap. There are twice more of them than the number of iterations, btw. |
makes sense, 2 new promises are created on each iteration |
Yes but there are no other way to do async loops than using recursion (without tricks likes |
Ah, yes, that is correct. |
|
@julien-f simple var i = 0;
;(function loop () {
if (++i % 100000 === 0) {
return Promise.resolve().then(() => process.nextTick(loop))
}
return Promise.resolve().then(loop);
})() |
|
@vkurchatkin Sure, but with real-world (spaghetti) code it's far less easy to do 😉 |
|
To be honest, I think it works as expected. You are basically trying to build an infinite linked list |
|
this is a spec thing, bluebird sligthly deviates from the spec. The differences are subtle differences in callback call order in certain constructed scenarios. Theres a couple open issues in promises spec gh repo about this. |
|
@petkaantonov I see :/ According to the standard, what is the best way to do a (long) loop? Do I have to do a trampoline myself using |
|
Leave out the return statement |
While recurseive @julien-f What @petkaantonov said. With the Though remember that this won't allow the event loop to continue, and will starve your application of I/O. |
|
Related links to what @petkaantonov said:
Direct links: |
|
Given that this is a spec thing, there seems to be nothing actionable here. |
|
I agree, closing. |
|
/cc @littledan just in case. |
|
We looked into fixing this once again (or rather @gsathya did) and found that it seemed like it would be impossible to fix due to this same spec reason. |
|
@littledan Btw, any chances that the spec could be fixed to allow these optimizations? There was a significant amount of discussion in promises-aplus/promises-spec. |
|
I can't understand why this issue has left closed and unresolved. I expected this to happen on infinite synchronous recursive loops because the stack, that makes sense. But asynchronous code does not retain the stack (it is lost, reset or whatever it happens) so I never expected a leak like this happening. Thanks and regards |
|
@danielo515 this issue has been closed, because it is a problem with the spec and is not actionable. One workaround is to use async functions and |
|
Hello @vkurchatkin thanks for your prompt response. I'm a bit worried about async functions having the same problem (according to #9339). Because node 8 has some problems with some of our tools, do you know if I can use async await in node 7 ? Maybe with an harmony flag? I'm a bit disappointed about promises 😢 thanks again |
|
@danielo515 You can use the harmony flag, just not for production. #9339 was fixed in V8 5.5 which is in v7.6.0+. EDIT: deleted unnecessarily cranky response. |
|
@TimothyGu , the problem is that I need it for production. According to the spec, async functions are available on node 7.10 without harmony flag, and it is the current LTS version. I'll give it a try. Regards |
|
@danielo515 The latest LTS is v6.x. v7.x is not supported by the project at all. Why don't you just use |
|
My mistake . However we have been using v7 for months on production without any problem, and as I said the harmony flag is not required for asynchronous functions |
|
@danielo515 You should upgrade. v7 is end-of-life, it doesn't get security fixes. We put out a security release earlier this month that fixed a denial-of-service; v7 is vulnerable. |
|
@bnoordhuis update to which version ? With v.7 I mean 7.10. If you mean that I should update to node v.8 that may be a problem |
|
@danielo515 either v6.x or v8.x, both are LTS release lines. |
|
Thanks @bnoordhuis , I'll give a try to v8.x |
|
Can anyone explain the result of this conversation? I'm using "Node v8.9.2" but there is still a leak for cyclic use Promise or async / await functions. |
|
@fenixphp do you mean that you experiment the leak just with promises or that you experiment it both with promises and async functions ? |
|
Thanks @danielo515, Experimentally, I came to the conclusion that promises begin to leak as the call chain looks like a tree. What can I replace the promises in a pair of async / await functions? |
|
You can use async await function calls inside an infinite loop without any problem. But if you use recursion you will end with the same stack size problem. |
|
Reading this thread, I am getting a little worried and confused. The following code will make me able to pile tasks wherever in my codebase, and make sure that each task is executed only after all the previous ones are done.
Thanks for your help |
|
@Sharcoux your code do not have the tail-recursive pattern like the example, and if your outer code do not have the pattern either, and only reference the tail of the promise chain, the GC should work and no memory leak. |
|
From what I tested, the pattern for promise memory leaking must include a tail-recursive setup, the promise creating function always recursively appear in itself's If we build a long/infinite promise chain gradually without the recursive code pattern, and only keep reference to the tail of the chain, the GC can took those un-referenced promise from the chain, whether the promise is pending or not. So I think chaining promise up is not always unsafe, it depends on the actual code. First part of test is 2 different OOM sample code and a fix attempt:
Then some common promise chain use pattern is tested:
The test code I used: (and an sample output from console.log('Testing Env:', process.version, process.arch, process.platform)
const spawnFuncWithExposeGC = (...funcList) => {
const { status } = require('child_process').spawnSync(
process.argv0,
[
'--expose-gc', // allow `global.gc()` call
'--max-old-space-size=64', // limit max memory usage for faster OOM
'--eval', `(${funcList.reduce((o, func) => `(${func})(global.gc, ${o})`, 'undefined')})`
],
{ stdio: 'inherit' }
)
console.log(`process exit with status ${status} `.padEnd(64, '='))
console.log('\n')
}
const commonFunc = (triggerGC) => {
const setTimeoutAsync = (wait = 0) => new Promise((resolve) => setTimeout(resolve, wait))
const formatMemory = (value) => `${String(value).padStart(10, ' ')}B`
const markMemory = async () => {
triggerGC()
await setTimeoutAsync(10)
triggerGC()
const { heapUsed, heapTotal, rss, external } = process.memoryUsage()
console.log([
`heapUsed: ${formatMemory(heapUsed)}`,
`heapTotal: ${formatMemory(heapTotal)}`,
`rss: ${formatMemory(rss)}`,
`external: ${formatMemory(external)}`
].join(' '))
}
const appendPromiseAdder = (promise, count = 0) => {
let index = 0
while (index++ !== count) promise = promise.then((result) => (result + 1))
return promise
}
return { setTimeoutAsync, markMemory, appendPromiseAdder }
}
spawnFuncWithExposeGC(async () => {
console.log('[OOM] tail-recursive promise setup, GH sample, edit & formatted. https://github.com/promises-aplus/promises-spec/issues/179#issuecomment-93453094')
const run = (i) => new Promise((resolve) => setImmediate(resolve))
.then(() => {
if (i % 1e5 === 0) console.log({ i })
return i < 99999999 ? run(i + 1) : i
})
await run(0).then((result) => console.log(result))
})
spawnFuncWithExposeGC(async () => {
console.log('[OOM] tail-recursive promise setup, edit & formatted. https://alexn.org/blog/2017/10/11/javascript-promise-leaks-memory.html')
const signal = (i) => new Promise((resolve) => setImmediate(() => resolve(i)))
const loop = (n) => signal(n).then(i => {
if (i % 1e5 === 0) console.log({ i })
return loop(n + 1)
})
await loop(0).catch(console.error)
})
spawnFuncWithExposeGC(commonFunc, async (triggerGC, { markMemory }) => {
console.log('[SAFE] no-recursive promise setup')
let i = 0
let promiseTail = Promise.resolve()
const token = setInterval(() => { // simulate user input or other outer timer adding batch of task to the queue
if (i >= 1e8) return clearInterval(token) // check finish
let n = 0
while (n++ !== 1e5) {
promiseTail = promiseTail.then(() => {
i = i + 1
if (i % 1e6 !== 0) return // check log
console.log({ i })
markMemory()
})
}
}, 0)
})
spawnFuncWithExposeGC(commonFunc, async (triggerGC, { setTimeoutAsync, markMemory, appendPromiseAdder }) => {
console.log('[OOM] holding the head & tail promise of a chain of pending promise')
const promiseHead = new Promise((resolve) => {})
let promiseTail = promiseHead
let loop = 0
while (loop++ !== 128) {
await markMemory()
promiseTail = appendPromiseAdder(promiseTail, 64 * 1024)
await setTimeoutAsync(10)
}
console.log({ promiseHead, promiseTail })
})
spawnFuncWithExposeGC(commonFunc, async (triggerGC, { setTimeoutAsync, markMemory, appendPromiseAdder }) => {
console.log('[OOM] holding the head-resolve & tail promise of a chain of pending promise')
let pendingResolve
let promiseTail = new Promise((resolve) => { pendingResolve = resolve })
let loop = 0
while (loop++ !== 128) {
await markMemory()
promiseTail = appendPromiseAdder(promiseTail, 64 * 1024)
await setTimeoutAsync(10)
}
console.log({ pendingResolve, promiseTail })
})
spawnFuncWithExposeGC(commonFunc, async (triggerGC, { setTimeoutAsync, markMemory, appendPromiseAdder }) => {
{
console.log('[SAFE] holding the tail promise of a chain of pending promise')
let promiseTail = new Promise((resolve) => {})
let loop = 0
while (loop++ !== 128) {
await markMemory()
promiseTail = appendPromiseAdder(promiseTail, 64 * 1024)
await setTimeoutAsync(10)
}
console.log({ promiseTail })
console.log('\n')
}
{
console.log('[SAFE] holding the head & tail promise of a chain of resolving promise')
const promiseHead = new Promise((resolve) => { resolve(0) })
let promiseTail = promiseHead
let loop = 0
while (loop++ !== 128) {
await markMemory()
promiseTail = appendPromiseAdder(promiseTail, 64 * 1024).then((result) => {
console.log({ result })
return result
})
await setTimeoutAsync(10)
}
console.log({ promiseHead, promiseTail })
console.log('\n')
}
{
console.log('[SAFE] holding the tail promise of a chain of resolving promise')
let promiseTail = new Promise((resolve) => { resolve(0) })
let loop = 0
while (loop++ !== 128) {
await markMemory()
promiseTail = appendPromiseAdder(promiseTail, 64 * 1024)
promiseTail = promiseTail.then((result) => {
console.log({ result })
return result
})
await setTimeoutAsync(10)
}
console.log({ promiseTail })
console.log('\n')
}
}) |
The code above increasingly consumes memory until it crashes with:
With Bluebird, the used memory never goes above 30MB and the program does not crash:
The text was updated successfully, but these errors were encountered: