Skip to content
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

Memory leaks in loops with Promise #6673

Closed
julien-f opened this issue May 10, 2016 · 50 comments
Closed

Memory leaks in loops with Promise #6673

julien-f opened this issue May 10, 2016 · 50 comments
Labels

Comments

@julien-f
Copy link
Contributor

@julien-f julien-f commented May 10, 2016

  • Version: Node v4.4.4 / Node v6.1.0
  • Platform: Linux 4.2.0-1-amd64 #1 SMP Debian 4.2.6-3 (2015-12-06) x86_64 GNU/Linux
;(function loop () {
  return Promise.resolve().then(loop)
})()

The code above increasingly consumes memory until it crashes with:

<--- Last few GCs --->

   16059 ms: Scavenge 1406.1 (1457.9) -> 1406.1 (1457.9) MB, 19.4 / 0 ms (+ 2.6 ms in 1 steps since last GC) [allocation failure] [incremental marking delaying mark-sweep].
   18001 ms: Mark-sweep 1406.1 (1457.9) -> 1406.1 (1457.9) MB, 1941.5 / 0 ms (+ 3.7 ms in 2 steps since start of marking, biggest step 2.6 ms) [last resort gc].
   19928 ms: Mark-sweep 1406.1 (1457.9) -> 1406.1 (1457.9) MB, 1927.5 / 0 ms [last resort gc].


<--- JS stacktrace --->

==== JS stack trace =========================================

Security context: 0x30f9e32b4629 <JS Object>
    1: PromiseSet(aka PromiseSet) [native promise.js:~38] [pc=0x21bc625235b] (this=0x30f9e32041b9 <undefined>,n=0x23be73509901 <a Promise with map 0x2d3fc3316dc9>,q=0,r=0x30f9e32041b9 <undefined>,t=0x23be73509961 <JS Array[0]>,u=0x23be73509941 <JS Array[0]>)
    2: PromiseInit(aka PromiseInit) [native promise.js:~53] [pc=0x21bc624d6e9] (this=0x30f9e32041b9 <undefined>,n=0x23be73509901 <a Promis...

FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - process out of memory

With Bluebird, the used memory never goes above 30MB and the program does not crash:

global.Promise = require('bluebird')

;(function loop () {
  return Promise.resolve().then(loop)
})()
@addaleax addaleax added the V8 Engine label May 10, 2016
@mscdex mscdex added the memory label May 10, 2016
@mscdex
Copy link
Contributor

@mscdex mscdex commented May 10, 2016

@Fishrock123
Copy link
Member

@Fishrock123 Fishrock123 commented May 10, 2016

Is this because each promise keeps a reference to the next?

@ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented May 10, 2016

@Fishrock123 Doesn't look like that, looks like mark-sweep isn't called for whatever reason.

Could be a duplicate of #6180.
Perhaps it's even fixed by #6398 in the current master, but I can't check atm. Update: it's not, #6180 isn't fixed yet.

@jeisinger
Copy link
Contributor

@jeisinger jeisinger commented May 10, 2016

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.

@ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented May 10, 2016

@jeisinger It's not a spec thing, adding a manual gc every e.g. 1e7 iterations fixes it.
Looks very similar to #6180 — scavege is triggered instead of mark-sweep, but the memory is reclaimed only by mark-sweep, not by scavenge.

Update: I made a mistake in the test, shame on me =).

@julien-f
Copy link
Contributor Author

@julien-f julien-f commented May 10, 2016

@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?

@ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented May 10, 2016

@jeisinger No idea why the mark-sweep at the end doesn't reclaim most of that, though.

@ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented May 10, 2016

@Fishrock123 @jeisinger I made a mistake in the test, ignore my previous comments. Sorry for the inconvenience.

@Fishrock123
Copy link
Member

@Fishrock123 Fishrock123 commented May 10, 2016

If it's a spec thing it's an issue because I don't know a better way to do a loop with promises :/

Unbounded recursive loops are typically not safe in JavaScript at the current time, promise or not apparently.

@ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented May 10, 2016

Yes, it looks like all Promise objects are retained in the heap. There are twice more of them than the number of iterations, btw.

@vkurchatkin
Copy link
Contributor

@vkurchatkin vkurchatkin commented May 10, 2016

twice more of them than the number of iterations, btw.

makes sense, 2 new promises are created on each iteration

@julien-f
Copy link
Contributor Author

@julien-f julien-f commented May 10, 2016

Unbounded recursive loops are typically not safe in JavaScript at the current time, promise or not apparently.

Yes but there are no other way to do async loops than using recursion (without tricks likes setImmediate() to break the stack).

@ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented May 10, 2016

makes sense, 2 new promises are created on each iteration

Ah, yes, that is correct.

@vkurchatkin
Copy link
Contributor

@vkurchatkin vkurchatkin commented May 10, 2016

@julien-f simple nextTick is enough in this case:

var i = 0;
;(function loop () {
  if (++i % 100000 === 0) {
    return Promise.resolve().then(() => process.nextTick(loop))
  }
  return Promise.resolve().then(loop);
})()
@julien-f
Copy link
Contributor Author

@julien-f julien-f commented May 10, 2016

@vkurchatkin Sure, but with real-world (spaghetti) code it's far less easy to do 😉

@vkurchatkin
Copy link
Contributor

@vkurchatkin vkurchatkin commented May 10, 2016

To be honest, I think it works as expected. You are basically trying to build an infinite linked list

@petkaantonov
Copy link
Contributor

