Updated connection cleanup process to handle expired connections and those exceeding config.maxIdle

[robert-pitt-foodhub]

Description

This PR addresses issue #3020 by improving the connection cleanup process in the MySQL pool. The changes ensure that both connections exceeding the config.maxIdle setting and expired idle connections (those exceeding the config.idleTimeout) are correctly cleaned up.

Summary of changes:

• lib/pool.js

  • Updated the connection cleanup logic to check for both config.maxIdle and expired connections based on config.idleTimeout.

• New tests:

  • Added a new test test-pool-release-idle-connection-timeout.test.cjs to verify the removal of idle connections after the timeout period.
  • Adjusted test-pool-release-idle-connection.test.cjs to reflect the new cleanup behaviour.

Key Changes:

• Improved connection cleanup mechanism in lib/pool.js.
• Added a new integration test to validate timeout-based cleanup (test-pool-release-idle-connection-timeout.test.cjs).
• Modified existing tests to cover new logic and ensure all scenarios are handled.

Checklist

• Code compiles correctly
• All tests passing
• Relevant documentation updated (if applicable)
• Verified with different pool configurations (e.g., low/high idleTimeout, maxIdle)

How to Test

1. Create a pool with the following settings:

   • connectionLimit: 3
   • maxIdle: 1
   • idleTimeout: 5000 (5 seconds)

2. Open and release 3 connections.

3. Ensure connections are removed after 5 seconds if idle.

4. Run the provided integration tests (npm test or equivalent).

Linked Issues

• Issue #3020

Screenshots (if applicable)

See linked issue

[wellwelwel]

Thanks for the PR and the detailed reports and explanations, @robert-pitt-foodhub 🤝

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.13%. Comparing base (dee0c08) to head (820569d).
Report is 6 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3022   +/-   ##
=======================================
  Coverage   88.13%   88.13%           
=======================================
  Files          71       71           
  Lines       12889    12890    +1     
  Branches     1352     1354    +2     
=======================================
+ Hits        11360    11361    +1     
  Misses       1529     1529           

Flag	Coverage Δ
compression-0	88.13% <100.00%> (+<0.01%)	⬆️
compression-1	88.13% <100.00%> (+<0.01%)	⬆️
tls-0	87.55% <100.00%> (+<0.01%)	⬆️
tls-1	87.89% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[robert-pitt-foodhub]

Thanks @wellwelwel, Happy to help get this bug resolved, I have made another commit to the PR to add a new test case that specifically validates the connections are removed after. they are expired, let me know if there is any other test cases you would like me to address or documentation that needs updating.

Thanks

[wellwelwel]

also replying #3020 (comment)

@robert-pitt-foodhub, although the changes to the code are relatively simple, this seems a complex behavior to reproduce in a test case.

I tried removing your changes, keeping your test, but it didn't fail even with the current logic.

[robert-pitt-foodhub]

Hey @wellwelwel, here's a failing test case that fail on the main branch but pass with the new code, Let me know if this helps

'use strict';

const createPool = require('../common.test.cjs').createPool;
const { assert } = require('poku');

const pool = new createPool({
  connectionLimit: 3,
  maxIdle: 2,
  idleTimeout: 1000,
});

/**
 * Create UP TO 3 connections, and allow a maximum of 2 idle connections, after
 * the 1000ms idle timeout has passed, within 1000ms of the timeout, all
 * connections should be closed if they are not in use.
 */

pool.getConnection((err1, connection1) => {
  assert.ifError(err1);
  assert.ok(connection1);
  // connection1.release();

  pool.getConnection((err2, connection2) => {
    assert.ifError(err2);
    assert.ok(connection2);
    // connection2.release();

    pool.getConnection((err3, connection3) => {
      assert.ifError(err3);
      assert.ok(connection3);
      // connection3.release();

      connection1.release();
      connection2.release();
      connection3.release();

      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}`,
        );
      }, 5000);
    });
  });
});

Main Branch

This Branch + LRU cache change

[wellwelwel]

here's a failing test case that fail on the main branch but pass with the new code

Thanks @robert-pitt-foodhub, I believe this last test is the ideal 🙋🏻‍♂️

[robert-pitt-foodhub]

Would you like to to add this test to the PR test cases @wellwelwel

[wellwelwel]

Would you like to to add this test to the PR test cases @wellwelwel

For fixes, it's usually helpful to have a test that expects a failure to ensure that a new change doesn't break again in the future 🙋🏻‍♂️

[wellwelwel]

The pool isn't being closed 🙋🏻‍♂️

[wellwelwel]

LGMT

Thanks again, @robert-pitt-foodhub 🤝
Can I merge or do you want to make some changes?

[robert-pitt-foodhub]

Im happy for to you merge this yes, there is one other outstanding change which I think needs to be addressed which I noted in the Issue, but that we can deal with separately if you like.

Thanks for your support as well, go ahead and merge when your ready