Why is there a timing problem with my promises inside a loop and how do I resolve it?

Question

I am running nuts with this issue on my hobby node project. I have a function (processDataSet) which is processing a data array (inputArray) and returns a promise. The function uses a for loop to iterate through the input array and calls saveObjectData function on each round. This save function handles a single data entry and also returns a promise.

It seems that if the saveObjectData function fails, processDataSet function catches that returned reject but its own reject doesn't seem to get called properly inside the for loop. I believe this is a timing problem, which I don't understand. See the results of output prints below the code.

function processDataSet(inputArray, scriptConfig) {
    var contentType = scriptConfig.contentType;
    return new Promise(function(resolve, reject) {
        if(!Array.isArray(inputArray)) {
            return reject(new Error("Input data is not an array. Cannot process."));
        }
        if(!scriptConfig) {
            return reject(new Error("Invalid scriptConfig"));
        }
        if(!typeof contentType === "string" && !contentType instanceof String) {
            return reject(new Error("Invalid contentType for the data set. The parameter should be a string."));
        }

        console.log("Post processing data for the script " + scriptConfig.name + " (type: " + scriptConfig.contentType + ")");

        // Iterate through the input array and handle the objects one-by-one
        for (var i = 0; i < inputArray.length; i++) {
            saveObjectData(inputArray[i], scriptConfig)
            .then(() => {
                //continue;
            })
            .catch(err => {
                console.log("TEST PRINT " + scriptConfig.name);
                return reject(new Error("Processing data object failed.", err));
            });
        }
        console.log("Resolve " + scriptConfig.name);
        return resolve();
    });
}

Output prints in console:

Post processing data for the script Script1 (type: Season)
Resolve Script1
TEST PRINT Script1

It seems that the last log line including "Resolve ..." gets printed before the "TEST PRINT ..." in the error handler. Why is that and how can I make the execution to wait for the full resolution of all data entries before returning from processDataSet ?

I'm not fully sure if it's redundant in my case to make processDataSet to return promises, but I made it as part of my troubleshooting.

Seems like you should be gathering up the promises from calling `saveObjectData()` in an array, and then return `Promise.all(thePromiseArray)`. That promise will only resolve when all the individual promises have resolved (or rejected). Also you don't need to `return` the result of calling the `resolve()` and `reject()` callbacks. — Pointy, Sep 11 '18 at 13:24

Bergi · Accepted Answer · 2018-09-11T13:57:11.683

Your for loop doesn't save the objects one by one. It starts saving the first, then the second, and so on, then the loop ends, and you immediately resolve your promise. Only after that the promises created in the loop will settle, and some of them might try to reject the already-fulfilled promise.

Avoid the Promise constructor antipattern, and chain your promises properly instead.

If you are ok with triggering all the saves immediately so that they run concurrently, you can wait for all the promises with Promise.all:

function processDataSet(inputArray, scriptConfig) {
    if (!Array.isArray(inputArray)) {
        return Promise.reject(new Error("Input data is not an array. Cannot process."));
    }
    if (!scriptConfig) {
        return Promise.reject(new Error("Invalid scriptConfig"));
    }
    var contentType = scriptConfig.contentType;
    if (typeof contentType !== "string") {
        return Promise.reject(new Error("Invalid contentType for the data set. The parameter should be a string."));
    }

    console.log("Post processing data for the script " + scriptConfig.name + " (type: " + scriptConfig.contentType + ")");

    return Promise.all(inputArray.map(input => {
        return saveObjectData(input, scriptConfig)
        .catch(err => {
            console.log("TEST PRINT " + scriptConfig.name);
            throw new Error("Processing data object failed.", input, err);
        });
    })).then(results => {
        console.log("Resolve " + scriptConfig.name, results);
        return;
    });
}

If you insist on saving them sequentially, I recommend to use async/await.

async function processDataSet(inputArray, scriptConfig) {
    if (!Array.isArray(inputArray)) {
        throw new Error("Input data is not an array. Cannot process.");
    }
    if (!scriptConfig) {
        throw new Error("Invalid scriptConfig");
    }
    var contentType = scriptConfig.contentType;
    if (typeof contentType !== "string") {
        throw new Error("Invalid contentType for the data set. The parameter should be a string.");
    }

    console.log("Post processing data for the script " + scriptConfig.name + " (type: " + scriptConfig.contentType + ")");

    for (var input of inputArray) {
        try {
             await saveObjectData(input, scriptConfig);
        } catch (err) {
            console.log("TEST PRINT " + scriptConfig.name);
            throw new Error("Processing data object failed.", input, err);
        }
    }
    console.log("Resolve " + scriptConfig.name);
}

Thanks for this! I think sequential execution is not mandatory so I tried your first option and it worked. There was just a small typo: missing one `)` before `.then` — quinz, Sep 11 '18 at 13:45

Steven Spungin · Answer 2 · 2018-09-11T13:37:49.307

Your loop is async and returns immediately.

    for (var i = 0; i < inputArray.length; i++) {
        saveObjectData(inputArray[i], scriptConfig)
        .then(() => {
            //continue;
        })
        .catch(err => {
            console.log("TEST PRINT " + scriptConfig.name);
            return reject(new Error("Processing data object failed.", err));
        });
    }

You need a promise from each iteration, wait on all the promises, and then call

console.log("Resolve " + scriptConfig.name);
        return resolve();

Something like this:

const promises = []
// Iterate through the input array and handle the objects one-by-one
for (var i = 0; i < inputArray.length; i++) {
  promises.push(saveObjectData(inputArray[i], scriptConfig))
}

Promise.all(promises).then(results => {
  resolve();
})

You will often see a map function used in this case...

const promises = inputArray.map(it => saveObjectData(it, scriptConfig))

score 2 · Answer 3 · answered Sep 11 '18 at 13:35

saveObjectData is asynchronous, and you're not waiting for it to complete. You have to wait for it to complete in .then or .catch. That is, your console.log('Resolve ' ... needs to be done in .then (or .catch). Of course since you have an array of promises by looping, you want to wait for all of these to complete. You can wait for an array of promises to complete with Promise.all.

function processDataSet(inputArray, scriptConfig) {
    const contentType = scriptConfig.contentType;
    return new Promise(function(resolve, reject) {
        if(!Array.isArray(inputArray)) {
            return reject(new Error("Input data is not an array. Cannot process."));
        }
        if(!scriptConfig) {
            return reject(new Error("Invalid scriptConfig"));
        }
        if(!typeof contentType === "string" && !contentType instanceof String) {
            return reject(new Error("Invalid contentType for the data set. The parameter should be a string."));
        }
        resolve();
    }).then(() => {
        console.log("Post processing data for the script " + scriptConfig.name + " (type: " + scriptConfig.contentType + ")");

        return Promise.all(inputArray.map(input =>
            saveObjectData(input, scriptConfig)
            .then(() => {
                //continue;
            })
            .catch(err => {
                console.log("TEST PRINT " + scriptConfig.name);
                return reject(new Error("Processing data object failed.", err));
            })
        ));
    }).then(() => console.log(`Resolve ${scripConfig.name}`));
}

I think using async/await will make this easier to reason about.

async function processDataSet(inputArray, scriptConfig) {
    const contentType = scriptConfig.contentType;
    if (!Array.isArray(inputArray)) {
        throw new Error("Input data is not an array. Cannot process.");
    }
    if (!scriptConfig) {
        throw new Error("Invalid scriptConfig");
    }
    if (!typeof contentType === "string" && !contentType instanceof String) {
        throw new Error("Invalid contentType for the data set. The parameter should be a string.");
    }

    console.log("Post processing data for the script " + scriptConfig.name + " (type: " + scriptConfig.contentType + ")");

    await Promise.all(inputArray.map(async input => {
        try {
            saveObjectData(input, scriptConfig);
            // continue
        } catch (err) {
            console.log("TEST PRINT " + scriptConfig.name);
            throw new Error("Processing data object failed.", err);
        }
    }));
    console.log(`Resolve ${scriptConfig.name}`);
}

Why is there a timing problem with my promises inside a loop and how do I resolve it?

3 Answers3