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

Using .return() causes promise to be resolved as an empty object in 2.9.31 #689

Closed
jviotti opened this Issue Jul 3, 2015 · 9 comments

Comments

Projects
None yet
4 participants
@jviotti

jviotti commented Jul 3, 2015

I'm suddenly having an issue with .return() in exactly 2.9.31.

exports.has = (name, callback) ->
    exports.get(name).return(true)
    .catch errors.ResinApplicationNotFound, ->
        return false
    .nodeify(callback)

If exports.get() resolves, then the promise is resolved as an empty object {} instead of true.

Switching to .then() fixes the issue:

exports.has = (name, callback) ->
    exports.get(name).then ->
        return true
    .catch errors.ResinApplicationNotFound, ->
        return false
    .nodeify(callback)

I've created a branch in my project (https://github.com/resin-io/resin-sdk/tree/issue/bluebird-return) locking to 2.9.31 which reproduces the issue (reverting to 2.9.30 fixes the issue).

$ git clone https://github.com/resin-io/resin-sdk
$ cd resin-sdk
$ git checkout issue/bluebird-return
$ gulp test

By running the test suite you should get something like:

 2 failing
  1) Application Model: .has() given an application should eventually be true:
     AssertionError: expected {} to be true


  2) Device Model: .has() given the device should eventually be true:
     AssertionError: expected {} to be true

Application Model .has() function definition: https://github.com/resin-io/resin-sdk/blob/issue/bluebird-return/lib/models/application.coffee#L104.
Application Model .has() failing test: https://github.com/resin-io/resin-sdk/blob/issue/bluebird-return/tests/models/application.spec.coffee#L123.
Device model .has() function definition: https://github.com/resin-io/resin-sdk/blob/issue/bluebird-return/lib/models/device.coffee#L180.
Device Model .has() failing test: https://github.com/resin-io/resin-sdk/blob/issue/bluebird-return/tests/models/device.spec.coffee#L332.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Jul 3, 2015

Collaborator

@jviotti any help you can offer in isolating this issue to a more reduced test case would be greatly appreciated.

Collaborator

benjamingr commented Jul 3, 2015

@jviotti any help you can offer in isolating this issue to a more reduced test case would be greatly appreciated.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Jul 3, 2015

Owner
$ git clone https://github.com/resin-io/resin-sdk
$ cd resin-sdk
$ git checkout issue/bluebird-return
$ npm install
$ gulp test
[22:07:13] Requiring external module coffee-script/register
[22:07:13] Warning: gulp version mismatch:
[22:07:13] Global gulp is 3.9.0
[22:07:13] Local gulp is 3.8.11
[22:07:13] Using gulpfile ~/resin-sdk/gulpfile.coffee
[22:07:13] Starting 'test'...


  102 passing (712ms)

[22:07:16] Finished 'test' after 2.3 s

Also between 2.9.31 and 2.9.30 there isn't any change that could cause anything like this

Owner

petkaantonov commented Jul 3, 2015

$ git clone https://github.com/resin-io/resin-sdk
$ cd resin-sdk
$ git checkout issue/bluebird-return
$ npm install
$ gulp test
[22:07:13] Requiring external module coffee-script/register
[22:07:13] Warning: gulp version mismatch:
[22:07:13] Global gulp is 3.9.0
[22:07:13] Local gulp is 3.8.11
[22:07:13] Using gulpfile ~/resin-sdk/gulpfile.coffee
[22:07:13] Starting 'test'...


  102 passing (712ms)

[22:07:16] Finished 'test' after 2.3 s

Also between 2.9.31 and 2.9.30 there isn't any change that could cause anything like this

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Jul 3, 2015

Owner

Ok the error comes when I use node 0.10 or node 0.12 (I was initially using io.js 2.3).

Owner

petkaantonov commented Jul 3, 2015

Ok the error comes when I use node 0.10 or node 0.12 (I was initially using io.js 2.3).

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Jul 3, 2015

Owner

Ok this is because in older node.js, the domain functions are not running in strict mode (this is milestoned for 0.13.1).

Bluebird uses an optimization where values are ferried in the this value of functions - this is fine because bluebirds functions are in strict mode. However, the 2.9.31 version introduced a fix where all functions are bound to domain properly - because these domain bound functions run in non-strict before 0.13.1, primitives will get wrapped I.E. the test is failing because it's actually a wrapped true valued Boolean object.

This demonstrates what is happening:

(function() {

    function DomainBind(fn) {
        return function() {
            return fn.call(this);
        }
    }

    var func = function() {
        "use strict";
        return this;
    };

    var boundFunc = DomainBind(func);

    var result = boundFunc.call(true);

    console.log(result instanceof Boolean); // true
    console.log(result === true); // false

    var strictResult = func.call(true);

    console.log(strictResult instanceof Boolean); // false
    console.log(strictResult === true); // true
})();
Owner

petkaantonov commented Jul 3, 2015

Ok this is because in older node.js, the domain functions are not running in strict mode (this is milestoned for 0.13.1).

