Add option to error if the pool is full

[kevinburkeshyp]

Currently, if you attempt to acquire a resource and the pool is full, the callback will not run until a space becomes available. This can lead to a deadlock if a pool member's release is blocked on the acquire callback running.

To mitigate the deadlock possibility, support a third argument pool.acquire(cb, priority, {block: false}), which will immediately hit the callback with an error if the pool is full. Adds a test that this behaves as expected.

Callers may want to wait for a small amount of time before giving up on the pool; I'm going to add that option in a second commit.

This work was sponsored by Shyp.

[sandfox]

Hi there Kevin.

I've currently got a half finished/started branch on my laptop that adds optional support for an acquire timeout, and allows for limiting the size of the waiting resource request list. (this is configured at pool creation time).

The basic idea is that each call to pool.acquire will check the request queue length then cb(err) immediately if the 'wait list' is full, else create a 'resource request' that either gets fulfilled or times-out.

[sandfox]

I totally didn't see the actual code that came with this PR!!! So sorry, let me have a look and I'll get back to you.

[kevinburkeshyp]

Thanks, @sandfox!

[kevinburkeshyp]

Ok - I think I pushed the right check - if I am reading correctly, _count is the list of objects which have been allocated (some of which may be available) and _availableObjects is the list of those available objects. 3d6d8ca includes a new test and a new _isFull helper.

[ensonik]

Thanks for this @kevinburkeshyp. Having no timeout is troublesome at best. I was not looking forward at the prospect of having to do this. Hopefully the chain of PR's you've prepped will go through quickly (for the better good)

[kevinburkeshyp]

@brianc proposed making these two options a single property: https://github.com/brianc/node-postgres/pull/939#discussion_r53528524

Also, I need to look at the test failure, there's likely a race somewhere.

Previously I was running into an issue where timers on node v0.10.30 were firing 50ms early, but I don't think this failure is that.

[sandfox]

Ok this looks pretty good with one small change needed:

One line 395

callback(new Error('Cannot acquire connection because the pool is full'))

this callback needs wrapping in a setImmediate/process.nextTick so that the callback is fired on the next cycle round the event loop so that it maintains acquires async behaviour otherwise library user's might find their code executing in an unexpected order.

because we support node 0.8 we'll need to use/do something like https://www.npmjs.com/package/setimmediate (I'd settle for something hacky in favour of another dependency which I want to avoid at all costs (a bundled dependency as long as $ANCIENT version of npm supports it would be an ok tradeoff)

All that said....

I think I prefer the idea of making acquireTimeout === 0 do what brianc propses and ditch the 'blocking' option in the acquire call. One tradeoff is that you can no longer choose per acquire call if you want block or not, but if you really really need that behaviour it's still do-able in userland code.

It's not pretttty, but acquireTimeout === 0 also makes future upgrades easier, such as an option limiting the number of waiting requests which would compliment acquireTimeout nicely.

Sorry again for taking a while to look at this!

[kevinburkeshyp]

this callback needs wrapping in a setImmediate/process.nextTick so that the callback is fired on the next cycle round the event loop so that it maintains acquires async behaviour otherwise library user's might find their code executing in an unexpected order.

OK. Is process.nextTick okay? My experience with setImmediate is not great, see here: https://github.com/sinonjs/sinon/issues/960, plus the shim you mentioned for 0.8. I'm not amazingly familiar with the event loop, but I can read about it.

I think I prefer the idea of making acquireTimeout === 0 do what brianc propses and ditch the 'blocking' option in the acquire call. One tradeoff is that you can no longer choose per acquire call if you want block or not, but if you really really need that behaviour it's still do-able in userland code.

Fine by me. Should I squash these two PR's into one? Push here or open a new one?

[kevinburkeshyp]

Closing this, since we decided not to add a block parameter. See #127 for more details.