From 9df093c1d46e1f8616c7a979324923205ac3dcd2 Mon Sep 17 00:00:00 2001 From: Matt Broadstone Date: Mon, 30 Nov 2020 08:48:06 -0500 Subject: [PATCH] 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 --- lib/core/topologies/replset.js | 17 +++++---- lib/core/topologies/server.js | 5 +-- .../core/rs_mocks/connection.test.js | 36 +++++++------------ 3 files changed, 26 insertions(+), 32 deletions(-) diff --git a/lib/core/topologies/replset.js b/lib/core/topologies/replset.js index deaa60fa7a..0be880f998 100644 --- 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 index ecf46135ca..c6a0bfa5db 100644 --- 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 index 34c4fb9a10..74a3f962be 100644 --- 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();