Release connections after connection time out

If a connection timeout was thrown, the connection would remain queued
and ultimately consume a connection from the pool. The connection wasn't
used, so was permanently removed from the pool. After enough timeouts,
the pool would be fully consumed and no connections would be released.

Author: c24w

index.js

@@ -18,12 +18,23 @@ function kill(connection, query) {
   query('KILL ' + connection.escape(connection.connection.threadId));
 }
 
+async function getConnectionWithTimeout(getConnection, timeoutMs) {
+  const gettingConnection = getConnection();
+  try {
+    return await Promise.race([
+      gettingConnection,
+      timeout('connect', timeoutMs)
+    ]);
+  } catch (err) {
+    gettingConnection.then(conn => conn.release()).catch(() => {});
+    throw err;
+  }
+}
+
 function queryWithTimeout(options, getConnection, query) {
   return async (firstArg, ...otherArgs) => {
-    const connection = await Promise.race([
-      getConnection(),
-      timeout('connect', options.acquireTimeout)
-    ]);
+
+    const connection = await getConnectionWithTimeout(getConnection, options.acquireTimeout);
 
     let queryTimeout = options.defaultQueryTimeout;
     if (typeof firstArg === 'object') {
@@ -61,4 +72,4 @@ async function connect({
   return pool;
 }
 
-module.exports = { connect };
+module.exports = { connect };

test.js

@@ -120,8 +120,8 @@ describe('database', () => {
     });
   });
 
-  describe('when all connections are in use', () => {
-    let queryError, queryDuration;
+  describe('when querying while all connections are in use', () => {
+    let queryError, queryDuration, connectionHog, queuedQuery;
     const acquireTimeout = 500;
 
     beforeEach(async () => {
@@ -133,21 +133,73 @@ describe('database', () => {
         connectionLimit: 1
       });
 
-      db.query('DO SLEEP(5)'); // Hog the only available connection
+      connectionHog = db.query('DO SLEEP(2)'); // Hog the only available connection
+
+      db.once('enqueue', () => queuedQuery = true);
+      const start = new Date();
+      try { await db.query('SELECT 1'); } // Query while connection is hogged
+      catch (e) { queryError = e; }
+      queryDuration = new Date() - start;
+    });
+
+    it('times out getting a connection', () => {
+      assert(queryError, 'query didn\'t error');
+      assert.equal(queryError.message, 'Database connect timed out after 500ms');
+
+      assert.isAtLeast(queryDuration, acquireTimeout - 1); // Sometimes comes in early?!
+      assert.isAtMost(queryDuration, acquireTimeout + 100); // 100ms grace
+    });
+
+    it('makes the timed out connection available for use', async () => {
+      assert.isTrue(queuedQuery, 'Timed out query was not queued');
+      await connectionHog;
+      assert.isDefined(await db.query('SELECT 1'));
+    }).timeout(5000);
+  });
+
+  describe('when acquiring the connection times out', () => {
+    let queryError, badDbServer, queryDuration;
+    const acquireTimeout = 500;
+
+    beforeEach(async () => {
+      badDbServer = require('net').createServer().listen(3306);
+
+      db = await database.connect({ acquireTimeout });
 
       const start = new Date();
       try { await db.query('SELECT 1'); }
       catch (e) { queryError = e; }
       queryDuration = new Date() - start;
     });
 
-    it('times out', async () => {
+    afterEach(function (done) {
+      this.timeout(10000);
+      if (badDbServer.listening) { badDbServer.close(done); }
+      else { done(); }
+    });
+
+    it('times out', () => {
       assert(queryError, 'query didn\'t error');
       assert.equal(queryError.message, 'Database connect timed out after 500ms');
 
       assert.isAtLeast(queryDuration, acquireTimeout - 1); // Sometimes comes in early?!
       assert.isAtMost(queryDuration, acquireTimeout + 100); // 100ms grace
     });
+
+    describe('when the connection then errors', () => {
+      let connError;
+      const unhandledError = err => connError = err;
+      beforeEach(() => process.once('unhandledRejection', unhandledError));
+      afterEach(() => process.off('unhandledRejection', unhandledError));
+
+      it('swallows the connection error', done => {
+        badDbServer.close(() => {
+          if (connError) { assert.fail(`Did not expect an error, but got '${connError}'`); }
+
+          done();
+        });
+      }).timeout(10000);
+    });
   });
 
   it('reuses connections', async () => {
@@ -161,4 +213,4 @@ describe('database', () => {
     const results = await db.query('SELECT 2');
     assert.isDefined(results);
   });
-});
+});