From a9298a47652ad660dd0e60ded4815ccbae5534e8 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 14 Oct 2024 17:04:13 -0400 Subject: [PATCH 01/10] wip --- src/mongo_client.ts | 19 +++++-- src/timeout.ts | 6 ++ .../node-specific/auto_connect.test.ts | 57 +++++++++++++++++++ 3 files changed, 76 insertions(+), 6 deletions(-) diff --git a/src/mongo_client.ts b/src/mongo_client.ts index 569f0e97c79..f9a5c2c1a40 100644 --- a/src/mongo_client.ts +++ b/src/mongo_client.ts @@ -47,6 +47,7 @@ import { readPreferenceServerSelector } from './sdam/server_selection'; import type { SrvPoller } from './sdam/srv_polling'; import { Topology, type TopologyEvents } from './sdam/topology'; import { ClientSession, type ClientSessionOptions, ServerSessionPool } from './sessions'; +import { Timeout } from './timeout'; import { COSMOS_DB_CHECK, COSMOS_DB_MSG, @@ -520,13 +521,19 @@ export class MongoClient extends TypedEventEmitter implements return await this.connectionLock; } - try { - this.connectionLock = this._connect(); - await this.connectionLock; - } finally { - // release + const timeoutMS = this[kOptions].timeoutMS; + + this.connectionLock = this._connect().finally(() => { + // Clear the lock only when this promise finishes: this.connectionLock = undefined; - } + }); + + await (timeoutMS == null + ? this.connectionLock + : Promise.race([ + this.connectionLock, + Timeout.expires(timeoutMS).catch(Timeout.convertError) + ])); return this; } diff --git a/src/timeout.ts b/src/timeout.ts index 9041ce4b88d..6465848ffca 100644 --- a/src/timeout.ts +++ b/src/timeout.ts @@ -126,6 +126,12 @@ export class Timeout extends Promise { static override reject(rejection?: Error): Timeout { return new Timeout(undefined, { duration: 0, unref: true, rejection }); } + + static convertError(this: void, cause: unknown): never { + if (TimeoutError.is(cause)) throw new MongoOperationTimeoutError('Timed out'); + if (cause instanceof Error) throw cause; + throw new MongoRuntimeError('Unknown error', { cause: cause as any }); + } } /** @internal */ diff --git a/test/integration/node-specific/auto_connect.test.ts b/test/integration/node-specific/auto_connect.test.ts index 7f8dbd1fe3b..adad2190f17 100644 --- a/test/integration/node-specific/auto_connect.test.ts +++ b/test/integration/node-specific/auto_connect.test.ts @@ -8,10 +8,12 @@ import { type Collection, type MongoClient, MongoNotConnectedError, + MongoOperationTimeoutError, ProfilingLevel, Topology, TopologyType } from '../../mongodb'; +import { type FailPoint } from '../../tools/utils'; describe('When executing an operation for the first time', () => { let client: MongoClient; @@ -821,4 +823,59 @@ describe('When executing an operation for the first time', () => { }); }); }); + + describe.only('and CSOT is enabled', function () { + let client: MongoClient; + + beforeEach(async function () { + client = this.configuration.newClient({ timeoutMS: 1000 }); + }); + + afterEach(async function () { + await client.close(); + }); + + describe('and nothing is wrong', function () { + it('should connect the client', async function () { + await client.connect(); + expect(client).to.have.property('topology').that.is.instanceOf(Topology); + }); + }); + + describe('and the server requires auth and ping is delayed', function () { + beforeEach(async function () { + // set failpoint to delay ping + // create new util client to avoid affecting the test client + const utilClient = this.configuration.newClient(); + await utilClient.db('admin').command({ + configureFailPoint: 'failCommand', + mode: { times: 1 }, + data: { failCommands: ['ping'], blockConnection: true, blockTimeMS: 2000 } + } as FailPoint); + await utilClient.close(); + }); + + it( + 'should throw an MongoOperationTimeoutError error from connect', + { requires: { auth: 'enabled' } }, + async function () { + const start = performance.now(); + const error = await client.connect().catch(error => error); + const end = performance.now(); + expect(error).to.be.instanceOf(MongoOperationTimeoutError); + expect(end - start).to.be.within(1000, 1500); + } + ); + + it( + 'still have a pending connect promise', + { requires: { auth: 'enabled' } }, + async function () { + const error = await client.connect().catch(error => error); + expect(client).to.have.property('connectionLock').that.is.instanceOf(Promise); + expect(error).to.be.instanceOf(MongoOperationTimeoutError); + } + ); + }); + }); }); From 67ae7275e920e6d7fb47abc14eea691a8b7e0444 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 15 Oct 2024 13:51:19 -0400 Subject: [PATCH 02/10] docs(NODE-6223): add documentation about auto-connect and CSOT --- src/mongo_client.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/mongo_client.ts b/src/mongo_client.ts index f9a5c2c1a40..e7202b9ac02 100644 --- a/src/mongo_client.ts +++ b/src/mongo_client.ts @@ -514,6 +514,13 @@ export class MongoClient extends TypedEventEmitter implements /** * Connect to MongoDB using a url * + * @remarks + * Calling connect is optional as the first operation you preform will call connect if it's needed. + * `timeoutMS` specified at the client-level will bound the time any operation can take before throwing a timeout error. + * However, when the operation being run is automatically connecting your MongoClient the timeoutMS will only be used for the operation portion of task. + * This means the time to setup the MongoClient does not count against timeoutMS. + * If you are using timeoutMS we recommend connecting your client explicitly in advance of any operation to avoid this inconsistent execution time. + * * @see docs.mongodb.org/manual/reference/connection-string/ */ async connect(): Promise { @@ -717,6 +724,13 @@ export class MongoClient extends TypedEventEmitter implements * Connect to MongoDB using a url * * @remarks + * Calling connect is optional as the first operation you preform will call connect if it's needed. + * `timeoutMS` specified at the client-level will bound the time any operation can take before throwing a timeout error. + * However, when the operation being run is automatically connecting your MongoClient the timeoutMS will only be used for the operation portion of task. + * This means the time to setup the MongoClient does not count against timeoutMS. + * If you are using timeoutMS we recommend connecting your client explicitly in advance of any operation to avoid this inconsistent execution time. + * + * @remarks * The programmatically provided options take precedence over the URI options. * * @see https://www.mongodb.com/docs/manual/reference/connection-string/ From 92a01802e40c55eddcdcca14a55d78f68d57a764 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 15 Oct 2024 14:18:08 -0400 Subject: [PATCH 03/10] chore: fix tests and remove changes --- src/mongo_client.ts | 19 +++----- src/timeout.ts | 6 --- .../node-specific/auto_connect.test.ts | 45 ++++++++++++++----- 3 files changed, 39 insertions(+), 31 deletions(-) diff --git a/src/mongo_client.ts b/src/mongo_client.ts index e7202b9ac02..bdc82f6d0d8 100644 --- a/src/mongo_client.ts +++ b/src/mongo_client.ts @@ -47,7 +47,6 @@ import { readPreferenceServerSelector } from './sdam/server_selection'; import type { SrvPoller } from './sdam/srv_polling'; import { Topology, type TopologyEvents } from './sdam/topology'; import { ClientSession, type ClientSessionOptions, ServerSessionPool } from './sessions'; -import { Timeout } from './timeout'; import { COSMOS_DB_CHECK, COSMOS_DB_MSG, @@ -528,19 +527,13 @@ export class MongoClient extends TypedEventEmitter implements return await this.connectionLock; } - const timeoutMS = this[kOptions].timeoutMS; - - this.connectionLock = this._connect().finally(() => { - // Clear the lock only when this promise finishes: + try { + this.connectionLock = this._connect(); + await this.connectionLock; + } finally { + // release this.connectionLock = undefined; - }); - - await (timeoutMS == null - ? this.connectionLock - : Promise.race([ - this.connectionLock, - Timeout.expires(timeoutMS).catch(Timeout.convertError) - ])); + } return this; } diff --git a/src/timeout.ts b/src/timeout.ts index 6465848ffca..9041ce4b88d 100644 --- a/src/timeout.ts +++ b/src/timeout.ts @@ -126,12 +126,6 @@ export class Timeout extends Promise { static override reject(rejection?: Error): Timeout { return new Timeout(undefined, { duration: 0, unref: true, rejection }); } - - static convertError(this: void, cause: unknown): never { - if (TimeoutError.is(cause)) throw new MongoOperationTimeoutError('Timed out'); - if (cause instanceof Error) throw cause; - throw new MongoRuntimeError('Unknown error', { cause: cause as any }); - } } /** @internal */ diff --git a/test/integration/node-specific/auto_connect.test.ts b/test/integration/node-specific/auto_connect.test.ts index adad2190f17..678e0c1b133 100644 --- a/test/integration/node-specific/auto_connect.test.ts +++ b/test/integration/node-specific/auto_connect.test.ts @@ -1,5 +1,6 @@ import { expect } from 'chai'; import { once } from 'events'; +import * as sinon from 'sinon'; import { BSONType, @@ -8,12 +9,11 @@ import { type Collection, type MongoClient, MongoNotConnectedError, - MongoOperationTimeoutError, ProfilingLevel, Topology, TopologyType } from '../../mongodb'; -import { type FailPoint } from '../../tools/utils'; +import { type FailPoint, sleep } from '../../tools/utils'; describe('When executing an operation for the first time', () => { let client: MongoClient; @@ -824,7 +824,7 @@ describe('When executing an operation for the first time', () => { }); }); - describe.only('and CSOT is enabled', function () { + describe('and CSOT is enabled', function () { let client: MongoClient; beforeEach(async function () { @@ -836,7 +836,7 @@ describe('When executing an operation for the first time', () => { }); describe('and nothing is wrong', function () { - it('should connect the client', async function () { + it('connects the client', async function () { await client.connect(); expect(client).to.have.property('topology').that.is.instanceOf(Topology); }); @@ -856,24 +856,45 @@ describe('When executing an operation for the first time', () => { }); it( - 'should throw an MongoOperationTimeoutError error from connect', + 'takes as long as ping is delayed for and does not throw a timeout error', { requires: { auth: 'enabled' } }, async function () { const start = performance.now(); - const error = await client.connect().catch(error => error); + const returnedClient = await client.connect(); const end = performance.now(); - expect(error).to.be.instanceOf(MongoOperationTimeoutError); - expect(end - start).to.be.within(1000, 1500); + expect(returnedClient).to.equal(client); + expect(end - start).to.be.within(2000, 2500); // timeoutMS is 1000, did not apply. } ); + }); + + describe('when server selection takes longer than the timeout', function () { + beforeEach(async function () { + const selectServerStub = sinon + .stub(Topology.prototype, 'selectServer') + .callsFake(async function (selector, options) { + await sleep(2000); + const result = selectServerStub.wrappedMethod.call(this, selector, options); + sinon.restore(); // restore after connect selection + return result; + }); + }); + + // restore sinon stub after test + afterEach(() => { + sinon.restore(); + }); it( - 'still have a pending connect promise', + 'takes as long as selectServer is delayed for and does not throw a timeout error', { requires: { auth: 'enabled' } }, async function () { - const error = await client.connect().catch(error => error); - expect(client).to.have.property('connectionLock').that.is.instanceOf(Promise); - expect(error).to.be.instanceOf(MongoOperationTimeoutError); + const start = performance.now(); + expect(client.topology).to.not.exist; // make sure not connected. + const res = await client.db().collection('test').insertOne({ a: 1 }, { timeoutMS: 1000 }); // auto-connect + const end = performance.now(); + expect(res).to.have.property('acknowledged', true); + expect(end - start).to.be.within(2000, 2500); // timeoutMS is 1000, did not apply. } ); }); From 512eea4ade1dd865a385c12abe3391b3f5ebec82 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 16 Oct 2024 11:09:27 -0400 Subject: [PATCH 04/10] chore: code review Co-authored-by: Aditi Khare <106987683+aditi-khare-mongoDB@users.noreply.github.com> --- src/mongo_client.ts | 16 ++++++++-------- .../node-specific/auto_connect.test.ts | 10 +++++----- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/mongo_client.ts b/src/mongo_client.ts index bdc82f6d0d8..de81519333d 100644 --- a/src/mongo_client.ts +++ b/src/mongo_client.ts @@ -514,11 +514,11 @@ export class MongoClient extends TypedEventEmitter implements * Connect to MongoDB using a url * * @remarks - * Calling connect is optional as the first operation you preform will call connect if it's needed. + * Calling `connect` is optional since the first operation you perform will call `connect` if it's needed. * `timeoutMS` specified at the client-level will bound the time any operation can take before throwing a timeout error. - * However, when the operation being run is automatically connecting your MongoClient the timeoutMS will only be used for the operation portion of task. - * This means the time to setup the MongoClient does not count against timeoutMS. - * If you are using timeoutMS we recommend connecting your client explicitly in advance of any operation to avoid this inconsistent execution time. + * However, when the operation being run is automatically connecting your `MongoClient` the timeoutMS will only be used for the operation portion of task. + * This means the time to setup the `MongoClient` does not count against `timeoutMS`. + * If you are using `timeoutMS` we recommend connecting your client explicitly in advance of any operation to avoid this inconsistent execution time. * * @see docs.mongodb.org/manual/reference/connection-string/ */ @@ -717,11 +717,11 @@ export class MongoClient extends TypedEventEmitter implements * Connect to MongoDB using a url * * @remarks - * Calling connect is optional as the first operation you preform will call connect if it's needed. + * Calling `connect` is optional since the first operation you perform will call `connect` if it's needed. * `timeoutMS` specified at the client-level will bound the time any operation can take before throwing a timeout error. - * However, when the operation being run is automatically connecting your MongoClient the timeoutMS will only be used for the operation portion of task. - * This means the time to setup the MongoClient does not count against timeoutMS. - * If you are using timeoutMS we recommend connecting your client explicitly in advance of any operation to avoid this inconsistent execution time. + * However, when the operation being run is automatically connecting your `MongoClient` the timeoutMS will only be used for the operation portion of task. + * This means the time to setup the `MongoClient` does not count against `timeoutMS`. + * If you are using `timeoutMS` we recommend connecting your client explicitly in advance of any operation to avoid this inconsistent execution time. * * @remarks * The programmatically provided options take precedence over the URI options. diff --git a/test/integration/node-specific/auto_connect.test.ts b/test/integration/node-specific/auto_connect.test.ts index 678e0c1b133..52b58257c84 100644 --- a/test/integration/node-specific/auto_connect.test.ts +++ b/test/integration/node-specific/auto_connect.test.ts @@ -824,7 +824,7 @@ describe('When executing an operation for the first time', () => { }); }); - describe('and CSOT is enabled', function () { + describe('when CSOT is enabled', function () { let client: MongoClient; beforeEach(async function () { @@ -835,14 +835,14 @@ describe('When executing an operation for the first time', () => { await client.close(); }); - describe('and nothing is wrong', function () { + describe('when nothing is wrong', function () { it('connects the client', async function () { await client.connect(); expect(client).to.have.property('topology').that.is.instanceOf(Topology); }); }); - describe('and the server requires auth and ping is delayed', function () { + describe('when the server requires auth and ping is delayed', function () { beforeEach(async function () { // set failpoint to delay ping // create new util client to avoid affecting the test client @@ -856,7 +856,7 @@ describe('When executing an operation for the first time', () => { }); it( - 'takes as long as ping is delayed for and does not throw a timeout error', + 'client.connect() takes as long as ping is delayed for and does not throw a timeout error', { requires: { auth: 'enabled' } }, async function () { const start = performance.now(); @@ -886,7 +886,7 @@ describe('When executing an operation for the first time', () => { }); it( - 'takes as long as selectServer is delayed for and does not throw a timeout error', + 'client.connect() 'takes as long as selectServer is delayed for and does not throw a timeout error', { requires: { auth: 'enabled' } }, async function () { const start = performance.now(); From ab546eb5869850a692829679fefd1e8be6aa1b1a Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 16 Oct 2024 11:11:26 -0400 Subject: [PATCH 05/10] chore: fix string --- test/integration/node-specific/auto_connect.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/node-specific/auto_connect.test.ts b/test/integration/node-specific/auto_connect.test.ts index 52b58257c84..78ac8302ecd 100644 --- a/test/integration/node-specific/auto_connect.test.ts +++ b/test/integration/node-specific/auto_connect.test.ts @@ -886,7 +886,7 @@ describe('When executing an operation for the first time', () => { }); it( - 'client.connect() 'takes as long as selectServer is delayed for and does not throw a timeout error', + 'client.connect() takes as long as selectServer is delayed for and does not throw a timeout error', { requires: { auth: 'enabled' } }, async function () { const start = performance.now(); From 33270d2c2022e35fdd44624bd3edd60efc25909b Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 17 Oct 2024 10:47:19 -0400 Subject: [PATCH 06/10] chore: comments Co-authored-by: Daria Pardue --- src/mongo_client.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/mongo_client.ts b/src/mongo_client.ts index de81519333d..c4217f323af 100644 --- a/src/mongo_client.ts +++ b/src/mongo_client.ts @@ -516,7 +516,7 @@ export class MongoClient extends TypedEventEmitter implements * @remarks * Calling `connect` is optional since the first operation you perform will call `connect` if it's needed. * `timeoutMS` specified at the client-level will bound the time any operation can take before throwing a timeout error. - * However, when the operation being run is automatically connecting your `MongoClient` the timeoutMS will only be used for the operation portion of task. + * However, when the operation being run is automatically connecting your `MongoClient` the `timeoutMS` will only be used for the operation portion of task. * This means the time to setup the `MongoClient` does not count against `timeoutMS`. * If you are using `timeoutMS` we recommend connecting your client explicitly in advance of any operation to avoid this inconsistent execution time. * @@ -719,7 +719,7 @@ export class MongoClient extends TypedEventEmitter implements * @remarks * Calling `connect` is optional since the first operation you perform will call `connect` if it's needed. * `timeoutMS` specified at the client-level will bound the time any operation can take before throwing a timeout error. - * However, when the operation being run is automatically connecting your `MongoClient` the timeoutMS will only be used for the operation portion of task. + * However, when the operation being run is automatically connecting your `MongoClient` the `timeoutMS` will only be used for the operation portion of task. * This means the time to setup the `MongoClient` does not count against `timeoutMS`. * If you are using `timeoutMS` we recommend connecting your client explicitly in advance of any operation to avoid this inconsistent execution time. * From f38430b0e8f886d398d5bcbdaeac3032c220782c Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 21 Oct 2024 11:20:23 -0400 Subject: [PATCH 07/10] chore: bailey's comments --- src/mongo_client.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/mongo_client.ts b/src/mongo_client.ts index c4217f323af..ce548756fa2 100644 --- a/src/mongo_client.ts +++ b/src/mongo_client.ts @@ -515,8 +515,8 @@ export class MongoClient extends TypedEventEmitter implements * * @remarks * Calling `connect` is optional since the first operation you perform will call `connect` if it's needed. - * `timeoutMS` specified at the client-level will bound the time any operation can take before throwing a timeout error. - * However, when the operation being run is automatically connecting your `MongoClient` the `timeoutMS` will only be used for the operation portion of task. + * `timeoutMS` will bound the time any operation can take before throwing a timeout error. + * However, when the operation being run is automatically connecting your `MongoClient` the `timeoutMS` will not apply to the time taken to connect the MongoClient. * This means the time to setup the `MongoClient` does not count against `timeoutMS`. * If you are using `timeoutMS` we recommend connecting your client explicitly in advance of any operation to avoid this inconsistent execution time. * @@ -718,8 +718,8 @@ export class MongoClient extends TypedEventEmitter implements * * @remarks * Calling `connect` is optional since the first operation you perform will call `connect` if it's needed. - * `timeoutMS` specified at the client-level will bound the time any operation can take before throwing a timeout error. - * However, when the operation being run is automatically connecting your `MongoClient` the `timeoutMS` will only be used for the operation portion of task. + * `timeoutMS` will bound the time any operation can take before throwing a timeout error. + * However, when the operation being run is automatically connecting your `MongoClient` the `timeoutMS` will not apply to the time taken to connect the MongoClient. * This means the time to setup the `MongoClient` does not count against `timeoutMS`. * If you are using `timeoutMS` we recommend connecting your client explicitly in advance of any operation to avoid this inconsistent execution time. * From ecd084ff79097199f9547caf30cbc5bba98aca80 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 22 Oct 2024 13:54:37 -0400 Subject: [PATCH 08/10] chore: add 4.4 filter --- .../node-specific/auto_connect.test.ts | 86 +++++++++---------- 1 file changed, 43 insertions(+), 43 deletions(-) diff --git a/test/integration/node-specific/auto_connect.test.ts b/test/integration/node-specific/auto_connect.test.ts index 78ac8302ecd..8a2d8123238 100644 --- a/test/integration/node-specific/auto_connect.test.ts +++ b/test/integration/node-specific/auto_connect.test.ts @@ -842,61 +842,61 @@ describe('When executing an operation for the first time', () => { }); }); - describe('when the server requires auth and ping is delayed', function () { - beforeEach(async function () { - // set failpoint to delay ping - // create new util client to avoid affecting the test client - const utilClient = this.configuration.newClient(); - await utilClient.db('admin').command({ - configureFailPoint: 'failCommand', - mode: { times: 1 }, - data: { failCommands: ['ping'], blockConnection: true, blockTimeMS: 2000 } - } as FailPoint); - await utilClient.close(); - }); - - it( - 'client.connect() takes as long as ping is delayed for and does not throw a timeout error', - { requires: { auth: 'enabled' } }, - async function () { + describe( + 'when the server requires auth and ping is delayed', + { requires: { auth: 'enabled', mongodb: '>=4.4' } }, + function () { + beforeEach(async function () { + // set failpoint to delay ping + // create new util client to avoid affecting the test client + const utilClient = this.configuration.newClient(); + await utilClient.db('admin').command({ + configureFailPoint: 'failCommand', + mode: { times: 1 }, + data: { failCommands: ['ping'], blockConnection: true, blockTimeMS: 2000 } + } as FailPoint); + await utilClient.close(); + }); + + it('client.connect() takes as long as ping is delayed for and does not throw a timeout error', async function () { const start = performance.now(); const returnedClient = await client.connect(); const end = performance.now(); expect(returnedClient).to.equal(client); expect(end - start).to.be.within(2000, 2500); // timeoutMS is 1000, did not apply. - } - ); - }); - - describe('when server selection takes longer than the timeout', function () { - beforeEach(async function () { - const selectServerStub = sinon - .stub(Topology.prototype, 'selectServer') - .callsFake(async function (selector, options) { - await sleep(2000); - const result = selectServerStub.wrappedMethod.call(this, selector, options); - sinon.restore(); // restore after connect selection - return result; - }); - }); + }); + } + ); + + describe( + 'when server selection takes longer than the timeout', + { requires: { auth: 'enabled', mongodb: '>=4.4' } }, + function () { + beforeEach(async function () { + const selectServerStub = sinon + .stub(Topology.prototype, 'selectServer') + .callsFake(async function (selector, options) { + await sleep(2000); + const result = selectServerStub.wrappedMethod.call(this, selector, options); + sinon.restore(); // restore after connect selection + return result; + }); + }); - // restore sinon stub after test - afterEach(() => { - sinon.restore(); - }); + // restore sinon stub after test + afterEach(() => { + sinon.restore(); + }); - it( - 'client.connect() takes as long as selectServer is delayed for and does not throw a timeout error', - { requires: { auth: 'enabled' } }, - async function () { + it('client.connect() takes as long as selectServer is delayed for and does not throw a timeout error', async function () { const start = performance.now(); expect(client.topology).to.not.exist; // make sure not connected. const res = await client.db().collection('test').insertOne({ a: 1 }, { timeoutMS: 1000 }); // auto-connect const end = performance.now(); expect(res).to.have.property('acknowledged', true); expect(end - start).to.be.within(2000, 2500); // timeoutMS is 1000, did not apply. - } - ); - }); + }); + } + ); }); }); From b97eb7326243f7178b27d6fcde98d907bce419e9 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 23 Oct 2024 14:26:54 -0400 Subject: [PATCH 09/10] test: agnostic connect coverage --- .../node-specific/auto_connect.test.ts | 38 +++++++++++++++---- 1 file changed, 31 insertions(+), 7 deletions(-) diff --git a/test/integration/node-specific/auto_connect.test.ts b/test/integration/node-specific/auto_connect.test.ts index 8a2d8123238..0c431a1de29 100644 --- a/test/integration/node-specific/auto_connect.test.ts +++ b/test/integration/node-specific/auto_connect.test.ts @@ -7,7 +7,7 @@ import { type ChangeStream, ClientSession, type Collection, - type MongoClient, + MongoClient, MongoNotConnectedError, ProfilingLevel, Topology, @@ -828,7 +828,7 @@ describe('When executing an operation for the first time', () => { let client: MongoClient; beforeEach(async function () { - client = this.configuration.newClient({ timeoutMS: 1000 }); + client = this.configuration.newClient({ timeoutMS: 500 }); }); afterEach(async function () { @@ -853,7 +853,7 @@ describe('When executing an operation for the first time', () => { await utilClient.db('admin').command({ configureFailPoint: 'failCommand', mode: { times: 1 }, - data: { failCommands: ['ping'], blockConnection: true, blockTimeMS: 2000 } + data: { failCommands: ['ping'], blockConnection: true, blockTimeMS: 1000 } } as FailPoint); await utilClient.close(); }); @@ -863,7 +863,7 @@ describe('When executing an operation for the first time', () => { const returnedClient = await client.connect(); const end = performance.now(); expect(returnedClient).to.equal(client); - expect(end - start).to.be.within(2000, 2500); // timeoutMS is 1000, did not apply. + expect(end - start).to.be.within(1000, 1500); // timeoutMS is 1000, did not apply. }); } ); @@ -876,7 +876,7 @@ describe('When executing an operation for the first time', () => { const selectServerStub = sinon .stub(Topology.prototype, 'selectServer') .callsFake(async function (selector, options) { - await sleep(2000); + await sleep(1000); const result = selectServerStub.wrappedMethod.call(this, selector, options); sinon.restore(); // restore after connect selection return result; @@ -891,12 +891,36 @@ describe('When executing an operation for the first time', () => { it('client.connect() takes as long as selectServer is delayed for and does not throw a timeout error', async function () { const start = performance.now(); expect(client.topology).to.not.exist; // make sure not connected. - const res = await client.db().collection('test').insertOne({ a: 1 }, { timeoutMS: 1000 }); // auto-connect + const res = await client.db().collection('test').insertOne({ a: 1 }, { timeoutMS: 500 }); // auto-connect const end = performance.now(); expect(res).to.have.property('acknowledged', true); - expect(end - start).to.be.within(2000, 2500); // timeoutMS is 1000, did not apply. + expect(end - start).to.be.within(1000, 1500); // timeoutMS is 1000, did not apply. }); } ); + + describe('when auto connect is used and connect() takes longer than timeoutMS', function () { + // This test stubs the connect method to check that connect() does not get timed out + // vs. the test above makes sure that the `ping` does not inherit the client's timeoutMS setting + beforeEach(async function () { + const connectStub = sinon + .stub(MongoClient.prototype, 'connect') + .callsFake(async function () { + await sleep(1000); + const result = connectStub.wrappedMethod.call(this); + sinon.restore(); // restore after connect selection + return result; + }); + }); + + it('the operation succeeds', async function () { + const start = performance.now(); + expect(client.topology).to.not.exist; // make sure not connected. + const res = await client.db().collection('test').insertOne({ a: 1 }); // auto-connect + const end = performance.now(); + expect(res).to.have.property('acknowledged', true); + expect(end - start).to.be.within(1000, 1500); // timeoutMS is 1000, did not apply. + }); + }); }); }); From b9e0010b5b538de159f5d062b44e265ec7d08aa4 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 23 Oct 2024 15:09:44 -0400 Subject: [PATCH 10/10] test name --- test/integration/node-specific/auto_connect.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/node-specific/auto_connect.test.ts b/test/integration/node-specific/auto_connect.test.ts index 0c431a1de29..3e56b69fbef 100644 --- a/test/integration/node-specific/auto_connect.test.ts +++ b/test/integration/node-specific/auto_connect.test.ts @@ -858,7 +858,7 @@ describe('When executing an operation for the first time', () => { await utilClient.close(); }); - it('client.connect() takes as long as ping is delayed for and does not throw a timeout error', async function () { + it('timeoutMS from the client is not used for the internal `ping`', async function () { const start = performance.now(); const returnedClient = await client.connect(); const end = performance.now();