search

coopernurse / node-pool

Generic resource pooling for node.js
2.38k stars 259 forks source link

Support timing out acquire() call #127

Closed kevinburkeshyp closed 7 years ago

kevinburkeshyp commented 8 years ago

The options dictionary for pool.acquire() now takes a second argument, timeout. Pass an integer number of milliseconds to cancel the acquire() call and hit the callback with pool.Full after the given amount of time has elapsed. Hits the callback with an immediate error if the timeout is a negative number, zero, or NaN.

Adds a number of tests that this logic behaves as expected.

This work was sponsored by Shyp.

kevinburkeshyp commented 8 years ago

Note, this builds on #125, so both commits are present in the diff. Happy to rebase/merge/submit against the branch/whatever you like.

kevinburkeshyp commented 8 years ago

I pushed 81c28ad with a fudge for the timer tests. I'm not too happy with it. I can rebase down before merge.

subeeshcbabu-zz commented 8 years ago

+1

pdufour commented 8 years ago

+1

sandfox commented 8 years ago

Just an update. I'm sorry for the delay, work and life have been rather hectic at the moment, but I've scheduled in some time to look at this after Wednesday. Thanks for the patience

kevinburkeshyp commented 8 years ago

Removed the block option in 5e4bf20. I can rebase down before merge.

kevinburkeshyp commented 8 years ago

Pushed the error callback to the next tick in 295e00a.

kevinburkeshyp commented 8 years ago

Lint errors should be fixed in 5556a81.

sandfox commented 8 years ago

thanks

(passive aggresive internal voice): I WILL finish looking at this soon and merge it!

subeeshcbabu-zz commented 8 years ago

Great stuff. Waiting for this publish.

kevinburkeshyp commented 8 years ago

@sandfox thoughts here? wondering if we should fork this lib & node-postgres or wait for upstream

sandfox commented 8 years ago

@kevinburkeshyp this is for reals actually open in my text editor and I'm looking at it now :-)

kevinburkeshyp commented 8 years ago

Thanks!

kevinburke commented 8 years ago

I've published this PR as a new package here: https://www.npmjs.com/package/generic-pool-timeout and submitted it for inclusion as part of node-postgres here: https://github.com/brianc/node-postgres/pull/1006.

kevinburkeshyp commented 8 years ago

hey @sandfox - anything I can do to help push this across the finish line? code review other PR's? make a donation?

sandfox commented 8 years ago

Hi Kevin,

Sorry for the haitus on this, I had a bit of collision of burnout, changing jobs, and some personal stuff. Anywayaaay, onto code! Whilst looking at adding some other features I had to make some more big refactors on top you work and after various iterations and dead ends I've ended up realising alot of the future work isn't possible with jamming event-emitters into the lib/api.

The branch much features should have an implementation of the feature you are after, with some slight changes to accomodate other features. There are still a couple of test failures outstanding which I don't think can fixed without add event-emitters (which incidentally is needed to properly isolate the resource creation cycle/aspect from the resource issuance aspect/cycle, originally the two were too tightly coupled. A thing requesting a resource shouldn't care/know/etc about the resource factory having errors creating individual resources (but it will still obviously care about an overall failure to be able to create resources)).

The side effect of this is that the API change will probably mean a major version number bump which I wanted to avoid, but maybe that isn't a bad thing.

sandfox commented 7 years ago

closing as this is now included as part of v3