The "resolve" callback runs before completion of for loop and therefore an empty array is passed, Is the syntax wrong or is there any other way?

Question

The for loop should finish first and then resolve function should run.

let promise = new Promise(function(resolve, reject) {
  arr = [];
  for (let i = 0; i < req.body.length; i++) {
    Customer.find({
      '_id': req.body[i]
    }).then(documents => {
      console.log("name", documents[0].name);
      arr.push(documents[0])
    });
  }
  resolve(arr);
});

promise.then(
  result => {
    console.log(result);
  }
)

because you are not resolving promise in the asynchronous calls. You are doing it after the loop. You should be using promise all also. — epascarello, Jul 14 '20 at 19:11
Here's what happens: Create `arr` on the global scope and set it to an empty array. Iterate over `req.body` and queue some calls to `Customer.find`. Without waiting for them, resolve the promise with the empty array. — Heretic Monkey, Jul 14 '20 at 19:13
`let promise = Promise.all( req.body.map( id => Customer.find({ '_id': id }) ) )` — Paul, Jul 14 '20 at 19:14
Avoid the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! — Bergi, Jul 14 '20 at 19:19
Very relevant: [mongoose findMany - find all documents with IDs listed in array](https://stackoverflow.com/q/8303900/1048572). Do not make N queries! — Bergi, Jul 14 '20 at 19:21
I cannot use FindMany because the array of Id`s can be for example [1,1,1,2] and it will return only two object. — Deepanshu Malviya, Jul 14 '20 at 19:27
@DeepanshuMalviya If you need to duplicate objects, then sure just do that in your code. But running the query multiple times when you know you have duplicate ids is even more inefficient! — Bergi, Jul 14 '20 at 19:38

Sven.hig · Accepted Answer · 2020-07-14T22:00:24.530

0

Your Custom.find method is asynchronous so while your asynchronous function is running to return a promise the rest of your function outside the function runs as normal hence why you get an empty array returned, the code executes before your async function finished inside the loop to get the array with values outside the loop make your promise return a value which you can access later outside the loop

let promise = new Promise(function(resolve, reject) {
  arr = [];
  for (let i = 0; i < req.body.length; i++) {
    var finalvalue=Customer.find({
      '_id': req.body[i]
    }).then(documents => {
      console.log("name", documents[0].name);
      arr.push(documents[0])
      return arr
    });
  }
  resolve(finalvalue)
});

promise.then(
  result => {
    console.log(result);
  }
)

here is a working example

cutosm=(i)=>new Promise(r=>{
  setTimeout(()=>{
      r(i)
  },300 +Math.random()*200)
})
async function smfunc(){
  arr = [];
  for (let i = 0; i < 8; i++) {
   var finalvalue= await cutosm(i).then(e => {
      arr.push(e)
      return arr
    });
  }

  return finalvalue

}
smfunc().then(r=>console.log(r))

edited Jul 14 '20 at 22:00

answered Jul 14 '20 at 19:37

Sven.hig

4,449
2
8
18

1

This uses the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it), and it doesn't work as it only waits (and handles errors) for the last `Customer.find()` call (which is not guaranteed to finish last). Try to add a random factor to your `setTimeout` and see it fail. – Bergi Jul 14 '20 at 19:47
@Bergi thank you for your comment can you give me an example of the random factor I should add to my setTimeout?, I have looked at the link you have added, and i can't see how is this a deferred anti-pattern, can you elaborate, what makes this code a deferred anti pattern ? – Sven.hig Jul 14 '20 at 20:38
@Sven I've taken the liberty to edit to show how the random factor affects the result. Try running the snippet multiple times. To avoid this, you need `Promise.all`. – Bergi Jul 14 '20 at 20:41
@Sven It's the explicit promise construction antipattern to wrap a call that already returns a promise (`Customer.find(…).then(…)`) inside a `new Promise`. It's completely unnecessary here, you could as well just write `let promise = finalvalue`. – Bergi Jul 14 '20 at 20:43
From what I understood in the edited example the loop keeps running while a random value generated in the setTimeout which will lead to random value been generated and some values will obviously be missing, can you give me a real life example where this may happen because i am failing to see where this example is relevant in this situation, because OP is making a `db` request and mongo db takes care of concurrency and locking – Sven.hig Jul 14 '20 at 21:41
It will fail if there are multiple api calls to different apis/dbs where we don't know for certain which promise will be returned first and in that situation I agree `Promise.all` or `Promise.allSettled()` are the best way to go, but in this situation all what OP wants is to make calls to the db after each loop to the same `db` also the example above can be easily managed by an async/await – Sven.hig Jul 14 '20 at 21:50

The "resolve" callback runs before completion of for loop and therefore an empty array is passed, Is the syntax wrong or is there any other way?

1 Answers1