Upgrade generic-pool to v3.1.7 (Attempt)

[wubzz]

• Check test in CI all dialects
• Look over how pool options are sent into knex and make appropiate adjustments

Relates to #1969 & #2106

[wubzz]

Not familiar enough with mysql to comprehend the issues in the docker tests, and unable to debug these locally atm..

[elhigu]

Breakage did seem to be consistent between different node versions. We need to figure out if the test is doing something wrong or if pool is again really leaking connections if database is closed and restarted. I don't have linux system currently installed either.

@smulesoft do you happen by any change to have time to check out why this fails?

[smulesoft]

Interesting, it doesn't work with postgres either. But the errors are different:

MySQL Test: npm run babel; DB=mysql npm run test

It fails when it tries to reuse the connection pool after the database is restarted.

Postgres Test: npm run babel; DB=postgres npm run test

It hangs when it tries to use the connection pool when the database is "offline"

I'll get more into it when I have more time. But because the errors are different, I would say that the new pool may require changes in the code of each specific dialect.

[jstewmon]

I got the tests working with the patch below. It seems that there were two problems:

1. testOnBorrow needs to be true so that connections are validated before acquisition (maybe knex should default this pool option to true, since the validation methods are all fast and synchronous)
2. validate must return a Promise that resolves with a boolean (or simply a boolean) - the promise should not reject.

I could submit a PR with these changes, but it might just be easier for @wubzz to apply the patch to his branch - I'm ok with that.

