Fix Redis::Distributed being 'mset_capable' when wrapped in a Connect…

…ionPool

Also adds some test-coverage for RedisCacheStore backed by Redis::Distributed / ConnectionPool

Fixes rails#48938

activesupport/CHANGELOG.md

@@ -1,3 +1,10 @@
+*   Fix CacheStore#write_multi when using a distributed Redis cache with a connection pool.
+
+    Fixes [#48938](https://github.com/rails/rails/issues/48938).
+
+    *Jonathan del Strother*
+
+
 ## Rails 7.0.7 (August 09, 2023) ##
 
 *   Fix `Cache::NullStore` with local caching for repeated reads.

activesupport/lib/active_support/cache/redis_cache_store.rb

@@ -313,13 +313,15 @@ def mset_capable? # :nodoc:
 
       private
         def set_redis_capabilities
-          case redis
-          when Redis::Distributed
-            @mget_capable = true
-            @mset_capable = false
-          else
-            @mget_capable = true
-            @mset_capable = true
+          redis.with do |c|
+            case c
+            when Redis::Distributed
+              @mget_capable = true
+              @mset_capable = false
+            else
+              @mget_capable = true
+              @mset_capable = true
+            end
           end
         end
 

activesupport/test/cache/stores/redis_cache_store_test.rb

@@ -139,7 +139,9 @@ def lookup_store(options = {})
 
     teardown do
       @cache.clear
-      @cache.redis.disconnect!
+      @cache.redis.with do |r|
+        r.respond_to?(:on_each_node, true) ? r.send(:on_each_node, :disconnect!) : r.disconnect!
+      end
     end
   end
 
@@ -153,63 +155,63 @@ class RedisCacheStoreCommonBehaviorTest < StoreTest
     include EncodedKeyCacheBehavior
 
     def test_fetch_multi_uses_redis_mget
-      assert_called(@cache.redis, :mget, returns: []) do
+      assert_called(redis_backend, :mget, returns: []) do
         @cache.fetch_multi("a", "b", "c") do |key|
           key * 2
         end
       end
     end
 
     def test_fetch_multi_with_namespace
-      assert_called_with(@cache.redis, :mget, ["custom-namespace:a", "custom-namespace:b", "custom-namespace:c"], returns: []) do
+      assert_called_with(redis_backend, :mget, ["custom-namespace:a", "custom-namespace:b", "custom-namespace:c"], returns: []) do
         @cache.fetch_multi("a", "b", "c", namespace: "custom-namespace") do |key|
           key * 2
         end
       end
     end
 
     def test_fetch_multi_without_names
-      assert_not_called(@cache.redis, :mget) do
+      assert_not_called(redis_backend, :mget) do
         @cache.fetch_multi() { }
       end
     end
 
     def test_increment_expires_in
-      assert_called_with @cache.redis, :incrby, [ "#{@namespace}:foo", 1 ] do
-        assert_called_with @cache.redis, :expire, [ "#{@namespace}:foo", 60 ] do
+      assert_called_with redis_backend, :incrby, [ "#{@namespace}:foo", 1 ] do
+        assert_called_with redis_backend, :expire, [ "#{@namespace}:foo", 60 ] do
           @cache.increment "foo", 1, expires_in: 60
         end
       end
 
       # key and ttl exist
-      @cache.redis.setex "#{@namespace}:bar", 120, 1
-      assert_not_called @cache.redis, :expire do
+      redis_backend { |r| r.setex "#{@namespace}:bar", 120, 1 }
+      assert_not_called redis_backend, :expire do
         @cache.increment "bar", 1, expires_in: 2.minutes
       end
 
       # key exist but not have expire
-      @cache.redis.set "#{@namespace}:dar", 10
-      assert_called_with @cache.redis, :expire, [ "#{@namespace}:dar", 60 ] do
+      redis_backend { |r| r.set "#{@namespace}:dar", 10 }
+      assert_called_with redis_backend, :expire, [ "#{@namespace}:dar", 60 ] do
         @cache.increment "dar", 1, expires_in: 60
       end
     end
 
     def test_decrement_expires_in
-      assert_called_with @cache.redis, :decrby, [ "#{@namespace}:foo", 1 ] do
-        assert_called_with @cache.redis, :expire, [ "#{@namespace}:foo", 60 ] do
+      assert_called_with redis_backend, :decrby, [ "#{@namespace}:foo", 1 ] do
+        assert_called_with redis_backend, :expire, [ "#{@namespace}:foo", 60 ] do
           @cache.decrement "foo", 1, expires_in: 60
         end
       end
 
       # key and ttl exist
-      @cache.redis.setex "#{@namespace}:bar", 120, 1
-      assert_not_called @cache.redis, :expire do
+      redis_backend { |r| r.setex "#{@namespace}:bar", 120, 1 }
+      assert_not_called redis_backend, :expire do
         @cache.decrement "bar", 1, expires_in: 2.minutes
       end
 
       # key exist but not have expire
-      @cache.redis.set "#{@namespace}:dar", 10
-      assert_called_with @cache.redis, :expire, [ "#{@namespace}:dar", 60 ] do
+      redis_backend { |r| r.set "#{@namespace}:dar", 10 }
+      assert_called_with redis_backend, :expire, [ "#{@namespace}:dar", 60 ] do
         @cache.decrement "dar", 1, expires_in: 60
       end
     end
@@ -221,6 +223,13 @@ def test_large_string_with_default_compression_settings
     def test_large_object_with_default_compression_settings
       assert_compressed(LARGE_OBJECT)
     end
+
+    def redis_backend
+      @cache.redis.with do |r|
+        yield r if block_given?
+        return r
+      end
+    end
   end
 
   class OptimizedRedisCacheStoreCommonBehaviorTest < RedisCacheStoreCommonBehaviorTest
@@ -260,6 +269,12 @@ def after_teardown
     end
   end
 
+  class RedisCacheStoreWithDistributedRedisTest < RedisCacheStoreCommonBehaviorTest
+    def lookup_store(options = {})
+      super(options.merge(pool_size: 5, url: [ENV["REDIS_URL"] || "redis://localhost:6379/0"] * 2))
+    end
+  end
+
   class ConnectionPoolBehaviourTest < StoreTest
     include ConnectionPoolBehavior