Sending response after forEach

Question

(Please note this is not a duplicate of two similarly titled questions, those two questions use Mongoose and the answers apply to Mongoose queries only)

I have a list of directories, each of these directories contains a file. I want to return a JSON list with the contents of each of these files. I can load the files no problem, but because I'm looping over the array with forEach, my empty response is sent before I've actually loaded the contents of the files:

function getInputDirectories() {
    return fs.readdirSync(src_path).filter(function(file) {
        return fs.statSync(path.join(src_path, file)).isDirectory();
    });
}

router.get('/list', function(req, res, next) {
    var modules = [];
    var input_dirs = getInputDirectories();

    input_dirs.forEach(function(dir) {
        path = __dirname+'/../../modules/input/'+dir+'/module.json'
        fs.readFile(path, 'utf8', function(err, data) {
            modules.push(data);
        });
    });

    res.status(200).json(modules);
});

How can I make sure that I only send down the modules array once it's fully loaded, ie: once the forEach is done.

Have you tried using [bluebird](http://bluebirdjs.com/docs/api-reference.html)? You can achieve that with [.each()](http://bluebirdjs.com/docs/api/promise.each.html) — Juan Stiza, Jan 11 '16 at 11:26

nbro · Accepted Answer · 2016-01-11T12:31:42.073

2

Since fs.readFile is asynchronous, the behaviour that you are having is most likely the expected one.

What you need to do is return your modules when all modules have been read. You could do this inside fs.readFile.

As far as I have understood, you can obtain the total number of directories through input_dirs.length (since I guess getInputDirectories() is returning an array). Now you need some kind of a counter that helps you understand if you have read the last directory or not, and if yes, then you return your modules. Something like this should work:

router.get('/list', function(req, res, next) {
    var modules = [];
    var input_dirs = getInputDirectories();
    var c = 0;

    input_dirs.forEach(function(dir) {

        path = __dirname+'/../../modules/input/' + dir + '/module.json'

        fs.readFile(path, 'utf8', function(err, data) {
            c++;

            modules.push(data);

            if(c == input_dirs.length) {
                return res.status(200).json(modules);
            }
        });
    });

});

edited Jan 11 '16 at 12:31

answered Jan 11 '16 at 11:28

nbro

15,395
32
113
196

Thanks for the answer. I believe you meant `c == input_dirs.length`. Also it kind of works in the sense that it sends the response when expected. But it's throwing a `Can't set headers after they are sent` error. – Juicy Jan 11 '16 at 11:38
Thanks for reporting it. Check [this post](http://stackoverflow.com/questions/7042340/node-js-error-cant-set-headers-after-they-are-sent), it might help. But what exactly are you doing in the function `getInputDirectories()`? It might be that you have already sent the headers there... – nbro Jan 11 '16 at 11:41
So I've figured the problem. If you do a `console.log(c)` on each iteration in `forEach` you can see that for some reason `c` is always `==inputs_dirs.length`. I don't understand why though. I have two directories, so the headers are getting sent once for the first directory. I've updated question with `getInputDirectories()` if you want to take a look at it. – Juicy Jan 11 '16 at 11:51
I guess it's because `readFile` is async as well. `forEach` loops through before `readFile` is done. I'm gonna try the synchronous readFileSync and get back to you. – Juicy Jan 11 '16 at 11:55
@Juicy How did you solve your problem then? Ok, I guess that the problem is not in `getInputDirectories()`, but what you are describing, i.e. `c == inputs_dirs.length` is always true. This might be again because `readFile` is asynchronous, as you are saying. There might be a solution (without using the synchronous version actually), I pretty sure... – nbro Jan 11 '16 at 12:13
Yes so for now I've managed to get it working with `readFileSync`. I'm sure there's another solution, possible with promises? – Juicy Jan 11 '16 at 12:18
Promises +1. You can control how many `readFile`s you want to do concurrently. – Juan Stiza Jan 11 '16 at 12:25
@Juicy Try to increment `c++` inside the callback of `readFile` but before the `if` statement...It should work, because we are incrementing `c` only when `readFile` finishes reading...I will edit my answer. – nbro Jan 11 '16 at 12:30
is this solution good ? or just is a trick to solve the problem ? I don't know if it's a good solution or not ? – Afsanefda Aug 10 '18 at 17:09

score 1 · Answer 2 · answered Jan 11 '16 at 12:42

I suggest you use Promises, example:

var Promise = require('bluebird');

router.get('/list', function(req, res, next) {
    var modules = [];
    var input_dirs = getInputDirectories();

    // 'each' will try to fulfill all promises, if one fails, it returns a
    // failed promise.
    return Promise.each(input_dirs, function(dir){
        path = __dirname+'/../../modules/input/'+dir+'/module.json';
        return new Promise(function(resolve, reject){
            fs.readFile(path, 'utf8', function(err, data) {
                if (err) return reject(err);
                return resolve(data);
            });
        });
    }).then(function(modules){
        return res.status(200).json(modules);
    })
    .catch(function(err){
        if (err) {
            //handle error
        }
    });

});

This way you move one once you fulfilled your promises.

Using a new module just because you can't find (not because it doesn't exist) a solution to an asynchronous environment seems a little bit excessive, IMO. Actually, Node is all about an asynchronous environment, so I would always encourage people to learn raw techniques to deal with that. — nbro, Jan 11 '16 at 13:27
Yes, I agree with you. Still, sometimes you need to get things done (specially at work). Lately that has been my experience, but you are right, it would be best to encourage raw techniques. Regards! — Juan Stiza, Jan 11 '16 at 13:29

score 0 · Answer 3 · answered Jan 11 '16 at 11:56

0

Instead of fs.readFile use fs.readFileSync

answered Jan 11 '16 at 11:56

varun

652
4
5

This is not a good advice, IMO. In this case it might work, but in general we should always prefer asynchronous functions over the synchronous ones in order to not block the interface... – nbro Jan 11 '16 at 12:36

Sending response after forEach

3 Answers3

Linked