-1

I am very confused to why the the return resolve({ status: true, data: newIDs }); is called before the for loop has finished.

Code

  createPallet: (data) => {
    return new Promise(async (resolve, reject) => {
      const newIDs = [];
      try {
        for (const record of data) {
          mysqlConnection.beginTransaction();
          mysqlConnection.query(
            "INSERT INTO ...",
            [record.BatchId, record.PalletNumber, record.PrimaryWeightId],
            async (error, results) => {
              if (error) {
                return reject(error);
              }

              // Create pallet sku record
              await PalletSkusService.createPalletSku(
                ...
              );

              console.log(results.insertId);
              newIDs.push(results.insertId);
            }
          );
        }
        return resolve({ status: true, data: newIDs });
      } catch (error) {
        mysqlConnection.rollback();
        return reject(error);
      }
    });
  },

Expectation

I expect the for loop to get all the new inserted id's and store them into newIDs array. Once that is done, I can return a resolve with the data.

However, this part I don't quite understand is - why does the resolve is ran before the for loop has finished?

What is the correct approach in this situation?

Eduards
  • 1,734
  • 2
  • 12
  • 37
  • 3
    Your for loop is probably finishing just fine, the _callback_ you passed to the .query function is not finished though. I'd recommend you use a mysql library that uses promises instead of callbacks. I wrote an article about the proper way of using mysql in node after seeing a lot of people struggling here: https://evertpot.com/executing-a-mysql-query-in-nodejs/ . It covers most of what you want to do and also addresses some bugs in your code you haven't found yet. – Evert Jan 31 '23 at 19:26
  • 1
    because the for is syncronous, you need to await Promise.all(newIDs) before the resolve – Bitzu Jan 31 '23 at 19:36
  • You really should not begin the transaction inside that loop… – Bergi Jan 31 '23 at 23:15
  • @Bergi Hi, thanks for your comment. Why do you say that? And I assume it needs to be outside the loop? – Eduards Feb 01 '23 at 18:20
  • @Eduards Well it just doesn't make sense, what were you trying to achieve by that? Indeed, you should begin the transaction before the loop. And after the loop you likely will want to commit it… Currently you only (sometimes) rollback it, and sometimes you even rollback a transaction that you never began… – Bergi Feb 01 '23 at 19:43
  • Btw, [never pass an `async function` as the executor to `new Promise`](https://stackoverflow.com/q/43036229/1048572)! And you shouldn't really pass a callback (`async`, or at all) to `mysqlConnection.query(…)`, rather you should [promisify](https://stackoverflow.com/q/22519784/1048572) and `await` it. – Bergi Feb 01 '23 at 19:54

1 Answers1

2

mysqlConnection.query is returning immediately and not waiting for the data to return. I don't know what library you're using for this but you want to find the promise based method to accomplish this. Then, modify the loop so it waits for an array of promises to complete before resolving.

async createPallet(data) {
  try {
    // modified loop
    const newIDs = await Promise.all(data.map((record) => {
      mysqlConnection.beginTransaction();
      // this is a best guess for how the a promise-style query would look
      const await result = await mysqlConnection.query(
        "INSERT INTO ...",
        [record.BatchId, record.PalletNumber, record.PrimaryWeightId]
      );

      // Create pallet sku record
      await PalletSkusService.createPalletSku(
        ...
      );

      console.log(results.insertId);
      return results.insertId;
    }));

    return { status: true, data: newIDs };
  } catch (error) {
    mysqlConnection.rollback();
    throw error;
  }
},
Bergi
  • 630,263
  • 148
  • 957
  • 1,375
NetByMatt
  • 703
  • 4
  • 13