Can not access outer scope variable inside chained promise used in for loop

Question

I need to do async/await inside for..loop, but that way I can't do parallel operations.

So I am pushing those promises into an variable promises later I will do promise.all() on it.

Here is my code.

const promises = [];
  for (let i = 0; i < todayAssignedJobs.length; i += 1) {
    promises.push(Leaderboard.findOrCreate({...}).then((leaderboard, created) => {
      if (!created) {
        const rating = (todayAssignedJobs[i].rating + leaderboard.rating * leaderboard.jobs_completed) / (leaderboard.jobs_completed + 1);
        const commission = todayAssignedJobs[i].commission + leaderboard.commission;
        const jobsCompleted = leaderboard.jobs_completed + 1;
        Leaderboard.update({
          rating,
          commission,
          jobs_completed: jobsCompleted,
          updated_by: 'system',
        }, {
          where: {
            id: leaderboard.id,
          },
        });
      }
      AssignedJob.update({
        is_leaderboard_generated: true,
      }, {
        where: {
          id: todayAssignedJobs[i].id,
        },
      });
    }));
  }
await Promise.all(promises)

Somehow, I am unable to get the id when I do Assigned.update({..}, { where: { id: todayAssignedJobs[i].id }})

Getting error:

Unhandled rejection Error: WHERE parameter "id" has invalid "undefined" value

Can someone explain what is going on? Also, please suggest can I do below?

promises.push(async () => { // I will use await here })

I would log the value of `todayAssignedJobs[i]`. My guess is that the object doesn't have an `id` property, since a for loop using `let` provides proper lexical scope for `i`, so asynchronicity isn't the problem. For a cleaner approach, notice you're basically re-implementing [`Array.prototype.map()`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/map), so just use that instead of a `for` loop to initialize `promises`. — Patrick Roberts, Apr 22 '20 at 09:17
@PatrickRoberts Thanks for your suggestion. I replaced `for` with `map` now it's much cleaner. :) — sujeet, Apr 22 '20 at 09:36
A `then` callback takes only a single parameter, `(leaderboard, created)` does not make sense — Bergi, Apr 22 '20 at 09:40

sujeet · Answer 1 · 2020-04-22T09:39:21.707

Problem was that findOrCreate() returns an array with two values [ {...}, boolean ]

I was passing them (leaderboard, created) here created was always undefined and leaderboard an array.

I made changes and now it's working fine.

const promises = todayAssignedJobs.map((todayAssigned) => Leaderboard.findOrCreate({...}).then(([leaderboard, created]) => {
    if (!created) {
      const rating = (todayAssigned.rating + leaderboard.rating * leaderboard.jobs_completed) / (leaderboard.jobs_completed + 1);
      const commission = todayAssigned.commission + leaderboard.commission;
      const jobsCompleted = leaderboard.jobs_completed + 1;
      Leaderboard.update({
        rating,
        commission,
        jobs_completed: jobsCompleted,
        updated_by: 'system',
      }, {
        where: {
          id: leaderboard.id,
        },
      });
    }
    AssignedJob.update({
      is_leaderboard_generated: true,
    }, {
      where: {
        id: todayAssigned.id,
      },
    });
  }));

await Promise.all(promises);

Patrick Roberts · Accepted Answer · 2020-04-22T10:29:26.990

Just some further improvements on the existing answer.

Leaderboard.update() and AssignedJob.update() are async functions that need to be awaited, so the callback function needs to be converted to an async function. This ensures that the promise doesn't resolve until all the database operations are completed, not just the findOrCreate():

const promises = todayAssignedJobs.map(async todayAssigned => {
  const [leaderboard, created] = await Leaderboard.findOrCreate({...});

  if (!created) {
    const rating = (todayAssigned.rating + leaderboard.rating * leaderboard.jobs_completed) / (leaderboard.jobs_completed + 1);
    const commission = todayAssigned.commission + leaderboard.commission;
    const jobsCompleted = leaderboard.jobs_completed + 1;

    await Leaderboard.update({
      rating,
      commission,
      jobs_completed: jobsCompleted,
      updated_by: 'system',
    }, {
      where: {
        id: leaderboard.id,
      },
    });
  }

  await AssignedJob.update({
    is_leaderboard_generated: true,
  }, {
    where: {
      id: todayAssigned.id,
    },
  });
});

await Promise.all(promises);

A more fundamental problem with this approach is that Leaderboard.findOrCreate() and Leaderboard.update() are not part of a single transaction. This is problematic because the update() depends on the current value of the entry in leaderboard, which creates a race condition in your database due to the non-atomic modification of the entry:

const rating = (todayAssigned.rating + leaderboard.rating * leaderboard.jobs_completed) / (leaderboard.jobs_completed + 1);
const commission = todayAssigned.commission + leaderboard.commission;
const jobsCompleted = leaderboard.jobs_completed + 1;

Each of the methods needs to be marked as part of a single transaction. With sequelize.js you can achieve that using a managed transaction:

const promises = todayAssignedJobs.map(
  todayAssigned => sequelize.transaction(async transaction => {
    const [leaderboard, created] = await Leaderboard.findOrCreate({
      transaction,
      ...
    });

    if (!created) {
      const rating = (todayAssigned.rating + leaderboard.rating * leaderboard.jobs_completed) / (leaderboard.jobs_completed + 1);
      const commission = todayAssigned.commission + leaderboard.commission;
      const jobsCompleted = leaderboard.jobs_completed + 1;

      await Leaderboard.update({
        rating,
        commission,
        jobs_completed: jobsCompleted,
        updated_by: 'system',
      }, {
        transaction,
        where: {
          id: leaderboard.id,
        },
      });
    }

    await AssignedJob.update({
      is_leaderboard_generated: true,
    }, {
      transaction,
      where: {
        id: todayAssigned.id,
      },
    })
  })
);

await Promise.all(promises);

I tried something same, but `promises` was returning an array `[ [AsyncFunction], [AsyncFunction] ]` after calling `await Promise.all(promises)` . Can you explain how `Promise.all(promises)` resolves them here? — sujeet, Apr 22 '20 at 09:59
Got it, but I am not talking about this problem here. Please look here. I tried the same approach. https://stackoverflow.com/questions/61337600/passing-async-functions-to-promise-all/61337736 — sujeet, Apr 22 '20 at 10:07
That's not the same at all, and your question has already been thoroughly answered by someone else. — Patrick Roberts, Apr 22 '20 at 10:09
But this line _The first problem is that Promise.all accepts an array of promises, not an array of functions - your current code won't work._ aren't we doing the same here? Passing an array of functions to `Promise.all()` — sujeet, Apr 22 '20 at 10:11
@SujeetAgrahari by your logic, `let x = [1, 2, 3, 4, 5].map(v => v * 2)` would make `x` an array of functions. `promises` is an array of _promises_, not functions. — Patrick Roberts, Apr 22 '20 at 10:15

Can not access outer scope variable inside chained promise used in for loop

2 Answers2