From ee110ac6fdf775d6b399e63e6ecf3727faa9c5bb Mon Sep 17 00:00:00 2001 From: Dan Aprahamian Date: Tue, 13 Mar 2018 13:34:33 -0400 Subject: [PATCH] fix(comments): adding fixes for PR comments --- lib/topologies/topology_base.js | 6 ++---- test/functional/examples_tests.js | 11 +++++------ test/functional/find_tests.js | 5 +++++ test/functional/gridfs_stream_tests.js | 16 ++++++++++++++++ .../operation_changestream_example_tests.js | 17 +++++++++++++++++ test/functional/operation_example_tests.js | 6 ++++-- .../operation_generators_example_tests.js | 3 +++ .../operation_promises_example_tests.js | 3 +++ test/functional/sessions_tests.js | 2 +- 9 files changed, 56 insertions(+), 13 deletions(-) diff --git a/lib/topologies/topology_base.js b/lib/topologies/topology_base.js index c1654d1094..1e3fe827fa 100644 --- a/lib/topologies/topology_base.js +++ b/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 })); } diff --git a/test/functional/examples_tests.js b/test/functional/examples_tests.js index 2d023fec86..1577affbb4 100644 --- a/test/functional/examples_tests.js +++ b/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 }); diff --git a/test/functional/find_tests.js b/test/functional/find_tests.js index 1a968cf780..f60f62bbc2 100644 --- a/test/functional/find_tests.js +++ b/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(); diff --git a/test/functional/gridfs_stream_tests.js b/test/functional/gridfs_stream_tests.js index 51d5875a13..87a5856937 100644 --- a/test/functional/gridfs_stream_tests.js +++ b/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(); diff --git a/test/functional/operation_changestream_example_tests.js b/test/functional/operation_changestream_example_tests.js index f0f3e304de..ec1c9a5ded 100644 --- a/test/functional/operation_changestream_example_tests.js +++ b/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(); diff --git a/test/functional/operation_example_tests.js b/test/functional/operation_example_tests.js index 1e66240c80..192e05b649 100644 --- a/test/functional/operation_example_tests.js +++ b/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(); diff --git a/test/functional/operation_generators_example_tests.js b/test/functional/operation_generators_example_tests.js index 7bbf2e27a0..b0caf35018 100644 --- a/test/functional/operation_generators_example_tests.js +++ b/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(); }); diff --git a/test/functional/operation_promises_example_tests.js b/test/functional/operation_promises_example_tests.js index 00966bae2a..51b30b3cf8 100644 --- a/test/functional/operation_promises_example_tests.js +++ b/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(); }); diff --git a/test/functional/sessions_tests.js b/test/functional/sessions_tests.js index 86be797d79..57d57aac1f 100644 --- a/test/functional/sessions_tests.js +++ b/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);