NodeJS - Promise inside loop

Question

My problem today has something to do with promises inside loops. Right down here I'll put my code and as you can see it does a few things. First of all it loop inside all my events and splits the date it finds inside of it into 3 fields to insert it into a SQL db.

In total I fill 2 tables, one with the dates and one with the actual events. The problem is that when this code gets executed, it loops all the way to the end and does the date assignments all at once and then it starts releasing the promises. That makes my function insert x times the same date and event (where x is the lenght of my events array).

Now, why is this happening? I studied and implemented promises exactly for this problem but it seems to still be around. Can someone explain to me what am I doing wrong? Also, do you think this code is a bit too "pyramid of doom" like? Thanks a lot in advance!

events.forEach(function (event) {
        // Simple date formatting
        dayFormatted = event.day.split("_");
        year = dayFormatted[0];
        month = dayFormatted[1];
        day = dayFormatted[2];

        // Check if the row already exists ...
        SQLCheckTableRow(`day`, year.toString(), month.toString(), day.toString()).then(function (skip) {
            // ... if not it skips all this part
            if (!skip) {
                // Create, if it doesn't exist already, a date table that is going to be connected to all the events and fill it
                SQLFillTable(`date, `null, "${year}" , "${month}" , "${day}", null`).then(function () {
                    let values = []
                    event.ons.forEach(function (on) {
                        on.states.forEach(function (event) {
                            values = [];
                            values.push(Object.keys(event.data).map(function (key) {
                                return event.data[key];
                            }));

                            SQLFillTable(`event`, values).then();
                        })
                    })
                })
            }
        })
    })

Just calling `.then()` doesn't make anything wait. Remember that promises are still asynchronous, you cannot do blocking. — Bergi, Mar 20 '18 at 14:25
@Bergi Then how do you block the execution of the loop and wait for the function to return? — Eugenio, Mar 20 '18 at 14:27
Only `async`/`await` ([without `forEach`](https://stackoverflow.com/q/37576685/1048572)) could block the execution of a loop. Use `Promise.all` to wait for multiple promises otherwise. — Bergi, Mar 20 '18 at 14:29
@Bergi Hum...I see, I'll be trying to dig deeper into async/await. I thought they were a substitute to promises — Eugenio, Mar 20 '18 at 14:31
No, `await` is syntactic sugar for `then` calls. It still builds upon promises. — Bergi, Mar 20 '18 at 14:35

score 5 · Accepted Answer · edited Mar 20 '18 at 16:09

When you make async call in forEach loop, it doesn't wait until the call finishes. If you would like to fire the async calls one after another, you should use 'for of' loop instead. If you want to run them in parallel, you can push the promises to an array, and then you can get the result of the promise calls using Promise.all(arrayOfPromises).then((result) => {}). Here result will be array of the async calls in the same order as the promise array.

If you want to make your code easier to read, not to have pyramid of doom, then get used to use async..await. Here I refactored your code a bit, using for..of loop. As I said above, if you want to run async calls in parallel, use Promise.all.

async function queryTable() {
  for (let event of events) {
    // Simple date formatting
    let dayFormatted = event.day.split("_");
    let year = dayFormatted[0];
    let month = dayFormatted[1];
    let day = dayFormatted[2];

    try{
      let skip = await SQLCheckTableRow(`day`, year.toString(), month.toString(), day.toString());
      if (!skip) {
        // Create, if it doesn't exist already, a date table that is going to be connected to all the events and fill it
        await SQLFillTable('date', null, year, month , day, null);

        for (let on of event.ons) {
          const values = [];
          values.push(Object.keys(on.data).map(key => on.data[key]));

          await SQLFillTable('event', values);
        }
      }
    } catch(err) {
      console.error(err);
    }
  }
}

Awesome work, deserved check and +1 just for the effort! Thanks indeed for the concise and simple answer. I am still kinda new to asyncronous in NodeJS and I'm still learning how to properly implement it. Cheers! — Eugenio, Mar 20 '18 at 15:17
Glad that I could help. If you can get used to promises and async..await, they're really very helpful to write concise code. Just be careful about where to catch the errors in promises though. There is no right or wrong answer as usual, but try to separate api (or any async) calls from business logic, and make api/async calls using promise and return promise itself (without attaching .then) to the business function, and handle either in .then().catch(), or try { await promiseCall } catch(err) {} — Ahmet Cetin, Mar 20 '18 at 15:23
`If you would like to fire the async calls one after another, you should use 'for of' loop instead``where is this in the doc ? I lost 2 days with this use case ! Thank you @AhmetCetin — Amadou Beye, Dec 21 '18 at 17:54

NodeJS - Promise inside loop

1 Answers1