Failsafe on Redis error replies in RedisCacheStoreProxy.

[cristiangreco]

RedisCacheStoreProxy will blow up when RedisCacheStore raises a
CommandError exception. In fact, by default the proxied store only
handles BaseConnectionError exceptions, but will let bubble up any other
type of exception from the underlying client.

This pull request uses the same approach from RedisProxy, where store
operations are wrapped in a rescuing block that rescues and ignores
BaseError exceptions (the most generic exception class that can be
raised by the Redis client).

Best viewed ignoring whitespaces.

[grzuy]

Thank you!

To be consistent I guess you would also want to rescue #read, right?

[cristiangreco]

@grzuy thanks for reviewing this PR and sorry for coming back to you so late!

To be consistent I guess you would also want to rescue #read, right?

Not sure what you exactly mean here? read is not reimplemented by RedisCacheStoreProxy, it's only used within increment which is already wrapped in rescuing block itself. Or did I misunderstood your suggestion?

[grzuy]

Hi @cristiangreco,

Sorry for my short and vague comment :-P

So, rack-attack will eventually call #read, #write and #increment on whichever cache store you configure.

Whether or not those methods are re-implemented on any particular proxy doesn't mean they aren't going to be called.

In particular, for the RedisCacheStoreProxy, #read is not re-implemented because it wasn't necessary up until now, but it is still being called and delegated to ActiveSupport::Cache::RedisCacheStore#read.

If you don't re-implement an rescue #read you will continue to get Redis::BaseError for those #read calls.

[cristiangreco]

@grzuy thanks for the thorough explanation! I've force pushed a change to wrap read operations as well.

[grzuy]

Thank you for the update!

[grzuy]

I think we are overriding ActiveSupport::Cache::Store#read(name, options = nil), which potentially can receive two arguments.

What about going with the following?

def read(*_args)
  rescuing { super }
end

So that we allow options argument, plus we make it forward compatible with any arity changes to the original #read?

[cristiangreco]

That looks sensible, I've updated it!

[grzuy]

Thank you!