upgrade generic-pool to 3.1.7 #2208

jstewmon · 2017-09-01T18:13:55Z

Building on @wubzz work from #2128

jstewmon · 2017-09-01T18:32:25Z

test/knexfile.js

@@ -142,7 +152,7 @@ var testConfigs = {
    connection: testConfig.sqlite3 || {
      filename: __dirname + '/test.sqlite3'
    },
-    pool: pool,
+    pool: Object.assign({max: 1}, pool),


@elhigu the sqlite3 tests were failing due to resource contention (multiple connections to the db file) on this branch w/o restricting the max pool size to 1. I'm not sure why the tests were passing on master, though b/c it seems sensible that the max pool size would have to be 1 for sqlite. Did we introduce a bug for sqlite3 in this PR for which the workaround is setting max pool size to 1?

I'm not sure... I believe that nowadays sqlite also supports multiple connections. Which test was failing with bigger pool size?

With max: 1, the tests always pass. Without it, I get two different failures, depending on whether test/test.sqlite3 exists...

$ rm test/test.sqlite3; DB=sqlite3 npm test gives:

knex.migrate (transactions disabled) knex.migrate.latest (all transactions disabled) Knex:warning - migrations failed with error: SELECT foo FROM unknown_table - SQLITE_ERROR: no such table: unknown_table ✓ should create column even in invalid migration knex.migrate.latest (per-migration transaction disabled) Knex:warning - migrations failed with error: SELECT foo FROM unknown_table - SQLITE_ERROR: no such table: unknown_table ✓ should run all working migration files in the specified directory 1) should create column in invalid migration with transaction disabled 758 passing (7s) 1 failing 1) sqlite3 | sqlite3 knex.migrate (transactions disabled) knex.migrate.latest (per-migration transaction disabled) should create column in invalid migration with transaction disabled: AssertionError: expected false to equal true + expected - actual -false +true at test/integration/migrate/index.js:300:29 From previous event: at SchemaBuilder.Target.then (lib/interface.js:9:1751) at Context.<anonymous> (test/integration/migrate/index.js:299:77)

If I leave test/test.sqlite3 existing, DB=sqlite3 npm test gives:

sqlite3 | sqlite3 ✓ #test number mssql should not allow unsafe bigint ✓ #test number mssql should allow safe bigint ✓ #1781 - decimal value must not be converted to integer Schema ✓ #1430 - .primary() & .dropPrimary() same for all dialects dropTable ✓ has a dropTableIfExists method createTable ✓ Callback function must be supplied 1) is possible to chain .catch 711 passing (2s) 1 failing 1) sqlite3 | sqlite3 Schema createTable is possible to chain .catch: create table `catch_test` (`id` integer not null primary key autoincrement) - SQLITE_ERROR: table `catch_test` already exists Error: SQLITE_ERROR: table `catch_test` already exists at Error (native) From previous event: at Client_SQLite3._query (lib/dialects/sqlite3/index.js:9:7581) at Client_SQLite3.query (lib/client.js:9:11871) at Runner.<anonymous> (lib/runner.js:9:7046) From previous event: at Runner.queryArray (lib/runner.js:9:9562) at lib/runner.js:9:3550 From previous event: at Runner.run (lib/runner.js:9:2779) at SchemaBuilder.Target.then (lib/interface.js:9:1687) at SchemaBuilder.Target.(anonymous function) [as catch] (lib/interface.js:9:4430) at Context.<anonymous> (test/integration/schema/index.js:68:19)

I think I figured it out - the sqlite3 dialect defines a poolDefaults method that sets max: 1, but that method is no longer called by client with the changes on this branch. I'll make another pass to figure out the fix.

Sounds right, indeed I remember that at some point default for sqlite was set to that (some years ago).

- restore Client.poolDefaults to allow dialect overrides - connection validators should always resolve with a boolean - rely on generic pool acquire timeouts to avoid unhandled rejections - simplify promise usage, avoiding Promise constructor anti-pattern - enable testOnBorrow in docker tests to evict broken connections - enable docker tests on Darwin - enable docker tests for mysql2 dialect

jstewmon · 2017-09-05T16:15:05Z

@elhigu this is ready for review.

elhigu · 2017-09-05T16:41:29Z

src/client.js

