search

coopernurse / node-pool

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

Version 2.4.1 breaks refreshIdle functionality #137

Closed ijaooo closed 7 years ago

ijaooo commented 8 years ago

Commit 2cee31a56fb7f20b78a6a1ba8149954def5d2829 broke the original refreshIdle functionality: if refreshIdle is true and min pool size is set, connections in the pool will never timeout.

Only workaround is to set min to 0, but this would defeat the purpose of having a pool.

Can we have the commit rolled back by changing
_this._factory.refreshIdle && (this.count... in line 214 back to _this._factory.refreshIdle || (this.count...

sandfox commented 8 years ago

Thanks for reporting this, I'll try and get round to looking at this soon, in the meantime something to consider that reverting that commit will re-introduce the bug that commit is removing which isn't helpful.

ijaooo commented 8 years ago

Agree. But this commit directly introduces a new bug which renders existing functionality unusable - we have no way to force the refresh of idle connections.

hyss777 commented 8 years ago

Still come up with similar bug with version2.4.2. The package fail to keep the connection in the latest version.

My solution for the time is add a error listener in node-mysql's connection object. as the below code goes，or roll back to the version 2.2.1

as

However, this should actually be a bug.

sandfox commented 7 years ago

ok, finally got to the bottom of this. It would seem the intent behind the refreshIdle flag was little unclear / misunderstood by me and the committer. I'm going to revert 2cee31a and probably add a litte note explaining the desired behaviour and release this on the 2.4 branch. v3 has the option to specify whether idleTimeouts affect should affect resources below the "min" count.

sandfox commented 7 years ago

fixed in version 2.4.6