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

[dtimofeev]

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]

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]

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]

Hello,

I have submitted a new pull request (https://github.com/sidorares/node-mysql2/pull/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]

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]

Using pooled connections, we upgraded from 3.9.4 to 3.11.3 and instantly saw huge memory leaks in many deployed production enviroments. See screenshots below from google cloud run where the line with the i and dotted lines marks the deployment of the libary 3.11.3. Its very clear that its the changes between 3.9.4 and 3.11.3

[iturn]

I downgraded to 3.9.7 and still have the memory leaks

[dstankovd]

Hi @iturn,

Since this does not appear to be directly related to the original warning mentioned in the first comment, I believe it would be best to open a new issue. Additionally, it would be helpful if you could share the pool options you are using so we can investigate the behavior of the code more thoroughly.

[dstankovd]

Hi @wellwelwel ,

I hope you're doing well!

Since it seems that @sidorares may not be active at the moment, I wanted to check if you'd be open to exploring a workaround to address this issue while maintaining the current implementation of the end method in the pool_connection.

Here are two potential solutions I've come up with:

1. Introduce a new method in the pool_connection class, perhaps called removeFromPoolAndEnd, which would, as the name suggests, remove the connection from the pool and then call _realEnd.
2. Directly call the connection's _removeFromPool and _realEnd methods from within the pool -> _removeIdleTimeoutConnections function.

I'd be more than happy to open a new merge request with either of these fixes (or any alternative approach you might suggest) to help close this issue.

Looking forward to hearing your thoughts!

Best regards, Dimitar

[wellwelwel]

I'd be more than happy to open a new merge request with either of these fixes (or any alternative approach you might suggest) to help close this issue.

Hi, @dstankovd 🙋🏻‍♂️

I'm fine with your implementation in #3180. Some time ago, I suggested a fairly simple idea to add an option to silence MySQL2 warnings that aren't actually errors, but since there were no answers, I just kept it open: https://github.com/sidorares/node-mysql2/issues/2481#issuecomment-1985891265.

Adding your changes to this possible option could solve the conflict of removing the deprecation warning, at the same time as closing issues #2481 and #2471.

What do you think about this approach?

[dstankovd]

Hi @wellwelwel ,

I have some concerns about this approach. While the option to silence warnings might have its use cases, it could also introduce risks depending on the database configuration. For example:

• Data truncated errors are sometimes returned as warnings (depends on the MySQL configuration), and if they’re silenced, they could go unnoticed, potentially leading to data loss.
• Division by zero is another one—MySQL treats it as a warning and just returns NULL instead of throwing an error. Silencing that kind of warning could result in unexpected behavior.

These are just the first examples that come to mind, but there could be other cases where important warnings get missed. So while adding this option is definitely doable and might be helpful in certain situations, I don’t think it’s the best solution to the original issue.

Cheers, Dimitar

[SavvasSunilBelakeri]

While you provide the appropriate solution for the same, is there a work around which can be implemented in consuming application ?

[dstankovd]

Hi @wellwelwel,

Just wanted to follow up on this change. Thinking about potential blockers, I'm guessing the concern might be around breaking compatibility with the original mysql library. To address that, what would you think about restoring the deprecation warning in the end method and adding a new method for the graceful close? I'm happy to code up the changes if that sounds like a good plan.

[wellwelwel]

Hi, @dstankovd 🙋🏻‍♂️

[!NOTE] I think it's worth bringing this comment back: https://github.com/sidorares/node-mysql2/issues/2481#issuecomment-2848102575.

When thinking about a new functionality for this, it could be either a new flag or a new method. Personally, I consider the above option to be more beneficial, especially in the long term, where at some point we might need to choose between one method (new) or another (old).

I'm happy to code up the changes if that sounds like a good plan.

Please, feel free to discuss the implementation idea and which approaches you find most interesting.

[dstankovd]

Hi @wellwelwel

The "Aborted connection" warning doesn't actually originate from this library, but from the MySQL server itself when a connection is dropped unexpectedly. Because of this, a library-side flag to suppress warnings wouldn't prevent the message from being logged by the server.

On principle, I also feel that adding options to suppress warnings can be risky, as it might encourage users to hide an underlying problem rather than addressing its root cause which in turn can lead to bigger problems.

Building on your idea of using a flag, we could use one to control the behavior of the end() method instead. This would maintain backward compatibility while allowing users to opt into the new, correct behavior. Here is a potential implementation I was thinking of:

• add a flag poolConnectionRealEnd to the config object that defaults to false
• in pool_connection change the end method to:

end() {
  if (this.config.poolConnectionRealEnd) {
    this._removeFromPool();
    super.end();
    return;
  }

  const err = new Error(
    'Calling conn.end() to release a pooled connection is ' +
      'deprecated. In next version calling conn.end() will be ' +
      'restored to default conn.end() behavior. Use ' +
      'conn.release() instead.'
  );
  this.emit('warn', err);
  console.warn(err.message);
  this.release();
}

• in pool, in the _removeIdleTimeoutConnection change this._freeConnections.get(0).destroy(); to check the config and call the appropriate pool_connection method:

if (this.config.poolConnectionRealEnd) {
  this._freeConnections.get(0).end();
} else {
  this._freeConnections.get(0).destroy();
}

[wellwelwel]

• add a flag poolConnectionRealEnd to the config object that defaults to false
• in pool_connection change the end method to:
• in pool, in the _removeIdleTimeoutConnection change this._freeConnections.get(0).destroy(); to check the config and call the appropriate pool_connection method:

That's a great approach. What do you think about naming it gracefulClose or gracefulEnd instead of poolConnectionRealEnd?

☁️ I wonder if someone might be puzzled by the use of “real end,” as if not using this option meant that the end wasn’t real, for example.

[dstankovd]

Hello @wellwelwel,

Following our discussion, I've implemented the changes with the new gracefulEnd config option.

Since this solution is quite different from the original, I've opened a new pull request for it here: #3776

This PR includes:

• The new gracefulEnd configuration key.
• Tests to validate the new graceful closing behavior and ensure the legacy default still works as expected.
• Updated documentation in extras.md. (Not sure if this is the right place)

Please let me know if there's anything else needed. Happy to make any changes!