38

I am new to this node.js ..I am little bit confused about this callback..In my app inside a for loop i am calling a asynchronous function call,i think my problem is that before i am getting response of async call my for loop get looped.

My code:

async.forEach(Object.keys(config), function(key, next) {
        search(config[key].query, function(err, result) { // 
        console.log("fffffffffff="+ util.inspect(result))-------- >>>Getting undefined..
            if (err) return next(err) // 
            var json = JSON.stringify({
                "result": result
            });
            results[key] = {
                "result": result
            }
            console.log("rrrrrrrr="+util.inspect(results[key]))
            next() // <---- critical piece.  This is how the forEach knows to continue to the next loop.  Must be called inside search's callback so that it doesn't loop prematurely.                   
        })
    },
    function(err) {
        console.log('iterating done');
        
         res.writeHead(200, {
        'content-type': 'application/json'
    });
    res.end(JSON.stringify(results));  
    });
    
       
}

Search function code:

var matches = [];
    var qrySubString = query.substring(0, 4);
    client.query("select * from xxxxxxxxx where level4 ILIKE '%" + query + "%'", function(err, row1, fields) {
        for (var id in row1.rows) {                
            var match, name;                
            if (query == row1.rows[id].level4) {
                match = true;
                name = row1.rows[id].level4;
            }
            else {
                match = false;
                name = query;
            }
            matches.push({
                "id": id,
                "name": row1.rows[id].level4,
                "score": 100,
                "match": match,
                "type": [{
                    "id": "/people/presidents",
                    "name": "US President"
                }]
            })
        }           
        callback(matches);
    })

I want to execute for loop after successful execution of 1 search function, I think I have to use async for loop.

starball
  • 20,030
  • 7
  • 43
  • 238
Subburaj
  • 5,114
  • 10
  • 44
  • 87

4 Answers4

79

I've reduced your code sample to the following lines to make it easier to understand the explanation of the concept.

var results = [];
var config = JSON.parse(queries);
for (var key in config) {
    var query = config[key].query;
    search(query, function(result) {
        results.push(result);
    });
}
res.writeHead( ... );
res.end(results);

The problem with the previous code is that the search function is asynchronous, so when the loop has ended, none of the callback functions have been called. Consequently, the list of results is empty.

To fix the problem, you have to put the code after the loop in the callback function.

    search(query, function(result) {
        results.push(result);
        // Put res.writeHead( ... ) and res.end(results) here
    });

However, since the callback function is called multiple times (once for every iteration), you need to somehow know that all callbacks have been called. To do that, you need to count the number of callbacks, and check whether the number is equal to the number of asynchronous function calls.

To get a list of all keys, use Object.keys. Then, to iterate through this list, I use .forEach (you can also use for (var i = 0, key = keys[i]; i < keys.length; ++i) { .. }, but that could give problems, see JavaScript closure inside loops – simple practical example).

Here's a complete example:

var results = [];
var config = JSON.parse(queries);
var onComplete = function() {
    res.writeHead( ... );
    res.end(results);
};
var keys = Object.keys(config);
var tasksToGo = keys.length;
if (tasksToGo === 0) {
   onComplete();
} else {
    // There is at least one element, so the callback will be called.
    keys.forEach(function(key) {
        var query = config[key].query;
        search(query, function(result) {
            results.push(result);
            if (--tasksToGo === 0) {
                // No tasks left, good to go
                onComplete();
            }
        });
    });
}

Note: The asynchronous code in the previous example are executed in parallel. If the functions need to be called in a specific order, then you can use recursion to get the desired effect:

var results = [];
var config = JSON.parse(queries);
var keys = Object.keys(config);
(function next(index) {
    if (index === keys.length) { // No items left
        res.writeHead( ... );
        res.end(results);
        return;
    }
    var key = keys[index];
    var query = config[key].query;
    search(query, function(result) {
        results.push(result);
        next(index + 1);
    });
})(0);

What I've shown are the concepts, you could use one of the many (third-party) NodeJS modules in your implementation, such as async.

Community
  • 1
  • 1
