factoryCreateError is not being triggered on create failure.

[pasupulaphani]

Here is sample code.

  const factory = {
    create: function () {
      return new Promise(function (resolve, reject) {
        const client = redis.createClient(redisOptions);

        client.on("error", err => {
          debug("Redis pool: ", name, err);
          reject(err);
        });
        resolve(client);
      })
    },
    destroy: function () { return Promise.resolve() }
  }

  const pool = createPool(factory)
  
  pool.on("factoryCreateError", e => {
    debug("factoryCreateError", e);
  });

  pool.acquire()

Any help is appreciated.

[sandfox]

This might not be related, but in your example, the factory.create function returns a promise that never calls resolve anywhere...

[pasupulaphani]

@sandfox Made a mistake while copying. Added it. No difference though.

[sandfox]

ok, in the modified example above, once resolve is called in that promise , calling reject will do nothing. A Promise can only be resolved or rejected once, any calls after the first reject/resolve are basically noops.

[pasupulaphani]

@sandfox you are right. Thank you.

[pasupulaphani]

@sandfox in the event of connection failure. It appears factoryCreateError is called continuously leading to infinite loop.

[sandfox]

yep, this is a current limitation of the pool. It has no backoff for failed factory calls, so it will as soon as one create promise rejects it will just try again . I have vague plans to add it in, but in the meantime it's possible to add a back-off/retry mechanism as a wrapper around your factory.create function using something like retry-as-promised

[pasupulaphani]

@sandfox No luck with retry.

[sandfox]

Hmmm ok, let me see if can knock up something that works...

[pasupulaphani]

I can see where this is happening. Shall I send u a PR if it helps.

[sandfox]

PR's or even just gists/comments are welcome.

I created this snippet

'use strict'

const genericPool = require('generic-pool')

let createAttempts = 0

const factory = {
  create: function () {
    createAttempts++
    if(createAttempts < 10){
      return Promise.reject(new Error(`failing on attempt ${createAttempts}`))
    }
    return Promise.resolve({name:'thingy'})
  },
  destroy: function () { return Promise.resolve() }
}

const pool = genericPool.createPool(factory)

pool.on("factoryCreateError", e => {
  console.log("factoryCreateError", e);
});

pool.acquire().then((resource)=>{
  console.log('got a resource', resource)
}).catch((err)=>{
  console.log('could not get a resource')
})

running this outputs...

factoryCreateError [Error: failing on attempt 1]
factoryCreateError [Error: failing on attempt 2]
factoryCreateError [Error: failing on attempt 3]
factoryCreateError [Error: failing on attempt 4]
factoryCreateError [Error: failing on attempt 5]
factoryCreateError [Error: failing on attempt 6]
factoryCreateError [Error: failing on attempt 7]
factoryCreateError [Error: failing on attempt 8]
factoryCreateError [Error: failing on attempt 9]
got a resource { name: 'thingy' }

so as long as your factory does eventually return a resource successfully, things will be ok. If it always fails though it will just loop for ever...

[pasupulaphani]

@sandfox yes, only happens on failure and hangs app.

[sandfox]

I've taken the above example and modified it so that is uses retry to at least slow down the failure rate.

'use strict'

const genericPool = require('generic-pool')
const retry = require('retry-as-promised')

let createAttempts = 0

const retryLog = function(msg){
  console.log(msg)
}

const create = function () {
  createAttempts++
  if(createAttempts < 5){
    return Promise.reject(new Error(`failing on attempt ${createAttempts}`))
  }
  return Promise.resolve({name:'thingy'})
}

const factory = {
  create: function () {
     return retry(create, {backoffBase: 1000, max: 10,name: 'factory.create', report:retryLog})
  },
  destroy: function () { return Promise.resolve() }
}



const pool = genericPool.createPool(factory)

pool.on("factoryCreateError", e => {
  console.log("factoryCreateError", e);
});

pool.acquire().then((resource)=>{
  console.log('got a resource', resource)
}).catch((err)=>{
  console.log('could not get a resource')
})

outputs

