Nodejs for loop change to Synchronous

Question

I am new to nodejs. I have a for loop and it tries upload one file at a time from the filearray. For uploading, it calls a method which has promise pattern. So, the for loop continues to execute without waiting for the promise to be returned and so the order in which the files are uploaded are lost. Could anyone help me with this?

  function uploadFiles(model, files){
    var deferred = Q.defer();
    var result = [];

    async.eachSeries(files, function(currFiles, callback) {
        async.eachSeries(currFiles, function(item, innerCallback) {
            var fieldname = item.fieldname;
            var tmp_path = item.path;
            var _file = fs.readFileSync(tmp_path);
            var fileuploaded = parseService.uploadFiles(model, fieldname,item.originalname, { base64 : new Buffer(_file).toString('base64')});
            fileuploaded.then(function(data) {
                result.push(data).then(function (res){
                    console.log('File upload success');
                    innerCallback();
                }, function(err){
                    console.log('File upload fail');
                    innerCallback();
                });
            }, function(err) {
                if (err) {
                    return deferred.reject(err);
                }
                console.log(result);
                deferred.resolve(result);
            });
    }, function() {
            callback();
        });
        return deferred.promise;
    });
};
        parseService.uploadFiles = function(fieldname, filename, file){
           logger.verbose('On uploadFile');
           var deferred = Q.defer();
           var parseFile = new Parse.File(filename, file);
           parseFile.save().then(function() {
             return deferred.resolve();}, 
             function(error) {
               deferred.reject(err);   
              });
             return deferred.promise;}

This is how my methods look. Currently the for loop keeps running and files are getting uploaded asynchronously and so getting uploaded in wrong order.

