From b091cf49d4165e991cb7c51dd6074be1c996a98e Mon Sep 17 00:00:00 2001 From: robert-pitt-foodhub <70318887+robert-pitt-foodhub@users.noreply.github.com> Date: Tue, 10 Sep 2024 10:49:37 +0100 Subject: [PATCH] fix: update connection cleanup process to handle expired connections and exceeding `config.maxIdle` (#3022) * Updated connection cleanup process to ensure we cleanup connections that have expired as well as the connections that exceed the config.maxIdle setting * Added test case to ensure connections that have expired are removed, even when below maxIdle * Added test case to cover future regressions of this change * Ensure connection pool is closed to ensure the test completes without open handles --------- Co-authored-by: robertpitt --- lib/pool.js | 7 +- ...release-idle-connection-replicate.test.cjs | 71 +++++++++++++++++++ ...l-release-idle-connection-timeout.test.cjs | 46 ++++++++++++ ...test-pool-release-idle-connection.test.cjs | 4 +- 4 files changed, 124 insertions(+), 4 deletions(-) create mode 100644 test/integration/test-pool-release-idle-connection-replicate.test.cjs create mode 100644 test/integration/test-pool-release-idle-connection-timeout.test.cjs diff --git a/lib/pool.js b/lib/pool.js index 1b4993ff1a..dc638d743e 100644 --- a/lib/pool.js +++ b/lib/pool.js @@ -200,9 +200,10 @@ class Pool extends EventEmitter { this._removeIdleTimeoutConnectionsTimer = setTimeout(() => { try { while ( - this._freeConnections.length > this.config.maxIdle && - Date.now() - this._freeConnections.get(0).lastActiveTime > - this.config.idleTimeout + this._freeConnections.length > this.config.maxIdle || + (this._freeConnections.length > 0 && + Date.now() - this._freeConnections.get(0).lastActiveTime > + this.config.idleTimeout) ) { this._freeConnections.get(0).destroy(); } diff --git a/test/integration/test-pool-release-idle-connection-replicate.test.cjs b/test/integration/test-pool-release-idle-connection-replicate.test.cjs new file mode 100644 index 0000000000..3e0b355bd7 --- /dev/null +++ b/test/integration/test-pool-release-idle-connection-replicate.test.cjs @@ -0,0 +1,71 @@ +'use strict'; +const createPool = require('../common.test.cjs').createPool; +const { assert } = require('poku'); + +/** + * This test case tests the scenario where released connections are not + * being destroyed after the idle timeout has passed, to do this we setup + * a pool with a connection limit of 3, and a max idle of 2, and an idle + * timeout of 1 second. We then create 3 connections, and release them + * after 1 second, we then check that the pool has 0 connections, and 0 + * free connections. + * + * @see https://github.com/sidorares/node-mysql2/issues/3020 + */ + +/** + * This test case + */ +const pool = new createPool({ + connectionLimit: 3, + maxIdle: 2, + idleTimeout: 1000, +}); + +/** + * Create the first connection and ensure it's in the pool as expected + */ +pool.getConnection((err1, connection1) => { + assert.ifError(err1); + assert.ok(connection1); + + /** + * Create the second connection and ensure it's in the pool as expected + */ + pool.getConnection((err2, connection2) => { + assert.ifError(err2); + assert.ok(connection2); + + /** + * Create the third connection and ensure it's in the pool as expected + */ + pool.getConnection((err3, connection3) => { + assert.ifError(err3); + assert.ok(connection3); + + /** + * Release all the connections + */ + connection1.release(); + connection2.release(); + connection3.release(); + + /** + * After the idle timeout has passed, check that all items in the in the pool + * that have been released are destroyed as expected. + */ + setTimeout(() => { + assert( + pool._allConnections.length === 0, + `Expected all connections to be closed, but found ${pool._allConnections.length}`, + ); + assert( + pool._freeConnections.length === 0, + `Expected all free connections to be closed, but found ${pool._freeConnections.length}`, + ); + + pool.end(); + }, 5000); + }); + }); +}); diff --git a/test/integration/test-pool-release-idle-connection-timeout.test.cjs b/test/integration/test-pool-release-idle-connection-timeout.test.cjs new file mode 100644 index 0000000000..d087be6303 --- /dev/null +++ b/test/integration/test-pool-release-idle-connection-timeout.test.cjs @@ -0,0 +1,46 @@ +'use strict'; + +const createPool = require('../common.test.cjs').createPool; +const { assert } = require('poku'); + +const pool = new createPool({ + connectionLimit: 3, // 5 connections + maxIdle: 1, // 1 idle connection + idleTimeout: 1000, // remove idle connections after 1 second +}); + +pool.getConnection((err1, connection1) => { + assert.ifError(err1); + assert.ok(connection1); + pool.getConnection((err2, connection2) => { + assert.ifError(err2); + assert.ok(connection2); + assert.notStrictEqual(connection1, connection2); + pool.getConnection((err3, connection3) => { + assert.ifError(err3); + assert.ok(connection3); + assert.notStrictEqual(connection1, connection3); + assert.notStrictEqual(connection2, connection3); + connection1.release(); + connection2.release(); + connection3.release(); + assert(pool._allConnections.length === 3); + assert(pool._freeConnections.length === 3); + // after two seconds, the above 3 connection should have been destroyed + setTimeout(() => { + assert(pool._allConnections.length === 0); + assert(pool._freeConnections.length === 0); + // Creating a new connection should create a fresh one + pool.getConnection((err4, connection4) => { + assert.ifError(err4); + assert.ok(connection4); + assert(pool._allConnections.length === 1); + assert(pool._freeConnections.length === 0); + connection4.release(); + connection4.destroy(); + pool.end(); + }); + }, 2000); + }); + }); +}); diff --git a/test/integration/test-pool-release-idle-connection.test.cjs b/test/integration/test-pool-release-idle-connection.test.cjs index 7155c28f1a..48c9f1dbdb 100644 --- a/test/integration/test-pool-release-idle-connection.test.cjs +++ b/test/integration/test-pool-release-idle-connection.test.cjs @@ -39,7 +39,9 @@ pool.getConnection((err1, connection1) => { connection4.destroy(); pool.end(); }); - }, 7000); + // Setting the time to a lower value than idleTimeout will ensure that the connection is not considered idle + // during our assertions + }, 4000); }); }); });