Promise.all() fires before promise arrays are fully populated

Question

I am trying to achieve multiple async calls and continue only when all are finished.

The process is like this and I need it done in this order: Get a list of file paths, Copy them to new destination, Convert bitmaps, Create thumbnails, Add information into an Array to bulk insert into DB

For this I am using arrays of promises and the Promise.all() method. This method fires too early, because it gets executed when the promise arrays aren't (fully) populated yet. If I delay for some time after the for loop it works, but the time needed depends of the number of files.

Here is the main call:

queueFiles(files).then(function () {
    // Add to DB
    console.log(current_import.sql.length); // Number of all datasets (0 at runtime)
    console.log(current_import.sql);        // An array of datasets   ([] at runtime)
});

This is the function which does not work as expected:

let import_promises = {
    copy_promises : [],
    convert_promises : [],
    thumbnail_promises : [],
    sql_promises : []
};

function queueFiles(files) {
    return new Promise(function (resolve, reject) {
        try {
            for (file of files) {
                let original_filename = file.split("\\").pop();
                let original_extension = original_filename.split(".").pop();

                let hashed_name = crypto.createHmac('sha256', secret).update(original_filename).digest('hex') + "." + original_extension;
                let new_path = data_folder + "/" + hashed_name;

                import_promises.copy_promises.push(fs.copy(file, new_path).then(function () {
                    if (original_extension == "bmp") {
                        import_promises.convert_promises.push(convertFile(new_path).then(function (result) {
                            import_promises.thumbnail_promises.push(createThumbnail(result.path, thumb_folder + "/" + result.path.split("/").pop(), 500).then(function () {
                                import_promises.sql_promises.push(addSql(result.data));
                            }));
                        }));
                    } else {
                        import_promises.thumbnail_promises.push(createThumbnail(new_path, thumb_folder + "/" + hashed_name, 500).then(function () {
                            import_promises.sql_promises.push(addSql(new_path));
                        }));
                    }
                }));
            }

            Promise.all([import_promises.copy_promises, import_promises.convert_promises, import_promises.thumbnail_promises, import_promises.sql_promises])
                .then(() => {
                    // All files copied, converted, thumbnails created and prepared to insert into DB
                    resolve();
                });
        }
        catch (err) {
            reject(err);
        }
    });
}

And these are the helper functions:

function createThumbnail(input, output, width) {
    return new Promise(function (resolve, reject) {
        try {
            gm(input)
                .scale(width)
                .write(output, function (err) {
                    if (err) {
                        throw(err);
                    }
                    resolve();
                });
        }
        catch (err) {
            reject(err);
        }
    });
}

function convertFile(newPath) {
    return new Promise(function (resolve, reject) {
        try {
            let convertedPath = newPath.replace(/\.[^/.]+$/, "") + ".png";

            execFile("convert", [newPath, convertedPath]).then(function () {
                fs.unlinkSync(newPath);
                resolve({path: convertedPath});
            });
        }
        catch (err) {
            reject(err);
        }
    });
}

function addSql(data) {
    return new Promise(function (resolve, reject) {
        try {
            current_import.sql.push(data);

            current_import.done++;
            updateProgressBar();

            resolve();
        }
        catch (err) {
            reject(err);
        }
    });
}

Edited Code (Working):

function createThumbnail(input, output, width) {
    return new Promise(function (resolve, reject) {
        gm(input)
            .scale(width)
            .write(output, function (err) {
                if (err) {
                    return reject(err);
                }
                resolve();
            });
    });
}

function convertFile(newPath) {
    let convertedPath = newPath.replace(/\.[^/.]+$/, "") + ".png";

    return execFile("convert", [newPath, convertedPath]).then(function () {
        fs.unlinkSync(newPath);
        return convertedPath;
    });
}

function addSql(data) {
    current_import.sql.push(data);

    current_import.done++;
    updateProgressBar();
}

function queueFiles(files) {
    return Promise.all(files.map(function (file) {
        let original_filename = file.split("\\").pop();
        let original_extension = original_filename.split(".").pop();

        let hashed_name = crypto.createHmac('sha256', secret).update(original_filename).digest('hex') + "." + original_extension;
        let new_path = data_folder + "/" + hashed_name;

        return fs.copy(file, new_path).then(function () {
            if (original_extension == "bmp") {
                return convertFile(new_path)
                    .then(function (path) {
                        return {path: path, hash: path.split("/").pop()};
                    });
            } else {
                return {path: new_path, hash: hashed_name}
            }
        }).then(function (result) {
            let outPath = thumb_folder + "/" + result.hash;
            return createThumbnail(result.path, outPath, 500)
                .then(function () {
                    return outPath;
                });
        }).then(function (thumb_path) {
            return addSql(thumb_path);
        });
    }));
}