Avoid the [deferred antipattern](http://stackoverflow.com/q/23803743/1048572)! — Bergi, Aug 30 '16 at 21:30
You cannot make it synchronous, promises are always asynchronous. Do you need to run the uploads sequentially or would parallel be fine as well? — Bergi, Aug 30 '16 at 21:31
I want to run it sequentially now. Because when I run it parallel (async), the upload order is changing — user3339128, Aug 30 '16 at 21:35
But I dont want to change the code in the function parseService.uploadFiles as it belongs to a utils class and is used in many places. So I still want to call this method from my first uploadFiles function. I am not sure how to use for loop and still achieve sequential — user3339128, Aug 30 '16 at 21:44
checkout [async mapSeries](http://caolan.github.io/async/docs.html#.mapSeries) — Michael Tallino, Aug 30 '16 at 21:47
possible duplicate of [How to sequentially run promises with Q in Javascript?](http://stackoverflow.com/q/18386753/1048572) — Bergi, Aug 30 '16 at 21:58
Where is `result` declared, and why does its `push` method return a promise? — Bergi, Aug 30 '16 at 22:00
I could help you write this a much better way, but I don't understand the data you start with. What is `files`? Is it a flat array of files? If so, then why do you call `async.eachSeries()` on each element of that array? — jfriend00, Aug 30 '16 at 23:39
And, do you really need all the file data in the `result` array as the final result? That's a lot of data in memory and you've already uploaded them. — jfriend00, Aug 30 '16 at 23:45

jfriend00 · Answer 1 · 2016-08-31T08:39:58.533

Assuming your initial files is an array of objects and you don't want to move to ES7 and transpile your code, then you can simplify your code a lot by only using promises. This uses a .reduce() pattern with promises to serialize the upload to one after another.

function uploadFiles(model, files) {
    var results = [];
    return files.reduce(function(p, item) {
        return p.then(function() {
            return fs.readFileAsync(item.path).then(function(fileData) {
                let uploadData = {base64: new Buffer(fileData).toString('base64')};
                return parseService.uploadFiles(item.fieldname, item.originalname, uploadData).then(function(data) {
                    results.push(data);
                });
            });
        });
    }, Promise.resolve()).then(function() {
        return results;
    });
}

parseService.uploadFiles = function (fieldname, filename, file) {
    logger.verbose('On uploadFile');
    var parseFile = new Parse.File(filename, file);
    return parseFile.save();
}

You would then call uploadFiles() like this:

uploadFiles(model, arrayOfFileObjects).then(function(results) {
     // results array here
}).catch(function(err) {
     // some error here
});

This assumes you already have a promisified version of the fs module. If you don't, you can get one using Bluebird like this:

const Promise = require('bluebird');
const fs = Promise.promisify(require('fs'));

Or, you can promisify just this one fs.readFile() function like this:

fs.readFileAsync = function(file, options) {
    return new Promise(function(resolve, reject) {
        fs.readFile(file, options, function(err, data) {
            if (err) return reject(err);
            resolve(data);
        });
    });
}

If you use Bluebird for promises, then you can use Promise.mapSeries() like this:

const Promise = require('bluebird');
const fs = Promise.promisify(require('fs'));

function uploadFiles(model, files) {
    return Promise.mapSeries(files, function(item) {
        return fs.readFileAsync(item.path).then(function(fileData) {
            let uploadData = {base64: new Buffer(fileData).toString('base64')};
            return parseService.uploadFiles(fieldname, item.originalname, uploadData).then(function(data) {
                return data;
            });
        });
    });
}

Notes:

This implementation assumes your original files argument is an array of objects, each object containing info about a file to upload. If it's something different than that, then please describe in your question what exactly it is.
You were calling parseService.uploadFiles() with four arguments, yet the function only accepts three arguments. I corrected that.
Note, my implementation of uploadFiles() and parseService.uploadFiles() do not manually create any new promises themselves. They just return promises that are already generated by the functions they use. This avoids the promise anti-pattern you were using where you wrapped an existing promise with another manually created promise. There are many common bugs that occur when people make mistakes doing that and it's completely unnecessary.
Returning the results like you are, accumulated all the files in memory in an array. This could consume a bunch of memory if the files are large and does not seem to be required for the functionality here.
You pass in the model, but you don't have any code that actually uses it.
If you use the Bluebird promise library for this, then you could use Promise.mapSeries() instead of the .reduce() to more simply serialize your operations.

Jason Livesay · Answer 2 · 2016-08-31T07:00:50.593

1

I use babel and async/await to simplify this a lot. Once you have your functions returning promises, you can just do something like this:

import readFile from 'fs-readfile-promise';

async function upload(files) {
  const results = [];

  for (let fname of files) {
    let file = await readFile(fname, 'utf8');
    let parsed = await parse(file);
    results.push(parsed);  
  }
  return results;
}

Also, just a side issue, but the naming is confusing since your uploadFiles parses the files instead of uploading them (it seems). Make sure the names make sense.

edited Aug 31 '16 at 07:00

answered Aug 31 '16 at 00:26

Jason Livesay

6,317
3
25
31

But you're using synchronous file reading. That's a no-no in any node.js server process. – jfriend00 Aug 31 '16 at 05:49
Thats a side issue and may not apply if he is only doing the uploading/parsing in that process, but I changed it for you anyway. – Jason Livesay Aug 31 '16 at 06:54
Also, where does the `parse()` function come from? The OP doesn't have a function named that and you don't show them how to do that or the upload either. I guess this is a simplified conceptual answer that only covers some concepts and is missing a lot of the other parts and therefore not something that the OP could actually use directly. – jfriend00 Aug 31 '16 at 07:36
It is slightly simplified but also trying to serve as an example of a clean coding style. The op could never use that other code directly anyway, as it is obviously only a portion of a longer file. As indicated by the fact that he said he is new and is mixing synch and async code, the op also has to master callbacks, promises, and sooner than later _will_ want to learn babel. So, you are free to continue with his coding style and try to provide code he can 'use', and I am free to try to help the way I can help. – Jason Livesay Aug 31 '16 at 08:25

vkstack · Answer 3 · 2016-08-30T21:59:51.927

-1

var async = require('async');

function uploadFiles(currFiles) {
    var deferred = Q.defer();
    var result = []
    async.eachSeries(currFiles, function(item, callback) {
        var fieldname = item.fieldname;
        var tmp_path = item.path;
        var _file = fs.readFileSync(tmp_path);
        var fileuploaded = parseService.uploadFiles(fieldname, item.originalname, {
            base64: new Buffer(_file).toString('base64')
        });
        fileuploaded.then(function(data) {
            result.push(data).then(function(res) {
                logger.verbose('File upload success');
                callback();
            }, function(err) {
                logger.verbose('File upload fail');
                callback();
            });
        });
    }, function(err) {
        if (err) {
            return deferred.reject(err);
        }
        console.log(result);
        deferred.resolve(result);
    });
    return deferred.promise;
}
parseService.uploadFiles = function(fieldname, filename, file) {
    logger.verbose('On uploadFile');
    var deferred = Q.defer();
    var parseFile = new Parse.File(filename, file);
    parseFile.save().then(function() {
            return deferred.resolve();
        },
        function(error) {
            deferred.reject(err);
        });
    return deferred.promise;
}

You can not use normal for loop whenever asynchronous behaviour of javascript is invoked.
Since the function uploadFiles is having asynchronous calls so it can not return any value except undefined. So you have to use either callback or promise approach

edited Aug 30 '16 at 21:59

answered Aug 30 '16 at 21:48

vkstack

1,582
11
24

You should not use async.js with promises. – Bergi Aug 30 '16 at 22:01
Thank you so much for the response. Just one last question. What if there is 2 level loop. – user3339128 Aug 30 '16 at 22:01
@Bergi Why should not async with promise – vkstack Aug 30 '16 at 22:03
@user3339128 That's on your own. Read async. – vkstack Aug 30 '16 at 22:05
@vkstack Because falling back to callbacks when you are already using promises is error-prone and results in ugly, unreadable code. – Bergi Aug 30 '16 at 22:14
@Bergi Thanks,I will keep that in mind from now, although async with promise solves the objective. – vkstack Aug 30 '16 at 22:18
Hi vkstack and Bergi Thanks for your inputs. @vkstack, what if I do 2 for loops one inside another? where do I add promise pattern in that? You can check out my original code now to the double loop. Thanks a lot! – user3339128 Aug 30 '16 at 22:24
You have to nest asyc calls. And resolve the promise in outer async call accordingly. – vkstack Aug 30 '16 at 22:26
I tried nested async. But it does not seem to loop. I have updated my original code above to show what I have done. Am I missing something? Thank you so much again! – user3339128 Aug 30 '16 at 22:34
You need to run it and check if it runs currectly or not! – vkstack Aug 30 '16 at 22:39

Nodejs for loop change to Synchronous

3 Answers3