From 77d96e483fa9a2502bbf635753348c5fd258d9b3 Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 6 Mar 2024 14:58:29 -0500 Subject: [PATCH 01/40] eslint and build fixes From f5bbb4308654fec6aab589039562245ef2cad949 Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 6 Mar 2024 15:26:50 -0500 Subject: [PATCH 02/40] remove force flag From 6c2b3c8715c34dc89354557bcd81249b893c24e8 Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 6 Mar 2024 17:18:23 -0500 Subject: [PATCH 03/40] remove import From a0da9ca39107aa13798089a35fc6f2daf4471f97 Mon Sep 17 00:00:00 2001 From: Warren James Date: Thu, 7 Mar 2024 15:45:36 -0500 Subject: [PATCH 04/40] fix???? From d0a1d3fe9e479e89633b7ef034fd28198f035f0f Mon Sep 17 00:00:00 2001 From: Warren James Date: Thu, 7 Mar 2024 16:34:36 -0500 Subject: [PATCH 05/40] remove DestroyOptions From ddd9f2e81219acdab70afac02f1bf588a0bc2b99 Mon Sep 17 00:00:00 2001 From: Warren James Date: Tue, 5 Mar 2024 15:17:47 -0500 Subject: [PATCH 06/40] refactor(NODE-5914): refactor Topology.selectServer to async-await --- src/change_stream.ts | 14 +++---- src/operations/execute_operation.ts | 4 +- src/sdam/topology.ts | 57 +++++++++++------------------ 3 files changed, 31 insertions(+), 44 deletions(-) diff --git a/src/change_stream.ts b/src/change_stream.ts index d8e4ad8a8cc..bdcb1c0abd1 100644 --- a/src/change_stream.ts +++ b/src/change_stream.ts @@ -934,14 +934,14 @@ export class ChangeStream< this.cursor.close().catch(() => null); const topology = getTopology(this.parent); - topology.selectServer( - this.cursor.readPreference, - { operationName: 'reconnect topology in change stream' }, - serverSelectionError => { + topology + .selectServer(this.cursor.readPreference, { + operationName: 'reconnect topology in change stream' + }) + .catch(serverSelectionError => { if (serverSelectionError) return this._closeEmitterModeWithError(changeStreamError); this.cursor = this._createChangeStreamCursor(this.cursor.resumeOptions); - } - ); + }); } else { this._closeEmitterModeWithError(changeStreamError); } @@ -966,7 +966,7 @@ export class ChangeStream< await this.cursor.close().catch(() => null); const topology = getTopology(this.parent); try { - await topology.selectServerAsync(this.cursor.readPreference, { + await topology.selectServer(this.cursor.readPreference, { operationName: 'reconnect topology in change stream' }); this.cursor = this._createChangeStreamCursor(this.cursor.resumeOptions); diff --git a/src/operations/execute_operation.ts b/src/operations/execute_operation.ts index 21d8b7e11b6..f1050914a5f 100644 --- a/src/operations/execute_operation.ts +++ b/src/operations/execute_operation.ts @@ -139,7 +139,7 @@ export async function executeOperation< selector = readPreference; } - const server = await topology.selectServerAsync(selector, { + const server = await topology.selectServer(selector, { session, operationName: operation.commandName }); @@ -244,7 +244,7 @@ async function retryOperation< } // select a new server, and attempt to retry the operation - const server = await topology.selectServerAsync(selector, { + const server = await topology.selectServer(selector, { session, operationName: operation.commandName, previousServer diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index 92364748ea6..b9baccc666a 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -44,6 +44,7 @@ import { makeStateMachine, now, ns, + promiseWithResolvers, shuffle, TimeoutController } from '../utils'; @@ -105,7 +106,8 @@ export interface ServerSelectionRequest { mongoLogger: MongoLogger | undefined; transaction?: Transaction; startTime: number; - callback: ServerSelectionCallback; + resolve: (server: Server) => void; + reject: (reason?: any) => void; [kCancelled]?: boolean; timeoutController: TimeoutController; operationName: string; @@ -238,11 +240,6 @@ export class Topology extends TypedEventEmitter { /** @event */ static readonly TIMEOUT = TIMEOUT; - selectServerAsync: ( - selector: string | ReadPreference | ServerSelector, - options: SelectServerOptions - ) => Promise; - /** * @param seedlist - a list of HostAddress instances to connect to */ @@ -254,14 +251,6 @@ export class Topology extends TypedEventEmitter { super(); this.client = client; - this.selectServerAsync = promisify( - ( - selector: string | ReadPreference | ServerSelector, - options: SelectServerOptions, - callback: (e: Error, r: Server) => void - ) => this.selectServer(selector, options, callback as any) - ); - // Options should only be undefined in tests, MongoClient will always have defined options options = options ?? { hosts: [HostAddress.fromString('localhost:27017')], @@ -464,15 +453,9 @@ export class Topology extends TypedEventEmitter { const readPreference = options.readPreference ?? ReadPreference.primary; const selectServerOptions = { operationName: 'ping', ...options }; - this.selectServer( - readPreferenceServerSelector(readPreference), - selectServerOptions, - (err, server) => { - if (err) { - this.close(); - return exitWithError(err); - } + this.selectServer(readPreferenceServerSelector(readPreference), selectServerOptions).then( + server => { const skipPingOnConnect = this.s.options[Symbol.for('@@mdb.skipPingOnConnect')] === true; if (!skipPingOnConnect && server && this.s.credentials) { server.command(ns('admin.$cmd'), { ping: 1 }, {}).then(() => { @@ -491,6 +474,9 @@ export class Topology extends TypedEventEmitter { this.emit(Topology.CONNECT, this); callback?.(undefined, this); + }, + error => { + return this.close({ force: false }, () => exitWithError(error)); } ); } @@ -533,11 +519,10 @@ export class Topology extends TypedEventEmitter { * @param callback - The callback used to indicate success or failure * @returns An instance of a `Server` meeting the criteria of the predicate provided */ - selectServer( + async selectServer( selector: string | ReadPreference | ServerSelector, - options: SelectServerOptions, - callback: Callback - ): void { + options: SelectServerOptions + ): Promise { let serverSelector; if (typeof selector !== 'function') { if (typeof selector === 'string') { @@ -588,16 +573,17 @@ export class Topology extends TypedEventEmitter { ) ); } - callback(undefined, transaction.server); - return; + return transaction.server; } + const { promise: serverPromise, resolve, reject } = promiseWithResolvers(); const waitQueueMember: ServerSelectionRequest = { serverSelector, topologyDescription: this.description, mongoLogger: this.client.mongoLogger, transaction, - callback, + resolve, + reject, timeoutController: new TimeoutController(options.serverSelectionTimeoutMS), startTime: now(), operationName: options.operationName, @@ -628,13 +614,14 @@ export class Topology extends TypedEventEmitter { ) ); } - waitQueueMember.callback(timeoutError); + waitQueueMember.reject(timeoutError); }); this[kWaitQueue].push(waitQueueMember); processWaitQueue(this); - } + return serverPromise; + } /** * Update the internal TopologyDescription with a ServerDescription * @@ -911,7 +898,7 @@ function drainWaitQueue(queue: List, err?: MongoDriverEr ); } } - waitQueueMember.callback(err); + waitQueueMember.reject(err); } } } @@ -964,7 +951,7 @@ function processWaitQueue(topology: Topology) { ) ); } - waitQueueMember.callback(e); + waitQueueMember.reject(e); continue; } @@ -1027,7 +1014,7 @@ function processWaitQueue(topology: Topology) { ) ); } - waitQueueMember.callback(error); + waitQueueMember.reject(error); return; } const transaction = waitQueueMember.transaction; @@ -1053,7 +1040,7 @@ function processWaitQueue(topology: Topology) { ) ); } - waitQueueMember.callback(undefined, selectedServer); + waitQueueMember.resolve(selectedServer); } if (topology[kWaitQueue].length > 0) { From 5883888001b607e00b7de58aff3d624ec40ffa6c Mon Sep 17 00:00:00 2001 From: Warren James Date: Tue, 5 Mar 2024 15:18:18 -0500 Subject: [PATCH 07/40] fix tests --- ...rver_discovery_and_monitoring.spec.test.ts | 5 +- .../server_selection_latency_window_utils.ts | 5 +- .../assorted/server_selection_spec_helper.js | 82 +++++++++---------- test/unit/error.test.ts | 12 +-- test/unit/sdam/server_selection.test.ts | 8 +- test/unit/sdam/topology.test.js | 77 ++++++++--------- 6 files changed, 85 insertions(+), 104 deletions(-) diff --git a/test/unit/assorted/server_discovery_and_monitoring.spec.test.ts b/test/unit/assorted/server_discovery_and_monitoring.spec.test.ts index 82dd7a609d0..3638a29278c 100644 --- a/test/unit/assorted/server_discovery_and_monitoring.spec.test.ts +++ b/test/unit/assorted/server_discovery_and_monitoring.spec.test.ts @@ -207,12 +207,11 @@ describe('Server Discovery and Monitoring (spec)', function () { topologySelectServers = sinon .stub(Topology.prototype, 'selectServer') - .callsFake(function (selector, options, callback) { + .callsFake(function (selector, options) { topologySelectServers.restore(); const fakeServer = { s: { state: 'connected' }, removeListener: () => true }; - // @ts-expect-error: stub doesn't need to be a full server - callback(undefined, fakeServer); + return Promise.resolve(fakeServer); }); }); diff --git a/test/unit/assorted/server_selection_latency_window_utils.ts b/test/unit/assorted/server_selection_latency_window_utils.ts index dd9db783a00..a72ada9d8f3 100644 --- a/test/unit/assorted/server_selection_latency_window_utils.ts +++ b/test/unit/assorted/server_selection_latency_window_utils.ts @@ -139,10 +139,7 @@ export async function runServerSelectionLatencyWindowTest(test: ServerSelectionL const selectedServers: Server[] = []; for (let i = 0; i < test.iterations; ++i) { - const server: Server = await promisify(topology.selectServer.bind(topology))( - ReadPreference.NEAREST, - {} - ); + const server: Server = await topology.selectServer.bind(topology)(ReadPreference.NEAREST, {}); selectedServers.push(server); } diff --git a/test/unit/assorted/server_selection_spec_helper.js b/test/unit/assorted/server_selection_spec_helper.js index c6e15a87316..deed0bd5b8c 100644 --- a/test/unit/assorted/server_selection_spec_helper.js +++ b/test/unit/assorted/server_selection_spec_helper.js @@ -102,11 +102,11 @@ function executeServerSelectionTest(testDefinition, testDone) { // call to `selectServers` call a fake, and then immediately restore the original behavior. let topologySelectServers = sinon .stub(Topology.prototype, 'selectServer') - .callsFake(function (selector, options, callback) { + .callsFake(function (selector, options) { topologySelectServers.restore(); const fakeServer = { s: { state: 'connected' }, removeListener: () => {} }; - callback(undefined, fakeServer); + return Promise.resolve(fakeServer); }); function done(err) { @@ -149,56 +149,54 @@ function executeServerSelectionTest(testDefinition, testDone) { } // default to serverSelectionTimeoutMS of `100` for unit tests - topology.selectServer(selector, { serverSelectionTimeoutMS: 50 }, (err, server) => { - // are we expecting an error? - if (testDefinition.error) { - if (!err) { - return done(new Error('Expected an error, but found none!')); + topology.selectServer(selector, { serverSelectionTimeoutMS: 50 }).then( + server => { + if (testDefinition.error) return done(new Error('Expected an error, but found none!')); + if (expectedServers.length === 0 && server !== null) { + return done(new Error('Found server, but expected none!')); } - return done(); - } - - if (err) { - // this is another expected error case - if (expectedServers.length === 0 && err instanceof MongoServerSelectionError) return done(); - return done(err); - } + const selectedServerDescription = server.description; - if (expectedServers.length === 0 && server !== null) { - return done(new Error('Found server, but expected none!')); - } + try { + const expectedServerArray = expectedServers.filter( + s => s.address === selectedServerDescription.address + ); - const selectedServerDescription = server.description; + if (!expectedServerArray.length) { + return done(new Error('No suitable servers found!')); + } - try { - const expectedServerArray = expectedServers.filter( - s => s.address === selectedServerDescription.address - ); + if (expectedServerArray.length > 1) { + return done(new Error('This test does not support multiple expected servers')); + } - if (!expectedServerArray.length) { - return done(new Error('No suitable servers found!')); + for (const [prop, value] of Object.entries(expectedServerArray[0])) { + if (prop === 'hosts') { + // we dynamically modify this prop during sever selection + continue; + } + expect(selectedServerDescription[prop]).to.deep.equal( + value, + `Mismatched selected server "${prop}"` + ); + } + done(); + } catch (e) { + done(e); } - - if (expectedServerArray.length > 1) { - return done(new Error('This test does not support multiple expected servers')); + }, + err => { + // are we expecting an error? + if (testDefinition.error) { + return done(); } - for (const [prop, value] of Object.entries(expectedServerArray[0])) { - if (prop === 'hosts') { - // we dynamically modify this prop during sever selection - continue; - } - expect(selectedServerDescription[prop]).to.deep.equal( - value, - `Mismatched selected server "${prop}"` - ); - } - done(); - } catch (e) { - done(e); + // this is another expected error case + if (expectedServers.length === 0 && err instanceof MongoServerSelectionError) return done(); + return done(err); } - }); + ); }); } diff --git a/test/unit/error.test.ts b/test/unit/error.test.ts index a4881150b0f..f5508e92d18 100644 --- a/test/unit/error.test.ts +++ b/test/unit/error.test.ts @@ -386,9 +386,7 @@ describe('MongoErrors', () => { return cleanup(err); } - topology.selectServer('primary', {}, (err, server) => { - expect(err).to.not.exist; - + topology.selectServer('primary', {}).then(server => { server .command(ns('db1'), Object.assign({}, RAW_USER_WRITE_CONCERN_CMD), {}) .then(expect.fail, err => { @@ -407,7 +405,7 @@ describe('MongoErrors', () => { cleanup(_err); } }); - }); + }, expect.fail); }); }); @@ -432,9 +430,7 @@ describe('MongoErrors', () => { return cleanup(err); } - topology.selectServer('primary', {}, (err, server) => { - expect(err).to.not.exist; - + topology.selectServer('primary', {}).then(server => { server .command(ns('db1'), Object.assign({}, RAW_USER_WRITE_CONCERN_CMD), {}) .then(expect.fail, err => { @@ -451,7 +447,7 @@ describe('MongoErrors', () => { cleanup(_err); } }); - }); + }, expect.fail); }); }); }); diff --git a/test/unit/sdam/server_selection.test.ts b/test/unit/sdam/server_selection.test.ts index c26f3b96510..8abe5f01fd1 100644 --- a/test/unit/sdam/server_selection.test.ts +++ b/test/unit/sdam/server_selection.test.ts @@ -628,19 +628,19 @@ describe('server selection', async function () { context('when willLog returns false', function () { const original = Object.getPrototypeOf(ServerSelectionEvent); let serverSelectionEventStub; - beforeEach(async () => { + beforeEach(() => { sinon.stub(MongoLogger.prototype, 'willLog').callsFake((_v, _w) => false); serverSelectionEventStub = sinon.stub(); Object.setPrototypeOf(ServerSelectionEvent, serverSelectionEventStub); }); - afterEach(async () => { + afterEach(() => { sinon.restore(); Object.setPrototypeOf(ServerSelectionEvent, original); }); - it('should not create server selection event instances', function () { - topology?.selectServer(topologyDescription, { operationName: 'test' }, v => v); + it('should not create server selection event instances', async function () { + await topology?.selectServer(topologyDescription, { operationName: 'test' }); expect(serverSelectionEventStub.getCall(0)).to.be.null; }); }); diff --git a/test/unit/sdam/topology.test.js b/test/unit/sdam/topology.test.js index edc594b967e..8d43ad526c0 100644 --- a/test/unit/sdam/topology.test.js +++ b/test/unit/sdam/topology.test.js @@ -1,4 +1,5 @@ 'use strict'; +test / unit / sdam / topolo; const { clearTimeout } = require('timers'); const mock = require('../../tools/mongodb-mock/index'); @@ -96,9 +97,7 @@ describe('Topology (unit)', function () { topology.connect(err => { expect(err).to.not.exist; - topology.selectServer('primary', {}, (err, server) => { - expect(err).to.not.exist; - + topology.selectServer('primary', {}).then(server => { server .command(ns('admin.$cmd'), { ping: 1 }, { socketTimeoutMS: 250 }) .then(expect.fail, err => { @@ -107,7 +106,7 @@ describe('Topology (unit)', function () { topology.close(); done(); }); - }); + }, expect.fail); }); }); }); @@ -157,9 +156,8 @@ describe('Topology (unit)', function () { sinon.stub(topology.s.servers, 'get').callsFake(() => { return undefined; }); - topology.selectServer('primary', {}, (err, server) => { + topology.selectServer('primary', {}).then(expect.fail, err => { expect(err).to.be.instanceOf(MongoServerSelectionError); - expect(server).not.to.exist; done(); }); }); @@ -177,9 +175,8 @@ describe('Topology (unit)', function () { sinon.stub(topology.s.servers, 'get').callsFake(() => { return undefined; }); - topology.selectServer('primary', {}, (err, server) => { + topology.selectServer('primary', {}).then(expect.fail, err => { expect(err).to.be.instanceOf(MongoServerSelectionError); - expect(server).not.to.exist; done(); }); }); @@ -204,9 +201,7 @@ describe('Topology (unit)', function () { topology.connect(err => { expect(err).to.not.exist; - topology.selectServer('primary', {}, (err, server) => { - expect(err).to.not.exist; - + topology.selectServer('primary', {}).then(server => { let serverDescription; server.on('descriptionReceived', sd => (serverDescription = sd)); @@ -219,7 +214,7 @@ describe('Topology (unit)', function () { expect(poolCleared).to.be.true; done(); }); - }); + }, expect.fail); }); }); @@ -239,9 +234,7 @@ describe('Topology (unit)', function () { topology.connect(err => { expect(err).to.not.exist; - topology.selectServer('primary', {}, (err, server) => { - expect(err).to.not.exist; - + topology.selectServer('primary', {}).then(server => { let serverDescription; server.on('descriptionReceived', sd => (serverDescription = sd)); @@ -255,7 +248,7 @@ describe('Topology (unit)', function () { topology.close(); done(); }); - }); + }, expect.fail); }); }); @@ -275,9 +268,7 @@ describe('Topology (unit)', function () { topology.connect(err => { expect(err).to.not.exist; - topology.selectServer('primary', {}, (err, server) => { - expect(err).to.not.exist; - + topology.selectServer('primary', {}).then(server => { let serverDescription; server.on('descriptionReceived', sd => (serverDescription = sd)); @@ -287,7 +278,7 @@ describe('Topology (unit)', function () { expect(server.description.type).to.equal('Unknown'); done(); }); - }); + }, expect.fail); }); }); @@ -457,10 +448,10 @@ describe('Topology (unit)', function () { // satisfy the initial connect, then restore the original method const selectServer = this.sinon .stub(Topology.prototype, 'selectServer') - .callsFake(function (selector, options, callback) { + .callsFake(function (selector, options) { const server = Array.from(this.s.servers.values())[0]; selectServer.restore(); - callback(null, server); + return Promise.resolve(server); }); this.sinon.stub(Server.prototype, 'connect').callsFake(function () { @@ -469,18 +460,19 @@ describe('Topology (unit)', function () { }); topology.connect(() => { - topology.selectServer(ReadPreference.secondary, { serverSelectionTimeoutMS: 1000 }, err => { - expect(err).to.exist; - expect(err).to.match(/Server selection timed out/); - expect(err).to.have.property('reason'); + topology + .selectServer(ReadPreference.secondary, { serverSelectionTimeoutMS: 1000 }) + .then(expect.fail, err => { + expect(err).to.exist; + expect(err).to.match(/Server selection timed out/); + expect(err).to.have.property('reason'); - // When server is created `connect` is called on the monitor. When server selection - // occurs `requestCheck` will be called for an immediate check. - expect(requestCheck).property('callCount').to.equal(1); + // When server is created `connect` is called on the monitor. When server selection + // occurs `requestCheck` will be called for an immediate check. + expect(requestCheck).property('callCount').to.equal(1); - topology.close(); - done(); - }); + topology.close({}, done); + }); }); }); @@ -500,14 +492,14 @@ describe('Topology (unit)', function () { }); describe('waitQueue', function () { - it('should process all wait queue members, including selection with errors', function (done) { + it('should process all wait queue members, including selection with errors', async function () { const topology = topologyWithPlaceholderClient('someserver:27019'); const selectServer = this.sinon .stub(Topology.prototype, 'selectServer') - .callsFake(function (selector, options, callback) { + .callsFake(async function (selector, options) { const server = Array.from(this.s.servers.values())[0]; selectServer.restore(); - callback(null, server); + return server; }); this.sinon.stub(Server.prototype, 'connect').callsFake(function () { @@ -517,11 +509,6 @@ describe('Topology (unit)', function () { const toSelect = 10; let completed = 0; - function finish() { - completed++; - if (completed === toSelect) done(); - } - // methodology: // - perform 9 server selections, a few with a selector that throws an error // - ensure each selection immediately returns an empty result (gated by a boolean) @@ -530,7 +517,7 @@ describe('Topology (unit)', function () { // returning their value // - verify that 10 callbacks were called - topology.connect(err => { + topology.connect(async err => { expect(err).to.not.exist; let preventSelection = true; @@ -547,11 +534,15 @@ describe('Topology (unit)', function () { preventSelection = true; for (let i = 0; i < toSelect - 1; ++i) { - topology.selectServer(i % 5 === 0 ? failingSelector : anySelector, {}, finish); + await topology.selectServer(i % 5 === 0 ? failingSelector : anySelector, {}); + completed++; } preventSelection = false; - topology.selectServer(anySelector, {}, finish); + await topology.selectServer(anySelector, {}); + completed++; + + expect(completed).to.equal(toSelect); }); }); }); From 1026ae94fe267a9f594b12750b0b668709cfe08d Mon Sep 17 00:00:00 2001 From: Warren James Date: Tue, 5 Mar 2024 15:31:28 -0500 Subject: [PATCH 08/40] fix types --- src/sdam/topology.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index b9baccc666a..488d43e6716 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -22,6 +22,7 @@ import { TOPOLOGY_OPENING } from '../constants'; import { + type AnyError, MongoCompatibilityError, type MongoDriverError, MongoError, @@ -107,7 +108,7 @@ export interface ServerSelectionRequest { transaction?: Transaction; startTime: number; resolve: (server: Server) => void; - reject: (reason?: any) => void; + reject: (error?: AnyError) => void; [kCancelled]?: boolean; timeoutController: TimeoutController; operationName: string; From 0b44449fa3326be3622e3bf7811dd9626063831f Mon Sep 17 00:00:00 2001 From: Warren James Date: Tue, 5 Mar 2024 15:31:38 -0500 Subject: [PATCH 09/40] eslint --- .../server_discovery_and_monitoring.spec.test.ts | 2 +- .../server_selection_latency_window_utils.ts | 1 - test/unit/assorted/server_selection_spec_helper.js | 12 +++++------- test/unit/sdam/topology.test.js | 4 ++-- 4 files changed, 8 insertions(+), 11 deletions(-) diff --git a/test/unit/assorted/server_discovery_and_monitoring.spec.test.ts b/test/unit/assorted/server_discovery_and_monitoring.spec.test.ts index 3638a29278c..b5c84b5afde 100644 --- a/test/unit/assorted/server_discovery_and_monitoring.spec.test.ts +++ b/test/unit/assorted/server_discovery_and_monitoring.spec.test.ts @@ -207,7 +207,7 @@ describe('Server Discovery and Monitoring (spec)', function () { topologySelectServers = sinon .stub(Topology.prototype, 'selectServer') - .callsFake(function (selector, options) { + .callsFake(function (_selector, _options) { topologySelectServers.restore(); const fakeServer = { s: { state: 'connected' }, removeListener: () => true }; diff --git a/test/unit/assorted/server_selection_latency_window_utils.ts b/test/unit/assorted/server_selection_latency_window_utils.ts index a72ada9d8f3..8247a51ca97 100644 --- a/test/unit/assorted/server_selection_latency_window_utils.ts +++ b/test/unit/assorted/server_selection_latency_window_utils.ts @@ -2,7 +2,6 @@ import { EJSON } from 'bson'; import { expect } from 'chai'; import { readdirSync, readFileSync } from 'fs'; import { join } from 'path'; -import { promisify } from 'util'; import { ReadPreference, diff --git a/test/unit/assorted/server_selection_spec_helper.js b/test/unit/assorted/server_selection_spec_helper.js index deed0bd5b8c..ee75b83674f 100644 --- a/test/unit/assorted/server_selection_spec_helper.js +++ b/test/unit/assorted/server_selection_spec_helper.js @@ -100,14 +100,12 @@ function executeServerSelectionTest(testDefinition, testDone) { const topology = topologyWithPlaceholderClient(seedData.seedlist, topologyOptions); // Each test will attempt to connect by doing server selection. We want to make the first // call to `selectServers` call a fake, and then immediately restore the original behavior. - let topologySelectServers = sinon - .stub(Topology.prototype, 'selectServer') - .callsFake(function (selector, options) { - topologySelectServers.restore(); + let topologySelectServers = sinon.stub(Topology.prototype, 'selectServer').callsFake(function () { + topologySelectServers.restore(); - const fakeServer = { s: { state: 'connected' }, removeListener: () => {} }; - return Promise.resolve(fakeServer); - }); + const fakeServer = { s: { state: 'connected' }, removeListener: () => {} }; + return Promise.resolve(fakeServer); + }); function done(err) { topology.close(); diff --git a/test/unit/sdam/topology.test.js b/test/unit/sdam/topology.test.js index 8d43ad526c0..45fbe9bbfcf 100644 --- a/test/unit/sdam/topology.test.js +++ b/test/unit/sdam/topology.test.js @@ -448,7 +448,7 @@ describe('Topology (unit)', function () { // satisfy the initial connect, then restore the original method const selectServer = this.sinon .stub(Topology.prototype, 'selectServer') - .callsFake(function (selector, options) { + .callsFake(function () { const server = Array.from(this.s.servers.values())[0]; selectServer.restore(); return Promise.resolve(server); @@ -496,7 +496,7 @@ describe('Topology (unit)', function () { const topology = topologyWithPlaceholderClient('someserver:27019'); const selectServer = this.sinon .stub(Topology.prototype, 'selectServer') - .callsFake(async function (selector, options) { + .callsFake(async function () { const server = Array.from(this.s.servers.values())[0]; selectServer.restore(); return server; From 1b8a19b289a2c9aa119cb0ae03c5285d5639876c Mon Sep 17 00:00:00 2001 From: Warren James Date: Fri, 8 Mar 2024 10:43:27 -0500 Subject: [PATCH 10/40] fix return --- src/sdam/topology.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index 488d43e6716..8769bd631e2 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -477,7 +477,8 @@ export class Topology extends TypedEventEmitter { callback?.(undefined, this); }, error => { - return this.close({ force: false }, () => exitWithError(error)); + this.close(); + return exitWithError(error); } ); } From 8d87204b3da3475dacd7ec89511209794226f52e Mon Sep 17 00:00:00 2001 From: Warren James Date: Fri, 8 Mar 2024 10:43:43 -0500 Subject: [PATCH 11/40] fix --- test/unit/sdam/topology.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/unit/sdam/topology.test.js b/test/unit/sdam/topology.test.js index 45fbe9bbfcf..fd675324c29 100644 --- a/test/unit/sdam/topology.test.js +++ b/test/unit/sdam/topology.test.js @@ -1,5 +1,4 @@ 'use strict'; -test / unit / sdam / topolo; const { clearTimeout } = require('timers'); const mock = require('../../tools/mongodb-mock/index'); From bfdaf520a835bd95b7823c442467feaf470d9edd Mon Sep 17 00:00:00 2001 From: Warren James Date: Fri, 8 Mar 2024 14:30:17 -0500 Subject: [PATCH 12/40] fix test --- test/unit/sdam/topology.test.js | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/test/unit/sdam/topology.test.js b/test/unit/sdam/topology.test.js index fd675324c29..0a08da0a5ee 100644 --- a/test/unit/sdam/topology.test.js +++ b/test/unit/sdam/topology.test.js @@ -470,7 +470,8 @@ describe('Topology (unit)', function () { // occurs `requestCheck` will be called for an immediate check. expect(requestCheck).property('callCount').to.equal(1); - topology.close({}, done); + topology.close(); + done(); }); }); }); @@ -483,11 +484,12 @@ describe('Topology (unit)', function () { }); topology.close(); - topology.selectServer(ReadPreference.primary, { serverSelectionTimeoutMS: 2000 }, err => { - expect(err).to.exist; - expect(err).to.match(/Topology is closed/); - done(); - }); + topology + .selectServer(ReadPreference.primary, { serverSelectionTimeoutMS: 2000 }) + .then(expect.fail, err => { + expect(err).to.match(/Topology is closed/); + done(); + }); }); describe('waitQueue', function () { From 91275efd6841bd3f0276a539c49b82f445ae2635 Mon Sep 17 00:00:00 2001 From: Warren James Date: Fri, 8 Mar 2024 14:30:43 -0500 Subject: [PATCH 13/40] remove import --- src/sdam/topology.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index 8769bd631e2..349e22ad121 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -1,5 +1,3 @@ -import { promisify } from 'util'; - import type { BSONSerializeOptions, Document } from '../bson'; import type { MongoCredentials } from '../cmap/auth/mongo_credentials'; import type { ConnectionEvents } from '../cmap/connection'; From af54e79bdde11ee7783cb65feeefebd9a4251eb2 Mon Sep 17 00:00:00 2001 From: Warren James Date: Fri, 8 Mar 2024 16:04:00 -0500 Subject: [PATCH 14/40] deprecate CloseOptions --- src/cmap/connection_pool.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/cmap/connection_pool.ts b/src/cmap/connection_pool.ts index 195ad4dc9c6..e0c0583d007 100644 --- a/src/cmap/connection_pool.ts +++ b/src/cmap/connection_pool.ts @@ -118,7 +118,9 @@ export const PoolState = Object.freeze({ closed: 'closed' } as const); -/** @public */ +/** @public + * @deprecated This interface is deprecated and will be removed in a future release as it is not used + * in the driver */ export interface CloseOptions { force?: boolean; } From 3f6ab2f142ea1664c2270eed6b6b4eb17db7abde Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 11 Mar 2024 13:51:36 -0400 Subject: [PATCH 15/40] add correct then handler --- src/change_stream.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/change_stream.ts b/src/change_stream.ts index bdcb1c0abd1..767a7d9746b 100644 --- a/src/change_stream.ts +++ b/src/change_stream.ts @@ -938,10 +938,12 @@ export class ChangeStream< .selectServer(this.cursor.readPreference, { operationName: 'reconnect topology in change stream' }) - .catch(serverSelectionError => { - if (serverSelectionError) return this._closeEmitterModeWithError(changeStreamError); - this.cursor = this._createChangeStreamCursor(this.cursor.resumeOptions); - }); + .then( + () => { + this.cursor = this._createChangeStreamCursor(this.cursor.resumeOptions); + }, + () => this._closeEmitterModeWithError(changeStreamError) + ); } else { this._closeEmitterModeWithError(changeStreamError); } From e033bd5562c31dd9859e6dc33e7a90710ac35c68 Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 11 Mar 2024 13:53:05 -0400 Subject: [PATCH 16/40] fix unit test --- test/unit/sdam/server_selection.test.ts | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/test/unit/sdam/server_selection.test.ts b/test/unit/sdam/server_selection.test.ts index 8abe5f01fd1..ede5bc45d17 100644 --- a/test/unit/sdam/server_selection.test.ts +++ b/test/unit/sdam/server_selection.test.ts @@ -611,14 +611,19 @@ describe('server selection', async function () { }); describe('server selection logging feature flagging', async function () { - const topologyDescription = sinon.stub(); - let mockServer; let topology; beforeEach(async () => { - mockServer = await mock.createServer(); + mockServer = await mock.createServer(27017, 'localhost'); topology = topologyWithPlaceholderClient(mockServer.hostAddress(), {}); + // NOTE: This is done to ensure that that processWaitQueueMember doesn't throw due to an + // invalid state + topology.s.state = 'connected'; + topology.s.servers.set('localhost:27017', mockServer); + topology.s.description.servers = new Map([ + [mockServer.address(), new ServerDescription('localhost:27017')] + ]); }); afterEach(async () => { @@ -640,7 +645,9 @@ describe('server selection', async function () { }); it('should not create server selection event instances', async function () { - await topology?.selectServer(topologyDescription, { operationName: 'test' }); + await topology?.selectServer(() => [new ServerDescription('localhost:27017')], { + operationName: 'test' + }); expect(serverSelectionEventStub.getCall(0)).to.be.null; }); }); From 6892362e7db4f8b5d0d2c21ba4b0fd1c60117d68 Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 11 Mar 2024 14:29:25 -0400 Subject: [PATCH 17/40] remove unintended deprecation --- src/cmap/connection_pool.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/cmap/connection_pool.ts b/src/cmap/connection_pool.ts index e0c0583d007..195ad4dc9c6 100644 --- a/src/cmap/connection_pool.ts +++ b/src/cmap/connection_pool.ts @@ -118,9 +118,7 @@ export const PoolState = Object.freeze({ closed: 'closed' } as const); -/** @public - * @deprecated This interface is deprecated and will be removed in a future release as it is not used - * in the driver */ +/** @public */ export interface CloseOptions { force?: boolean; } From cf07a5dbcd269512839b8aefcd6ffae7ee40cef0 Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 11 Mar 2024 15:58:13 -0400 Subject: [PATCH 18/40] convert Topology.connect to async/await --- src/mongo_client.ts | 3 +- src/sdam/topology.ts | 67 ++++++++++++++++++++++---------------------- 2 files changed, 34 insertions(+), 36 deletions(-) diff --git a/src/mongo_client.ts b/src/mongo_client.ts index f47558e443d..86ef54b57c5 100644 --- a/src/mongo_client.ts +++ b/src/mongo_client.ts @@ -1,7 +1,6 @@ import { promises as fs } from 'fs'; import type { TcpNetConnectOpts } from 'net'; import type { ConnectionOptions as TLSConnectionOptions, TLSSocketOptions } from 'tls'; -import { promisify } from 'util'; import { type BSONSerializeOptions, type Document, resolveBSONOptions } from './bson'; import { ChangeStream, type ChangeStreamDocument, type ChangeStreamOptions } from './change_stream'; @@ -550,7 +549,7 @@ export class MongoClient extends TypedEventEmitter { const topologyConnect = async () => { try { - await promisify(callback => this.topology?.connect(options, callback))(); + await this.topology?.connect(options); } catch (error) { this.topology?.close(); throw error; diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index 349e22ad121..5bf247ee9da 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -399,17 +399,10 @@ export class Topology extends TypedEventEmitter { } /** Initiate server connect */ - connect(callback: Callback): void; - connect(options: ConnectOptions, callback: Callback): void; - connect(options?: ConnectOptions | Callback, callback?: Callback): void { - if (typeof options === 'function') (callback = options), (options = {}); + async connect(options?: ConnectOptions): Promise { options = options ?? {}; if (this.s.state === STATE_CONNECTED) { - if (typeof callback === 'function') { - callback(); - } - - return; + return this; } stateTransition(this, STATE_CONNECTING); @@ -447,38 +440,44 @@ export class Topology extends TypedEventEmitter { } } - const exitWithError = (error: Error) => - callback ? callback(error) : this.emit(Topology.ERROR, error); + const exitWithError = (error: Error) => { + this.emit(Topology.ERROR, error); + throw error; + }; const readPreference = options.readPreference ?? ReadPreference.primary; const selectServerOptions = { operationName: 'ping', ...options }; + try { + const server = await this.selectServer( + readPreferenceServerSelector(readPreference), + selectServerOptions + ); - this.selectServer(readPreferenceServerSelector(readPreference), selectServerOptions).then( - server => { - const skipPingOnConnect = this.s.options[Symbol.for('@@mdb.skipPingOnConnect')] === true; - if (!skipPingOnConnect && server && this.s.credentials) { - server.command(ns('admin.$cmd'), { ping: 1 }, {}).then(() => { - stateTransition(this, STATE_CONNECTED); - this.emit(Topology.OPEN, this); - this.emit(Topology.CONNECT, this); - - callback?.(undefined, this); - }, exitWithError); - - return; + const skipPingOnConnect = this.s.options[Symbol.for('@@mdb.skipPingOnConnect')] === true; + if (!skipPingOnConnect && server && this.s.credentials) { + try { + await server.command(ns('admin.$cmd'), { ping: 1 }, {}); + stateTransition(this, STATE_CONNECTED); + this.emit(Topology.OPEN, this); + this.emit(Topology.CONNECT, this); + + return this; + } catch (error) { + exitWithError(error); } - stateTransition(this, STATE_CONNECTED); - this.emit(Topology.OPEN, this); - this.emit(Topology.CONNECT, this); - - callback?.(undefined, this); - }, - error => { - this.close(); - return exitWithError(error); + return this; } - ); + + stateTransition(this, STATE_CONNECTED); + this.emit(Topology.OPEN, this); + this.emit(Topology.CONNECT, this); + + return this; + } catch (error) { + this.close(); + return exitWithError(error); + } } /** Close this topology */ From 94539c51023064569d9391f5ff5e14d52ae72be7 Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 11 Mar 2024 15:58:47 -0400 Subject: [PATCH 19/40] fix unit tests --- test/tools/start_load_balanced.sh | 31 +++++++++++++ ...records_for_mongos_discovery.prose.test.ts | 8 +--- .../assorted/server_selection_spec_helper.js | 6 +-- test/unit/sdam/server_selection.test.ts | 14 +++--- test/unit/sdam/topology.test.js | 43 +++++++------------ 5 files changed, 59 insertions(+), 43 deletions(-) create mode 100755 test/tools/start_load_balanced.sh diff --git a/test/tools/start_load_balanced.sh b/test/tools/start_load_balanced.sh new file mode 100755 index 00000000000..28d7d8630af --- /dev/null +++ b/test/tools/start_load_balanced.sh @@ -0,0 +1,31 @@ +#!/bin/bash +# +DATA_DIR=${DATA_DIR:-data} +LOADBALANCED_DIR=${LOADBALANCED_DIR:-$DATA_DIR/load_balanced} +DRIVERS_TOOLS=${DRIVERS_TOOLS:-../drivers-tools/} + +mkdir -p $LOADBALANCED_DIR +mlaunch init --dir data --ipv6 --replicaset --nodes 2 --port 51000 --name testing --setParameter enableTestCommands=1 --sharded 1 --mongos 2 +mlaunch stop + +# Update .mlaunch_startup file +TMP_MLAUNCH_STARTUP=$(mktemp) + +jq '.startup_info."51000"=(.startup_info."51000" + " --setParameter \"loadBalancerPort=27050\"") | .startup_info."51001"=(.startup_info."51001" + " --setParameter \"loadBalancerPort=27051\"")' $DATA_DIR/.mlaunch_startup > $TMP_MLAUNCH_STARTUP +mv $TMP_MLAUNCH_STARTUP $DATA_DIR/.mlaunch_startup + +mlaunch start +export MONGODB_URI="mongodb://bob:pwd123@localhost:51000,localhost:51001" +echo $MONGODB_URI + +$DRIVERS_TOOLS/.evergreen/run-load-balancer.sh start + +# generate env file +cat lb-expansion.yml | sed 's/: /=/g' > lb.env + +source lb.env +export SINGLE_MONGOS_LB_URI +export MULTI_MONGOS_LB_URI + +export LOAD_BALANCER=true +export AUTH=noauth diff --git a/test/unit/assorted/polling_srv_records_for_mongos_discovery.prose.test.ts b/test/unit/assorted/polling_srv_records_for_mongos_discovery.prose.test.ts index 0dfde6ba2d7..8dea4bf89eb 100644 --- a/test/unit/assorted/polling_srv_records_for_mongos_discovery.prose.test.ts +++ b/test/unit/assorted/polling_srv_records_for_mongos_discovery.prose.test.ts @@ -144,11 +144,7 @@ describe('Polling Srv Records for Mongos Discovery', () => { } as TopologyOptions); const topology = context.topology; - topology.connect({}, err => { - if (err) { - return done(err); - } - + topology.connect({}).then(() => { try { expect(topology.description).to.have.property('type', TopologyType.Sharded); const servers = Array.from(topology.description.servers.keys()); @@ -167,7 +163,7 @@ describe('Polling Srv Records for Mongos Discovery', () => { } catch (e) { done(e); } - }); + }, done); } // The addition of a new DNS record: diff --git a/test/unit/assorted/server_selection_spec_helper.js b/test/unit/assorted/server_selection_spec_helper.js index ee75b83674f..01916865980 100644 --- a/test/unit/assorted/server_selection_spec_helper.js +++ b/test/unit/assorted/server_selection_spec_helper.js @@ -112,9 +112,7 @@ function executeServerSelectionTest(testDefinition, testDone) { testDone(err); } - topology.connect(err => { - expect(err).to.not.exist; - + topology.connect().then(() => { // Update topologies with server descriptions. topologyDescription.servers.forEach(server => { const serverDescription = serverDescriptionFromDefinition(server, seedData.hosts); @@ -195,7 +193,7 @@ function executeServerSelectionTest(testDefinition, testDone) { return done(err); } ); - }); + }, expect.fail); } module.exports = { executeServerSelectionTest, serverDescriptionFromDefinition }; diff --git a/test/unit/sdam/server_selection.test.ts b/test/unit/sdam/server_selection.test.ts index ede5bc45d17..d815d9bd201 100644 --- a/test/unit/sdam/server_selection.test.ts +++ b/test/unit/sdam/server_selection.test.ts @@ -613,16 +613,18 @@ describe('server selection', async function () { describe('server selection logging feature flagging', async function () { let mockServer; let topology; + let address; beforeEach(async () => { - mockServer = await mock.createServer(27017, 'localhost'); + mockServer = await mock.createServer(undefined, 'localhost'); topology = topologyWithPlaceholderClient(mockServer.hostAddress(), {}); - // NOTE: This is done to ensure that that processWaitQueueMember doesn't throw due to an - // invalid state + // NOTE: This is done to ensure that that processWaitQueueMember doesn't throw due to the + // topology being in an invalid state + address = `localhost:${mockServer.port}`; topology.s.state = 'connected'; - topology.s.servers.set('localhost:27017', mockServer); + topology.s.servers.set(address, mockServer); topology.s.description.servers = new Map([ - [mockServer.address(), new ServerDescription('localhost:27017')] + [address, new ServerDescription(mockServer.hostAddress())] ]); }); @@ -645,7 +647,7 @@ describe('server selection', async function () { }); it('should not create server selection event instances', async function () { - await topology?.selectServer(() => [new ServerDescription('localhost:27017')], { + await topology?.selectServer(() => [new ServerDescription(address)], { operationName: 'test' }); expect(serverSelectionEventStub.getCall(0)).to.be.null; diff --git a/test/unit/sdam/topology.test.js b/test/unit/sdam/topology.test.js index 0a08da0a5ee..064602bcbf8 100644 --- a/test/unit/sdam/topology.test.js +++ b/test/unit/sdam/topology.test.js @@ -93,9 +93,7 @@ describe('Topology (unit)', function () { }); const topology = topologyWithPlaceholderClient(mockServer.hostAddress()); - topology.connect(err => { - expect(err).to.not.exist; - + topology.connect().then(() => { topology.selectServer('primary', {}).then(server => { server .command(ns('admin.$cmd'), { ping: 1 }, { socketTimeoutMS: 250 }) @@ -106,7 +104,7 @@ describe('Topology (unit)', function () { done(); }); }, expect.fail); - }); + }, expect.fail); }); }); @@ -150,8 +148,7 @@ describe('Topology (unit)', function () { secondMockServer.hostAddress() ]); - topology.connect(err => { - expect(err).to.not.exist; + topology.connect().then(_ => { sinon.stub(topology.s.servers, 'get').callsFake(() => { return undefined; }); @@ -159,7 +156,7 @@ describe('Topology (unit)', function () { expect(err).to.be.instanceOf(MongoServerSelectionError); done(); }); - }); + }, expect.fail); }); }); context('when the topology originally contained more than one server', function () { @@ -169,8 +166,7 @@ describe('Topology (unit)', function () { secondMockServer.hostAddress() ]); - topology.connect(err => { - expect(err).to.not.exist; + topology.connect().then(() => { sinon.stub(topology.s.servers, 'get').callsFake(() => { return undefined; }); @@ -178,7 +174,7 @@ describe('Topology (unit)', function () { expect(err).to.be.instanceOf(MongoServerSelectionError); done(); }); - }); + }, expect.fail); }); }); } @@ -197,9 +193,7 @@ describe('Topology (unit)', function () { }); topology = topologyWithPlaceholderClient(mockServer.hostAddress()); - topology.connect(err => { - expect(err).to.not.exist; - + topology.connect().then(() => { topology.selectServer('primary', {}).then(server => { let serverDescription; server.on('descriptionReceived', sd => (serverDescription = sd)); @@ -214,7 +208,7 @@ describe('Topology (unit)', function () { done(); }); }, expect.fail); - }); + }, expect.fail); }); it('should set server to unknown and NOT reset pool on stepdown errors', function (done) { @@ -230,9 +224,7 @@ describe('Topology (unit)', function () { }); const topology = topologyWithPlaceholderClient(mockServer.hostAddress()); - topology.connect(err => { - expect(err).to.not.exist; - + topology.connect().then(() => { topology.selectServer('primary', {}).then(server => { let serverDescription; server.on('descriptionReceived', sd => (serverDescription = sd)); @@ -248,7 +240,7 @@ describe('Topology (unit)', function () { done(); }); }, expect.fail); - }); + }, expect.fail); }); it('should set server to unknown on non-timeout network error', function (done) { @@ -264,9 +256,7 @@ describe('Topology (unit)', function () { }); topology = topologyWithPlaceholderClient(mockServer.hostAddress()); - topology.connect(err => { - expect(err).to.not.exist; - + topology.connect().then(() => { topology.selectServer('primary', {}).then(server => { let serverDescription; server.on('descriptionReceived', sd => (serverDescription = sd)); @@ -278,7 +268,7 @@ describe('Topology (unit)', function () { done(); }); }, expect.fail); - }); + }, expect.fail); }); it('should encounter a server selection timeout on garbled server responses', function (done) { @@ -456,9 +446,10 @@ describe('Topology (unit)', function () { this.sinon.stub(Server.prototype, 'connect').callsFake(function () { this.s.state = 'connected'; this.emit('connect'); + return Promise.resolve(); }); - topology.connect(() => { + topology.connect().then(() => { topology .selectServer(ReadPreference.secondary, { serverSelectionTimeoutMS: 1000 }) .then(expect.fail, err => { @@ -518,9 +509,7 @@ describe('Topology (unit)', function () { // returning their value // - verify that 10 callbacks were called - topology.connect(async err => { - expect(err).to.not.exist; - + topology.connect().then(async () => { let preventSelection = true; const anySelector = td => { if (preventSelection) return []; @@ -544,7 +533,7 @@ describe('Topology (unit)', function () { completed++; expect(completed).to.equal(toSelect); - }); + }, expect.fail); }); }); }); From 331d124834ff6e9d1bba96e373119e5a55ce8089 Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 11 Mar 2024 16:34:21 -0400 Subject: [PATCH 20/40] remove unneeded script --- test/tools/start_load_balanced.sh | 31 ------------------------------- 1 file changed, 31 deletions(-) delete mode 100755 test/tools/start_load_balanced.sh diff --git a/test/tools/start_load_balanced.sh b/test/tools/start_load_balanced.sh deleted file mode 100755 index 28d7d8630af..00000000000 --- a/test/tools/start_load_balanced.sh +++ /dev/null @@ -1,31 +0,0 @@ -#!/bin/bash -# -DATA_DIR=${DATA_DIR:-data} -LOADBALANCED_DIR=${LOADBALANCED_DIR:-$DATA_DIR/load_balanced} -DRIVERS_TOOLS=${DRIVERS_TOOLS:-../drivers-tools/} - -mkdir -p $LOADBALANCED_DIR -mlaunch init --dir data --ipv6 --replicaset --nodes 2 --port 51000 --name testing --setParameter enableTestCommands=1 --sharded 1 --mongos 2 -mlaunch stop - -# Update .mlaunch_startup file -TMP_MLAUNCH_STARTUP=$(mktemp) - -jq '.startup_info."51000"=(.startup_info."51000" + " --setParameter \"loadBalancerPort=27050\"") | .startup_info."51001"=(.startup_info."51001" + " --setParameter \"loadBalancerPort=27051\"")' $DATA_DIR/.mlaunch_startup > $TMP_MLAUNCH_STARTUP -mv $TMP_MLAUNCH_STARTUP $DATA_DIR/.mlaunch_startup - -mlaunch start -export MONGODB_URI="mongodb://bob:pwd123@localhost:51000,localhost:51001" -echo $MONGODB_URI - -$DRIVERS_TOOLS/.evergreen/run-load-balancer.sh start - -# generate env file -cat lb-expansion.yml | sed 's/: /=/g' > lb.env - -source lb.env -export SINGLE_MONGOS_LB_URI -export MULTI_MONGOS_LB_URI - -export LOAD_BALANCER=true -export AUTH=noauth From 5e4182b068d40afe96ed4f771bc9f606933d2a29 Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 11 Mar 2024 16:43:03 -0400 Subject: [PATCH 21/40] fix uri test --- test/integration/uri-options/uri.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/uri-options/uri.test.js b/test/integration/uri-options/uri.test.js index a7c64b9c6d9..abf1198f7a8 100644 --- a/test/integration/uri-options/uri.test.js +++ b/test/integration/uri-options/uri.test.js @@ -135,14 +135,14 @@ describe('URI', function () { expect(options.credentials.mechanism).to.eql('MONGODB-X509'); connectStub.restore(); - done(); + return Promise.resolve(); } const topologyPrototype = Topology.prototype; const connectStub = sinon.stub(topologyPrototype, 'connect').callsFake(validateConnect); const uri = 'mongodb://some-hostname/test?ssl=true&authMechanism=MONGODB-X509&replicaSet=rs0'; const client = this.configuration.newClient(uri); - client.connect(); + client.connect().finally(client.close).finally(done); } }); }); From 3d2e854235a504987cecbc83e74a6c1c7b725f1f Mon Sep 17 00:00:00 2001 From: Warren James Date: Tue, 12 Mar 2024 13:14:57 -0400 Subject: [PATCH 22/40] lint fix --- test/unit/sdam/topology.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/sdam/topology.test.js b/test/unit/sdam/topology.test.js index 064602bcbf8..a244fbb9e8d 100644 --- a/test/unit/sdam/topology.test.js +++ b/test/unit/sdam/topology.test.js @@ -148,7 +148,7 @@ describe('Topology (unit)', function () { secondMockServer.hostAddress() ]); - topology.connect().then(_ => { + topology.connect().then(() => { sinon.stub(topology.s.servers, 'get').callsFake(() => { return undefined; }); From 988f549ed26ce7e87c67ad8705b96ad9a5c56195 Mon Sep 17 00:00:00 2001 From: Warren James Date: Tue, 12 Mar 2024 13:16:28 -0400 Subject: [PATCH 23/40] lint fix --- test/unit/sdam/topology.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/sdam/topology.test.js b/test/unit/sdam/topology.test.js index a244fbb9e8d..6088913a150 100644 --- a/test/unit/sdam/topology.test.js +++ b/test/unit/sdam/topology.test.js @@ -148,7 +148,7 @@ describe('Topology (unit)', function () { secondMockServer.hostAddress() ]); - topology.connect().then(() => { + topology.connect().then(() => { sinon.stub(topology.s.servers, 'get').callsFake(() => { return undefined; }); From df1d677c9ea55436bf02d4ddd3fa97e58d6dd52f Mon Sep 17 00:00:00 2001 From: Warren James Date: Tue, 12 Mar 2024 13:23:26 -0400 Subject: [PATCH 24/40] fix test --- test/integration/uri-options/uri.test.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/integration/uri-options/uri.test.js b/test/integration/uri-options/uri.test.js index abf1198f7a8..6b58d6ac59c 100644 --- a/test/integration/uri-options/uri.test.js +++ b/test/integration/uri-options/uri.test.js @@ -142,7 +142,10 @@ describe('URI', function () { const connectStub = sinon.stub(topologyPrototype, 'connect').callsFake(validateConnect); const uri = 'mongodb://some-hostname/test?ssl=true&authMechanism=MONGODB-X509&replicaSet=rs0'; const client = this.configuration.newClient(uri); - client.connect().finally(client.close).finally(done); + client.connect((err, client) => { + expect(err).to.not.exist; + client.close(done); + }); } }); }); From e485ec58adba13d2e4a46720383d2883b2d04dda Mon Sep 17 00:00:00 2001 From: Warren James Date: Tue, 12 Mar 2024 14:18:00 -0400 Subject: [PATCH 25/40] add connectionLock --- src/sdam/topology.ts | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index 5bf247ee9da..fd0ba0b3180 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -216,6 +216,9 @@ export class Topology extends TypedEventEmitter { client!: MongoClient; + /** @internal */ + private connectionLock?: Promise; + /** @event */ static readonly SERVER_OPENING = SERVER_OPENING; /** @event */ @@ -339,6 +342,7 @@ export class Topology extends TypedEventEmitter { this.on(Topology.TOPOLOGY_DESCRIPTION_CHANGED, this.s.detectShardedTopology); } + this.connectionLock = undefined; } private detectShardedTopology(event: TopologyDescriptionChangedEvent) { @@ -400,6 +404,22 @@ export class Topology extends TypedEventEmitter { /** Initiate server connect */ async connect(options?: ConnectOptions): Promise { + if (this.connectionLock) { + return this.connectionLock; + } + + try { + this.connectionLock = this._connect(options); + await this.connectionLock; + } finally { + // release + this.connectionLock = undefined; + } + + return this; + } + + private async _connect(options?: ConnectOptions): Promise { options = options ?? {}; if (this.s.state === STATE_CONNECTED) { return this; From 09814fb3c0b248396321a2ed4580b59a919d4a9a Mon Sep 17 00:00:00 2001 From: Warren James Date: Tue, 12 Mar 2024 15:19:26 -0400 Subject: [PATCH 26/40] fix find test --- test/integration/crud/find.test.js | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/test/integration/crud/find.test.js b/test/integration/crud/find.test.js index c930a86a4ea..5de6a427bda 100644 --- a/test/integration/crud/find.test.js +++ b/test/integration/crud/find.test.js @@ -2327,15 +2327,11 @@ describe('Find', function () { let selectedServer; const topology = client.topology; - const selectServerStub = sinon.stub(topology, 'selectServer').callsFake(function () { + const selectServerStub = sinon.stub(topology, 'selectServer').callsFake(async function () { const args = Array.prototype.slice.call(arguments); - const originalCallback = args.pop(); - args.push((err, server) => { - selectedServer = server; - originalCallback(err, server); - }); - - return topology.selectServer.wrappedMethod.apply(this, args); + const server = topology.selectServer.wrappedMethod.apply(this, args); + selectedServer = await server; + return selectedServer; }); const collection = client.db().collection('test_read_preference'); From 99ec556ad29be1450245c9494934532fd63a45f7 Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 13 Mar 2024 11:20:15 -0400 Subject: [PATCH 27/40] Apply suggestions from code review Co-authored-by: Neal Beeken --- test/integration/uri-options/uri.test.js | 5 +---- .../polling_srv_records_for_mongos_discovery.prose.test.ts | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/test/integration/uri-options/uri.test.js b/test/integration/uri-options/uri.test.js index 6b58d6ac59c..d8fd3c14620 100644 --- a/test/integration/uri-options/uri.test.js +++ b/test/integration/uri-options/uri.test.js @@ -142,10 +142,7 @@ describe('URI', function () { const connectStub = sinon.stub(topologyPrototype, 'connect').callsFake(validateConnect); const uri = 'mongodb://some-hostname/test?ssl=true&authMechanism=MONGODB-X509&replicaSet=rs0'; const client = this.configuration.newClient(uri); - client.connect((err, client) => { - expect(err).to.not.exist; - client.close(done); - }); + return client.connect().finally(() => client.close()); } }); }); diff --git a/test/unit/assorted/polling_srv_records_for_mongos_discovery.prose.test.ts b/test/unit/assorted/polling_srv_records_for_mongos_discovery.prose.test.ts index 8dea4bf89eb..be0989cb453 100644 --- a/test/unit/assorted/polling_srv_records_for_mongos_discovery.prose.test.ts +++ b/test/unit/assorted/polling_srv_records_for_mongos_discovery.prose.test.ts @@ -144,7 +144,7 @@ describe('Polling Srv Records for Mongos Discovery', () => { } as TopologyOptions); const topology = context.topology; - topology.connect({}).then(() => { + return topology.connect({}).then(() => { try { expect(topology.description).to.have.property('type', TopologyType.Sharded); const servers = Array.from(topology.description.servers.keys()); From 83deb48316585982f883495189b9becd5a2bab72 Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 13 Mar 2024 11:23:00 -0400 Subject: [PATCH 28/40] test refactor fixes --- test/integration/uri-options/uri.test.js | 2 +- test/unit/assorted/max_staleness.spec.test.js | 4 +- ...rver_discovery_and_monitoring.spec.test.ts | 5 +- .../server_selection_latency_window_utils.ts | 2 +- .../assorted/server_selection_spec_helper.js | 154 ++++++++---------- test/unit/error.test.ts | 59 +++---- test/unit/sdam/topology.test.js | 57 ++++--- 7 files changed, 133 insertions(+), 150 deletions(-) diff --git a/test/integration/uri-options/uri.test.js b/test/integration/uri-options/uri.test.js index d8fd3c14620..299b0011e66 100644 --- a/test/integration/uri-options/uri.test.js +++ b/test/integration/uri-options/uri.test.js @@ -129,7 +129,7 @@ describe('URI', function () { it('should generate valid credentials with X509', { metadata: { requires: { topology: 'single' } }, - test: function (done) { + test: function () { function validateConnect(options) { expect(options).to.have.property('credentials'); expect(options.credentials.mechanism).to.eql('MONGODB-X509'); diff --git a/test/unit/assorted/max_staleness.spec.test.js b/test/unit/assorted/max_staleness.spec.test.js index 969d45f5adc..c31b225c0cf 100644 --- a/test/unit/assorted/max_staleness.spec.test.js +++ b/test/unit/assorted/max_staleness.spec.test.js @@ -47,8 +47,8 @@ describe('Max Staleness (spec)', function () { Object.keys(specTests).forEach(specTestName => { describe(specTestName, () => { specTests[specTestName].forEach(testData => { - it(testData.description, function (done) { - executeServerSelectionTest(testData, done); + it(testData.description, async function () { + return executeServerSelectionTest(testData); }); }); }); diff --git a/test/unit/assorted/server_discovery_and_monitoring.spec.test.ts b/test/unit/assorted/server_discovery_and_monitoring.spec.test.ts index b5c84b5afde..bafa26074a1 100644 --- a/test/unit/assorted/server_discovery_and_monitoring.spec.test.ts +++ b/test/unit/assorted/server_discovery_and_monitoring.spec.test.ts @@ -206,12 +206,11 @@ describe('Server Discovery and Monitoring (spec)', function () { // call to `selectServers` call a fake, and then immediately restore the original behavior. topologySelectServers = sinon .stub(Topology.prototype, 'selectServer') - - .callsFake(function (_selector, _options) { + .callsFake(async function (_selector, _options) { topologySelectServers.restore(); const fakeServer = { s: { state: 'connected' }, removeListener: () => true }; - return Promise.resolve(fakeServer); + return fakeServer; }); }); diff --git a/test/unit/assorted/server_selection_latency_window_utils.ts b/test/unit/assorted/server_selection_latency_window_utils.ts index 8247a51ca97..8b6fd7e66ce 100644 --- a/test/unit/assorted/server_selection_latency_window_utils.ts +++ b/test/unit/assorted/server_selection_latency_window_utils.ts @@ -138,7 +138,7 @@ export async function runServerSelectionLatencyWindowTest(test: ServerSelectionL const selectedServers: Server[] = []; for (let i = 0; i < test.iterations; ++i) { - const server: Server = await topology.selectServer.bind(topology)(ReadPreference.NEAREST, {}); + const server: Server = await topology.selectServer(ReadPreference.NEAREST, {}); selectedServers.push(server); } diff --git a/test/unit/assorted/server_selection_spec_helper.js b/test/unit/assorted/server_selection_spec_helper.js index 01916865980..d1eedad8a73 100644 --- a/test/unit/assorted/server_selection_spec_helper.js +++ b/test/unit/assorted/server_selection_spec_helper.js @@ -80,7 +80,7 @@ function readPreferenceFromDefinition(definition) { return new ReadPreference(mode, tags, options); } -function executeServerSelectionTest(testDefinition, testDone) { +async function executeServerSelectionTest(testDefinition) { const topologyDescription = testDefinition.topology_description; const seedData = topologyDescription.servers.reduce( (result, seed) => { @@ -100,100 +100,86 @@ function executeServerSelectionTest(testDefinition, testDone) { const topology = topologyWithPlaceholderClient(seedData.seedlist, topologyOptions); // Each test will attempt to connect by doing server selection. We want to make the first // call to `selectServers` call a fake, and then immediately restore the original behavior. - let topologySelectServers = sinon.stub(Topology.prototype, 'selectServer').callsFake(function () { - topologySelectServers.restore(); + let topologySelectServers = sinon + .stub(Topology.prototype, 'selectServer') + .callsFake(async function () { + topologySelectServers.restore(); - const fakeServer = { s: { state: 'connected' }, removeListener: () => {} }; - return Promise.resolve(fakeServer); + const fakeServer = { s: { state: 'connected' }, removeListener: () => {} }; + return fakeServer; + }); + + await topology.connect(); + topologyDescription.servers.forEach(server => { + const serverDescription = serverDescriptionFromDefinition(server, seedData.hosts); + topology.serverUpdateHandler(serverDescription); }); - function done(err) { - topology.close(); - testDone(err); + let selector; + if (testDefinition.operation === 'write') { + selector = ServerSelectors.writableServerSelector(); + } else if (testDefinition.operation === 'read' || testDefinition.read_preference) { + try { + const readPreference = readPreferenceFromDefinition(testDefinition.read_preference); + selector = ServerSelectors.readPreferenceServerSelector(readPreference); + } catch (e) { + if (testDefinition.error) return; + throw e; + } + } else { + throw new Error('received neither read nor write, and did not receive a read preference'); } - topology.connect().then(() => { - // Update topologies with server descriptions. - topologyDescription.servers.forEach(server => { - const serverDescription = serverDescriptionFromDefinition(server, seedData.hosts); - topology.serverUpdateHandler(serverDescription); - }); + // expectations + let expectedServers; + if (!testDefinition.error) { + expectedServers = testDefinition.in_latency_window.map(s => serverDescriptionFromDefinition(s)); + } - let selector; - if (testDefinition.operation === 'write') { - selector = ServerSelectors.writableServerSelector(); - } else if (testDefinition.operation === 'read' || testDefinition.read_preference) { - try { - const readPreference = readPreferenceFromDefinition(testDefinition.read_preference); - selector = ServerSelectors.readPreferenceServerSelector(readPreference); - } catch (e) { - if (testDefinition.error) return done(); - return done(e); - } - } else { - return done( - new Error('received neither read nor write, and did not receive a read preference') - ); + // default to serverSelectionTimeoutMS of `100` for unit tests + try { + const server = await topology.selectServer(selector, { serverSelectionTimeoutMS: 50 }); + + if (testDefinition.error) throw new Error('Expected an error, but found none!'); + if (expectedServers.length === 0 && server !== null) { + throw new Error('Found server, but expected none!'); } - // expectations - let expectedServers; - if (!testDefinition.error) { - expectedServers = testDefinition.in_latency_window.map(s => - serverDescriptionFromDefinition(s) - ); + const selectedServerDescription = server.description; + + const expectedServerArray = expectedServers.filter( + s => s.address === selectedServerDescription.address + ); + + if (!expectedServerArray.length) { + throw new Error('No suitable servers found!'); } - // default to serverSelectionTimeoutMS of `100` for unit tests - topology.selectServer(selector, { serverSelectionTimeoutMS: 50 }).then( - server => { - if (testDefinition.error) return done(new Error('Expected an error, but found none!')); - if (expectedServers.length === 0 && server !== null) { - return done(new Error('Found server, but expected none!')); - } - - const selectedServerDescription = server.description; - - try { - const expectedServerArray = expectedServers.filter( - s => s.address === selectedServerDescription.address - ); - - if (!expectedServerArray.length) { - return done(new Error('No suitable servers found!')); - } - - if (expectedServerArray.length > 1) { - return done(new Error('This test does not support multiple expected servers')); - } - - for (const [prop, value] of Object.entries(expectedServerArray[0])) { - if (prop === 'hosts') { - // we dynamically modify this prop during sever selection - continue; - } - expect(selectedServerDescription[prop]).to.deep.equal( - value, - `Mismatched selected server "${prop}"` - ); - } - done(); - } catch (e) { - done(e); - } - }, - err => { - // are we expecting an error? - if (testDefinition.error) { - return done(); - } - - // this is another expected error case - if (expectedServers.length === 0 && err instanceof MongoServerSelectionError) return done(); - return done(err); + if (expectedServerArray.length > 1) { + throw new Error('This test does not support multiple expected servers'); + } + + for (const [prop, value] of Object.entries(expectedServerArray[0])) { + if (prop === 'hosts') { + // we dynamically modify this prop during sever selection + continue; } - ); - }, expect.fail); + expect(selectedServerDescription[prop]).to.deep.equal( + value, + `Mismatched selected server "${prop}"` + ); + } + return; + } catch (err) { + // are we expecting an error? + if (testDefinition.error) { + return; + } + + // this is another expected error case + if (expectedServers.length === 0 && err instanceof MongoServerSelectionError) return; + throw err; + } } module.exports = { executeServerSelectionTest, serverDescriptionFromDefinition }; diff --git a/test/unit/error.test.ts b/test/unit/error.test.ts index f5508e92d18..fc674c32951 100644 --- a/test/unit/error.test.ts +++ b/test/unit/error.test.ts @@ -365,7 +365,7 @@ describe('MongoErrors', () => { replSet.connect(); } - it('should expose a user command writeConcern error like a normal WriteConcernError', function (done) { + it('should expose a user command writeConcern error like a normal WriteConcernError', function () { test.primaryServer.setMessageHandler(request => { const doc = request.document; if (isHello(doc)) { @@ -374,39 +374,32 @@ describe('MongoErrors', () => { setTimeout(() => request.reply(RAW_USER_WRITE_CONCERN_ERROR)); } }); + const replSet = topologyWithPlaceholderClient( + [test.primaryServer.hostAddress(), test.firstSecondaryServer.hostAddress()], + { replicaSet: 'rs' } as TopologyOptions + ); - makeAndConnectReplSet((err, topology) => { - // cleanup the server before calling done - const cleanup = err => { - topology.close(); - done(err); - }; - - if (err) { - return cleanup(err); - } - - topology.selectServer('primary', {}).then(server => { - server - .command(ns('db1'), Object.assign({}, RAW_USER_WRITE_CONCERN_CMD), {}) - .then(expect.fail, err => { - let _err; - try { - expect(err).to.be.an.instanceOf(MongoWriteConcernError); - expect(err.result).to.exist; - expect(err.result).to.have.property('ok', 1); - expect(err.result).to.not.have.property('errmsg'); - expect(err.result).to.not.have.property('code'); - expect(err.result).to.not.have.property('codeName'); - expect(err.result).to.have.property('writeConcernError'); - } catch (e) { - _err = e; - } finally { - cleanup(_err); - } - }); - }, expect.fail); - }); + replSet + .connect() + .then(topology => topology.selectServer('primary', {})) + .then(server => + server.command(ns('db1'), Object.assign({}, RAW_USER_WRITE_CONCERN_CMD), {}) + ) + .then( + () => expect.fail('expected command to fail'), + err => { + expect(err).to.be.an.instanceOf(MongoWriteConcernError); + expect(err.result).to.exist; + expect(err.result).to.have.property('ok', 1); + expect(err.result).to.not.have.property('errmsg'); + expect(err.result).to.not.have.property('code'); + expect(err.result).to.not.have.property('codeName'); + expect(err.result).to.have.property('writeConcernError'); + } + ) + .finally(() => { + replSet.close(); + }); }); it('should propagate writeConcernError.errInfo ', function (done) { diff --git a/test/unit/sdam/topology.test.js b/test/unit/sdam/topology.test.js index 6088913a150..4ac9e1e4971 100644 --- a/test/unit/sdam/topology.test.js +++ b/test/unit/sdam/topology.test.js @@ -484,13 +484,19 @@ describe('Topology (unit)', function () { }); describe('waitQueue', function () { + let selectServer; + let topology; + afterEach(() => { + selectServer.restore(); + topology.close(); + }); + it('should process all wait queue members, including selection with errors', async function () { - const topology = topologyWithPlaceholderClient('someserver:27019'); - const selectServer = this.sinon + topology = topologyWithPlaceholderClient('someserver:27019'); + selectServer = this.sinon .stub(Topology.prototype, 'selectServer') .callsFake(async function () { const server = Array.from(this.s.servers.values())[0]; - selectServer.restore(); return server; }); @@ -509,31 +515,30 @@ describe('Topology (unit)', function () { // returning their value // - verify that 10 callbacks were called - topology.connect().then(async () => { - let preventSelection = true; - const anySelector = td => { - if (preventSelection) return []; - const server = Array.from(td.servers.values())[0]; - return [server]; - }; - - const failingSelector = () => { - if (preventSelection) return []; - throw new TypeError('bad news!'); - }; - - preventSelection = true; - for (let i = 0; i < toSelect - 1; ++i) { - await topology.selectServer(i % 5 === 0 ? failingSelector : anySelector, {}); - completed++; - } - - preventSelection = false; - await topology.selectServer(anySelector, {}); + await topology.connect(); + + let preventSelection = true; + const anySelector = td => { + if (preventSelection) return []; + const server = Array.from(td.servers.values())[0]; + return [server]; + }; + + const failingSelector = () => { + if (preventSelection) return []; + throw new TypeError('bad news!'); + }; + + preventSelection = true; + for (let i = 0; i < toSelect - 1; ++i) { + await topology.selectServer(i % 5 === 0 ? failingSelector : anySelector, {}); completed++; + } + preventSelection = false; + await topology.selectServer(anySelector, {}); + completed++; - expect(completed).to.equal(toSelect); - }, expect.fail); + expect(completed).to.equal(toSelect); }); }); }); From 04d7880b76f7af23f968fc02eaa062c876961b4a Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 13 Mar 2024 11:32:25 -0400 Subject: [PATCH 29/40] narrow types --- src/sdam/topology.ts | 34 ++++++++++++++++------------------ src/utils.ts | 10 +++++++--- 2 files changed, 23 insertions(+), 21 deletions(-) diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index fd0ba0b3180..cb1b90ce188 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -106,7 +106,7 @@ export interface ServerSelectionRequest { transaction?: Transaction; startTime: number; resolve: (server: Server) => void; - reject: (error?: AnyError) => void; + reject: (error: AnyError) => void; [kCancelled]?: boolean; timeoutController: TimeoutController; operationName: string; @@ -889,7 +889,7 @@ function updateServers(topology: Topology, incomingServerDescription?: ServerDes } } -function drainWaitQueue(queue: List, err?: MongoDriverError) { +function drainWaitQueue(queue: List, err: MongoDriverError) { while (queue.length) { const waitQueueMember = queue.shift(); if (!waitQueueMember) { @@ -899,23 +899,21 @@ function drainWaitQueue(queue: List, err?: MongoDriverEr waitQueueMember.timeoutController.clear(); if (!waitQueueMember[kCancelled]) { - if (err) { - if ( - waitQueueMember.mongoLogger?.willLog( - MongoLoggableComponent.SERVER_SELECTION, - SeverityLevel.DEBUG + if ( + waitQueueMember.mongoLogger?.willLog( + MongoLoggableComponent.SERVER_SELECTION, + SeverityLevel.DEBUG + ) + ) { + waitQueueMember.mongoLogger?.debug( + MongoLoggableComponent.SERVER_SELECTION, + new ServerSelectionFailedEvent( + waitQueueMember.serverSelector, + waitQueueMember.topologyDescription, + err, + waitQueueMember.operationName ) - ) { - waitQueueMember.mongoLogger?.debug( - MongoLoggableComponent.SERVER_SELECTION, - new ServerSelectionFailedEvent( - waitQueueMember.serverSelector, - waitQueueMember.topologyDescription, - err, - waitQueueMember.operationName - ) - ); - } + ); } waitQueueMember.reject(err); } diff --git a/src/utils.ts b/src/utils.ts index 98e3950fcb2..8b2faf7d3b3 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -1245,9 +1245,13 @@ export function isHostMatch(match: RegExp, host?: string): boolean { return host && match.test(host.toLowerCase()) ? true : false; } -export function promiseWithResolvers() { - let resolve!: Parameters>[0]>[0]; - let reject!: Parameters>[0]>[1]; +export function promiseWithResolvers(): { + promise: Promise; + resolve: (value: T) => void; + reject: (error: Error) => void; +} { + let resolve!: (value: T) => void; + let reject!: (error: Error) => void; const promise = new Promise(function withResolversExecutor(promiseResolve, promiseReject) { resolve = promiseResolve; reject = promiseReject; From bc46586fbf72599bfe1a528c0e57426ab20e39db Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 13 Mar 2024 12:44:58 -0400 Subject: [PATCH 30/40] remove unused branch --- src/sdam/topology.ts | 33 +++++++++++---------------------- 1 file changed, 11 insertions(+), 22 deletions(-) diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index cb1b90ce188..3f2177f762b 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -460,11 +460,6 @@ export class Topology extends TypedEventEmitter { } } - const exitWithError = (error: Error) => { - this.emit(Topology.ERROR, error); - throw error; - }; - const readPreference = options.readPreference ?? ReadPreference.primary; const selectServerOptions = { operationName: 'ping', ...options }; try { @@ -475,16 +470,10 @@ export class Topology extends TypedEventEmitter { const skipPingOnConnect = this.s.options[Symbol.for('@@mdb.skipPingOnConnect')] === true; if (!skipPingOnConnect && server && this.s.credentials) { - try { - await server.command(ns('admin.$cmd'), { ping: 1 }, {}); - stateTransition(this, STATE_CONNECTED); - this.emit(Topology.OPEN, this); - this.emit(Topology.CONNECT, this); - - return this; - } catch (error) { - exitWithError(error); - } + await server.command(ns('admin.$cmd'), { ping: 1 }, {}); + stateTransition(this, STATE_CONNECTED); + this.emit(Topology.OPEN, this); + this.emit(Topology.CONNECT, this); return this; } @@ -496,7 +485,7 @@ export class Topology extends TypedEventEmitter { return this; } catch (error) { this.close(); - return exitWithError(error); + throw error; } } @@ -950,7 +939,7 @@ function processWaitQueue(topology: Topology) { previousServer ? [previousServer] : [] ) : serverDescriptions; - } catch (e) { + } catch (serverSelectionError) { waitQueueMember.timeoutController.clear(); if ( topology.client.mongoLogger?.willLog( @@ -963,12 +952,12 @@ function processWaitQueue(topology: Topology) { new ServerSelectionFailedEvent( waitQueueMember.serverSelector, topology.description, - e, + serverSelectionError, waitQueueMember.operationName ) ); } - waitQueueMember.reject(e); + waitQueueMember.reject(serverSelectionError); continue; } @@ -1011,7 +1000,7 @@ function processWaitQueue(topology: Topology) { } if (!selectedServer) { - const error = new MongoServerSelectionError( + const serverSelectionError = new MongoServerSelectionError( 'server selection returned a server description but the server was not found in the topology', topology.description ); @@ -1026,12 +1015,12 @@ function processWaitQueue(topology: Topology) { new ServerSelectionFailedEvent( waitQueueMember.serverSelector, topology.description, - error, + serverSelectionError, waitQueueMember.operationName ) ); } - waitQueueMember.reject(error); + waitQueueMember.reject(serverSelectionError); return; } const transaction = waitQueueMember.transaction; From c1d32656915fe7912aa67d2c4d802c5a3ba69b06 Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 13 Mar 2024 12:47:57 -0400 Subject: [PATCH 31/40] renames --- src/sdam/topology.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index 3f2177f762b..695f304ffa3 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -878,7 +878,7 @@ function updateServers(topology: Topology, incomingServerDescription?: ServerDes } } -function drainWaitQueue(queue: List, err: MongoDriverError) { +function drainWaitQueue(queue: List, drainError: MongoDriverError) { while (queue.length) { const waitQueueMember = queue.shift(); if (!waitQueueMember) { @@ -899,12 +899,12 @@ function drainWaitQueue(queue: List, err: MongoDriverErr new ServerSelectionFailedEvent( waitQueueMember.serverSelector, waitQueueMember.topologyDescription, - err, + drainError, waitQueueMember.operationName ) ); } - waitQueueMember.reject(err); + waitQueueMember.reject(drainError); } } } @@ -939,7 +939,7 @@ function processWaitQueue(topology: Topology) { previousServer ? [previousServer] : [] ) : serverDescriptions; - } catch (serverSelectionError) { + } catch (selectorError) { waitQueueMember.timeoutController.clear(); if ( topology.client.mongoLogger?.willLog( @@ -952,12 +952,12 @@ function processWaitQueue(topology: Topology) { new ServerSelectionFailedEvent( waitQueueMember.serverSelector, topology.description, - serverSelectionError, + selectorError, waitQueueMember.operationName ) ); } - waitQueueMember.reject(serverSelectionError); + waitQueueMember.reject(selectorError); continue; } From a7202ecedd8f6cea4ca2db580df5f03d8e1b1646 Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 13 Mar 2024 13:08:40 -0400 Subject: [PATCH 32/40] review suggestions --- src/sdam/topology.ts | 3 +-- test/integration/uri-options/uri.test.js | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index 695f304ffa3..3f01b2428bd 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -20,7 +20,6 @@ import { TOPOLOGY_OPENING } from '../constants'; import { - type AnyError, MongoCompatibilityError, type MongoDriverError, MongoError, @@ -106,7 +105,7 @@ export interface ServerSelectionRequest { transaction?: Transaction; startTime: number; resolve: (server: Server) => void; - reject: (error: AnyError) => void; + reject: (error: MongoError) => void; [kCancelled]?: boolean; timeoutController: TimeoutController; operationName: string; diff --git a/test/integration/uri-options/uri.test.js b/test/integration/uri-options/uri.test.js index 299b0011e66..c0f8b1dea66 100644 --- a/test/integration/uri-options/uri.test.js +++ b/test/integration/uri-options/uri.test.js @@ -130,12 +130,12 @@ describe('URI', function () { it('should generate valid credentials with X509', { metadata: { requires: { topology: 'single' } }, test: function () { - function validateConnect(options) { + async function validateConnect(options) { expect(options).to.have.property('credentials'); expect(options.credentials.mechanism).to.eql('MONGODB-X509'); connectStub.restore(); - return Promise.resolve(); + return; } const topologyPrototype = Topology.prototype; From d2f480c375bdc2a3e74f28928569d39d22f9bfab Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 13 Mar 2024 15:16:33 -0400 Subject: [PATCH 33/40] fix failing unit test --- test/unit/sdam/topology.test.js | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/test/unit/sdam/topology.test.js b/test/unit/sdam/topology.test.js index 4ac9e1e4971..047a224c980 100644 --- a/test/unit/sdam/topology.test.js +++ b/test/unit/sdam/topology.test.js @@ -430,29 +430,33 @@ describe('Topology (unit)', function () { this.sinon.restore(); }); - it('should schedule monitoring if no suitable server is found', function (done) { + it('should schedule monitoring if no suitable server is found', function () { const topology = topologyWithPlaceholderClient('someserver:27019'); const requestCheck = this.sinon.stub(Server.prototype, 'requestCheck'); // satisfy the initial connect, then restore the original method const selectServer = this.sinon .stub(Topology.prototype, 'selectServer') - .callsFake(function () { + .callsFake(async function () { const server = Array.from(this.s.servers.values())[0]; selectServer.restore(); - return Promise.resolve(server); + return server; }); this.sinon.stub(Server.prototype, 'connect').callsFake(function () { this.s.state = 'connected'; this.emit('connect'); - return Promise.resolve(); + return; }); - topology.connect().then(() => { - topology - .selectServer(ReadPreference.secondary, { serverSelectionTimeoutMS: 1000 }) - .then(expect.fail, err => { + return topology + .connect() + .then(topology => + topology.selectServer(ReadPreference.secondary, { serverSelectionTimeoutMS: 1000 }) + ) + .then( + () => expect.fail('expected error'), + err => { expect(err).to.exist; expect(err).to.match(/Server selection timed out/); expect(err).to.have.property('reason'); @@ -462,9 +466,11 @@ describe('Topology (unit)', function () { expect(requestCheck).property('callCount').to.equal(1); topology.close(); - done(); - }); - }); + } + ) + .finally(() => { + topology.close(); + }); }); it('should disallow selection when the topology is explicitly closed', function (done) { From 3fcfe315c28e58b7c86f66c1725872a7c4d027ba Mon Sep 17 00:00:00 2001 From: Warren James Date: Thu, 14 Mar 2024 10:33:20 -0400 Subject: [PATCH 34/40] fix test failure --- test/unit/error.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/error.test.ts b/test/unit/error.test.ts index fc674c32951..8b32fc31930 100644 --- a/test/unit/error.test.ts +++ b/test/unit/error.test.ts @@ -379,7 +379,7 @@ describe('MongoErrors', () => { { replicaSet: 'rs' } as TopologyOptions ); - replSet + return replSet .connect() .then(topology => topology.selectServer('primary', {})) .then(server => From 2bf383f1f3db54c990e94a738d4a9dc7c7ab7b5b Mon Sep 17 00:00:00 2001 From: Warren James Date: Thu, 14 Mar 2024 14:53:33 -0400 Subject: [PATCH 35/40] fix async tests --- ...records_for_mongos_discovery.prose.test.ts | 54 ++- .../{topology.test.js => topology.test.ts} | 390 +++++++++--------- 2 files changed, 207 insertions(+), 237 deletions(-) rename test/unit/sdam/{topology.test.js => topology.test.ts} (59%) diff --git a/test/unit/assorted/polling_srv_records_for_mongos_discovery.prose.test.ts b/test/unit/assorted/polling_srv_records_for_mongos_discovery.prose.test.ts index be0989cb453..28e503782da 100644 --- a/test/unit/assorted/polling_srv_records_for_mongos_discovery.prose.test.ts +++ b/test/unit/assorted/polling_srv_records_for_mongos_discovery.prose.test.ts @@ -11,12 +11,14 @@ import { type SrvPollerOptions, SrvPollingEvent, type TopologyOptions, - TopologyType + TopologyType, + Topology } from '../../mongodb'; import * as mock from '../../tools/mongodb-mock/index'; import type { MockServer } from '../../tools/mongodb-mock/src/server'; import { processTick, topologyWithPlaceholderClient } from '../../tools/utils'; import { createTimerSandbox } from '../timer_sandbox'; +import { once } from 'events'; /* The SRV Prose Tests make use of the following REAL DNS records. @@ -122,7 +124,7 @@ describe('Polling Srv Records for Mongos Discovery', () => { } }); - function runSrvPollerTest(recordSets, done) { + async function runSrvPollerTest(recordSets) { context.servers.forEach(server => { server.setMessageHandler(request => { const doc = request.document; @@ -142,33 +144,23 @@ describe('Polling Srv Records for Mongos Discovery', () => { srvPoller: srvPoller as SrvPoller, srvHost: SRV_HOST } as TopologyOptions); - const topology = context.topology; + const topology: Topology = context.topology; - return topology.connect({}).then(() => { - try { - expect(topology.description).to.have.property('type', TopologyType.Sharded); - const servers = Array.from(topology.description.servers.keys()); - - expect(servers).to.deep.equal(srvAddresses(recordSets[0])); - - topology.once('topologyDescriptionChanged', function () { - tryDone(done, function () { - const servers = Array.from(topology.description.servers.keys()); + await topology.connect({}); + expect(topology.description).to.have.property('type', TopologyType.Sharded); + const servers = Array.from(topology.description.servers.keys()); + expect(servers).to.deep.equal(srvAddresses(recordSets[0])); + process.nextTick(() => srvPoller.trigger(recordSets[1])); - expect(servers).to.deep.equal(srvAddresses(recordSets[1])); - }); - }); + await once(topology, 'topologyDescriptionChanged') - process.nextTick(() => srvPoller.trigger(recordSets[1])); - } catch (e) { - done(e); - } - }, done); + const server = Array.from(topology.description.servers.keys()); + expect(server).to.deep.equal(srvAddresses(recordSets[1])); } // The addition of a new DNS record: // _mongodb._tcp.test1.test.build.10gen.cc. 86400 IN SRV 27019 localhost.test.build.10gen.cc. - it('1. Addition of a new DNS record', function (done) { + it('1. Addition of a new DNS record', async function () { const recordSets = [ [srvRecord(context.servers[0]), srvRecord(context.servers[1])], [ @@ -177,50 +169,50 @@ describe('Polling Srv Records for Mongos Discovery', () => { srvRecord(context.servers[2]) ] ]; - runSrvPollerTest(recordSets, done); + await runSrvPollerTest(recordSets); }); // The removal of an existing DNS record: // _mongodb._tcp.test1.test.build.10gen.cc. 86400 IN SRV 27018 localhost.test.build.10gen.cc. - it('2. Removal of an existing DNS record', function (done) { + it('2. Removal of an existing DNS record', async function () { const recordSets = [ [srvRecord(context.servers[0]), srvRecord(context.servers[1])], [srvRecord(context.servers[0])] ]; - runSrvPollerTest(recordSets, done); + await runSrvPollerTest(recordSets); }); // The replacement of a DNS record: // _mongodb._tcp.test1.test.build.10gen.cc. 86400 IN SRV 27018 localhost.test.build.10gen.cc. // replace by: // _mongodb._tcp.test1.test.build.10gen.cc. 86400 IN SRV 27019 localhost.test.build.10gen.cc. - it('3. Replacement of a DNS record', function (done) { + it('3. Replacement of a DNS record', async function () { const recordSets = [ [srvRecord(context.servers[0]), srvRecord(context.servers[1])], [srvRecord(context.servers[0]), srvRecord(context.servers[2])] ]; - runSrvPollerTest(recordSets, done); + await runSrvPollerTest(recordSets); }); // The replacement of both existing DNS records with one new record: // _mongodb._tcp.test1.test.build.10gen.cc. 86400 IN SRV 27019 localhost.test.build.10gen.cc. - it('4. replacement of both existing DNS records with one new record', function (done) { + it('4. replacement of both existing DNS records with one new record', async function () { const recordSets = [ [srvRecord(context.servers[0]), srvRecord(context.servers[1])], [srvRecord(context.servers[2])] ]; - runSrvPollerTest(recordSets, done); + await runSrvPollerTest(recordSets); }); // The replacement of both existing DNS records with two new records: // _mongodb._tcp.test1.test.build.10gen.cc. 86400 IN SRV 27019 localhost.test.build.10gen.cc. // _mongodb._tcp.test1.test.build.10gen.cc. 86400 IN SRV 27020 localhost.test.build.10gen.cc. - it('5. Replacement of both existing DNS records with two new records', function (done) { + it('5. Replacement of both existing DNS records with two new records', async function () { const recordSets = [ [srvRecord(context.servers[0]), srvRecord(context.servers[1])], [srvRecord(context.servers[2]), srvRecord(context.servers[3])] ]; - runSrvPollerTest(recordSets, done); + await runSrvPollerTest(recordSets); }); }); diff --git a/test/unit/sdam/topology.test.js b/test/unit/sdam/topology.test.ts similarity index 59% rename from test/unit/sdam/topology.test.js rename to test/unit/sdam/topology.test.ts index 047a224c980..7e1b806e17d 100644 --- a/test/unit/sdam/topology.test.js +++ b/test/unit/sdam/topology.test.ts @@ -1,26 +1,35 @@ -'use strict'; - -const { clearTimeout } = require('timers'); -const mock = require('../../tools/mongodb-mock/index'); -const { expect } = require('chai'); -const sinon = require('sinon'); -const net = require('net'); -const { MongoClient, MongoServerSelectionError, ReadPreference } = require('../../mongodb'); -const { Topology } = require('../../mongodb'); -const { Server } = require('../../mongodb'); -const { ns, makeClientMetadata, isHello } = require('../../mongodb'); -const { TopologyDescriptionChangedEvent } = require('../../mongodb'); -const { TopologyDescription } = require('../../mongodb'); -const { TopologyType } = require('../../mongodb'); -const { SrvPoller, SrvPollingEvent } = require('../../mongodb'); -const { getSymbolFrom, topologyWithPlaceholderClient } = require('../../tools/utils'); -const { LEGACY_NOT_WRITABLE_PRIMARY_ERROR_MESSAGE } = require('../../mongodb'); -const { coerce } = require('semver'); +import { expect } from 'chai'; +import { once } from 'events'; +import * as net from 'net'; +import { type AddressInfo } from 'net'; +import { coerce, type SemVer } from 'semver'; +import * as sinon from 'sinon'; +import { clearTimeout } from 'timers'; + +import { + isHello, + LEGACY_NOT_WRITABLE_PRIMARY_ERROR_MESSAGE, + makeClientMetadata, + MongoClient, + MongoServerSelectionError, + ns, + ReadPreference, + Server, + SrvPoller, + SrvPollingEvent, + Topology, + TopologyDescription, + TopologyDescriptionChangedEvent, + TopologyType +} from '../../mongodb'; +import * as mock from '../../tools/mongodb-mock/index'; +import { getSymbolFrom, topologyWithPlaceholderClient } from '../../tools/utils'; describe('Topology (unit)', function () { let client, topology; afterEach(async () => { + sinon.restore(); if (client) { await client.close(); } @@ -32,23 +41,24 @@ describe('Topology (unit)', function () { describe('client metadata', function () { let mockServer; - before(() => mock.createServer().then(server => (mockServer = server))); + before(async () => { + mockServer = await mock.createServer(); + }); after(() => mock.cleanup()); - it('should correctly pass appname', function (done) { - const server = topologyWithPlaceholderClient([`localhost:27017`], { + it('should correctly pass appname', function () { + const server: Topology = topologyWithPlaceholderClient([`localhost:27017`], { metadata: makeClientMetadata({ appName: 'My application name', driverInfo: {} }) }); - expect(server.clientMetadata.application.name).to.equal('My application name'); - done(); + expect(server.clientMetadata?.application.name).to.equal('My application name'); }); it('should report the correct platform in client metadata', async function () { - const helloRequests = []; + const helloRequests: any[] = []; mockServer.setMessageHandler(request => { const doc = request.document; if (isHello(doc)) { @@ -76,7 +86,7 @@ describe('Topology (unit)', function () { describe('black holes', function () { let mockServer; - beforeEach(() => mock.createServer().then(server => (mockServer = server))); + beforeEach(async () => (mockServer = await mock.createServer())); afterEach(() => mock.cleanup()); it('should time out operations against servers that have been blackholed', function (done) { @@ -92,17 +102,18 @@ describe('Topology (unit)', function () { } }); - const topology = topologyWithPlaceholderClient(mockServer.hostAddress()); + const topology = topologyWithPlaceholderClient(mockServer.hostAddress(), {}); topology.connect().then(() => { topology.selectServer('primary', {}).then(server => { - server - .command(ns('admin.$cmd'), { ping: 1 }, { socketTimeoutMS: 250 }) - .then(expect.fail, err => { + server.command(ns('admin.$cmd'), { ping: 1 }, { socketTimeoutMS: 250 }).then( + () => expect.fail('expected command to fail'), + err => { expect(err).to.exist; expect(err).to.match(/timed out/); topology.close(); done(); - }); + } + ); }, expect.fail); }, expect.fail); }); @@ -112,8 +123,8 @@ describe('Topology (unit)', function () { let mockServer; let secondMockServer; beforeEach(async () => { - await mock.createServer().then(server => (mockServer = server)); - await mock.createServer().then(server => (secondMockServer = server)); + mockServer = await mock.createServer(); + secondMockServer = await mock.createServer(); }); afterEach(async () => { await mock.cleanup(); @@ -142,45 +153,47 @@ describe('Topology (unit)', function () { }); }); context('when the topology originally only contained one server', function () { - it('returns a MongoServerSelectionError', function (done) { - topology = topologyWithPlaceholderClient([ - mockServer.hostAddress(), - secondMockServer.hostAddress() - ]); - - topology.connect().then(() => { - sinon.stub(topology.s.servers, 'get').callsFake(() => { - return undefined; - }); - topology.selectServer('primary', {}).then(expect.fail, err => { - expect(err).to.be.instanceOf(MongoServerSelectionError); - done(); - }); - }, expect.fail); + it('returns a MongoServerSelectionError', async function () { + topology = topologyWithPlaceholderClient( + [mockServer.hostAddress(), secondMockServer.hostAddress()], + {} + ); + + await topology.connect(); + sinon.stub(topology.s.servers, 'get').callsFake(() => { + return undefined; + }); + try { + await topology.selectServer('primary', {}); + } catch (err) { + expect(err).to.be.instanceOf(MongoServerSelectionError); + } }); }); + context('when the topology originally contained more than one server', function () { - it('returns a MongoServerSelectionError', function (done) { - topology = topologyWithPlaceholderClient([ - mockServer.hostAddress(), - secondMockServer.hostAddress() - ]); - - topology.connect().then(() => { - sinon.stub(topology.s.servers, 'get').callsFake(() => { - return undefined; - }); - topology.selectServer('primary', {}).then(expect.fail, err => { - expect(err).to.be.instanceOf(MongoServerSelectionError); - done(); - }); - }, expect.fail); + it('returns a MongoServerSelectionError', async function () { + topology = topologyWithPlaceholderClient( + [mockServer.hostAddress(), secondMockServer.hostAddress()], + {} + ); + + await topology.connect(); + sinon.stub(topology.s.servers, 'get').callsFake(() => { + return undefined; + }); + try { + await topology.selectServer('primary', {}); + expect.fail('expected server selection to fail'); + } catch (err) { + expect(err).to.be.instanceOf(MongoServerSelectionError); + } }); }); } ); - it('should set server to unknown and reset pool on `node is recovering` error', function (done) { + it('should set server to unknown and reset pool on `node is recovering` error', async function () { mockServer.setMessageHandler(request => { const doc = request.document; if (isHello(doc)) { @@ -192,26 +205,26 @@ describe('Topology (unit)', function () { } }); - topology = topologyWithPlaceholderClient(mockServer.hostAddress()); - topology.connect().then(() => { - topology.selectServer('primary', {}).then(server => { - let serverDescription; - server.on('descriptionReceived', sd => (serverDescription = sd)); + topology = topologyWithPlaceholderClient(mockServer.hostAddress(), {}); + await topology.connect(); + const server = await topology.selectServer('primary', {}); - let poolCleared = false; - topology.on('connectionPoolCleared', () => (poolCleared = true)); + let serverDescription; + server.on('descriptionReceived', sd => (serverDescription = sd)); - server.command(ns('test.test'), { insert: { a: 42 } }, {}).then(expect.fail, err => { - expect(err).to.exist; - expect(err).to.eql(serverDescription.error); - expect(poolCleared).to.be.true; - done(); - }); - }, expect.fail); - }, expect.fail); + let poolCleared = false; + topology.on('connectionPoolCleared', () => (poolCleared = true)); + + try { + await server.command(ns('test.test'), { insert: { a: 42 } }, {}); + expect.fail('expected command to fail'); + } catch (err) { + expect(err).to.eql(serverDescription.error); + expect(poolCleared).to.be.true; + } }); - it('should set server to unknown and NOT reset pool on stepdown errors', function (done) { + it('should set server to unknown and NOT reset pool on stepdown errors', async function () { mockServer.setMessageHandler(request => { const doc = request.document; if (isHello(doc)) { @@ -223,27 +236,26 @@ describe('Topology (unit)', function () { } }); - const topology = topologyWithPlaceholderClient(mockServer.hostAddress()); - topology.connect().then(() => { - topology.selectServer('primary', {}).then(server => { - let serverDescription; - server.on('descriptionReceived', sd => (serverDescription = sd)); - - let poolCleared = false; - topology.on('connectionPoolCleared', () => (poolCleared = true)); - - server.command(ns('test.test'), { insert: { a: 42 } }, {}).then(expect.fail, err => { - expect(err).to.exist; - expect(err).to.eql(serverDescription.error); - expect(poolCleared).to.be.false; - topology.close(); - done(); - }); - }, expect.fail); - }, expect.fail); + const topology = topologyWithPlaceholderClient(mockServer.hostAddress(), {}); + await topology.connect(); + const server = await topology.selectServer('primary', {}); + let serverDescription; + server.on('descriptionReceived', sd => (serverDescription = sd)); + + let poolCleared = false; + topology.on('connectionPoolCleared', () => (poolCleared = true)); + + try { + await server.command(ns('test.test'), { insert: { a: 42 } }, {}); + expect.fail('expected command to fail'); + } catch (err) { + expect(err).to.eql(serverDescription.error); + expect(poolCleared).to.be.false; + topology.close(); + } }); - it('should set server to unknown on non-timeout network error', function (done) { + it('should set server to unknown on non-timeout network error', async function () { mockServer.setMessageHandler(request => { const doc = request.document; if (isHello(doc)) { @@ -255,26 +267,25 @@ describe('Topology (unit)', function () { } }); - topology = topologyWithPlaceholderClient(mockServer.hostAddress()); - topology.connect().then(() => { - topology.selectServer('primary', {}).then(server => { - let serverDescription; - server.on('descriptionReceived', sd => (serverDescription = sd)); - - server.command(ns('test.test'), { insert: { a: 42 } }, {}).then(expect.fail, err => { - expect(err).to.exist; - expect(err).to.eql(serverDescription.error); - expect(server.description.type).to.equal('Unknown'); - done(); - }); - }, expect.fail); - }, expect.fail); + topology = topologyWithPlaceholderClient(mockServer.hostAddress(), {}); + await topology.connect(); + const server = await topology.selectServer('primary', {}); + let serverDescription; + server.on('descriptionReceived', sd => (serverDescription = sd)); + + try { + await server.command(ns('test.test'), { insert: { a: 42 } }, {}); + expect.fail('expected command to fail'); + } catch (err) { + expect(err).to.eql(serverDescription.error); + expect(server.description.type).to.equal('Unknown'); + } }); - it('should encounter a server selection timeout on garbled server responses', function (done) { + it('should encounter a server selection timeout on garbled server responses', function () { const test = this.test; - const { major } = coerce(process.version); + const { major } = coerce(process.version) as SemVer; test.skipReason = major === 18 || major === 20 ? 'TODO(NODE-5666): fix failing unit tests on Node18' @@ -283,40 +294,24 @@ describe('Topology (unit)', function () { if (test.skipReason) this.skip(); const server = net.createServer(); - const p = Promise.resolve(); - let unexpectedError, expectedError; - server.listen(0, 'localhost', 2, () => { + server.listen(0, 'localhost', 2, async () => { server.on('connection', c => c.on('data', () => c.write('garbage_data'))); - const { address, port } = server.address(); + const { address, port } = server.address() as AddressInfo; const client = new MongoClient(`mongodb://${address}:${port}`, { serverSelectionTimeoutMS: 1000 }); - p.then(() => - client - .connect() - .then(() => { - unexpectedError = new Error('Expected a server selection error but got none'); - }) - .catch(error => { - expectedError = error; - }) - .then(() => { - server.close(); - return client.close(err => { - if (!unexpectedError) { - unexpectedError = err; - } - }); - }) - .finally(() => { - if (unexpectedError) { - return done(unexpectedError); - } - if (expectedError) { - return done(); - } - }) - ); + try { + await client.connect(); + expect.fail('Expected a server selection error but got none'); + } catch (err) { + expect(err).to.be.instanceOf(MongoServerSelectionError); + expect(err) + .to.have.property('message') + .that.matches(/Server selection timed out/); + } + + server.close(); + await client.close(); }); }); @@ -366,29 +361,29 @@ describe('Topology (unit)', function () { expect(topologyChangeListeners[0]).to.equal(topology.s.detectShardedTopology); }); - it('should emit topologyDescriptionChange event', function () { - topology.once(Topology.TOPOLOGY_DESCRIPTION_CHANGED, ev => { - // The first event we get here is caused by the srv record discovery event below - expect(ev).to.have.nested.property('newDescription.servers'); - expect(ev.newDescription.servers.get('fake:2')) - .to.be.a('object') - .with.property('address', 'fake:2'); - }); + it('should emit topologyDescriptionChange event', async function () { + const p = once(topology, Topology.TOPOLOGY_DESCRIPTION_CHANGED); topology.s.srvPoller.emit( SrvPoller.SRV_RECORD_DISCOVERY, new SrvPollingEvent([{ priority: 1, weight: 1, port: 2, name: 'fake' }]) ); + + const [ev] = await p; + // The first event we get here is caused by the srv record discovery event below + expect(ev).to.have.nested.property('newDescription.servers'); + expect(ev.newDescription.servers.get('fake:2')) + .to.be.a('object') + .with.property('address', 'fake:2'); }); - it('should clean up listeners on close', function (done) { + it('should clean up listeners on close', function () { topology.s.state = 'connected'; // fake state to test clean up logic topology.close(); const srvPollerListeners = topology.s.srvPoller.listeners(SrvPoller.SRV_RECORD_DISCOVERY); expect(srvPollerListeners).to.have.lengthOf(0); const topologyChangeListeners = topology.listeners(Topology.TOPOLOGY_DESCRIPTION_CHANGED); expect(topologyChangeListeners).to.have.lengthOf(0); - done(); }); }); @@ -422,20 +417,12 @@ describe('Topology (unit)', function () { }); describe('selectServer()', function () { - beforeEach(function () { - this.sinon = sinon.createSandbox(); - }); - - afterEach(function () { - this.sinon.restore(); - }); - - it('should schedule monitoring if no suitable server is found', function () { - const topology = topologyWithPlaceholderClient('someserver:27019'); - const requestCheck = this.sinon.stub(Server.prototype, 'requestCheck'); + it('should schedule monitoring if no suitable server is found', async function () { + const topology = topologyWithPlaceholderClient('someserver:27019', {}); + const requestCheck = sinon.stub(Server.prototype, 'requestCheck'); // satisfy the initial connect, then restore the original method - const selectServer = this.sinon + const selectServer = sinon .stub(Topology.prototype, 'selectServer') .callsFake(async function () { const server = Array.from(this.s.servers.values())[0]; @@ -443,50 +430,43 @@ describe('Topology (unit)', function () { return server; }); - this.sinon.stub(Server.prototype, 'connect').callsFake(function () { + sinon.stub(Server.prototype, 'connect').callsFake(function () { this.s.state = 'connected'; this.emit('connect'); return; }); - return topology - .connect() - .then(topology => - topology.selectServer(ReadPreference.secondary, { serverSelectionTimeoutMS: 1000 }) - ) - .then( - () => expect.fail('expected error'), - err => { - expect(err).to.exist; - expect(err).to.match(/Server selection timed out/); - expect(err).to.have.property('reason'); - - // When server is created `connect` is called on the monitor. When server selection - // occurs `requestCheck` will be called for an immediate check. - expect(requestCheck).property('callCount').to.equal(1); - - topology.close(); - } - ) - .finally(() => { - topology.close(); - }); + await topology.connect(); + try { + await topology.selectServer(ReadPreference.secondary, { serverSelectionTimeoutMS: 1000 }); + expect.fail('expected server selection to fail'); + } catch (err) { + expect(err).to.exist; + expect(err).to.match(/Server selection timed out/); + expect(err).to.have.property('reason'); + // When server is created `connect` is called on the monitor. When server selection + // occurs `requestCheck` will be called for an immediate check. + expect(requestCheck).to.have.been.calledOnce; + } finally { + topology.close(); + } }); - it('should disallow selection when the topology is explicitly closed', function (done) { - const topology = topologyWithPlaceholderClient('someserver:27019'); - this.sinon.stub(Server.prototype, 'connect').callsFake(function () { + it('should disallow selection when the topology is explicitly closed', async function () { + const topology = topologyWithPlaceholderClient('someserver:27019', {}); + sinon.stub(Server.prototype, 'connect').callsFake(function () { this.s.state = 'connected'; this.emit('connect'); }); topology.close(); - topology - .selectServer(ReadPreference.primary, { serverSelectionTimeoutMS: 2000 }) - .then(expect.fail, err => { - expect(err).to.match(/Topology is closed/); - done(); - }); + + try { + await topology.selectServer(ReadPreference.primary, { serverSelectionTimeoutMS: 2000 }); + expect.fail('expected server selection to fail'); + } catch (err) { + expect(err).to.match(/Topology is closed/); + } }); describe('waitQueue', function () { @@ -498,15 +478,13 @@ describe('Topology (unit)', function () { }); it('should process all wait queue members, including selection with errors', async function () { - topology = topologyWithPlaceholderClient('someserver:27019'); - selectServer = this.sinon - .stub(Topology.prototype, 'selectServer') - .callsFake(async function () { - const server = Array.from(this.s.servers.values())[0]; - return server; - }); + topology = topologyWithPlaceholderClient('someserver:27019', {}); + selectServer = sinon.stub(Topology.prototype, 'selectServer').callsFake(async function () { + const server = Array.from(this.s.servers.values())[0]; + return server; + }); - this.sinon.stub(Server.prototype, 'connect').callsFake(function () { + sinon.stub(Server.prototype, 'connect').callsFake(function () { this.s.state = 'connected'; this.emit('connect'); }); From 228e622197bab9b3fc0e480d17c5dd6a8a2a6655 Mon Sep 17 00:00:00 2001 From: Warren James Date: Thu, 14 Mar 2024 15:09:48 -0400 Subject: [PATCH 36/40] test fixes --- ...records_for_mongos_discovery.prose.test.ts | 18 +-- test/unit/sdam/topology.test.ts | 124 ++++++++---------- 2 files changed, 62 insertions(+), 80 deletions(-) diff --git a/test/unit/assorted/polling_srv_records_for_mongos_discovery.prose.test.ts b/test/unit/assorted/polling_srv_records_for_mongos_discovery.prose.test.ts index 28e503782da..7d58118d37b 100644 --- a/test/unit/assorted/polling_srv_records_for_mongos_discovery.prose.test.ts +++ b/test/unit/assorted/polling_srv_records_for_mongos_discovery.prose.test.ts @@ -1,5 +1,6 @@ import { expect } from 'chai'; import * as dns from 'dns'; +import { once } from 'events'; import { coerce } from 'semver'; import * as sinon from 'sinon'; @@ -10,15 +11,14 @@ import { SrvPoller, type SrvPollerOptions, SrvPollingEvent, + type Topology, type TopologyOptions, - TopologyType, - Topology + TopologyType } from '../../mongodb'; import * as mock from '../../tools/mongodb-mock/index'; import type { MockServer } from '../../tools/mongodb-mock/src/server'; import { processTick, topologyWithPlaceholderClient } from '../../tools/utils'; import { createTimerSandbox } from '../timer_sandbox'; -import { once } from 'events'; /* The SRV Prose Tests make use of the following REAL DNS records. @@ -75,16 +75,6 @@ describe('Polling Srv Records for Mongos Discovery', () => { }; } - function tryDone(done, handle) { - process.nextTick(() => { - try { - handle(); - done(); - } catch (e) { - done(e); - } - }); - } class FakeSrvPoller extends SrvPoller { start() { return; @@ -152,7 +142,7 @@ describe('Polling Srv Records for Mongos Discovery', () => { expect(servers).to.deep.equal(srvAddresses(recordSets[0])); process.nextTick(() => srvPoller.trigger(recordSets[1])); - await once(topology, 'topologyDescriptionChanged') + await once(topology, 'topologyDescriptionChanged'); const server = Array.from(topology.description.servers.keys()); expect(server).to.deep.equal(srvAddresses(recordSets[1])); diff --git a/test/unit/sdam/topology.test.ts b/test/unit/sdam/topology.test.ts index 7e1b806e17d..f2d98f14f6e 100644 --- a/test/unit/sdam/topology.test.ts +++ b/test/unit/sdam/topology.test.ts @@ -154,20 +154,17 @@ describe('Topology (unit)', function () { }); context('when the topology originally only contained one server', function () { it('returns a MongoServerSelectionError', async function () { - topology = topologyWithPlaceholderClient( - [mockServer.hostAddress(), secondMockServer.hostAddress()], - {} - ); + topology = topologyWithPlaceholderClient([mockServer.hostAddress()], {}); await topology.connect(); sinon.stub(topology.s.servers, 'get').callsFake(() => { return undefined; }); - try { - await topology.selectServer('primary', {}); - } catch (err) { - expect(err).to.be.instanceOf(MongoServerSelectionError); - } + const err = await topology.selectServer('primary', {}).then( + () => null, + e => e + ); + expect(err).to.be.instanceOf(MongoServerSelectionError); }); }); @@ -182,12 +179,11 @@ describe('Topology (unit)', function () { sinon.stub(topology.s.servers, 'get').callsFake(() => { return undefined; }); - try { - await topology.selectServer('primary', {}); - expect.fail('expected server selection to fail'); - } catch (err) { - expect(err).to.be.instanceOf(MongoServerSelectionError); - } + const err = await topology.selectServer('primary', {}).then( + () => null, + e => e + ); + expect(err).to.be.instanceOf(MongoServerSelectionError); }); }); } @@ -215,13 +211,12 @@ describe('Topology (unit)', function () { let poolCleared = false; topology.on('connectionPoolCleared', () => (poolCleared = true)); - try { - await server.command(ns('test.test'), { insert: { a: 42 } }, {}); - expect.fail('expected command to fail'); - } catch (err) { - expect(err).to.eql(serverDescription.error); - expect(poolCleared).to.be.true; - } + const err = await server.command(ns('test.test'), { insert: { a: 42 } }, {}).then( + () => null, + e => e + ); + expect(err).to.eql(serverDescription.error); + expect(poolCleared).to.be.true; }); it('should set server to unknown and NOT reset pool on stepdown errors', async function () { @@ -245,14 +240,13 @@ describe('Topology (unit)', function () { let poolCleared = false; topology.on('connectionPoolCleared', () => (poolCleared = true)); - try { - await server.command(ns('test.test'), { insert: { a: 42 } }, {}); - expect.fail('expected command to fail'); - } catch (err) { - expect(err).to.eql(serverDescription.error); - expect(poolCleared).to.be.false; - topology.close(); - } + const err = await server.command(ns('test.test'), { insert: { a: 42 } }, {}).then( + () => null, + e => e + ); + expect(err).to.eql(serverDescription.error); + expect(poolCleared).to.be.false; + topology.close(); }); it('should set server to unknown on non-timeout network error', async function () { @@ -273,13 +267,12 @@ describe('Topology (unit)', function () { let serverDescription; server.on('descriptionReceived', sd => (serverDescription = sd)); - try { - await server.command(ns('test.test'), { insert: { a: 42 } }, {}); - expect.fail('expected command to fail'); - } catch (err) { - expect(err).to.eql(serverDescription.error); - expect(server.description.type).to.equal('Unknown'); - } + const err = await server.command(ns('test.test'), { insert: { a: 42 } }, {}).then( + () => null, + e => e + ); + expect(err).to.eql(serverDescription.error); + expect(server.description.type).to.equal('Unknown'); }); it('should encounter a server selection timeout on garbled server responses', function () { @@ -300,15 +293,14 @@ describe('Topology (unit)', function () { const client = new MongoClient(`mongodb://${address}:${port}`, { serverSelectionTimeoutMS: 1000 }); - try { - await client.connect(); - expect.fail('Expected a server selection error but got none'); - } catch (err) { - expect(err).to.be.instanceOf(MongoServerSelectionError); - expect(err) - .to.have.property('message') - .that.matches(/Server selection timed out/); - } + const err = await client.connect().then( + () => null, + e => e + ); + expect(err).to.be.instanceOf(MongoServerSelectionError); + expect(err) + .to.have.property('message') + .that.matches(/Server selection timed out/); server.close(); await client.close(); @@ -316,7 +308,6 @@ describe('Topology (unit)', function () { }); describe('srv event listeners', function () { - /** @type {Topology} */ let topology; beforeEach(() => { @@ -437,19 +428,18 @@ describe('Topology (unit)', function () { }); await topology.connect(); - try { - await topology.selectServer(ReadPreference.secondary, { serverSelectionTimeoutMS: 1000 }); - expect.fail('expected server selection to fail'); - } catch (err) { - expect(err).to.exist; - expect(err).to.match(/Server selection timed out/); - expect(err).to.have.property('reason'); - // When server is created `connect` is called on the monitor. When server selection - // occurs `requestCheck` will be called for an immediate check. - expect(requestCheck).to.have.been.calledOnce; - } finally { - topology.close(); - } + const err = await topology + .selectServer(ReadPreference.secondary, { serverSelectionTimeoutMS: 1000 }) + .then( + () => null, + e => e + ); + expect(err).to.match(/Server selection timed out/); + expect(err).to.have.property('reason'); + // When server is created `connect` is called on the monitor. When server selection + // occurs `requestCheck` will be called for an immediate check. + expect(requestCheck).to.have.been.calledOnce; + topology.close(); }); it('should disallow selection when the topology is explicitly closed', async function () { @@ -461,12 +451,14 @@ describe('Topology (unit)', function () { topology.close(); - try { - await topology.selectServer(ReadPreference.primary, { serverSelectionTimeoutMS: 2000 }); - expect.fail('expected server selection to fail'); - } catch (err) { - expect(err).to.match(/Topology is closed/); - } + const err = await topology + .selectServer(ReadPreference.primary, { serverSelectionTimeoutMS: 2000 }) + .then( + () => null, + e => e + ); + expect.fail('expected server selection to fail'); + expect(err).to.match(/Topology is closed/); }); describe('waitQueue', function () { From f13977e89b56fb196cf68740d1c658225f214c67 Mon Sep 17 00:00:00 2001 From: Warren James Date: Thu, 14 Mar 2024 16:48:06 -0400 Subject: [PATCH 37/40] remove expect.fail --- test/unit/sdam/topology.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/test/unit/sdam/topology.test.ts b/test/unit/sdam/topology.test.ts index f2d98f14f6e..2f268a5be14 100644 --- a/test/unit/sdam/topology.test.ts +++ b/test/unit/sdam/topology.test.ts @@ -457,7 +457,6 @@ describe('Topology (unit)', function () { () => null, e => e ); - expect.fail('expected server selection to fail'); expect(err).to.match(/Topology is closed/); }); From d2045299fe7e99288129f5daa3989ca250624762 Mon Sep 17 00:00:00 2001 From: Warren James Date: Thu, 14 Mar 2024 16:49:48 -0400 Subject: [PATCH 38/40] Update src/sdam/topology.ts Co-authored-by: Bailey Pearson --- src/sdam/topology.ts | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index 3f01b2428bd..3787981cfd5 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -403,16 +403,12 @@ export class Topology extends TypedEventEmitter { /** Initiate server connect */ async connect(options?: ConnectOptions): Promise { - if (this.connectionLock) { - return this.connectionLock; - } - + this.connectionLock ??= this._connect(options); try { - this.connectionLock = this._connect(options); - await this.connectionLock; + await this.connectionLock; + return this; } finally { - // release - this.connectionLock = undefined; + this.connectionLock = undefined; } return this; From b2c9d37b317d4f794c1595eafdcc221f34d78657 Mon Sep 17 00:00:00 2001 From: Warren James Date: Thu, 14 Mar 2024 16:51:30 -0400 Subject: [PATCH 39/40] fix comment --- test/unit/assorted/server_selection_spec_helper.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/assorted/server_selection_spec_helper.js b/test/unit/assorted/server_selection_spec_helper.js index d1eedad8a73..81303f3e6a1 100644 --- a/test/unit/assorted/server_selection_spec_helper.js +++ b/test/unit/assorted/server_selection_spec_helper.js @@ -171,7 +171,7 @@ async function executeServerSelectionTest(testDefinition) { } return; } catch (err) { - // are we expecting an error? + // if we are expecting and error, immediately succeed if (testDefinition.error) { return; } From 9d738402e3e702d558f73d2a9106b25b219d2eb3 Mon Sep 17 00:00:00 2001 From: Warren James Date: Fri, 15 Mar 2024 10:23:28 -0400 Subject: [PATCH 40/40] lint --- src/sdam/topology.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index 3787981cfd5..9e0fd9dec7f 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -405,10 +405,10 @@ export class Topology extends TypedEventEmitter { async connect(options?: ConnectOptions): Promise { this.connectionLock ??= this._connect(options); try { - await this.connectionLock; - return this; + await this.connectionLock; + return this; } finally { - this.connectionLock = undefined; + this.connectionLock = undefined; } return this;