From 68da44615ac07764d34125379d4976d883ecbac6 Mon Sep 17 00:00:00 2001 From: Aditi Khare Date: Wed, 22 Jan 2025 13:25:55 -0500 Subject: [PATCH 1/5] src code changes --- src/mongo_client.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/mongo_client.ts b/src/mongo_client.ts index 61cc94d94cd..478bc646d5c 100644 --- a/src/mongo_client.ts +++ b/src/mongo_client.ts @@ -366,6 +366,8 @@ export class MongoClient extends TypedEventEmitter implements override readonly mongoLogger: MongoLogger | undefined; /** @internal */ private connectionLock?: Promise; + /** @internal */ + private closeLock?: Promise /** * The consolidate, parsed, transformed and merged options. @@ -638,6 +640,20 @@ export class MongoClient extends TypedEventEmitter implements * @param force - Force close, emitting no events */ async close(force = false): Promise { + if (this.closeLock) { + return await this.closeLock; + } + + try { + this.closeLock = this._close(force); + await this.closeLock; + } finally { + // release + this.connectionLock = undefined; + } + } + + async _close(force = false): Promise { // There's no way to set hasBeenClosed back to false Object.defineProperty(this.s, 'hasBeenClosed', { value: true, From ba9a9e42d2c42098637cb4441894a57627216351 Mon Sep 17 00:00:00 2001 From: Aditi Khare Date: Wed, 22 Jan 2025 14:21:23 -0500 Subject: [PATCH 2/5] tests added --- src/mongo_client.ts | 6 +++--- .../node-specific/mongo_client.test.ts | 16 ++++++++++++++++ test/unit/mongo_client.test.ts | 18 ++++++++++++++++++ 3 files changed, 37 insertions(+), 3 deletions(-) diff --git a/src/mongo_client.ts b/src/mongo_client.ts index 478bc646d5c..24e1add64c1 100644 --- a/src/mongo_client.ts +++ b/src/mongo_client.ts @@ -367,7 +367,7 @@ export class MongoClient extends TypedEventEmitter implements /** @internal */ private connectionLock?: Promise; /** @internal */ - private closeLock?: Promise + private closeLock?: Promise; /** * The consolidate, parsed, transformed and merged options. @@ -642,14 +642,14 @@ export class MongoClient extends TypedEventEmitter implements async close(force = false): Promise { if (this.closeLock) { return await this.closeLock; - } + } try { this.closeLock = this._close(force); await this.closeLock; } finally { // release - this.connectionLock = undefined; + this.closeLock = undefined; } } diff --git a/test/integration/node-specific/mongo_client.test.ts b/test/integration/node-specific/mongo_client.test.ts index 059d9d82c2d..c9aa68e828e 100644 --- a/test/integration/node-specific/mongo_client.test.ts +++ b/test/integration/node-specific/mongo_client.test.ts @@ -722,6 +722,22 @@ describe('class MongoClient', function () { expect(client.s.sessionPool.sessions).to.have.lengthOf(1); }); }); + + context('concurrent calls', () => { + context('when two concurrent calls to close() occur', () => { + it('no error is thrown', async function () { + expect(() => Promise.all([client.close(), client.close()])).to.not.throw; + }); + }); + + context('when more than two concurrent calls to close() occur', () => { + it('no error is thrown', async function () { + expect(() => + Promise.all([client.close(), client.close(), client.close(), client.close()]) + ).to.not.throw; + }); + }); + }); }); context('when connecting', function () { diff --git a/test/unit/mongo_client.test.ts b/test/unit/mongo_client.test.ts index 2dcc9210278..82669c1ce9d 100644 --- a/test/unit/mongo_client.test.ts +++ b/test/unit/mongo_client.test.ts @@ -1223,4 +1223,22 @@ describe('MongoClient', function () { }); }); }); + + describe('closeLock', function () { + let client; + + beforeEach(async function () { + client = new MongoClient('mongodb://blah'); + }); + + it('when client.close is pending, client.closeLock is set', () => { + client.close(); + expect(client.closeLock).to.be.instanceOf(Promise); + }); + + it('when client.close is not pending, client.closeLock is not set', async () => { + await client.close(); + expect(client.closeLock).to.be.undefined; + }); + }); }); From e5546c127aabdea9be39c5c64ae63b46d4c05c1f Mon Sep 17 00:00:00 2001 From: Aditi Khare Date: Fri, 24 Jan 2025 11:43:51 -0500 Subject: [PATCH 3/5] half comments addressed --- src/mongo_client.ts | 3 ++- .../node-specific/mongo_client.test.ts | 20 +++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/mongo_client.ts b/src/mongo_client.ts index 508a2500704..483b64d1945 100644 --- a/src/mongo_client.ts +++ b/src/mongo_client.ts @@ -657,7 +657,8 @@ export class MongoClient extends TypedEventEmitter implements } } - async _close(force = false): Promise { + /* @internal */ + private async _close(force = false): Promise { // There's no way to set hasBeenClosed back to false Object.defineProperty(this.s, 'hasBeenClosed', { value: true, diff --git a/test/integration/node-specific/mongo_client.test.ts b/test/integration/node-specific/mongo_client.test.ts index c9aa68e828e..29f042e4f53 100644 --- a/test/integration/node-specific/mongo_client.test.ts +++ b/test/integration/node-specific/mongo_client.test.ts @@ -737,6 +737,26 @@ describe('class MongoClient', function () { ).to.not.throw; }); }); + + it('when connect rejects lock is released regardless', async function () { + await client.connect(); + expect(client.topology?.isConnected()).to.be.true; + + const closeStub = sinon.stub(client, 'close'); + closeStub.onFirstCall().rejects(new Error('cannot close')); + + // first call rejected to simulate a close failure + const error = await client.close().catch(error => error); + expect(error).to.match(/cannot close/); + + expect(client.topology?.isConnected()).to.be.true; + closeStub.restore(); + + // second call should close + await client.close(); + + expect(client.topology).to.be.undefined; + }); }); }); From 229253f3ee255df9f32796721c670fbc51a098ba Mon Sep 17 00:00:00 2001 From: Aditi Khare Date: Tue, 28 Jan 2025 10:51:21 -0500 Subject: [PATCH 4/5] requested changes 2 --- .../node-specific/mongo_client.test.ts | 41 +++++++++++++++---- 1 file changed, 34 insertions(+), 7 deletions(-) diff --git a/test/integration/node-specific/mongo_client.test.ts b/test/integration/node-specific/mongo_client.test.ts index 29f042e4f53..1286ed39d01 100644 --- a/test/integration/node-specific/mongo_client.test.ts +++ b/test/integration/node-specific/mongo_client.test.ts @@ -724,22 +724,49 @@ describe('class MongoClient', function () { }); context('concurrent calls', () => { + let topologyClosedSpy; + beforeEach(async function () { + await client.connect(); + const coll = client.db('db').collection('concurrentCalls'); + const session = client.startSession(); + await coll.findOne({}, { session: session }); + topologyClosedSpy = sinon.spy(Topology.prototype, 'close'); + }); + + afterEach(async function () { + sinon.restore(); + }); + context('when two concurrent calls to close() occur', () => { - it('no error is thrown', async function () { - expect(() => Promise.all([client.close(), client.close()])).to.not.throw; + it('does not throw', async function () { + await Promise.all([client.close(), client.close()]); + }); + + it('clean-up logic is performed only once', async function () { + await Promise.all([client.close(), client.close()]); + expect(topologyClosedSpy).to.have.been.calledOnce; }); }); context('when more than two concurrent calls to close() occur', () => { - it('no error is thrown', async function () { - expect(() => - Promise.all([client.close(), client.close(), client.close(), client.close()]) - ).to.not.throw; + it('does not throw', async function () { + await Promise.all([client.close(), client.close(), client.close(), client.close()]); + }); + + it('clean-up logic is performed only once', async function () { + await client.connect(); + await Promise.all([ + client.close(), + client.close(), + client.close(), + client.close(), + client.close() + ]); + expect(topologyClosedSpy).to.have.been.calledOnce; }); }); it('when connect rejects lock is released regardless', async function () { - await client.connect(); expect(client.topology?.isConnected()).to.be.true; const closeStub = sinon.stub(client, 'close'); From ddc7f9ae2b8ff0d30696036d44d8fc5d435089bb Mon Sep 17 00:00:00 2001 From: Aditi Khare Date: Tue, 28 Jan 2025 14:52:44 -0500 Subject: [PATCH 5/5] lint fix from rebase --- test/integration/node-specific/mongo_client.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/integration/node-specific/mongo_client.test.ts b/test/integration/node-specific/mongo_client.test.ts index c79ca9de75c..3c4b776a825 100644 --- a/test/integration/node-specific/mongo_client.test.ts +++ b/test/integration/node-specific/mongo_client.test.ts @@ -820,6 +820,8 @@ describe('class MongoClient', function () { await client.close(); expect(client.topology).to.be.undefined; + }); + }); describe('active cursors', function () { let collection: Collection<{ _id: number }>;