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

Bugfix/knex hangs when pool is filled with trs #1177

Merged
merged 6 commits into from Feb 3, 2016

Conversation

Projects
None yet
2 participants
@wubzz
Collaborator

wubzz commented Feb 1, 2016

@tgriesser @elhigu @rhys-vdw In regards to #1040 and #1171 , this change will throw a Timeout error including information about the query that was run, when acquiring a connection is not possible or simply takes too long.

I am in no way implying that this is the correct way of fixing this, but it does work. The issues mentioned it needs to be configurable, so a new option has been added to the knex options object. (not the 'Pool' object since technically it has no relevance there)

Not so sure the hacky stuff done in the test file is allowed, but couldn't figure out a better way of doing it.. :)

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 3, 2016

@wubzz Awesome! I'll check this later on today.

});
}).catch(trx.rollback);
})
.then(function() {

This comment has been minimized.

@elhigu

elhigu Feb 3, 2016

Collaborator

IIRC you could remove done() calls and just return a promise from test case to make code slightly more robust.

var knexConfig = _.clone(knex.client.config);
knexConfig.pool.min = 0;
knexConfig.pool.max = 1;
knexConfig.acquireConnectionTimeout = 10000;

This comment has been minimized.

@elhigu

elhigu Feb 3, 2016

Collaborator

Does this test take now 10 seconds to run? If so could we set timeout to < 1 sec to prevent test from taking so long?

//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();

This comment has been minimized.

@elhigu

elhigu Feb 3, 2016

Collaborator

Is intention of this to always fail if this then block is executed? In that case expect(false).to.be.ok() would be more readable to me, since it fail independently from res.

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();

This comment has been minimized.

@elhigu

elhigu Feb 3, 2016

Collaborator

If you return promise from transaction, calling trx.commit / trx.rollback wouldn't be necessary. Not sure why you commit transaction on error though :)

This comment has been minimized.

@wubzz

wubzz Feb 3, 2016

Collaborator

I called commit to release the connection and enter the transaction's callback which in turn called done(), but as you mentioned simply returning knexDb.transaction is easier than using 'done'. Switching to that! 👍

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 3, 2016

Generally code looks really good :) There were few things I didn't understand completely. Only serious issue of those was the 10 second timeout during testrun.

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Feb 3, 2016

Yeah, not sure what I was thinking with the 10secs.. Changed it to 1sec. Simplified the test code as well.

elhigu added a commit that referenced this pull request Feb 3, 2016

Merge pull request #1177 from wubzz/bugfix/knex_hangs_when_pool_is_fi…
…lled_with_trs

Bugfix/knex hangs when pool is filled with trs

@elhigu elhigu merged commit 081f77f into tgriesser:master Feb 3, 2016

1 check passed

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

This comment has been minimized.

Collaborator

elhigu commented Feb 3, 2016

@wubzz thanks a lot, this was really important feature 👍 most of the time people had no idea why their queries just hang.

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