Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pool not releasing connections if maxIdle is not set #2493

Open
danj1980 opened this issue Mar 12, 2024 · 14 comments
Open

Pool not releasing connections if maxIdle is not set #2493

danj1980 opened this issue Mar 12, 2024 · 14 comments

Comments

@danj1980
Copy link

danj1980 commented Mar 12, 2024

All of my queries include dbConnection.release(), however the connections do not get released and once connectionLimit is reached, the execution pauses waiting for an available connection.

I've re-worked my code so that every pool.getConnection() has a dbConnection.release() right next to it, to make absolutely certain that I'm not accidentally leaving any connections unreleased.

However, this did not help, and it seems that there is a bug in the releasing of pool connections.

I store my pool in a static variable with a class called Global.

class globals
{
	static dbPool = null;
}

Runs once on startup

		globals.dbPool = await mysql.createPool({
				host: databaseServer,
				port: databasePort,
				user: databaseUsername,
				password: databasePassword,
				database: databaseName,
				connectionLimit: 10
			});

Example usage

        const dbConnection = await globals.dbPool.getConnection();

        let mysqlResponse = null;

        await dbConnection.query(`SELECT * FROM guild;`).then(result => {
            mysqlResponse = result[0];

        }).catch(err => console.log(err));

        dbConnection.release();
Result after some queries have been executed 
![image](https://github.com/sidorares/node-mysql2/assets/12071228/17c7aca3-567e-4202-8ebe-b202fcba6c5d)
@danj1980
Copy link
Author

image

@wellwelwel
Copy link
Collaborator

wellwelwel commented Mar 12, 2024

Hi @danj1980, 🙋🏻‍♂️

It's not directly related, but can you check if this PR affects this behavior in some way?

Also, this workaround:


Possibly related

@danj1980
Copy link
Author

danj1980 commented Mar 12, 2024

It seems I've found the issue.

If maxIdle is not set on the pool, _removeIdleTimeoutConnections is never executed, and therefore there is never any cleanup of released connections.
I think it's this line that's the problem.

if (this.config.maxIdle < this.config.connectionLimit) {

If maxIdle is set, the released connections get cleaned up.

@danj1980
Copy link
Author

Side note, if I manually kill a connection on MySQL server, execution continues in the pool until the connections fill back up again.

@danj1980
Copy link
Author

Hi @danj1980, 🙋🏻‍♂️

It's not directly related, but can you check if this PR affects this behavior in some way?

I dont think that's the same issue, as the CPU usage is 0% when the connections run out.

Also, this workaround:

This workaround looks similar to my test where I'm getting a connection, querying it, and then immediately releasing it.

@danj1980 danj1980 changed the title Pool not releasing connections Pool not releasing connections if maxIdle is not set Mar 12, 2024
@wellwelwel
Copy link
Collaborator

wellwelwel commented Mar 12, 2024

This workaround looks similar to my test where I'm getting a connection, querying it, and then immediately releasing it.

@danj1980, can you provide a minimal repro for this issue?


According to the documentation, that should be the default behavior for maxIdle:

{
  connectionLimit: 10,
  maxIdle: 10, // max idle connections, the default value is the same as `connectionLimit`
}

@danj1980
Copy link
Author

danj1980 commented Mar 12, 2024

I'm sorry. Let me clarify my fix....

In order to get it to cleanup old released connections, I had to set maxIdle < connectionLimit.
Without this, my connections maxed out for no reason, and I ended up with 10 idle connections, and no further queries would execute.

If maxIdle is default (eg. maxIdle => connectionLimit), no cleanup of old released connections occurs as shown below.

if (this.config.maxIdle < this.config.connectionLimit) {

Unfortunately, I cannot create a demo repo. I've never posted code to github and wouldn't know where to start.

@danj1980
Copy link
Author

There is also a factor that I've not quite figured out. Sometimes connections are reused. Sometimes they aren't.
It's when they aren't, and new connections are created that the problem starts occurring.

I would guess that the connection's idleTimeout is reached, that connection is marked as "old" and pending releasing, and therefore a new connection is created, but the old ones aren't cleaned up.

@wellwelwel
Copy link
Collaborator

So, currently there are these tests for idle connections:

const pool = new createPool({
connectionLimit: 5, // 5 connections
maxIdle: 1, // 1 idle connection
idleTimeout: 5000, // 5 seconds
});
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);
setTimeout(() => {
assert(pool._allConnections.length === 1);
assert(pool._freeConnections.length === 1);
pool.getConnection((err4, connection4) => {
assert.ifError(err4);
assert.ok(connection4);
assert.strictEqual(connection3, connection4);
assert(pool._allConnections.length === 1);
assert(pool._freeConnections.length === 0);
connection4.release();
connection4.destroy();
pool.end();
});
}, 7000);
});
});
});

