refactor: align isSDAMUnrecoverableError with spec guidance

This makes the error checking for whether an error is an SDAM unrecoverable error line up with the pseudocode in the SDAM specification.
mongodb · Aug 22, 2019 · 5188bda · 5188bda
1 parent dd79a22
commit 5188bda
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 17 deletions.
diff --git a/lib/core/error.js b/lib/core/error.js
@@ -153,28 +153,62 @@ function isRetryableError(error) {
   );
 }
 
-const SDAM_UNRECOVERABLE_ERROR_CODES = new Set([
+const SDAM_RECOVERING_CODES = new Set([
   91, // ShutdownInProgress
   189, // PrimarySteppedDown
-  10107, // NotMaster
   11600, // InterruptedAtShutdown
   11602, // InterruptedDueToReplStateChange
-  13435, // NotMasterNoSlaveOk
   13436 // NotMasterOrSecondary
 ]);
 
+const SDAM_NOTMASTER_CODES = new Set([
+  10107, // NotMaster
+  13435 // NotMasterNoSlaveOk
+]);
+
+const SDAM_NODE_SHUTTING_DOWN_ERROR_CODES = new Set([
+  11600, // InterruptedAtShutdown
+  91 // ShutdownInProgress
+]);
+
+function isRecoveringError(err) {
+  if (err.code && SDAM_RECOVERING_CODES.has(err.code)) {
+    return true;
+  }
+
+  return err.message.match(/not master or secondary/) || err.message.match(/node is recovering/);
+}
+
+function isNotMasterError(err) {
+  if (err.code && SDAM_NOTMASTER_CODES.has(err.code)) {
+    return true;
+  }
+
+  if (isRecoveringError(err)) {
+    return false;
+  }
+
+  return err.message.match(/not master/);
+}
+
+function isNodeShuttingDownError(err) {
+  return SDAM_NODE_SHUTTING_DOWN_ERROR_CODES.has(err.code);
+}
+
 /**
- * Determines whether an error is a "node is recovering" error or
- * a "not master" error for SDAM retryability.
+ * Determines whether SDAM can recover from a given error. If it cannot
+ * then the pool will be cleared, and server state will completely reset
+ * locally.
  *
  * @see https://github.com/mongodb/specifications/blob/master/source/server-discovery-and-monitoring/server-discovery-and-monitoring.rst#not-master-and-node-is-recovering
  * @param {MongoError|Error} error
+ * @param {Server} server
  */
-function isSDAMUnrecoverableError(error) {
+function isSDAMUnrecoverableError(error, server) {
   return (
-    SDAM_UNRECOVERABLE_ERROR_CODES.has(error.code) ||
-    (error.message &&
-      (error.message.match(/not master/) || error.message.match(/node is recovering/)))
+    error instanceof MongoParseError ||
+    ((isRecoveringError(error) || isNotMasterError(error)) &&
+      (maxWireVersion(server) >= 8 && isNodeShuttingDownError(error)))
   );
 }
 

diff --git a/lib/core/sdam/server.js b/lib/core/sdam/server.js
@@ -245,7 +245,7 @@ class Server extends EventEmitter {
           options.session.serverSession.isDirty = true;
         }
 
-        if (isSDAMUnrecoverableError(err)) {
+        if (isSDAMUnrecoverableError(err, this)) {
           this.emit('error', err);
         }
       }
@@ -269,7 +269,7 @@ class Server extends EventEmitter {
           options.session.serverSession.isDirty = true;
         }
 
-        if (isSDAMUnrecoverableError(err)) {
+        if (isSDAMUnrecoverableError(err, this)) {
           this.emit('error', err);
         }
       }
@@ -293,7 +293,7 @@ class Server extends EventEmitter {
           options.session.serverSession.isDirty = true;
         }
 
-        if (isSDAMUnrecoverableError(err)) {
+        if (isSDAMUnrecoverableError(err, this)) {
           this.emit('error', err);
         }
       }
@@ -311,7 +311,7 @@ class Server extends EventEmitter {
    */
   killCursors(ns, cursorState, callback) {
     wireProtocol.killCursors(this, ns, cursorState, (err, result) => {
-      if (err && isSDAMUnrecoverableError(err)) {
+      if (err && isSDAMUnrecoverableError(err, this)) {
         this.emit('error', err);
       }
 
@@ -419,7 +419,7 @@ function executeWriteOperation(args, options, callback) {
   }
 
   if (collationNotSupported(server, options)) {
-    callback(new MongoError(`server ${this.name} does not support collation`));
+    callback(new MongoError(`server ${server.name} does not support collation`));
     return;
   }
 
@@ -429,7 +429,7 @@ function executeWriteOperation(args, options, callback) {
         options.session.serverSession.isDirty = true;
       }
 
-      if (isSDAMUnrecoverableError(err)) {
+      if (isSDAMUnrecoverableError(err, server)) {
         server.emit('error', err);
       }
     }

diff --git a/lib/core/sdam/topology.js b/lib/core/sdam/topology.js
@@ -18,7 +18,6 @@ const deprecate = require('util').deprecate;
 const BSON = require('../connection/utils').retrieveBSON();
 const createCompressionInfo = require('../topologies/shared').createCompressionInfo;
 const isRetryableError = require('../error').isRetryableError;
-const MongoParseError = require('../error').MongoParseError;
 const isSDAMUnrecoverableError = require('../error').isSDAMUnrecoverableError;
 const ClientSession = require('../sessions').ClientSession;
 const createClientInfo = require('../topologies/shared').createClientInfo;
@@ -938,7 +937,7 @@ function serverErrorEventHandler(server, topology) {
       new monitoring.ServerClosedEvent(topology.s.id, server.description.address)
     );
 
-    if (err instanceof MongoParseError || isSDAMUnrecoverableError(err)) {
+    if (isSDAMUnrecoverableError(err, server)) {
       resetServerState(server, err, { clearPool: true });
       return;
     }