Using `new Promise` like this is often an [antipattern](http://stackoverflow.com/q/23803743) — Frxstrem, Jan 29 '17 at 21:04

Jaromanda X · Accepted Answer · 2017-01-29T22:27:47.237

Firstly, convertFiles and queueFiles don't need Promise constructor

but the problem is ... import_promises.copy_promises is an array of promises, import_promises.sql_promises is an array of promises, and import_promises.convert_promises is an array of promises ... so, you are executing Promise.all on an array of array of promises ... Promise.all expects an array of promises

chnage

Promise.all([import_promises.copy_promises, import_promises.convert_promises, import_promises.thumbnail_promises, import_promises.sql_promises]).then ...

to

Promise.all([].concat(import_promises.copy_promises, import_promises.convert_promises, import_promises.thumbnail_promises, import_promises.sql_promises).then ...

as for the anipatterns, converFiles can be simply

function convertFile(newPath) {
    let convertedPath = newPath.replace(/\.[^/.]+$/, "") + ".png";

    return execFile("convert", [newPath, convertedPath]).then(function () {
        fs.unlinkSync(newPath);
        resolve({path: convertedPath});
    });
}

addsql isn't even asynchronous, so why it returns a promise is a mystery -

and queueFiles (without looking too deep into some of the other issues there) can be

function queueFiles(files) {
    for (file of files) {
        let original_filename = file.split("\\").pop();
        let original_extension = original_filename.split(".").pop();

        let hashed_name = crypto.createHmac('sha256', secret).update(original_filename).digest('hex') + "." + original_extension;
        let new_path = data_folder + "/" + hashed_name;

        import_promises.copy_promises.push(fs.copy(file, new_path).then(function () {
            if (original_extension == "bmp") {
                import_promises.convert_promises.push(convertFile(new_path).then(function (result) {
                    import_promises.thumbnail_promises.push(createThumbnail(result.path, thumb_folder + "/" + result.path.split("/").pop(), 500).then(function () {
                        import_promises.sql_promises.push(addSql(result.data));
                    }));
                }));
            } else {
                import_promises.thumbnail_promises.push(createThumbnail(new_path, thumb_folder + "/" + hashed_name, 500).then(function () {
                    import_promises.sql_promises.push(addSql(new_path));
                }));
            }
        }));
    }

    return Promise.all([import_promises.copy_promises, import_promises.convert_promises, import_promises.thumbnail_promises, import_promises.sql_promises]);
}

after trying to make sense of the flow, I've come up with this, the only part I don't understand is addSql function, why it was a promise,

function convertFile(newPath) {
    return execFile("convert", [newPath, convertedPath]).then(function () {
        fs.unlinkSync(newPath);
        return convertedPath;
    });
}

function createThumbnail(input, output, width) {
    return new Promise(function(resolve, reject) {
        gm(input)
        .scale(width)
        .write(output, function (err) {
            if (err) {
                return reject(err);
            }
            resolve();
        });
    });
}

function addSql(data) {
    current_import.sql.push(data);

    current_import.done++;
    updateProgressBar();
}

function queueFiles(files) {
    return Promise.all(files.map(function(file) {
        return fs.copy(file, new_path).then(function () {
            if (original_extension == "bmp") {
                return convertFile(new_path)
                .then(function(path) {
                    return {path: path, hash: path.split("/").pop()};
                });
            } else {
                return {path: new_path, hash: hashed_name}
            }
        }).then(function (result) {
            return createThumbnail(result.path, thumb_folder + "/" + result.hash, 500)
            .then(function() {
                return result.path;
            });
        }).then(function(result) {
            return addSql(result);
        });

    }));
}

Thank you. With this change I don't get the immediate result of an empty array, but still only part of the data. I also don't understand how to change convertFiles and queueFiles methods. Should I return the function call instead of creating a Promise? — pedroheeenriqu, Jan 29 '17 at 21:39
I just edited the answer ... the maze of arrays with pushing in a function whose return value is pushed .... it's doing my head in trying to figure out what you need to wait for - it seems you aren't using one of the nicest features of Promises, that's chaining — Jaromanda X, Jan 29 '17 at 21:41
I am new to promises and how to structure code with taking advantage of them. I simply need the order as described in my question. Files need to be copied first for them to be converted. They need to be converted first for thumbnails to be created and when this is done I can insert the info containing the thumbnail path into an array. — pedroheeenriqu, Jan 29 '17 at 21:45
I tried it, but I still get the immediate return of the promise resulting in an empty array. For the functionality it does what is expected. For the addSql method, I think I just wasn't sure what I was doing. I edited my question to show the current code (Some minor changes were neccessary). — pedroheeenriqu, Jan 29 '17 at 22:20
Now it works perfect! Thank you! Can I buy you a beer or something? :) — pedroheeenriqu, Jan 29 '17 at 22:28
re the "antipattern", putting synchronous code outside a promise in a promise API can have unwanted side affects. Errors can be thrown when the function is called rather than the promise being rejected which can play havoc when trying to `.catch` errors. — Matt, Jan 29 '17 at 23:21
`outside a promise in a promise` - you've lost me @Matt - which part of the code are you referring to - — Jaromanda X, Jan 29 '17 at 23:41
hah, yeah, sorry... I meant doing that "in a promise API" i.e. in an API that is expected to return promises. In the `convertFile` function, say for arguments sake `newPath` was a Number. That will throw an error when the function is called rather than eventually rejecting a promise. — Matt, Jan 30 '17 at 00:33
Most times you won't notice as it only affects errors. Sometimes you'll `.catch` errors somewhere above the function and the error will propagate up fine. But, if you try and `.catch` the error on that promise it won't be picked up. As soon as I start writing code before a `return promiseFunc()` it gets wrapped in a promise. I guess that's my anti-anti pattern? — Matt, Jan 30 '17 at 00:34
`That will throw an error when the function is called rather than eventually rejecting a promise` I was under the impression, that if a Promise (and it is in a Promise, as the function is called in a `.then`) throws an error, then the Promise returned by that `.then` would be "rejected" - every Promise/A+ library I've looked at will do so — Jaromanda X, Jan 30 '17 at 02:09

Promise.all() fires before promise arrays are fully populated

1 Answers1