Sequelize wait until loop finished with callback

Question

coming from a php background, I'm trying to get my head around this callback stuff.

Basically I wanna get some rows, then I would like to loop through these rows and check them against an other model (different db). I want the call back to wait until they all have been looped through and checked.

The callback gets called before sequelize has looped through all the results.

Basically I want the function to be 'blocking'. What do I have to change?

toexport.getlasttransactions = function(lower,upper,callback){
    var deferred = Q.defer();
    var transactionsToUpdate = [];
    ///////////////////////////
    // set import conditions //
    ///////////////////////////
    var lowerbound = (lower) ? lower.format() : moment.utc().subtract(10, 'minutes').format();
    var upperbound = (upper) ? upper.format() : moment.utc().format();

    ///////////////////////////////
    // get IDs From Failed syncs //
    ///////////////////////////////
    FailedSync.find({ limit: 100 })
    .then(function(res){
        var FailedIDs = [];
        _.each(res, function(value,index){
            FailedIDs.push(value.transaction_id);
        });

        // build condition
        var queryCondition = { where: { updated_at: { between: [lowerbound,upperbound] } }, limit: 3 };
        if(FailedIDs.length > 0){
            queryCondition = {
                where: Sequelize.and({ updated_at: { between: [lowerbound,upperbound] } },
                Sequelize.or(
                  { id: FailedIDs }
                ))
            }
        }
        //////////////////////////////
        // get Phoenix Transactions //
        //////////////////////////////
        PhoenixTransaction
        .findAll(queryCondition)
        .then(function(poenixTrx){

            _.each(poenixTrx, function(value, index){

                Transaction.findOne({ where: { id: value.id }})
                .then(function(result){

                    if(!result || result.length === 0){
                        transactionsToUpdate.push(value);
                        console.log('!result || result.length === 0')
                    }
                    else if(result && result.length === 1){
                        if(result.hash != value.hash){
                            transactionsToUpdate.push(value);
                            console.log('result.hash != poenixTrx[i].hash')
                        }
                    }




                })
                .catch(function(err) {
                  console.log(err)
                })


            })
            deferred.resolve(transactionsToUpdate);


        })
        .catch(function(err){
          throw new Error("Something went wrong getting PhoenixTransaction") 
        })

    })

    deferred.promise.nodeify(callback);
    return deferred.promise;    

}

What version of node are you using (Or are you using the newer io.js?)? — Benjamin Gruenbaum, Feb 13 '15 at 16:27
Also - http://stackoverflow.com/questions/23803743/what-is-the-deferred-antipattern-and-how-do-i-avoid-it — Benjamin Gruenbaum, Feb 13 '15 at 16:27

score 3 · Answer 1 · answered Feb 13 '15 at 16:40

You have a lot of patterns new promise users have in your code:

You're using a deferred when you don't need to.
You're not using promise aggregation methods
You're not waiting for things in appropriate places but nesting instead.

Promises represent a value over time. You can use promises and access their result via then at a later point and not just right away - Sequelize's promises are based on bluebird and offer a rich API that does aggregation for you. Here is an annotated version of cleaned up code - note it is not nesting:

