node-pg-pool icon indicating copy to clipboard operation
node-pg-pool copied to clipboard

Max checkout timeout event

Open johanneswuerbach opened this issue 7 years ago • 5 comments

Based on https://github.com/brianc/node-pg-pool/pull/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.

johanneswuerbach avatar Dec 03 '18 00:12 johanneswuerbach

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.

brianc avatar Dec 14 '18 18:12 brianc

@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 avatar Dec 16 '18 20:12 johanneswuerbach

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

johanneswuerbach avatar Jan 02 '19 19:01 johanneswuerbach

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

johanneswuerbach avatar Feb 11 '19 20:02 johanneswuerbach

@brianc anything else required to move this forward?

johanneswuerbach avatar Apr 17 '19 06:04 johanneswuerbach