-
Notifications
You must be signed in to change notification settings - Fork 64
Max checkout timeout event #110
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one comment for now since the diff is obscured.
index.js
Outdated
client.end() | ||
}, options.forceUnlockTimeoutMillis) | ||
return callback(undefined, client, (err) => { | ||
clearTimeout(tid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it needs to replace client.release
– client.release
and the third argument to the connect
callback should be equivalent.
3b90ea1
to
1068272
Compare
Yeah I can't quite review this now until it's rebased on top of master after #109 lands. Initial thoughts:
This even will let folks who have scattered access directly to an instance of a pool throughout their application to diagnose things though which is good. |
Emit a `maxCheckoutExceeded` event when a connection has been checked out longer then the user defined `maxCheckoutMillis` timeout.
1068272
to
8174166
Compare
@brianc thank for the review, I rebased this PR, removed the force-unlock logic and only left the event emitting. Let me know what you think. |
@brianc happy new year 🎉 and friendly ping :-) |
@brianc if you have time, I would appreciate another look :-) |
@brianc anything else required to move this forward? |
Based on #109
Adds a new force unlock timeout, which is by default disabled to stay backwards compatible.
This timeout forcefully ends the client if a client was taken from the pool longer then
forceUnlockTimeoutMillis
and the main use-case for this is preventing any kind of pool client leaks, which could render a pool consumers completely unusable.WIP but happy about comments, will add tests as soon as possible.