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

Added option.maxTries #184

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Aleksandras-Novikovas
Copy link

Setting this option stops trying to create resource after specified
consequent failures. Prevents pool going into infinite loop if resource
can not be created.

@elhigu
Copy link

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.
@Aleksandras-Novikovas
Copy link
Author

I have re-based my pull request against latest version.

@sandfox sandfox self-assigned this Feb 12, 2018
@Aleksandras-Novikovas
Copy link
Author

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

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

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

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

Callback factoryCreateErrorwould be useful if we could:

  1. know exactly which acquire call fails
  2. stop further attempts
  3. bubble up exception to a calling code

@elhigu
Copy link

elhigu commented Jun 27, 2019

@gsbelarus is it your own code which is using node-pool or 3rd party library?

@gsbelarus
Copy link

gsbelarus commented Jun 27, 2019 via email

@elhigu
Copy link

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

Thanks! Will give it a try.

@Kikobeats
Copy link
Collaborator

Please resolve the conflicts and I will be happy to merge this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants