closes #56 - idle resources below the "min" value not destroyed #57

Merged
merged 2 commits into from Jan 17, 2013

Conversation

Projects
None yet
3 participants
Contributor

wshaver commented Dec 14, 2012

No description provided.

Contributor

wshaver commented Dec 14, 2012

I've reviewed the travis build failure, but can't figure out why or what test is failing.

Owner

coopernurse commented Dec 15, 2012

Hi there,

I haven't looked at the build failure yet either, but I'd like to discuss this change. The current implementation respects both the "min" and "idleTimeoutMillis" properties on the pool. The behavior you saw in #56 doesn't surprise me too much, although I'd have to see your full program to know if the timings match what I would expect.

Your idleTimeoutMillis was set quite low, so the observed behavior was pathological. But some folks are using this setting to ensure that their pool of, say, database connections, hasn't gone stale. idleTimeout might be something like 2 minutes, and the pool promises that the elements in the pool have been active within idleTimeout.

I don't want to lose that behavior, as some folks are relying on it.

Given your settings it seems that idleTimeoutMillis is never doing much for you. What if we tweak your patch so that your change takes effect if idleTimeoutMillis < 0? That would allow your pool to dynamically size between min/max, but whenever removeIdle() runs you'll size down to min. Effectively reapIntervalMillis would govern how quickly your pool sizes back down to min, and you could adjust that depending on how quickly you wanted to size down.

thoughts?

Contributor

wshaver commented Dec 15, 2012

If users are relying on "refreshing" some resources in the pool, perhaps we
should have a "refreshIdle" flag. I think an explicit flag, default true,
would be better than a setting the idle timeout. Perhaps a user doesn't
want to refresh, but also doesn't want to cull resources that have been
used recently. (Actually my use case. I'm using the pooler to maintain DOM
windows on the server side for server side rendering of client side
JavaScript. Expensive to build and attach jQuery etc, no need to ever
refresh.)
On Dec 15, 2012 8:12 AM, "James Cooper" notifications@github.com wrote:

Hi there,

I haven't looked at the build failure yet either, but I'd like to discuss
this change. The current implementation respects both the "min" and
"idleTimeoutMillis" properties on the pool. The behavior you saw in #56https://github.com/coopernurse/node-pool/issues/56doesn't surprise me too much, although I'd have to see your full program to
know if the timings match what I would expect.

Your idleTimeoutMillis was set quite low, so the observed behavior was
pathological. But some folks are using this setting to ensure that their
pool of, say, database connections, hasn't gone stale. idleTimeout might be
something like 2 minutes, and the pool promises that the elements in the
pool have been active within idleTimeout.

I don't want to lose that behavior, as some folks are relying on it.

Given your settings it seems that idleTimeoutMillis is never doing much
for you. What if we tweak your patch so that your change takes effect if
idleTimeoutMillis < 0? That would allow your pool to dynamically size
between min/max, but whenever removeIdle() runs you'll size down to min.
Effectively reapIntervalMillis would govern how quickly your pool sizes
back down to min, and you could adjust that depending on how quickly you
wanted to size down.

thoughts?


Reply to this email directly or view it on GitHubhttps://github.com/coopernurse/node-pool/pull/57#issuecomment-11406083.

Contributor

wshaver commented Dec 19, 2012

I've added the refreshIdle flag as discussed above, defaulting to 'true' so that original functionality is maintained.

sricc commented Jan 16, 2013

Will this issue be merged soon? I've applied this commit locally for testing and it fixes a runaway pool for me when a database error is thrown. Without this fix, my getPoolSize() shows ever decreasing negative numbers and the availableObjectsCount() increases well passed the max connections until the database throws a TOO MANY CONNECTIONS error.

Without this fix it makes it very difficult for me to use generic-pool, which is a shame because I've used it successfully in the past!

Thanks.

coopernurse merged commit 69992f7 into coopernurse:master Jan 17, 2013

1 check passed

default The Travis build passed
Details
Owner

coopernurse commented Jan 17, 2013

Ok this is in master -- Could either of you give it a go? If I get a thumbs up I'll publish to npm

sricc commented Jan 17, 2013

I can confirm that with the new merge and setting refreshIdle to false my issue is fixed.

Thanks!

Owner

coopernurse commented Jan 17, 2013

Terrific. Thanks for the quick reply. v2.0.3 is tagged and published to npm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment