Fixed passing connection errors directly to the query (#2336)

* Fixed passing connection errors directly to the query

* Added missing use strict and fixed select to be oracle compatible.

* Fixed dialect name which is mixed in different configs

* Oracledb didnt have database config attribute

src/client.js

@@ -224,14 +224,32 @@ assign(Client.prototype, {
               if (poolConfig.afterCreate) {
                 return Promise.promisify(poolConfig.afterCreate)(connection)
               }
+            })
+            .catch(err => {
+              // Acquire connection must never reject, because generic-pool
+              // will retry trying to get connection until acquireConnectionTimeout is
+              // reached. acquireConnectionTimeout should trigger in knex only 
+              // in that case if aquiring connection waits because pool is full
+              // https://github.com/coopernurse/node-pool/pull/184
+              // https://github.com/tgriesser/knex/issues/2325
+              return {
+                genericPoolMissingRetryCountHack: true,
+                __knex__disposed: err,
+                query: () => {
+                  throw err; // pass error to query
+                }
+              };
             });
         },
         destroy: (connection) => {
+          if (connection.genericPoolMissingRetryCountHack) {
+            return;
+          }
           if (poolConfig.beforeDestroy) {
             helpers.warn(`
-            beforeDestroy is deprecated, please open an issue if you use this
-            to discuss alternative apis
-          `)
+              beforeDestroy is deprecated, please open an issue if you use this
+              to discuss alternative apis
+            `)
             poolConfig.beforeDestroy(connection, function() {})
           }
           if (connection !== void 0) {

test/tape/index.js

@@ -12,6 +12,7 @@ require('./seed')
 require('./migrate')
 require('./pool')
 require('./knex')
+require('./invalid-db-setup')(knexfile)
 
 Object.keys(knexfile).forEach(function(key) {
 
@@ -23,7 +24,7 @@ Object.keys(knexfile).forEach(function(key) {
   // Tear down the knex connection
   tape(knex.client.driverName + ' - transactions: after', function(t) {
     knex.destroy(function() {
-      t.ok(true, 'Knex client destroyed')
+      t.pass('Knex client destroyed')
       t.end()
     })
   })

test/tape/invalid-db-setup.js

@@ -0,0 +1,96 @@
+'use strict';
+
+const tape = require('tape')
+const _ = require('lodash');
+const makeKnex = require('../../knex')
+const pool = require('generic-pool');
+const Bluebird = require('bluebird');
+
+module.exports = (knexfile) => {
+  Object.keys(knexfile).forEach((key) => {
+    const dialect = knexfile[key].dialect || knexfile[key].client;
+
+    if (dialect !== 'sqlite3' && dialect !== 'oracledb') {
+      const knexConf = _.cloneDeep(knexfile[key]);
+      knexConf.connection.database = knexConf.connection.db = 'I-refuse-to-exist';
+      knexConf.acquireConnectionTimeout = 4000;
+      const knex = makeKnex(knexConf);
+
+      tape(dialect + ' - propagate error when DB does not exist', t => {
+        t.plan(1);
+        t.timeoutAfter(1000);
+        knex('accounts').select(1)
+          .then(res => {
+            t.fail(`Query should have failed, got: ${JSON.stringify(res)}`);
+          })
+          .catch(Bluebird.TimeoutError, e => {
+            t.fail(`Query should have failed with non timeout error`);
+          })
+          .catch(e => {
+            t.ok(e.message.indexOf('I-refuse-to-exist') > 0, `all good, failed as expected with msg: ${e.message}`);
+          });
+      });
+
+      tape(dialect + ' - propagate error when DB does not exist for stream', t => {
+        t.plan(1);
+        t.timeoutAfter(1000);
+
+        knex.select(1)
+          .stream(stream => {})
+          .then(res => {
+            t.fail(`Stream query should have failed, got: ${JSON.stringify(res)}`);
+          })
+          .catch(Bluebird.TimeoutError, e => {
+            t.fail(`Stream query should have failed with non timeout error`);
+          })
+          .catch(e => {
+            t.ok(e.message.indexOf('I-refuse-to-exist') > 0, `all good, failed as expected with msg: ${e.message}`);
+          });
+      });
+
+      tape.onFinish(() => {
+        knex.destroy();
+      });
+    }
+
+    if (dialect !== 'sqlite3') {
+      const knexConf = _.cloneDeep(knexfile[key]);
+      knexConf.acquireConnectionTimeout = 10;
+      knexConf.pool = { max: 1 };
+      const knex = makeKnex(knexConf);
+
+      tape(dialect + ' - acquireConnectionTimeout works', t => {
+        t.plan(2);
+        t.timeoutAfter(1000);
+
+        let hoggerTrx;
+        knex.transaction(trx => {
+          // just hog the only connection.
+          hoggerTrx = trx;
+        }).then(() => {
+          t.pass('transaction was resolved');
+          t.end();
+        });
+
+        knex('accounts').select(1)
+          .then(() => {
+            t.fail('query should have stalled');
+          })
+          .catch(Bluebird.TimeoutError, e => {
+            t.pass('Got acquireTimeout error');
+          })
+          .catch(e => {
+            t.fail(`should have got acquire timeout error, but got ${e.message} instead.`);
+          })
+          .finally(() => {
+            hoggerTrx.commit(); // release stuff
+          });
+      });
+
+      tape.onFinish(() => {
+        knex.destroy();
+      });
+    }
+
+  });
+};