Repeated connection.release() leads to connection pool inconsistency or application crash (infinite loop)

[dg-korolev]

When connection.release() is called multiple times, there is no logic implemented to properly handle the repeated release of the same connection.

Below is the source code for the releaseConnection method:

  releaseConnection(connection) {
    let cb;
    if (!connection._pool) {
      // The connection has been removed from the pool and is no longer good.
      if (this._connectionQueue.length) {
        cb = this._connectionQueue.shift();
        process.nextTick(this.getConnection.bind(this, cb));
      }
    } else if (this._connectionQueue.length) {
      cb = this._connectionQueue.shift();
      process.nextTick(cb.bind(null, null, connection));
    } else {
      this._freeConnections.push(connection);
      this.emit('release', connection);
    }
  }

Each repeated call to releaseConnection either results in a duplicate reference to the same connection being added to the _freeConnections queue, or — if there are pending handlers in the _connectionQueue — in assigning the same connection to multiple different request handlers (cb).

This leads to the following issues:

1. Assigning the same connection to multiple handlers
   Re-adding the same connection object to _freeConnections may cause the getConnection method to return the same connection to two different handlers.
   This breaks the isolation of the handler and can lead to, for example, a violation of transaction isolation.

2. Infinite loop when calling _removeIdleTimeoutConnections (CPU utilization 100% !!!)
   If there are multiple references to the same connection in _freeConnections, the following happens:

• On the first iteration, connection._pool is set to null (connection._pool = null), and the connection is removed from the _freeConnections queue.
• On the next iteration, another reference to the same connection is processed again.
  However, in the connection.destroy() → connection._removeFromPool() flow, removal from _freeConnections does not happen due to the !this._pool condition in _removeFromPool.
  As a result, the connection remains in the _freeConnections queue, causing an infinite processing loop

Here are some illustrative tests:

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

const pool = new createPool({
  connectionLimit: 5,
  maxIdle: 5,
  idleTimeout: 2000,
});

pool.getConnection((_err1, connection1) => {
  connection1.release();
  connection1.release();

  setTimeout(() => {
    try {
      assert(pool._freeConnections.length === 1, 'expect 1 connections');
    } finally {
      pool.end();
    }
  }, 300);
});

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

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

pool.getConnection((_err1, connection1) => {
  connection1.release();
  connection1.release();

  setTimeout(() => {
    pool.getConnection((_err1, connection1) => {
      pool.getConnection((_err1, connection2) => {
        try {
          assert(connection1 !== connection2, 'Should be uniq connections');
        } finally {
          pool.end();
        }
      })
    })
  }, 500);
});

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

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

pool.getConnection((_err1, connection1) => {
  connection1.release();
  connection1.release();


  setTimeout(() => {
    try {
      assert(true, 'No infinite loop');
    } finally {
      pool.end();
    }
  }, 3000);
});

[dg-korolev]

Perhaps this problem will be solved by an additional wrapper over connection, it can be given outside the package, for example, from getConnection, and implement in this wrapper over connection protections against repeated release calls

[aarond-sp]

We've encountered the same issue, which led to transactions being committed prematurely and rollbacks not applying as expected.

While this is partly due to incorrect usage of the library, I believe it would be a valuable improvement to add safeguards that help prevent developers from running into such issues.

The same behaviour has also already been described here: #2325 (comment)

[dg-korolev]

I think this issue mentions the same behavior with infinite loop
#2171