Allow for better control over connection issues #6
Conversation
|
Some early feedback: If one should be able to set a retry limit then that should be as a feature addition to https://github.com/voxpelli/node-promised-retry – this library just adds the minimal amount of postgres stuff on top of that generic retry mechanism. I will have to revisit this later though when I have more proper time to read and think about your feedback, don't feel I can give it the proper attention right now unfortunately. |
| return new Promise(function (resolve, reject) { | ||
| db.on('error', reject); |
There was a problem hiding this comment.
Some other early feedback: This wouldn't work if there's a database error after the initial connect as the Promise has already been resolved by then and a Promise can only be resolved or rejected once.
The current error handler is meant to catch errors that happens after a successful connect so that if the database somehow eg. shuts down or otherwise closes the connection, that a new connection can automatically be established the next time one is needed.
Any solution would have to take that scenario into account as well.
There was a problem hiding this comment.
@voxpelli Okay, I see. I'll take this into consideration and adjust the patch accordingly.
We're currently testing this change internally. When I change anything, I'll push -f into this PR.
Thanks for the feedback! :)
|
@voxpelli Regarding the limit, the change will respect a property |
|
Some feedback on the limit then: It's good that it is configurable, but I would prefer the default to be unlimited – both because that's how I want it to be in my applications where I use this module and because then this change would be backwards compatible. The check for whether the limit has been reached or not would be better added to the Could you move the check-part of this PR to that module instead? And just have this module forward its new |
|
@voxpelli Sounds reasonable. Expect a PR soon. |
|
Step 1 should be done with voxpelli/node-promised-retry#3 |
|
@voxpelli What do you think about this implementation? I'll work on tests as well. |
|
I'm horrible at writing tests :P I'd really appreciate some guidance if you can give any. |
Previously, the error handler on the pg.Client would cause an infinite connection loop. Additionally, the connection would be retried infinitely either way, because the amount of reconnection attempts could not be limited. Closes #5
|
Does this test actually reproduce the error from #5? Don't think I've fully grasped the scenario in which this error happens. When trying to connect to an invalid host, as the current test does, the connect should return an error itself – no? Rather than sending it as an event? And will there ever be a case where an event will be sent before the connect callback has been called? I may need to double check with the |
|
Well, I observed the event being sent, which caused the infinite reconnect loop. Although I noticed different behavior between debug and non-debug runs. |
|
@voxpelli I'm wondering if what I observed is related to brianc/node-postgres#746 |
|
@oliversalzburg That sounds likely – feels like this is something that (Perhaps @brianc himself has some thoughts?) |
|
@voxpelli I agree. I'm already working on a fix at brianc/node-postgres#843 |
|
@oliversalzburg Is there still something you want to get merged here now after the PR to the |
|
@voxpelli Thanks for reminding me. It seems like the issue has been resolved by the changes in pg. We have not observed any more issues. |
Previously, the error handler on the pg.Client would cause an infinite connection loop. Additionally, the connection would be retried infinitely either way, because the amount of reconnection attempts could not be limited.
Closes #5