fix(sessions): ensure that we ignore session details from arbiters

Arbiters in a ReplSet should not be taken into consideration if they report a `logicalSessionTimeoutMinutes`. NODE-1188
mongodb-js · Dec 11, 2017 · de0105c · de0105c
1 parent 65dcca4
commit de0105c
Show file tree

Hide file tree

Showing 2 changed files with 54 additions and 34 deletions.
diff --git a/lib/topologies/replset_state.js b/lib/topologies/replset_state.js
@@ -200,6 +200,8 @@ ReplSetState.prototype.remove = function(server, options) {
   }
 };
 
+const isArbiter = ismaster => ismaster.arbiterOnly && ismaster.setName;
+
 ReplSetState.prototype.update = function(server) {
   var self = this;
   // Get the current ismaster
@@ -266,7 +268,7 @@ ReplSetState.prototype.update = function(server) {
   }
 
   // Update logicalSessionTimeoutMinutes
-  if (ismaster.logicalSessionTimeoutMinutes !== undefined) {
+  if (ismaster.logicalSessionTimeoutMinutes !== undefined && !isArbiter(ismaster)) {
     if (
       self.logicalSessionTimeoutMinutes === undefined ||
       ismaster.logicalSessionTimeoutMinutes === null
@@ -597,8 +599,7 @@ ReplSetState.prototype.update = function(server) {
   // Arbiter handling
   //
   if (
-    ismaster.arbiterOnly &&
-    ismaster.setName &&
+    isArbiter(ismaster) &&
     !inList(ismaster, server, this.arbiters) &&
     this.setName &&
     this.setName === ismaster.setName

diff --git a/test/tests/unit/replset/sessions_tests.js b/test/tests/unit/replset/sessions_tests.js
@@ -8,7 +8,7 @@ var expect = require('chai').expect,
 const test = new ReplSetFixture();
 describe('Sessions (ReplSet)', function() {
   afterEach(() => mock.cleanup());
-  beforeEach(() => test.setup());
+  beforeEach(() => test.setup({ ismaster: mock.DEFAULT_ISMASTER_36 }));
 
   it('should track the highest `clusterTime` seen in a replica set', {
     metadata: { requires: { topology: 'single' } },
@@ -31,14 +31,10 @@ describe('Sessions (ReplSet)', function() {
         }
       );
 
-      let serverCount = 0;
-      replset.on('joined', () => {
-        serverCount++;
-        if (serverCount === 3) {
-          expect(replset.clusterTime).to.eql(futureClusterTime);
-          replset.destroy();
-          done();
-        }
+      replset.on('all', () => {
+        expect(replset.clusterTime).to.eql(futureClusterTime);
+        replset.destroy();
+        done();
       });
 
       replset.on('error', done);
@@ -67,19 +63,15 @@ describe('Sessions (ReplSet)', function() {
         }
       );
 
-      let serverCount = 0;
-      replset.on('joined', () => {
-        serverCount++;
-        if (serverCount === 3) {
-          expect(replset.clusterTime).to.eql(futureClusterTime);
-          const servers = replset.s.replicaSetState.secondaries
-            .concat(replset.s.replicaSetState.arbiters)
-            .concat([replset.s.replicaSetState.primary]);
-          servers.forEach(server => expect(server.clusterTime).to.eql(futureClusterTime));
+      replset.on('all', () => {
+        expect(replset.clusterTime).to.eql(futureClusterTime);
+        const servers = replset.s.replicaSetState.secondaries
+          .concat(replset.s.replicaSetState.arbiters)
+          .concat([replset.s.replicaSetState.primary]);
+        servers.forEach(server => expect(server.clusterTime).to.eql(futureClusterTime));
 
-          replset.destroy();
-          done();
-        }
+        replset.destroy();
+        done();
       });
 
       replset.on('error', done);
@@ -90,6 +82,8 @@ describe('Sessions (ReplSet)', function() {
   it('should set `logicalSessionTimeoutMinutes` to `null` if any incoming server is `null`', {
     metadata: { requires: { topology: 'single' } },
     test: function(done) {
+      test.firstSecondaryStates[0].logicalSessionTimeoutMinutes = null;
+
       const replset = new ReplSet(
         [test.primaryServer.address(), test.firstSecondaryServer.address()],
         {
@@ -102,7 +96,7 @@ describe('Sessions (ReplSet)', function() {
       );
 
       replset.on('error', done);
-      replset.once('connect', () => {
+      replset.once('all', () => {
         expect(replset.logicalSessionTimeoutMinutes).to.equal(null);
         replset.destroy();
         done();
@@ -132,20 +126,45 @@ describe('Sessions (ReplSet)', function() {
           }
         );
 
-        let joinCount = 0;
-        replset.on('joined', () => {
-          joinCount++;
-
-          if (joinCount === 3) {
-            expect(replset.logicalSessionTimeoutMinutes).to.equal(1);
-            replset.destroy();
-            done();
-          }
+        replset.on('all', () => {
+          expect(replset.logicalSessionTimeoutMinutes).to.equal(1);
+          replset.destroy();
+          done();
         });
 
         replset.on('error', done);
         replset.connect();
       }
     }
   );
+
+  it('should exclude arbiters when tracking `logicalSessionTimeoutMinutes`', {
+    metadata: { requires: { topology: 'single' } },
+    test: function(done) {
+      test.arbiterServer.setMessageHandler(req => {
+        const doc = req.document;
+        if (doc.ismaster) {
+          req.reply(Object.assign({}, test.arbiterStates[0], { logicalSessionTimeoutMinutes: 2 }));
+        }
+      });
+
+      const replset = new ReplSet(test.servers.map(s => s.address()), {
+        setName: 'rs',
+        connectionTimeout: 3000,
+        socketTimeout: 0,
+        haInterval: 100,
+        size: 1
+      });
+
+      replset.on('joined', type => {
+        if (type === 'arbiter') {
+          expect(replset.logicalSessionTimeoutMinutes).to.equal(10);
+          done();
+        }
+      });
+
+      replset.on('error', done);
+      replset.connect();
+    }
+  });
 });