Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(NODE-5225): concurrent MongoClient.close() calls each attempt to close the client #4376

Merged
merged 10 commits into from
Jan 29, 2025
17 changes: 17 additions & 0 deletions src/mongo_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,8 @@ export class MongoClient extends TypedEventEmitter<MongoClientEvents> implements
override readonly mongoLogger: MongoLogger | undefined;
/** @internal */
private connectionLock?: Promise<this>;
/** @internal */
private closeLock?: Promise<void>;

/**
* The consolidate, parsed, transformed and merged options.
Expand Down Expand Up @@ -650,6 +652,21 @@ export class MongoClient extends TypedEventEmitter<MongoClientEvents> implements
* @param force - Force close, emitting no events
*/
async close(force = false): Promise<void> {
if (this.closeLock) {
return await this.closeLock;
}

try {
this.closeLock = this._close(force);
await this.closeLock;
} finally {
// release
this.closeLock = undefined;
}
}

/* @internal */
private async _close(force = false): Promise<void> {
// There's no way to set hasBeenClosed back to false
Object.defineProperty(this.s, 'hasBeenClosed', {
value: true,
Expand Down
63 changes: 63 additions & 0 deletions test/integration/node-specific/mongo_client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -760,6 +760,69 @@ describe('class MongoClient', function () {
});
});

context('concurrent calls', () => {
baileympearson marked this conversation as resolved.
Show resolved Hide resolved
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', () => {
baileympearson marked this conversation as resolved.
Show resolved Hide resolved
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('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 () {
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;
});
});

describe('active cursors', function () {
let collection: Collection<{ _id: number }>;
const kills = [];
Expand Down
18 changes: 18 additions & 0 deletions test/unit/mongo_client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
});
});
});