-
Notifications
You must be signed in to change notification settings - Fork 260
Added option.maxTries #184
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
Added option.maxTries #184
Conversation
elhigu
commented
Nov 16, 2017
PR seems to be missing tests.
I would really need this feature for knex since now if create fails due to bad DB setup, pool just waits until acquireConnectionTimeout.
This feature would allow us to decide if we want to wait only in that case if create() method itself is waiting or pool is full. We would get an error directly in case if create() call rejected. For now it looks like I need to implement timeout back to knex side or do some other nasty hacks.
@coopernurse If this PR is fixed is will it be merged?
Setting this option stops trying to create resource after specified consequent failures. Prevents pool going into infinite loop if resource can not be created.
59889c4 to
02ab9aa
Compare
Aleksandras-Novikovas
commented
Feb 11, 2018
I have re-based my pull request against latest version.
Added autotest for maxTries.
Aleksandras-Novikovas
commented
May 13, 2018
I've added tests for maxTries feature.
Could you merge this PR?
I really need it. As I can see many others need this functionality too.
gsbelarus
commented
Jun 26, 2019
Is it hard to merge this PR? We desperately need in ability to stop attempts to create resource (attach to database in our case) after first try failed.
sandfox
commented
Jun 27, 2019
Is it possible to use the existing
pool.on('factoryCreateError', function(err){
//log stuff maybe
})
to achieve the same outcome, or is that sufficiently overcomplicated to pipe together?
gsbelarus
commented
Jun 27, 2019
Consider we are pooling connections to db server. Somehow wrong login/password were specified. Current behavior is that the library will try to reconnect again and again... Correct work in this case would be only one attempt and then throw exception to a calling code.
Now, to emulate this we do first connection without pooling. If it succeeds then we disconnect and run through pool library. If it fails we analyze return value and throw appropriate exception.
gsbelarus
commented
Jun 27, 2019
Callback factoryCreateErrorwould be useful if we could:
- know exactly which acquire call fails
- stop further attempts
- bubble up exception to a calling code
elhigu
commented
Jun 27, 2019
@gsbelarus is it your own code which is using node-pool or 3rd party library?
gsbelarus
commented
Jun 27, 2019
via email
elhigu
commented
Jun 27, 2019
We ended up writing our own pool implementation which hat that error propagation feature in addition to various timeout configurations to be used with knex. You migth want to take a look https://github.com/vincit/tarn.js propagateCreateError is the option for that.
That pool was written for knex, so it will probably not get any additional features in the future, but those current features are really robust.
gsbelarus
commented
Jun 27, 2019
Thanks! Will give it a try.
Kikobeats
commented
Jul 12, 2021
Please resolve the conflicts and I will be happy to merge this!
Setting this option stops trying to create resource after specified
consequent failures. Prevents pool going into infinite loop if resource
can not be created.