Waiting on a long-running code block in a loop

Question

So I'm trying to wrap my head around why my loop doesn't appear to be executing in the right order. This is the call made by the worker thread when it's done sorting out the values.

worker.on('message',function(data) {
                console.log(data)
                for (var i of data.entries) { 
                    console.log(i.Email) 
                    db.rota.checkUserHasShifts(i.Email,function(flag) {
                        if (flag) {
                            console.log('e', i.Email)
                             db.rota.getShiftsForUser(i.Email,function(err, shiftData) {
                                if (!shiftData) return
                                shiftData.Shifts = i.Shifts
                                shiftData.save(function(err) {
                                    if (err) throw err;
                                })
                            })
                        } else {
                            console.log('n', i.Email)
                            var newShift = new db.rota({Email:i.Email,Shifts:i.Shifts})
                            newShift.save(function (err){
                                if (err) throw err
                            });
                        }
                    })  
                };
                console.log("Spreadsheet processed")
            })

For a set of 20 entries, this will:

Print the 'data' object out, which has 20 {Email:"someemail",Shifts:{date:time}} objects in an array
Print each email out, in the right order
Then execute 20 lots of the db code on the last email in the list, giving me 20 lines of n lastemail@example.com in console and 20 identical entries in the database.

The intended behaviour is obviously to wait for each entry before moving on to the next, or at least to queue an entry for each email in the set. How do I get the loop to wait and properly execute the db.rota.checkUserHasShifts method and callback on all the entries, not just the last one?

https://stackoverflow.com/questions/750486/javascript-closure-inside-loops-simple-practical-example — epascarello, Jan 22 '20 at 23:31
so, in essence, are you asking how to wait for `db.rota.checkUserHasShifts(i.Email,function(flag) {` in each iteration — Jaromanda X, Jan 22 '20 at 23:34

Patrick Klug · Accepted Answer · 2020-09-07T21:52:26.173

You have to scope your i variable before calling checkUserHasShifts otherwise it will be called 20 times with the last i value in your loop.

for (var i in items){
 (function(i){ db.rota.checkUserHasShifts(i)..})(i);
}

to understand why, I found section 6 "Scope Revisited" of this excellent article very useful (although it is targeted at folks coming from a C# background):

https://mauricebutler.wordpress.com/tag/c-javascript/

Relevant quote:

JavaScript does not have "Block Scope" thus the for loop has not introduced new scope. This means that each time the element variable is accessed, the same memory location is updated

This SO question also demonstrates the effect very well: JavaScript closure inside loops – simple practical example

This worked perfectly and was a quick fix, spot on! – QuartzShard Jan 23 '20 at 20:40 — QuartzShard, Jan 23 '20 at 20:40

score 0 · Answer 2 · answered Jan 23 '20 at 00:31

The easiest way to approach this is with Promises and async/await - makes the code very clean compared to using callbacks that check if there's more to process

So, firstly, if you "promisify" the checkUserHasShifts and getShiftsForUser functions, the main loop is surprisingly simple!

Also, creating functions that handle .save and return a Promise also makes it simpler

worker.on('message', async (data) => {
    // note the "async" in the above
    // the Promisified functions
    const makeNewShift = (Email, Shifts) => new Promise((resolve, reject) => {
        const newShift = new db.rota({ Email, Shifts });
        newShift.save((err) => {
            if (err) {
                reject(err);
            } else {
                resolve();
            }
        });
    });
    const updateShifts = shiftData => new Promise((resolve, reject) => shiftData.save((err) => {
        if (err) {
            reject(err);
        } else {
            resolve();
        }
    }));

    const getShiftsForUser = email => new Promise((resolve, reject) => db.rota.getShiftsForUser(email, (err, shiftData) => {
        if (err) {
            reject(err);
        } else {
            resolve(shiftData);
        }
    }));

    const checkUserHasShifts = email => new Promise(resolve => db.rota.checkUserHasShifts(email, resolve));

    // Now on to the logic

    console.log(data);

    for (let i of data.entries) {
        console.log(i.Email);
        const flag = await checkUserHasShifts(i.Email);
        if (flag) {
            console.log('e', i.Email);
            const shiftData = await getShiftsForUser(i.Email);
            if (shiftData) {
                shiftData.Shifts = i.Shifts;
                await updateShifts(shiftData);
            }
        } else {
            console.log('n', i.Email);
            await makeNewShift(i.Email, i.Shifts);
        }
    };
    console.log("Spreadsheet processed");
})

Waiting on a long-running code block in a loop

2 Answers2