Node.js Promises within promises not waiting for for loop to return data

Question

The return Promise.all([photoArray]) returns an empty array, seemingly not waiting for the callFB to return its promise that then pushes into the array.

I am not sure what I am doing wrong but am relatively new to Promises with for loops and Ifs.

I am not sure exactly if I am using the correct number of Promises but I seem to not be able to get the 3rd tier Promise.all to wait for the for loop to actually finish (in this scenario, the for loop has to look through many item so this is causing an issue where it is not triggering callFeedback for all the items it should before context.done() gets called.

I have tried using Q.all also for the Promise.all([photoArray]) but have been unable to get that working.

module.exports = function (context, myBlob) {
                var res = myBlob
                var promiseResolved = checkPhoto(res,context);
                var promiseResolved2 = checkVideo(res,context); 


            Promise.all([promiseResolved, promiseResolved2]).then(function(results){
            context.log(results[0], results[1]);
 //           context.done();
            });


        });  
    };
};

function checkPhoto(res, context){
    return new Promise((resolve, reject) => {
    if (res.photos.length > 0) {
        var photoArray = [];
        for (var j = 0; j < res.photos.length; j++) {
            if (res.photos[j].feedbackId !== null){
                var feedbackId = res.photos[j].feedbackId;
                var callFB = callFeedback(context, feedbackId);                    
                Promise.all([callFB]).then(function(results){
                    photoArray.push(results[0]);
                 });        
            } else {    
                photoArray.push("Photo " + j + " has no feedback");
            }

        }

        return Promise.all([photoArray]).then(function(results){
            context.log("end results: " + results);
            resolve(photoArray);
        });
    } else {
        resolve('No photos');
    }

})
}
function checkVideo(res, context){
        return new Promise((resolve, reject) => {
            same as checkPhoto
    })
    }
function callFeedback(context, feedbackId) {
        return new Promise((resolve, reject) => {

        var requestUrl = url.parse( URL );

            var requestBody = {
                "id": feedbackId
            };

            // send message to httptrigger to message bot
            var body = JSON.stringify( requestBody );

            const requestOptions = {
            standard
            };

            var request = https.request(requestOptions, function(res) {
                var data ="";
                res.on('data', function (chunk) {
                    data += chunk
    //                context.log('Data: ' + data)
                });
                res.on('end', function () {
                resolve("callFeedback: " + true);
                })
            }).on('error', function(error) {
            });
            request.write(body);
            request.end();
            })

}

Answer 1

The code suffers from promise construction antipattern. If there's already a promise (Promise.all(...)), there is never a need to create a new one.

Wrong behaviour is caused by that Promise.all(...).then(...) promise isn't chained. Errors aren't handled and photoArray.push(results[0]) causes race conditions because it is evaluated later than Promise.all([photoArray])....

In case things should be processed in parallel:

function checkPhoto(res, context){
    if (res.photos.length > 0) {
        var photoArray = [];
        for (var j = 0; j < res.photos.length; j++) {
            if (res.photos[j].feedbackId !== null){
                var feedbackId = res.photos[j].feedbackId;
                var callFB = callFeedback(context, feedbackId);
                // likely no need to wait for callFB result
                // and no need for Promise.all
                photoArray.push(callFB);
            } else {    
                photoArray.push("Photo " + j + " has no feedback");
            }
        }

        return Promise.all(photoArray); // not [photoArray]
    } else {
        return 'No photos';
    };
}

callFB promises don't depend on each other and thus can safely be resolved concurrently. This allows to process requests faster.

Promise.all serves a good purpose only if it's used to resolve promises in parallel, while the original code tried to resolve the results (results[0]).

In case things should be processed in series the function benefits from async..await:

async function checkPhoto(res, context){
    if (res.photos.length > 0) {
        var photoArray = [];
        for (var j = 0; j < res.photos.length; j++) {
            if (res.photos[j].feedbackId !== null){
                var feedbackId = res.photos[j].feedbackId;
                const callFBResult = await callFeedback(context, feedbackId);
                // no need for Promise.all
                photoArray.push(callFBResult);
            } else {    
                photoArray.push("Photo " + j + " has no feedback");
            }
        }

        return photoArray; // no need for Promise.all, the array contains results
    } else {
        return 'No photos';
    };
}

Add try..catch to taste.