Skip to content
This repository has been archived by the owner on Dec 30, 2019. It is now read-only.

Max checkout timeout event #110

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

johanneswuerbach
Copy link
Contributor

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.

Copy link
Collaborator

@charmander charmander left a 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)
Copy link
Collaborator

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.releaseclient.release and the third argument to the connect callback should be equivalent.

@johanneswuerbach johanneswuerbach force-pushed the force-unlock branch 3 times, most recently from 3b90ea1 to 1068272 Compare December 3, 2018 11:08
@johanneswuerbach johanneswuerbach mentioned this pull request Dec 14, 2018
@brianc
Copy link
Owner

brianc commented Dec 14, 2018

Yeah I can't quite review this now until it's rebased on top of master after #109 lands. Initial thoughts:

  • Might want to call it maxCheckoutMillis or something and the event name maxCheckoutExceeded and if maxCheckoutMillis is exceeded you can emit a message w/ the client. The thing is...in this situation you probably want to crash your node app because you're doing something wrong and leaking clients...but having a generic event like pool.on('maxCheckoutExceeded', (client) => {}) . The idea of 'forcing an unlock' is likely going to just further jack up your application since you'll be using clients which should have been checked in, but aren't...this will make it easier to diagnose though in your app if you're leaking clients
  • The diff is a bit hard to read, so I'll wait to review the behavior exactly until after 109 is merged.
  • I recommend wrapping pg-pool in a library in your own app that proxies through to it...this allows you to do this kinda thing in your own code: https://node-postgres.com/guides/project-structure

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.
@johanneswuerbach
Copy link
Contributor Author

@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.

@johanneswuerbach
Copy link
Contributor Author

@brianc happy new year 🎉 and friendly ping :-)

@johanneswuerbach johanneswuerbach changed the title Force unlock timeout Max checkout timeout event Jan 15, 2019
@johanneswuerbach
Copy link
Contributor Author

@brianc if you have time, I would appreciate another look :-)

@johanneswuerbach
Copy link
Contributor Author

@brianc anything else required to move this forward?

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

Successfully merging this pull request may close these issues.

None yet

3 participants