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

Possible memory leak with Promise.each #1057

Closed
tdzienniak opened this Issue Mar 29, 2016 · 8 comments

Comments

Projects
None yet
3 participants
@tdzienniak

tdzienniak commented Mar 29, 2016

I'm using bluebird v3.3.4 running on node v4.4.0. Leaky code:

const Promise = require('bluebird');

Promise.each(new Array(3000), () => {
    const longString = (new Array(1000000)).join('1');

    return Promise.resolve(longString);
});

Not leaky code:

const Promise = require('bluebird');

Promise.each(new Array(3000), () => {
    const longString = (new Array(1000000)).join('1');

    return Promise.resolve(longString).then(() => void 0);
});

It seems that the results of the promises are stored somewhere, but they shouldn't be, at least this is what I understand reading the docs of Promise.each:

Resolves to the original array unmodified, this method is meant to be used for side effects.

Issue is happening on earlier versions of bluebird too.

@tdzienniak

This comment has been minimized.

Show comment
Hide comment
@tdzienniak

tdzienniak Mar 29, 2016

I've noticed, that second snippet is also leaking memory when long stack traces are enabled. Is this desired behaviour?

tdzienniak commented Mar 29, 2016

I've noticed, that second snippet is also leaking memory when long stack traces are enabled. Is this desired behaviour?

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Mar 29, 2016

Collaborator

@tdzienniak this is because each is currently implemented through mapSeries https://github.com/petkaantonov/bluebird/blob/master/src/each.js#L14-L17 - the behavior of each was modified in 3.0 and then reverted when people complained.

Note that this is not a memory leak - it is merely wasteful behavior.

Collaborator

benjamingr commented Mar 29, 2016

@tdzienniak this is because each is currently implemented through mapSeries https://github.com/petkaantonov/bluebird/blob/master/src/each.js#L14-L17 - the behavior of each was modified in 3.0 and then reverted when people complained.

Note that this is not a memory leak - it is merely wasteful behavior.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Mar 29, 2016

Collaborator

We can implement directly with _iterate and overriding shouldCopyValues

PromiseArray.prototype.shouldCopyValues = function () {
return true;
};
- petka, I can take a shot at it but it's probably faster for you to fix it than to me to fix it and you to verify.

Collaborator

benjamingr commented Mar 29, 2016

We can implement directly with _iterate and overriding shouldCopyValues

PromiseArray.prototype.shouldCopyValues = function () {
return true;
};
- petka, I can take a shot at it but it's probably faster for you to fix it than to me to fix it and you to verify.

@tdzienniak

This comment has been minimized.

Show comment
Hide comment
@tdzienniak

tdzienniak Mar 29, 2016

@benjamingr yes, I've noticed that it is implemented through mapSeries, which is why with conjunction with long stack traces is wasting a lot of memory. I spent a lot of time trying to figure out why my node process is running out of memory. Maybe a note in the docs describing this behaviour will be helpful for others?

And well, this in not memory leak per se, but in context of desired Promise.each behaviour we can call it that.

tdzienniak commented Mar 29, 2016

@benjamingr yes, I've noticed that it is implemented through mapSeries, which is why with conjunction with long stack traces is wasting a lot of memory. I spent a lot of time trying to figure out why my node process is running out of memory. Maybe a note in the docs describing this behaviour will be helpful for others?

And well, this in not memory leak per se, but in context of desired Promise.each behaviour we can call it that.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr
Collaborator

benjamingr commented Apr 7, 2016

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Apr 7, 2016

Owner

@benjamingr feel free to give a shot at fixing it

Owner

petkaantonov commented Apr 7, 2016

@benjamingr feel free to give a shot at fixing it

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Aug 29, 2016

Collaborator

@tdzienniak if you feel like it - I would appreciate it if you take a look at the code with the fix and see if it fixes the issue for you.

Collaborator

benjamingr commented Aug 29, 2016

@tdzienniak if you feel like it - I would appreciate it if you take a look at the code with the fix and see if it fixes the issue for you.

@tdzienniak

This comment has been minimized.

Show comment
Hide comment
@tdzienniak

tdzienniak Aug 29, 2016

@benjamingr ok, I can test the examples from my initial post.

tdzienniak commented Aug 29, 2016

@benjamingr ok, I can test the examples from my initial post.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment