feat: expand use of error labels for retryable writes

The work here encompasses changes to the retryable writes logic of the driver, such that the `RetryableWriteError` label becomes the primary means of determining whether an operation will be retried. 4.4+ servers will attach this label server-side, so this change allows us to gracefully remove client-side checking of retryable write errors. NODE-2379
mongodb · Feb 2, 2020 · c775a4a · c775a4a
1 parent 7bbad9c
commit c775a4a
Show file tree

Hide file tree

Showing 12 changed files with 86 additions and 19 deletions.
diff --git a/lib/core/error.js b/lib/core/error.js
@@ -168,6 +168,10 @@ class MongoWriteConcernError extends MongoError {
     super(message);
     this.name = 'MongoWriteConcernError';
 
+    if (result && Array.isArray(result.errorLabels)) {
+      this[kErrorLabels] = new Set(result.errorLabels);
+    }
+
     if (result != null) {
       this.result = makeWriteConcernResultObject(result);
     }
@@ -189,6 +193,32 @@ const RETRYABLE_ERROR_CODES = new Set([
   13436 // NotMasterOrSecondary
 ]);
 
+const RETRYABLE_WRITE_ERROR_CODES = new Set([
+  11600, // InterruptedAtShutdown
+  11602, // InterruptedDueToReplStateChange
+  10107, // NotMaster
+  13435, // NotMasterNoSlaveOk
+  13436, // NotMasterOrSecondary
+  189, // PrimarySteppedDown
+  91, // ShutdownInProgress
+  7, // HostNotFound
+  6, // HostUnreachable
+  89, // NetworkTimeout
+  9001, // SocketException
+  262 // ExceededTimeLimit
+]);
+
+function isRetryableWriteError(error) {
+  if (error instanceof MongoWriteConcernError) {
+    return (
+      RETRYABLE_WRITE_ERROR_CODES.has(error.code) ||
+      RETRYABLE_WRITE_ERROR_CODES.has(error.result.code)
+    );
+  }
+
+  return RETRYABLE_WRITE_ERROR_CODES.has(error.code);
+}
+
 /**
  * Determines whether an error is something the driver should attempt to retry
  *
@@ -284,5 +314,6 @@ module.exports = {
   isRetryableError,
   isSDAMUnrecoverableError,
   isNodeShuttingDownError,
-  isNetworkTimeoutError
+  isNetworkTimeoutError,
+  isRetryableWriteError
 };
diff --git a/lib/core/sdam/server.js b/lib/core/sdam/server.js
@@ -14,8 +14,10 @@ const collationNotSupported = require('../utils').collationNotSupported;
 const debugOptions = require('../connection/utils').debugOptions;
 const isSDAMUnrecoverableError = require('../error').isSDAMUnrecoverableError;
 const isNetworkTimeoutError = require('../error').isNetworkTimeoutError;
+const isRetryableWriteError = require('../error').isRetryableWriteError;
 const makeStateMachine = require('../utils').makeStateMachine;
 const common = require('./common');
+const ServerType = common.ServerType;
 
 // Used for filtering out fields for logging
 const DEBUG_FIELDS = [
@@ -404,6 +406,14 @@ Object.defineProperty(Server.prototype, 'clusterTime', {
   }
 });
 
+function supportsRetryableWrites(server) {
+  return (
+    server.description.maxWireVersion >= 6 &&
+    server.description.logicalSessionTimeoutMinutes &&
+    server.description.type !== ServerType.Standalone
+  );
+}
+
 function calculateRoundTripTime(oldRtt, duration) {
   const alpha = 0.2;
   return alpha * duration + (1 - alpha) * oldRtt;
@@ -449,11 +459,22 @@ function makeOperationHandler(server, options, callback) {
           options.session.serverSession.isDirty = true;
         }
 
+        if (supportsRetryableWrites(server)) {
+          err.addErrorLabel('RetryableWriteError');
+        }
+
         if (!isNetworkTimeoutError(err)) {
           server.emit('error', err);
         }
-      } else if (isSDAMUnrecoverableError(err)) {
-        server.emit('error', err);
+      } else {
+        // if pre-4.4 server, then add error label if its a retryable write error
+        if (server.description.maxWireVersion < 9 && isRetryableWriteError(err)) {
+          err.addErrorLabel('RetryableWriteError');
+        }
+
+        if (isSDAMUnrecoverableError(err)) {
+          server.emit('error', err);
+        }
       }
     }
 

diff --git a/lib/core/sdam/topology.js b/lib/core/sdam/topology.js
@@ -680,7 +680,7 @@ class Topology extends EventEmitter {
 
       const cb = (err, result) => {
         if (!err) return callback(null, result);
-        if (!isRetryableError(err)) {
+        if (!err.hasErrorLabel('RetryableWriteError')) {
           return callback(err);
         }
 
@@ -944,7 +944,7 @@ function executeWriteOperation(args, options, callback) {
 
     const handler = (err, result) => {
       if (!err) return callback(null, result);
-      if (!isRetryableError(err)) {
+      if (!err.hasErrorLabel('RetryableWriteError')) {
         err = getMMAPError(err);
         return callback(err);
       }

diff --git a/test/functional/retryable_writes.test.js b/test/functional/retryable_writes.test.js
@@ -71,7 +71,11 @@ function executeScenarioSetup(scenario, test, config, ctx) {
         throw err;
       }
     })
-    .then(() => (scenario.data ? ctx.collection.insertMany(scenario.data) : {}))
+    .then(() =>
+      Array.isArray(scenario.data) && scenario.data.length
+        ? ctx.collection.insertMany(scenario.data)
+        : {}
+    )
     .then(() => (test.failPoint ? ctx.db.executeDbAdminCommand(test.failPoint) : {}));
 }
 

diff --git a/test/spec/retryable-writes/bulkWrite-errorLabels.json b/test/spec/retryable-writes/bulkWrite-errorLabels.json
@@ -166,13 +166,13 @@
         },
         "collection": {
           "data": [
-            {
-              "_id": 1,
-              "x": 11
-            },
             {
               "_id": 2,
               "x": 22
+            },
+            {
+              "_id": 3,
+              "x": 33
             }
           ]
         }

diff --git a/test/spec/retryable-writes/bulkWrite-errorLabels.yml b/test/spec/retryable-writes/bulkWrite-errorLabels.yml
@@ -73,5 +73,5 @@ tests:
               errorLabelsOmit: ["RetryableWriteError"]
           collection:
               data:
-                  - { _id: 1, x: 11 }
                   - { _id: 2, x: 22 }
+                  - { _id: 3, x: 33 }
diff --git a/test/spec/retryable-writes/bulkWrite-serverErrors.json b/test/spec/retryable-writes/bulkWrite-serverErrors.json
@@ -252,7 +252,7 @@
           "data": [
             {
               "_id": 2,
-              "x": 23
+              "x": 22
             },
             {
               "_id": 3,

diff --git a/test/spec/retryable-writes/bulkWrite-serverErrors.yml b/test/spec/retryable-writes/bulkWrite-serverErrors.yml
@@ -124,5 +124,5 @@ tests:
                 errorLabelsContain: ["RetryableWriteError"]
           collection:
                 data:
-                    - { _id: 2, x: 23 }
+                    - { _id: 2, x: 22 }
                     - { _id: 3, x: 33 }
diff --git a/test/spec/retryable-writes/findOneAndReplace-errorLabels.json b/test/spec/retryable-writes/findOneAndReplace-errorLabels.json
@@ -28,7 +28,7 @@
         },
         "data": {
           "failCommands": [
-            "findOneAndModify"
+            "findAndModify"
           ],
           "errorCode": 112,
           "errorLabels": [
@@ -77,7 +77,7 @@
         },
         "data": {
           "failCommands": [
-            "findOneAndModify"
+            "findAndModify"
           ],
           "errorCode": 11600,
           "errorLabels": []

diff --git a/test/spec/retryable-writes/findOneAndReplace-errorLabels.yml b/test/spec/retryable-writes/findOneAndReplace-errorLabels.yml
@@ -12,7 +12,7 @@ tests:
           configureFailPoint: failCommand
           mode: { times: 1 }
           data:
-              failCommands: ["findOneAndModify"]
+              failCommands: ["findAndModify"]
               errorCode: 112 # WriteConflict, not a retryable error code
               errorLabels: ["RetryableWriteError"] # Override server behavior: send RetryableWriteError label with non-retryable error code
       operation:
@@ -33,7 +33,7 @@ tests:
           configureFailPoint: failCommand
           mode: { times: 1 }
           data:
-              failCommands: ["findOneAndModify"]
+              failCommands: ["findAndModify"]
               errorCode: 11600 # InterruptedAtShutdown, normally a retryable error code
               errorLabels: []  # Override server behavior: do not send RetryableWriteError label with retryable code
       operation:

diff --git a/test/spec/retryable-writes/insertOne-serverErrors.json b/test/spec/retryable-writes/insertOne-serverErrors.json
@@ -1046,7 +1046,16 @@
           ]
         },
         "collection": {
-          "data": []
+          "data": [
+            {
+              "_id": 1,
+              "x": 11
+            },
+            {
+              "_id": 2,
+              "x": 22
+            }
+          ]
         }
       }
     }

diff --git a/test/spec/retryable-writes/insertOne-serverErrors.yml b/test/spec/retryable-writes/insertOne-serverErrors.yml
@@ -481,4 +481,6 @@ tests:
             result:
                 errorLabelsContain: ["RetryableWriteError"]
             collection:
-                data: []
+                data:
+                    - { _id: 1, x: 11 }
+                    - { _id: 2, x: 22 }