Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Revert "Merge pull request #31447 from fatkodima/redis_cache-connecti…
…on_pool"

This reverts commit ac74e2c, reversing
changes made to ffdb061.
  • Loading branch information
georgeclaghorn committed Jan 31, 2018
1 parent 8a05dff commit 148d007
Show file tree
Hide file tree
Showing 5 changed files with 24 additions and 104 deletions.
2 changes: 1 addition & 1 deletion Gemfile
Expand Up @@ -52,7 +52,7 @@ end
gem "dalli", ">= 2.2.1"
gem "listen", ">= 3.0.5", "< 3.2", require: false
gem "libxml-ruby", platforms: :ruby
gem "connection_pool", require: false
gem "connection_pool"

# for railties app_generator_test
gem "bootsnap", ">= 1.1.0", require: false
Expand Down
17 changes: 0 additions & 17 deletions activesupport/lib/active_support/cache.rb
Expand Up @@ -160,23 +160,6 @@ class Store
attr_reader :silence, :options
alias :silence? :silence

class << self
private
def retrieve_pool_options(options)
{}.tap do |pool_options|
pool_options[:size] = options[:pool_size] if options[:pool_size]
pool_options[:timeout] = options[:pool_timeout] if options[:pool_timeout]
end
end

def ensure_connection_pool_added!
require "connection_pool"
rescue LoadError => e
$stderr.puts "You don't have connection_pool installed in your application. Please add it to your Gemfile and run bundle install"
raise e
end
end

# Creates a new cache. The options will be passed to any write method calls
# except for <tt>:namespace</tt> which can be used to set the global
# namespace for the cache.
Expand Down
13 changes: 11 additions & 2 deletions activesupport/lib/active_support/cache/mem_cache_store.rb
Expand Up @@ -63,12 +63,21 @@ def self.build_mem_cache(*addresses) # :nodoc:
addresses = addresses.flatten
options = addresses.extract_options!
addresses = ["localhost:11211"] if addresses.empty?
pool_options = retrieve_pool_options(options)

pool_options = {}
pool_options[:size] = options[:pool_size] if options[:pool_size]
pool_options[:timeout] = options[:pool_timeout] if options[:pool_timeout]

if pool_options.empty?
Dalli::Client.new(addresses, options)
else
ensure_connection_pool_added!
begin
require "connection_pool"
rescue LoadError => e
$stderr.puts "You don't have connection_pool installed in your application. Please add it to your Gemfile and run bundle install"
raise e
end

ConnectionPool.new(pool_options) { Dalli::Client.new(addresses, options.merge(threadsafe: false)) }
end
end
Expand Down
64 changes: 12 additions & 52 deletions activesupport/lib/active_support/cache/redis_cache_store.rb
Expand Up @@ -20,31 +20,6 @@

module ActiveSupport
module Cache
module ConnectionPoolLike
def with
yield self
end
end

::Redis.include(ConnectionPoolLike)

class RedisDistributedWithConnectionPool < ::Redis::Distributed
def add_node(options)
pool_options = {}
pool_options[:size] = options[:pool_size] if options[:pool_size]
pool_options[:timeout] = options[:pool_timeout] if options[:pool_timeout]

if pool_options.empty?
super
else
options = { url: options } if options.is_a?(String)
options = @default_options.merge(options)
pool = ConnectionPool.new(pool_options) { ::Redis.new(options) }
@ring.add_node(pool)
end
end
end

# Redis cache store.
#
# Deployment note: Take care to use a *dedicated Redis cache* rather
Expand Down Expand Up @@ -147,7 +122,7 @@ def build_redis(redis: nil, url: nil, **redis_options) #:nodoc:

