From b4bb0334c6eb035464ea7447e53c86ad1779ef63 Mon Sep 17 00:00:00 2001 From: clemclx Date: Fri, 10 Mar 2023 16:29:27 +0100 Subject: [PATCH 01/10] fix(NODE-5106): Add MongoClient connect lock mechanism to avoid topology leak --- src/mongo_client.ts | 73 ++++++++++++++++++++++++++------------------- 1 file changed, 43 insertions(+), 30 deletions(-) diff --git a/src/mongo_client.ts b/src/mongo_client.ts index 7dcd712d299..4e1e1991bed 100644 --- a/src/mongo_client.ts +++ b/src/mongo_client.ts @@ -318,6 +318,8 @@ export class MongoClient extends TypedEventEmitter { topology?: Topology; /** @internal */ readonly mongoLogger: MongoLogger; + /** @internal */ + private connectionLock?: Promise; /** * The consolidate, parsed, transformed and merged options. @@ -409,46 +411,57 @@ export class MongoClient extends TypedEventEmitter { return this; } - const options = this[kOptions]; + if (this.connectionLock) { + return this.connectionLock; + } + + this.connectionLock = (async () => { + const options = this[kOptions]; - if (typeof options.srvHost === 'string') { - const hosts = await resolveSRVRecord(options); + if (typeof options.srvHost === 'string') { + const hosts = await resolveSRVRecord(options); - for (const [index, host] of hosts.entries()) { - options.hosts[index] = host; + for (const [index, host] of hosts.entries()) { + options.hosts[index] = host; + } } - } - const topology = new Topology(options.hosts, options); - // Events can be emitted before initialization is complete so we have to - // save the reference to the topology on the client ASAP if the event handlers need to access it - this.topology = topology; - topology.client = this; + const topology = new Topology(options.hosts, options); + // Events can be emitted before initialization is complete so we have to + // save the reference to the topology on the client ASAP if the event handlers need to access it + this.topology = topology; + topology.client = this; - topology.once(Topology.OPEN, () => this.emit('open', this)); + topology.once(Topology.OPEN, () => this.emit('open', this)); - for (const event of MONGO_CLIENT_EVENTS) { - topology.on(event, (...args: any[]) => this.emit(event, ...(args as any))); - } + for (const event of MONGO_CLIENT_EVENTS) { + topology.on(event, (...args: any[]) => this.emit(event, ...(args as any))); + } - const topologyConnect = async () => { - try { - await promisify(callback => topology.connect(options, callback))(); - } catch (error) { - topology.close({ force: true }); - throw error; + const topologyConnect = async () => { + try { + await promisify(callback => topology.connect(options, callback))(); + } catch (error) { + topology.close({ force: true }); + throw error; + } + }; + + if (this.autoEncrypter) { + const initAutoEncrypter = promisify(callback => this.autoEncrypter?.init(callback)); + await initAutoEncrypter(); + await topologyConnect(); + await options.encrypter.connectInternalClient(); + } else { + await topologyConnect(); } - }; - if (this.autoEncrypter) { - const initAutoEncrypter = promisify(callback => this.autoEncrypter?.init(callback)); - await initAutoEncrypter(); - await topologyConnect(); - await options.encrypter.connectInternalClient(); - } else { - await topologyConnect(); - } + return this; + })(); + await this.connectionLock; + // release + this.connectionLock = undefined; return this; } From b139f17f26e43916c8c214bb1779a389bdd66298 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Cloux?= Date: Sat, 11 Mar 2023 13:47:44 +0000 Subject: [PATCH 02/10] fix(NODE-5106): MongoClient connect release lock if promise throw --- src/mongo_client.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/mongo_client.ts b/src/mongo_client.ts index 4e1e1991bed..fa286b2efa9 100644 --- a/src/mongo_client.ts +++ b/src/mongo_client.ts @@ -459,9 +459,13 @@ export class MongoClient extends TypedEventEmitter { return this; })(); - await this.connectionLock; - // release - this.connectionLock = undefined; + try { + await this.connectionLock; + } finally { + // release + this.connectionLock = undefined; + } + return this; } From 9bc400dbaf9989ffa9b21feacda8a6646e078597 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Cloux?= Date: Mon, 13 Mar 2023 21:41:17 +0100 Subject: [PATCH 03/10] fix(NODE-5106): refactor connect locking and add test --- src/mongo_client.ts | 97 +++++++++++++++------------ test/integration/mongo_client.test.ts | 50 ++++++++++++++ 2 files changed, 103 insertions(+), 44 deletions(-) create mode 100644 test/integration/mongo_client.test.ts diff --git a/src/mongo_client.ts b/src/mongo_client.ts index fa286b2efa9..97d6e9232ed 100644 --- a/src/mongo_client.ts +++ b/src/mongo_client.ts @@ -407,63 +407,72 @@ export class MongoClient extends TypedEventEmitter { * @see docs.mongodb.org/manual/reference/connection-string/ */ async connect(): Promise { - if (this.topology && this.topology.isConnected()) { - return this; - } - if (this.connectionLock) { return this.connectionLock; } - this.connectionLock = (async () => { - const options = this[kOptions]; + try { + this.connectionLock = this._connect(); + await this.connectionLock; + } finally { + // release + this.connectionLock = undefined; + } - if (typeof options.srvHost === 'string') { - const hosts = await resolveSRVRecord(options); + return this; + } - for (const [index, host] of hosts.entries()) { - options.hosts[index] = host; - } - } + /** + * Create a topology to open the connection, must be locked to avoid topology leaks in concurrency scenario. + * Locking is enforced by the connect method. + * + * When decorators available put implementation back to original connect methods + * and enforce locking via a dedicated decorator. + * @see https://github.com/mongodb/node-mongodb-native/pull/3596#discussion_r1134211500 + */ + private async _connect(): Promise { + if (this.topology && this.topology.isConnected()) { + return this; + } - const topology = new Topology(options.hosts, options); - // Events can be emitted before initialization is complete so we have to - // save the reference to the topology on the client ASAP if the event handlers need to access it - this.topology = topology; - topology.client = this; + const options = this[kOptions]; - topology.once(Topology.OPEN, () => this.emit('open', this)); + if (typeof options.srvHost === 'string') { + const hosts = await resolveSRVRecord(options); - for (const event of MONGO_CLIENT_EVENTS) { - topology.on(event, (...args: any[]) => this.emit(event, ...(args as any))); + for (const [index, host] of hosts.entries()) { + options.hosts[index] = host; } + } - const topologyConnect = async () => { - try { - await promisify(callback => topology.connect(options, callback))(); - } catch (error) { - topology.close({ force: true }); - throw error; - } - }; - - if (this.autoEncrypter) { - const initAutoEncrypter = promisify(callback => this.autoEncrypter?.init(callback)); - await initAutoEncrypter(); - await topologyConnect(); - await options.encrypter.connectInternalClient(); - } else { - await topologyConnect(); - } + const topology = new Topology(options.hosts, options); + // Events can be emitted before initialization is complete so we have to + // save the reference to the topology on the client ASAP if the event handlers need to access it + this.topology = topology; + topology.client = this; - return this; - })(); + topology.once(Topology.OPEN, () => this.emit('open', this)); - try { - await this.connectionLock; - } finally { - // release - this.connectionLock = undefined; + for (const event of MONGO_CLIENT_EVENTS) { + topology.on(event, (...args: any[]) => this.emit(event, ...(args as any))); + } + + const topologyConnect = async () => { + try { + await promisify(callback => topology.connect(options, callback))(); + } catch (error) { + topology.close({ force: true }); + throw error; + } + }; + + if (this.autoEncrypter) { + const initAutoEncrypter = promisify(callback => this.autoEncrypter?.init(callback)); + await initAutoEncrypter(); + await topologyConnect(); + await options.encrypter.connectInternalClient(); + } else { + await topologyConnect(); } return this; diff --git a/test/integration/mongo_client.test.ts b/test/integration/mongo_client.test.ts new file mode 100644 index 00000000000..7de30e161f2 --- /dev/null +++ b/test/integration/mongo_client.test.ts @@ -0,0 +1,50 @@ +import { expect } from 'chai'; +import * as sinon from 'sinon'; + +import { MongoClient } from '../../src'; + +describe('MongoClient', () => { + let client: MongoClient; + let topologyOpenEvents; + + beforeEach(async function () { + client = this.configuration.newClient(); + topologyOpenEvents = []; + client.on('open', event => topologyOpenEvents.push(event)); + }); + + afterEach(async function () { + await client.close(); + }); + + it('Concurrents client connect correctly locked (only one topology created)', async function () { + await Promise.all([client.connect(), client.connect(), client.connect()]); + + expect(topologyOpenEvents).to.have.lengthOf(1); + expect(client.topology?.isConnected()).to.be.true; + }); + + it('Failed client connect must properly release lock', async function () { + const internalConnectStub = sinon.stub(client, '_connect' as keyof MongoClient); + internalConnectStub.onFirstCall().rejects(); + + // first call rejected to simulate a connection failure + try { + await client.connect(); + } catch (err) { + expect(err).to.exist; + } + + internalConnectStub.restore(); + + // second call should connect + try { + await client.connect(); + } catch (err) { + expect.fail(`client connect throwed unexpected error`); + } + + expect(topologyOpenEvents).to.have.lengthOf(1); + expect(client.topology?.isConnected()).to.be.true; + }); +}); From ca3fa4dae0e91055346fc3034771161b62f6e455 Mon Sep 17 00:00:00 2001 From: clemclx Date: Wed, 15 Mar 2023 14:16:44 +0100 Subject: [PATCH 04/10] fix(NODE-5106): Fix tests to pass leak_checker --- test/integration/mongo_client.test.ts | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/test/integration/mongo_client.test.ts b/test/integration/mongo_client.test.ts index 7de30e161f2..7f71251e014 100644 --- a/test/integration/mongo_client.test.ts +++ b/test/integration/mongo_client.test.ts @@ -7,18 +7,36 @@ describe('MongoClient', () => { let client: MongoClient; let topologyOpenEvents; + /** Keep track number of call to client connect to close as many as connect (otherwise leak_checker hook will failed) */ + let clientConnectCounter: number; + + /** + * Wrap the connect method of the client to keep track + * of number of times connect is called + */ + function clientConnect() { + if (!client) { + return; + } + clientConnectCounter++; + return client.connect(); + } + beforeEach(async function () { client = this.configuration.newClient(); topologyOpenEvents = []; + clientConnectCounter = 0; client.on('open', event => topologyOpenEvents.push(event)); }); afterEach(async function () { - await client.close(); + /** Close as many times as connect calls in the runned test (tracked by clientConnectCounter) */ + const clientClosePromises = [...new Array(clientConnectCounter)].map(() => client.close()); + await Promise.all(clientClosePromises); }); it('Concurrents client connect correctly locked (only one topology created)', async function () { - await Promise.all([client.connect(), client.connect(), client.connect()]); + await Promise.all([clientConnect(), clientConnect(), clientConnect()]); expect(topologyOpenEvents).to.have.lengthOf(1); expect(client.topology?.isConnected()).to.be.true; @@ -30,7 +48,7 @@ describe('MongoClient', () => { // first call rejected to simulate a connection failure try { - await client.connect(); + await clientConnect(); } catch (err) { expect(err).to.exist; } @@ -39,7 +57,7 @@ describe('MongoClient', () => { // second call should connect try { - await client.connect(); + await clientConnect(); } catch (err) { expect.fail(`client connect throwed unexpected error`); } From d87fc74568226bbbb790e5bf34e92e486eb0c0e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Cloux?= Date: Thu, 23 Mar 2023 20:38:38 +0100 Subject: [PATCH 05/10] fix(NODE-5106): fix test MongoClient import Co-authored-by: Neal Beeken --- test/integration/mongo_client.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/mongo_client.test.ts b/test/integration/mongo_client.test.ts index 7f71251e014..b98f5e9098c 100644 --- a/test/integration/mongo_client.test.ts +++ b/test/integration/mongo_client.test.ts @@ -1,7 +1,7 @@ import { expect } from 'chai'; import * as sinon from 'sinon'; -import { MongoClient } from '../../src'; +import { MongoClient } from '../mongodb'; describe('MongoClient', () => { let client: MongoClient; From 3987c0055d3bb01855ac47102e10db487f42e632 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Cloux?= Date: Thu, 23 Mar 2023 20:41:06 +0100 Subject: [PATCH 06/10] fix(NODE-5106): Improve promises close in test Co-authored-by: Neal Beeken --- test/integration/mongo_client.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/mongo_client.test.ts b/test/integration/mongo_client.test.ts index b98f5e9098c..0c537649cbf 100644 --- a/test/integration/mongo_client.test.ts +++ b/test/integration/mongo_client.test.ts @@ -31,7 +31,7 @@ describe('MongoClient', () => { afterEach(async function () { /** Close as many times as connect calls in the runned test (tracked by clientConnectCounter) */ - const clientClosePromises = [...new Array(clientConnectCounter)].map(() => client.close()); + const clientClosePromises = Array.from({ length: clientConnectCounter }, () => client.close()); await Promise.all(clientClosePromises); }); From 79b7db57127254ebad927e140a6e82ff01d36b91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Cloux?= Date: Thu, 23 Mar 2023 20:41:35 +0100 Subject: [PATCH 07/10] fix(NODE-5106): Improve doc Co-authored-by: Neal Beeken --- test/integration/mongo_client.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/mongo_client.test.ts b/test/integration/mongo_client.test.ts index 0c537649cbf..7847952839a 100644 --- a/test/integration/mongo_client.test.ts +++ b/test/integration/mongo_client.test.ts @@ -30,7 +30,7 @@ describe('MongoClient', () => { }); afterEach(async function () { - /** Close as many times as connect calls in the runned test (tracked by clientConnectCounter) */ + // close `clientConnectCounter` times const clientClosePromises = Array.from({ length: clientConnectCounter }, () => client.close()); await Promise.all(clientClosePromises); }); From 5840d7913aad71cba30c61b77254941fe3279b79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Cloux?= Date: Thu, 23 Mar 2023 20:42:01 +0100 Subject: [PATCH 08/10] fix(NODE-5106): Improve doc Co-authored-by: Neal Beeken --- test/integration/mongo_client.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/mongo_client.test.ts b/test/integration/mongo_client.test.ts index 7847952839a..496d63e1a40 100644 --- a/test/integration/mongo_client.test.ts +++ b/test/integration/mongo_client.test.ts @@ -35,7 +35,7 @@ describe('MongoClient', () => { await Promise.all(clientClosePromises); }); - it('Concurrents client connect correctly locked (only one topology created)', async function () { + it('parallel client connect calls only create one topology', async function () { await Promise.all([clientConnect(), clientConnect(), clientConnect()]); expect(topologyOpenEvents).to.have.lengthOf(1); From 8ca8f819e238fb525977dc9f76400c3bb1aee8ad Mon Sep 17 00:00:00 2001 From: clemclx Date: Tue, 28 Mar 2023 13:58:02 +0200 Subject: [PATCH 09/10] fix(NODE-5106): refactor testing --- src/mongo_client.ts | 4 +- test/integration/mongo_client.test.ts | 68 ------------------- .../node-specific/mongo_client.test.ts | 59 ++++++++++++++++ 3 files changed, 60 insertions(+), 71 deletions(-) delete mode 100644 test/integration/mongo_client.test.ts diff --git a/src/mongo_client.ts b/src/mongo_client.ts index 97d6e9232ed..44372d653c0 100644 --- a/src/mongo_client.ts +++ b/src/mongo_client.ts @@ -426,9 +426,7 @@ export class MongoClient extends TypedEventEmitter { * Create a topology to open the connection, must be locked to avoid topology leaks in concurrency scenario. * Locking is enforced by the connect method. * - * When decorators available put implementation back to original connect methods - * and enforce locking via a dedicated decorator. - * @see https://github.com/mongodb/node-mongodb-native/pull/3596#discussion_r1134211500 + * @internal */ private async _connect(): Promise { if (this.topology && this.topology.isConnected()) { diff --git a/test/integration/mongo_client.test.ts b/test/integration/mongo_client.test.ts deleted file mode 100644 index 496d63e1a40..00000000000 --- a/test/integration/mongo_client.test.ts +++ /dev/null @@ -1,68 +0,0 @@ -import { expect } from 'chai'; -import * as sinon from 'sinon'; - -import { MongoClient } from '../mongodb'; - -describe('MongoClient', () => { - let client: MongoClient; - let topologyOpenEvents; - - /** Keep track number of call to client connect to close as many as connect (otherwise leak_checker hook will failed) */ - let clientConnectCounter: number; - - /** - * Wrap the connect method of the client to keep track - * of number of times connect is called - */ - function clientConnect() { - if (!client) { - return; - } - clientConnectCounter++; - return client.connect(); - } - - beforeEach(async function () { - client = this.configuration.newClient(); - topologyOpenEvents = []; - clientConnectCounter = 0; - client.on('open', event => topologyOpenEvents.push(event)); - }); - - afterEach(async function () { - // close `clientConnectCounter` times - const clientClosePromises = Array.from({ length: clientConnectCounter }, () => client.close()); - await Promise.all(clientClosePromises); - }); - - it('parallel client connect calls only create one topology', async function () { - await Promise.all([clientConnect(), clientConnect(), clientConnect()]); - - expect(topologyOpenEvents).to.have.lengthOf(1); - expect(client.topology?.isConnected()).to.be.true; - }); - - it('Failed client connect must properly release lock', async function () { - const internalConnectStub = sinon.stub(client, '_connect' as keyof MongoClient); - internalConnectStub.onFirstCall().rejects(); - - // first call rejected to simulate a connection failure - try { - await clientConnect(); - } catch (err) { - expect(err).to.exist; - } - - internalConnectStub.restore(); - - // second call should connect - try { - await clientConnect(); - } catch (err) { - expect.fail(`client connect throwed unexpected error`); - } - - expect(topologyOpenEvents).to.have.lengthOf(1); - expect(client.topology?.isConnected()).to.be.true; - }); -}); diff --git a/test/integration/node-specific/mongo_client.test.ts b/test/integration/node-specific/mongo_client.test.ts index 4e0103b3490..5b1ae1d973d 100644 --- a/test/integration/node-specific/mongo_client.test.ts +++ b/test/integration/node-specific/mongo_client.test.ts @@ -517,6 +517,65 @@ describe('class MongoClient', function () { ); }); + context('concurrent #connect()', () => { + let client: MongoClient; + let topologyOpenEvents; + + /** Keep track number of call to client connect to close as many as connect (otherwise leak_checker hook will failed) */ + let clientConnectCounter: number; + + /** + * Wrap the connect method of the client to keep track + * of number of times connect is called + */ + function clientConnect() { + if (!client) { + return; + } + clientConnectCounter++; + return client.connect(); + } + + beforeEach(async function () { + client = this.configuration.newClient(); + topologyOpenEvents = []; + clientConnectCounter = 0; + client.on('open', event => topologyOpenEvents.push(event)); + }); + + afterEach(async function () { + // close `clientConnectCounter` times + const clientClosePromises = Array.from({ length: clientConnectCounter }, () => + client.close() + ); + await Promise.all(clientClosePromises); + }); + + it('parallel client connect calls only create one topology', async function () { + await Promise.all([clientConnect(), clientConnect(), clientConnect()]); + + expect(topologyOpenEvents).to.have.lengthOf(1); + expect(client.topology?.isConnected()).to.be.true; + }); + + it('when connect rejects lock is released regardless', async function () { + const internalConnectStub = sinon.stub(client, '_connect' as keyof MongoClient); + internalConnectStub.onFirstCall().rejects(new Error('cannot connect')); + + // first call rejected to simulate a connection failure + const error = await clientConnect()!.catch(error => error); + expect(error).to.match(/cannot connect/); + + internalConnectStub.restore(); + + // second call should connect + await clientConnect(); + + expect(topologyOpenEvents).to.have.lengthOf(1); + expect(client.topology?.isConnected()).to.be.true; + }); + }); + context('#close()', () => { let client: MongoClient; let db: Db; From cb65e4b565140f03635af7e026ba6d372ab4289e Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Tue, 28 Mar 2023 12:51:49 -0400 Subject: [PATCH 10/10] chore: fix lint --- test/integration/node-specific/mongo_client.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/node-specific/mongo_client.test.ts b/test/integration/node-specific/mongo_client.test.ts index 5b1ae1d973d..f92ccee05ac 100644 --- a/test/integration/node-specific/mongo_client.test.ts +++ b/test/integration/node-specific/mongo_client.test.ts @@ -528,7 +528,7 @@ describe('class MongoClient', function () { * Wrap the connect method of the client to keep track * of number of times connect is called */ - function clientConnect() { + async function clientConnect() { if (!client) { return; } @@ -563,7 +563,7 @@ describe('class MongoClient', function () { internalConnectStub.onFirstCall().rejects(new Error('cannot connect')); // first call rejected to simulate a connection failure - const error = await clientConnect()!.catch(error => error); + const error = await clientConnect().catch(error => error); expect(error).to.match(/cannot connect/); internalConnectStub.restore();