Correct handling of async Mongo actions in Node Pr

问题:

I have a function which performs multiple Mongo actions, and the last action is to close the database after all other actions have completed. I was fairly confident of my handling of the issue, but I have had some external comments raise concerns, and I wanted to verify if my solution is correct.

A suggested solution:

function updateDatabase (name, token) {
  return new Promise((resolve, reject) => {
    MongoClient.connect(MONGODB_URL)
      .then( (database) => {
          return database.collection('testCollection').update({name}, {$pull: {tokens: {$in: [token]}}})
            .then( () => {
              database.collection('log').insert({
                name,
                token
              });
              return database;
            })
      })
      .then( (db) => {
        db.close(true);
        resolve(`Invalid token: ${token} has been removed from: ${name}`);
      })
      .catch( (err) => {
        reject(err);
      })
  });
}

My original solution:

function updateDatabase (name, token) {
  return new Promise((resolve, reject) => {
    MongoClient.connect(MONGODB_URL)
      .then( (database) => {
        return database;
      })
      .then( (db) => {
        database.collection('testCollection').update({name}, {$pull: {tokens: {$in: [token]}}})
        return database;
      })
      .then( () => {
        database.collection('log').insert({
          name,
          token
        });
        return database;
      })
      .then( (db) => {
        db.close(true);
        resolve(`Invalid token: ${token} has been removed from: ${name}`);
      })
      .catch( (err) => {
        reject(err);
      })
  });
}

Is my original solution way off the mark or is the suggested solution a better approach? Both work in testing but against a production grade load, I need to make sure the db does not close until other actions are complete, which I believe I have accomplished in my original solution.

回答1:

Since all asynchronous operations of the MongoDB driver already return a promise, you shouldn't use new Promise at all, but set up a promise chain:

function updateDatabase(name, token) {
  let database;
  return MongoClient.connect(MONGODB_URL).then(db => {
    database = db;
    return database
      .collection("testCollection")
      .update({ name }, { $pull: { tokens: { $in: [token] } } });
  })
  .then(() => {
    return database.collection("log").insert({
      name,
      token
    });
  })
  .then(() => {
    database.close(true);
  })
  .catch(err => {
    database.close(true);
    throw err;
  });
}

I understand that you want to pass database as an argument for the next then, but you'll run into the issue that it won't be available in the catch handler. One solution is to use a function-scoped variable that gets assigned after opening the connection, like the code above does.

If you don't like that, you can create a new promise chain inside the .then handler for MongoClient.connect:

function updateDatabase(name, token) {
  return MongoClient.connect(MONGODB_URL).then(database => {
    return database
      .collection("testCollection")
      .update({ name }, { $pull: { tokens: { $in: [token] } } })
      .then(() => {
        return database.collection("log").insert({
          name,
          token
        });
      })
      .then(() => {
        database.close(true);
      })
      .catch(err => {
        database.close(true);
        throw err;
      });
  });
}

回答2:

I have tried to outline some major problems with the original code:

function updateDatabase (name, token) {
  //useless. MongoClient.connect already returns the promise
  return new Promise((resolve, reject) => {
    MongoClient.connect(MONGODB_URL)
      .then( (database) => {
        // useless. The function does nothing and can be removed
        return database;  
      })
      .then( (db) => {
        // should be db, not database
        // update is async. Should be added to the chain.
        database.collection('testCollection').update({name}, {$pull: {tokens: {$in: [token]}}})
        // what's the point to return database, if the following function does not accept any parameters
        return database;
      })      
      .then( () => {
        // insert is async. Should be added to the chain.
        database.collection('log').insert({
          name,
          token
        });
        return database;
      })
      .then( (db) => {
        // close is async. Should be added to the chain.
        db.close(true);
        resolve(`Invalid token: ${token} has been removed from: ${name}`);
      })
      .catch( (err) => {
        reject(err);
      })
  });
}

So the function should really look like:

function updateDatabase (name, token) {
    return MongoClient.connect(MONGODB_URL)
      .then( db => 
        db.collection('testCollection').update({name}, {$pull: {tokens: {$in: [token]}}})
          .then(()=>db)
      })      
      .then( db => 
        db.collection('log').insert({name, token})
          .then(()=>db)
      })
      .then( db => db.close(true))
      .then(()=>`Invalid token: ${token} has been removed from: ${name}`);
}

And if the order of queries doesn't matter, you can benefit from Promise.all:

function updateDatabase (name, token) {
    return MongoClient.connect(MONGODB_URL)
      .then( db => Promise.all([
        Promise.resolve(db),
        db.collection('testCollection').update({name}, {$pull: {tokens: {$in: [token]}}}),
        db.collection('log').insert({name, token}),
      ])
      .then( ([db]) => db.close(true))
      .then(()=>`Invalid token: ${token} has been removed from: ${name}`);
}

回答3:

The common problem of both solutions is .then(function) handler. Instead of returning Promise for MongoDB operation, handler returns database, therefore next chain handler will be called not waiting for MongoDB operation to complete. Instead Promises for MongoDB operations should be chained in order of execution. Also Mongo methods return Promise, so no need for new Promise:

function updateDatabase (name, token) {
  return MongoClient.connect(MONGODB_URL)
     .then( (db) => 
          Promise.all([
            db.collection('testCollection').update({name}, {$pull: {tokens: {$in: [token]}}}),
            db.collection('log').insert({name, token})
          ]).then(() => db.close(true)
          ).then(`Invalid token: ${token} has been removed from: ${name}`
          ).catch((err) => { db.close(true); throw err });
     )
}

or with async/await syntax:

async function updateDatabase (name, token) {
    let db = await MongoClient.connect(MONGODB_URL);
    try {
      await db.collection('testCollection').update({name}, {$pull: {tokens: {$in: [token]}}});
      await db.collection('log').insert({name, token});
      return `Invalid token: ${token} has been removed from: ${name}`;
    } finally { 
      await db.close(true);
    }
}