Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

ActiveSupport::Cache::RedisCacheStore should support Redis instances for redis kwarg #32233

Conversation

@as3richa
Copy link

@as3richa as3richa commented Mar 13, 2018

Summary

Fixes a bug in #31134

ActiveSupport::Cache::RedisCacheStore should support operating upon a Redis instance given in its initializer (via the redis keyword argument). Ref: https://github.com/rails/rails/blob/master/activesupport/lib/active_support/cache/redis_cache_store.rb#L109-L116

In actual fact, because Redis.new.respond_to?(:call), RedisCacheStore will instead treat a Redis instance like a proc, calling it to populate its @redis: https://github.com/rails/rails/blob/master/activesupport/lib/active_support/cache/redis_cache_store.rb#L121-L122

Obviously this doesn't have the correct semantics. The test case added in my first commit fails with a Redis::TimeoutError (if indeed a Redis instance is running on 127.0.0.1:6379).

With the fix given in my second commit, RedisCacheStore#redis is precisely equal to the Redis instance given to the initializer, if indeed one was given.

Other Information

I'm a first-time contributor 馃榿 please let me know if I need to update any changelog or open this PR against a branch other than master.

activesupport/lib/active_support/cache/redis_cache_store.rb Outdated
if redis.respond_to?(:call)
if redis.is_a?(Redis)
redis
elsif redis.respond_to?(:call)
redis.call
elsif redis
redis

This comment has been minimized.

@as3richa

as3richa Mar 13, 2018
Author

I left this branch untouched. My rationale is that someone may want to provide an object that is workalike with Redis (ie. implementing get, set, etc. with a different backend) but that doesn't inherit from Redis. Should this branch remain?

This comment has been minimized.

@matthewd

matthewd Mar 13, 2018
Member

Hmm... the triple branch does seem a bit ugly.

If we're going to have to use an is_a? check to distinguish them, maybe we'd be better changing the existing condition to is_a?(Proc)?

This comment has been minimized.

@as3richa

as3richa Mar 13, 2018
Author

Sure, SGTM

This comment has been minimized.

@as3richa

as3richa Mar 13, 2018
Author

Pushed

@as3richa as3richa force-pushed the as3richa:as3richa/redis-cache-store-with-redis-instance branch to cf7e25d Mar 13, 2018
@jeremy jeremy added this to the 5.2.0 milestone Mar 13, 2018
jeremy added a commit that referenced this pull request Mar 13, 2018
Since `Redis#call` duck types as a Proc, we'd call `#call` on it,
thinking it's a Proc. Fixed by check for the Proc explicitly instead of
duck typing on `#call`.

Closes #32233
jeremy added a commit that referenced this pull request Mar 13, 2018
Since `Redis#call` duck types as a Proc, we'd call `#call` on it,
thinking it's a Proc. Fixed by check for the Proc explicitly instead of
duck typing on `#call`.

References #32233
@jeremy
Copy link
Member

@jeremy jeremy commented Mar 13, 2018

Thanks @as3richa! Backported to 5-2-stable @ 2ebd68b

@jeremy jeremy closed this Mar 13, 2018
@as3richa as3richa deleted the as3richa:as3richa/redis-cache-store-with-redis-instance branch Mar 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants