fix(comments): adding fixes for PR comments

Author: daprahamian

lib/topologies/topology_base.js

@@ -394,11 +394,9 @@ class TopologyBase extends EventEmitter {
   }
 
   close(forceClosed) {
-    // If we have sessions, we want to send a single `endSessions` command for them,
-    // and then individually clean them up.  They will be removed from the internal state
-    // when they emit their `ended` events.
+    // If we have sessions, we want to individually move them to the session pool,
+    // and then send a single endSessions call.
     if (this.s.sessions.length) {
-      this.endSessions(this.s.sessions.map(session => session.id));
       this.s.sessions.forEach(session => session.endSession({ skipCommand: true }));
     }
 

test/functional/examples_tests.js

@@ -1230,18 +1230,17 @@ describe('Examples', function() {
    * @ignore
    */
   it('CausalConsistency', {
-    metadata: { requires: { topology: ['single'], mongodb: '>=3.6.0' } },
+    metadata: {
+      requires: { topology: ['single'], mongodb: '>=3.6.0' },
+      sessions: { skipLeakTests: true }
+    },
 
     test: function(done) {
       const configuration = this.configuration;
       const client = configuration.newClient(configuration.writeConcernMax(), { poolSize: 1 });
 
       client.connect(function(err, client) {
-        let session;
         const cleanup = e => {
-          if (session) {
-            session.endSession();
-          }
           client.close();
           done(e);
         };
@@ -1250,7 +1249,7 @@ describe('Examples', function() {
 
         const db = client.db(configuration.db);
         const collection = db.collection('causalConsistencyExample');
-        session = client.startSession({ causalConsistency: true });
+        const session = client.startSession({ causalConsistency: true });
 
         collection.insertOne({ darmok: 'jalad' }, { session });
         collection.updateOne({ darmok: 'jalad' }, { $set: { darmok: 'tanagra' } }, { session });

test/functional/find_tests.js

@@ -2733,6 +2733,8 @@ describe('Find', function() {
               cursors[0].next(function(err) {
                 test.equal(null, err);
 
+                // We need to close the cursor since it is not exhausted,
+                // and we need to end the implicit session
                 cursors[0].close();
                 client.close();
                 done();
@@ -2780,6 +2782,9 @@ describe('Find', function() {
 
             cursor.next(function(err) {
               test.ok(err !== null);
+
+              // We need to close the cursor since it is not exhausted,
+              // and we need to end the implicit session
               cursor.close();
               client.close();
               done();

test/functional/gridfs_stream_tests.js

@@ -1124,11 +1124,19 @@ describe('GridFS Stream', function() {
                 download.on('error', function(error) {
                   if (!testSpec.assert.error) {
                     test.ok(false);
+
+                    // We need to abort in order to close the underlying cursor,
+                    // and by extension the implicit session used for the cursor.
+                    // This is only necessary if the cursor is not exhausted
                     download.abort();
                     client.close();
                     done();
                   }
                   test.ok(error.toString().indexOf(testSpec.assert.error) !== -1);
+
+                  // We need to abort in order to close the underlying cursor,
+                  // and by extension the implicit session used for the cursor.
+                  // This is only necessary if the cursor is not exhausted
                   download.abort();
                   client.close();
                   done();
@@ -1138,12 +1146,20 @@ describe('GridFS Stream', function() {
                   var result = testSpec.assert.result;
                   if (!result) {
                     test.ok(false);
+
+                    // We need to abort in order to close the underlying cursor,
+                    // and by extension the implicit session used for the cursor.
+                    // This is only necessary if the cursor is not exhausted
                     download.abort();
                     client.close();
                     done();
                   }
 
                   test.equal(res.toString('hex'), result.$hex);
+
+                  // We need to abort in order to close the underlying cursor,
+                  // and by extension the implicit session used for the cursor.
+                  // This is only necessary if the cursor is not exhausted
                   download.abort();
                   client.close();
                   done();

test/functional/operation_changestream_example_tests.js

@@ -29,6 +29,9 @@ describe('Changestream Examples', function() {
             if (err) return console.log(err);
             expect(err).to.equal(null);
             expect(next).to.exist;
+
+            // Since changeStream has an implicit seession,
+            // we need to close the changeStream for unit testing purposes
             changeStream.close();
             client.close();
             done();
@@ -67,6 +70,9 @@ describe('Changestream Examples', function() {
         const changeStream = collection.watch();
         changeStream.on('change', function(change) {
           expect(change).to.exist;
+
+          // Since changeStream has an implicit seession,
+          // we need to close the changeStream for unit testing purposes
           changeStream.close();
           client.close();
           done();
@@ -102,6 +108,9 @@ describe('Changestream Examples', function() {
 
         changeStream.stream({ transform: JSON.stringify }).once('data', function(chunk) {
           expect(chunk).to.exist;
+
+          // Since changeStream has an implicit seession,
+          // we need to close the changeStream for unit testing purposes
           changeStream.close();
           client.close();
           done();
@@ -138,6 +147,9 @@ describe('Changestream Examples', function() {
         const changeStream = collection.watch({ fullDocument: 'updateLookup' });
         changeStream.on('change', function(change) {
           expect(change).to.exist;
+
+          // Since changeStream has an implicit seession,
+          // we need to close the changeStream for unit testing purposes
           changeStream.close();
           client.close();
           done();
@@ -196,6 +208,9 @@ describe('Changestream Examples', function() {
                 if (err) return console.log(err);
                 expect(err).to.equal(null);
                 expect(next).to.exist;
+
+                // Since changeStream has an implicit seession,
+                // we need to close the changeStream for unit testing purposes
                 newChangeStream.close();
                 client.close();
                 done();
@@ -245,6 +260,8 @@ describe('Changestream Examples', function() {
           expect(next.newField).to.exist;
           expect(next.newField).to.equal('this is an added field!');
 
+          // Since changeStream has an implicit seession,
+          // we need to close the changeStream for unit testing purposes
           changeStream.close();
           client.close();
           done();

test/functional/operation_example_tests.js

@@ -364,7 +364,8 @@ describe('Operation Examples', function() {
             test.ok(docs);
             test.equal(null, err);
 
-            // Need to close cursor since batchsize = 1
+            // Need to close cursor since cursor is not
+            // exhausted, and implicit session is still open
             cursor.close();
             client.close();
             done();
@@ -6097,7 +6098,8 @@ describe('Operation Examples', function() {
               test.equal(null, err);
               test.equal(1, item.a);
 
-              // Need to close cursor, since it was not exhausted
+              // Need to close cursor, since it was not exhausted,
+              // and implicit session is still open
               cursor.close();
               client.close();
               done();

test/functional/operation_generators_example_tests.js

@@ -172,6 +172,9 @@ describe('Operation (Generators)', function() {
         );
         // Get all the aggregation results
         yield cursor.next();
+
+        // Closing cursor to close implicit session,
+        // since the cursor is not exhausted
         cursor.close();
         client.close();
       });

test/functional/operation_promises_example_tests.js

@@ -182,6 +182,9 @@ describe('Operation (Promises)', function() {
           })
           .then(function(docs) {
             test.ok(docs);
+
+            // Need to close cursor to close implicit session,
+            // since cursor is not exhausted
             cursor.close();
             client.close();
           });

test/functional/sessions_tests.js

@@ -39,7 +39,7 @@ describe('Sessions', function() {
 
         client.close(err => {
           expect(err).to.not.exist;
-          expect(test.commands.started).to.have.length(2);
+          expect(test.commands.started).to.have.length(1);
           expect(test.commands.started[0].commandName).to.equal('endSessions');
           expect(test.commands.started[0].command.endSessions).to.include.deep.members(sessions);