Do not override resolve and reject functions with null

[irnc]

Promise resolver, the function(success, error), is called synchronously. This is documented behavior of bluebird. However callback passed to self.exec is expected to be called asynchronously, but it is actually called synchronously in case of CastError (happening on mongoose side).

Calling callback passed to self.exec asynchronously will require careful refactoring, leaving the root cause aside, we can fix one use case right now with small change. I propose to remove setting resolve and reject to null as a fix for this use case, as it does not make sense to null them: these variables are not accessible from callback scope, thus they will not be put in environment when executing callback, so they will be eventually cleared by GC.

Consider following mongoose code:

Model.findOne({ numericProperty: '1a' }).then(success, error);

It is expected to execute error asynchronously, but it actually try to call it synchronously after setting reject to null and fail with

Warning: .then() only accepts functions but was passed: [object Null], [object Null]
    at Query.then ([path]\node_modules\mongoose\node_modules\mquery\lib\mquery.js:2354:18)
...
Unhandled rejection CastError: Cast to number failed for value "1a" at path "numericProperty"
    at SchemaNumber.cast ([path]\node_modules\mongoose\lib\schema\number.js:217:9)

[vkarpov15]

Hmm could we just work around this by wrapping the self.exec() call in a setImmediate()? Would be weird in case other promises libraries don't do it that way, but at least we wouldn't crash with the default promises library.

[irnc]

I think the root issue should be fixed in mongoose, in the code that calls callback synchronously. Can not check it right now, but I assume the issue is in mongoose's Promise implementation which calls listeners synchronously.

Leaving mongoose aside, is there a point in setting three variables from patch to null?

[vkarpov15]

I think the general idea is that subsequent calls to then() error out. FWIW, this is a bug in bluebird and probably should be fixed there, Promises/A+ 2.2.1 specifies that rejected must be called asynchronously.

[irnc]

@vkarpov15 This is not a bluebird's bug, there is one in mquery for nulling resolve and reject and there is one in mpromise: callback passed to exec should be executed asynchronously.

If there is no objections to proposed change, please apply it to allow catching casting errors, as fixing mpromise may be a complex task, which does not make sense as it may be replaced with other libraries in future.

[Jeff-Lewis]

@vkarpov15 I think this issue possibly suffered from some poor communication. Let me know your thoughts please.

• The only 'promise resolver', that I know of, that is called synchronously in Bluebird is the function passed into new Promise(myPromiseResolverFunc). The Bluebird docs on creating a new Promise() show how the parent scoped resolver function, which is passed into new Promise() is called synchronously to ensure the closure variables are setup properly. See the theResolverFunction below.

function getPromiseResolveFn() {
    var res;
    new Promise(function theResolverFunction(resolve) {
        res = resolve;
    });
    // res is guaranteed to be set
    return res;
}

Does the Promise/A+ spec cover how a promise is created? It wasn't obvious to me if it does.

• According to Bluebird's docs it passes the Promises/A+ test suite and is fully spec compliant. Thus it always calls onRejected asynchronously.
• I know mpromise will be going away soon but the actual bug @irnc is trying to report in (Do not override resolve and reject functions with null #66) is that sometimes the onReject in mpromise was being called synchronously, possibly in the case of CastErrors, which is not Promises/A+ compliant.
• @irnc has since deleted is pull request so I'm not sure what he was suggesting. It appears to me @irnc was asking for mpromise to "callbacks passed to exec should be executed asynchronously.", not synchronously. Yet, the commit by Aaron added MORE support for exec cb being called synchronously.

Do you know off hand if this is the case with mpromise? Are CastErrors called synchronously on the cb or onRejected() of the promise?

I'm interested in this issue as I see Bluebird complaining often about Mongoose CastErrors in our app.