diff --git a/src/client.js b/src/client.js
index 92c261b..5293519 100644
--- a/src/client.js
+++ b/src/client.js
@@ -200,7 +200,7 @@ assign(Client.prototype, {
         validate: (connection) => {
           if (connection.__knex__disposed) {
             helpers.warn(`Connection Error: ${connection.__knex__disposed}`)
-            return Promise.reject();
+            return Promise.resolve(false);
           }
           return this.validateConnection(connection)
         }
@@ -220,7 +220,7 @@ assign(Client.prototype, {
   },
 
   validateConnection(connection) {
-    return Promise.resolve();
+    return Promise.resolve(true);
   },
 
   // Acquire a connection from the pool.
diff --git a/src/dialects/maria/index.js b/src/dialects/maria/index.js
index d4f7c1f..4d9d77f 100644
--- a/src/dialects/maria/index.js
+++ b/src/dialects/maria/index.js
@@ -45,9 +45,9 @@ assign(Client_MariaSQL.prototype, {
 
   validateConnection(connection) {
     if(connection.connected === true) {
-      return Promise.resolve();
+      return Promise.resolve(true);
     }
-    return Promise.reject();
+    return Promise.resolve(false);
   },
 
   // Used to explicitly close a connection, called internally by the pool
diff --git a/src/dialects/mssql/index.js b/src/dialects/mssql/index.js
index cd17e8b..27985cf 100644
--- a/src/dialects/mssql/index.js
+++ b/src/dialects/mssql/index.js
@@ -89,9 +89,9 @@ assign(Client_MSSQL.prototype, {
 
   validateConnection(connection) {
     if(connection.connected === true) {
-      return Promise.resolve();
+      return Promise.resolve(true);
     }
-    return Promise.reject();
+    return Promise.resolve(false);
   },
 
   // Used to explicitly close a connection, called internally by the pool
diff --git a/src/dialects/mysql/index.js b/src/dialects/mysql/index.js
index 0654fc5..be6d267 100644
--- a/src/dialects/mysql/index.js
+++ b/src/dialects/mysql/index.js
@@ -86,9 +86,9 @@ assign(Client_MySQL.prototype, {
 
   validateConnection(connection) {
     if(connection.state === 'connected' || connection.state === 'authenticated') {
-      return Promise.resolve();
+      return Promise.resolve(true);
     }
-    return Promise.reject();
+    return Promise.resolve(false);
   },
 
   // Grab a connection, run the query via the MySQL streaming interface,
diff --git a/src/dialects/mysql2/index.js b/src/dialects/mysql2/index.js
index 29621e9..ca24b99 100644
--- a/src/dialects/mysql2/index.js
+++ b/src/dialects/mysql2/index.js
@@ -29,8 +29,11 @@ assign(Client_MySQL2.prototype, {
     return require('mysql2')
   },
 
-  validateConnection() {
-    return Promise.resolve();
+  validateConnection(connection) {
+    if (connection._fatalError) {
+      return Promise.resolve(false)
+    }
+    return Promise.resolve(true);
   },
 
   // Get a raw connection, called by the `pool` whenever a new
diff --git a/test/docker/index.js b/test/docker/index.js
index 58b201a..0db3a2e 100644
--- a/test/docker/index.js
+++ b/test/docker/index.js
@@ -1,27 +1,29 @@
+/*global describe*/
+
 'use strict';
 
-var os      = require('os');
-var proc    = require('child_process')
-var config  = require('../knexfile');
-var knex    = require('../../knex');
+const os      = require('os');
+const proc    = require('child_process')
+const config  = require('../knexfile');
+const knex    = require('../../knex');
 
 if (canRunDockerTests()) {
-  var dialectName;
-  for (dialectName in config) {
+  for (const dialectName in config) {
     if (config[dialectName].docker) {
-      require('./reconnect')(config[dialectName], knex);
+      describe(`${dialectName} dialect`, function() {
+        require('./reconnect')(config[dialectName], knex);
+      })
     }
   }
 }
 
 function canRunDockerTests() {
-  var isLinux   = os.platform() === 'linux';
-
+  const isLinux   = os.platform() === 'linux';
+  const isDarwin  = os.platform() === 'darwin'
   // dont even try on windows / osx for now
-  var hasDocker = false;
-  if (isLinux) {
+  let hasDocker = false;
+  if (isLinux || isDarwin) {
     hasDocker = proc.execSync('docker -v 1>/dev/null 2>&1 ; echo $?').toString('utf-8') === '0\n';
   }
-
-  return isLinux && hasDocker;
+  return hasDocker;
 }
diff --git a/test/docker/reconnect.js b/test/docker/reconnect.js
index fd6b4ea..3c6610e 100644
--- a/test/docker/reconnect.js
+++ b/test/docker/reconnect.js
@@ -119,7 +119,8 @@ module.exports = function(config, knex) {
         max:                       7,
         idleTimeoutMillis:         IDLE_TIMEOUT_MILLIS,
         acquireTimeoutMillis:      ACQUIRE_TIMEOUT_MILLIS,
-        evictionRunIntervalMillis: EVICTION_RUN_INTERVAL_MILLIS
+        evictionRunIntervalMillis: EVICTION_RUN_INTERVAL_MILLIS,
+        testOnBorrow: true
       },
       connection:               {
         database: dockerConf.database,
diff --git a/test/knexfile.js b/test/knexfile.js
index d127ad2..c5b6d18 100644
--- a/test/knexfile.js
+++ b/test/knexfile.js
@@ -89,7 +89,18 @@ var testConfigs = {
     },
     pool: mysqlPool,
     migrations: migrations,
-    seeds: seeds
+    seeds: seeds,
+    docker: {
+      factory:   './mysql/index.js',
+      container: 'knex-test-mysql2',
+      image:     'mysql:5.7',
+      database:  'mysql',
+      username:  'root',
+      password:  'root',
+      hostPort:  '49153',
+      client:    'mysql',
+      timeout:   60 * 1000
+    }
   },
 
   oracle: {
@@ -135,7 +146,7 @@ var testConfigs = {
       password:  '',
       hostPort:  '49152',
       client:    'pg',
-      timeout:   10 * 1000
+      timeout:   60 * 1000
     }
   },
 

[wubzz]

@jstewmon Thanks for looking into this. Pushed your changes to this PR.

[smulesoft]

Wow, so it seems the docker tests were annoying albeit helpful this time!

[jstewmon]

Looks like the sqlite3 tests eventually failed because multiple connections were in use. Constraining the sqlite3 pool size to 1 fixes the issue, but I wonder why this is a new problem?

diff --git a/test/knexfile.js b/test/knexfile.js
index 41ecb4a..e6ce2d9 100644
--- a/test/knexfile.js
+++ b/test/knexfile.js
@@ -142,7 +142,7 @@ var testConfigs = {
     connection: testConfig.sqlite3 || {
       filename: __dirname + '/test.sqlite3'
     },
-    pool: pool,
+    pool: Object.assign({max: 1}, pool),
     migrations: migrations,
     seeds: seeds
   },

[jstewmon]

The node 6 build failed because one of the docker tests took more than 60s. All others passed.

The docker tests revealed some unhandled rejection warnings, which are related to the wrapping of timeouts by the knex client.

I've fixed those on my branch.

I'm just going to work on my branch b/c @wubzz only partially applied my patch, which made fixing forward on this PR too inconvenient.

@elhigu would you like for me to just open a new PR?

[wubzz]

@jstewmon Unless I'm mistaken I applied everything except the increase in timeouts. That was intentional as I generally am not fond of increasing timeouts without a valid reason. Something obviously must have caused the need for increase - what might that be?

I'll close this and you can open your own PR.

[jstewmon]

@wubzz no worries. Thanks for the great start on this!

I got a mocha timeout with the 10s timeout on the postgres docker tests. I think you omitted my changes to test/docker/index.js and the addition of a docker config for mysql2 in test/knexfile.js. Anyway, I figured making my own branch based on yours was easier than juggling my local changes and posting patches in comments.

The new PR is #2208