Memory leaks in loops with Promise

[julien-f]

• 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)
})()

[mscdex]

/cc @nodejs/v8

[Fishrock123]

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

[ChALkeR]

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

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]

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

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

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

[ChALkeR]

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

[Fishrock123]

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]

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]

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

makes sense, 2 new promises are created on each iteration

[julien-f]

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]

makes sense, 2 new promises are created on each iteration

Ah, yes, that is correct.

[vkurchatkin]

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

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

[vkurchatkin]

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

[petkaantonov]

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]

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

Leave out the return statement

[trevnorris]

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]

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:

• https://github.com/promises-aplus/promises-spec/issues/179
• https://github.com/promises-aplus/promises-spec/issues/183
• https://github.com/hax/test-promises/blob/master/promises-spec.md

[ChALkeR]

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]

I agree, closing.

[ChALkeR]

/cc @littledan just in case.

[littledan]

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]

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

[littledan]

@ChALkeR That discussion resulted in the spec we have today. Do you have any significant points that weren't mentioned there? For example, a concrete use case which is negatively impacted by the leak? I imagine most real applications will involve some real event loop behavior and not loop infinitely in microtasks. Otherwise, why not just use synchronous code?

[petkaantonov]

@littledan The leak isn't caused by infinitely looping in microtasks, it is caused by how promise reference chain is formed when there is a resolution chain. Afaik, the reason it was not discussed or fixed in the spec is because it simply hadn't occurred to anyone at that point.

Any asynchronous loop with promises is negatively impacted by the leak. For example the natural promise equivalent of:

while (true) {
    sleep(1);
    print("slept 1 second");
}

is

(function loop() {
    return delay(1000).then(function() {
        console.log("slept 1 second");
    }).then(loop);
})()

And will leak memory unless the return statement is removed.

[julien-f]

I'm wondering if it also the case with ES2016's async functions, it's probably the case with transpiling which usually implement them using promises (sometimes with generators) but I don't know about the spec.

(async () => {
  while (true) {
    await delay(1000)
    print('slept 1 second')
  }
})()

[vkurchatkin]

It shouldn't leak if you do anything async, like setTimeout or nextTick

[petkaantonov]

delay uses setTimeout

function delay(ms) {
    return new Promise(function(resolve) {
        setTimeout(resolve, ms);
    });
}

[rjmunro]

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

[danielo515]

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]

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

Hello @vkurchatkin thanks for your prompt response. I'm a bit worried about async functions having the same problem (according to https://github.com/nodejs/node/issues/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]

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

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

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

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]

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

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

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

[danielo515]

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

[fenixphp]

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]

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

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]

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]

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]

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

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:

• [OOM] tail-recursive promise setup, GH sample, edit & formatted.
  • https://github.com/promises-aplus/promises-spec/issues/179#issuecomment-93453094
• [OOM] tail-recursive promise setup, edit & formatted.
  • https://alexn.org/blog/2017/10/11/javascript-promise-leaks-memory.html
• [SAFE] no-recursive promise setup

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')
  }
})