From a0dfc5bfaa097446999cd54ca5a979c0c7544657 Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Thu, 24 Feb 2022 19:00:33 +0100 Subject: [PATCH 01/12] fix(NODE-3521): remove session support checks --- src/cmap/connection.ts | 8 +- src/cursor/abstract_cursor.ts | 2 +- src/operations/execute_operation.ts | 20 ++--- src/sdam/topology.ts | 7 -- src/utils.ts | 2 +- test/unit/assorted/sessions_client.test.js | 91 +--------------------- test/unit/cmap/connection.test.js | 48 +----------- 7 files changed, 12 insertions(+), 166 deletions(-) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index c8feb308984..0987a19c3c0 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -402,7 +402,7 @@ export class Connection extends TypedEventEmitter { if (deprecationErrors != null) finalCmd.apiDeprecationErrors = deprecationErrors; } - if (hasSessionSupport(this) && session) { + if (session) { if ( session.clusterTime && clusterTime && @@ -683,12 +683,6 @@ export class CryptoConnection extends Connection { } } -/** @internal */ -export function hasSessionSupport(conn: Connection): boolean { - const description = conn.description; - return description.logicalSessionTimeoutMinutes != null || !!description.loadBalanced; -} - function supportsOpMsg(conn: Connection) { const description = conn.description; if (description == null) { diff --git a/src/cursor/abstract_cursor.ts b/src/cursor/abstract_cursor.ts index d729dc38a8f..1d0e5423ca9 100644 --- a/src/cursor/abstract_cursor.ts +++ b/src/cursor/abstract_cursor.ts @@ -653,7 +653,7 @@ function next(cursor: AbstractCursor, blocking: boolean, callback: Callback { return !this.description.hasDataBearingServers; } - /** - * @returns Whether sessions are supported on the current topology - */ - hasSessionSupport(): boolean { - return this.loadBalanced || this.description.logicalSessionTimeoutMinutes != null; - } - /** Start a logical session */ startSession(options: ClientSessionOptions, clientOptions?: MongoOptions): ClientSession { const session = new ClientSession(this, this.s.sessionPool, options, clientOptions); diff --git a/src/utils.ts b/src/utils.ts index ac787252f62..612d573ea64 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -229,7 +229,7 @@ export function executeLegacyOperation( let session: ClientSession; let opOptions: any; let owner: any; - if (!options.skipSessions && topology.hasSessionSupport()) { + if (!options.skipSessions) { opOptions = args[args.length - 2]; if (opOptions == null || opOptions.session == null) { owner = Symbol(); diff --git a/test/unit/assorted/sessions_client.test.js b/test/unit/assorted/sessions_client.test.js index 75e0e8c4ca4..367539826e9 100644 --- a/test/unit/assorted/sessions_client.test.js +++ b/test/unit/assorted/sessions_client.test.js @@ -2,7 +2,6 @@ const expect = require('chai').expect; const mock = require('../../tools/mongodb-mock/index'); -const { ReplSetFixture } = require('../../tools/common'); const { isHello } = require('../../../src/utils'); const { MongoClient } = require('../../../src'); @@ -16,95 +15,7 @@ describe('Sessions - client/unit', function () { }); }); - it('should not throw a synchronous exception if sessions are not supported', function () { - test.server.setMessageHandler(request => { - var doc = request.document; - if (isHello(doc)) { - request.reply(Object.assign({}, mock.HELLO)); - } else if (doc.endSessions) { - request.reply({ ok: 1 }); - } - }); - - const client = new MongoClient(`mongodb://${test.server.uri()}/test`); - return client.connect().then(() => { - expect(() => client.startSession()).to.not.throw( - 'Current topology does not support sessions' - ); - return client.close(); - }); - }); - - it('should throw an exception if sessions are not supported on some servers', function () { - const replicaSetMock = new ReplSetFixture(); - let testClient; - return replicaSetMock - .setup({ doNotInitHandlers: true }) - .then(() => { - replicaSetMock.firstSecondaryServer.setMessageHandler(request => { - var doc = request.document; - if (isHello(doc)) { - const hello = replicaSetMock.firstSecondaryStates[0]; - hello.logicalSessionTimeoutMinutes = 20; - request.reply(hello); - } else if (doc.endSessions) { - request.reply({ ok: 1 }); - } - }); - - replicaSetMock.secondSecondaryServer.setMessageHandler(request => { - var doc = request.document; - if (isHello(doc)) { - const hello = replicaSetMock.secondSecondaryStates[0]; - hello.logicalSessionTimeoutMinutes = 10; - request.reply(hello); - } else if (doc.endSessions) { - request.reply({ ok: 1 }); - } - }); - - replicaSetMock.arbiterServer.setMessageHandler(request => { - var doc = request.document; - if (isHello(doc)) { - const hello = replicaSetMock.arbiterStates[0]; - hello.logicalSessionTimeoutMinutes = 30; - request.reply(hello); - } else if (doc.endSessions) { - request.reply({ ok: 1 }); - } - }); - - replicaSetMock.primaryServer.setMessageHandler(request => { - var doc = request.document; - if (isHello(doc)) { - const hello = replicaSetMock.primaryStates[0]; - hello.logicalSessionTimeoutMinutes = null; - request.reply(hello); - } else if (doc.endSessions) { - request.reply({ ok: 1 }); - } - }); - - return replicaSetMock.uri(); - }) - .then(uri => { - testClient = new MongoClient(uri); - return testClient.connect(); - }) - .then(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'); - }) - .finally(() => (testClient ? testClient.close() : null)); - }); - - it('should return a client session when requested if the topology supports it', function (done) { + it('should return a client session when requested', function (done) { test.server.setMessageHandler(request => { var doc = request.document; if (isHello(doc)) { diff --git a/test/unit/cmap/connection.test.js b/test/unit/cmap/connection.test.js index 4b2a1fa09e3..5fdf9d3bfab 100644 --- a/test/unit/cmap/connection.test.js +++ b/test/unit/cmap/connection.test.js @@ -2,9 +2,8 @@ const mock = require('../../tools/mongodb-mock/index'); const { connect } = require('../../../src/cmap/connect'); -const { Connection, hasSessionSupport } = require('../../../src/cmap/connection'); +const { Connection } = require('../../../src/cmap/connection'); const { expect } = require('chai'); -const { Socket } = require('net'); const { ns, isHello } = require('../../../src/utils'); const { getSymbolFrom } = require('../../tools/utils'); @@ -107,49 +106,4 @@ describe('Connection - unit/cmap', function () { done(); }); }); - - describe('.hasSessionSupport', function () { - let connection; - const stream = new Socket(); - - context('when logicalSessionTimeoutMinutes is present', function () { - beforeEach(function () { - connection = new Connection(stream, { - hostAddress: server.hostAddress(), - logicalSessionTimeoutMinutes: 5 - }); - }); - - it('returns true', function () { - expect(hasSessionSupport(connection)).to.be.true; - }); - }); - - context('when logicalSessionTimeoutMinutes is not present', function () { - context('when in load balancing mode', function () { - beforeEach(function () { - connection = new Connection(stream, { - hostAddress: server.hostAddress(), - loadBalanced: true - }); - }); - - it('returns true', function () { - expect(hasSessionSupport(connection)).to.be.true; - }); - }); - - context('when not in load balancing mode', function () { - beforeEach(function () { - connection = new Connection(stream, { - hostAddress: server.hostAddress() - }); - }); - - it('returns false', function () { - expect(hasSessionSupport(connection)).to.be.false; - }); - }); - }); - }); }); From 0f8cd6e4aa791aab65689e3fe8bc8768c06c3ddb Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Fri, 25 Feb 2022 01:41:43 +0100 Subject: [PATCH 02/12] fix(NODE-3521): remove extra server selection --- src/operations/execute_operation.ts | 8 --- src/sdam/topology.ts | 11 ----- test/unit/sdam/topology.test.js | 75 ----------------------------- 3 files changed, 94 deletions(-) diff --git a/src/operations/execute_operation.ts b/src/operations/execute_operation.ts index 00321c119d9..0565d344569 100644 --- a/src/operations/execute_operation.ts +++ b/src/operations/execute_operation.ts @@ -77,14 +77,6 @@ export function executeOperation< } return maybePromise(callback, cb => { - if (topology.shouldCheckForSessionSupport()) { - return topology.selectServer(ReadPreference.primaryPreferred, err => { - if (err) return cb(err); - - executeOperation(topology, operation, cb); - }); - } - // The driver sessions spec mandates that we implicitly create sessions for operations // that are not explicitly provided with a session. let session: ClientSession | undefined = operation.session; diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index 5a49257e98a..e3018b1f487 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -630,17 +630,6 @@ export class Topology extends TypedEventEmitter { // Sessions related methods - /** - * @returns Whether the topology should initiate selection to determine session support - */ - shouldCheckForSessionSupport(): boolean { - if (this.description.type === TopologyType.Single) { - return !this.description.hasKnownServers; - } - - return !this.description.hasDataBearingServers; - } - /** Start a logical session */ startSession(options: ClientSessionOptions, clientOptions?: MongoOptions): ClientSession { const session = new ClientSession(this, this.s.sessionPool, options, clientOptions); diff --git a/test/unit/sdam/topology.test.js b/test/unit/sdam/topology.test.js index 55a3d12ce9d..898f9226c2c 100644 --- a/test/unit/sdam/topology.test.js +++ b/test/unit/sdam/topology.test.js @@ -7,7 +7,6 @@ const net = require('net'); const { MongoClient, ReadPreference } = require('../../../src'); const { Topology } = require('../../../src/sdam/topology'); const { Server } = require('../../../src/sdam/server'); -const { ServerDescription } = require('../../../src/sdam/server_description'); const { ns, makeClientMetadata, isHello } = require('../../../src/utils'); const { TopologyDescriptionChangedEvent } = require('../../../src/sdam/events'); const { TopologyDescription } = require('../../../src/sdam/topology_description'); @@ -77,80 +76,6 @@ describe('Topology (unit)', function () { }); }); - describe('shouldCheckForSessionSupport', function () { - beforeEach(function () { - this.sinon = sinon.createSandbox(); - - // these are mocks we want across all tests - this.sinon.stub(Server.prototype, 'requestCheck'); - this.sinon - .stub(Topology.prototype, 'selectServer') - .callsFake(function (selector, options, callback) { - setTimeout(() => { - const server = Array.from(this.s.servers.values())[0]; - callback(null, server); - }, 50); - }); - }); - - afterEach(function () { - this.sinon.restore(); - }); - - it('should check for sessions if connected to a single server and has no known servers', function (done) { - const topology = new Topology('someserver:27019'); - this.sinon.stub(Server.prototype, 'connect').callsFake(function () { - this.s.state = 'connected'; - this.emit('connect'); - }); - - topology.connect(() => { - expect(topology.shouldCheckForSessionSupport()).to.be.true; - topology.close(done); - }); - }); - - it('should not check for sessions if connected to a single server', function (done) { - const topology = new Topology('someserver:27019'); - this.sinon.stub(Server.prototype, 'connect').callsFake(function () { - this.s.state = 'connected'; - this.emit('connect'); - - setTimeout(() => { - this.emit( - 'descriptionReceived', - new ServerDescription('someserver:27019', { ok: 1, maxWireVersion: 6 }) - ); - }, 20); - }); - - topology.connect(() => { - expect(topology.shouldCheckForSessionSupport()).to.be.false; - topology.close(done); - }); - }); - - it('should check for sessions if there are no data-bearing nodes', function (done) { - const topology = new Topology(['mongos:27019', 'mongos:27018', 'mongos:27017'], {}); - this.sinon.stub(Server.prototype, 'connect').callsFake(function () { - this.s.state = 'connected'; - this.emit('connect'); - - setTimeout(() => { - this.emit( - 'descriptionReceived', - new ServerDescription(this.name, { ok: 1, msg: 'isdbgrid', maxWireVersion: 6 }) - ); - }, 20); - }); - - topology.connect(() => { - expect(topology.shouldCheckForSessionSupport()).to.be.false; - topology.close(done); - }); - }); - }); - describe('black holes', function () { let mockServer; beforeEach(() => mock.createServer().then(server => (mockServer = server))); From ef6894ddac5f91fb495e50f2d603335dce86378f Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Mon, 28 Feb 2022 15:25:12 +0100 Subject: [PATCH 03/12] Revert "fix(NODE-3521): remove extra server selection" This reverts commit 0f8cd6e4aa791aab65689e3fe8bc8768c06c3ddb. --- src/operations/execute_operation.ts | 8 +++ src/sdam/topology.ts | 11 +++++ test/unit/sdam/topology.test.js | 75 +++++++++++++++++++++++++++++ 3 files changed, 94 insertions(+) diff --git a/src/operations/execute_operation.ts b/src/operations/execute_operation.ts index 0565d344569..00321c119d9 100644 --- a/src/operations/execute_operation.ts +++ b/src/operations/execute_operation.ts @@ -77,6 +77,14 @@ export function executeOperation< } return maybePromise(callback, cb => { + if (topology.shouldCheckForSessionSupport()) { + return topology.selectServer(ReadPreference.primaryPreferred, err => { + if (err) return cb(err); + + executeOperation(topology, operation, cb); + }); + } + // The driver sessions spec mandates that we implicitly create sessions for operations // that are not explicitly provided with a session. let session: ClientSession | undefined = operation.session; diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index e3018b1f487..5a49257e98a 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -630,6 +630,17 @@ export class Topology extends TypedEventEmitter { // Sessions related methods + /** + * @returns Whether the topology should initiate selection to determine session support + */ + shouldCheckForSessionSupport(): boolean { + if (this.description.type === TopologyType.Single) { + return !this.description.hasKnownServers; + } + + return !this.description.hasDataBearingServers; + } + /** Start a logical session */ startSession(options: ClientSessionOptions, clientOptions?: MongoOptions): ClientSession { const session = new ClientSession(this, this.s.sessionPool, options, clientOptions); diff --git a/test/unit/sdam/topology.test.js b/test/unit/sdam/topology.test.js index 898f9226c2c..55a3d12ce9d 100644 --- a/test/unit/sdam/topology.test.js +++ b/test/unit/sdam/topology.test.js @@ -7,6 +7,7 @@ const net = require('net'); const { MongoClient, ReadPreference } = require('../../../src'); const { Topology } = require('../../../src/sdam/topology'); const { Server } = require('../../../src/sdam/server'); +const { ServerDescription } = require('../../../src/sdam/server_description'); const { ns, makeClientMetadata, isHello } = require('../../../src/utils'); const { TopologyDescriptionChangedEvent } = require('../../../src/sdam/events'); const { TopologyDescription } = require('../../../src/sdam/topology_description'); @@ -76,6 +77,80 @@ describe('Topology (unit)', function () { }); }); + describe('shouldCheckForSessionSupport', function () { + beforeEach(function () { + this.sinon = sinon.createSandbox(); + + // these are mocks we want across all tests + this.sinon.stub(Server.prototype, 'requestCheck'); + this.sinon + .stub(Topology.prototype, 'selectServer') + .callsFake(function (selector, options, callback) { + setTimeout(() => { + const server = Array.from(this.s.servers.values())[0]; + callback(null, server); + }, 50); + }); + }); + + afterEach(function () { + this.sinon.restore(); + }); + + it('should check for sessions if connected to a single server and has no known servers', function (done) { + const topology = new Topology('someserver:27019'); + this.sinon.stub(Server.prototype, 'connect').callsFake(function () { + this.s.state = 'connected'; + this.emit('connect'); + }); + + topology.connect(() => { + expect(topology.shouldCheckForSessionSupport()).to.be.true; + topology.close(done); + }); + }); + + it('should not check for sessions if connected to a single server', function (done) { + const topology = new Topology('someserver:27019'); + this.sinon.stub(Server.prototype, 'connect').callsFake(function () { + this.s.state = 'connected'; + this.emit('connect'); + + setTimeout(() => { + this.emit( + 'descriptionReceived', + new ServerDescription('someserver:27019', { ok: 1, maxWireVersion: 6 }) + ); + }, 20); + }); + + topology.connect(() => { + expect(topology.shouldCheckForSessionSupport()).to.be.false; + topology.close(done); + }); + }); + + it('should check for sessions if there are no data-bearing nodes', function (done) { + const topology = new Topology(['mongos:27019', 'mongos:27018', 'mongos:27017'], {}); + this.sinon.stub(Server.prototype, 'connect').callsFake(function () { + this.s.state = 'connected'; + this.emit('connect'); + + setTimeout(() => { + this.emit( + 'descriptionReceived', + new ServerDescription(this.name, { ok: 1, msg: 'isdbgrid', maxWireVersion: 6 }) + ); + }, 20); + }); + + topology.connect(() => { + expect(topology.shouldCheckForSessionSupport()).to.be.false; + topology.close(done); + }); + }); + }); + describe('black holes', function () { let mockServer; beforeEach(() => mock.createServer().then(server => (mockServer = server))); From b0ca0b6beea22608c4b7b8b9c21020bab40e0c74 Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Mon, 28 Feb 2022 15:25:40 +0100 Subject: [PATCH 04/12] Revert "fix(NODE-3521): remove session support checks" This reverts commit a0dfc5bfaa097446999cd54ca5a979c0c7544657. --- src/cmap/connection.ts | 8 +- src/cursor/abstract_cursor.ts | 2 +- src/operations/execute_operation.ts | 20 +++-- src/sdam/topology.ts | 7 ++ src/utils.ts | 2 +- test/unit/assorted/sessions_client.test.js | 91 +++++++++++++++++++++- test/unit/cmap/connection.test.js | 48 +++++++++++- 7 files changed, 166 insertions(+), 12 deletions(-) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index 0987a19c3c0..c8feb308984 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -402,7 +402,7 @@ export class Connection extends TypedEventEmitter { if (deprecationErrors != null) finalCmd.apiDeprecationErrors = deprecationErrors; } - if (session) { + if (hasSessionSupport(this) && session) { if ( session.clusterTime && clusterTime && @@ -683,6 +683,12 @@ export class CryptoConnection extends Connection { } } +/** @internal */ +export function hasSessionSupport(conn: Connection): boolean { + const description = conn.description; + return description.logicalSessionTimeoutMinutes != null || !!description.loadBalanced; +} + function supportsOpMsg(conn: Connection) { const description = conn.description; if (description == null) { diff --git a/src/cursor/abstract_cursor.ts b/src/cursor/abstract_cursor.ts index 1d0e5423ca9..d729dc38a8f 100644 --- a/src/cursor/abstract_cursor.ts +++ b/src/cursor/abstract_cursor.ts @@ -653,7 +653,7 @@ function next(cursor: AbstractCursor, blocking: boolean, callback: Callback { return !this.description.hasDataBearingServers; } + /** + * @returns Whether sessions are supported on the current topology + */ + hasSessionSupport(): boolean { + return this.loadBalanced || this.description.logicalSessionTimeoutMinutes != null; + } + /** Start a logical session */ startSession(options: ClientSessionOptions, clientOptions?: MongoOptions): ClientSession { const session = new ClientSession(this, this.s.sessionPool, options, clientOptions); diff --git a/src/utils.ts b/src/utils.ts index 612d573ea64..ac787252f62 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -229,7 +229,7 @@ export function executeLegacyOperation( let session: ClientSession; let opOptions: any; let owner: any; - if (!options.skipSessions) { + if (!options.skipSessions && topology.hasSessionSupport()) { opOptions = args[args.length - 2]; if (opOptions == null || opOptions.session == null) { owner = Symbol(); diff --git a/test/unit/assorted/sessions_client.test.js b/test/unit/assorted/sessions_client.test.js index 367539826e9..75e0e8c4ca4 100644 --- a/test/unit/assorted/sessions_client.test.js +++ b/test/unit/assorted/sessions_client.test.js @@ -2,6 +2,7 @@ const expect = require('chai').expect; const mock = require('../../tools/mongodb-mock/index'); +const { ReplSetFixture } = require('../../tools/common'); const { isHello } = require('../../../src/utils'); const { MongoClient } = require('../../../src'); @@ -15,7 +16,95 @@ describe('Sessions - client/unit', function () { }); }); - it('should return a client session when requested', function (done) { + it('should not throw a synchronous exception if sessions are not supported', function () { + test.server.setMessageHandler(request => { + var doc = request.document; + if (isHello(doc)) { + request.reply(Object.assign({}, mock.HELLO)); + } else if (doc.endSessions) { + request.reply({ ok: 1 }); + } + }); + + const client = new MongoClient(`mongodb://${test.server.uri()}/test`); + return client.connect().then(() => { + expect(() => client.startSession()).to.not.throw( + 'Current topology does not support sessions' + ); + return client.close(); + }); + }); + + it('should throw an exception if sessions are not supported on some servers', function () { + const replicaSetMock = new ReplSetFixture(); + let testClient; + return replicaSetMock + .setup({ doNotInitHandlers: true }) + .then(() => { + replicaSetMock.firstSecondaryServer.setMessageHandler(request => { + var doc = request.document; + if (isHello(doc)) { + const hello = replicaSetMock.firstSecondaryStates[0]; + hello.logicalSessionTimeoutMinutes = 20; + request.reply(hello); + } else if (doc.endSessions) { + request.reply({ ok: 1 }); + } + }); + + replicaSetMock.secondSecondaryServer.setMessageHandler(request => { + var doc = request.document; + if (isHello(doc)) { + const hello = replicaSetMock.secondSecondaryStates[0]; + hello.logicalSessionTimeoutMinutes = 10; + request.reply(hello); + } else if (doc.endSessions) { + request.reply({ ok: 1 }); + } + }); + + replicaSetMock.arbiterServer.setMessageHandler(request => { + var doc = request.document; + if (isHello(doc)) { + const hello = replicaSetMock.arbiterStates[0]; + hello.logicalSessionTimeoutMinutes = 30; + request.reply(hello); + } else if (doc.endSessions) { + request.reply({ ok: 1 }); + } + }); + + replicaSetMock.primaryServer.setMessageHandler(request => { + var doc = request.document; + if (isHello(doc)) { + const hello = replicaSetMock.primaryStates[0]; + hello.logicalSessionTimeoutMinutes = null; + request.reply(hello); + } else if (doc.endSessions) { + request.reply({ ok: 1 }); + } + }); + + return replicaSetMock.uri(); + }) + .then(uri => { + testClient = new MongoClient(uri); + return testClient.connect(); + }) + .then(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'); + }) + .finally(() => (testClient ? testClient.close() : null)); + }); + + it('should return a client session when requested if the topology supports it', function (done) { test.server.setMessageHandler(request => { var doc = request.document; if (isHello(doc)) { diff --git a/test/unit/cmap/connection.test.js b/test/unit/cmap/connection.test.js index 5fdf9d3bfab..4b2a1fa09e3 100644 --- a/test/unit/cmap/connection.test.js +++ b/test/unit/cmap/connection.test.js @@ -2,8 +2,9 @@ const mock = require('../../tools/mongodb-mock/index'); const { connect } = require('../../../src/cmap/connect'); -const { Connection } = require('../../../src/cmap/connection'); +const { Connection, hasSessionSupport } = require('../../../src/cmap/connection'); const { expect } = require('chai'); +const { Socket } = require('net'); const { ns, isHello } = require('../../../src/utils'); const { getSymbolFrom } = require('../../tools/utils'); @@ -106,4 +107,49 @@ describe('Connection - unit/cmap', function () { done(); }); }); + + describe('.hasSessionSupport', function () { + let connection; + const stream = new Socket(); + + context('when logicalSessionTimeoutMinutes is present', function () { + beforeEach(function () { + connection = new Connection(stream, { + hostAddress: server.hostAddress(), + logicalSessionTimeoutMinutes: 5 + }); + }); + + it('returns true', function () { + expect(hasSessionSupport(connection)).to.be.true; + }); + }); + + context('when logicalSessionTimeoutMinutes is not present', function () { + context('when in load balancing mode', function () { + beforeEach(function () { + connection = new Connection(stream, { + hostAddress: server.hostAddress(), + loadBalanced: true + }); + }); + + it('returns true', function () { + expect(hasSessionSupport(connection)).to.be.true; + }); + }); + + context('when not in load balancing mode', function () { + beforeEach(function () { + connection = new Connection(stream, { + hostAddress: server.hostAddress() + }); + }); + + it('returns false', function () { + expect(hasSessionSupport(connection)).to.be.false; + }); + }); + }); + }); }); From d3e21096c2e7328d96e9aa03dc33de82a8146773 Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Mon, 28 Feb 2022 15:57:41 +0100 Subject: [PATCH 05/12] fix(NODE-3521): properly check session check --- src/cursor/abstract_cursor.ts | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/cursor/abstract_cursor.ts b/src/cursor/abstract_cursor.ts index d729dc38a8f..c4969abc27d 100644 --- a/src/cursor/abstract_cursor.ts +++ b/src/cursor/abstract_cursor.ts @@ -653,8 +653,15 @@ function next(cursor: AbstractCursor, blocking: boolean, callback: Callback { + if (err) return callback(err); + return next(cursor, blocking, callback); + }); + } else if (cursor[kTopology].hasSessionSupport()) { + cursor[kSession] = cursor[kTopology].startSession({ owner: cursor, explicit: false }); + } } cursor._initialize(cursor[kSession], (err, state) => { From ddc4d2485470e9428649066b49333738bdf0569b Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Tue, 1 Mar 2022 13:31:12 +0100 Subject: [PATCH 06/12] test(NODE-3521): add next tests for session --- test/unit/cursor/find_cursor.test.js | 105 +++++++++++++++++++++++++++ 1 file changed, 105 insertions(+) diff --git a/test/unit/cursor/find_cursor.test.js b/test/unit/cursor/find_cursor.test.js index 1aa8165640f..f17d3b95b43 100644 --- a/test/unit/cursor/find_cursor.test.js +++ b/test/unit/cursor/find_cursor.test.js @@ -10,6 +10,111 @@ const { FindCursor } = require('../../../src/cursor/find_cursor'); const test = {}; describe('Find Cursor', function () { + describe('#next', function () { + afterEach(function () { + mock.cleanup() + }); + beforeEach(function () { + return mock.createServer().then(mockServer => { + test.server = mockServer; + }); + }); + + context('when there is a data bearing server', function () { + beforeEach(function () { + test.server.setMessageHandler(request => { + const doc = request.document; + if (isHello(doc)) { + request.reply(mock.HELLO); + } else if (doc.find) { + request.reply({ + cursor: { + id: Long.fromNumber(1), + ns: 'test.test', + firstBatch: [{ _id: 1, name: 'test' }] + }, + ok: 1 + }); + } + }); + }); + + it('sets the session on the cursor', function (done) { + const topology = new Topology(test.server.hostAddress()); + const cursor = new FindCursor(topology, MongoDBNamespace.fromString('test.test'), {}, {}); + topology.connect(function () { + cursor.next(function () { + expect(cursor.session).to.exist; + topology.close(done); + }); + }); + }); + }); + + context('when there is no data bearing server', function () { + beforeEach(function () { + test.server.setMessageHandler(request => { + const doc = request.document; + if (isHello(doc)) { + request.reply({ errmsg: 'network error' }); + } else if (doc.find) { + request.reply({ + cursor: { + id: Long.fromNumber(1), + ns: 'test.test', + firstBatch: [{ _id: 1, name: 'test' }] + }, + ok: 1 + }); + } + }); + }); + + it('does not set the session on the cursor', function (done) { + const topology = new Topology(test.server.hostAddress(), { serverSelectionTimeoutMS: 2 }); + const cursor = new FindCursor(topology, MongoDBNamespace.fromString('test.test'), {}, {}); + topology.connect(function () { + cursor.next(function () { + expect(cursor.session).to.not.exist; + topology.close(done); + }); + }); + }); + }); + + context('when a data bearing server becomes available', function () { + beforeEach(function () { + let helloCalls = 0; + test.server.setMessageHandler(request => { + const doc = request.document; + if (isHello(doc)) { + request.reply(helloCalls > 0 ? { errmsg: 'network error' } : mock.HELLO); + } else if (doc.find) { + request.reply({ + cursor: { + id: Long.fromNumber(1), + ns: 'test.test', + firstBatch: [{ _id: 1, name: 'test' }] + }, + ok: 1 + }); + } + }); + }); + + it('sets the session on the cursor', function (done) { + const topology = new Topology(test.server.hostAddress(), { serverSelectionTimeoutMS: 2 }); + const cursor = new FindCursor(topology, MongoDBNamespace.fromString('test.test'), {}, {}); + topology.connect(function () { + cursor.next(function () { + expect(cursor.session).to.exist; + topology.close(done); + }); + }); + }); + }); + }); + describe('Response', function () { afterEach(() => mock.cleanup()); beforeEach(() => { From dc621737414614100f976c5403f1c009234b9ebf Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Tue, 1 Mar 2022 13:55:33 +0100 Subject: [PATCH 07/12] chore(NODE-3521): find/agg cursor session always there --- src/cursor/aggregation_cursor.ts | 2 +- src/cursor/find_cursor.ts | 2 +- test/unit/cursor/aggregation_cursor.test.js | 135 ++++++++++++++++++++ test/unit/cursor/find_cursor.test.js | 10 +- 4 files changed, 144 insertions(+), 5 deletions(-) create mode 100644 test/unit/cursor/aggregation_cursor.test.js diff --git a/src/cursor/aggregation_cursor.ts b/src/cursor/aggregation_cursor.ts index 281e2f0cb67..45353c114d1 100644 --- a/src/cursor/aggregation_cursor.ts +++ b/src/cursor/aggregation_cursor.ts @@ -61,7 +61,7 @@ export class AggregationCursor extends AbstractCursor): void { + _initialize(session: ClientSession, callback: Callback): void { const aggregateOperation = new AggregateOperation(this.namespace, this[kPipeline], { ...this[kOptions], ...this.cursorOptions, diff --git a/src/cursor/find_cursor.ts b/src/cursor/find_cursor.ts index 154adee342d..cab4ee6b203 100644 --- a/src/cursor/find_cursor.ts +++ b/src/cursor/find_cursor.ts @@ -68,7 +68,7 @@ export class FindCursor extends AbstractCursor { } /** @internal */ - _initialize(session: ClientSession | undefined, callback: Callback): void { + _initialize(session: ClientSession, callback: Callback): void { const findOperation = new FindOperation(undefined, this.namespace, this[kFilter], { ...this[kBuiltOptions], // NOTE: order matters here, we may need to refine this ...this.cursorOptions, diff --git a/test/unit/cursor/aggregation_cursor.test.js b/test/unit/cursor/aggregation_cursor.test.js new file mode 100644 index 00000000000..0ea9f1711f8 --- /dev/null +++ b/test/unit/cursor/aggregation_cursor.test.js @@ -0,0 +1,135 @@ +'use strict'; + +const expect = require('chai').expect; +const mock = require('../../tools/mongodb-mock/index'); +const { Topology } = require('../../../src/sdam/topology'); +const { Long } = require('bson'); +const { MongoDBNamespace, isHello } = require('../../../src/utils'); +const { AggregationCursor } = require('../../../src/cursor/aggregation_cursor'); + +const test = {}; +describe('Aggregation Cursor', function () { + describe('#next', function () { + afterEach(function () { + mock.cleanup(); + }); + beforeEach(function () { + return mock.createServer().then(mockServer => { + test.server = mockServer; + }); + }); + + context('when there is a data bearing server', function () { + beforeEach(function () { + test.server.setMessageHandler(request => { + const doc = request.document; + if (isHello(doc)) { + request.reply(mock.HELLO); + } else if (doc.aggregate) { + request.reply({ + cursor: { + id: Long.fromNumber(1), + ns: 'test.test', + firstBatch: [{ _id: 1, name: 'test' }] + }, + ok: 1 + }); + } + }); + }); + + it('sets the session on the cursor', function (done) { + const topology = new Topology(test.server.hostAddress()); + const cursor = new AggregationCursor( + topology, + MongoDBNamespace.fromString('test.test'), + [], + {} + ); + topology.connect(function () { + cursor.next(function () { + expect(cursor.session).to.exist; + topology.close(done); + }); + }); + }); + }); + + context('when there is no data bearing server', function () { + beforeEach(function () { + test.server.setMessageHandler(request => { + const doc = request.document; + if (isHello(doc)) { + request.reply({ errmsg: 'network error' }); + } else if (doc.aggregate) { + request.reply({ + cursor: { + id: Long.fromNumber(1), + ns: 'test.test', + firstBatch: [{ _id: 1, name: 'test' }] + }, + ok: 1 + }); + } + }); + }); + + it('does not set the session on the cursor', function (done) { + const topology = new Topology(test.server.hostAddress(), { + serverSelectionTimeoutMS: 1000 + }); + const cursor = new AggregationCursor( + topology, + MongoDBNamespace.fromString('test.test'), + [], + {} + ); + topology.connect(function () { + cursor.next(function () { + expect(cursor.session).to.not.exist; + topology.close(done); + }); + }); + }); + }); + + context('when a data bearing server becomes available', function () { + beforeEach(function () { + let helloCalls = 0; + test.server.setMessageHandler(request => { + const doc = request.document; + if (isHello(doc)) { + request.reply(helloCalls > 0 ? { errmsg: 'network error' } : mock.HELLO); + } else if (doc.aggregate) { + request.reply({ + cursor: { + id: Long.fromNumber(1), + ns: 'test.test', + firstBatch: [{ _id: 1, name: 'test' }] + }, + ok: 1 + }); + } + }); + }); + + it('sets the session on the cursor', function (done) { + const topology = new Topology(test.server.hostAddress(), { + serverSelectionTimeoutMS: 1000 + }); + const cursor = new AggregationCursor( + topology, + MongoDBNamespace.fromString('test.test'), + [], + {} + ); + topology.connect(function () { + cursor.next(function () { + expect(cursor.session).to.exist; + topology.close(done); + }); + }); + }); + }); + }); +}); diff --git a/test/unit/cursor/find_cursor.test.js b/test/unit/cursor/find_cursor.test.js index f17d3b95b43..dccf8eee188 100644 --- a/test/unit/cursor/find_cursor.test.js +++ b/test/unit/cursor/find_cursor.test.js @@ -12,7 +12,7 @@ const test = {}; describe('Find Cursor', function () { describe('#next', function () { afterEach(function () { - mock.cleanup() + mock.cleanup(); }); beforeEach(function () { return mock.createServer().then(mockServer => { @@ -71,7 +71,9 @@ describe('Find Cursor', function () { }); it('does not set the session on the cursor', function (done) { - const topology = new Topology(test.server.hostAddress(), { serverSelectionTimeoutMS: 2 }); + const topology = new Topology(test.server.hostAddress(), { + serverSelectionTimeoutMS: 1000 + }); const cursor = new FindCursor(topology, MongoDBNamespace.fromString('test.test'), {}, {}); topology.connect(function () { cursor.next(function () { @@ -103,7 +105,9 @@ describe('Find Cursor', function () { }); it('sets the session on the cursor', function (done) { - const topology = new Topology(test.server.hostAddress(), { serverSelectionTimeoutMS: 2 }); + const topology = new Topology(test.server.hostAddress(), { + serverSelectionTimeoutMS: 1000 + }); const cursor = new FindCursor(topology, MongoDBNamespace.fromString('test.test'), {}, {}); topology.connect(function () { cursor.next(function () { From 5839dae8b178c12064506a09ae536a62bbc001a1 Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Wed, 2 Mar 2022 07:05:04 +0100 Subject: [PATCH 08/12] test(NODE-3521): fix cursor server selection tests --- test/unit/cursor/aggregation_cursor.test.js | 7 ++++++- test/unit/cursor/find_cursor.test.js | 7 ++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/test/unit/cursor/aggregation_cursor.test.js b/test/unit/cursor/aggregation_cursor.test.js index 0ea9f1711f8..44f074958e6 100644 --- a/test/unit/cursor/aggregation_cursor.test.js +++ b/test/unit/cursor/aggregation_cursor.test.js @@ -95,11 +95,16 @@ describe('Aggregation Cursor', function () { context('when a data bearing server becomes available', function () { beforeEach(function () { + // Set the count of times hello has been called. let helloCalls = 0; test.server.setMessageHandler(request => { const doc = request.document; if (isHello(doc)) { - request.reply(helloCalls > 0 ? { errmsg: 'network error' } : mock.HELLO); + // After the first hello call errors indicating no data bearing server is + // available, any subsequent hello call should succeed after server selection. + // This gives us a data bearing server available for the next call. + request.reply(helloCalls > 0 ? mock.HELLO : { errmsg: 'network error' }); + helloCalls++; } else if (doc.aggregate) { request.reply({ cursor: { diff --git a/test/unit/cursor/find_cursor.test.js b/test/unit/cursor/find_cursor.test.js index dccf8eee188..1cc4c7f4b59 100644 --- a/test/unit/cursor/find_cursor.test.js +++ b/test/unit/cursor/find_cursor.test.js @@ -86,11 +86,16 @@ describe('Find Cursor', function () { context('when a data bearing server becomes available', function () { beforeEach(function () { + // Set the count of times hello has been called. let helloCalls = 0; test.server.setMessageHandler(request => { const doc = request.document; if (isHello(doc)) { - request.reply(helloCalls > 0 ? { errmsg: 'network error' } : mock.HELLO); + // After the first hello call errors indicating no data bearing server is + // available, any subsequent hello call should succeed after server selection. + // This gives us a data bearing server available for the next call. + request.reply(helloCalls > 0 ? mock.HELLO : { errmsg: 'network error' }); + helloCalls++; } else if (doc.find) { request.reply({ cursor: { From 68365bf6c5830a86befb9e19f36801cd7c4ba0e4 Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Wed, 2 Mar 2022 23:05:27 +0100 Subject: [PATCH 09/12] Update test/unit/cursor/aggregation_cursor.test.js Co-authored-by: Bailey Pearson --- test/unit/cursor/aggregation_cursor.test.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/unit/cursor/aggregation_cursor.test.js b/test/unit/cursor/aggregation_cursor.test.js index 44f074958e6..921550da52c 100644 --- a/test/unit/cursor/aggregation_cursor.test.js +++ b/test/unit/cursor/aggregation_cursor.test.js @@ -1,6 +1,7 @@ 'use strict'; -const expect = require('chai').expect; +const { expect } = require('chai'); +const { createServer, cleanup, isHello } = require('../../tools/mongodb-mock/index') const mock = require('../../tools/mongodb-mock/index'); const { Topology } = require('../../../src/sdam/topology'); const { Long } = require('bson'); From dd594f670bd0385a146b6fca19f1abd15a8c9ba4 Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Wed, 2 Mar 2022 23:05:37 +0100 Subject: [PATCH 10/12] Update test/unit/cursor/aggregation_cursor.test.js Co-authored-by: Bailey Pearson --- test/unit/cursor/aggregation_cursor.test.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/unit/cursor/aggregation_cursor.test.js b/test/unit/cursor/aggregation_cursor.test.js index 921550da52c..fc4da6466f3 100644 --- a/test/unit/cursor/aggregation_cursor.test.js +++ b/test/unit/cursor/aggregation_cursor.test.js @@ -14,7 +14,9 @@ describe('Aggregation Cursor', function () { afterEach(function () { mock.cleanup(); }); - beforeEach(function () { +beforeEach(async function() { + test.server = await mock.createServer(); +}) return mock.createServer().then(mockServer => { test.server = mockServer; }); From 05ad2d29f32bbc5db2fd30a7695d15ad9765b5bd Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Wed, 2 Mar 2022 23:05:52 +0100 Subject: [PATCH 11/12] Update test/unit/cursor/find_cursor.test.js Co-authored-by: Bailey Pearson --- test/unit/cursor/find_cursor.test.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/unit/cursor/find_cursor.test.js b/test/unit/cursor/find_cursor.test.js index 1cc4c7f4b59..af17f2c250d 100644 --- a/test/unit/cursor/find_cursor.test.js +++ b/test/unit/cursor/find_cursor.test.js @@ -14,7 +14,9 @@ describe('Find Cursor', function () { afterEach(function () { mock.cleanup(); }); - beforeEach(function () { +beforeEach(async function() { + test.server = await mock.createServer(); +}) return mock.createServer().then(mockServer => { test.server = mockServer; }); From 8ec9ca0c4d079899aa778811177b0447d98f18e9 Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Thu, 3 Mar 2022 14:26:49 +0100 Subject: [PATCH 12/12] fix(NODE-3521): lint errors --- test/unit/cursor/aggregation_cursor.test.js | 9 ++------- test/unit/cursor/find_cursor.test.js | 8 ++------ 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/test/unit/cursor/aggregation_cursor.test.js b/test/unit/cursor/aggregation_cursor.test.js index fc4da6466f3..65a654dc097 100644 --- a/test/unit/cursor/aggregation_cursor.test.js +++ b/test/unit/cursor/aggregation_cursor.test.js @@ -1,7 +1,6 @@ 'use strict'; const { expect } = require('chai'); -const { createServer, cleanup, isHello } = require('../../tools/mongodb-mock/index') const mock = require('../../tools/mongodb-mock/index'); const { Topology } = require('../../../src/sdam/topology'); const { Long } = require('bson'); @@ -14,12 +13,8 @@ describe('Aggregation Cursor', function () { afterEach(function () { mock.cleanup(); }); -beforeEach(async function() { - test.server = await mock.createServer(); -}) - return mock.createServer().then(mockServer => { - test.server = mockServer; - }); + beforeEach(async function () { + test.server = await mock.createServer(); }); context('when there is a data bearing server', function () { diff --git a/test/unit/cursor/find_cursor.test.js b/test/unit/cursor/find_cursor.test.js index af17f2c250d..4e4bb8d54bf 100644 --- a/test/unit/cursor/find_cursor.test.js +++ b/test/unit/cursor/find_cursor.test.js @@ -14,12 +14,8 @@ describe('Find Cursor', function () { afterEach(function () { mock.cleanup(); }); -beforeEach(async function() { - test.server = await mock.createServer(); -}) - return mock.createServer().then(mockServer => { - test.server = mockServer; - }); + beforeEach(async function () { + test.server = await mock.createServer(); }); context('when there is a data bearing server', function () {