+            poolConfig.beforeDestroy(connection, function() {})
+          }
+          if (connection !== void 0) {
+            this.destroyRawConnection(connection)


If destroyRawConnection returns a promise it could be also returned.

elhigu · 2017-09-05T16:45:17Z

src/client.js

+    const timeouts = [
+      this.config.acquireConnectionTimeout,
+      poolConfig.acquireTimeoutMillis
+    ].map(parseInt).filter(x => x > 0);


Throwing an error on invalid parameters would be better than just filtering them out silently.

Also there was default acquireConnectionTimeout of 60000, should it be added here?

I agree that throwing would be better...

Previously, an invalid value for config.acquireConnectionTimeout would result in the timeout being 1 (default behavior when delay is not a valid, positive integer). If pool.acquireTimeoutMillis is invalid, generic-pool will throw throw new Error('delay must be a positive int') during acquisition.

I'd like to use joi, ajv or similar to define a schema for the config object to validate the options passed in the constructor, so that validation isn't sprinkled throughout the implementation. I could start with a very simple schema that covers just these properties, and others could extend the schema to include more validation later. How do you feel about that?

Alternatively, we can explicitly validate both settings here and throw an error if either one is value !== undefined && value !== null && (isNaN(parseInt(value)) || parseInt(value) < 0)? I think that's the smallest possible variation from the current behavior that results in an error, though the error will be moved up to client construction from connection acquisition in the case that pool.acquireTimeoutMillis has an invalid value.

Being able to validate config objects (and that way also document configuration better) with real validator sounds like a really good idea, but I think adding that deserves its own PR.

So I would go with alternative in this PR. Later on if proper config validation is added, many places in code can be revisited and checks that configuration vars are fine may be dropped.

elhigu · 2017-09-05T16:51:03Z

src/client.js

-      })
-    })
+        )
+      });


