.acquire() doesn't reject when resource creation fails

[alexpenev-s]

Hi,
Consider this code:

var genericPool = require('generic-pool');
const factory = {
    create: function(){  return Promise.reject(new Error('cannot create resource'))}
    ,destroy:  function () { return Promise.resolve() }
}

var myPool = genericPool.createPool(factory, {})
myPool.acquire().then(function() {console.log('ok')}).catch((err)=>{console.log(err)})

Here I get an infinite loop of _dispense() calling _createResource calling _dispense()
How do I get myPool.acquire() to fail?

[sandfox]

Hi!

very short answer - some of this is "known problem": if every promise returned byfactory.create "rejects" then the Pool will just spin round and round....
I'm looking at adding some sort of backoff/retry options to the library, but in the meantime if you require retry/backoff logic it has to sit inside your factory.create function. There is an example of doing this here -> https://github.com/pasupulaphani/node-redis-connection-pool/blob/master/lib/redis_pool.js#L55

It is possible to get your myPool.acquire() to fail/reject by setting opts.acquireTimeoutMillis.

from the docs:

• acquireTimeoutMillis: max milliseconds an acquire call will wait for a resource before timing out. (default no limit), if supplied should non-zero positive integer.

const genericPool = require('generic-pool');
const factory = {
    create: function(){  return Promise.reject(new Error('cannot create resource'))}
    ,destroy:  function () { return Promise.resolve() }
}

const opts = {
  acquireTimeoutMillis: 10000
}

const myPool = genericPool.createPool(factory, opts)

myPool.acquire().then(function() {console.log('ok')}).catch((err)=>{console.log(err)})

I hope this helps.

[alexpenev-s]

The example doesn't work. It loops until forever. Isn't acquireTimeoutMillis for a single request that takes over that much time.

Something like: maxAcquireRetries would be very useful.

[sandfox]

hmmm.. I haven't actually tested it myself... (the retry logic bit). I'll try to put some demo together and see whats happening with it (although if you have any code you paste her or in a gist that would be very helpful.)

acquireTimeoutMillis just puts a hard limit on how long you are prepared to wait for the pool to give you a resource, I admit it's a bit of hack for your problem. If your pool has no "min size" and your acquire call times out then the pool should stop trying to create a resource because there is no longer any demand for a resource.

The design of the pool is such that factory errors are isolated away from pool.acquire calls. Any rejected promises from pool.create are emitted by the pool.

pool.on('factoryCreateError`, (err)=>{})

which could be used to detect constant failures and drain/stop the pool (again this a little bit of hack and requires a more code)

[alexpenev-s]

Here is the example with acquireTimeoutMillis (set to 1 sec):
https://gist.github.com/anonymous/0b8c3c5fbd2b5ce749541f96af2fefc1

Note that even after 1 second it still calls _createResource ()

[sandfox]

@alexpenev-s thanks! I'll try and take a look tonight.

[sandfox]

Ok, I forked your gist a modified it a bit so I could keep out with the output :-) and ran it. Once the acquire call timeouts, no more calls are made to factory.create but there still be a single outstanding promise internally that resolve/rejects after the acquire call rejects. After that it stops trying to to create new resources (unless there are further acquire calls queued up).

I'm still not sure what the best way to handle constantly failing factory.create calls. I suspect it's going to involve some kind of circuit breaker pattern and I don't know if thats best living inside the pool or as a bolt-on. I'll probably start by implementing it as an external library and see what extra APIs I need to add to the generic-pool.

Incase you are interested my vague idea involved counting the factoryCreateError events in a leaky bucket and if the count exceeds some high water mark, take some kind of action (crash / pause / whatever)

[whiteatom]

Hi all,
Totally to all this, but I believe my problem is related. I'm pooling mysql connections, and with generic-pool v2 it worked perfectly, trying to upgrade to v3 (using promises, which I don't fully understand) and it works great until I start testing errors. Set the mysql password to be wrong and I get infinite MySQL errors; set the timeout as suggested above and I get and UnhandledPromiseRejectionWarning, but my error catching is never triggered.

Would one of you be able to take a quick look at my Stackoverflow post?

http://stackoverflow.com/questions/41902030/node-js-connection-pool-produces-infinite-loop-on-error

[sandfox]

hey @whiteatom - yep you've hit "known problem" land...

for some history...
In v2 a given call to pool.acquire was directly tied to a single call to factory.create and an error in the factory.create would bubble through to the pool.acquire. (this was generally bad ™️ )
In v3 calls to pool.acquire were no longed tied to calls to factory.create and therefore any errors in factory.create could not bubble up to pool.acquire calls because it didn't make any semantic sense. Instead factory errors are now exposed via the Pool itself via event emitters

So to sum up slightly: the promise returned by pool.acquire will only reject due to errors specifically related to the acquire call (such as timeout), and not because of any other errors in the pool. To catch general errors with the pool you will need to attach some event listeners to the Pool instance you have.

There is still the problem that it's possible for the pool to end up in some infinite loop if your factory.create returns promises that only ever reject. To get around this you can build backoff functionality into your factory.create function (this is a bit of a hack though, and I really need to find some way to put backoff in the pool itself.)

[osm-vishnukyatannawar]

This is an issue for us now as well ... if MariaDB fails, our create factory is being called multiple times making the server become un-responsive.

[SamuelBrucksch]

We have something like this as well. We reject, if the connection fails and until now we expected, that the reject would be given back to the acquire function, however it is caught and we never get the reject which is hard to deal with from our code as we expect a reject in error case. However instead it tries to reconnect all the time. Is there a way to somehow disable the event emitter so that the reject is returned instead of being caught?

[restjohn]

When I encountered this problem, my first attempted solution was to drain() and clear() the pool from a factoryCreateError event handler. When that didn't work, I inspected the code and found that neither of those methods removes any clients from the _waitingClientsQueue or rejects any outstanding requests, but instead continues to call _factory.create() indefinitely.

It seems what's necessary is some API to cancel and reject all waiting client acquisition requests. This could be something like Pool.stop(), complementary to Pool.start(). Alternatively, or maybe additionally, there could exist some feedback from the factory create() method to the pool that it has encountered an unrecoverable error and no potential exists to satisfy any further create() requests.

[dhensby]

When I encountered this problem, my first attempted solution was to drain() and clear() the pool from a factoryCreateError event handler. When that didn't work, I inspected the code and found that neither of those methods removes any clients from the _waitingClientsQueue or rejects any outstanding requests, but instead continues to call _factory.create() indefinitely.

Yes, this is exactly what I've found. I opened #256 to track that

I don't see that a Pool.stop() function is needed to solve this problem, the drain / dispense logic just needs to deal with a draining pool better

[sandfox]

There currently isn't any way to "ungracefully" stop the pool. And there isn't any way to dump both the waiting clients. I could probably envisage adding a method that would:

• stop any new acquire calls
• reject all waiting clients
• destroy any resources as they returned

I'm not sure if the pool would wait for borrowed resources to be returned, or just abandon them.

[dhensby]

My view (having not thought it through particularly thoroughly) would be that if you want to close the pool then any resources that haven't been released (or created) should no be released after drain is called, even if they are in the process of being created.

If the pool is closed a resource should never be released

[radarfox]

Following solution originaly posted here by ImHype works for me like a charm.

const factory = {
  create: () => Promise.reject(new Error()),
  destroy: () => {}
};
const pool = genericPool.createPool(factory);
pool.on('factoryCreateError', (error) => {
  pool._waitingClientsQueue.dequeue()
  .reject(error);
});
pool.acquire()
.then(console.log, console.error);