private
def build_redis_distributed_client(urls:, **redis_options)
RedisDistributedWithConnectionPool.new([], DEFAULT_REDIS_OPTIONS.merge(redis_options)).tap do |dist|
::Redis::Distributed.new([], DEFAULT_REDIS_OPTIONS.merge(redis_options)).tap do |dist|
urls.each { |u| dist.add_node url: u }
end
end
Expand Down Expand Up @@ -197,7 +172,7 @@ def initialize(namespace: nil, compress: true, compress_threshold: 1.kilobyte, e
end

def redis
@redis ||= wrap_in_connection_pool(self.class.build_redis(**redis_options))
@redis ||= self.class.build_redis(**redis_options)
end

def inspect
Expand Down Expand Up @@ -236,7 +211,7 @@ def delete_matched(matcher, options = nil)
instrument :delete_matched, matcher do
case matcher
when String
redis.with { |c| c.eval DELETE_GLOB_LUA, [], [namespace_key(matcher, options)] }
redis.eval DELETE_GLOB_LUA, [], [namespace_key(matcher, options)]
else
raise ArgumentError, "Only Redis glob strings are supported: #{matcher.inspect}"
end
Expand All @@ -254,7 +229,7 @@ def delete_matched(matcher, options = nil)
def increment(name, amount = 1, options = nil)
instrument :increment, name, amount: amount do
failsafe :increment do
redis.with { |c| c.incrby normalize_key(name, options), amount }
redis.incrby normalize_key(name, options), amount
end
end
end
Expand All @@ -270,7 +245,7 @@ def increment(name, amount = 1, options = nil)
def decrement(name, amount = 1, options = nil)
instrument :decrement, name, amount: amount do
failsafe :decrement do
redis.with { |c| c.decrby normalize_key(name, options), amount }
redis.decrby normalize_key(name, options), amount
end
end
end
Expand All @@ -292,7 +267,7 @@ def clear(options = nil)
if namespace = merged_options(options)[namespace]
delete_matched "*", namespace: namespace
else
redis.with { |c| c.flushdb }
redis.flushdb
end
end
end
Expand All @@ -308,21 +283,6 @@ def mset_capable? #:nodoc:
end

private
def wrap_in_connection_pool(redis_connection)
if redis_connection.is_a?(::Redis)
pool_options = self.class.send(:retrieve_pool_options, redis_options)

if pool_options.empty?
redis_connection
else
self.class.send(:ensure_connection_pool_added!)
ConnectionPool.new(pool_options) { redis_connection }
end
else
redis_connection
end
end

def set_redis_capabilities
case redis
when Redis::Distributed
Expand All @@ -338,7 +298,7 @@ def set_redis_capabilities
# Read an entry from the cache.
def read_entry(key, options = nil)
failsafe :read_entry do
deserialize_entry redis.with { |c| c.get(key) }
deserialize_entry redis.get(key)
end
end

Expand All @@ -349,7 +309,7 @@ def read_multi_mget(*names)
keys = names.map { |name| normalize_key(name, options) }

values = failsafe(:read_multi_mget, returning: {}) do
redis.with { |c| c.mget(*keys) }
redis.mget(*keys)
end

names.zip(values).each_with_object({}) do |(name, value), results|
Expand Down Expand Up @@ -381,17 +341,17 @@ def write_entry(key, entry, unless_exist: false, raw: false, expires_in: nil, ra
modifiers[:nx] = unless_exist
modifiers[:px] = (1000 * expires_in.to_f).ceil if expires_in

redis.with { |c| c.set key, value, modifiers }
redis.set key, value, modifiers
else
redis.with { |c| c.set key, value }
redis.set key, value
end
end
end

# Delete an entry from the cache.
def delete_entry(key, options)
failsafe :delete_entry, returning: false do
redis.with { |c| c.del key }
redis.del key
end
end

Expand All @@ -400,7 +360,7 @@ def write_multi_entries(entries, expires_in: nil, **options)
if entries.any?
if mset_capable? && expires_in.nil?
failsafe :write_multi_entries do
redis.with { |c| c.mapped_mset(entries) }
redis.mapped_mset(entries)
end
else
super
Expand Down
32 changes: 0 additions & 32 deletions activesupport/test/cache/stores/redis_cache_store_test.rb
Expand Up @@ -8,18 +8,6 @@
module ActiveSupport::Cache::RedisCacheStoreTests
DRIVER = %w[ ruby hiredis ].include?(ENV["REDIS_DRIVER"]) ? ENV["REDIS_DRIVER"] : "hiredis"

# Emulates a latency on Redis's back-end for the key latency to facilitate
# connection pool testing.
class SlowRedis < Redis
def get(key, options = {})
if key =~ /latency/
sleep 3
else
super
end
end
end

class LookupTest < ActiveSupport::TestCase
test "may be looked up as :redis_cache_store" do
assert_kind_of ActiveSupport::Cache::RedisCacheStore,
Expand Down Expand Up @@ -122,26 +110,6 @@ class RedisCacheStoreCommonBehaviorTest < StoreTest
include AutoloadingCacheBehavior
end

class RedisCacheStoreConnectionPoolBehaviourTest < StoreTest
include ConnectionPoolBehavior

private

def store
:redis_cache_store
end

def emulating_latency
old_redis = Object.send(:remove_const, :Redis)
Object.const_set(:Redis, SlowRedis)

yield
ensure
Object.send(:remove_const, :Redis)
Object.const_set(:Redis, old_redis)
end
end

# Separate test class so we can omit the namespace which causes expected,
# appropriate complaints about incompatible string encodings.
class KeyEncodingSafetyTest < StoreTest
Expand Down

3 comments on commit 148d007

@sobrinho
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for reverting this?

@kamipo
Copy link
Member

@kamipo kamipo commented on 148d007 Feb 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sobrinho
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! ❤️

Please sign in to comment.