New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

upgrade generic-pool to 3.1.7 #2208

Merged
merged 8 commits into from Sep 6, 2017

Conversation

Projects
None yet
3 participants
@jstewmon
Contributor

jstewmon commented Sep 1, 2017

Building on @wubzz work from #2128

@jstewmon jstewmon referenced this pull request Sep 1, 2017

Closed

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

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

This comment has been minimized.

@jstewmon

jstewmon Sep 1, 2017

Contributor

@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?

This comment has been minimized.

@elhigu

elhigu Sep 4, 2017

Collaborator

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

This comment has been minimized.

@jstewmon

jstewmon Sep 4, 2017

Contributor

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)

This comment has been minimized.

@jstewmon

jstewmon Sep 4, 2017

Contributor

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.

This comment has been minimized.

@elhigu

elhigu Sep 5, 2017

Collaborator

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

fix compatibility issues with generic-pool v3
- 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

This comment has been minimized.

Contributor

jstewmon commented Sep 5, 2017

@elhigu this is ready for review.

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

This comment has been minimized.

@elhigu

elhigu Sep 5, 2017

Collaborator

If destroyRawConnection returns a promise it could be also returned.

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

This comment has been minimized.

@elhigu

elhigu Sep 5, 2017

Collaborator

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

This comment has been minimized.

@elhigu

elhigu Sep 5, 2017

Collaborator

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

This comment has been minimized.

@jstewmon

jstewmon Sep 5, 2017

Contributor

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.

This comment has been minimized.

@elhigu

elhigu Sep 5, 2017

Collaborator

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.

})
})
)
});

This comment has been minimized.

@elhigu

elhigu Sep 5, 2017

Collaborator

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)

This comment has been minimized.

@jstewmon

jstewmon Sep 5, 2017

Contributor

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.

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

This comment has been minimized.

@elhigu

elhigu Sep 5, 2017

Collaborator

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.

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

This comment has been minimized.

@elhigu

elhigu Sep 5, 2017

Collaborator

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.

This comment has been minimized.

@jstewmon

jstewmon Sep 5, 2017

Contributor

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.

This comment has been minimized.

@elhigu

elhigu Sep 5, 2017

Collaborator

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.

@elhigu

elhigu approved these changes Sep 5, 2017

@jstewmon

This comment has been minimized.

Contributor

jstewmon commented Sep 5, 2017

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

@jstewmon

This comment has been minimized.

Contributor

jstewmon commented Sep 6, 2017

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

This comment has been minimized.

Contributor

jstewmon commented Sep 6, 2017

@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

This comment has been minimized.

Collaborator

elhigu commented Sep 6, 2017

Very nice. Thank you!

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jstewmon jstewmon deleted the jstewmon:generic-pool-v3 branch Sep 6, 2017

@wubzz wubzz referenced this pull request Dec 14, 2017

Closed

Memory leak in 0.14.x #2383

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment