fix: close all connections in pool.end()

[mikeroelens]

There is an issue with pool.end(). If you have multiple idle connections and then call pool.end() it will actually only close a subset of the idle connections. The reason is that as part of onDestory there is a splice operation on the global connections array. As pool.end() is mapping over connections we're removing items from the array so it misses some.

So what actually happens today:

1. Open 5 connections
2. Call await pool.end()
3. Only 3 of the connections are closed, pool.end() resolves
4. Once idleTimeout expires for the remaining 2 connections, those 2 will be closed as well
   • This is where it gets problematic. If you have a high idleTimeout setting, there will be hanging connections until idleTimeout is hit, even after pool.end() resolves

Simple reproduction:

const { createPool, sql } = require('slonik');

async function main() {
  const pool = await createPool('postgres://postgres:postgres@localhost:5432/mike', {
    maximumPoolSize: 5,
    idleTimeout: 10000,
  });

  let i = 1;
  async function executeQuery() {
    try {
      console.log('Running query ', i++);
      await pool.query(sql.unsafe`SELECT 1`);
    } catch (err) {
      console.error('Query failed:', err);
    }
  }

  const queryPromises = Array(5).fill().map(() => executeQuery());
  await Promise.all(queryPromises);

  console.log('\nPool state', pool.state());

  // Close the pool
  console.log('\nClosing pool...')
  await pool.end();
  console.log('Pool closed');

  console.log('\nPool state', pool.state());
}

main().catch(err => console.error('Error in main:', err));

This will ouptut:

Running query  1
Running query  2
Running query  3
Running query  4
Running query  5

Pool state {
  acquiredConnections: 0,
  idleConnections: 5,
  pendingDestroyConnections: 0,
  pendingReleaseConnections: 0,
  state: 'ACTIVE',
  waitingClients: 0
}

Closing pool...
Pool closed

# 👀 Note how there are still 2 idle connections even after the pool says its 'ENDED'
Pool state {
  acquiredConnections: 0,
  idleConnections: 2,
  pendingDestroyConnections: 0,
  pendingReleaseConnections: 0,
  state: 'ENDED',
  waitingClients: 0
}

# ⏳ The process now hangs for 10s (idleTimeout setting)

[changeset-bot]

🦋 Changeset detected

Latest commit: 2186809

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 13 packages

Name	Type
slonik	Major
@slonik/benchmark	Major
@slonik/dataloaders	Major
slonik-interceptor-field-name-transformation	Major
slonik-interceptor-query-cache	Major
slonik-interceptor-query-logging	Major
slonik-sql-tag-raw	Major
@slonik/driver	Major
@slonik/errors	Major
@slonik/pg-driver	Major
@slonik/sql-tag	Major
@slonik/types	Major
@slonik/utilities	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[mikeroelens]

This delay in this test was actually masking this issue. What was happening was that pool.end() didn't actually close all connections like it was supposed to, we were simply waiting for the idleTimeout to expire so the connections automatically get closed.

Removed the delay and set idleTimeout: 'DISABLE_TIMEOUT' so that if we break this functionality the test should timeout.

[mikeroelens]

Note: Marked this as major. It's a bugfix but technically I think user's applications could've inadvertently relied on this buggy behaviour