Bluebird uses an optimization where values are ferried in the this value of functions - this is fine because bluebirds functions are in strict mode. However, the 2.9.31 version introduced a fix where all functions are bound to domain properly - because these domain bound functions run in non-strict before 0.13.1, primitives will get wrapped I.E. the test is failing because it's actually a wrapped true valued Boolean object.

This demonstrates what is happening:

(function() {

    function DomainBind(fn) {
        return function() {
            return fn.call(this);
        }
    }

    var func = function() {
        "use strict";
        return this;
    };

    var boundFunc = DomainBind(func);

    var result = boundFunc.call(true);

    console.log(result instanceof Boolean); // true
    console.log(result === true); // false

    var strictResult = func.call(true);

    console.log(strictResult instanceof Boolean); // false
    console.log(strictResult === true); // true
})();
@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Jul 3, 2015

Owner

Fixed in 2.9.32

Owner

petkaantonov commented Jul 3, 2015

Fixed in 2.9.32

@myndzi

This comment has been minimized.

Show comment
Hide comment
@myndzi

myndzi Jul 4, 2015

I'm actually having an issue with the domains change and the traceview module. I'm inclined to put the fault with traceview since it's by nature meddling with bluebird's workings, but perhaps this issue applies after all.

/var/www/sites-backend/node_modules/bluebird/js/main/promise.js:367
                domain === null ? fulfill : domain.bind(fulfill);
                                                   ^
TypeError: Cannot read property 'bind' of undefined
    at Promise._addCallbacks (/var/www/sites-backend/node_modules/bluebird/js/main/promise.js:367:52)
    at Promise.ns_addCallbacks [as _addCallbacks] (/var/www/sites-backend/node_modules/@eros/app-framework/node_modules/traceview/node_modules/cls-bluebird/shim.js:18:34)
    at Promise._then (/var/www/sites-backend/node_modules/bluebird/js/main/promise.js:227:32)
    at Promise._passThroughHandler (/var/www/sites-backend/node_modules/bluebird/js/main/finally.js:84:17)
    at Promise.tap (/var/www/sites-backend/node_modules/bluebird/js/main/finally.js:96:17)
    at module.exports (/var/www/sites-backend/node_modules/@eros/app-framework/lib/index.js:74:6)

I don't actually know anything about domains, but maybe you can spot what's wrong so I can add it to my ticket with appneta?

myndzi commented Jul 4, 2015

I'm actually having an issue with the domains change and the traceview module. I'm inclined to put the fault with traceview since it's by nature meddling with bluebird's workings, but perhaps this issue applies after all.

/var/www/sites-backend/node_modules/bluebird/js/main/promise.js:367
                domain === null ? fulfill : domain.bind(fulfill);
                                                   ^
TypeError: Cannot read property 'bind' of undefined
    at Promise._addCallbacks (/var/www/sites-backend/node_modules/bluebird/js/main/promise.js:367:52)
    at Promise.ns_addCallbacks [as _addCallbacks] (/var/www/sites-backend/node_modules/@eros/app-framework/node_modules/traceview/node_modules/cls-bluebird/shim.js:18:34)
    at Promise._then (/var/www/sites-backend/node_modules/bluebird/js/main/promise.js:227:32)
    at Promise._passThroughHandler (/var/www/sites-backend/node_modules/bluebird/js/main/finally.js:84:17)
    at Promise.tap (/var/www/sites-backend/node_modules/bluebird/js/main/finally.js:96:17)
    at module.exports (/var/www/sites-backend/node_modules/@eros/app-framework/lib/index.js:74:6)

I don't actually know anything about domains, but maybe you can spot what's wrong so I can add it to my ticket with appneta?

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Jul 4, 2015

Owner

They are calling the internal _addCallbacks method incorrectly from

at Promise.ns_addCallbacks [as _addCallbacks] (/var/www/sites-backend/node_modules/@eros/app-framework/node_modules/traceview/node_modules/cls-bluebird/shim.js:18:34)
Owner

petkaantonov commented Jul 4, 2015

They are calling the internal _addCallbacks method incorrectly from

at Promise.ns_addCallbacks [as _addCallbacks] (/var/www/sites-backend/node_modules/@eros/app-framework/node_modules/traceview/node_modules/cls-bluebird/shim.js:18:34)
@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Jul 4, 2015

Owner
return _addCallbacks.call(this, fulfill, reject, progress, promise, receiver);

should be

return _addCallbacks.call(this, fulfill, reject, progress, promise, receiver, null);

in https://github.com/TimBeyer/cls-bluebird/blob/master/shim.js#L18

Owner

petkaantonov commented Jul 4, 2015

return _addCallbacks.call(this, fulfill, reject, progress, promise, receiver);

should be

return _addCallbacks.call(this, fulfill, reject, progress, promise, receiver, null);

in https://github.com/TimBeyer/cls-bluebird/blob/master/shim.js#L18

@myndzi

This comment has been minimized.

Show comment
Hide comment
@myndzi

myndzi Jul 4, 2015

That would explain it, thanks!

myndzi commented Jul 4, 2015

That would explain it, thanks!

jviotti pushed a commit to resin-io/resin-sdk that referenced this issue Jul 6, 2015

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