chain of never resolved promises create memory leaks

[jonathanong]

is this the right place to ask this?

tj/co#180 (comment)

[ForbesLindesay]

This is part of the reason for having 2.3.2 in the Promise Resolution Procedure:

If x is a promise, adopt its state

This is separate from the specification of the resolution procedure for thenables. Because it doesn't specify how that state is adopted, we allow the promise to use some internal means to unravel the stack (providing all promises come from the same implementation). This probably hasn't been implemented in practice though, and the spec does not explicitly state that a reference should not be held.

I'll have a go at testing and fixing this in then/promise and post back here to let you know how that goes.

[ForbesLindesay]

I have made something that I think fixes the memory leak in then/promise (then/promise#67) It would be great if you could give it a more thorough testing to see when/if it fails. I'm particularly concerned that it might have created memory leaks where there was none before.

[bitinn]

We implement a job queue using this exact pattern, should we be worried? Seem like a mistake a lot of developer will make, eg. http://stackoverflow.com/questions/15027192/how-do-i-stop-memory-leaks-with-recursive-javascript-promises

Please let me know if I missed the point...

[ForbesLindesay]

@bitinn yes, unfortunately that will leak memory (until the promise chain unravells) with most current promise implementations. It should work fine with the fix linked to above (using then/promise rather than Q).

The solution is very similar to the concept of tail recursion in synchronous code.

[bitinn]

@ForbesLindesay

seem like if I do this it won't leak (return p instead of p2 will leak), is this a good approach or still implementation dependent?

var i = 0;

function run() {
    var p = new Promise(function(resolve) {
        if (i % 10000 === 0) {
            global.gc();
            console.log(process.memoryUsage());
        }
        i++;
        setImmediate(resolve);
    }).then(run);

    var p2 = new Promise(function() {
        return p;
    });

    return p2;
}

run();

[petkaantonov]

@bitinn much simpler would just be leaving out the return if you don't need it

var i = 0;

function run() {
    new Promise(function(resolve) {
        if (i % 10000 === 0) {
            global.gc();
            console.log(process.memoryUsage());
        }
        i++;
        setImmediate(resolve);
    }).then(run);
}

run();

[bitinn]

@petkaantonov got it, cheers, we kinda need it to return promise as we called it with this.run().catch(...) but I can see it's not required for recursion.

[ForbesLindesay]

@bitinn

var p2 = new Promise(function() {
  return p;
});

is equivalent to:

var p2 = new Promise(function() {
});

because the return value of the factory function is ignored. You probably meant to do:

var p2 = new Promise(function(resolve) {
  resolve(p);
});

which would have still leaked.

For the original example of doing something asynchronous continuously:

function run() {
  return doAsyncWork().then(run);
}

run();

you could fix the memory leak in all promise implementations by breaking the chain of promises:

function run() {
  return new Promise(function (resolve, reject) {
    function recurse() {
      doAsyncWork().then(recurse, reject);
    }
    recurse();
  });
}

run();

It's a little dirty, but by having an inner recurse function that does not return a promise, I am able to break the chain each time the function recurses. To make sue each recursion also handles errors by passing them to reject I maintain the same error handling behaviour as the original.

[ForbesLindesay]

An interesting question, should we specify that promises are not permitted to leak memory in this use case? What would such a specification look like.

[petkaantonov]

Well the whole 2.3.2 needs to be redone.

As far as I can tell we both fixed the memory leak in our implementations by turning promise into a proxy for x: any operation performed on promise will be redirected to x. Any operation already performed on promise are immediately redirected to x (which is possible because both must still be pending at this point).

This is different from what the spec currently says, promise is now never pending, fulfilled or rejected, it doesn't have its own state at all. If it had, that means x would need to inform promise when it changed state so that promise can change its state and that means x holding reference to promise which leads to the original leak.

In the case x is already fulfilled or rejected then it's simply same case as if the original handler returned direct value or threw direct error.

[stefanpenner]

An interesting question, should we specify that promises are not permitted to leak memory in this use case?

The spec as written today does allow implementors to avoid this leak. This leads me to believe that it may not be a spec concern rather the concern of implementors. I suppose one could argue that if many implementations exhibit this behavior, the spec wording may need to be refined. That being said, this feels like the responsibility of a supplementary compliance test suite.

[ForbesLindesay]

@stefanpenner As suggested by @petkaantonov, we haven't really done what the spec says to do. In order to fix the leak we had to make promise into a proxy for x, which is not quite the same thing as making promise adopt the state of x. The difference is not observable to the outside world, but we aren't really complying with what the spec says to do.

[stefanpenner]

The difference is not observable to the outside world

I believe the spec intends to describe the observable behavior, not implementation.

I do believe the wording can be improved, but I am nervous of the spec dictating implementation details.

[petkaantonov]

What I just specified does change observable behavior

var resolveA;
var a = new Promise(function() {
    resolveA = arguments[0];
});

a.then(function() {
    console.log("first");
});

var resolveB;
var b = new Promise(function() {
    resolveB = arguments[0];
});

b.then(function() {
    console.log("second");
});

resolveA(b);

b.then(function() {
    console.log("third");
});

resolveB();