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

Connections are not returned to the pool if acquireConnectionTimeout is triggered #1694

Closed
koskimas opened this Issue Sep 26, 2016 · 3 comments

Comments

Projects
None yet
2 participants
@koskimas
Collaborator

koskimas commented Sep 26, 2016

This test fails on 0.12.1

const _ = require('lodash');
const Knex = require('knex');
const Promise = require('bluebird');

const knex = Knex({
  client: 'postgres',
  pool: {
    min: 0,
    max: 2
  },
  connection: {
    host: '127.0.0.1',
    database: 'test'
  },
  acquireConnectionTimeout: 100
});

// Start two transactions so that the pool is filled.
_.times(2, (idx) => {
  knex.transaction(() => {
    return Promise.delay(500);
  }).then(() => {
    console.log(`transaction ${idx} ended`);
  });
});

knex.raw('select 1').catch(() => {
  console.log('query failed as expected because of acquireConnectionTimeout');
});

setTimeout(() => {
  // By the time this is called, all the transactions should have been committed and
  // the connections returned to the pool, but the connection for which the acquireConnectionTimeout
  // was called, was never returned to the pool.
  console.log('connections in use:', knex.client.pool._inUseObjects.length, 'should be 0 right?');
}, 1500);
@koskimas

This comment has been minimized.

Collaborator

koskimas commented Sep 26, 2016

Could this line be the culprit? From what I understand from the pool's docs, acquire method returns a boolean, for which the line above calls abort.

@tgriesser

This comment has been minimized.

Owner

tgriesser commented Sep 27, 2016

Sounds like a bug. Thanks for the test case, will look to get this tested/patched soon... I think I must've missed the tickets where the abort stuff was added, I'll dive into it soon and see if I can figure it out.

@tgriesser tgriesser added the bug label Sep 27, 2016

@koskimas

This comment has been minimized.

Collaborator

koskimas commented Sep 27, 2016

I actually looked into it already and there seems to be no good way to fix this using the generic-pool since it has no way to abort connection acquires. In addition generic-pool doesn't have a built-in acquire timeout so that timeout stuff could be moved inside the pool.

There is an issue for both of these problems in generic-pool repository, but they are not fixed. There are even pull requests for them that a have been around for quite some time. generic-pool doesn't seem to be that well maintained anymore.

Because of these reasons and the fact that our server is pretty much useless at the moment because of the pool bugs, I wrote my own pool Tarn.js that only has the needed features. Here's a commit that patches knex to use it.

I was going to create a pull request and start a discussion if Tarn.js could be used as knex's default pool. I'm committed to molding Tarn.js to serve knex.js as well as possible and writing a comprehensive test suite for Tarn and even knex so that bugs like this never surface again. Knex is the sole reason I wrote it. I'm also happy to give you push rights to the repo if you want.

What do you think @tgriesser?

tgriesser added a commit that referenced this issue Oct 9, 2016

@tgriesser tgriesser referenced this issue Oct 9, 2016

Merged

0.12.3 #1731

tgriesser added a commit that referenced this issue Oct 9, 2016

tgriesser added a commit that referenced this issue Oct 9, 2016

Merge branch 'master' into refactor
* master:
  release 0.12.3
  Update changelog
  Fix #1710
  Fix #1694
  release 0.12.2
  Update changelog for 0.12.2 features
  Don't force master as release branch
  Remove unused pool2 dependency
  Fix #1701
  Fix #1675
  Fix for #1691

RubenSlabbert added a commit to RubenSlabbert/knex that referenced this issue Nov 9, 2016

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