Merge pull request rails#53845 from joshuay03/prevent-double-wrapped-redis-connection-pool-by-default

Don't wrap redis in `ConnectionPool` if already given one for `ActiveSupport::Cache::RedisCacheStore`

Author: byroot

activesupport/CHANGELOG.md

@@ -1,3 +1,8 @@
+*   Avoid wrapping redis in a `ConnectionPool` when using `ActiveSupport::Cache::RedisCacheStore` if the `:redis`
+    option is already a `ConnectionPool`.
+
+    *Joshua Young*
+
 *   Alter `ERB::Util.tokenize` to return :PLAIN token with full input string when string doesn't contain ERB tags.
 
     *Martin Emde*

activesupport/lib/active_support/cache.rb

@@ -186,6 +186,9 @@ def retrieve_store_class(store)
     #   @last_mod_time = Time.now  # Invalidate the entire cache by changing namespace
     #
     class Store
+      # Default +ConnectionPool+ options
+      DEFAULT_POOL_OPTIONS = { size: 5, timeout: 5 }.freeze
+
       cattr_accessor :logger, instance_writer: true
       cattr_accessor :raise_on_invalid_cache_expiration_time, default: false
 
@@ -194,9 +197,6 @@ class Store
 
       class << self
         private
-          DEFAULT_POOL_OPTIONS = { size: 5, timeout: 5 }.freeze
-          private_constant :DEFAULT_POOL_OPTIONS
-
           def retrieve_pool_options(options)
             if options.key?(:pool)
               pool_options = options.delete(:pool)

activesupport/lib/active_support/cache/redis_cache_store.rb

@@ -111,15 +111,25 @@ def build_redis_client(**redis_options)
 
       # Creates a new Redis cache store.
       #
-      # There are four ways to provide the Redis client used by the cache: the
-      # +:redis+ param can be a Redis instance or a block that returns a Redis
-      # instance, or the +:url+ param can be a string or an array of strings
-      # which will be used to create a Redis instance or a +Redis::Distributed+
-      # instance.
+      # There are a few ways to provide the Redis client used by the cache:
+      #
+      # 1. The +:redis+ param can be:
+      #    - A Redis instance.
+      #    - A +ConnectionPool+ instance wrapping a Redis instance.
+      #    - A block that returns a Redis instance.
+      #
+      # 2. The +:url+ param can be:
+      #    - A string used to create a Redis instance.
+      #    - An array of strings used to create a +Redis::Distributed+ instance.
+      #
+      # If the final Redis instance is not already a +ConnectionPool+, it will
+      # be wrapped in one using +ActiveSupport::Cache::Store::DEFAULT_POOL_OPTIONS+.
+      # These options can be overridden with the +:pool+ param, or the pool can be
+      # disabled with +:pool: false+.
       #
       #   Option  Class       Result
-      #   :redis  Proc    ->  options[:redis].call
       #   :redis  Object  ->  options[:redis]
+      #   :redis  Proc    ->  options[:redis].call
       #   :url    String  ->  Redis.new(url: …)
       #   :url    Array   ->  Redis::Distributed.new([{ url: … }, { url: … }, …])
       #
@@ -149,7 +159,8 @@ def build_redis_client(**redis_options)
       def initialize(error_handler: DEFAULT_ERROR_HANDLER, **redis_options)
         universal_options = redis_options.extract!(*UNIVERSAL_OPTIONS)
 
-        if pool_options = self.class.send(:retrieve_pool_options, redis_options)
+        already_pool = redis_options[:redis].instance_of?(::ConnectionPool)
+        if !already_pool && pool_options = self.class.send(:retrieve_pool_options, redis_options)
           @redis = ::ConnectionPool.new(pool_options) { self.class.build_redis(**redis_options) }
         else
           @redis = self.class.build_redis(**redis_options)

activesupport/test/cache/stores/redis_cache_store_test.rb

@@ -328,6 +328,16 @@ def test_connection_pooling_by_default
       assert_equal 5, pool.instance_variable_get(:@timeout)
     end
 
+    def test_no_connection_pooling_by_default_when_already_pool
+      redis = ::ConnectionPool.new(size: 10, timeout: 2.5) { Redis.new }
+      cache = ActiveSupport::Cache.lookup_store(:redis_cache_store, redis: redis)
+      pool = cache.redis
+      assert_kind_of ::ConnectionPool, pool
+      assert_same redis, pool
+      assert_equal 10, pool.size
+      assert_equal 2.5, pool.instance_variable_get(:@timeout)
+    end
+
     private
       def store
         [:redis_cache_store]