From eea94a88963603a36290f38de598c8342fd3b1e8 Mon Sep 17 00:00:00 2001 From: wubzz Date: Mon, 1 Feb 2016 20:40:06 +0100 Subject: [PATCH 1/4] TimeoutError when acquiring con is not possible --- index.html | 21 ++++++++++++++ src/runner.js | 20 ++++++++++++- test/integration/builder/transaction.js | 37 +++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 1 deletion(-) diff --git a/index.html b/index.html index 8ff966657f..5bb5b94928 100644 --- a/index.html +++ b/index.html @@ -60,6 +60,7 @@
  •   – client
  •   – debug
  •   – pooling
    • +
  •   – acquireConnectionTimeout
  •   – migrations
    • @@ -504,6 +505,26 @@

    Pooling

    You may use knex.destroy by passing a callback, or by chaining as a promise, just not both.

    + +

    acquireConnectionTimeout

    + +

    + acquireConnectionTimeout defaults to 60000ms and is used to determine how long knex should wait before throwing a timeout error when acquiring a connection is not possible. + The most common cause for this is using up all the pool for transaction connections and then attempting to run queries outside of transactions while the pool is still full. + The error thrown will provide information on the query the connection was for to simplify the job of locating the culprit. +

    + +
    +var knex = require('knex')({
+  client: 'pg',
+  connection: {...},
+  pool: {...},
+  acquireConnectionTimeout: 10000
+  });
+
+
    + +

    Migrations

    diff --git a/src/runner.js b/src/runner.js index 4143eea82e..29072f3e25 100644 --- a/src/runner.js +++ b/src/runner.js @@ -135,8 +135,26 @@ assign(Runner.prototype, { // Check whether there's a transaction flag, and that it has a connection. ensureConnection: function() { var runner = this + var acquireConnectionTimeout = runner.client.config.acquireConnectionTimeout || 60000 return Promise.try(function() { - return runner.connection || runner.client.acquireConnection() + return runner.connection || new Promise((resolver, rejecter) => { + runner.client.acquireConnection() + .timeout(acquireConnectionTimeout) + .then(resolver) + .catch(Promise.TimeoutError, (error) => { + var timeoutError = new Error('Knex: Timeout acquiring a connection. The pool is probably full. Are you missing a .transacting(trx) call?') + var additionalErrorInformation = { + timeoutStack: error.stack + } + if(runner.builder) { + additionalErrorInformation.sql = runner.builder.sql + additionalErrorInformation.bindings = runner.builder.bindings + } + assign(timeoutError, additionalErrorInformation) + rejecter(timeoutError) + }) + .catch(rejecter) + }) }).disposer(function() { if (runner.connection.__knex__disposed) return runner.client.releaseConnection(runner.connection) diff --git a/test/integration/builder/transaction.js b/test/integration/builder/transaction.js index d2619ff93e..0e33d6de43 100644 --- a/test/integration/builder/transaction.js +++ b/test/integration/builder/transaction.js @@ -3,6 +3,8 @@ 'use strict'; var Promise = testPromise; +var Knex = require('../../../knex'); +var _ = require('lodash'); module.exports = function(knex) { @@ -277,6 +279,41 @@ module.exports = function(knex) { }); + it('#1040, #1171 - When pool is filled with transaction connections, Non-transaction queries should not hang the application, but instead throw a timeout error', function(done) { + this.timeout(15000); + //To make this test easier, I'm changing the pool settings to max 1. + var knexConfig = _.clone(knex.client.config); + knexConfig.pool.min = 0; + knexConfig.pool.max = 1; + knexConfig.acquireConnectionTimeout = 10000; + + var knexDb = new Knex(knexConfig); + + //Create a transaction that will occupy the only available connection, and avoid trx.commit. + knexDb.transaction(function(trx) { + trx.raw('SELECT version()').then(function() { + + //No connection is available, so try issuing a query without transaction. + //Since there is no available connection, it should throw a timeout error based on knex pool config. + knexDb.raw('select * FROM accounts WHERE username = ?', ['Test']) + .then(function(res) { + expect(res).to.not.exist(); + }) + .catch(function(error) { + expect(error.bindings).to.be.an('array'); + expect(error.bindings[0]).to.equal('Test'); + expect(error.sql).to.equal('select * FROM accounts WHERE username = ?'); + expect(error.message).to.equal('Knex: Timeout acquiring a connection. The pool is probably full. Are you missing a .transacting(trx) call?'); + trx.commit(); + }); + }).catch(trx.rollback); + }) + .then(function() { + done(); + }) + .catch(done); + }); + }); }; From 80ee5a8021920020b2d87988cd451f67d618aea0 Mon Sep 17 00:00:00 2001 From: wubzz Date: Mon, 1 Feb 2016 20:54:14 +0100 Subject: [PATCH 2/4] select version() => select 1 = 1 for test version() doesn't exist in all dbs. --- test/integration/builder/transaction.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/builder/transaction.js b/test/integration/builder/transaction.js index 0e33d6de43..60d2753ac4 100644 --- a/test/integration/builder/transaction.js +++ b/test/integration/builder/transaction.js @@ -291,7 +291,7 @@ module.exports = function(knex) { //Create a transaction that will occupy the only available connection, and avoid trx.commit. knexDb.transaction(function(trx) { - trx.raw('SELECT version()').then(function() { + trx.raw('SELECT 1 = 1').then(function() { //No connection is available, so try issuing a query without transaction. //Since there is no available connection, it should throw a timeout error based on knex pool config. From 57bac6cd583b7a4f1fff450a369ec975b4c6d26a Mon Sep 17 00:00:00 2001 From: wubzz Date: Mon, 1 Feb 2016 21:27:30 +0100 Subject: [PATCH 3/4] Fix some indentation and test comment --- src/runner.js | 3 +++ test/integration/builder/transaction.js | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/runner.js b/src/runner.js index 29072f3e25..7c8d64c447 100644 --- a/src/runner.js +++ b/src/runner.js @@ -146,11 +146,14 @@ assign(Runner.prototype, { var additionalErrorInformation = { timeoutStack: error.stack } + if(runner.builder) { additionalErrorInformation.sql = runner.builder.sql additionalErrorInformation.bindings = runner.builder.bindings } + assign(timeoutError, additionalErrorInformation) + rejecter(timeoutError) }) .catch(rejecter) diff --git a/test/integration/builder/transaction.js b/test/integration/builder/transaction.js index 60d2753ac4..83318eee42 100644 --- a/test/integration/builder/transaction.js +++ b/test/integration/builder/transaction.js @@ -294,7 +294,7 @@ module.exports = function(knex) { trx.raw('SELECT 1 = 1').then(function() { //No connection is available, so try issuing a query without transaction. - //Since there is no available connection, it should throw a timeout error based on knex pool config. + //Since there is no available connection, it should throw a timeout error based on `aquireConnectionTimeout` from the knex config. knexDb.raw('select * FROM accounts WHERE username = ?', ['Test']) .then(function(res) { expect(res).to.not.exist(); From 4244bb1ec266783e4a9d5f1acabfbd24d0c2b491 Mon Sep 17 00:00:00 2001 From: wubzz Date: Wed, 3 Feb 2016 22:50:45 +0100 Subject: [PATCH 4/4] Simplify the test and lower the time to 1sec --- test/integration/builder/transaction.js | 44 +++++++++++-------------- 1 file changed, 19 insertions(+), 25 deletions(-) diff --git a/test/integration/builder/transaction.js b/test/integration/builder/transaction.js index 83318eee42..cc0a23cead 100644 --- a/test/integration/builder/transaction.js +++ b/test/integration/builder/transaction.js @@ -279,39 +279,33 @@ module.exports = function(knex) { }); - it('#1040, #1171 - When pool is filled with transaction connections, Non-transaction queries should not hang the application, but instead throw a timeout error', function(done) { - this.timeout(15000); + it('#1040, #1171 - When pool is filled with transaction connections, Non-transaction queries should not hang the application, but instead throw a timeout error', function() { //To make this test easier, I'm changing the pool settings to max 1. var knexConfig = _.clone(knex.client.config); knexConfig.pool.min = 0; knexConfig.pool.max = 1; - knexConfig.acquireConnectionTimeout = 10000; + knexConfig.acquireConnectionTimeout = 1000; var knexDb = new Knex(knexConfig); //Create a transaction that will occupy the only available connection, and avoid trx.commit. - knexDb.transaction(function(trx) { - trx.raw('SELECT 1 = 1').then(function() { - - //No connection is available, so try issuing a query without transaction. - //Since there is no available connection, it should throw a timeout error based on `aquireConnectionTimeout` from the knex config. - knexDb.raw('select * FROM accounts WHERE username = ?', ['Test']) - .then(function(res) { - expect(res).to.not.exist(); - }) - .catch(function(error) { - expect(error.bindings).to.be.an('array'); - expect(error.bindings[0]).to.equal('Test'); - expect(error.sql).to.equal('select * FROM accounts WHERE username = ?'); - expect(error.message).to.equal('Knex: Timeout acquiring a connection. The pool is probably full. Are you missing a .transacting(trx) call?'); - trx.commit(); - }); - }).catch(trx.rollback); - }) - .then(function() { - done(); - }) - .catch(done); + return knexDb.transaction(function(trx) { + trx.raw('SELECT 1 = 1').then(function () { + //No connection is available, so try issuing a query without transaction. + //Since there is no available connection, it should throw a timeout error based on `aquireConnectionTimeout` from the knex config. + return knexDb.raw('select * FROM accounts WHERE username = ?', ['Test']) + }) + .then(function () { + //Should never reach this point + expect(false).to.be.ok(); + }).catch(function (error) { + expect(error.bindings).to.be.an('array'); + expect(error.bindings[0]).to.equal('Test'); + expect(error.sql).to.equal('select * FROM accounts WHERE username = ?'); + expect(error.message).to.equal('Knex: Timeout acquiring a connection. The pool is probably full. Are you missing a .transacting(trx) call?'); + trx.commit();//Test done + }); + }); }); });