Skip to content
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

Conversation

@wubzz
Copy link
Member

@wubzz 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
Copy link
Member

@elhigu 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
Member

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
Member

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
Member

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
Member

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
Author Member

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
Copy link
Member

@elhigu 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
Copy link
Member Author

@wubzz 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
…lled_with_trs

Bugfix/knex hangs when pool is filled with trs
@elhigu elhigu merged commit 081f77f into knex:master Feb 3, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@elhigu
Copy link
Member

@elhigu 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
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants