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 connections are not gracefully closed once idle connection cleanup occurs. #3148

Open
dtimofeev opened this issue Oct 22, 2024 · 7 comments · May be fixed by #3180
Open

Pool connections are not gracefully closed once idle connection cleanup occurs. #3148

dtimofeev opened this issue Oct 22, 2024 · 7 comments · May be fixed by #3180

Comments

@dtimofeev
Copy link

dtimofeev commented Oct 22, 2024

Hi,
When Pool._removeIdleTimeoutConnections() is executed and an idle connection is selected for removal (regardless of the conditions) the following code is executed:
https://github.com/sidorares/node-mysql2/blob/master/lib/pool.js#L208
This in turn removes the connection from the pool and directly (non-gracefully) closes it by calling PoolConnection.destroy() instead of PoolConnection.end().
https://github.com/sidorares/node-mysql2/blob/master/lib/pool_connection.js#L50
As a result the connection is closed without notifying the server in any way.

Because of the above the SQL(MariaDB) server is flooded with warnings like:
Aborted connection 123 to db: 'dbname' user: 'dbuser' host: '127.0.0.1' (Got an error reading communication packets)

A possible solution might be to change the current PoolConnection.end() implementation with:

end() {
    this._removeFromPool();
    super.end();
}

but I'm not sure if this solves the issue correctly.

Looking forward to your thoughts on this.

@dtimofeev
Copy link
Author

Reproduce script:

const mysql = require("mysql2/promise");

(function () {
  const pool = mysql.createPool({
    host: "localhost",
    user: "root",
    password: "random",
    database: "tests",
    idleTimeout: 1000,
    connectionLimit: 5,
    // this has to be lower than connectionLimit in order for cleanup process to start at all
    maxIdle: 4,
  });

  setInterval(() => {
    pool.query("SELECT 1");
  }, 1000);
})();

@dtimofeev
Copy link
Author

The issue also occurs in latest MySQL:

docker run  --env=MYSQL_ROOT_PASSWORD=random -p 3306:3306 -d mysql:latest --log_error_verbosity=3
docker logs -f <ID from the above command>
[Note] [MY-010914] [Server] Aborted connection 8 to db: 'unconnected' user: 'root' host: '172.17.0.1' (Got an error reading communication packets).

dstankovd pushed a commit to dstankovd/node-mysql2 that referenced this issue Nov 4, 2024
@dstankovd
Copy link

Hello,

I have submitted a new pull request (#3180) that addresses this issue with minimal changes.

In my view, an improved long-term solution would involve removing the deprecation warning, as it has been in place for some time, and implementing proper end functionality in pool_connection without relying on _realEnd. If you would like me to work on this enhancement, I would be happy to update the pull request accordingly.

Best regards,
Dimitar

@dstankovd dstankovd linked a pull request Nov 4, 2024 that will close this issue
@dstankovd
Copy link

Upon further consideration, it seems more appropriate to implement proper end functionality directly within pool_connection, as the current fix could be susceptible to unexpected edge cases. For example, when using the _realEnd function, the connection is removed from the pool upon the "end" event, which can create a brief window during which a closed connection might still be allocated.

@iturn

This comment was marked as off-topic.

@iturn

This comment was marked as off-topic.

dstankovd added a commit to dstankovd/node-mysql2 that referenced this issue Nov 5, 2024
@dstankovd

This comment was marked as off-topic.

dstankovd pushed a commit to dstankovd/node-mysql2 that referenced this issue Nov 14, 2024
dstankovd added a commit to dstankovd/node-mysql2 that referenced this issue Nov 14, 2024
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

Successfully merging a pull request may close this issue.

4 participants