redis client does not have an error handler

[jeffmcaffer]

If the user passes in redis options (rather than an actual client instance), this code makes a new client. That's great but it needs to have an .on('error', ...) handler. Without it, any errors in the redis client will immediately crash the app. Even something as simple as the following would do the trick.

    redisClient.on('error', error => console.log(`Redis client error: ${error}`));

[dlebech]

Thanks for reporting this, @jeffmcaffer. I think there might be three ways of approaching this:

1. Returning the client instance so the caller can decide how to do error handling.
2. Include an error-handling configuration option.
3. Make a catch-all handler like you propose.

I'm not a huge fan of either 2. or 3., but that's probably also because we actually "crash-by-design" in our app when something low-level like a connection goes down. As far as I can tell, a connection issue is going to be the most common scenario for error handling here, as each individual library call has its own error handling.

If we only do 1, it should be made clear that the library doesn't handle errors of course.

What do you think?

[jeffmcaffer]

That seems reasonable. People can also construct up their own client and pass it in (that's what we do now). I mostly opened this because I didn't foresee the crash-by-design scenario.

On a slightly different front, what is the need for the passed in client to be an actual instance of redis.RedisClient? Forcing the use of that class restricts us from using ioredis or mocking up a redis client with plain old objects. Again, not a huge deal, more curious than anything.

Thanks for the library. We are making good use of it in GHCrawler

[dlebech]

Thanks for the feedback. Would it work for you then, if we made it explicit that low-level errors (as reported by the client itself) are not being handled specifically by this library?

Regarding your second note: To be honest, the ability to re-use a client was an afterthought and micro-optimization (we wanted to re-use an existing connection, basically), and we just based it on the redis library since that's what we were already using in both places (our app and redis-metrics). The specific library calls also fit the redis client. That being said, I've opened this issue. It would be interesting to at least test the library against ioredis and just see what happens. It looks like the function calls are pretty much the same. I might be able to take a look at this later this week.

I'm really happy to hear that this library is being used by others libraries! It's a good motivation to take a look at this lib again and perhaps update it to ES6 and Node 4+ only support, including native promises. I see you are using ES6 in your library, so I imagine an update like that would work without hiccups as well (It would still be a new major version, of course).

[jeffmcaffer]

For our case we create a redis client and supply that so we are not blocked/affected now. You could make it clear in the doc that folks should either, pass in their own, get the one you make and add an error handler or crash-by-design.

For different redis libraries, we were considering moving to ioredis for various reasons (mostly some of the auto reconnect behavior). It was not a major issue but one of the things that stopped that move was this library's hard dependency on the specific redis lib. It did appear that they would be compatible but we also did not spend a long time on it.

Thanks for engaging here.