But unfortunately, I can't reproduce this behavior to implement a specific test case for it 😕

@danj1980
Copy link
Author

danj1980 commented Mar 12, 2024

In that test,
Try removing maxIdle so it's default.
Then run the test, pause in the middle for at least 5 seconds to allow idleTimeout to be reached, and then see if it releases the connections that have reached the idleTimeout.
Also see if you can create a 6th connection after the idleTimeout has been reached on the first 5 connections. It should allow a 6th connection if the first 5 exceed their idleTimeout.
I'd try it myself but I dont know how to run those tests.

@wellwelwel
Copy link
Collaborator

Thanks, @danj1980 🚀

I'll show how I perform the tests locally using Docker, but you can also see the Contributing.md.


Choose a directory

For this example, I'll use the Desktop

mkdir -p Desktop/sidorares
cd Desktop/sidorares

Clone this repository

git clone https://github.com/sidorares/node-mysql2.git

Create a Docker Compose file

Desktop/sidorares/docker-compose.yml:

version: '3.9'
services:
  db:
    image: mysql:latest
    restart: always
    environment:
      MYSQL_DATABASE: 'test'
      MYSQL_ALLOW_EMPTY_PASSWORD: 'yes'
    ports:
      - '127.0.0.1:3306:3306'
    healthcheck:
      test: ['CMD', 'mysqladmin', 'ping', '-h', 'localhost', '-u', 'root']
      interval: 10s
      timeout: 5s
      retries: 5
      
  node-mysql2:
    image: node:lts-alpine
    working_dir: /usr/app
    volumes:
      - ./node-mysql2:/usr/app
    command: npm run test
    environment:
      MYSQL_HOST: db
      FILTER: test-pool-release-idle-connection
    depends_on:
      db:
        condition: service_healthy

Then, edit the Desktop/sidorares/node-mysql2/test/integration/test-pool-release-idle-connection.test.cjs to include your test case.

Finally, test it:

docker compose up

Then, press Ctrl + C to exit.

As it should be a case not covered, it's expected to exit with code 1 (error).

@wellwelwel
Copy link
Collaborator

wellwelwel commented Mar 12, 2024

Usually, this way (above) is when we want to submit a PR (which are always welcome).

But if you prefer to show just a minimal code to reproduce this error, we can try that too 🙋🏻‍♂️

@Jasermon
Copy link

This happens to me too. I'm having exactly the same problem. Here is my codes;

db.js

import getConfig from "next/config";
import mysql from "mysql2/promise";

const { serverRuntimeConfig } = getConfig();
const { host, port, user, password, database } = serverRuntimeConfig.dbConfig;

const pool = mysql.createPool({
  host,
  port,
  user,
  password,
  database,
  timezone: "+00:00",
  waitForConnections: true,
  enableKeepAlive: true,
  connectionLimit: 10,
  keepAliveInitialDelay: 0,
  queueLimit: 0,
});

export { pool };

users-repo.js

export const usersRepo = {
  ...
  getAllUsers,
  SomeFunc,
};

async function getAllUsers(data) {
  const db = await pool.getConnection();
  try {
    const tableName = data.companyCodeShort + "_users";
    const [allusers] = await db.query("SELECT * FROM ?? WHERE rank != '-1'", [
      tableName,
    ]);

    allusers.forEach((a) => {
      delete a.hash;
      delete a.id;
    });

    return allusers;
  } finally {
    db.release();
  }
}

async function SomeFunc(data) {
  const db = await pool.getConnection();
  try {
    const [users] = await db.query("SHOW STATUS LIKE '%Threads_%'");
    return users;
  } finally {
    db.release();
  }
}

SomeFunc output before getAllUsers function

[
  { Variable_name: 'Threads_cached', Value: '0' },
  { Variable_name: 'Threads_connected', Value: '4' },
  { Variable_name: 'Threads_created', Value: '39' },
  { Variable_name: 'Threads_running', Value: '1' }
]

SomeFunc output after getAllUsers function

[
  { Variable_name: 'Threads_cached', Value: '0' },
  { Variable_name: 'Threads_connected', Value: '5' },
  { Variable_name: 'Threads_created', Value: '39' },
  { Variable_name: 'Threads_running', Value: '1' }
]

SomeFunc output after some time has passed

[
  { Variable_name: 'Threads_cached', Value: '0' },
  { Variable_name: 'Threads_connected', Value: '5' },
  { Variable_name: 'Threads_created', Value: '39' },
  { Variable_name: 'Threads_running', Value: '1' }
]

@Jasermon
Copy link

Any solution?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants