Node - Async for Loop not waiting causes Mongo to disconnect too early

Question

I have an array of objects that I'm parsing. I check Mongo to see if an entry exists for the record. If so, I update the record. otherwise, I create the record. This is working fine. However, after the loop processes I want to execute mongoose.disconnect(). However this happens during the loop. I've done this before, but this time is slightly different and I'm not having any luck. The only thing different is I'm calling another function that requires an await when saving the new entries.

mongoose.set('useCreateIndex', true);
  mongoose
    .connect(db, { useNewUrlParser: true })
    .then(() => console.log('MongoDB Connected'))
    .then(async () => {
      await parseWIP(converted);
      mongoose.disconnect();
    })
    .catch(err => console.log(err));
});

function parseWIP(array) {
  return new Promise(async (resolve, reject) => {
    for (let wip of array) {

      WIP.findOne({ query_here })
        .then(async existingWip => {
          if (existingWip && existingWip.id) {
            // Update existing record

            const salesOrderState = getPatientReadableStatus(
              wip.work_in_progress_wip_state
            );

            existingWip.salesOrder.state = salesOrderState.state;
            existingWip.salesOrder.info = salesOrderState.info;
            existingWip.salesOrder.progress = salesOrderState.progress;
            existingWip.lastModified = moment.now();
            await existingWip.save().then(updatedWip => {
              console.log(`Updated WIP`);
            });
          } else {
            // Create new record
            await createNewWip(wip);
          }
        })
        .catch(err => {
          console.log(err);
          reject(err);
        });
    }
    resolve();
  });
}

function createNewWip(wip) {
  return new Promise(async (resolve, reject) => {
    let patientPhone = wip.patient_phone;
    if (!wip.patient_phone && wip.patient_mobile) {
      patientPhone = wip.patient_mobile;
    }

    const branch = await getBranchContactInfo(wip.sales_order_branch_office);
    const salesOrderState = getPatientReadableStatus(
      wip.work_in_progress_wip_state
    );

    let wipRecord = { ... objects ... };

    const entry = new WIP(wipRecord);

    await entry
      .save()
      .then(savedWipRecord => {
        console.log(savedWipRecord._id);
      })
      .catch(err => reject(err));
    resolve();
  });
}

I have tried forEach for(let wip of array) and for(let wip in array). Why is the promise returning immediately ?

Answer 1

If you want to call await on a function, the function itself has to return a Promise, otherwise, it will definitely return immediately since there's nothing telling the system to wait on what part of your code.

So, the solution would be something like this (untested, but the idea should be fairly simple)

function parseWIP(array) {
    return new Promise(async (resolve, reject) => {
        for (let wip of array) {
            //array.forEach(async wip => {
            //let wip = array[key];

            await WIP.findOne({ query_goes_here })
              .then(async existingWip => {
                if (existingWip && existingWip.id) {
                  // Update existing record
                  console.log(`Existing WIP: ${existingWip.id}`);
                  ... assign some values ...
                  existingWip.lastModified = moment.now();

                  await existingWip.save().then(updatedWip => {
                    console.log(`Updated WIP ${updatedWip.id}`);
                  });
                } else {
                  // Create new record
                  await createNewWip(wip);
                }
              })
              .catch(err => {
                  console.log(err);
                  reject(err);
                });
        }  
        console.log('here we are at end of loop'); 
        resolve();
    });
}

Answer 2

Why mix async/await with standard Promise syntax when you can just write it all in the one standard way. Also all mongoose methods return a Promise anyway, so I don't see why you even have anything attempting to wrap a callback with a Promise.

The listing basically shows a misunderstanding of Promises and async/await, so the example here should clear some things up:

// Require mongoose and add your model definitions

const uri = 'mongodb://localhost:27017/test';
const opts = { useNewUrlParser: true };

(async function() {

  try {

    const conn = await mongoose.connect(uri, opts);

    // get your 'data' from somewhere of course.

    for ( let wip of data ) {

      let existingWIP = await WIP.findOne(query_goes_here);

      if (existingWip) {             // Asking for "id" on null would be an error

        // Update existing record
        console.log(`Existing WIP: ${existingWip.id}`);
        ... assign some values ...
        existingWip.lastModified = moment.now();

        let updatedWip = await existingWip.save()
        console.log(`Updated WIP ${updatedWip.id}`); // though you should understand this does not change
                                                     // as id is immutable


      } else {
        let newWip = await WIP.create(wip);  // not sure why you are creating a function
                                           // but all mongoose methods return a promise anyway
        // maybe do something
       }

    }
  } catch (e) {
    console.error(e);
  } finally {
    mongoose.disconnect();
  }

})()

Noting that await basically means you don't need to do then() because they are in fact the same thing, but just with a cleaner syntax. Same applies to .catch() since it's again much cleaner to try..catch instead.

Break this up functionally if you must, but if you're just doing a quick script to load and update some things, then there probably is not much point. Just make sure function() implementations all return a Promise ( i.e the native mongoose method result ) and that you await them.

Also, you might want to basically look at findOneAndUpdate() and the "upsert" option in particular. That alone would basically remove your if..then..else condition and do it all in one request instead of a separated find() and save()

// Require mongoose and add your model definitions

const uri = 'mongodb://localhost:27017/test';
const opts = { useNewUrlParser: true };

(async function() {

  try {

    const conn = await mongoose.connect(uri, opts);

    // get your 'data' from somewhere of course.

    for ( let wip of data ) {

      let updatedWip = await WIP.findOneAndUpdate(
        query_goes_here,
        update_statement_goes_here,
        { upsert: true, new: true }     // need these options
      );

      console.log(`Updated WIP ${updatedWip.id}`);
    }
  } catch (e) {
    console.error(e);
  } finally {
    mongoose.disconnect();
  }

})()

Or of course if you just don't need to actually do anything in the "loop", then you can just use bulkWrite():

// Require mongoose and add your model definitions

const uri = 'mongodb://localhost:27017/test';
const opts = { useNewUrlParser: true };

(async function() {

  try {

    const conn = await mongoose.connect(uri, opts);

    // get your 'data' from somewhere of course.

    let result = await WIP.bulkWrite(
      data.map(wip => 
       ({
          updateOne: {
            filter: query_based_on_wip_values
            update: update_based_on_wip_values,
            upsert: true
          }
       })
     )
   );

  } catch (e) {
    console.error(e);
  } finally {
    mongoose.disconnect();
  }

})()

And that of course only requires one request to the server with one response for all the content in the array. If the array is particularly large, then you might want to break that up. But then again for a "large array" you should be loading the data in pieces so it would not all be in an array.

Overall, pick one structural pattern and stick with it, and take the time to understand the API methods and what they do. Typically the find() then modify in code and save() pattern is really bad practice, and basically introduces additional overhead of requests back and forth, plus the obvious problem that data you read has possibly changed by another process/update by the time you decide to write it back.