Node-redis: returning Error from retry_strategy does not emit on(error) event

[crunchie84]

• Version: node 4.2.1 , redis 2.6.2
• Platform: Mac OS 10.11.5
• Description:

We had implemented a retry_strategy which emits an error after a certain amount of retries. This would then bubble to the on('error') handler of Redis to be logged and exit the application. When updating our redis library from 2.4.2 to 2.6.2 we noticed that this behaviour is no longer the same.

Repro case:

const redis = require('redis');
const MAX_ATTEMPTS = 10;
var client = redis.createClient(port, host, { 
  retry_strategy: redisRetryStrategy
});
client.on('error', onError);

function OnError(err) {
  console.log(err);
  throw new Error('SHUTDOWN THE APP, REDIS IS NOT RESPONDING??');
}

function redisRetryStrategy(host, options) {
  if (options.attempt >= MAX_ATTEMPTS) {
    // Stop reconnecting after reaching the maximum number of attempts
    return new Error('Maximum redis reconnect attempts reached');
  }
  return 1000; // Schedule next reconnection attempt after 1 sec.
}

[BridgeAR]

This is actually something that was done on purpose. The reason is that you already have the information that the client failed, so it does not have to be emitted anymore. This is indeed not a good change about keeping semver up :/ I'm sorry about that.

I would like to have a small poll to see what other people are thinking. Ping @NodeRedis/contributors @dirkbonhomme

[BridgeAR]

Yes: Bubble up the error to the error listener

[BridgeAR]

No: Keep the current solution

[drew-r]

Just to offer a use case in favour of the former, connect-redis provides a logErrors option which becomes (non-obviously) mutually exclusive with retry_strategy if errors don't bubble up.

new RedisStore({
  logErrors: (err) => console.error, // does nothing
  retry_strategy: (options) => {
    if (options.error.code === 'ECONNREFUSED' || options.error.code === 'ENOTFOUND') {
      return new Error('The server could not be found or refused the connection');
    }
    return 1000;
  }
})

@tj

[raijinsetsu]

I think the current functionality is fine as long as it's in the documentation, which it isn't. I think it would also be acceptable to add an option that would still bubble up errors returned from the retry strategy.
Ex.:

redis.createClient({
    retry_strategy: ...,
    retry_strategy_bubble_up: true or false (default to false to keep existing functionality)
});

Just spent the last day trying to figure out why errors weren't being logged. Now that the logging is in the retry_strategy callback, everything is as I want it in my application.

[alannesta]

Instead of throwing the error, I am manually emitting the error event which could be caught by the error handler:

retry_strategy: function(options) {
            logger.debug('Retry redis connection: ', options.attempt);

            // 5 mins
            if (options.total_retry_time > 1000 * 60 * 5) {
                // End reconnecting after a specific timeout and flush all commands with a individual error
                return logger.error('Redis: maximum retry time exhausted');
            }
            if (options.attempt > 5) {
                // End reconnecting with built in error
                logger.error('Redis: maximum connection retry reached');
                return client.emit('error', 'maximum retry');
            }
            return 1000 * 2;
        },

[AaronJan]

Bubble up ! (or something else)

Custom error emitter like @alannesta wrote is messy, retry_strategy is one of the constructor's parameters, it shouldn't accessing object that outside the function.

NodeRedis already had an event system, just use it!

Since end is been taken, and it means "connection is closed", we can add a new event (let's just call it "shutdown" for now) that indicating "we did everything we could, this connection is dead, from now on, we do nothing about it, you better do some clean up jobs".

I really need this change.

[lp-belliot]

Did anything happen with this issue?

[Tapppi]

Is there a reason this is not fixed or just priority? What is the benefit of not emitting the event? There is no obvious way to shut down a service with no more working redis connection when using retry_strategy and that should be a pretty basic use case. The best solution though would probably be a separate event as @AaronJan suggested..

[Salakar]

I've released v3.0.0 which now bubbles any returned retry_strategy errors up to the error event listener.