From 0a74415ced9cfc2e031371dc9d959ebc6d1ea874 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 10 Nov 2023 12:03:56 -0500 Subject: [PATCH 01/10] fix(NODE-5749): RTTPinger always sends legacy hello --- src/sdam/monitor.ts | 52 +++++------ src/sdam/server.ts | 25 +++--- .../rtt_pinger.test.ts | 86 +++++++++++++++++++ 3 files changed, 122 insertions(+), 41 deletions(-) create mode 100644 test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts diff --git a/src/sdam/monitor.ts b/src/sdam/monitor.ts index 1e510d0deae..3525f677e5a 100644 --- a/src/sdam/monitor.ts +++ b/src/sdam/monitor.ts @@ -26,8 +26,6 @@ const kConnection = Symbol('connection'); /** @internal */ const kCancellationToken = Symbol('cancellationToken'); /** @internal */ -const kRTTPinger = Symbol('rttPinger'); -/** @internal */ const kRoundTripTime = Symbol('roundTripTime'); const STATE_IDLE = 'idle'; @@ -81,7 +79,7 @@ export class Monitor extends TypedEventEmitter { [kCancellationToken]: CancellationToken; /** @internal */ [kMonitorId]?: MonitorInterval; - [kRTTPinger]?: RTTPinger; + rttPinger?: RTTPinger; get connection(): Connection | undefined { return this[kConnection]; @@ -198,8 +196,8 @@ function resetMonitorState(monitor: Monitor) { monitor[kMonitorId]?.stop(); monitor[kMonitorId] = undefined; - monitor[kRTTPinger]?.close(); - monitor[kRTTPinger] = undefined; + monitor.rttPinger?.close(); + monitor.rttPinger = undefined; monitor[kCancellationToken].emit('cancel'); @@ -262,8 +260,8 @@ function checkServer(monitor: Monitor, callback: Callback) { } : { socketTimeoutMS: connectTimeoutMS }; - if (isAwaitable && monitor[kRTTPinger] == null) { - monitor[kRTTPinger] = new RTTPinger( + if (isAwaitable && monitor.rttPinger == null) { + monitor.rttPinger = new RTTPinger( monitor[kCancellationToken], Object.assign( { heartbeatFrequencyMS: monitor.options.heartbeatFrequencyMS }, @@ -282,9 +280,10 @@ function checkServer(monitor: Monitor, callback: Callback) { hello.isWritablePrimary = hello[LEGACY_HELLO_COMMAND]; } - const rttPinger = monitor[kRTTPinger]; const duration = - isAwaitable && rttPinger ? rttPinger.roundTripTime : calculateDurationInMs(start); + isAwaitable && monitor.rttPinger + ? monitor.rttPinger.roundTripTime + : calculateDurationInMs(start); const awaited = isAwaitable && hello.topologyVersion != null; monitor.emit( @@ -301,8 +300,8 @@ function checkServer(monitor: Monitor, callback: Callback) { ); start = now(); } else { - monitor[kRTTPinger]?.close(); - monitor[kRTTPinger] = undefined; + monitor.rttPinger?.close(); + monitor.rttPinger = undefined; callback(undefined, hello); } @@ -399,8 +398,7 @@ export interface RTTPingerOptions extends ConnectionOptions { /** @internal */ export class RTTPinger { - /** @internal */ - [kConnection]?: Connection; + connection?: Connection; /** @internal */ [kCancellationToken]: CancellationToken; /** @internal */ @@ -410,7 +408,7 @@ export class RTTPinger { closed: boolean; constructor(cancellationToken: CancellationToken, options: RTTPingerOptions) { - this[kConnection] = undefined; + this.connection = undefined; this[kCancellationToken] = cancellationToken; this[kRoundTripTime] = 0; this.closed = false; @@ -427,8 +425,8 @@ export class RTTPinger { this.closed = true; clearTimeout(this[kMonitorId]); - this[kConnection]?.destroy({ force: true }); - this[kConnection] = undefined; + this.connection?.destroy({ force: true }); + this.connection = undefined; } } @@ -447,8 +445,8 @@ function measureRoundTripTime(rttPinger: RTTPinger, options: RTTPingerOptions) { return; } - if (rttPinger[kConnection] == null) { - rttPinger[kConnection] = conn; + if (rttPinger.connection == null) { + rttPinger.connection = conn; } rttPinger[kRoundTripTime] = calculateDurationInMs(start); @@ -458,11 +456,11 @@ function measureRoundTripTime(rttPinger: RTTPinger, options: RTTPingerOptions) { ); } - const connection = rttPinger[kConnection]; + const connection = rttPinger.connection; if (connection == null) { connect(options, (err, conn) => { if (err) { - rttPinger[kConnection] = undefined; + rttPinger.connection = undefined; rttPinger[kRoundTripTime] = 0; return; } @@ -473,15 +471,17 @@ function measureRoundTripTime(rttPinger: RTTPinger, options: RTTPingerOptions) { return; } - connection.command(ns('admin.$cmd'), { [LEGACY_HELLO_COMMAND]: 1 }, undefined, err => { - if (err) { - rttPinger[kConnection] = undefined; + const commandName = + connection.serverApi?.version || connection.helloOk ? 'hello' : LEGACY_HELLO_COMMAND; + connection.commandAsync(ns('admin.$cmd'), { [commandName]: 1 }, undefined).then( + () => measureAndReschedule(), + () => { + rttPinger.connection?.destroy({ force: true }); + rttPinger.connection = undefined; rttPinger[kRoundTripTime] = 0; return; } - - measureAndReschedule(); - }); + ); } /** diff --git a/src/sdam/server.ts b/src/sdam/server.ts index 5ef614db077..8e6d2b13d39 100644 --- a/src/sdam/server.ts +++ b/src/sdam/server.ts @@ -74,9 +74,6 @@ const stateTransition = makeStateMachine({ [STATE_CLOSING]: [STATE_CLOSING, STATE_CLOSED] }); -/** @internal */ -const kMonitor = Symbol('monitor'); - /** @internal */ export type ServerOptions = Omit & MonitorOptions; @@ -119,7 +116,7 @@ export class Server extends TypedEventEmitter { serverApi?: ServerApi; hello?: Document; commandAsync: (ns: MongoDBNamespace, cmd: Document, options: CommandOptions) => Promise; - [kMonitor]: Monitor | null; + monitor: Monitor | null; /** @event */ static readonly SERVER_HEARTBEAT_STARTED = SERVER_HEARTBEAT_STARTED; @@ -175,22 +172,20 @@ export class Server extends TypedEventEmitter { }); if (this.loadBalanced) { - this[kMonitor] = null; + this.monitor = null; // monitoring is disabled in load balancing mode return; } // create the monitor - // TODO(NODE-4144): Remove new variable for type narrowing - const monitor = new Monitor(this, this.s.options); - this[kMonitor] = monitor; + this.monitor = new Monitor(this, this.s.options); for (const event of HEARTBEAT_EVENTS) { - monitor.on(event, (e: any) => this.emit(event, e)); + this.monitor.on(event, (e: any) => this.emit(event, e)); } - monitor.on('resetServer', (error: MongoError) => markServerUnknown(this, error)); - monitor.on(Server.SERVER_HEARTBEAT_SUCCEEDED, (event: ServerHeartbeatSucceededEvent) => { + this.monitor.on('resetServer', (error: MongoError) => markServerUnknown(this, error)); + this.monitor.on(Server.SERVER_HEARTBEAT_SUCCEEDED, (event: ServerHeartbeatSucceededEvent) => { this.emit( Server.DESCRIPTION_RECEIVED, new ServerDescription(this.description.hostAddress, event.reply, { @@ -246,7 +241,7 @@ export class Server extends TypedEventEmitter { // a load balancer. It never transitions out of this state and // has no monitor. if (!this.loadBalanced) { - this[kMonitor]?.connect(); + this.monitor?.connect(); } else { stateTransition(this, STATE_CONNECTED); this.emit(Server.CONNECT, this); @@ -272,7 +267,7 @@ export class Server extends TypedEventEmitter { stateTransition(this, STATE_CLOSING); if (!this.loadBalanced) { - this[kMonitor]?.close(); + this.monitor?.close(); } this.pool.close(options, err => { @@ -290,7 +285,7 @@ export class Server extends TypedEventEmitter { */ requestCheck(): void { if (!this.loadBalanced) { - this[kMonitor]?.requestCheck(); + this.monitor?.requestCheck(); } } @@ -465,7 +460,7 @@ function markServerUnknown(server: Server, error?: MongoServerError) { } if (error instanceof MongoNetworkError && !(error instanceof MongoNetworkTimeoutError)) { - server[kMonitor]?.reset(); + server.monitor?.reset(); } server.emit( diff --git a/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts b/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts new file mode 100644 index 00000000000..32f29175d3c --- /dev/null +++ b/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts @@ -0,0 +1,86 @@ +import { expect } from 'chai'; +import * as semver from 'semver'; +import * as sinon from 'sinon'; + +import { type MongoClient } from '../../mongodb'; +import { sleep } from '../../tools/utils'; + +describe('class RTTPinger', () => { + afterEach(() => sinon.restore()); + + context('when serverApi is enabled', () => { + let serverApiClient: MongoClient; + beforeEach(async function () { + if (semver.gte('5.0.0', this.configuration.version)) { + if (this.currentTest) + this.currentTest.skipReason = 'Test requires serverApi, needs to be on MongoDB 5.0+'; + return this.skip(); + } + + serverApiClient = this.configuration.newClient( + {}, + { serverApi: { version: '1', strict: true }, heartbeatFrequencyMS: 10 } + ); + }); + + afterEach(async () => { + await serverApiClient?.close(); + }); + + it('measures rtt with a hello command', async function () { + await serverApiClient.connect(); + await sleep(1001); // rttPinger creation + + const rttPingers = Array.from(serverApiClient.topology?.s.servers.values() ?? [], s => { + if (s.monitor?.rttPinger) return s.monitor?.rttPinger; + else expect.fail('expected rttPinger to be defined'); + }); + + await sleep(11); // rttPinger connection creation + + const spies = rttPingers.map(rtt => sinon.spy(rtt.connection!, 'command')); + + await sleep(11); // allow for another ping after spies have been made + + expect(spies).to.have.length.above(1); + for (const spy of spies) { + expect(spy).to.have.been.calledWith(sinon.match.any, { hello: 1 }, sinon.match.any); + } + }); + }); + + context('when rtt hello receives an error', () => { + let client: MongoClient; + beforeEach(async function () { + client = this.configuration.newClient({}, { heartbeatFrequencyMS: 10 }); + }); + + afterEach(async () => { + await client?.close(); + }); + + it('destroys the connection', async function () { + await client.connect(); + await sleep(1001); // rttPinger creation + + const rttPingers = Array.from(client.topology?.s.servers.values() ?? [], s => { + if (s.monitor?.rttPinger) return s.monitor?.rttPinger; + else expect.fail('expected rttPinger to be defined'); + }); + + await sleep(11); // rttPinger connection creation + + const spies = rttPingers.map(rtt => + sinon.stub(rtt.connection!, 'command').yieldsRight(new Error('any')) + ); + const destroySpies = rttPingers.map(rtt => sinon.spy(rtt.connection!, 'destroy')); + + await sleep(11); // allow for another ping after spies have been made + + expect(destroySpies).to.have.length.above(1); + for (const spy of spies) { + expect(spy).to.have.been.called; + } + }); + }); +}); From 54c8af602e435fdb88396583d5125c4e8618dbcb Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 10 Nov 2023 12:32:08 -0500 Subject: [PATCH 02/10] test: fix assertion for standalone --- .../rtt_pinger.test.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts b/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts index 32f29175d3c..5d045fa93e1 100644 --- a/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts +++ b/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts @@ -42,7 +42,7 @@ describe('class RTTPinger', () => { await sleep(11); // allow for another ping after spies have been made - expect(spies).to.have.length.above(1); + expect(spies).to.have.lengthOf.at.least(1); for (const spy of spies) { expect(spy).to.have.been.calledWith(sinon.match.any, { hello: 1 }, sinon.match.any); } @@ -70,14 +70,14 @@ describe('class RTTPinger', () => { await sleep(11); // rttPinger connection creation - const spies = rttPingers.map(rtt => - sinon.stub(rtt.connection!, 'command').yieldsRight(new Error('any')) - ); - const destroySpies = rttPingers.map(rtt => sinon.spy(rtt.connection!, 'destroy')); + for (const rtt of rttPingers) + sinon.stub(rtt.connection!, 'command').yieldsRight(new Error('any')); + + const spies = rttPingers.map(rtt => sinon.spy(rtt.connection!, 'destroy')); await sleep(11); // allow for another ping after spies have been made - expect(destroySpies).to.have.length.above(1); + expect(spies).to.have.lengthOf.at.least(1); for (const spy of spies) { expect(spy).to.have.been.called; } From ef86f7b727ec5416f4f7ecd0e6c51f0bb36effb9 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 10 Nov 2023 13:03:53 -0500 Subject: [PATCH 03/10] test: above 4.4 --- .../rtt_pinger.test.ts | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts b/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts index 5d045fa93e1..d5e6f605a45 100644 --- a/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts +++ b/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts @@ -8,6 +8,15 @@ import { sleep } from '../../tools/utils'; describe('class RTTPinger', () => { afterEach(() => sinon.restore()); + beforeEach(async function () { + if (semver.gte('4.4.0', this.configuration.version)) { + if (this.currentTest) + this.currentTest.skipReason = + 'Test requires streaming monitoring, needs to be on MongoDB 4.4+'; + return this.skip(); + } + }); + context('when serverApi is enabled', () => { let serverApiClient: MongoClient; beforeEach(async function () { @@ -38,7 +47,7 @@ describe('class RTTPinger', () => { await sleep(11); // rttPinger connection creation - const spies = rttPingers.map(rtt => sinon.spy(rtt.connection!, 'command')); + const spies = rttPingers.map(rtt => rtt.connection && sinon.spy(rtt.connection, 'command')); await sleep(11); // allow for another ping after spies have been made @@ -71,9 +80,9 @@ describe('class RTTPinger', () => { await sleep(11); // rttPinger connection creation for (const rtt of rttPingers) - sinon.stub(rtt.connection!, 'command').yieldsRight(new Error('any')); + rtt.connection && sinon.stub(rtt.connection, 'command').yieldsRight(new Error('any')); - const spies = rttPingers.map(rtt => sinon.spy(rtt.connection!, 'destroy')); + const spies = rttPingers.map(rtt => rtt.connection && sinon.spy(rtt.connection, 'destroy')); await sleep(11); // allow for another ping after spies have been made From efed7ebeadefa9f27d920ccb51769a699e1f0349 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 10 Nov 2023 13:09:18 -0500 Subject: [PATCH 04/10] test: skip LB mode --- .../connection-monitoring-and-pooling/rtt_pinger.test.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts b/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts index d5e6f605a45..1dc2b430899 100644 --- a/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts +++ b/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts @@ -9,6 +9,11 @@ describe('class RTTPinger', () => { afterEach(() => sinon.restore()); beforeEach(async function () { + if (this.configuration.isLoadBalanced) { + if (this.currentTest) + this.currentTest.skipReason = 'No monitoring in LB mode, test not relevant'; + return this.skip(); + } if (semver.gte('4.4.0', this.configuration.version)) { if (this.currentTest) this.currentTest.skipReason = From 3cf3ed2fd91d4088b8f4a495a4f573c23434f8a5 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 10 Nov 2023 14:37:46 -0500 Subject: [PATCH 05/10] test: address flakiness by waiting for necessary condition --- .../rtt_pinger.test.ts | 53 +++++++++++-------- test/unit/sdam/topology.test.js | 5 +- 2 files changed, 33 insertions(+), 25 deletions(-) diff --git a/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts b/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts index 1dc2b430899..aa1b10cad93 100644 --- a/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts +++ b/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts @@ -2,9 +2,32 @@ import { expect } from 'chai'; import * as semver from 'semver'; import * as sinon from 'sinon'; -import { type MongoClient } from '../../mongodb'; +import { type Connection, type MongoClient, type RTTPinger } from '../../mongodb'; import { sleep } from '../../tools/utils'; +/** + * RTTPinger creation depends on getting a response to the monitor's initial hello + * and that hello containing a topologyVersion. + * Subsequently the rttPinger creates its connection asynchronously + * + * I just went with a sleepy loop, until we have what we need, One could also use SDAM events in a clever way perhaps? + */ +async function getRTTPingers(client: MongoClient) { + // eslint-disable-next-line no-constant-condition + while (true) { + const rttPingers = Array.from(client.topology?.s.servers.values() ?? [], s => { + if (s.monitor?.rttPinger?.connection != null) return s.monitor?.rttPinger; + else null; + }).filter(rtt => rtt != null); + + if (rttPingers.length !== 0) { + return rttPingers as (Omit & { connection: Connection })[]; + } + + await sleep(5); + } +} + describe('class RTTPinger', () => { afterEach(() => sinon.restore()); @@ -43,16 +66,9 @@ describe('class RTTPinger', () => { it('measures rtt with a hello command', async function () { await serverApiClient.connect(); - await sleep(1001); // rttPinger creation - - const rttPingers = Array.from(serverApiClient.topology?.s.servers.values() ?? [], s => { - if (s.monitor?.rttPinger) return s.monitor?.rttPinger; - else expect.fail('expected rttPinger to be defined'); - }); - - await sleep(11); // rttPinger connection creation + const rttPingers = await getRTTPingers(serverApiClient); - const spies = rttPingers.map(rtt => rtt.connection && sinon.spy(rtt.connection, 'command')); + const spies = rttPingers.map(rtt => sinon.spy(rtt.connection, 'command')); await sleep(11); // allow for another ping after spies have been made @@ -75,19 +91,12 @@ describe('class RTTPinger', () => { it('destroys the connection', async function () { await client.connect(); - await sleep(1001); // rttPinger creation + const rttPingers = await getRTTPingers(client); - const rttPingers = Array.from(client.topology?.s.servers.values() ?? [], s => { - if (s.monitor?.rttPinger) return s.monitor?.rttPinger; - else expect.fail('expected rttPinger to be defined'); - }); - - await sleep(11); // rttPinger connection creation - - for (const rtt of rttPingers) - rtt.connection && sinon.stub(rtt.connection, 'command').yieldsRight(new Error('any')); - - const spies = rttPingers.map(rtt => rtt.connection && sinon.spy(rtt.connection, 'destroy')); + for (const rtt of rttPingers) { + sinon.stub(rtt.connection, 'command').yieldsRight(new Error('any')); + } + const spies = rttPingers.map(rtt => sinon.spy(rtt.connection, 'destroy')); await sleep(11); // allow for another ping after spies have been made diff --git a/test/unit/sdam/topology.test.js b/test/unit/sdam/topology.test.js index f99247c1ace..261a363c6df 100644 --- a/test/unit/sdam/topology.test.js +++ b/test/unit/sdam/topology.test.js @@ -355,9 +355,8 @@ describe('Topology (unit)', function () { afterEach(() => { // The srv event starts a monitor that we need to clean up for (const [, server] of topology.s.servers) { - const kMonitor = getSymbolFrom(server, 'monitor'); - const kMonitorId = getSymbolFrom(server[kMonitor], 'monitorId'); - server[kMonitor][kMonitorId].stop(); + const kMonitorId = getSymbolFrom(server.monitor, 'monitorId'); + server.monitor[kMonitorId].stop(); } }); From 937ea95ccd966a8fc996f827beda020b82de75ae Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 13 Nov 2023 16:44:48 -0500 Subject: [PATCH 06/10] test: cleanup rtt finder and beforeEach --- .../rtt_pinger.test.ts | 35 ++++++++++--------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts b/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts index aa1b10cad93..56c5b8b8061 100644 --- a/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts +++ b/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts @@ -6,22 +6,24 @@ import { type Connection, type MongoClient, type RTTPinger } from '../../mongodb import { sleep } from '../../tools/utils'; /** - * RTTPinger creation depends on getting a response to the monitor's initial hello - * and that hello containing a topologyVersion. - * Subsequently the rttPinger creates its connection asynchronously + * RTTPingers are only created after getting a hello from the server that defines topologyVersion + * Each monitor is reaching out to a different node and rttPinger's are created async as a result. * - * I just went with a sleepy loop, until we have what we need, One could also use SDAM events in a clever way perhaps? + * This function checks for rttPingers and sleeps if none are found. */ async function getRTTPingers(client: MongoClient) { + type RTTPingerConnection = Omit & { connection: Connection }; + const pingers = (rtt => rtt?.connection != null) as (r?: RTTPinger) => r is RTTPingerConnection; + + if (!client.topology) expect.fail('Must provide a connected client'); + // eslint-disable-next-line no-constant-condition while (true) { - const rttPingers = Array.from(client.topology?.s.servers.values() ?? [], s => { - if (s.monitor?.rttPinger?.connection != null) return s.monitor?.rttPinger; - else null; - }).filter(rtt => rtt != null); + const servers = client.topology.s.servers.values(); + const rttPingers = Array.from(servers, s => s.monitor?.rttPinger).filter(pingers); if (rttPingers.length !== 0) { - return rttPingers as (Omit & { connection: Connection })[]; + return rttPingers; } await sleep(5); @@ -32,25 +34,26 @@ describe('class RTTPinger', () => { afterEach(() => sinon.restore()); beforeEach(async function () { + if (!this.currentTest) return; if (this.configuration.isLoadBalanced) { - if (this.currentTest) - this.currentTest.skipReason = 'No monitoring in LB mode, test not relevant'; + this.currentTest.skipReason = 'No monitoring in LB mode, test not relevant'; return this.skip(); } if (semver.gte('4.4.0', this.configuration.version)) { - if (this.currentTest) - this.currentTest.skipReason = - 'Test requires streaming monitoring, needs to be on MongoDB 4.4+'; + this.currentTest.skipReason = + 'Test requires streaming monitoring, needs to be on MongoDB 4.4+'; return this.skip(); } }); context('when serverApi is enabled', () => { let serverApiClient: MongoClient; + beforeEach(async function () { + if (!this.currentTest) return; + if (semver.gte('5.0.0', this.configuration.version)) { - if (this.currentTest) - this.currentTest.skipReason = 'Test requires serverApi, needs to be on MongoDB 5.0+'; + this.currentTest.skipReason = 'Test requires serverApi, needs to be on MongoDB 5.0+'; return this.skip(); } From 698ad1cab2943823c01d67661d8ac4658a4ed741 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 13 Nov 2023 16:54:50 -0500 Subject: [PATCH 07/10] test: add serverApi disabled test --- .../rtt_pinger.test.ts | 42 ++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts b/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts index 56c5b8b8061..58181201b85 100644 --- a/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts +++ b/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts @@ -2,7 +2,12 @@ import { expect } from 'chai'; import * as semver from 'semver'; import * as sinon from 'sinon'; -import { type Connection, type MongoClient, type RTTPinger } from '../../mongodb'; +import { + type Connection, + LEGACY_HELLO_COMMAND, + type MongoClient, + type RTTPinger +} from '../../mongodb'; import { sleep } from '../../tools/utils'; /** @@ -82,6 +87,41 @@ describe('class RTTPinger', () => { }); }); + context.only('when serverApi is not enabled and connected to a pre-hello server', () => { + let client: MongoClient; + + beforeEach(async function () { + client = this.configuration.newClient({}, { heartbeatFrequencyMS: 10 }); + }); + + afterEach(async () => { + await client?.close(); + }); + + it('measures rtt with a LEGACY_HELLO_COMMAND command', async function () { + await client.connect(); + const rttPingers = await getRTTPingers(client); + + // Fake pre-hello server. + // Hello was back-ported to feature versions of the server so we would need to pin + // versions prior to 4.4.2, 4.2.10, 4.0.21, and 3.6.21 to integration test + for (const rtt of rttPingers) rtt.connection.helloOk = false; + + const spies = rttPingers.map(rtt => sinon.spy(rtt.connection, 'command')); + + await sleep(11); // allow for another ping after spies have been made + + expect(spies).to.have.lengthOf.at.least(1); + for (const spy of spies) { + expect(spy).to.have.been.calledWith( + sinon.match.any, + { [LEGACY_HELLO_COMMAND]: 1 }, + sinon.match.any + ); + } + }); + }); + context('when rtt hello receives an error', () => { let client: MongoClient; beforeEach(async function () { From 2f540f1963b99c9ad683b53b7285a33e4f1ea159 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 13 Nov 2023 17:10:26 -0500 Subject: [PATCH 08/10] test: update naming for error case --- .../rtt_pinger.test.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts b/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts index 58181201b85..2446c2033ca 100644 --- a/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts +++ b/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts @@ -87,7 +87,7 @@ describe('class RTTPinger', () => { }); }); - context.only('when serverApi is not enabled and connected to a pre-hello server', () => { + context('when serverApi is not enabled and connected to a pre-hello server', () => { let client: MongoClient; beforeEach(async function () { @@ -122,7 +122,7 @@ describe('class RTTPinger', () => { }); }); - context('when rtt hello receives an error', () => { + context(`when the RTTPinger's hello command receives any error`, () => { let client: MongoClient; beforeEach(async function () { client = this.configuration.newClient({}, { heartbeatFrequencyMS: 10 }); @@ -132,12 +132,12 @@ describe('class RTTPinger', () => { await client?.close(); }); - it('destroys the connection', async function () { + it('destroys the connection with force=true', async function () { await client.connect(); const rttPingers = await getRTTPingers(client); for (const rtt of rttPingers) { - sinon.stub(rtt.connection, 'command').yieldsRight(new Error('any')); + sinon.stub(rtt.connection, 'command').yieldsRight(new Error('non MongoError')); } const spies = rttPingers.map(rtt => sinon.spy(rtt.connection, 'destroy')); @@ -145,7 +145,7 @@ describe('class RTTPinger', () => { expect(spies).to.have.lengthOf.at.least(1); for (const spy of spies) { - expect(spy).to.have.been.called; + expect(spy).to.have.been.calledWithExactly({ force: true }); } }); }); From 0e810ec767ca6ba23f18f140448b5244dd229a93 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 13 Nov 2023 17:31:37 -0500 Subject: [PATCH 09/10] test: skip when serverApi is enabled --- .../connection-monitoring-and-pooling/rtt_pinger.test.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts b/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts index 2446c2033ca..d064c253c73 100644 --- a/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts +++ b/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts @@ -91,6 +91,12 @@ describe('class RTTPinger', () => { let client: MongoClient; beforeEach(async function () { + if (!this.currentTest) return; + if (this.configuration.serverApi) { + this.currentTest.skipReason = 'Test requires serverApi to NOT be enabled'; + return this.skip(); + } + client = this.configuration.newClient({}, { heartbeatFrequencyMS: 10 }); }); From d91df42e04f57c0377e473925e75f7403b974054 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 14 Nov 2023 13:18:45 -0500 Subject: [PATCH 10/10] test: add api disable and helloOk test, fix err msg --- .../rtt_pinger.test.ts | 59 +++++++++++++------ 1 file changed, 40 insertions(+), 19 deletions(-) diff --git a/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts b/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts index d064c253c73..af584aabe20 100644 --- a/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts +++ b/test/integration/connection-monitoring-and-pooling/rtt_pinger.test.ts @@ -87,7 +87,7 @@ describe('class RTTPinger', () => { }); }); - context('when serverApi is not enabled and connected to a pre-hello server', () => { + context('when serverApi is disabled', () => { let client: MongoClient; beforeEach(async function () { @@ -104,27 +104,48 @@ describe('class RTTPinger', () => { await client?.close(); }); - it('measures rtt with a LEGACY_HELLO_COMMAND command', async function () { - await client.connect(); - const rttPingers = await getRTTPingers(client); + context('connected to a pre-hello server', () => { + it('measures rtt with a LEGACY_HELLO_COMMAND command', async function () { + await client.connect(); + const rttPingers = await getRTTPingers(client); + + // Fake pre-hello server. + // Hello was back-ported to feature versions of the server so we would need to pin + // versions prior to 4.4.2, 4.2.10, 4.0.21, and 3.6.21 to integration test + for (const rtt of rttPingers) rtt.connection.helloOk = false; + + const spies = rttPingers.map(rtt => sinon.spy(rtt.connection, 'command')); + + await sleep(11); // allow for another ping after spies have been made + + expect(spies).to.have.lengthOf.at.least(1); + for (const spy of spies) { + expect(spy).to.have.been.calledWith( + sinon.match.any, + { [LEGACY_HELLO_COMMAND]: 1 }, + sinon.match.any + ); + } + }); + }); - // Fake pre-hello server. - // Hello was back-ported to feature versions of the server so we would need to pin - // versions prior to 4.4.2, 4.2.10, 4.0.21, and 3.6.21 to integration test - for (const rtt of rttPingers) rtt.connection.helloOk = false; + context('connected to a helloOk server', () => { + it('measures rtt with a hello command', async function () { + await client.connect(); + const rttPingers = await getRTTPingers(client); - const spies = rttPingers.map(rtt => sinon.spy(rtt.connection, 'command')); + const spies = rttPingers.map(rtt => sinon.spy(rtt.connection, 'command')); - await sleep(11); // allow for another ping after spies have been made + // We should always be connected to helloOk servers + for (const rtt of rttPingers) expect(rtt.connection).to.have.property('helloOk', true); - expect(spies).to.have.lengthOf.at.least(1); - for (const spy of spies) { - expect(spy).to.have.been.calledWith( - sinon.match.any, - { [LEGACY_HELLO_COMMAND]: 1 }, - sinon.match.any - ); - } + await sleep(11); // allow for another ping after spies have been made + + expect(spies).to.have.lengthOf.at.least(1); + for (const spy of spies) { + expect(spy).to.have.been.calledWith(sinon.match.any, { hello: 1 }, sinon.match.any); + } + }); }); }); @@ -143,7 +164,7 @@ describe('class RTTPinger', () => { const rttPingers = await getRTTPingers(client); for (const rtt of rttPingers) { - sinon.stub(rtt.connection, 'command').yieldsRight(new Error('non MongoError')); + sinon.stub(rtt.connection, 'command').yieldsRight(new Error('any error')); } const spies = rttPingers.map(rtt => sinon.spy(rtt.connection, 'destroy'));