Error after Promise upgrade to 7.0.0

[kcparashar]

Everything was working until yesterday. Any ideas on what broke?

Users/USER/Documents/APPLICATION/api/node_modules/pg-promise/node_modules/promise/node_modules/asap/asap.js:45
throw e;
^
ReferenceError: reason is not defined
at /Users/USER/Documents/APPLICATION/api/app/routes/home.js:8:19
at /Users/USER/Documents/APPLICATIONUSER/Documents/APPLICATION/api/node_modules/pg-promise/node_modules/promise/node_modules/asap/asap.js:27:13)
at process._tickCallback (node.js:355:11)
[23:24:18] 'dev:app' errored after 19 s
[23:24:18] Error in plugin 'gulp-shell'
Message:
Command node app/app.js failed with exit code 1
Error running task sequence: { task: 'dev:app',
message: 'dev:app stream',
duration: 18.899271645,
hrDuration: [ 18, 899271645 ],
err:
{ [Error: Command node app/app.js failed with exit code 1]
message: 'Command node app/app.js failed with exit code 1',
showStack: false,
showProperties: true,
plugin: 'gulp-shell',
__safety: { toString: [Function] } } }

[vitaly-t]

Do I take it right that you use the latest version 0.9.2? If that's the case, the only thing that changed there - the default Promise library was upgraded from 6.1 to 7.0.

And according to your error log, this is exactly what's causing the problem. To prove it, use a different promise library, like Promise 6.1 or Bluebird. The default promise is very easy to override during the library's initialization: https://github.com/vitaly-t/pg-promise#promiselib

Let me know if this solved the problem, and then we can investigate further what it is exactly that's causing the failure inside that promise library. But for the time being this really looks like something got broken in Promise 7.0

[ForbesLindesay]

Are you using domains?

[vitaly-t]

@ForbesLindesay I understand this question is for the original poster and not myself. I hope he will come back to us tomorrow with some results and details. But if you want to elaborate in the meantime about the implication from domains usage, that'd be excellent. Cheers!

[kcparashar]

@vitaly-t It is indeed an issue with the Promise module (I believe thru the asap module). I tried Bluebird and an older version of Promise and they both work. Thanks for your help and for forwarding the issue on to the Promise developer.

@ForbesLindesay, sorry I am not sure what you mean by "using domains". Could you please elaborate a little bit?

[ForbesLindesay]

domains are a feature of node.js that allows you to handle certain errors even when they are thrown into the global context. The latest promise update removed support for using domains with promises, so it could cause your bug. In general, domains are not recommended as they tend to leave applications in a really inconsistent state.

The only other breaking change I'm aware of is that .then is no-longer automatically bound to the promise in the newest version. e.g.

var foo = new Promise(function (resolve) {
  setTimeout(function () {
    resolve(42);
  }, 100);
});
var then = foo.then;
then(function (result) {// this throws an error, because `then` is not bound to `foo`.
  console.log(result);
});

[vitaly-t]

@ForbesLindesay your example throws an error different from the one you described:

..\promise\lib\core.js:71
  if (this.constructor !== Promise) return this._10(onFulfilled, onRejected);

And I did a simpler test to understand it, so the following code works in both 6.1 and 7.0, printing test

function work(){
    return new promise(function(resolve, reject){
        resolve('test');
    });
}
var w = work();
w.then(function(data){
    console.log(data);
});

But if we change it to:

function work(){
    return new promise(function(resolve, reject){
        resolve('test');
    });
}
var w = work().then;
w(function(data){
    console.log(data);
});

then similarly the same code works with 6.1, but with 7.0 it throws the same error that I quoted above.

I do not know anything about the domains and the implications of using them, but we haven't established that as the reason. In fact, my first guess was and still is, one of the updated dependencies in Promise got broken. This guess is also backed up by the fact that this library (pg-promise) never uses that broken pattern above.

Another point of concern and/or argument - Bluebird works for the guy, no issues.

@parashar I'm glad that you have resolved the issue in production with the default promise override, so you can help find the problem without being under pressure.

[ForbesLindesay]

