Handling multiple requests in a loop causes sync issues

Question

I call getLogs() through a post request and get a list of LogFileID(filename) from a DB and then I pass this LogFileID to do an additional request by calling _getLogFileUrls which gives me a signed url for that ID in response. I push all of them one by one into a global array and return it the end response.

I know it's incorrect to use setTimeout but the problem is not using, it gives me a different result into the array every time. What could I do to resolve this issue? How do I correct this code so that the loop iterates to the next only when the signed url is stored into the global array.

function _getLogFileUrls(logFileId, callback){

var request = require('request'),
    config = require('../../config.js');    

var fileParams = {
    fileName: 'xyzdirectory/' + logFileId       
};
request.post({                    
          url: config.filesServiceUrl + 'get-logfile-urls',
          json: fileParams                            
        },function(error, response, body) {

        if (!error && response.statusCode === 200) {                                                                                
            callback(body);         
        } else {                    
            res.status(400).send('Error requesting file service for logs:');                                                
        }
    }).on('error', function(err) {
        console.log('File service error for Logs: ' + err);         
    });     
}

function getLogs(req, res){

        if(!req.body.id){
            return res.status(400).send('Please check the params!');
        }

        var date;

        if(req.body.date){
            date = req.body.date;
        } else {
            date = new Date().toISOString().slice(0,10);
        }       

        var sqlQuery = "SELECT `LogFileID` FROM `logs_data` WHERE `EmpID` = '" + req.body.id + "' AND DATE(`Timestamp`) = '" + date + "'",
        resArray= [];

        hitThisQueryForMe(sqlQuery, res, function(rows){

            if(!rows.length) res.json(rows);

            _.each(rows, function(item){            
                console.log('item: ' + item.LogFileID);
                _getLogFileUrls(item.LogFileID, function(response){
                    resArray.push(response);            
                });         
            });

            setTimeout(function(){          
                res.send(resArray);
                resArray = [];
            }, 4000);

        });         

    }

Similar to [Callback after all asynchronous forEach callbacks are completed](https://stackoverflow.com/questions/18983138/callback-after-all-asynchronous-foreach-callbacks-are-completed), the answer covers the different ways to run a "final handler" after multiple requests — Patrick Barr, Jul 18 '17 at 18:32

score 1 · Answer 1 · answered Jul 18 '17 at 18:37

SQL injection alert

First of all, your code has a serious SQL injection vulnerability. Never use string concatenation to create SQL using user-provided data or otherwise anyone will be able to read, modify and delete anything in your database. This is very serious security issue. For more details see those answers:

The answer

Now to answer your question. To handle what you try to do here you should either stick to callbacks and use a good module to handle concurrency like Async:

https://caolan.github.io/async/

Or you can use promises with a good module to help with concurrency like Q or Bluebird:

Additionally when working with promises you can use generator-based coroutines with tools like co or Bluebird.coroutine:

Or you can use ES8 async/await:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function

Those are the main ways to handle cases like yours. Reinventing the wheel of concurrency handling can lead (as you can see here) to error-prone, hard to maintain code.

I recommend using the right tool for the job.

Upvoted because the most important issues (i.e. security vulnerabilities in the code) were addressed at the top. Good answer! — Patrick Roberts, Jul 18 '17 at 19:53

kushang · Answer 2 · 2017-07-21T17:28:00.687

Use async/await

Install asyncawait library and its dependency bluebird:

npm install asyncawait --save
npm install bluebird --save

Your edited code should look like:

const async = require('asyncawait/async');
const await = require('asyncawait/await');
const Promise = require('bluebird');
const request = require('request');
const config = require('../../config.js');

function _getLogFileUrls(logFileId) {
    return new Promise((resolve, reject) => {
        var fileParams = {
            fileName: 'xyzdirectory/' + logFileId
        };

        request.post({
            url: config.filesServiceUrl + 'get-logfile-urls',
            json: fileParams
        }, function (error, response, body) {
            if (!error && response.statusCode === 200) {
                resolve(body);
            } else {
                reject('Error requesting file service for logs:');
            }
        }).on('error', function (err) {
            console.log('File service error for Logs: ' + err);
        });
    });
}

function getLogs(req, res) {

    if (!req.body.id) {
        return res.status(400).send('Please check the params!');
    }

    var date;

    if (req.body.date) {
        date = req.body.date;
    } else {
        date = new Date().toISOString().slice(0, 10);
    }

    var sqlQuery = "SELECT `LogFileID` FROM `logs_data` WHERE `EmpID` = '" + req.body.id + "' AND DATE(`Timestamp`) = '" + date + "'",
        resArray = [];

    hitThisQueryForMe(sqlQuery, res, function (rows) {

        if (!rows.length) res.json(rows);

        _.each(rows, (async function (item) {
            console.log('item: ' + item.LogFileID);
            var logFileUrlResponse = await (_getLogFileUrls(item.LogFileID));
            resArray.push(logFileUrlResponse);
        }));

        res.send(resArray);
        resArray = [];
    });
}

In your example you're not using the `async` and `await` functions from the `asyncawait` module even though it may seem that you do. Those functions need parentheses for invocation - like `async(function () { ...})` and when you use it like `async function () { ... }` then you're using a built-in `async` keyword, not the function that you got from the `asyncawait` module. The same is with `await`. Besides if you're using async/await then your code will be **much simpler** if you use `request-promise` (or `request-promise-native`) instead of `request` so you can `await` on it directly. — rsp, Jul 19 '17 at 09:36

Handling multiple requests in a loop causes sync issues

2 Answers2

SQL injection alert

The answer