25

I want to do a for each loop but have it run synchronously. Each iteration of the loop will do an http.get call and that will return json for it to insert the values into a database. The problem is that the for loop runs asynchronously and that causes all of the http.gets to all run at once and my database doesn't end up inserting all of the data.I am using async-foreach to try to do what I want it to do, but I don't have to use it if I can do it the right way.

mCardImport = require('m_cardImport.js');
var http = require('http');
app.get('/path/hi', function(req, res) {

mCardImport.getList(function(sets) {
  forEach(sets, function(item, index, arr) {
    theUrl = 'http://' + sets.set_code + '.json';
    http.get(theUrl, function(res) {

      var jsonData = '';
      res.on('data', function(chunk) {
        jsonData += chunk;
      });

      res.on('end', function() {
        var theResponse = JSON.parse(jsonData);
        mCardImport.importResponse(theResponse.list, theResponse.code, function(theSet) {
          console.log("SET: " + theSet);
        });
      });
    });
  });
});
});

and my model

exports.importResponse = function(cardList, setCode, callback) {

mysqlLib.getConnection(function(err, connection) {

forEach(cardList, function(item, index, arr) {

  var theSql = "INSERT INTO table (name, code, multid, collector_set_num) VALUES "
   + "(?, ?, ?, ?) ON DUPLICATE KEY UPDATE id=id";
  connection.query(theSql, [item.name, setCode, item.multid, item.number], function(err, results) {
    if (err) {
      console.log(err);
    };
  });
});
});
callback(setCode);
};
AlexMA
  • 9,842
  • 7
  • 42
  • 64
user3447415
  • 1,135
  • 2
  • 9
  • 9
  • You could use `async.each` though given what you've posted you should be able to do this async/in parallel, so I'd be trying to figure out why all of your data isn't inserting. No reason to slow this down for no reason. – Joe May 16 '14 at 12:39
  • I am not sure why either. But I expect on the order of 24,000 records and it does a lot less. It is something like 3,000 or 5,000 the number isn't always the same when i truncate the table and re-run it again. So I thought that I was throwing too many http.get requests and/or too many MYSQL calls in too short of a time frame causing things to be missed/dropped. – user3447415 May 16 '14 at 19:07
  • 1
    @Joe I would like to thank you for suggesting that I re-look at my code, and I found out I wasn't explicitly calling connection.release() at the end of each exports.importResponse(). I thought the connection.release() was called automatically but when I checked again and explicitly added it, it now works and all 24,000 records get added as expected. Thanks again! – user3447415 May 16 '14 at 21:55

6 Answers6

64

With recursion the code is pretty clean. Wait for the http response to come back then fire off next attempt. This will work in all versions of node.

var urls = ['http://stackoverflow.com/', 'http://security.stackexchange.com/', 'http://unix.stackexchange.com/'];

var processItems = function(x){
  if( x < urls.length ) {
    http.get(urls[x], function(res) {

      // add some code here to process the response

      processItems(x+1);
    });
  }
};

processItems(0);

A solution using promises would also work well, and is more terse. For example, if you have a version of get that returns a promise and Node v7.6+, you could write an async/await function like this example, which uses some new JS features.

const urls = ['http://stackoverflow.com/', 'http://security.stackexchange.com/', 'http://unix.stackexchange.com/'];

async function processItems(urls){
  for(const url of urls) {
    const response = await promisifiedHttpGet(url);    
    // add some code here to process the response.
  }
};

processItems(urls);

Note: both of these examples skip over error handling, but you should probably have that in a production app.

AlexMA
  • 9,842
  • 7
  • 42
  • 64
6

To loop and synchronously chain asynchronous actions, the cleanest solution is probably to use a promise library (promises are being introduced in ES6, this is the way to go).

Using Bluebird, this could be

Var p = Promise.resolve();
forEach(sets, function(item, index, arr) {
    p.then(new Promise(function(resolve, reject) {
         http.get(theUrl, function(res) {
         ....
         res.on('end', function() {
              ...
              resolve();
         }
    }));
});
p.then(function(){
   // all tasks launched in the loop are finished
});
Denys Séguret
  • 372,613
  • 87
  • 782
  • 758
  • So the loop iteration starts, passes to p.then, the loop pauses, once the p.then returns the next loop iteration starts? Did I understand that correctly? – user3447415 May 16 '14 at 15:39
  • not exactly : the loop itself doesn't pause. But the callbacks you pass using then are called only one after the other. – Denys Séguret May 16 '14 at 17:11
  • 1
    so if I using for(let i=0;i<10;i++ ) how can I preserve the i for each loop? – seasonqwe Jul 20 '16 at 04:54
2

I found out that I wasn't releasing my mysql connections after I was done with each call and this tied up the connections causing it to fail and appear to be an issue with synchronization.

After explicitly calling connection.release(); it caused my code to work 100% correctly even in an asynchronous fashion.

Thanks for those who posted to this question.

Neil
  • 24,551
  • 15
  • 60
  • 81
user3447415
  • 1,135
  • 2
  • 9
  • 9
  • 2
    Note that JavaScript doesn't offer tail recursion and that recursive functions should only be used when there are hard limits on the depth of the recursion. If you don't know what that means and you know you will have many loops, just go with promises : ) – Indolering May 17 '14 at 23:22
0
"use strict";

var Promise = require("bluebird");
var some = require('promise-sequence/lib/some');

var pinger = function(wht) {
    return new Promise(function(resolve, reject) {
        setTimeout(function () { 

            console.log('I`ll Be Waiting: ' + wht);
            resolve(wht);

        }, Math.random() * (2000 - 1500) + 1500);
    });
}

var result = [];
for (var i = 0; i <= 12; i++) {
    result.push(i);
}

some(result, pinger).then(function(result){
  console.log(result);
});
user956584
  • 5,316
  • 3
  • 40
  • 50
0

Just wrap the loop in an async function. This example illustrates what I mean:

const oneSecond = async () => 
    new Promise((res, _) => setTimeout(res, 1000));

This function completes after just 1 second:

const syncFun = () => {
    for (let i = 0; i < 5; i++) {
        oneSecond().then(() => console.log(`${i}`));
    }
}

syncFun(); // Completes after 1 second ❌

This one works as expected, finishing after 5 seconds:

const asyncFun = async () => {
    for (let i = 0; i < 5; i++) {
        await oneSecond();
        console.log(`${i}`);
    }
}

asyncFun(); // Completes after 5 seconds ✅
FloatingRock
  • 6,741
  • 6
  • 42
  • 75
-4
var urls = ['http://stackoverflow.com/', 'http://security.stackexchange.com/', 'http://unix.stackexchange.com/'];

for (i = 0; i < urls.length; i++){
   http.get(urls[i], function(res) {
       // add some code here to process the response
   });
}
RAHUL.S
  • 11
  • 2
  • You should add some explanation to your code, such that other can learn from it – Nico Haase Feb 12 '19 at 11:04
  • 1
    this will not work as expected since the for loop will complete before the asynchronous http.get calls do. Also this way does not guarantee order of operations. If stackoverflow is slow one day, it may return its result after all the other urls. – PRS Apr 23 '19 at 03:51