That is definitely interesting. I'd love to see an example that reproduces the error, then I could try and fix it, but without that I really can't do much. Promise passes the full Promises/A+ test suite, as well as a number of its own tests, some of which test some pretty stringent edge cases.

My guess is that @parashar is depending on some unspecified ordering/race condition somewhere, and that the two different promises fire the handlers in a subtly different order.

[vitaly-t]

@ForbesLindesay I can only say the same about pg-promise that it runs every test I could think of without any issue. At some point though, I changed the test suit to use Bluebird, because I had to use method any for one test, which is missing in Promise, but that doesn't matter. I just changed it here locally to Promise 7.0, commented out one test that uses any, and it's all pass.

At this point I agree, we need more details from the author, otherwise we cannot proceed.

[vitaly-t]

@parashar, Please get back to us with more details, otherwise we cannot proceed.

[kcparashar]

@vitaly-t, @ForbesLindesay
Thanks for trying to resolve this. Since I am quite new to using promises in general, I have used very sparingly; in fact this is really the only spot where there is any sort of complexity:

exports.query = function (query, q_params, cb) {
    var params;
    if (arguments.length == 3) {
        params = q_params;
    }
    if (arguments.length == 2) {
        cb = q_params;
    }
    db.connect()
        .then(function (conn) {
            return conn.query(query, params);
        },
        function (err) {
            cb(reason, null);
        })
        .then(function (data) {
            cb(null, data);
        },
        function (err) {
            cb(err, null);
        })
        .done();
};

I don't think I know enough about this subject to aptly contribute to this discussion. But perhaps if their are particular queries I may be able to answer let me know!

[vitaly-t]

@parashar , there is a number of issues with your code:

1. Variable param is set only if arguments.length == 3 , I don't know if this is by design or an omission;
2. If an error happens during a connection, you do not return a promise, so the chain that follows it is broken;
3. You call done(), but do not actually close the connection that you open above. The connection needs to be closed using conn.done(); in your case.
4. Why do you open the connection in the first place, when it is not even needed there?
5. If your function cb throws an exception anywhere, your code doesn't handle it, breaking the promise chain.

In all, your whole code example looks like an attempt to take a promise chain and turn it into a callback chain, which is reversing the point of using promises in the first place.

So, first, let's simplify the code:

exports.query = function (query, q_params, cb) {
    var params;
    if (arguments.length == 3) {
        params = q_params;
    }
    if (arguments.length == 2) {
        cb = q_params;
    }
    db.query(query, params)
        .then(function (data) {
            cb(null, data);
        }, function (reason) {
            cb(reason, null);
        });
};

And second, you need to check whether your callback can throw an exception, and then also add .catch(err) to the end of your chain.

Other than that, I really suggest that you look at how to use promises properly. And as for this library, Learn by Example tutorial might be of help also ;)

In the meantime, I'm closing the issue, because it is quite possible for the issues in your code to break promises. If you review and change it to a proper usage and still find errors happening, then we can reopen it and see what's going on.

[ForbesLindesay]

@parashar The problem is that .then(null, function (err) { cb(reason, null); }) is always going to result in a reference error on reason whenever any other error is thrown. If you are using then/promise, you should be able to just use the nodeify api to interface with promises. So I would re-write your code as:

exports.query = function (query, q_params, cb) {
    var params;
    if (arguments.length == 3) {
        params = q_params;
    }
    if (arguments.length == 2) {
        cb = q_params;
    }
    db.connect()
        .then(function (conn) {
            return conn.query(query, params);
        })
        .nodeify(cb);
};

This will still leak db connections as @vitaly-t says. If you want to learn how to use promises properly, and why they are important, try reading https://www.promisejs.org/ (or alternatively watch https://www.youtube.com/watch?v=qbKWsbJ76-s if you'd prefer it given as a talk).

[vitaly-t]

@ForbesLindesay , thank you for your help! 👍

[kcparashar]

@ForbesLindesay, @vitaly-t, Thank you! You all are awesome!

[kcparashar]

@AndrewBrinker

[vitaly-t]

@parashar, Unrelated to this, but if you liked this library, I really appreciate if you can give me a feedback on this small addition: pg-monitor