Destroying inactive resources can cause unhandled promise rejection

[sfc-gh-ljagielski]

Hi, I wanted to share a possible bug. If destroy method passed to Pool constructor is async, then its rejection might cause an unhandled promise rejection, for example in idle timer (it doesn't seem to await resource destruction). Here's a test that demonstrates the behavior (it fails with an unhandled rejection)

import * as tap from 'tap';
import { Pool } from '../../src';

tap.test('should finish the test without unhandled rejection', async (t) => {
  const pool = new Pool<{ foo: number }>({
    create: () => Promise.resolve({ foo: 1 }),
    // Pool doesn't await destroy when closing idle connections
    destroy: () => Promise.reject('Ohnoo'),
    validate: () => true,
    max: 5,
    min: 0,
    idleTimeoutMillis: 500,
    reapIntervalMillis: 1000,
  });

  const res = await pool.acquire(); // create first resource, it will be kept
  t.equal(res, { foo: 1 });
  pool.release(res);
  // wait 3 seconds for idle timer to cause unhandled rejection
  await new Promise((resolve) => {
    setTimeout(resolve, 3000);
  });
  await pool.destroyAllNow().then(
    () => console.log('Destroyed'),
    (err) => console.log('Error, whatever' + err),
  );
});

Thanks for looking at this,

[sushantdhiman]

Thanks for reporting this, I have taken a look at this.

When the pool.release is called for a given resource, that method does not wait for the pool.destroy method, as pool.release itself is sync method. This can cause un-handled rejection if factory.destroy is rejected.

The simplest way to solve this would be to convert the pool.release method to async. This would be a breaking change.