Merge pull request #31487 from fatkodima/improve_cache_fault_tolerance

Improve fault tolerance for redis cache store

5 files changed, 137 insertions, 10 deletions

diff --git a/activesupport/lib/active_support/cache/redis_cache_store.rb b/activesupport/lib/active_support/cache/redis_cache_store.rb
index 3347576651..c4cd9c4761 100644
--- a/activesupport/lib/active_support/cache/redis_cache_store.rb
+++ b/activesupport/lib/active_support/cache/redis_cache_store.rb
@@ -253,7 +253,9 @@ module ActiveSupport
       # Failsafe: Raises errors.
       def increment(name, amount = 1, options = nil)
         instrument :increment, name, amount: amount do
-          redis.with { |c| c.incrby normalize_key(name, options), amount }
+          failsafe :increment do
+            redis.with { |c| c.incrby normalize_key(name, options), amount }
+          end
         end
       end
 
@@ -267,7 +269,9 @@ module ActiveSupport
       # Failsafe: Raises errors.
       def decrement(name, amount = 1, options = nil)
         instrument :decrement, name, amount: amount do
-          redis.with { |c| c.decrby normalize_key(name, options), amount }
+          failsafe :decrement do
+            redis.with { |c| c.decrby normalize_key(name, options), amount }
+          end
         end
       end
 
@@ -343,7 +347,10 @@ module ActiveSupport
           options = merged_options(options)
 
           keys = names.map { |name| normalize_key(name, options) }
-          values = redis.with { |c| c.mget(*keys) }
+
+          values = failsafe(:read_multi_mget, returning: {}) do
+            redis.with { |c| c.mget(*keys) }
+          end
 
           names.zip(values).each_with_object({}) do |(name, value), results|
             if value
@@ -368,7 +375,7 @@ module ActiveSupport
             expires_in += 5.minutes
           end
 
-          failsafe :write_entry do
+          failsafe :write_entry, returning: false do
             if unless_exist || expires_in
               modifiers = {}
               modifiers[:nx] = unless_exist
diff --git a/activesupport/test/cache/behaviors.rb b/activesupport/test/cache/behaviors.rb
index 0e07e2319d..745dc09e2c 100644
--- a/activesupport/test/cache/behaviors.rb
+++ b/activesupport/test/cache/behaviors.rb
@@ -7,4 +7,5 @@ require_relative "behaviors/cache_store_behavior"
 require_relative "behaviors/cache_store_version_behavior"
 require_relative "behaviors/connection_pool_behavior"
 require_relative "behaviors/encoded_key_cache_behavior"
+require_relative "behaviors/failure_safety_behavior"
 require_relative "behaviors/local_cache_behavior"
