Errors from the redis library are ignored

[olivierkaisin]

Hi!

We are currently using the library and we experience critical issues with it. Sometimes , due to network errors, our redis server becomes unreachable for a small amount of time, creating errors such as:

17 Dec 15:44:57 - Error: Redis connection to redisx.mdx2mz.0001.use1.cache.amazonaws.com:6379 failed - connect ETIMEDOUT
    at RedisClient.on_error (/var/www/app/node_modules/redis/index.js:189:24)
    at Socket.<anonymous> (/var/www/app/node_modules/redis/index.js:95:14)
    at Socket.EventEmitter.emit (events.js:95:17)
    at net.js:441:14

After some investigation we realised that most errors from the redis library are ignored and that there's no handler for the default error event of both this._redis and this._subscriber.

Also, in most callbacks, the error event is ignored, being an open door to unhandled exceptions.

i.e. In the following, when error is defined, clients isn't:

var notify = function(error, clients) {
  clients.forEach(function(clientId) {
    var queue = self._ns + '/clients/' + clientId + '/messages';

    self._server.debug('Queueing for client ?: ?', clientId, message);
    self._redis.rpush(queue, jsonMessage);
    self._redis.publish(self._messageChannel, clientId);

    self.clientExists(clientId, function(exists) {
      if (!exists) self._redis.del(queue);
    });
  });
};

Throwing TypeError: Cannot call method 'forEach' of undefined.

Hope we can help to get this fixed. Thanks!

[banks]

+1 this is preventing this being used in production for me. Myspace's sharded version seems to work better and supports unsharded use anyway so I will try that for now...

[fatso83]

For anyone encountering this problem, and looking at the countless other forks that are out there, you should simply check out Gitter's fork (which is the PR in which #13 is based). The fork is often updated and regularly pulls in changes from this original branch as well.

This is how to set it up

// attach listeners for the various redis events that might arise, like 'error','end','connect','reconnecting'
function attachListeners (redisClientInstance, clientName) {
    redisClientInstance.on('error',function onRedisError (error) {
        logger.error(util.format('Redis (%s) reported error: %s', clientName, error.message));
    });
    // other events ...
});

redisClient = redis.createClient(redisConfig.port, redisConfig.host, redisConfig);
subscriberRedisClient = redis.createClient(redisConfig.port, redisConfig.host, redisConfig);

attachListeners(redisClient, 'faye redis client');
attachListeners(subscriberRedisClient, 'faye redis subscriber client');

var engineConfig = {
    type             : FayeRedisEngine,
    client           : redisClient,
    subscriberClient : subscriberRedisClient,
};

var nodeAdapterOptions = {
    mount   : config.faye.mount,
    timeout : config.faye.timeout,
    engine  : engineConfig
};

var bayeux = new faye.NodeAdapter(nodeAdapterOptions);

[dynajoe]

@fatso83 Is the fork you mentioned still a viable option? Unhandled exceptions due to Redis connectivity in our node process has been a real pain.

@jcoglan Would you be willing to accept PR's to address this issue?

[fatso83]

@joeandaverde, would not know. The GitterHq fork was fine until I left the project I was using it in a year ago. The PR for this is still sitting idle, so I think you have the answer to your second question :-)