Above only TimeoutError is catched, should also generic errors be catched and is there need to release connection from pool in some cases? (I'm just comparing, what code did earlier and what it seems to be doing now)

Previously, wasRejected would only be true if timer t fired, in which case, pool.acquire may have resolved with a resource that needed to be released. Now that we simply call pool.acquire, there is no resource to be released in the catch handler.

elhigu · 2017-09-05T16:57:52Z

src/client.js

    })
  },

  // Destroy the current connection pool for the client.
  destroy(callback) {
-    const promise = new Promise((resolver) => {
+    return new Promise((resolver) => {


could this be written:

return Promise.resolve( this.pool && this.pool.drain() .then(() => this.pool.clear()) .then(() => { this.pool = void 0; if(typeof callback === 'function') { callback(); } }) );

to prevent need for new Promise() + remembering to call resolve function.

elhigu · 2017-09-05T17:03:13Z

test/docker/index.js

-  var hasDocker = false;
-  if (isLinux) {
+  let hasDocker = false;
+  if (isLinux || isDarwin) {


I cant remember why I didn't enable this to osx earlier, I'm pretty sure there was a reason for that, but this is fine like this. If it causes problems it can always be changed.

I didn't find any issues. Maybe the problem before was that docker machine and docker for mac did not support the unix domain socket at /var/run/docker.sock... It has been supported for some time now, so I think this should be ok.

Yeah I think that was the reason 👍

elhigu

This looks great! Code is a lot simpler in many places after this one 👍 I added some comments which I would like to have an answer, before merging this though.

jstewmon · 2017-09-05T22:43:20Z

The return destroyRawConnection promise from dialects commit is causing tests to fail. I'll get that sorted out along with the timeout validation.

jstewmon · 2017-09-06T15:25:59Z

The tests should pass this time. There were a couple of issues:

Promise.fromCallback usage wasn't binding the connection methods to the connection instance
maria's close event seems to only fire if the connection was connected or connecting

jstewmon · 2017-09-06T16:50:14Z

@elhigu I think this is in good shape now. I did not add any new tests for the timeout validation b/c I didn't see a good place to add them. One idea would be to add some expected failures to test/integration/index.js as illustrated in the patch below. (The knexfile.js changes are for demonstration purposes only.)

diff --git a/test/integration/index.js b/test/integration/index.js
index 86c1f06..5a165b7 100644
--- a/test/integration/index.js
+++ b/test/integration/index.js
@@ -1,4 +1,4 @@
-/*global after*/
+/*global after it*/
 
 'use strict';
 
@@ -9,8 +9,11 @@ var fs     = require('fs');
 
 var Promise = require('bluebird')
 
-Promise.each(Object.keys(config), function(dialectName) {
-  return require('./suite')(logger(knex(config[dialectName])));
+it('should run cleanly', function () {
+  return Promise.each(Object.keys(config), function(dialectName) {
+    return require('./suite')(logger(knex(config[dialectName])));
+  })
+  // TODO: we could test for bad config values here
 })
 
 after(function(done) {
diff --git a/test/knexfile.js b/test/knexfile.js
index b4ffc52..c5e9278 100644
--- a/test/knexfile.js
+++ b/test/knexfile.js
@@ -16,6 +16,7 @@ var pool = {
 };
 
 var mysqlPool = _.extend({}, pool, {
+  acquireTimeoutMillis: -1,
   afterCreate: function(connection, callback) {
     Promise.promisify(connection.query, {context: connection})("SET sql_mode='TRADITIONAL';", []).then(function() {
       callback(null, connection);
@@ -59,6 +60,7 @@ var testConfigs = {
 
   mysql: {
     dialect: 'mysql',
+    // acquireConnectionTimeout: -1,
     connection: testConfig.mysql || {
       database: "knex_test",
       user: "root",

elhigu · 2017-09-06T19:51:01Z

Very nice. Thank you!

wubzz added 4 commits September 1, 2017 12:38

Upgrade generic-pool to v3.1.7 (Attempt)

76daa95

Remove callback from pool create function

f9c6c3e

Fix ESLint

d08e318

Send whole poolConfig to generic-pool (for idleTimeoutMillis etc)

1992a78

jstewmon mentioned this pull request Sep 1, 2017

Upgrade generic-pool to v3.1.7 (Attempt) #2128

Closed

2 tasks

jstewmon commented Sep 1, 2017

View reviewed changes

jstewmon force-pushed the generic-pool-v3 branch from 81374a7 to 04704f6 Compare September 5, 2017 15:27

jstewmon force-pushed the generic-pool-v3 branch from 04704f6 to 0da3e4f Compare September 5, 2017 15:37

elhigu reviewed Sep 5, 2017

View reviewed changes

elhigu requested changes Sep 5, 2017

View reviewed changes

jstewmon force-pushed the generic-pool-v3 branch from b77ec81 to b8af249 Compare September 5, 2017 20:04

elhigu approved these changes Sep 5, 2017

View reviewed changes

Jonathan Stewmon added 2 commits September 6, 2017 09:11

simplify Client.destroy promise handling

584b5ba

validate timeout settings and to 60s

0975ed7

jstewmon force-pushed the generic-pool-v3 branch from b8af249 to fc376d3 Compare September 6, 2017 15:23

jstewmon force-pushed the generic-pool-v3 branch from fc376d3 to aff0327 Compare September 6, 2017 15:45

return destroyRawConnection promise from dialects

d10cd50

jstewmon force-pushed the generic-pool-v3 branch from aff0327 to d10cd50 Compare September 6, 2017 16:45

elhigu merged commit 9122cbf into knex:master Sep 6, 2017

jstewmon deleted the generic-pool-v3 branch September 6, 2017 20:12

This was referenced Nov 1, 2017

Memory leak in knex v0.13.0 #2165

Closed

Memory leak with knex 0.12.6 #2106

Closed

wubzz mentioned this pull request Nov 13, 2017

Pool settings confusing in 0.14 (IdleTimeout) #2322

Closed

wubzz mentioned this pull request Dec 14, 2017

Memory leak in 0.14.x #2383

Closed

wubzz mentioned this pull request Feb 2, 2018

Lets use Tarn.js as a pool instead of generic-pool #2339

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upgrade generic-pool to 3.1.7 #2208

upgrade generic-pool to 3.1.7 #2208

jstewmon commented Sep 1, 2017

jstewmon Sep 1, 2017

elhigu Sep 4, 2017

jstewmon Sep 4, 2017

jstewmon Sep 4, 2017

elhigu Sep 5, 2017

jstewmon commented Sep 5, 2017

elhigu Sep 5, 2017

elhigu Sep 5, 2017

elhigu Sep 5, 2017

jstewmon Sep 5, 2017

elhigu Sep 5, 2017

elhigu Sep 5, 2017

jstewmon Sep 5, 2017

elhigu Sep 5, 2017

elhigu Sep 5, 2017

jstewmon Sep 5, 2017

elhigu Sep 5, 2017

elhigu left a comment

jstewmon commented Sep 5, 2017

jstewmon commented Sep 6, 2017

jstewmon commented Sep 6, 2017

elhigu commented Sep 6, 2017

upgrade generic-pool to 3.1.7 #2208

upgrade generic-pool to 3.1.7 #2208

Conversation

jstewmon commented Sep 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstewmon commented Sep 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elhigu left a comment

Choose a reason for hiding this comment

jstewmon commented Sep 5, 2017

jstewmon commented Sep 6, 2017

jstewmon commented Sep 6, 2017

elhigu commented Sep 6, 2017