diff --git a/activesupport/test/cache/behaviors/failure_safety_behavior.rb b/activesupport/test/cache/behaviors/failure_safety_behavior.rb
new file mode 100644
index 0000000000..53bda4f942
--- /dev/null
+++ b/activesupport/test/cache/behaviors/failure_safety_behavior.rb
@@ -0,0 +1,91 @@
+# frozen_string_literal: true
+
+module FailureSafetyBehavior
+  def test_fetch_read_failure_returns_nil
+    @cache.write("foo", "bar")
+
+    emulating_unavailability do |cache|
+      assert_nil cache.fetch("foo")
+    end
+  end
+
+  def test_fetch_read_failure_does_not_attempt_to_write
+  end
+
+  def test_read_failure_returns_nil
+    @cache.write("foo", "bar")
+
+    emulating_unavailability do |cache|
+      assert_nil cache.read("foo")
+    end
+  end
+
+  def test_read_multi_failure_returns_empty_hash
+    @cache.write_multi("foo" => "bar", "baz" => "quux")
+
+    emulating_unavailability do |cache|
+      assert_equal Hash.new, cache.read_multi("foo", "baz")
+    end
+  end
+
+  def test_write_failure_returns_false
+    emulating_unavailability do |cache|
+      assert_equal false, cache.write("foo", "bar")
+    end
+  end
+
+  def test_write_multi_failure_not_raises
+    emulating_unavailability do |cache|
+      assert_nothing_raised do
+        cache.write_multi("foo" => "bar", "baz" => "quux")
+      end
+    end
+  end
+
+  def test_fetch_multi_failure_returns_fallback_results
+    @cache.write_multi("foo" => "bar", "baz" => "quux")
+
+    emulating_unavailability do |cache|
+      fetched = cache.fetch_multi("foo", "baz") { |k| "unavailable" }
+      assert_equal Hash["foo" => "unavailable", "baz" => "unavailable"], fetched
+    end
+  end
+
+  def test_delete_failure_returns_false
+    @cache.write("foo", "bar")
+
+    emulating_unavailability do |cache|
+      assert_equal false, cache.delete("foo")
+    end
+  end
+
+  def test_exist_failure_returns_false
+    @cache.write("foo", "bar")
+
+    emulating_unavailability do |cache|
+      assert !cache.exist?("foo")
+    end
+  end
+
+  def test_increment_failure_returns_nil
+    @cache.write("foo", 1, raw: true)
+
+    emulating_unavailability do |cache|
+      assert_nil cache.increment("foo")
+    end
+  end
+
+  def test_decrement_failure_returns_nil
+    @cache.write("foo", 1, raw: true)
+
+    emulating_unavailability do |cache|
+      assert_nil cache.decrement("foo")
+    end
+  end
+
+  def test_clear_failure_returns_nil
+    emulating_unavailability do |cache|
+      assert_nil cache.clear
+    end
+  end
+end
diff --git a/activesupport/test/cache/stores/mem_cache_store_test.rb b/activesupport/test/cache/stores/mem_cache_store_test.rb
index ccb3b7403f..3e2316f217 100644
--- a/activesupport/test/cache/stores/mem_cache_store_test.rb
+++ b/activesupport/test/cache/stores/mem_cache_store_test.rb
@@ -17,6 +17,12 @@ class SlowDalliClient < Dalli::Client
   end
 end
 
+class UnavailableDalliServer < Dalli::Server
+  def alive?
+    false
+  end
+end
+
 class MemCacheStoreTest < ActiveSupport::TestCase
   begin
     ss = Dalli::Client.new("localhost:11211").stats
@@ -46,6 +52,7 @@ class MemCacheStoreTest < ActiveSupport::TestCase
   include EncodedKeyCacheBehavior
   include AutoloadingCacheBehavior
   include ConnectionPoolBehavior
+  include FailureSafetyBehavior
 
   def test_raw_values
     cache = ActiveSupport::Cache.lookup_store(:mem_cache_store, raw: true)
@@ -118,4 +125,14 @@ class MemCacheStoreTest < ActiveSupport::TestCase
       Dalli.send(:remove_const, :Client)
       Dalli.const_set(:Client, old_client)
     end
+
+    def emulating_unavailability
+      old_server = Dalli.send(:remove_const, :Server)
+      Dalli.const_set(:Server, UnavailableDalliServer)
+
+      yield ActiveSupport::Cache::MemCacheStore.new
+    ensure
+      Dalli.send(:remove_const, :Server)
+      Dalli.const_set(:Server, old_server)
+    end
 end
diff --git a/activesupport/test/cache/stores/redis_cache_store_test.rb b/activesupport/test/cache/stores/redis_cache_store_test.rb
index cdbb5dc4c6..7c1286a115 100644
--- a/activesupport/test/cache/stores/redis_cache_store_test.rb
+++ b/activesupport/test/cache/stores/redis_cache_store_test.rb
@@ -156,15 +156,26 @@ module ActiveSupport::Cache::RedisCacheStoreTests
   class StoreAPITest < StoreTest
   end
 
-  class FailureSafetyTest < StoreTest
-    test "fetch read failure returns nil" do
+  class UnavailableRedisClient < Redis::Client
+    def ensure_connected
+      raise Redis::BaseConnectionError
     end
+  end
 
-    test "fetch read failure does not attempt to write" do
-    end
+  class FailureSafetyTest < StoreTest
+    include FailureSafetyBehavior
 
-    test "write failure returns nil" do
-    end
+    private
+
+      def emulating_unavailability
+        old_client = Redis.send(:remove_const, :Client)
+        Redis.const_set(:Client, UnavailableRedisClient)
+
+        yield ActiveSupport::Cache::RedisCacheStore.new
+      ensure
+        Redis.send(:remove_const, :Client)
+        Redis.const_set(:Client, old_client)
+      end
   end
 
   class DeleteMatchedTest < StoreTest