Trying factory.create #1 at 12:20:55 AM
Try factory.create #1 failed: Error: failing on attempt 1
Delaying retry of factory.create by 1995.262314968881
Trying factory.create #2 at 12:20:57 AM
Try factory.create #2 failed: Error: failing on attempt 2
Delaying retry of factory.create by 4265.795188015933
Trying factory.create #3 at 12:21:01 AM
Try factory.create #3 failed: Error: failing on attempt 3
Delaying retry of factory.create by 9840.11105761136
Trying factory.create #4 at 12:21:11 AM
Try factory.create #4 failed: Error: failing on attempt 4
Delaying retry of factory.create by 24677.434053558605
Trying factory.create #5 at 12:21:36 AM
got a resource { name: 'thingy' }

[pasupulaphani]

I've published this with retry support. May be you can use this as reference to others who are having similar issues. https://github.com/pasupulaphani/simple-redis-pool/blob/master/lib/redis_pool.js

[pasupulaphani]

@sandfox thank you for your help so far.

A small issue, with retry the factoryCreateError events are not fired anymore. Caller is not able to know if conn has failed.

[sushantdhiman]

+1, basically when pool is creating connections which are bound to be invalid such as incorrect username, password, errors are not propagated back to caller. These connection with invalid config can't be retried in hope that they would eventually connect. This creates an infinite loop and pool keeps on calling create and exhaust all memory.

This issue is blocking Sequelize to bump its generic-pool version :)

[sushantdhiman]

Studied the code a bit, I think createResource in _dispense OR dispatchResource in _dispense call should report failures to awaiting resourcePromises. Upon createFailure awaiting resourcePromise queue is never notified, so the acquire call promise never resolves/rejects.

I think we should reject all awaiting promises in resourcePromise queue when create call fails. What do you think @sandfox , would that be an acceptable solution ?

[sandfox]

@pasupulaphani - I'll be sure to point people to that as example. As for the errors not bubbling up to the Pool, yep, definitely a problem/annoyance. If the function at factory.create call fails too many times and retry gives up on it, you still see a rejection, but you are losing sight/visibility on all the retry attempts unless you attach your own .catch or logging code to it yourself, which is kinda ugly/complex. I definitely want to put retry logic in the Pool but I want to come up with a suitabley generic way to configure / override it as people tend to have varying requirements of it.

@sushantdhiman - in response to your 2nd comment: something to bear in mind is that the Pool doesn't know the difference between temporary failures that may "fix" themselves at some point and problems which can only be fixed changing the code/config of the application. Rejecting all awaiting calls just because there was something like a laggy network causing db.connect to fail temporarily would be overly aggressive. Only the user (or their code) can make that kind of decision, and I'm not sure there is any good way to put that logic in the pool, so I feel it's best to expose enough information from the pool to allow the user to build something suitable. Infact thinking about it, we're are heading into "circuit breaker" territory, e.g Netflix hystrix.

That all said I'm sure that there are various improvements to the Pool that could be made to make make userland code for this easier.

in response to your first comment: agreed, the only hacky workaround is to somehow expose the errors that retry is swallowing to some other part of your app so that it can detect the permanent failures and take some action. I'm going to give this some more thought and see if I can come up with either a less hacky workaround or a good retry config.

@sushantdhiman do you have a link to the issue in Sequelize ?

Thanks both for your input on this!

[sushantdhiman]

do you have a link to the issue in Sequelize ?

Its just a blocked PR https://github.com/sequelize/sequelize/pull/6777 , I was able to refactor our codebased to use new pool (locally) but some tests for invalid config were failing, which lead me to this thread :)

I think you can retry a few time before reporting the error (via promise queue cancellation), but acquire calls should be rejected after some time, they should not hang forever.

[sandfox]

@sushantdhiman thanks for the link. Pool 3 has a new config option acquireTimeoutMillis - which if set, will cause an acquire call to reject after the timeout period passes. Would this help at all (at least in the short-term)?

[sushantdhiman]

Hi @sandfox , Sorry for this late reply, acquireTimeoutMills couldn't help either because it will throw different error than original error.

I was able to fix my issue with watching for factoryCreate error using timers. I would like to suggest something. If factoryCreateError is thrown _dispence calls _createResource so many time that server might crash.

It would be helpful to wrap _createResource call in process.nextTick and call it out side of loop so you create resources one by one