Merge pull request rails#43086 from nstuart-at-salesforce/nas_addRaiseErrorsParam

Removed rescue block from `ActiveSupport::Cache::RedisCacheStore#handle_exception`

Author: byroot

activesupport/CHANGELOG.md

@@ -1,3 +1,11 @@
+*   Removed rescue block from `ActiveSupport::Cache::RedisCacheStore#handle_exception`
+
+    Previously, if you provided a `error_handler` to `redis_cache_store`, any errors thrown by
+    the error handler would be rescued and logged only. Removed the `rescue` clause from `handle_exception`
+    to allow these to be thrown. 
+    
+    *Nicholas A. Stuart*
+
 *   Allow entirely opting out of deprecation warnings.
 
     Previously if you did `app.config.active_support.deprecation = :silence`, some work would

activesupport/lib/active_support/cache/redis_cache_store.rb

@@ -467,8 +467,6 @@ def handle_exception(exception:, method:, returning:)
           if @error_handler
             @error_handler.(method: method, exception: exception, returning: returning)
           end
-        rescue => failsafe
-          warn "RedisCacheStore ignored exception in handle_exception: #{failsafe.class}: #{failsafe.message}\n  #{failsafe.backtrace.join("\n  ")}"
         end
     end
   end

activesupport/test/cache/behaviors.rb

@@ -9,4 +9,5 @@
 require_relative "behaviors/connection_pool_behavior"
 require_relative "behaviors/encoded_key_cache_behavior"
 require_relative "behaviors/failure_safety_behavior"
+require_relative "behaviors/failure_raising_behavior"
 require_relative "behaviors/local_cache_behavior"

activesupport/test/cache/behaviors/failure_raising_behavior.rb

@@ -0,0 +1,119 @@
+# frozen_string_literal: true
+
+module FailureRaisingBehavior
+  def test_fetch_read_failure_raises
+    @cache.write("foo", "bar")
+
+    assert_raise Redis::BaseError do
+      emulating_unavailability do |cache|
+        cache.fetch("foo")
+      end
+    end
+  end
+
+  def test_fetch_with_block_read_failure_raises
+    @cache.write("foo", "bar")
+
+    assert_raise Redis::BaseError do
+      emulating_unavailability do |cache|
+        cache.fetch("foo") { "1" }
+      end
+    end
+
+    assert_equal "bar", @cache.read("foo")
+  end
+
+  def test_read_failure_raises
+    @cache.write("foo", "bar")
+
+    assert_raise Redis::BaseError do
+      emulating_unavailability do |cache|
+        cache.read("foo")
+      end
+    end
+  end
+
+  def test_read_multi_failure_raises
+    @cache.write_multi("foo" => "bar", "baz" => "quux")
+
+    assert_raise Redis::BaseError do
+      emulating_unavailability do |cache|
+        cache.read_multi("foo", "baz")
+      end
+    end
+  end
+
+  def test_write_failure_raises
+    assert_raise Redis::BaseError do
+      emulating_unavailability do |cache|
+        cache.write("foo", "bar")
+      end
+    end
+  end
+
+  def test_write_multi_failure_raises
+    assert_raise Redis::BaseError do
+      emulating_unavailability do |cache|
+        cache.write_multi("foo" => "bar", "baz" => "quux")
+      end
+    end
+  end
+
+  def test_fetch_multi_failure_raises
+    @cache.write_multi("foo" => "bar", "baz" => "quux")
+
+    assert_raise Redis::BaseError do
+      emulating_unavailability do |cache|
+        cache.fetch_multi("foo", "baz") { |k| "unavailable" }
+      end
+    end
+  end
+
+  def test_delete_failure_raises
+    @cache.write("foo", "bar")
+
+    assert_raise Redis::BaseError do
+      emulating_unavailability do |cache|
+        cache.delete("foo")
+      end
+    end
+  end
+
+  def test_exist_failure_raises
+    @cache.write("foo", "bar")
+
+    assert_raise Redis::BaseError do
+      emulating_unavailability do |cache|
+        cache.exist?("foo")
+      end
+    end
+  end
+
+  def test_increment_failure_raises
+    @cache.write("foo", 1, raw: true)
+
+    assert_raise Redis::BaseError do
+      emulating_unavailability do |cache|
+        cache.increment("foo")
+      end
+    end
+  end
+
+  def test_decrement_failure_raises
+    @cache.write("foo", 1, raw: true)
+
+    assert_raise Redis::BaseError do
+      emulating_unavailability do |cache|
+        cache.decrement("foo")
+      end
+    end
+  end
+
+  def test_clear_failure_returns_nil
+    assert_raise Redis::BaseError do
+      emulating_unavailability do |cache|
+        cache.clear
+      end
+    end
+  end
+end

activesupport/test/cache/behaviors/failure_safety_behavior.rb

@@ -10,6 +10,18 @@ def test_fetch_read_failure_returns_nil
   end
 
   def test_fetch_read_failure_does_not_attempt_to_write
+    @cache.write("foo", "bar")
+
+    emulating_unavailability do |cache|
+      val = cache.fetch("foo") { "1" }
+
+      ##
+      # Though the `write` part of fetch fails for the same reason
+      # `read` will, the block result is still executed and returned.
+      assert_equal "1", val
+    end
+
+    assert_equal "bar", @cache.read("foo")
   end
 
   def test_read_failure_returns_nil

activesupport/test/cache/stores/redis_cache_store_test.rb

@@ -283,6 +283,38 @@ def ensure_connected
     end
   end
 
+  class FailureRaisingFromUnavailableClientTest < StoreTest
+    include FailureRaisingBehavior
+
+    private
+      def emulating_unavailability
+        old_client = Redis.send(:remove_const, :Client)
+        Redis.const_set(:Client, UnavailableRedisClient)
+
+        yield ActiveSupport::Cache::RedisCacheStore.new(namespace: @namespace,
+                                                        error_handler: -> (method:, returning:, exception:) { raise exception })
+      ensure
+        Redis.send(:remove_const, :Client)
+        Redis.const_set(:Client, old_client)
+      end
+  end
+
+  class FailureRaisingFromMaxClientsReachedErrorTest < StoreTest
+    include FailureRaisingBehavior
+
+    private
+      def emulating_unavailability
+        old_client = Redis.send(:remove_const, :Client)
+        Redis.const_set(:Client, MaxClientsReachedRedisClient)
+
+        yield ActiveSupport::Cache::RedisCacheStore.new(namespace: @namespace,
+                                                        error_handler: -> (method:, returning:, exception:) { raise exception })
+      ensure
+        Redis.send(:remove_const, :Client)
+        Redis.const_set(:Client, old_client)
+      end
+  end
+
   class FailureSafetyFromUnavailableClientTest < StoreTest
     include FailureSafetyBehavior