From cb14ee3817516fc47cd562cae509bc1091b62d3b Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 16 Feb 2021 13:59:21 -0500 Subject: [PATCH 1/5] fix: move session support check to operation layer Monitor checks run on a timer and network errors cause a reset of the topology that would be corrected in the next cycle of the monitor, we were checking a property that gets updated by this async work. By moving the check to the operations layer we will allow users to obtain a session regardless of support and then emit an error if there is not support when the session is used in an operation. NODE-3100 --- lib/mongo_client.js | 4 --- lib/operations/execute_operation.js | 2 ++ test/unit/sessions/client.test.js | 48 ++++++++++++++++++++--------- 3 files changed, 36 insertions(+), 18 deletions(-) diff --git a/lib/mongo_client.js b/lib/mongo_client.js index cc8c8a39ec2..7ace024256a 100644 --- a/lib/mongo_client.js +++ b/lib/mongo_client.js @@ -457,10 +457,6 @@ MongoClient.prototype.startSession = function(options) { throw new MongoError('Must connect to a server before calling this method'); } - if (!this.topology.hasSessionSupport()) { - throw new MongoError('Current topology does not support sessions'); - } - return this.topology.startSession(options, this.s.options); }; diff --git a/lib/operations/execute_operation.js b/lib/operations/execute_operation.js index 80d57857e86..10432e11a72 100644 --- a/lib/operations/execute_operation.js +++ b/lib/operations/execute_operation.js @@ -47,6 +47,8 @@ function executeOperation(topology, operation, callback) { } else if (operation.session.hasEnded) { throw new MongoError('Use of expired sessions is not permitted'); } + } else if (operation.session && operation.session.explicit) { + throw new MongoError('Current topology does not support sessions'); } let result; diff --git a/test/unit/sessions/client.test.js b/test/unit/sessions/client.test.js index 7d9005a816c..a55c1eededd 100644 --- a/test/unit/sessions/client.test.js +++ b/test/unit/sessions/client.test.js @@ -16,7 +16,7 @@ describe('Sessions', function() { it('should throw an exception if sessions are not supported', { metadata: { requires: { topology: 'single' } }, - test: function(done) { + test() { test.server.setMessageHandler(request => { var doc = request.document; if (doc.ismaster) { @@ -27,14 +27,24 @@ describe('Sessions', function() { }); const client = this.configuration.newClient(`mongodb://${test.server.uri()}/test`); - client.connect(function(err, client) { - expect(err).to.not.exist; - expect(() => { - client.startSession(); - }).to.throw(/Current topology does not support sessions/); - - client.close(done); - }); + return client + .connect() + .then(function(client) { + const session = client.startSession(); + return client + .db() + .collection('t') + .insertOne({ a: 1 }, { session }); + }) + .then(() => { + expect.fail('Expected an error to be thrown about not supporting sessions'); + }) + .catch(error => { + expect(error.message).to.equal('Current topology does not support sessions'); + }) + .then(() => { + return client.close(); + }); } }); @@ -42,6 +52,7 @@ describe('Sessions', function() { metadata: { requires: { topology: 'single' } }, test() { const replicaSetMock = new ReplSetFixture(); + let client; return replicaSetMock .setup({ doNotInitHandlers: true }) .then(() => { @@ -92,14 +103,23 @@ describe('Sessions', function() { return replicaSetMock.uri(); }) .then(uri => { - const client = this.configuration.newClient(uri); + client = this.configuration.newClient(uri); return client.connect(); }) .then(client => { - expect(client.topology.s.description.logicalSessionTimeoutMinutes).to.not.exist; - expect(() => { - client.startSession(); - }).to.throw(/Current topology does not support sessions/); + const session = client.startSession(); + return client + .db() + .collection('t') + .insertOne({ a: 1 }, { session }); + }) + .then(() => { + expect.fail('Expected an error to be thrown about not supporting sessions'); + }) + .catch(error => { + expect(error.message).to.equal('Current topology does not support sessions'); + }) + .then(() => { return client.close(); }); } From 8b4e69543f1a63dc39e3dc9f64c5f0cb3ca92aa2 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 17 Feb 2021 14:25:23 -0500 Subject: [PATCH 2/5] Move session support check into server selection for unified topology --- lib/operations/execute_operation.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/operations/execute_operation.js b/lib/operations/execute_operation.js index 10432e11a72..f7fb0f53be3 100644 --- a/lib/operations/execute_operation.js +++ b/lib/operations/execute_operation.js @@ -142,6 +142,10 @@ function executeWithServerSelection(topology, operation, callback) { return; } + if (serverSelectionOptions.session && topology.hasSessionSupport()) { + return callback(new MongoError('Current topology does not support sessions')); + } + const shouldRetryReads = topology.s.options.retryReads !== false && operation.session && From 8a0e3ef961aea8789364cfb6580206aebd23d48f Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 3 Mar 2021 12:35:18 -0500 Subject: [PATCH 3/5] fix: update to simple fix --- lib/operations/execute_operation.js | 66 ++++++++++++----------------- 1 file changed, 27 insertions(+), 39 deletions(-) diff --git a/lib/operations/execute_operation.js b/lib/operations/execute_operation.js index f7fb0f53be3..ca390833664 100644 --- a/lib/operations/execute_operation.js +++ b/lib/operations/execute_operation.js @@ -30,11 +30,25 @@ function executeOperation(topology, operation, callback) { throw new TypeError('This method requires a valid operation instance'); } - if (isUnifiedTopology(topology) && topology.shouldCheckForSessionSupport()) { - return selectServerForSessionSupport(topology, operation, callback); + const Promise = topology.s.promiseLibrary; + + let result; + if (typeof callback !== 'function') { + result = new Promise((resolve, reject) => { + callback = (err, res) => { + if (err) return reject(err); + resolve(res); + }; + }); } - const Promise = topology.s.promiseLibrary; + if (isUnifiedTopology(topology) && topology.shouldCheckForSessionSupport()) { + // Recursive call to executeOperation after a server selection + // shouldCheckForSessionSupport should return false after a server selection because + // there will be data bearing servers + selectServerForSessionSupport(topology, operation, callback); + return result; + } // The driver sessions spec mandates that we implicitly create sessions for operations // that are not explicitly provided with a session. @@ -45,20 +59,14 @@ function executeOperation(topology, operation, callback) { session = topology.startSession({ owner }); operation.session = session; } else if (operation.session.hasEnded) { - throw new MongoError('Use of expired sessions is not permitted'); + callback(new MongoError('Use of expired sessions is not permitted')); + return result; } - } else if (operation.session && operation.session.explicit) { - throw new MongoError('Current topology does not support sessions'); - } - - let result; - if (typeof callback !== 'function') { - result = new Promise((resolve, reject) => { - callback = (err, res) => { - if (err) return reject(err); - resolve(res); - }; - }); + } else if (operation.session && !topology.hasSessionSupport()) { + // If the user passed an explicit session and we are still, after server selection, + // trying to run against a topology that doesn't support sessions we error out. + callback(new MongoError('Current topology does not support sessions')); + return result; } function executeCallback(err, result) { @@ -78,7 +86,7 @@ function executeOperation(topology, operation, callback) { } else { operation.execute(executeCallback); } - } catch (e) { + } catch (error) { if (session && session.owner === owner) { session.endSession(); if (operation.session === session) { @@ -86,7 +94,7 @@ function executeOperation(topology, operation, callback) { } } - throw e; + callback(error); } return result; @@ -141,11 +149,6 @@ function executeWithServerSelection(topology, operation, callback) { callback(err, null); return; } - - if (serverSelectionOptions.session && topology.hasSessionSupport()) { - return callback(new MongoError('Current topology does not support sessions')); - } - const shouldRetryReads = topology.s.options.retryReads !== false && operation.session && @@ -165,28 +168,13 @@ function executeWithServerSelection(topology, operation, callback) { // TODO: This is only supported for unified topology, it should go away once // we remove support for legacy topology types. function selectServerForSessionSupport(topology, operation, callback) { - const Promise = topology.s.promiseLibrary; - - let result; - if (typeof callback !== 'function') { - result = new Promise((resolve, reject) => { - callback = (err, result) => { - if (err) return reject(err); - resolve(result); - }; - }); - } - topology.selectServer(ReadPreference.primaryPreferred, err => { if (err) { - callback(err); - return; + return callback(err); } executeOperation(topology, operation, callback); }); - - return result; } module.exports = executeOperation; From f621a498caea16014b2251aa6619fc37505447a5 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 9 Mar 2021 12:27:12 -0500 Subject: [PATCH 4/5] refactor to use maybePromise, clean up comments and organize control flow --- lib/operations/execute_operation.js | 108 ++++++++++++---------------- test/unit/sessions/client.test.js | 26 ++----- 2 files changed, 53 insertions(+), 81 deletions(-) diff --git a/lib/operations/execute_operation.js b/lib/operations/execute_operation.js index ca390833664..c2255980d15 100644 --- a/lib/operations/execute_operation.js +++ b/lib/operations/execute_operation.js @@ -1,5 +1,6 @@ 'use strict'; +const maybePromise = require('../utils').maybePromise; const MongoError = require('../core/error').MongoError; const Aspect = require('./operation').Aspect; const OperationBase = require('./operation').OperationBase; @@ -21,7 +22,7 @@ const isUnifiedTopology = require('../core/utils').isUnifiedTopology; * @param {Operation} operation The operation to execute * @param {function} callback The command result callback */ -function executeOperation(topology, operation, callback) { +function executeOperation(topology, operation, cb) { if (topology == null) { throw new TypeError('This method requires a valid topology instance'); } @@ -30,74 +31,57 @@ function executeOperation(topology, operation, callback) { throw new TypeError('This method requires a valid operation instance'); } - const Promise = topology.s.promiseLibrary; - - let result; - if (typeof callback !== 'function') { - result = new Promise((resolve, reject) => { - callback = (err, res) => { - if (err) return reject(err); - resolve(res); - }; - }); - } - - if (isUnifiedTopology(topology) && topology.shouldCheckForSessionSupport()) { - // Recursive call to executeOperation after a server selection - // shouldCheckForSessionSupport should return false after a server selection because - // there will be data bearing servers - selectServerForSessionSupport(topology, operation, callback); - return result; - } - - // The driver sessions spec mandates that we implicitly create sessions for operations - // that are not explicitly provided with a session. - let session, owner; - if (topology.hasSessionSupport()) { - if (operation.session == null) { - owner = Symbol(); - session = topology.startSession({ owner }); - operation.session = session; - } else if (operation.session.hasEnded) { - callback(new MongoError('Use of expired sessions is not permitted')); - return result; + return maybePromise(topology, cb, callback => { + if (isUnifiedTopology(topology) && topology.shouldCheckForSessionSupport()) { + // Recursive call to executeOperation after a server selection + return selectServerForSessionSupport(topology, operation, callback); } - } else if (operation.session && !topology.hasSessionSupport()) { - // If the user passed an explicit session and we are still, after server selection, - // trying to run against a topology that doesn't support sessions we error out. - callback(new MongoError('Current topology does not support sessions')); - return result; - } - function executeCallback(err, result) { - if (session && session.owner === owner) { - session.endSession(); - if (operation.session === session) { - operation.clearSession(); + // The driver sessions spec mandates that we implicitly create sessions for operations + // that are not explicitly provided with a session. + let session, owner; + if (topology.hasSessionSupport()) { + if (operation.session == null) { + owner = Symbol(); + session = topology.startSession({ owner }); + operation.session = session; + } else if (operation.session.hasEnded) { + return callback(new MongoError('Use of expired sessions is not permitted')); } + } else if (operation.session) { + // If the user passed an explicit session and we are still, after server selection, + // trying to run against a topology that doesn't support sessions we error out. + return callback(new MongoError('Current topology does not support sessions')); } - callback(err, result); - } - - try { - if (operation.hasAspect(Aspect.EXECUTE_WITH_SELECTION)) { - executeWithServerSelection(topology, operation, executeCallback); - } else { - operation.execute(executeCallback); - } - } catch (error) { - if (session && session.owner === owner) { - session.endSession(); - if (operation.session === session) { - operation.clearSession(); + function executeCallback(err, result) { + if (session && session.owner === owner) { + session.endSession(); + if (operation.session === session) { + operation.clearSession(); + } } + + callback(err, result); } - callback(error); - } + try { + if (operation.hasAspect(Aspect.EXECUTE_WITH_SELECTION)) { + executeWithServerSelection(topology, operation, executeCallback); + } else { + operation.execute(executeCallback); + } + } catch (error) { + if (session && session.owner === owner) { + session.endSession(); + if (operation.session === session) { + operation.clearSession(); + } + } - return result; + callback(error); + } + }); } function supportsRetryableReads(server) { @@ -165,8 +149,8 @@ function executeWithServerSelection(topology, operation, callback) { }); } -// TODO: This is only supported for unified topology, it should go away once -// we remove support for legacy topology types. +// The Unified Topology runs serverSelection before executing every operation +// Session support is determined by the result of a monitoring check triggered by this selection function selectServerForSessionSupport(topology, operation, callback) { topology.selectServer(ReadPreference.primaryPreferred, err => { if (err) { diff --git a/test/unit/sessions/client.test.js b/test/unit/sessions/client.test.js index a55c1eededd..faca91fe874 100644 --- a/test/unit/sessions/client.test.js +++ b/test/unit/sessions/client.test.js @@ -14,7 +14,7 @@ describe('Sessions', function() { }); }); - it('should throw an exception if sessions are not supported', { + it('should not throw a synchronous exception if sessions are not supported', { metadata: { requires: { topology: 'single' } }, test() { test.server.setMessageHandler(request => { @@ -27,24 +27,12 @@ describe('Sessions', function() { }); const client = this.configuration.newClient(`mongodb://${test.server.uri()}/test`); - return client - .connect() - .then(function(client) { - const session = client.startSession(); - return client - .db() - .collection('t') - .insertOne({ a: 1 }, { session }); - }) - .then(() => { - expect.fail('Expected an error to be thrown about not supporting sessions'); - }) - .catch(error => { - expect(error.message).to.equal('Current topology does not support sessions'); - }) - .then(() => { - return client.close(); - }); + return client.connect().then(() => { + expect(() => client.startSession()).to.not.throw( + 'Current topology does not support sessions' + ); + return client.close(); + }); } }); From b22fe8731a2aa28c1ae7655f4db3ab1413697443 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 9 Mar 2021 14:53:35 -0500 Subject: [PATCH 5/5] fix: maybePromise refactor passes undefined to callbacks --- test/functional/find.test.js | 4 ++-- test/functional/insert.test.js | 10 +++++----- test/functional/operation_example.test.js | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/test/functional/find.test.js b/test/functional/find.test.js index 08454c63dd7..da8d4c95640 100644 --- a/test/functional/find.test.js +++ b/test/functional/find.test.js @@ -1067,7 +1067,7 @@ describe('Find', function() { { $set: { name: 'test2' } }, {}, function(err, updated_doc) { - test.equal(null, updated_doc); + expect(updated_doc).to.not.exist; test.ok(err != null); client.close(done); } @@ -1305,7 +1305,7 @@ describe('Find', function() { { a: 10, b: 10, failIndex: 2 }, { w: 1, upsert: true }, function(err, result) { - test.equal(null, result); + expect(result).to.not.exist; test.ok(err.errmsg.match('duplicate key')); client.close(done); } diff --git a/test/functional/insert.test.js b/test/functional/insert.test.js index d560ea77f34..2bd933f39a7 100644 --- a/test/functional/insert.test.js +++ b/test/functional/insert.test.js @@ -1097,8 +1097,8 @@ describe('Insert', function() { var db = client.db(configuration.db); var collection = db.collection('Should_fail_on_insert_due_to_key_starting_with'); collection.insert(doc, configuration.writeConcernMax(), function(err, result) { - test.ok(err != null); - test.equal(null, result); + expect(err).to.exist; + expect(result).to.not.exist; client.close(done); }); @@ -1348,7 +1348,7 @@ describe('Insert', function() { // Update two fields collection.insert({ _id: 1 }, configuration.writeConcernMax(), function(err, r) { - test.equal(r, null); + expect(r).to.not.exist; test.ok(err != null); test.ok(err.result); @@ -2560,7 +2560,7 @@ describe('Insert', function() { [{ a: 1 }, { a: 2 }, { a: 1 }, { a: 3 }, { a: 1 }], { ordered: true }, function(err, r) { - test.equal(r, null); + expect(r).to.not.exist; test.ok(err != null); test.ok(err.result); @@ -2601,7 +2601,7 @@ describe('Insert', function() { [{ a: 1 }, { a: 2 }, { a: 1 }, { a: 3 }, { a: 1 }], { ordered: true }, function(err, r) { - test.equal(r, null); + expect(r).to.not.exist; test.ok(err != null); test.ok(err.result); diff --git a/test/functional/operation_example.test.js b/test/functional/operation_example.test.js index 189e7ee069a..34e87c5ddf4 100644 --- a/test/functional/operation_example.test.js +++ b/test/functional/operation_example.test.js @@ -2657,7 +2657,7 @@ describe('Operation Examples', function() { ], { w: 1, keepGoing: true }, function(err, result) { - test.equal(result, null); + expect(result).to.not.exist; test.ok(err); test.ok(err.result); @@ -3115,7 +3115,7 @@ describe('Operation Examples', function() { // Attemp to rename the first collection to the second one, this will fail collection1.rename('test_rename_collection2', function(err, collection) { - test.equal(null, collection); + expect(collection).to.not.exist; test.ok(err instanceof Error); test.ok(err.message.length > 0);