Error : Callback was already called when using pg-promise with async series

Question

I'm having trouble understanding the output printed why executing this code :

1 2 Unhandled rejection Error: Callback was already called.

It seems like both then and catch are executed when the query is successful.

Any idea ?

Cheers

    async.series([
            function(callback) {
                db.none(query)
                    .then(function () {
                        return callback(null, true);
                    })
                    .catch(function (err) {
                        return callback(err, null);
                    });
            },
            function(callback) {
                db.any(query)
                    .then(function (data) {
                        console.log('1')
                        return callback(null, data);
                    })
                    .catch(function (err) {
                        console.log('2')
                        console.log(err);
                        return callback(err, null);
                    });
            }
        ],
        function(err, results) {
           if (results && !results[1].isEmpty()) {
              // do something
           }
        });

EDIT :

TypeError: results[1].isEmpty is not a function

It seems like the problem come from the rest of the code and was just a simple undefined function error, thanks.

But i still don't understand something : why is this error catched inside the second query instead of outside the async queries ?

Have you tried `console.log(err)` and seeing if it gives you an idea of what is wrong? — Skabbi, Mar 14 '17 at 13:26
thanks, i thought i already did it but sometimes i forgot the simplest things :-) — , Mar 14 '17 at 13:38
can i ask why ? i had to, since i'm using pg-promise module to use postgres — , Mar 14 '17 at 17:16
Because promises already come with all the utilities you need, and trying to put callbacks atop of them just leads to mistakes such as this (and in general, very error-prone code). — Bergi, Mar 14 '17 at 18:39

vitaly-t · Accepted Answer · 2017-03-15T09:40:20.080

2

I'm the author of pg-promise.

You should never use async library with pg-promise, it goes against the concept of shared/reusable connections.

Implementation with proper use of the same connection, via a task:

db.task(t => {
    return t.batch([
        t.none(query1),
        t.any(query2)
    ]);
})
    .then(data => {
        // data[0] = null - result of the first query
        // data[1] = [rows...] - result of the second query

        callback(null, data); // this will work, but ill-advised
    })
    .catch(error => {
        callback(error, null); // this will work, but ill-advised
    });

See also: Chaining Queries.

However, in your case it looks like when you call the successful callback(null, data), it throws an error, which in turn results in it being caught in the following .catch section. To test this, you can change your promise handler like this:

    .then(data => {
        callback(null, data);
    }, error => {
        callback(error, null);
    });

It should normally throw an error about Promise missing .catch because you threw an error while in .then and there is no corresponding .catch chained below, which you can also check through this code:

    .then(data => {
        callback(null, data);
    }, error => {
        callback(error, null);
    })
    .catch(error => {
        // if we are here, it means our callback(null, data) threw an error
    });

P.S. You really should learn to use promises properly, and avoid any callbacks altogether. I only provided an example consistent with your own, but in general, converting promises into callbacks is a very bad coding technique.

edited Mar 15 '17 at 09:40

answered Mar 14 '17 at 15:46

vitaly-t

24,279
15
116
138

`then(… callback …).catch(… callback …)` is still wrong. Please remove it from your example code. – Bergi Mar 14 '17 at 18:37
@Bergi that's what the author was looking for, so it is quite bad, but not wrong, as it will work. That's why I added a comment following the code example. As much as I like promises myself, I'm trying to educate about them without preaching :) – vitaly-t Mar 14 '17 at 20:04
My point is that it indeed doesn't work. If one is tied to callbacks, one should at least use [`.then(res => callback(null, res), err => callback(err))`](http://stackoverflow.com/q/24662289/1048572) instead of `catch` – Bergi Mar 14 '17 at 21:31
@Bergi Why? I don't understand now. What you showed is identical to what I did, except for returning the callback results, which is irrelevant, and also according to the Bluebird's author that's considered an anti-pattern: https://github.com/petkaantonov/bluebird/wiki/Promise-anti-patterns#the-thensuccess-fail-anti-pattern – vitaly-t Mar 15 '17 at 07:35
1

No, `.then(res => { callback(null, res) }).catch(err => { callback(err) })` [is not identical](http://stackoverflow.com/q/24662289/1048572) to `.then(res => { callback(null, res) }, err => { callback(err) })`. And we want the latter here (please check the linked answer). – Bergi Mar 15 '17 at 07:46
Of course, given that pg-promise is using Bluebird (thanks for the reminder), you should recommend `.asCallback(callback)` :-) – Bergi Mar 15 '17 at 07:47
pg-promise can use Bluebird, among all others, which is also the recommended one, but doesn't mean the question's author uses it ;) – vitaly-t Mar 15 '17 at 08:45
Thanks a lot for this answer. As you say i'm just using promises because people told me "dont use callbacks, use promises instead" without really understanding it, i need to study a bit more i guess :-) but for the moment i will just use db.task as you suggested. Cheers – Mar 15 '17 at 14:53

score 1 · Answer 2 · answered Mar 14 '17 at 13:45

1

This is what happens:

callback(null, data) is called within the context of the .then();
async notices that this was the last item of the series, so it calls the final handler (still within the context of the .then());
the final handler throws an error;
because the code runs in the context of .then(), the promise implementation catches the error and calls the .catch();
this calls the callback again;

PoC:

const async = require('async');

async.series([
  callback => {
    Promise.resolve().then(() => {
      callback(null);
    }).catch(e => {
      callback(e);
    });
  }
], err => {
  throw Error();
})

answered Mar 14 '17 at 13:45

robertklep

198,204
35
394
381

The way to fix this would be [`.then(res => callback(null, res), err => callback(err))`](http://stackoverflow.com/q/24662289/1048572) – Bergi Mar 14 '17 at 17:14
@Bergi if the callback would fail, you'd still end up with an unhandled rejection. – robertklep Mar 14 '17 at 18:39
2

Yes, though I guess that's still better than "*Error: Callback was already*" (especially since hardly any callback consumers check for this, even async.js only added it because the erroneous behaviour confused so many users who made this mistake). In any case, dropping async.js is probably the best solution – Bergi Mar 14 '17 at 18:40

score -2 · Answer 3 · answered Mar 14 '17 at 13:31

Have you try to define your function externally:

function onYourFunction() {
   console.log('Hi function');
}

and than do:

.then(onYourFunction)    //-->(onYourFunction without parentheses )

Unfortunately i don't use pg-promise but i can advise promise

at this point i create all promises that are necessary:

function createPromise(currObj) {
return new Promise(function (resolve, reject) {
    currObj.save(function (errSaving, savedObj) {
        if(errSaving){
            console.log("reject!");
            return reject(errSaving, response);
        }

        console.log('currObj:' + currObj);
        return resolve(savedObj);
    });
});

}

and then in cascades:

var allPromiseOs = Promise.all(promise1, promise2, promise3);

Error : Callback was already called when using pg-promise with async series

3 Answers3