Skip to content

Commit

Permalink
fix(NODE-5106): prevent multiple mongo client connect()s from leaking…
Browse files Browse the repository at this point in the history
… topology (#3596)

Co-authored-by: Neal Beeken <neal.beeken@mongodb.com>
Co-authored-by: Bailey Pearson <bailey.pearson@mongodb.com>
  • Loading branch information
3 people authored Mar 28, 2023
1 parent 2647b61 commit eb836bb
Show file tree
Hide file tree
Showing 2 changed files with 83 additions and 0 deletions.
24 changes: 24 additions & 0 deletions src/mongo_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,8 @@ export class MongoClient extends TypedEventEmitter<MongoClientEvents> {
topology?: Topology;
/** @internal */
readonly mongoLogger: MongoLogger;
/** @internal */
private connectionLock?: Promise<this>;

/**
* The consolidate, parsed, transformed and merged options.
Expand Down Expand Up @@ -405,6 +407,28 @@ export class MongoClient extends TypedEventEmitter<MongoClientEvents> {
* @see docs.mongodb.org/manual/reference/connection-string/
*/
async connect(): Promise<this> {
if (this.connectionLock) {
return this.connectionLock;
}

try {
this.connectionLock = this._connect();
await this.connectionLock;
} finally {
// release
this.connectionLock = undefined;
}

return this;
}

/**
* Create a topology to open the connection, must be locked to avoid topology leaks in concurrency scenario.
* Locking is enforced by the connect method.
*
* @internal
*/
private async _connect(): Promise<this> {
if (this.topology && this.topology.isConnected()) {
return this;
}
Expand Down
59 changes: 59 additions & 0 deletions test/integration/node-specific/mongo_client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
async 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;
Expand Down

0 comments on commit eb836bb

Please sign in to comment.