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 <[email protected]>

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();
         }

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);
+    });
+  });
+});

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);
+    });
+  });
+});

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);
     });
   });
 });