Add option to error if the pool is full #125

kevinburkeshyp · 2016-02-16T09:20:42Z

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 · 2016-02-16T10:26:33Z

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 · 2016-02-16T22:26:41Z

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 · 2016-02-17T02:17:43Z

Thanks, @sandfox!

Currently if you call pg.connect(), the call will block indefinitely until a connection becomes available. In many cases, if a connection is not available after some period of time, it's preferable to return an error (and call control) to the client, instead of tying up resources forever. Blocking on resource checkout also makes it easier for clients to deadlock - recently at Shyp, we had a situation where a row got locked and the thread that could unlock it was blocked waiting for a connection to become available, leading to deadlock. In that situation, it would be better to abort the checkout, which would have errored, but also broken the deadlock. Add two new settings to defaults: `block`, which, if false, will immediately return an error if the pool is full when you attempt to check out a new connection. Also adds `acquireTimeout`, which will wait for `acquireTimeout` milliseconds before giving up and returning an error. This builds on two pull requests against `generic-pool`: - Support options.block: coopernurse/node-pool#125 - Support options.timeout: coopernurse/node-pool#127 For the moment I'm pointing `generic-pool` at a branch that incorporates both of these commits. I'm marking this as a proof-of-concept until those go through, which hopefully they will soon. I'd also like feedback on the API. Adds semicolons in many places that omitted them and fixes several typos. I'm happy to pull those out into a different commit. Sets the TZ=GMT environment variable before running the tests; without this value set, and with a Postgres server set to the America/Los_Angeles timezone, a timezone test failed. Fixes brianc#782 and brianc#805. Will help alleviate brianc#902. May help with brianc#397.

kevinburkeshyp · 2016-02-18T02:36:22Z

lib/generic-pool.js

  if (this._draining) {
    throw new Error('pool is draining and cannot accept work')
  }
+  if (options !== null && typeof options !== "undefined" && options.block === false && this._count >= this._factory.max) {


This needs to be _total, not _count. My fault. I'll add a test for this fix it tomorrow PST.

kevinburkeshyp · 2016-02-18T17:19:25Z

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 · 2016-02-19T16:15:32Z

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 · 2016-02-20T17:44:55Z

@brianc proposed making these two options a single property: brianc/node-postgres#939 (comment)

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.

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. Adds a helper function pool._isFull, which checks both the _count and the list of _availableObjects - I think we have to check both to ensure that the pool is full, and that this abstraction doesn't exist yet. 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](https://shyp.com).

sandfox · 2016-02-23T22:06:03Z

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 · 2016-02-25T07:23:58Z

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: sinonjs/sinon#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?

coopernurse#125 (comment)

kevinburkeshyp · 2016-03-17T19:25:51Z

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

This was referenced Feb 17, 2016

Support timing out acquire() call #127

Closed

how to set timeout for resource acquiring, NOT resource creating? #85

Closed

kevinburkeshyp mentioned this pull request Feb 17, 2016

POC: Time out pg.connect() call brianc/node-postgres#939

Closed

kevinburkeshyp reviewed Feb 18, 2016
View reviewed changes

kevinburkeshyp force-pushed the block-false branch from 4cb05b2 to 3d6d8ca Compare February 18, 2016 17:18

sandfox added this to the 2.5.0 milestone Feb 20, 2016

kevinburkeshyp force-pushed the block-false branch from 3d6d8ca to 6d1dd23 Compare February 20, 2016 19:02

sandfox mentioned this pull request Feb 24, 2016

Question: What is the expected behavior when you try to acquire a connection exceeding the max number? #130

Closed

kevinburkeshyp pushed a commit to Shyp/node-pool that referenced this pull request Feb 27, 2016

Call back with the error in the next tick

2c40ea8

coopernurse#125 (comment)

kevinburkeshyp closed this Mar 17, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to error if the pool is full #125

Add option to error if the pool is full #125

kevinburkeshyp commented Feb 16, 2016

sandfox commented Feb 16, 2016

sandfox commented Feb 16, 2016

kevinburkeshyp commented Feb 17, 2016

kevinburkeshyp Feb 18, 2016

kevinburkeshyp commented Feb 18, 2016

ensonik commented Feb 19, 2016

kevinburkeshyp commented Feb 20, 2016

sandfox commented Feb 23, 2016

kevinburkeshyp commented Feb 25, 2016

kevinburkeshyp commented Mar 17, 2016

Add option to error if the pool is full #125

Add option to error if the pool is full #125

Conversation

kevinburkeshyp commented Feb 16, 2016

sandfox commented Feb 16, 2016

sandfox commented Feb 16, 2016

kevinburkeshyp commented Feb 17, 2016

kevinburkeshyp Feb 18, 2016

Choose a reason for hiding this comment

kevinburkeshyp commented Feb 18, 2016

ensonik commented Feb 19, 2016

kevinburkeshyp commented Feb 20, 2016

sandfox commented Feb 23, 2016

kevinburkeshyp commented Feb 25, 2016

kevinburkeshyp commented Mar 17, 2016