Rob W
  • 341,306
  • 83
  • 791
  • 678
  • 5
    @Subburaj While I appreciate the +1, you should rate answers by its value, not by efforts of the user who posted it. If an answer does not help at all, don't upvote it. If an answer is factually wrong, downvote it. If an answer solves your problem, accept it. – Rob W Jan 17 '14 at 11:56
  • I followed your 2nd option..Its work fine..Can you please clear me some more doubts: As you said after executing for loop there is no callback executed so the results is empty..Now forEach loop where the callback is called .. – Subburaj Jan 17 '14 at 12:55
  • @Subburaj In the first example, when all tasks have been processed, the number of remaining tasks is zero. Then, `onComplete` is called. – Rob W Jan 17 '14 at 15:07
  • Hey man, great explanation. 1 quick question - can the stuff in OnComplete be a callback function call? – djbhindi Jul 25 '14 at 21:23
  • on the first example you have to call `--tasksToGo` after the if statement. Otherwise `onComplete` will be called one to early. – mles Apr 06 '15 at 21:00
  • 1
    @mles Why do you think that the callback will be called one too early? Imagine that there is only one key. Then `tasksToGo = keys.length = 1`. After finishing the task, `--tasksToGo == 0` will be true, and `onComplete` is called. If there were 2 items, then the pre-increment operator would give 1, which is not 0 and therefore the callback is not called too early. – Rob W Apr 06 '15 at 22:25
  • Ahhh you're right! I was using your solution with an array that starts at index zero. I was off by one. +1 for you – mles Apr 07 '15 at 05:06
  • really appreciate for your solution. node.js async mechanism drive me crazy. +1024 – Will Wu Oct 07 '16 at 05:27
  • The last example of recursion helped me eliminate a for loop which helped me in achieving the desired behavior. – ajaysinghdav10d Mar 16 '17 at 12:29
31

You've correctly diagnosed your problem, so good job. Once you call into your search code, the for loop just keeps right on going.

I'm a big fan of https://github.com/caolan/async, and it serves me well. Basically with it you'd end up with something like:

var async = require('async')
async.eachSeries(Object.keys(config), function (key, next){ 
  search(config[key].query, function(err, result) { // <----- I added an err here
    if (err) return next(err)  // <---- don't keep going if there was an error

    var json = JSON.stringify({
      "result": result
    });
    results[key] = {
      "result": result
    }
    next()    /* <---- critical piece.  This is how the forEach knows to continue to
                       the next loop.  Must be called inside search's callback so that
                       it doesn't loop prematurely.*/
  })
}, function(err) {
  console.log('iterating done');
}); 

I hope that helps!

juanpaco
  • 6,303
  • 2
  • 29
  • 22
  • 1
    i have some problem when i tried to implement your code..Please see my edited question.. – Subburaj Jan 17 '14 at 12:05
  • The problem you were having is that I changed the function signature on your callback that you pass to `search`. The callback I wrote expects an error object as its first parameter and not the results object. If you were to take this approach to doing it, you'd need to change your ` callback(matches);` call to something like ` callback(err,matches);`. Then the caller could look at the error object. – juanpaco Jan 17 '14 at 16:28
  • I don't find a forEach in async :S only each, which has no callback aparently – coiso Jun 19 '14 at 20:50
  • 1
    Try: async.eachSeries(arr, iterator, callback) – smg Oct 08 '14 at 08:10
  • 1
    your sample misses a ) in the end – atom2ueki Aug 28 '16 at 17:18
  • Thanks! Fixed it. – juanpaco Aug 29 '16 at 21:19
  • @juanpaco Is there a way to call next() properly when having nested async.eachOfSeries() ? Seems that next() is calling the outer loop. – Jose Bernhardt Oct 11 '16 at 04:56
  • If you change the name given to `next` in the inner one, that should work. There is no requirement that the function be named `next`. That said, nesting like that could get hairy. Do you have a gist of what you're trying to do? There may be a more readable way to structure it. Here's a gist illustrating the 2 things I mentioned above: https://gist.github.com/juanpaco/046c1b580bc5de1742927c2d29d655f0 – juanpaco Oct 11 '16 at 13:33
5

I like to use the recursive pattern for this scenario. For example, something like this:

// If config is an array of queries
var config = JSON.parse(queries.queryArray);   

// Array of results
var results;

processQueries(config);

function processQueries(queries) {
    var searchQuery;

    if (queries.length == 0) {
        // All queries complete
        res.writeHead(200, {'content-type': 'application/json'});
        res.end(JSON.stringify({results: results}));
        return;
    }

    searchQuery = queries.pop();

    search(searchQuery, function(result) {
        results.push(JSON.stringify({result: result}); 
        processQueries();            
    });
}

processQueries is a recursive function that will pull a query element out of an array of queries to process. Then the callback function calls processQueries again when the query is complete. The processQueries knows to end when there are no queries left.

It is easiest to do this using arrays, but it could be modified to work with object key/values I imagine.

marc_s
  • 732,580
  • 175
  • 1,330
  • 1,459
Svbaker
  • 706
  • 3
  • 4
5

Node.js introduced async await in 7.6 so this makes Javascript more beautiful.

var results = [];
var config = JSON.parse(queries);
for (var key in config) {
  var query = config[key].query;
  results.push(await search(query));
}
res.writeHead( ... );
res.end(results);

For this to work search fucntion has to return a promise or it has to be async function

If it is not returning a Promise you can help it to return a Promise

function asyncSearch(query) {
  return new Promise((resolve, reject) => {
   search(query,(result)=>{
    resolve(result);
   })
  })
}

Then replace this line await search(query); by await asyncSearch(query);

Praveena
  • 6,340
  • 2
  • 40
  • 53
  • @PrashantTapase You should always wrap await with `try catch` and promise `reject` will go to `catch` – Praveena Sep 06 '19 at 04:32