@petkaantonov petkaantonov commented May 10, 2016

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.

@julien-f
Copy link
Contributor Author

@julien-f julien-f commented May 11, 2016

@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 nextTick() or similar?

@petkaantonov
Copy link
Contributor

@petkaantonov petkaantonov commented May 11, 2016

Leave out the return statement

@trevnorris
Copy link
Contributor

@trevnorris trevnorris commented May 11, 2016

Unbounded recursive loops are typically not safe in JavaScript at the current time, promise or not apparently.

While recurseive nextTick() isn't the smartest idea, it's also not unsafe.

@julien-f What @petkaantonov said. With the return you are telling the Promise to chain, which doesn't allow the promises to be GC'd until the chain is resolved. So these two are almost functionally the same:

(function foo() {
  process.nextTick(foo);
}());

(function bar() {
  Promise.resolve().then(bar);
}());

Though remember that this won't allow the event loop to continue, and will starve your application of I/O.

@ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented May 11, 2016

Related links to what @petkaantonov said:

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.

Direct links:

@ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented May 11, 2016

Given that this is a spec thing, there seems to be nothing actionable here.
I propose we close this and redirect further discussion to promises-aplus/promises-spec repo.

@bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented May 12, 2016

I agree, closing.

@bnoordhuis bnoordhuis closed this May 12, 2016
@ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented Jun 22, 2016

/cc @littledan just in case.

@littledan
Copy link

@littledan littledan commented Jun 22, 2016

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.

@ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented Jul 25, 2016

@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.

@petkaantonov
Copy link
Contributor

@petkaantonov petkaantonov commented Sep 2, 2016

delay uses setTimeout

function delay(ms) {
    return new Promise(function(resolve) {
        setTimeout(resolve, ms);
    });
}
@rjmunro
Copy link

@rjmunro rjmunro commented Mar 20, 2017

If you miss out the return, you can't .catch() in one place at the end of the loop.

@danielo515
Copy link

@danielo515 danielo515 commented Jul 26, 2017

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.
I understand that every promise keeps a reference to the next promise, it makes sense to keep the chain, but it is very inconvenient to reach problems like this.
I would love to hear any solution/workaround that does not require breaking the promise chain, or at least don't use ugly hacks that make me want to take out my eyeballs.

Thanks and regards

@vkurchatkin
Copy link
Contributor

@vkurchatkin vkurchatkin commented Jul 26, 2017

@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 while loop.

@danielo515
Copy link

@danielo515 danielo515 commented Jul 26, 2017

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

@TimothyGu
Copy link
Member

@TimothyGu TimothyGu commented Jul 26, 2017

@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.

@danielo515
Copy link

@danielo515 danielo515 commented Jul 27, 2017

@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

@TimothyGu
Copy link
Member

@TimothyGu TimothyGu commented Jul 27, 2017

@danielo515 The latest LTS is v6.x. v7.x is not supported by the project at all. Why don't you just use setImmediate as a workaround until you can use v8.x? Again, if you use 7.x, or if you use any V8 experimental harmony flags, you are on your own, which is… not ideal for production.

@danielo515
Copy link

@danielo515 danielo515 commented Jul 28, 2017

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

@bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Jul 28, 2017

@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.

@danielo515
Copy link

@danielo515 danielo515 commented Jul 28, 2017

@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

@bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Jul 28, 2017

@danielo515 either v6.x or v8.x, both are LTS release lines.

@danielo515
Copy link

@danielo515 danielo515 commented Jul 28, 2017

Thanks @bnoordhuis , I'll give a try to v8.x

@fenixphp
Copy link

@fenixphp fenixphp commented Jan 26, 2018

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.

@danielo515
Copy link

@danielo515 danielo515 commented Jan 27, 2018

@fenixphp do you mean that you experiment the leak just with promises or that you experiment it both with promises and async functions ?
Theoretically this only happens on promises because they keep a chain that grows wild, which is not the case of async functions.

@fenixphp
Copy link

@fenixphp fenixphp commented Jan 27, 2018

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?

@danielo515
Copy link

@danielo515 danielo515 commented Jan 28, 2018

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.

@Sharcoux
Copy link

@Sharcoux Sharcoux commented Nov 18, 2019

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.

function task() {
  //dummy async treatment
  return delay(100);
}
let stack = Promise.resolve('the stack is empty');

function addTask() {
  return stack = stack.then(task);
}
  1. When repeatedly calling addTask over the time, like every second, will I have a memory leak?
  2. If I do, how should I work around this?

Thanks for your help

@dr-js
Copy link

@dr-js dr-js commented Mar 15, 2020

@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.

@dr-js
Copy link

@dr-js dr-js commented Mar 15, 2020

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 promise.then code.
The recursive code cause the promise implementation need to keep extra reference/stack info so the GC can not claim those already-resolved promise from the chain.

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.
And if the promise version of while (true) { /* async code */ } is needed, better use a async function instead of the tail-recursive promise pattern.


First part of test is 2 different OOM sample code and a fix attempt:

Then some common promise chain use pattern is tested:

  • [OOM] holding the head & tail promise of a chain of pending promise
  • [OOM] holding the head-resolve & tail promise of a chain of pending promise
  • [SAFE] holding the tail promise of a chain of pending promise
  • [SAFE] holding the head & tail promise of a chain of resolving promise
  • [SAFE] holding the tail promise of a chain of resolving promise

The test code I used: (and an sample output from v13.11.0 x64 win32)

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')
  }
})
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.