Connection errors are silent

[StarpTech]

Prerequisites

• I have written a descriptive issue title
• I have searched existing issues to ensure it has not already been reported

Fastify version

3.18.0

Plugin version

No response

Node.js version

14.x

Operating system

Linux

Operating system version (i.e. 20.04, 11.3, 10)

20.04

Description

Fastify-redis rely on ioredis that doesn't exit the process when the connection can't be restored.

emits when an error occurs while connecting.
However, ioredis emits all error events silently (only emits when there's at least one listener) so that your application won't crash if you're not listening to the error event.

We need to register an error handler on ioedis and graceful shutdown the application.

Steps to Reproduce

Pass an invalid redis url or shutdown the redis instance during connection. No error is thrown but the app is still alive.

Expected Behavior

If the application can't recover itself the instance must exit so that your orchestration software can't mark it as unhealthy and create a new one.

[mcollina]

Good spot!

[valerio-pizzichini]

Hi, I've encountered the same problem while working with the plugin. Can i try to give it a shot to fix it?

[mcollina]

go for it!

[valerio-pizzichini]

I was thinking about two possible solutions:

1. Handling the errors passing a callback from the fastify server like

const onErrorCallback = (err) => {
  console.log(err);
  process.exit(1);
}

fastify.register(fastifyRedis, {
  host: '127.0.0.1',
  maxRetriesPerRequest: 5,
  onError: onErrorCallback
})

2. Making an attempt to retrieve the connection status before finishing the registration so we can continue or abort if the connection to the instance is not correctly established, something like

return client.info((err, _) => {
    return err ? next(new Error(err)) : next()
  })

Have you any strong opinion on one approach or another? I could prepare both PRs to see a working examples of the approaches and then decide which would fit better.

[mcollina]

I would go with the second approach.

[StarpTech]

I'd reopen this issue. The connection is checked at server startup but when the connection is exited after retry the app will stay in an unhealthy state forever. The plugin needs to listen to the error handler and gracefully shut down the client via .quit(). Therefore in ioredis there is a ready event this might be a better candidate than .info()

If enableReadyCheck is true, client will emit ready when the server reports that it is ready to receive commands (e.g. finish loading data from disk).
Otherwise, ready will be emitted immediately right after the connect event.

Sry, I had no bandwidth to check the PR.

[StarpTech]

I have to correct myself. In order to allow reconnecting the error must not abort the process. You need to listen on the end event. I did in this way:

const maxConnectionRetry = 5;
const retryStrategy = function (times) {
	if (times >= maxConnectionRetry) {
		return null;
	}
	const delay = Math.min(Math.pow(2, times) * 50, 2000);

	return delay;
}

const redisClient = new Redis(options.url, {
  commandTimeout: 2000, // ms
  enableReadyCheck: true,
  retryStrategy,
});

this.client
.on('reconnecting', (times: number) => {
	this.loggingService.log.warn(
		`Redis reconnecting in ${times}ms`,
	);
})
.on('end', () => {
	const err = new Error('Redis max retry');
	this.loggingService.log.error(
		err,
		'redis max connection retries',
	);
    // (pseudo) will lead to graceful shutdown 
	gracefulShutdown()
})
.on('error', (err) => {
	this.loggingService.log.error(err, 'redis error');
	// we swallow errors here to run the reconnection logic
	// if the client can't connect the `end` event will fire
})
.on('ready', async () => {
	this.loggingService.log.info('Connected with redis');
});