Memory leak in nodejs promise design?

Question

I noticed some people here recommend to use await/async and promise instead of setTimeout directly when you want to delay the execution of something. This is the code:

async wait (ms){
  return new Promise(resolve => setTimeout(resolve, ms));
}

So I would use

await wait(3000);
my_function();

instead of

setTimeout(() => {
  my_function();
}, 3000);

It makes sense but I noticed that if I do that I get increased memory usage and eventually the app crashes with heap of out memory after a few hours.

Is this an issue in the promise design of nodejs, or am I missing something here?

This code reproduces the issue:

const heapdump = require('heapdump'),
      fs = require('fs');
class test {
  constructor(){
    this.repeatFunc();
  }
  async wait (ms){
    return new Promise(resolve => setTimeout(resolve, ms));
  }
  async repeatFunc(){ 
    // do some work...
    heapdump.writeSnapshot(__dirname + '/' + Date.now() + '.heapsnapshot');    

    await this.wait(5000); 
    await this.repeatFunc();
  }
}
new test();

Notice heap dump keeps increasing every 5 seconds

With setInterval this doesn't happen:

const heapdump = require('heapdump'),
      fs = require('fs');
class test {
  constructor() {
    setInterval(this.repeatFunc, 5000);
  }
  repeatFunc() { 
    // do some work...
    heapdump.writeSnapshot(__dirname + '/' + Date.now() + '.heapsnapshot');    
  }
}
new test();

Please post the complete code of the app (or at least a [mcve] that reproduces the memory leak). No, promises should not leak memory on their own. How are you calling the function that does the waiting? — Bergi, Sep 20 '18 at 14:23
Your code most likely crashes because of a stack overflow, from infinite recursion of `repeatFunc`. Nothing to do with promises of `async`/`await` specifically :-) — Bergi, Sep 20 '18 at 14:35
That said, if you had written your code with tail recursion (`return this.repeatFunc()`), then [a good promise implementation should not leak memory](https://stackoverflow.com/q/29925948/1048572) — Bergi, Sep 20 '18 at 14:36
it still increases if I put `return this.repeatFunc` instead of `await this.repeatFunc` — Johnny, Sep 20 '18 at 14:47
But if I replace the whole thing with `setInterval`, it doesnt leak anymore and every heap snapshot is the same. So it's def a problem with nodejs async/await — Johnny, Sep 20 '18 at 14:48
I guess node still hasn't implemented tail call optimisation yet. — Bergi, Sep 20 '18 at 14:50
@Bergi - Or, hasn't implemented tail optimization on async functions which may well be more complicated. — jfriend00, Sep 20 '18 at 15:29
@jfriend00 Not much. The main issue is about not collecting stack data, which should be the same as with sync functions (or even be easier, as promises by default don't carry stack traces). [Resolving promises with deeply resolved promises](https://stackoverflow.com/a/29931657/1048572) is a solved problem. — Bergi, Sep 20 '18 at 15:38

Bergi · Answer 1 · 2018-09-20T15:33:31.267

2

You have written an infinitely recursive function, and each function call returns a new promise. Each promise is waiting to be resolved from the inner one - so yes, it of course is accumulating memory. If the code was synchronous, you would have gotten a stack overflow exception instead.

Just use a loop instead:

const heapdump = require('heapdump'),
      fs = require('fs');

async function wait(ms){
  return new Promise(resolve => setTimeout(resolve, ms));
}
async function repeat() {
  while (true) {
    // do some work...
    heapdump.writeSnapshot(__dirname + '/' + Date.now() + '.heapsnapshot');    

    await wait(5000); 
  }
}

repeat().then(() => console.log("all done"), console.error);

I noticed some people here recommend to use await/async and promise instead of setTimeout directly when you want to delay the execution of something.

Well that includes me, as promises are much easier to work with, especially when you want to return a result value from your asynchronous task or handle errors. But if you're not convinced by any of the advantages of promises, there is nothing that forces you convert your already-working code to promises. Just carry on using callback style until you find a good use for promises.

edited Sep 20 '18 at 15:33

answered Sep 20 '18 at 14:53

Bergi

630,263
148
957
1,375

Or just use `setInterval()` and skip `async/await` here entirely. – jfriend00 Sep 20 '18 at 15:14
@jfriend00 But I can't easily `.catch(…)` errors from a `setInterval` process. Also the loop might contain other `await`s – Bergi Sep 20 '18 at 15:18
So, you're arguing that ALL cases where one would have originally (before promises) used `setInterval()` should be replaced with this type of structure? Yes, there may be some situations where promises are the desired structure, but neither you or the OP shows any need for promises here so I'm letting the OP know there is also a simple alternative that has no risk of the problem they ran into. And, you don't even show a `.catch()` in your example. – jfriend00 Sep 20 '18 at 15:24
@jfriend00 No, probably not. The OP however used a recursive `setTimeout` scheme, not `setInterval`, which lead me to believe that `// do some work` was asynchronous in itself. – Bergi Sep 20 '18 at 15:26
BTW, I upvoted you (not sure why someone downvoted). I just think a mention of `setInterval()` or a non-promise example just using `setTimeout()` as a safe functioning alternative would be useful too. – jfriend00 Sep 20 '18 at 15:28
@jfriend00 I only want to suggest that if you have a recursive promise-based function, and want to convert to async/await syntax, one needs to use iteration to avoid the memory leak issue that the question is about. But yeah, I also added a reminder that promises might not be necessary at all. – Bergi Sep 20 '18 at 15:34

score -7 · Answer 2 · answered Sep 20 '18 at 15:23

-7

To avoid memory leaks within a promise it's a good idea to clear all your timeouts after you use them.

function wait(delay) {
    return new Promise(function(resolve) {
        let id = setTimeout(function() { resolve(); clearTimeout(id);  }, delay);
    });
}

(async () => {
   await wait(2000);
   alert("Test");
})();

answered Sep 20 '18 at 15:23

Ryan Marin

94
1
5

2

There is no memory leak from `setTimeout()`. There is no need to call `clearTimeout()` on a timer that has fired. If you think otherwise, please provide some reference that shows that. – jfriend00 Sep 20 '18 at 15:26

Memory leak in nodejs promise design?

2 Answers2

Linked