From 409f4014f3462a23ef6aba96ed2e31a0569227c4 Mon Sep 17 00:00:00 2001 From: bailey Date: Thu, 26 Jun 2025 14:08:16 -0600 Subject: [PATCH 1/6] Add regression test --- .../connection_pool.test.ts | 57 ++++++++++++++++++- 1 file changed, 55 insertions(+), 2 deletions(-) diff --git a/test/integration/connection-monitoring-and-pooling/connection_pool.test.ts b/test/integration/connection-monitoring-and-pooling/connection_pool.test.ts index a765f42afe..f0517f5ba1 100644 --- a/test/integration/connection-monitoring-and-pooling/connection_pool.test.ts +++ b/test/integration/connection-monitoring-and-pooling/connection_pool.test.ts @@ -1,8 +1,14 @@ import { once } from 'node:events'; import { expect } from 'chai'; - -import { type ConnectionPoolCreatedEvent, type Db, type MongoClient } from '../../mongodb'; +import * as sinon from 'sinon'; + +import { + type ConnectionPoolCreatedEvent, + type Db, + type MongoClient, + type Server +} from '../../mongodb'; import { clearFailPoint, configureFailPoint, sleep } from '../../tools/utils'; describe('Connection Pool', function () { @@ -150,4 +156,51 @@ describe('Connection Pool', function () { }); }); }); + + describe( + 'background task cleans up connections when minPoolSize=0', + { requires: { topology: 'single' } }, + function () { + let server: Server; + let ensureMinPoolSizeSpy: sinon.SinonSpy; + + beforeEach(async function () { + client = this.configuration.newClient( + {}, + { + maxConnecting: 10, + minPoolSize: 0, + maxIdleTimeMS: 100 + } + ); + + await client.connect(); + + await Promise.all( + Array.from({ length: 10 }).map(() => { + return client.db('foo').collection('bar').insertOne({ a: 1 }); + }) + ); + + server = Array.from(client.topology.s.servers.entries())[0][1]; + expect( + server.pool.availableConnectionCount, + 'pool was not filled with connections' + ).to.equal(10); + + ensureMinPoolSizeSpy = sinon.spy(server.pool, 'ensureMinPoolSize'); + }); + + it( + 'prunes idle connections when minPoolSize=0', + { requires: { topology: 'single' } }, + async function () { + await sleep(500); + expect(server.pool.availableConnectionCount).to.equal(0); + + expect(ensureMinPoolSizeSpy).to.have.been.called; + } + ); + } + ); }); From 1ac30001cbedff64663b3ffd15a2fbc425ec0dd0 Mon Sep 17 00:00:00 2001 From: bailey Date: Thu, 26 Jun 2025 14:08:45 -0600 Subject: [PATCH 2/6] fix --- src/cmap/connection_pool.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cmap/connection_pool.ts b/src/cmap/connection_pool.ts index 00321ec234..976e280aae 100644 --- a/src/cmap/connection_pool.ts +++ b/src/cmap/connection_pool.ts @@ -691,7 +691,7 @@ export class ConnectionPool extends TypedEventEmitter { private ensureMinPoolSize() { const minPoolSize = this.options.minPoolSize; - if (this.poolState !== PoolState.ready || minPoolSize === 0) { + if (this.poolState !== PoolState.ready) { return; } From 8fed111ecf786d97d7a69ae6fbcfacbed452c846 Mon Sep 17 00:00:00 2001 From: bailey Date: Fri, 27 Jun 2025 11:30:51 -0600 Subject: [PATCH 3/6] fix leaks in unit tests --- .../connection_pool.test.ts | 2 +- test/unit/assorted/max_staleness.spec.test.js | 7 +- .../server_selection_latency_window_utils.ts | 2 + .../assorted/server_selection_spec_helper.js | 4 +- test/unit/cmap/connection_pool.test.js | 15 ++- test/unit/mongo_logger.test.ts | 97 +++++++++---------- 6 files changed, 67 insertions(+), 60 deletions(-) diff --git a/test/integration/connection-monitoring-and-pooling/connection_pool.test.ts b/test/integration/connection-monitoring-and-pooling/connection_pool.test.ts index f0517f5ba1..d073b5a112 100644 --- a/test/integration/connection-monitoring-and-pooling/connection_pool.test.ts +++ b/test/integration/connection-monitoring-and-pooling/connection_pool.test.ts @@ -186,7 +186,7 @@ describe('Connection Pool', function () { expect( server.pool.availableConnectionCount, 'pool was not filled with connections' - ).to.equal(10); + ).to.be.greaterThan(0); ensureMinPoolSizeSpy = sinon.spy(server.pool, 'ensureMinPoolSize'); }); diff --git a/test/unit/assorted/max_staleness.spec.test.js b/test/unit/assorted/max_staleness.spec.test.js index 2cdfbdc368..f0355cb1cc 100644 --- a/test/unit/assorted/max_staleness.spec.test.js +++ b/test/unit/assorted/max_staleness.spec.test.js @@ -45,13 +45,12 @@ describe('Max Staleness (spec)', function () { }); const specTests = collectStalenessTests(maxStalenessDir); - Object.keys(specTests).forEach(specTestName => { + for (const [specTestName, test] of Object.entries(specTests)) { describe(specTestName, () => { - specTests[specTestName].forEach(testData => { + for (const testData of test) it(testData.description, async function () { return executeServerSelectionTest(testData); }); - }); }); - }); + } }); diff --git a/test/unit/assorted/server_selection_latency_window_utils.ts b/test/unit/assorted/server_selection_latency_window_utils.ts index 8b6fd7e66c..9aa8dd2481 100644 --- a/test/unit/assorted/server_selection_latency_window_utils.ts +++ b/test/unit/assorted/server_selection_latency_window_utils.ts @@ -145,4 +145,6 @@ export async function runServerSelectionLatencyWindowTest(test: ServerSelectionL const observedFrequencies = calculateObservedFrequencies(selectedServers); compareResultsToExpected(test.outcome, observedFrequencies); + + await topology.close(); } diff --git a/test/unit/assorted/server_selection_spec_helper.js b/test/unit/assorted/server_selection_spec_helper.js index 8fb4bee676..20bcea87a2 100644 --- a/test/unit/assorted/server_selection_spec_helper.js +++ b/test/unit/assorted/server_selection_spec_helper.js @@ -123,7 +123,7 @@ async function executeServerSelectionTest(testDefinition) { const readPreference = readPreferenceFromDefinition(testDefinition.read_preference); selector = ServerSelectors.readPreferenceServerSelector(readPreference); } catch (e) { - if (testDefinition.error) return; + if (testDefinition.error) return topology.close(); throw e; } } else { @@ -179,6 +179,8 @@ async function executeServerSelectionTest(testDefinition) { // this is another expected error case if (expectedServers.length === 0 && err instanceof MongoServerSelectionError) return; throw err; + } finally { + topology.close(); } } diff --git a/test/unit/cmap/connection_pool.test.js b/test/unit/cmap/connection_pool.test.js index 1604cd82d8..61fa73fc0e 100644 --- a/test/unit/cmap/connection_pool.test.js +++ b/test/unit/cmap/connection_pool.test.js @@ -15,6 +15,7 @@ const { TimeoutContext } = require('../../mongodb'); describe('Connection Pool', function () { let timeoutContext; let mockMongod; + let pool; const stubServer = { topology: { client: { @@ -50,6 +51,8 @@ describe('Connection Pool', function () { timeoutContext = TimeoutContext.create({ waitQueueTimeoutMS: 0, serverSelectionTimeoutMS: 0 }); }); + afterEach(() => pool?.close()); + it('should destroy connections which have been closed', async function () { mockMongod.setMessageHandler(request => { const doc = request.document; @@ -61,7 +64,7 @@ describe('Connection Pool', function () { } }); - const pool = new ConnectionPool(stubServer, { + pool = new ConnectionPool(stubServer, { maxPoolSize: 1, hostAddress: mockMongod.hostAddress() }); @@ -81,6 +84,8 @@ describe('Connection Pool', function () { expect(events).to.have.length(1); const closeEvent = events[0]; expect(closeEvent).have.property('reason').equal('error'); + + conn.destroy(); }); it('should propagate socket timeouts to connections', async function () { @@ -93,7 +98,7 @@ describe('Connection Pool', function () { } }); - const pool = new ConnectionPool(stubServer, { + pool = new ConnectionPool(stubServer, { maxPoolSize: 1, socketTimeoutMS: 200, hostAddress: mockMongod.hostAddress() @@ -117,7 +122,7 @@ describe('Connection Pool', function () { } }); - const pool = new ConnectionPool(stubServer, { + pool = new ConnectionPool(stubServer, { maxPoolSize: 1, waitQueueTimeoutMS: 200, hostAddress: mockMongod.hostAddress() @@ -157,7 +162,7 @@ describe('Connection Pool', function () { }); it('should respect the minPoolSizeCheckFrequencyMS option', function () { - const pool = new ConnectionPool(stubServer, { + pool = new ConnectionPool(stubServer, { minPoolSize: 2, minPoolSizeCheckFrequencyMS: 42, hostAddress: mockMongod.hostAddress() @@ -193,7 +198,7 @@ describe('Connection Pool', function () { }); it('should default minPoolSizeCheckFrequencyMS to 100ms', function () { - const pool = new ConnectionPool(stubServer, { + pool = new ConnectionPool(stubServer, { minPoolSize: 2, hostAddress: mockMongod.hostAddress() }); diff --git a/test/unit/mongo_logger.test.ts b/test/unit/mongo_logger.test.ts index da47bd8399..35bef3aa6d 100644 --- a/test/unit/mongo_logger.test.ts +++ b/test/unit/mongo_logger.test.ts @@ -356,31 +356,31 @@ describe('class MongoLogger', function () { context: string; outcome: string; }> = [ - { - input: undefined, - expected: 1000, - context: 'when unset', - outcome: 'defaults to 1000' - }, - { - input: '33', - context: 'when set to parsable uint', - outcome: 'sets `maxDocumentLength` to the parsed value', - expected: 33 - }, - { - input: '', - context: 'when set to an empty string', - outcome: 'defaults to 1000', - expected: 1000 - }, - { - input: 'asdf', - context: 'when set to a non-integer string', - outcome: 'defaults to 1000', - expected: 1000 - } - ]; + { + input: undefined, + expected: 1000, + context: 'when unset', + outcome: 'defaults to 1000' + }, + { + input: '33', + context: 'when set to parsable uint', + outcome: 'sets `maxDocumentLength` to the parsed value', + expected: 33 + }, + { + input: '', + context: 'when set to an empty string', + outcome: 'defaults to 1000', + expected: 1000 + }, + { + input: 'asdf', + context: 'when set to a non-integer string', + outcome: 'defaults to 1000', + expected: 1000 + } + ]; for (const { input, outcome, expected, context: _context } of tests) { context(_context, () => { @@ -613,22 +613,21 @@ describe('class MongoLogger', function () { context('when mongodbLogPath is set to valid client option', function () { for (const validEnvironmentOption of validEnvironmentOptions) { for (const validValue of validClientOptions) { - it(`{environment: "${validEnvironmentOption}", client: ${ - typeof validValue === 'object' - ? 'new ' + validValue.constructor.name + '(...)' - : '"' + validValue.toString() + '"' - }} uses the value from the client options`, function () { - const options = MongoLogger.resolveOptions( - { - MONGODB_LOG_PATH: validEnvironmentOption, - MONGODB_LOG_COMMAND: 'error' - }, - { mongodbLogPath: validValue as any } - ); - const correctDestination = validOptions.get(validValue); - options.logDestination.write({ t: new Date(), c: 'command', s: 'error' }); - expect(correctDestination?.write).to.have.been.calledOnce; - }); + it(`{environment: "${validEnvironmentOption}", client: ${typeof validValue === 'object' + ? 'new ' + validValue.constructor.name + '(...)' + : '"' + validValue.toString() + '"' + }} uses the value from the client options`, function () { + const options = MongoLogger.resolveOptions( + { + MONGODB_LOG_PATH: validEnvironmentOption, + MONGODB_LOG_COMMAND: 'error' + }, + { mongodbLogPath: validValue as any } + ); + const correctDestination = validOptions.get(validValue); + options.logDestination.write({ t: new Date(), c: 'command', s: 'error' }); + expect(correctDestination?.write).to.have.been.calledOnce; + }); } } }); @@ -1121,10 +1120,10 @@ describe('class MongoLogger', function () { const event = reason === 'connectionError' ? { - ...connectionCheckOutFailed, - reason, - error: new Error('this is an error') - } + ...connectionCheckOutFailed, + reason, + error: new Error('this is an error') + } : { ...connectionCheckOutFailed, reason }; logger[severityLevel]('connection', event); expect(stream.buffer).to.have.lengthOf(1); @@ -1562,9 +1561,9 @@ describe('class MongoLogger', function () { component === value ? (severities[component] = severityLevel) : (severities[value] = - severityLevels[index + 1] === 'off' - ? severityLevel - : severityLevels[index + 1]); + severityLevels[index + 1] === 'off' + ? severityLevel + : severityLevels[index + 1]); return severities; }, {}); logger = new MongoLogger({ @@ -1613,7 +1612,7 @@ describe('class MongoLogger', function () { }); }); -describe('stringifyWithMaxLen', function () { +describe.skip('stringifyWithMaxLen', function () { describe('when stringifying a string field', function () { it('does not prematurely redact the next key', function () { const doc = { From ab28f3dc0f27bc47a9952c4069319bd8c85a9df0 Mon Sep 17 00:00:00 2001 From: bailey Date: Fri, 27 Jun 2025 11:31:34 -0600 Subject: [PATCH 4/6] lint --- test/unit/mongo_logger.test.ts | 95 +++++++++++++++++----------------- 1 file changed, 48 insertions(+), 47 deletions(-) diff --git a/test/unit/mongo_logger.test.ts b/test/unit/mongo_logger.test.ts index 35bef3aa6d..46d9165a9e 100644 --- a/test/unit/mongo_logger.test.ts +++ b/test/unit/mongo_logger.test.ts @@ -356,31 +356,31 @@ describe('class MongoLogger', function () { context: string; outcome: string; }> = [ - { - input: undefined, - expected: 1000, - context: 'when unset', - outcome: 'defaults to 1000' - }, - { - input: '33', - context: 'when set to parsable uint', - outcome: 'sets `maxDocumentLength` to the parsed value', - expected: 33 - }, - { - input: '', - context: 'when set to an empty string', - outcome: 'defaults to 1000', - expected: 1000 - }, - { - input: 'asdf', - context: 'when set to a non-integer string', - outcome: 'defaults to 1000', - expected: 1000 - } - ]; + { + input: undefined, + expected: 1000, + context: 'when unset', + outcome: 'defaults to 1000' + }, + { + input: '33', + context: 'when set to parsable uint', + outcome: 'sets `maxDocumentLength` to the parsed value', + expected: 33 + }, + { + input: '', + context: 'when set to an empty string', + outcome: 'defaults to 1000', + expected: 1000 + }, + { + input: 'asdf', + context: 'when set to a non-integer string', + outcome: 'defaults to 1000', + expected: 1000 + } + ]; for (const { input, outcome, expected, context: _context } of tests) { context(_context, () => { @@ -613,21 +613,22 @@ describe('class MongoLogger', function () { context('when mongodbLogPath is set to valid client option', function () { for (const validEnvironmentOption of validEnvironmentOptions) { for (const validValue of validClientOptions) { - it(`{environment: "${validEnvironmentOption}", client: ${typeof validValue === 'object' - ? 'new ' + validValue.constructor.name + '(...)' - : '"' + validValue.toString() + '"' - }} uses the value from the client options`, function () { - const options = MongoLogger.resolveOptions( - { - MONGODB_LOG_PATH: validEnvironmentOption, - MONGODB_LOG_COMMAND: 'error' - }, - { mongodbLogPath: validValue as any } - ); - const correctDestination = validOptions.get(validValue); - options.logDestination.write({ t: new Date(), c: 'command', s: 'error' }); - expect(correctDestination?.write).to.have.been.calledOnce; - }); + it(`{environment: "${validEnvironmentOption}", client: ${ + typeof validValue === 'object' + ? 'new ' + validValue.constructor.name + '(...)' + : '"' + validValue.toString() + '"' + }} uses the value from the client options`, function () { + const options = MongoLogger.resolveOptions( + { + MONGODB_LOG_PATH: validEnvironmentOption, + MONGODB_LOG_COMMAND: 'error' + }, + { mongodbLogPath: validValue as any } + ); + const correctDestination = validOptions.get(validValue); + options.logDestination.write({ t: new Date(), c: 'command', s: 'error' }); + expect(correctDestination?.write).to.have.been.calledOnce; + }); } } }); @@ -1120,10 +1121,10 @@ describe('class MongoLogger', function () { const event = reason === 'connectionError' ? { - ...connectionCheckOutFailed, - reason, - error: new Error('this is an error') - } + ...connectionCheckOutFailed, + reason, + error: new Error('this is an error') + } : { ...connectionCheckOutFailed, reason }; logger[severityLevel]('connection', event); expect(stream.buffer).to.have.lengthOf(1); @@ -1561,9 +1562,9 @@ describe('class MongoLogger', function () { component === value ? (severities[component] = severityLevel) : (severities[value] = - severityLevels[index + 1] === 'off' - ? severityLevel - : severityLevels[index + 1]); + severityLevels[index + 1] === 'off' + ? severityLevel + : severityLevels[index + 1]); return severities; }, {}); logger = new MongoLogger({ From e054d28922cb0694b01ccb72677b98154a2cceac Mon Sep 17 00:00:00 2001 From: bailey Date: Fri, 27 Jun 2025 11:54:45 -0600 Subject: [PATCH 5/6] remove accidental test skip --- test/unit/mongo_logger.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/mongo_logger.test.ts b/test/unit/mongo_logger.test.ts index 46d9165a9e..da47bd8399 100644 --- a/test/unit/mongo_logger.test.ts +++ b/test/unit/mongo_logger.test.ts @@ -1613,7 +1613,7 @@ describe('class MongoLogger', function () { }); }); -describe.skip('stringifyWithMaxLen', function () { +describe('stringifyWithMaxLen', function () { describe('when stringifying a string field', function () { it('does not prematurely redact the next key', function () { const doc = { From b2c96750fe77f60cb9ec92f7940e45e48e0c3b19 Mon Sep 17 00:00:00 2001 From: bailey Date: Fri, 27 Jun 2025 13:45:05 -0600 Subject: [PATCH 6/6] fix flake --- test/integration/crud/insert.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/crud/insert.test.js b/test/integration/crud/insert.test.js index c7c212d91d..8fa676574a 100644 --- a/test/integration/crud/insert.test.js +++ b/test/integration/crud/insert.test.js @@ -1825,7 +1825,7 @@ describe('crud - insert', function () { .toArray(); const doc = docs.pop(); expect(doc.a._bsontype).to.equal('Long'); - client.close(); + await client.close(); } });