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

bluebird circular promise error while q-promise works #682

Closed
Ajkcki opened this Issue Jun 30, 2015 · 4 comments

Comments

Projects
None yet
4 participants
@Ajkcki

Ajkcki commented Jun 30, 2015

The following code uses q-library to wrap PouchDB as a promise

var Promise = require("q");
var PouchDB = require("pouchdb");
var PouchdbPromise = (function () {
    function PouchdbPromise(pouchdbName) {
        this._pouchdbP = null;
        this.pouchdbName = pouchdbName;
    }
    PouchdbPromise.prototype.getPouchdb = function () {
        var self = this;
        if (self._pouchdbP !== null)
            return self._pouchdbP;
        var deferred = Promise.defer();
        try {
            var _pouchdb = new PouchDB(self.pouchdbName);
            deferred.resolve(_pouchdb);
        }
        catch (e) {
            deferred.reject("Pouchdb init error:" + e);
        }
        self._pouchdbP = deferred.promise;
        return self._pouchdbP;
    };
    return PouchdbPromise;
})();

var pdbP = new PouchdbPromise("abc");
pdbP.getPouchdb().then(function (abc) {
    abc.get("1");
});

It works fine. But switching to bluebird by changing the first line to

var Promise = require("bluebird")

leads to the following error:

Unhandled rejection TypeError: circular promise resolution chain

    See http://goo.gl/LhFpo0

    at process._tickCallback (node.js:415:13)
From previous event:
    at PouchdbPromise.getPouchdb (SOMEDIR/app/scripts/playground/bluebird2.js:20:22)
    at Object. (SOMEDIR/app/scripts/playground/bluebird2.js:31:6)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Function.Module.runMain (module.js:497:10)
    at startup (node.js:119:16)
    at node.js:902:3

the error was thrown at line 20 deferred.resolve(_pouchdb).

I know Promise.defer() is depreciated, using new Promise gets the same error.

I also tried to run the code in a browser (Chrome 43), bluebird throws the same error and both q and Chrome's native promise work fine. I'm using bluebird version 2.9.30

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Jul 1, 2015

Owner

This is because pouchDb resolves with itself which normally leads to infinite loop. In fact doing this in Q leads to infinite loop while bluebird currently gives the circular error immediately:

var o = {
    then: function(f) {
        setTimeout(function() {
            f(o);
        }, 1);
    }
};

Q(o).then(function(){
    // Never happens.
})

The reason pouchDB works in Q is that pouchDB actually does something like this:

var o = {
    then: function(f) {
        setTimeout(function() {
            delete o.then;
            f(o);
        }, 1);
    }
};
Q(o).then(function(){
    // No problem.
})

So it won't lead to infinite loop because on second iteration there is no .then.

It's still a bug because Promises/A+ clearly requires an implementation to go into an infinite loop into the first scenario instead of rejecting with the circular resolution error.

Owner

petkaantonov commented Jul 1, 2015

This is because pouchDb resolves with itself which normally leads to infinite loop. In fact doing this in Q leads to infinite loop while bluebird currently gives the circular error immediately:

var o = {
    then: function(f) {
        setTimeout(function() {
            f(o);
        }, 1);
    }
};

Q(o).then(function(){
    // Never happens.
})

The reason pouchDB works in Q is that pouchDB actually does something like this:

var o = {
    then: function(f) {
        setTimeout(function() {
            delete o.then;
            f(o);
        }, 1);
    }
};
Q(o).then(function(){
    // No problem.
})

So it won't lead to infinite loop because on second iteration there is no .then.

It's still a bug because Promises/A+ clearly requires an implementation to go into an infinite loop into the first scenario instead of rejecting with the circular resolution error.

@bergus

This comment has been minimized.

Show comment
Hide comment
@bergus

bergus Jul 2, 2015

Contributor

Yeah, looks like a bug in Promises/A+. Would you mind to report it?

Contributor

bergus commented Jul 2, 2015

Yeah, looks like a bug in Promises/A+. Would you mind to report it?

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Jul 2, 2015

Collaborator

I think before opening an actual bug at A+ it'd be wise to ping @domenic
and ask about it. It's entirely possible this requirement has a perfectly
valid reason. Did you mean to reply on the Q issue I opened or here?

On Thu, Jul 2, 2015 at 2:05 PM, Bergi notifications@github.com wrote:

Yeah, looks like a bug in Promises/A+. Would you mind to report it?


Reply to this email directly or view it on GitHub
#682 (comment)
.

Collaborator

benjamingr commented Jul 2, 2015

I think before opening an actual bug at A+ it'd be wise to ping @domenic
and ask about it. It's entirely possible this requirement has a perfectly
valid reason. Did you mean to reply on the Q issue I opened or here?

On Thu, Jul 2, 2015 at 2:05 PM, Bergi notifications@github.com wrote:

Yeah, looks like a bug in Promises/A+. Would you mind to report it?


Reply to this email directly or view it on GitHub
#682 (comment)
.

@bergus

This comment has been minimized.

Show comment
Hide comment
@bergus

bergus Jul 2, 2015

Contributor

I meant to reply where I did :-) I don't think discussing this in a promisesaplus issue would have been wrong, even if the bug is invalid.

Contributor

bergus commented Jul 2, 2015

I meant to reply where I did :-) I don't think discussing this in a promisesaplus issue would have been wrong, even if the bug is invalid.

petkaantonov added a commit that referenced this issue Jul 9, 2015

Fixes #682
Promises/A+ requires not protecting against circular
resolution with thenables
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment