petkaantonov / bluebird

:bird: :zap: Bluebird is a full featured promise library with unmatched performance.
http://bluebirdjs.com
MIT License
20.45k stars 2.33k forks source link

Inconsistent behavior of execution when using multiple copies of bluebird #1533

Closed fogine closed 6 years ago

fogine commented 6 years ago

1) What version of bluebird is the issue happening on?

v3.5.1

2) What platform and version? (For example Node.js 0.12 or Google Chrome 32)

Node.js 6.8.1 & Node.js 8.11.1

3) Did this issue happen with earlier version of bluebird?

It also happens with bluebird v3.4.7

To replicate:

  1. mkdir bluebird-test
  2. cd bluebird-test
  3. npm init --yes
  4. npm i bluebird --save
  5. cp -r ./node_modules/bluebird ./node_modules/bluebird2
  6. touch index.js
  7. copy & paste the following into the index.js
  8. node index.js
const Promise = require('bluebird');
const Promise2 = require('bluebird2');

function second() {
    console.log('SECOND');
}

function start() {
    return new Promise(function(resolve, reject) {

        process.nextTick(tick);

        function tick() {
            return setup().catch(function(err) {
                console.log('FIRST');
            });
        }
    });
}

function setup() {
    return Promise.resolve().then(function() {
        //Promise2 executes/is handled differently
        return Promise2.map([1], function() {
            process.nextTick(function() {
                second();
            });
            throw new Error('test error');
        });
    });
}

return start();

stdout:

SECOND
FIRST

expected stdout:

FIRST
SECOND

When only a single bluebird package is used as in the script bellow, I get the expected output;

const Promise = require('bluebird');

function second() {
    console.log('SECOND');
}

function start() {
    return new Promise(function(resolve, reject) {

        process.nextTick(tick);

        function tick() {
            return setup().catch(function(err) {
                console.log('FIRST');
            });
        }
    });
}

function setup() {
    return Promise.resolve().then(function() {
        return Promise.map([1], function() {
            process.nextTick(function() {
                second();
            });
            throw new Error('test error');
        });
    });
}

return start();

This is the case when some dependent npm package requires different version of bluebird or simply project's dependencies are not flatten out. Understandably this breaks things.

Despite that, Thank you for the amazing library!

benjamingr commented 6 years ago

Bluebird instances identify each other and do a performance optimization where they don't need to do the whole ResolveThenableJob dance they need to with native promises.

This makes sense since a large portion of the Node.js ecosystem uses bluebird so it is very common for the ORM and fs library to both use bluebird and for them to consume each other.

Note bluebird makes no guarantees about the order it schedules stuff - other than it does not schedule it synchronously. You can tell bluebird explicitly what to do with setScheduler.

We're not very inclined to make further non bugfix changes to bluebird since it is used by such a large portion of the platform to be honest - but we've discussed changing the scheduler to microtask (Rather than macrotask) semantics.

fogine commented 6 years ago

Thank you for your prompt response @benjamingr .
Even though I've come around the issue with more robust implementation. I'm still trying to understand bluebird's internals and how it addresses the optimization in this case.

I've nailed the code example down to:

const Promise = require('bluebird');
const Promise2 = Promise.getNewLibraryCopy();

return Promise.resolve().then(function then() {
    return new Promise2(function(resolve, reject) {
        process.nextTick(function() {
            console.log('SECOND'); //is first written to stdout 
        })
        reject(new Error('testerror'));
    });
}).catch(function() {
    console.log('FIRST'); //is written to stdout second
});

As far as I can currently see, it comes down to how Promise2 is being resolved in thenables.js#L17 as it falls down to Promise2 not being recognized as direct instanceof Promise thus wrapped in a new internal Promise thenable.

What I do not currently understand is why it needs to be handled this way and eventually what's the performance optimization. @benjamingr, please could you try to shed some light on it? My intentions are purely educational.
Also, I do not think it is possible to embrace existence of setScheduler to effectively change order of scheduling so that above example would behave identically to the same code example which doesn't involve a promise of different "library copy" in its promise chain. Am I wrong? If so, could you provide an example?

benjamingr commented 6 years ago

Bluebird makes no guarantees on scheduling other than what the Promises/APlus spec requires.

Are you asking why it does this?

benjamingr commented 6 years ago

It just short-circuits to ._then https://github.com/petkaantonov/bluebird/blob/master/src/thenables.js#L19

fogine commented 6 years ago

Bluebird instances identify each other and do a performance optimization where they don't need to do the whole ResolveThenableJob dance they need to with native promises.

What's the implementation reason behind NOT returning the original promise object for object that isAnyBluebirdPromise as it is done: https://github.com/petkaantonov/bluebird/blob/49da1ac256c7ee0fb1e07679791399f24648b933/src/thenables.js#L10

benjamingr commented 6 years ago

What's the implementation reason behind NOT returning the original promise object for object that isAnyBluebirdPromise as it is done:

It might be a different version of bluebird and not contain the same methods - for example Bluebird 2 and Bluebird 3.