myndzi / pool2

38 stars 8 forks source link

When connection acquisition timeout occurs and min is 0, pool2 silently does nothing #25

Closed txase closed 8 years ago

txase commented 8 years ago

Scenario:

Example log:

Sat, 06 Aug 2016 19:18:47 GMT pool2 New ResourceRequest (id=0, timeout=Infinity) 
Sat, 06 Aug 2016 19:18:47 GMT pool2 Growing pool: no resource to serve request (p=1, tba=0, tbt=0, max=1) 
Sat, 06 Aug 2016 19:18:47 GMT pool2 Attempting to acquire resource (cur=0, ac=0) 
Sat, 06 Aug 2016 19:18:48 GMT pool2 Timed out acquiring resource 
Sat, 06 Aug 2016 19:18:48 GMT pool2 Couldn't allocate new resource: Timed out acquiring resource 
Sat, 06 Aug 2016 19:18:49 GMT pool2 Attempting to gracefully clean up late-arrived resource (id=0) 
Sat, 06 Aug 2016 19:18:49 GMT pool2 Attempting to gracefully remove resource (id=0)

When acquisition fails, we get to: https://github.com/myndzi/pool2/blob/310728e528fc96520cf6e2f57129888d7eb0ea6d/lib/pool.js#L456-L472

Because the pool is not live yet and bailAfter is set to Infinity, we fall through to the following line:

setTimeout(this._ensureMinimum.bind(this), this.backoff.next());

We then wait the backoff period only to end up calling _ensureMinimum, which returns immediately because min is 0.

What needs to happen is one of the following:

myndzi commented 8 years ago

Thoroughly reported, thanks :)

The intent was "I had a problem, do whatever needs doing"; in the case of a minimum resource count this operates as intended, but in this case ensureMinimum should be checking whether there are pending requests in addition to the count, or more likely, don't "reuse" ensureMinimum for this scenario, instead providing something with a more correct semantic meaning.

myndzi commented 8 years ago

Published a new version

txase commented 8 years ago

Looks good! I'll try it out over the next day or so, and I'll follow up if there are any issues. Otherwise, thanks a bunch!