Eagerly validate pool arguments in Redis and MemCache stores

Fix: rails#48352

While we should ensure instantiating the store doesn't immediately
attempt to connect, we should eagerly process arguments so that
if they are somehow invalid we fail early during boot rather than at
runtime.

Additionally, since it's common to get pool parameters from environment
variable, we can use `Integer` and `Float` so that string representations
are valid.

Author: byroot

activesupport/lib/active_support/cache.rb

@@ -186,35 +186,44 @@ class << self
           private_constant :DEFAULT_POOL_OPTIONS
 
           def retrieve_pool_options(options)
-            unless options.key?(:pool) || options.key?(:pool_size) || options.key?(:pool_timeout)
-              options[:pool] = true
-            end
+            if options.key?(:pool)
+              pool_options = options.delete(:pool)
+            elsif options.key?(:pool_size) || options.key?(:pool_timeout)
+              pool_options = {}
+
+              if options.key?(:pool_size)
+                ActiveSupport.deprecator.warn(<<~MSG)
+                  Using :pool_size is deprecated and will be removed in Rails 7.2.
+                  Use `pool: { size: #{options[:pool_size].inspect} }` instead.
+                MSG
+                pool_options[:size] = options.delete(:pool_size)
+              end
 
-            if (pool_options = options.delete(:pool))
-              if Hash === pool_options
-                DEFAULT_POOL_OPTIONS.merge(pool_options)
-              else
-                DEFAULT_POOL_OPTIONS
+              if options.key?(:pool_timeout)
+                ActiveSupport.deprecator.warn(<<~MSG)
+                  Using :pool_timeout is deprecated and will be removed in Rails 7.2.
+                  Use `pool: { timeout: #{options[:pool_timeout].inspect} }` instead.
+                MSG
+                pool_options[:timeout] = options.delete(:pool_timeout)
               end
             else
-              {}.tap do |pool_options|
-                if options[:pool_size]
-                  ActiveSupport.deprecator.warn(<<~MSG)
-                    Using :pool_size is deprecated and will be removed in Rails 7.2.
-                    Use `pool: { size: #{options[:pool_size].inspect} }` instead.
-                  MSG
-                  pool_options[:size] = options.delete(:pool_size)
-                end
-
-                if options[:pool_timeout]
-                  ActiveSupport.deprecator.warn(<<~MSG)
-                    Using :pool_timeout is deprecated and will be removed in Rails 7.2.
-                    Use `pool: { timeout: #{options[:pool_timeout].inspect} }` instead.
-                  MSG
-                  pool_options[:timeout] = options.delete(:pool_timeout)
-                end
-              end
+              pool_options = true
             end
+
+            case pool_options
+            when false, nil
+              return false
+            when true
+              pool_options = DEFAULT_POOL_OPTIONS
+            when Hash
+              pool_options[:size] = Integer(pool_options[:size]) if pool_options.key?(:size)
+              pool_options[:timeout] = Float(pool_options[:timeout]) if pool_options.key?(:timeout)
+              pool_options = DEFAULT_POOL_OPTIONS.merge(pool_options)
+            else
+              raise TypeError, "Invalid :pool argument, expected Hash, got: #{pool_options.inspect}"
+            end
+
+            pool_options unless pool_options.empty?
           end
       end
 

activesupport/lib/active_support/cache/mem_cache_store.rb

@@ -99,10 +99,10 @@ def self.build_mem_cache(*addresses) # :nodoc:
         addresses = nil if addresses.compact.empty?
         pool_options = retrieve_pool_options(options)
 
-        if pool_options.empty?
-          Dalli::Client.new(addresses, options)
-        else
+        if pool_options
           ConnectionPool.new(pool_options) { Dalli::Client.new(addresses, options.merge(threadsafe: false)) }
+        else
+          Dalli::Client.new(addresses, options)
         end
       end
 

activesupport/lib/active_support/cache/redis_cache_store.rb

@@ -105,6 +105,7 @@ def build_redis_client(**redis_options)
 
       attr_reader :redis_options
       attr_reader :max_key_bytesize
+      attr_reader :redis
 
       # Creates a new Redis cache store.
       #
@@ -141,7 +142,11 @@ def build_redis_client(**redis_options)
       #   cache.exist?('foo') # => true
       #   cache.exist?('bar') # => false
       def initialize(namespace: nil, compress: true, compress_threshold: 1.kilobyte, coder: default_coder, expires_in: nil, race_condition_ttl: nil, error_handler: DEFAULT_ERROR_HANDLER, skip_nil: false, **redis_options)
-        @redis_options = redis_options
+        if 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)
+        end
 
         @max_key_bytesize = MAX_KEY_BYTESIZE
         @error_handler = error_handler
@@ -153,21 +158,8 @@ def initialize(namespace: nil, compress: true, compress_threshold: 1.kilobyte, c
           coder: coder, skip_nil: skip_nil
       end
 
-      def redis
-        @redis ||= begin
-          pool_options = self.class.send(:retrieve_pool_options, redis_options)
-
-          if pool_options.any?
-            ::ConnectionPool.new(pool_options) { self.class.build_redis(**redis_options) }
-          else
-            self.class.build_redis(**redis_options)
-          end
-        end
-      end
-
       def inspect
-        instance = @redis || @redis_options
-        "#<#{self.class} options=#{options.inspect} redis=#{instance.inspect}>"
+        "#<#{self.class} options=#{options.inspect} redis=#{redis.inspect}>"
       end
 
       # Cache Store API implementation.

activesupport/test/cache/stores/mem_cache_store_test.rb

@@ -86,6 +86,22 @@ def after_teardown
   include ConnectionPoolBehavior
   include FailureSafetyBehavior
 
+  test "validate pool arguments" do
+    assert_raises TypeError do
+      ActiveSupport::Cache::MemCacheStore.new(pool: { size: [] })
+    end
+
+    assert_raises TypeError do
+      ActiveSupport::Cache::MemCacheStore.new(pool: { timeout: [] })
+    end
+
+    ActiveSupport::Cache::MemCacheStore.new(pool: { size: "12", timeout: "1.5" })
+  end
+
+  test "instantiating the store doesn't connect to Memcache" do
+    ActiveSupport::Cache::MemCacheStore.new("memcached://localhost:1")
+  end
+
   # Overrides test from LocalCacheBehavior in order to stub out the cache clear
   # and replace it with a delete.
   def test_clear_also_clears_local_cache

activesupport/test/cache/stores/redis_cache_store_test.rb

@@ -110,9 +110,25 @@ class InitializationTest < ActiveSupport::TestCase
       assert_same @cache.redis, redis_instance
     end
 
+    test "validate pool arguments" do
+      assert_raises TypeError do
+        build(url: REDIS_URL, pool: { size: [] })
+      end
+
+      assert_raises TypeError do
+        build(url: REDIS_URL, pool: { timeout: [] })
+      end
+
+      build(url: REDIS_URL, pool: { size: "12", timeout: "1.5" })
+    end
+
+    test "instantiating the store doesn't connect to Redis" do
+      build(url: "redis://localhost:1")
+    end
+
     private
       def build(**kwargs)
-        ActiveSupport::Cache::RedisCacheStore.new(**kwargs.merge(pool: false)).tap(&:redis)
+        ActiveSupport::Cache::RedisCacheStore.new(pool: false, **kwargs).tap(&:redis)
       end
   end