Strange null rejection related to method wrapping, finally and binding (regression)

[Joris-van-der-Wel]

Tested in 2.5.0, this does not occur in bluebird 2.4.2

I am not sure what is going wrong, but this is what I reduced my issue to:

function(t)
{
    var bar = P.method(function()
    {
        return P.bind(this)
        .then(P.method(function()
        {
            t.ok(true);
        }));
    });

    var foo = P.method(function()
    {
        return P.bind(this)
        .then(P.method(function()
        {
            return bar();
        }))
        .bind(this)
        .finally(P.method(function()
        {
            t.ok(true);
        }));
    });

    t.expect(3);

    foo()
    .then(function()
    {
        t.ok(true);
        t.done();
    });
}

[Joris-van-der-Wel]

Oh I forgot to mention that this gives me the following error:

Possibly unhandled Error: undefined
    at process._tickCallback (node.js:419:13)
From previous event:
From previous event:
    at D:\Users\joris_000\Documents\DOCS\Projects\node.js\stateful-controller\test\Controller.js:835:34
From previous event:
    at Object.module.exports.bluebird (D:\Users\joris_000\Documents\DOCS\Projects\node.js\stateful-controller\test\Controller.js:849:17)
    at Object.<anonymous> (D:\Users\joris_000\AppData\Roaming\npm\node_modules\nodeunit\lib\core.js:236:16)
    at D:\Users\joris_000\AppData\Roaming\npm\node_modules\nodeunit\lib\core.js:236:16
    at Object.exports.runTest (D:\Users\joris_000\AppData\Roaming\npm\node_modules\nodeunit\lib\core.js:70:9)
    at D:\Users\joris_000\AppData\Roaming\npm\node_modules\nodeunit\lib\core.js:118:25
    at D:\Users\joris_000\AppData\Roaming\npm\node_modules\nodeunit\deps\async.js:513:13
    at iterate (D:\Users\joris_000\AppData\Roaming\npm\node_modules\nodeunit\deps\async.js:123:13)
    at D:\Users\joris_000\AppData\Roaming\npm\node_modules\nodeunit\deps\async.js:134:25

[petkaantonov]

If this is what reduced case looks like, I am scared what the real code is :D Anyway, thanks I am looking into it.

[Joris-van-der-Wel]

haha it's a bit more clean in my library. There are probably somethings related to promises that I can implement in a smarter way, but I find that in certain cases it's hard to predict how some things in bluebird are going to behave, so in those cases I am a little bit more verbose to be on the safe side. (still 100x better than callbacks or fibers though :D)

The issue is very finicky, if I remove certain lines (e.g. the bind, the finally or the method call) the issue is suddenly gone. The issue is also gone if I add a .catch() before the finally()

[petkaantonov]

Yeah the library code is clean (I was wondering about all the method() calls in the repro), however you only need .bind once, it will keep it that way until you rebind.

My repro is currently:

        var bound = new String("asd");
        var bar = Promise.try(Promise.delay, 0, 0);

        Promise.resolve()
            .thenReturn(bar)
            .bind(bound)
            .finally(function() {
                return Promise.resolve(1);
            })
            .then(function() {
                done();
            });

[petkaantonov]

This is a race condition in the deep core, nothing special (as in other than constructor and .then) needed to repro:

var promise = new Promise(function(resolve) {
    resolve(Promise.resolve().then(function() {
        return new Promise(function(resolve) {
            setTimeout(resolve, 0);
        });
    }));
});

promise.then(function() {
    assert(promise.isResolved());
    done();
});

promise.isResolved() must be true, otherwise we wouldn't be inside the success handler. Finally makes this assumption that's why it manifests there.

[Joris-van-der-Wel]

Do you think this issue was introduced in the last two versions or did those changes just affect the timing?

[petkaantonov]

2.4.3 introduced a fix for the memory leak described here, which forced a big change in the foundations.

I specified the change needed in the spec to avoid the memory leak here promises-aplus/promises-spec#179 (comment)

The race condition is related to the part Any operation already performed on promise are immediately redirected to x, where x is already in the middle of calling its callbacks and promise moves its operations to x at the same time. Turned out to be a simple assumption error in my implementation... never mind