toexport.getlasttransactions = function(lower,upper){ // no need for callback
    var lowerbound = (lower || moment.utc().subtract(10, 'minutes')).format();
    var upperbound = (upper || moment.utc()).format();
    // use `map` over a `each` with a push.
    var failedIds = FailedSync.find({ limit: 100 }).map(function(value){ 
        return value.transaction_id;
    });
    // build condition.
    var queryCondition = {
        where: { updated_at: { between: [lowerbound,upperbound] } }, limit: 3 
    };
    var query = failedIds.then(function(ids){ // use promise as proxy
        if(ids.length === 0) return queryCondition;
        return { // You can return a value or a promise from `then`
            where: Sequelize.and({ updated_at: { between: [lowerbound,upperbound] } },
                   Sequelize.or({ id: ids});
        };
    });
    var pheonixTransactions = query.then(function(condition){
        return PhoenixTransaction.findAll(queryCondition); // filter based on result
    });
    return pheonixTransactions.map(function(value){ // again, map over each
        return Transaction.findOne({ where: { id: value.id }}); // get the relevant one
    }).filter(function(result){ // filter over if chain and push
        return (!result || result.length === 0) || 
               ((result && result.length === 1) && result.hash != value.hash);
    });
};

score 0 · Answer 2 · answered Feb 13 '15 at 16:22

Ideally you'll want to either use something like Bluebird's reduce with an array of promises, but I'll provide an async.series implementation as its easier to understand.

Install async

npm install async

Require it in your file

var async = require('async')

Then implement it as such:

        //////////////////////////////
        // get Phoenix Transactions //
        //////////////////////////////
        PhoenixTransaction
        .findAll(queryCondition)
        .then(function(poenixTrx){

            var queryArray = poenixTrx.map(function(value){
                return function(callback){
                    Transaction.findOne({ where: { id: value.id }})
                    .then(function(result){

                        if(!result || result.length === 0){
                            transactionsToUpdate.push(value);
                            console.log('!result || result.length === 0')
                        }
                        else if(result && result.length === 1){
                            if(result.hash != value.hash){
                                transactionsToUpdate.push(value);
                                console.log('result.hash != poenixTrx[i].hash')
                            }
                        }

                        // trigger callback with any result you want
                        callback(null, result)
                    })
                    .catch(function(err) {
                      console.log(err)
                      // trigger  error callback
                      callback(err)
                    })
                }

            })

            // async.series will loop through he queryArray, and execute each function one by one until they are all completed or an error is thrown.
            // for additional information see https://github.com/caolan/async#seriestasks-callback
            async.series(queryArray, function(err, callback){
                // after all your queries are done, execution will be here
                // resolve the promise with the transactionToUpdate array
                deferred.resolve(transactionsToUpdate);
            })


        })
        .catch(function(err){
          throw new Error("Something went wrong getting PhoenixTransaction") 
        })

Sequelize uses bluebird promises you can just use them. – Benjamin Gruenbaum Feb 13 '15 at 16:26 — Benjamin Gruenbaum, Feb 13 '15 at 16:26

Ben A. · Answer 3 · 2015-02-13T16:56:00.280

The whole thing is a little messy to be honest. Especially the promise/callback mix up will probably cause you problems at some point. Anyway you use the deferred.resolve on the transactionsToUpdate which is just an array so it calls the callback right away.

If you keep that script as it is use instead of _.each something like async (https://github.com/caolan/async) to run your transactions in paralell and use that as callback.

It could look like this:

toexport.getlasttransactions = function(lower,upper,callback){
    var transactionsToUpdate = [];
    ///////////////////////////
    // set import conditions //
    ///////////////////////////
    var lowerbound = (lower) ? lower.format() : moment.utc().subtract(10, 'minutes').format();
    var upperbound = (upper) ? upper.format() : moment.utc().format();

    ///////////////////////////////
    // get IDs From Failed syncs //
    ///////////////////////////////
    FailedSync.find({ limit: 100 })
    .then(function(res){
        var FailedIDs = [];
        _.each(res, function(value,index){
            FailedIDs.push(value.transaction_id);
        });

        // build condition
        var queryCondition = { where: { updated_at: { between: [lowerbound,upperbound] } }, limit: 3 };
        if(FailedIDs.length > 0){
            queryCondition = {
                where: Sequelize.and({ updated_at: { between: [lowerbound,upperbound] } },
                Sequelize.or(
                  { id: FailedIDs }
                ))
            }
        }
        //////////////////////////////
        // get Phoenix Transactions //
        //////////////////////////////
        PhoenixTransaction
        .findAll(queryCondition)
        .then(function(poenixTrx){

            async.each(poenixTrx, function(value, next){

                Transaction.findOne({ where: { id: value.id }})
                .then(function(result){

                    if(!result || result.length === 0){
                        transactionsToUpdate.push(value);
                        console.log('!result || result.length === 0')
                    }
                    else if(result && result.length === 1){
                        if(result.hash != value.hash){
                            transactionsToUpdate.push(value);
                            console.log('result.hash != poenixTrx[i].hash')
                        }
                    }

                    next();
                })
                .catch(function(err) {
                  console.log(err)
                })


            }, function(err) {
              //Return the array transactionsToUpdate in your callback for further use
              return callback(err, transactionsToUpdate);
            });
        })
        .catch(function(err){
          throw new Error("Something went wrong getting PhoenixTransaction") 
        })

    })

}

Which would be the way with a callback. But you need make your mind up what you want to use: callback OR promises. Don't use both together (as in: If your method expects a callback it shouldn't return a promise or if it returns a promise it shouldn't expect a callback).

Additional if you use callback you don't want to throw errors, you just call the callback and give the error in the callback - whoever uses your method can check the error from the callback and handle it.

Hope that kinda makes sense to you, I know the whole callback and promises thing is a little strange if you come from something like php and it needs some getting used to :)

You realize you can just `.each` with bluebird promises right? Also you're not waiting for it to end. — Benjamin Gruenbaum, Feb 13 '15 at 16:41
I don't know the Q lib to well, it still doesn't make sense to return a promise and call a callback. I'm sure there is an elegant solution using promises but than you should get rid of the callback altogether. And yup, you're right. That didn't make sense. Fixed it. Didn't try it though..might need to change something else as well — Ben A., Feb 13 '15 at 16:57
I'm not trying to put you down - but to give you a general idea of how much simpler this can be you can check my solution - it does not suppress errors (like the `.catch` in your code) nor does it ignore them - what happens for instance if one of the earlier levels of the promises fail? — Benjamin Gruenbaum, Feb 13 '15 at 17:00

score 0 · Accepted Answer · answered Feb 20 '15 at 14:13

thanks for explaining the differences. I think working with promises is the way forward, because it makes the code look nicer and avoids this "callback hell".

For example:

PhoenixSyncTransactions.getlasttransactions(lastTimeSynced,null)
.then(function(res){

    return PersistTransaction.prepareTransactions(res).then(function(preparedTrx){
      return preparedTrx;
    })
}).then(function(preparedTrx){
    return PersistTransaction.persistToDB(preparedTrx).then(function(Processes){
      return Processes;
    })
})
.then(function(Processes){
    return PersistTransaction.checkIfMultiProcess(Processes).then(function(result){
      return result;
    })

})
.then(function(result){
  console.log('All jobs done');
})

The whole code is easier to read.

Sequelize wait until loop finished with callback

4 Answers4