fix: transition topology state before async calls (#2637)

When the legacy ReplSet and Server topology types are destroyed they transition their internal state after a potentially long async close process has completed. With the recently introduced infinite default socket timeout, we are hitting this race condition more consistently. This patch ensures the types transition their state immediately, reducing the chance that such race conditions can occur. NODE-2916
mongodb · Nov 30, 2020 · 9df093c · 9df093c
1 parent dd356f0
commit 9df093c
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 32 deletions.
diff --git a/lib/core/topologies/replset.js b/lib/core/topologies/replset.js
@@ -541,7 +541,7 @@ var monitorServer = function(host, self, options) {
               self.s.options.secondaryOnlyConnectionAllowed) ||
               self.s.replicaSetState.hasPrimary())
           ) {
-            self.state = CONNECTED;
+            stateTransition(self, CONNECTED);
 
             // Emit connected sign
             process.nextTick(function() {
@@ -558,7 +558,7 @@ var monitorServer = function(host, self, options) {
               self.s.options.secondaryOnlyConnectionAllowed) ||
               self.s.replicaSetState.hasPrimary())
           ) {
-            self.state = CONNECTED;
+            stateTransition(self, CONNECTED);
 
             // Rexecute any stalled operation
             rexecuteOperations(self);
@@ -787,7 +787,7 @@ function handleInitialConnectEvent(self, event) {
         // Do we have a primary or primaryAndSecondary
         if (shouldTriggerConnect(self)) {
           // We are connected
-          self.state = CONNECTED;
+          stateTransition(self, CONNECTED);
 
           // Set initial connect state
           self.initialConnectState.connect = true;
@@ -975,14 +975,19 @@ ReplSet.prototype.destroy = function(options, callback) {
     // Emit toplogy closing event
     emitSDAMEvent(this, 'topologyClosed', { topologyId: this.id });
 
-    // Transition state
-    stateTransition(this, DESTROYED);
-
     if (typeof callback === 'function') {
       callback(null, null);
     }
   };
 
+  if (this.state === DESTROYED) {
+    if (typeof callback === 'function') callback(null, null);
+    return;
+  }
+
+  // Transition state
+  stateTransition(this, DESTROYED);
+
   // Clear out any monitoring process
   if (this.haTimeoutId) clearTimeout(this.haTimeoutId);
 

diff --git a/lib/core/topologies/server.js b/lib/core/topologies/server.js
@@ -864,12 +864,14 @@ Server.prototype.destroy = function(options, callback) {
   }
 
   // No pool, return
-  if (!self.s.pool) {
+  if (!self.s.pool || this._destroyed) {
     this._destroyed = true;
     if (typeof callback === 'function') callback(null, null);
     return;
   }
 
+  this._destroyed = true;
+
   // Emit close event
   if (options.emitClose) {
     self.emit('close', self);
@@ -900,7 +902,6 @@ Server.prototype.destroy = function(options, callback) {
 
   // Destroy the pool
   this.s.pool.destroy(options.force, callback);
-  this._destroyed = true;
 };
 
 /**

diff --git a/test/functional/core/rs_mocks/connection.test.js b/test/functional/core/rs_mocks/connection.test.js
@@ -135,8 +135,7 @@ describe('ReplSet Connection Tests (mocks)', function() {
               expect(server.s.replicaSetState.primary).to.not.be.null;
               expect(server.s.replicaSetState.primary.name).to.equal('localhost:32000');
 
-              server.destroy();
-              done();
+              server.destroy(done);
             }
           }
         });
@@ -251,8 +250,7 @@ describe('ReplSet Connection Tests (mocks)', function() {
                 expect(server.s.replicaSetState.primary).to.not.be.null;
                 expect(server.s.replicaSetState.primary.name).to.equal('localhost:32000');
 
-                server.destroy();
-                done();
+                server.destroy(done);
               }
             }
           });
@@ -351,8 +349,7 @@ describe('ReplSet Connection Tests (mocks)', function() {
           expect(server.s.replicaSetState.primary).to.not.be.null;
           expect(server.s.replicaSetState.primary.name).to.equal('localhost:32000');
 
-          server.destroy();
-          done();
+          server.destroy(done);
         }
 
         // Joined
@@ -428,8 +425,7 @@ describe('ReplSet Connection Tests (mocks)', function() {
         );
 
         server.on('error', function() {
-          server.destroy();
-          done();
+          server.destroy(done);
         });
 
         server.connect();
@@ -528,8 +524,7 @@ describe('ReplSet Connection Tests (mocks)', function() {
 
               expect(server.s.replicaSetState.primary).to.be.null;
 
-              server.destroy();
-              done();
+              server.destroy(done);
             }
           });
 
@@ -652,8 +647,7 @@ describe('ReplSet Connection Tests (mocks)', function() {
                 expect(server.s.replicaSetState.primary).to.not.be.null;
                 expect(server.s.replicaSetState.primary.name).to.equal('localhost:32000');
 
-                server.destroy();
-                done();
+                server.destroy(done);
               }
             }
           });
@@ -759,8 +753,7 @@ describe('ReplSet Connection Tests (mocks)', function() {
         );
 
         server.on('error', function() {
-          server.destroy();
-          done();
+          server.destroy(done);
         });
 
         // Gives proxies a chance to boot up
@@ -857,8 +850,7 @@ describe('ReplSet Connection Tests (mocks)', function() {
               expect(server.s.replicaSetState.primary).to.not.be.null;
               expect(server.s.replicaSetState.primary.name).to.equal('localhost:32000');
 
-              server.destroy();
-              done();
+              server.destroy(done);
             }
           }
         });
@@ -979,8 +971,7 @@ describe('ReplSet Connection Tests (mocks)', function() {
                 expect(server.s.replicaSetState.primary).to.not.be.null;
                 expect(server.s.replicaSetState.primary.name).to.equal('localhost:32000');
 
-                server.destroy();
-                done();
+                server.destroy(done);
               }
             }
           });
@@ -1070,8 +1061,7 @@ describe('ReplSet Connection Tests (mocks)', function() {
               expect(server.s.replicaSetState.primary).to.not.be.null;
               expect(server.s.replicaSetState.primary.name).to.equal('localhost:32000');
 
-              server.destroy();
-              done();
+              server.destroy(done);
             }
           }
         });
@@ -1182,8 +1172,7 @@ describe('ReplSet Connection Tests (mocks)', function() {
             expect(server.__connected).to.be.true;
             expect(server.__fullsetup).to.be.true;
 
-            server.destroy();
-            done();
+            server.destroy(done);
           });
 
           server.connect();
@@ -1274,8 +1263,7 @@ describe('ReplSet Connection Tests (mocks)', function() {
             var result = server.lastIsMaster();
             expect(result).to.exist;
 
-            server.destroy();
-            done();
+            server.destroy(done);
           });
 
           server.connect();