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 leak trying to create a while loop with promises #502

Closed
colmaengus opened this issue Feb 19, 2015 · 22 comments
Closed

Memory leak trying to create a while loop with promises #502

colmaengus opened this issue Feb 19, 2015 · 22 comments

Comments

@colmaengus
Copy link

@colmaengus colmaengus commented Feb 19, 2015

I'm trying to create a while loop using promises and I'm getting a memory leak.
An example of the pattern I'm using is

var promiseFor = BPromise.method(function(delay, condition, action, value) {
  if (!condition(value)) {
    return value;
  } else {
    return action(value).delay(delay).then(promiseFor.bind(null, delay, condition, action));
  }
});

var condition = function(value) {
  return value != 100000;
};

var action = function(value) {
  return BPromise.delay(10)
    .then(function() {
      console.log('value:' + value);
      return value + 1;
    });
};

var delay = 0;

promiseFor(delay, condition, action, 0)
  .then(function() {
    console.log('done');
  })
  .catch(function(e) {
    console.error(e.message);
  });

When I run this the memory used by node keeps increasing.
I'm using bluebird 2.9.10.
I've tried another approach to avoid the recursive nature of the approach above but it also has a leak.
Any suggestions on how to correctly implement a promise while loop ?

@benjamingr
Copy link
Collaborator

@benjamingr benjamingr commented Feb 19, 2015

Here is how I'd implement a while loop using bluebird:

var promiseWhile  = BPromise.coroutine(function*(delay, condition, action, value) {
    while(!condition(value){
          value = yield action(value);
          yield Promise.delay(delay);
    }
});
@colmaengus
Copy link
Author

@colmaengus colmaengus commented Feb 19, 2015

Looks nice but unfortunately for now we are using node 0.10 so we can't use generators.

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Feb 19, 2015

There is no need to implement any helper in the first place, just use recursion

(function loop(value) {
  if (value != 100000) {
    return Promise.delay(10).then(function() {
      console.log('value:' + value);
      return value + 1;
    }).then(loop);
  }
  return Promise.resolve(value);
})(0);
@colmaengus
Copy link
Author

@colmaengus colmaengus commented Feb 19, 2015

@petkaantonov unfortunately this approach also has a leak. On windows the node process gained about 14 MB in 20 mins. I'm looking for some way to keep running a promise function over a long period of time without leaking memory.

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Feb 19, 2015

@colmaengus It appears you are not actually using 2.9.10 then. Can you log this for me and let us know what it logs:

console.log(BPromise.prototype._rejectCallback + "");
@colmaengus
Copy link
Author

@colmaengus colmaengus commented Feb 19, 2015

function (reason, synchronous, shouldNotMarkOriginatingFromRejection) {
    if (!shouldNotMarkOriginatingFromRejection) {
        util.markAsOriginatingFromRejection(reason);
    }
    var trace = util.ensureErrorObject(reason);
    var hasStack = trace === reason;
    this._attachExtraTrace(trace, synchronous ? hasStack : false);
    this._reject(reason, hasStack ? undefined : trace);
}
@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Feb 19, 2015

Ok so we are surely using the same version then. I cannot reproduce any memory leak. Just to make sure we are on the same page here, can you run this:

var Promise = require('bluebird');

function immediate() {
    return new Promise(function(resolve) {
        setImmediate(resolve);
    });
}

(function loop(value) {
    return immediate().then(function() {
      if (value % 10000 === 0) {
        console.log(process.memoryUsage().heapUsed, value);
      }
      return value + 1;
    }).then(loop);
})(0);

If there is a real leak, the heap usage will grow rapidly.

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Feb 19, 2015

I ran it until 1110000 (equal to having the original code run for 3 hours) and the heap usage is almost same as when it started.

@colmaengus
Copy link
Author

@colmaengus colmaengus commented Feb 19, 2015

That seems to be pretty stable. The node process memory climbed to 44,304 K and pretty much stays there.
image
The heapUsed also seems pretty stable.
There are a few places I need to use this and in some cases the check condition uses a class function so I need a clean way to pass 'this' around while trying to keep the code readable.

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Feb 19, 2015

So do you agree there is no memory leak?

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Feb 19, 2015

Ok somehow the original code is not equivalent to the straight-forward recursion example and it indeed leaks : https://gist.github.com/petkaantonov/b73d52b9092b6f857271

@colmaengus
Copy link
Author

@colmaengus colmaengus commented Feb 19, 2015

I agree there is no memory leak leaving the interesting question of how to use while loop in practice.

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Feb 19, 2015

Well you can use your original code or my code.

It leaks when there is a .then() added for the console.log("Done"); but this didn't leak in 2.5.2 which fixed the original memory leak issue with promises in general.

@colmaengus
Copy link
Author

@colmaengus colmaengus commented Feb 19, 2015

Many thanks for your help. I let you know how I get on.

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Feb 19, 2015

This memory leak was introduced in 2.8.2 -> 2.9.0.

So you could use earlier version before I fix it (2.8.2 is the latest with no leak). 2.9.0 and later are affected

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Feb 19, 2015

Fixed in 2.9.12

@robey
Copy link

@robey robey commented Jun 27, 2015

I hate to pile on, but I think I'm running into another form of the same bug, using recursion as a while loop: https://gist.github.com/robey/de6717a41734d802c864

20.26 mb 1
34.39 mb 2
47.85 mb 3
67.94 mb 4
80.96 mb 5
99.04 mb 6
110.18 mb 7
123.17 mb 8
136.16 mb 9
156.67 mb 10
169.56 mb 11
182.55 mb 12
194.06 mb 13
@robey
Copy link

@robey robey commented Jun 27, 2015

removing the "catch" block also removes the leak. weird.

@robey robey mentioned this issue Jun 29, 2015
@spion spion reopened this Jun 29, 2015
@robey
Copy link

@robey robey commented Jun 29, 2015

This variant also works around it, which I'm using now:

    return get().catch(function (error) {
      active = false;
    }).then(function () {
      return active ? loop() : Promise.resolve();
    });
@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Jul 1, 2015

@robey That is not a similar bug or a bug at all, unfortunately. In the original bug all the promises have settled before starting the next iteration - there is no way to reach them and any memory used by them is therefore considered memory leak.

In your code you are creating an additional promise before moving on to the next iteration. The additional promise created can only be settled after the loop has ended which means it's reachable and therefore your example doesn't have a memory leak.

Let's write 5 iterations of your loop manually with logs placed:

function get() {
    return Promise.delay(5);
}

return get().then(function (v) {
    return get().then(function (v) {
        return get().then(function (v) {
            return get().then(function (v) {
                return get().then(function (v) {
                    console.log("finished loop");
                }).then(function (error) {
                    console.log("1 reached");
                });
            }).then(function (error) {
                console.log("2 reached");
            });
        }).then(function (error) {
            console.log("3 reached");
        });
    }).then(function (error) {
        console.log("4 reached");
    });
}).then(function (error) {
    console.log("5 reached");
});

This will log:

finished loop
1 reached
2 reached
3 reached
4 reached
5 reached

Which clearly shows that the promises (and their callbacks) must be reachable after the loop ends.

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Jul 1, 2015

@robey also using .catch for every iteration as you have done has actually no good purpose here. Rejection during any iteration of the loop will already stop the loop even if you place no catch at all.

@robey
Copy link

@robey robey commented Jul 2, 2015

so my workaround fixes my own bug by making sure the loop is the last call in the promise. makes sense; thanks for digging into it! :)

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
5 participants
You can’t perform that action at this time.