fix(execute-operation): don't swallow synchronous errors

This simplifies the logic for `executeOperation` such that errors are not swallowed accidentally when they are thrown synchronously. NODE-2400
mongodb · Dec 26, 2019 · 0a2d4e9 · 0a2d4e9
1 parent cca5b49
commit 0a2d4e9
Show file tree

Hide file tree

Showing 3 changed files with 46 additions and 52 deletions.
diff --git a/lib/operations/execute_operation.js b/lib/operations/execute_operation.js
@@ -53,54 +53,45 @@ function executeOperation(topology, operation, callback) {
     }
   }
 
-  const makeExecuteCallback = (resolve, reject) =>
-    function executeCallback(err, result) {
-      if (session && session.owner === owner) {
-        session.endSession(() => {
-          if (operation.session === session) {
-            operation.clearSession();
-          }
-          if (err) return reject(err);
-          resolve(result);
-        });
-      } else {
+  let result;
+  if (typeof callback !== 'function') {
+    result = new Promise((resolve, reject) => {
+      callback = (err, res) => {
         if (err) return reject(err);
-        resolve(result);
-      }
-    };
-
-  // Execute using callback
-  if (typeof callback === 'function') {
-    const handler = makeExecuteCallback(
-      result => callback(null, result),
-      err => callback(err, null)
-    );
+        resolve(res);
+      };
+    });
+  }
 
-    try {
-      if (operation.hasAspect(Aspect.EXECUTE_WITH_SELECTION)) {
-        return executeWithServerSelection(topology, operation, handler);
-      } else {
-        return operation.execute(handler);
+  function executeCallback(err, result) {
+    if (session && session.owner === owner) {
+      session.endSession();
+      if (operation.session === session) {
+        operation.clearSession();
       }
-    } catch (e) {
-      handler(e);
-      throw e;
     }
-  }
 
-  return new Promise(function(resolve, reject) {
-    const handler = makeExecuteCallback(resolve, reject);
+    callback(err, result);
+  }
 
-    try {
-      if (operation.hasAspect(Aspect.EXECUTE_WITH_SELECTION)) {
-        return executeWithServerSelection(topology, operation, handler);
-      } else {
-        return operation.execute(handler);
+  try {
+    if (operation.hasAspect(Aspect.EXECUTE_WITH_SELECTION)) {
+      executeWithServerSelection(topology, operation, executeCallback);
+    } else {
+      operation.execute(executeCallback);
+    }
+  } catch (e) {
+    if (session && session.owner === owner) {
+      session.endSession();
+      if (operation.session === session) {
+        operation.clearSession();
       }
-    } catch (e) {
-      handler(e);
     }
-  });
+
+    throw e;
+  }
+
+  return result;
 }
 
 function supportsRetryableReads(server) {

diff --git a/lib/operations/find_one.js b/lib/operations/find_one.js
@@ -17,16 +17,20 @@ class FindOneOperation extends OperationBase {
     const query = this.query;
     const options = this.options;
 
-    const cursor = coll
-      .find(query, options)
-      .limit(-1)
-      .batchSize(1);
+    try {
+      const cursor = coll
+        .find(query, options)
+        .limit(-1)
+        .batchSize(1);
 
-    // Return the item
-    cursor.next((err, item) => {
-      if (err != null) return handleCallback(callback, toError(err), null);
-      handleCallback(callback, null, item);
-    });
+      // Return the item
+      cursor.next((err, item) => {
+        if (err != null) return handleCallback(callback, toError(err), null);
+        handleCallback(callback, null, item);
+      });
+    } catch (e) {
+      callback(e);
+    }
   }
 }
 

diff --git a/test/functional/operation_example.test.js b/test/functional/operation_example.test.js
@@ -3718,7 +3718,7 @@ describe('Operation Examples', function() {
       var configuration = this.configuration;
       var client = configuration.newClient(configuration.writeConcernMax(), { poolSize: 1 });
       client.connect(function(err, client) {
-        test.equal(null, err);
+        expect(err).to.not.exist;
 
         // LINE var MongoClient = require('mongodb').MongoClient,
         // LINE   test = require('assert');
@@ -3732,8 +3732,7 @@ describe('Operation Examples', function() {
         // BEGIN
         // Close the connection with a callback that is optional
         client.close(function(err) {
-          test.equal(null, err);
-
+          expect(err).to.not.exist;
           done();
         });
       });