From 71ff60fd1c3b2183e7f2f9509d9bd2a0d392ddc0 Mon Sep 17 00:00:00 2001 From: John Bolliger Date: Wed, 30 Sep 2026 16:57:21 -0700 Subject: [PATCH 1/5] Add opt-in handling for cache provider errors Brings the :handle_cache_error and :cache_error_proc options from the unmerged PR #16 (released internally as 0.3.0.rc2) forward onto 1.3.0. When :handle_cache_error is true, an error from the cache provider acts as a cache miss and goes to :cache_error_proc. Both are off by default. An error from the fetch block (the RPC call, or RemoteRecordNotFound from a bang finder) still goes to the caller, and the block runs at most once. fetch keeps the provider fetch, so :race_condition_ttl keeps working. Co-Authored-By: Claude Opus 5.5 (1M context) --- README.md | 27 +++++ lib/active_remote/cached/cache.rb | 76 +++++++++++++- spec/active_remote/cached/cache_spec.rb | 125 ++++++++++++++++++++++++ spec/active_remote/cached_spec.rb | 30 ++++++ 4 files changed, 256 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 97553c4..43c8f44 100644 --- a/README.md +++ b/README.md @@ -100,6 +100,33 @@ ActiveRemote::Cached.default_options_overwrite({}) Without `:expires_in`, a cached finder writes an entry that never expires. +#### Cache errors + +By default, an error from the cache provider goes to the caller. To make a +cache error act as a cache miss, set `:handle_cache_error`: + +```ruby +# config/initializers/active_remote_cached.rb +ActiveRemote::Cached.default_options( + :handle_cache_error => true, + :cache_error_proc => lambda { |error| Rails.logger.error(error) } +) +``` + +| Option | Default | Description | +|---|---|---| +| `:handle_cache_error` | not set (off) | When true, a cache error does not go to the caller. `read` and `write` return nil, `exist?` returns false, `delete` returns nil, and `fetch` calls the block without the cache. | +| `:cache_error_proc` | not set | A callable that receives the cache error. It runs only when `:handle_cache_error` is true. | + +The library reads these two options from `default_options` only, and does not +pass them to the cache provider. An error from the `fetch` block (for example, +`ActiveRemote::RemoteRecordNotFound` from a bang finder, or an RPC error) always +goes to the caller. The block runs at most once for each `fetch`. + +`ActiveSupport::Cache::RedisCacheStore` already catches Redis connection errors +and sends them to its own `:error_handler`. These options also catch the errors +that the store does not catch, for example an entry that fails to deserialize. + #### Local overrides Each finder as takes an optional options hash that will override the options passed to the caching provider (override from the global defaults setup for ActiveRemote::Cached) diff --git a/lib/active_remote/cached/cache.rb b/lib/active_remote/cached/cache.rb index 187098e..16d74ae 100644 --- a/lib/active_remote/cached/cache.rb +++ b/lib/active_remote/cached/cache.rb @@ -9,6 +9,10 @@ class Cache < ::SimpleDelegator # calls on it. class InvalidCacheProvider < ::StandardError; end + # Options that control error handling here. They are not passed to the + # cache provider. + ERROR_HANDLING_OPTIONS = %i[handle_cache_error cache_error_proc].freeze + attr_reader :cache_provider def initialize(new_cache_provider) @@ -27,6 +31,9 @@ def initialize(new_cache_provider) def delete(*args) nested_cache_provider.delete(*args) super + rescue StandardError => e + handle_or_reraise_cache_error(e) + nil end def enable_nested_caching! @@ -39,34 +46,99 @@ def nested_caching? def exist?(*args) nested_cache_provider.exist?(*args) || super + rescue StandardError => e + handle_or_reraise_cache_error(e) + false end - def fetch(name, options = {}) + # An error from the block (the RPC call) always goes to the caller. Only + # an error from a cache provider goes to handle_or_reraise_cache_error. + # When that error is handled, the block value is returned without the + # cache, and the block runs at most once. + def fetch(name, options = {}, &block) + block_result = FetchBlockResult.new(block) provider_options = provider_fetch_options(options) - fetch_value = nested_cache_provider.fetch(name, provider_options) { super(name, provider_options) } + fetch_value = provider_fetch(name, provider_options, block && block_result) delete(name) if delete_after_fetch?(fetch_value, options, provider_options) fetch_value + rescue StandardError => e + raise if block_result.raised?(e) + + handle_or_reraise_cache_error(e) + block_result.value end def read(*args) nested_cache_provider.read(*args) || super + rescue StandardError => e + handle_or_reraise_cache_error(e) + nil end def write(*args) nested_cache_provider.write(*args) super + rescue StandardError => e + handle_or_reraise_cache_error(e) + nil end private attr_reader :nested_cache_provider + # Runs the fetch block at most once, on the first call to #value, and + # keeps its value or its error. + class FetchBlockResult + def initialize(block) + @block = block + end + + def value + run unless defined?(@value) + raise @error if @error + + @value + end + + def raised?(error) + !@error.nil? && @error.equal?(error) + end + + private + + def run + @value = @block&.call + rescue StandardError => e + @value = nil + @error = e + end + end + private_constant :FetchBlockResult + + # Without a block, the provider gets no block, as before. + def provider_fetch(name, options, block_result) + provider_block = block_result && proc { block_result.value } + + nested_cache_provider.fetch(name, options) do + cache_provider.fetch(name, options, &provider_block) + end + end + + def handle_or_reraise_cache_error(error) + raise error unless ::ActiveRemote::Cached.default_options[:handle_cache_error] + + error_proc = ::ActiveRemote::Cached.default_options[:cache_error_proc] + error_proc.call(error) if error_proc.respond_to?(:call) + end + # :skip_nil tells the provider not to write a nil at all, which saves a # write and the delete that follows it. Only an ActiveSupport store is # known to honor the option. def provider_fetch_options(options) + options = options.except(*ERROR_HANDLING_OPTIONS) return options if options.fetch(:allow_nil, false) return options unless cache_provider.is_a?(::ActiveSupport::Cache::Store) diff --git a/spec/active_remote/cached/cache_spec.rb b/spec/active_remote/cached/cache_spec.rb index 8defcf7..efe7021 100644 --- a/spec/active_remote/cached/cache_spec.rb +++ b/spec/active_remote/cached/cache_spec.rb @@ -72,6 +72,11 @@ expect(cache_provider.exist?('key')).to eq(true) end + it 'does not persist anything on a miss with no block' do + expect(cache.fetch('key', :allow_nil => true)).to be_nil + expect(cache_provider.exist?('key')).to eq(false) + end + it 'persists a value that is neither nil nor empty' do expect(cache.fetch('key') { [:record] }).to eq([:record]) expect(cache_provider.exist?('key')).to eq(true) @@ -125,6 +130,126 @@ def delete(*args, **options) end end + describe 'cache error handling' do + let(:failing_provider) do + Class.new(::ActiveSupport::Cache::MemoryStore) do + %i[delete exist? fetch read write].each do |method_name| + define_method(method_name) { |*| raise ::IOError, "#{method_name} failed" } + end + end.new + end + let(:cache) { ::ActiveRemote::Cached::Cache.new(failing_provider) } + let(:handled_errors) { [] } + + after do + ::ActiveRemote::Cached.default_options_overwrite({}) + end + + context 'when :handle_cache_error is not set' do + it 'raises the provider error from each method' do + expect { cache.delete('key') }.to raise_error(::IOError, 'delete failed') + expect { cache.exist?('key') }.to raise_error(::IOError, 'exist? failed') + expect { cache.fetch('key') { :value } }.to raise_error(::IOError, 'fetch failed') + expect { cache.read('key') }.to raise_error(::IOError, 'read failed') + expect { cache.write('key', :value) }.to raise_error(::IOError, 'write failed') + end + end + + context 'when :handle_cache_error is true' do + before do + ::ActiveRemote::Cached.default_options( + :handle_cache_error => true, + :cache_error_proc => lambda { |error| handled_errors << error.message } + ) + end + + it 'returns a cache miss from #read' do + expect(cache.read('key')).to be_nil + expect(handled_errors).to eq(['read failed']) + end + + it 'returns false from #exist?' do + expect(cache.exist?('key')).to eq(false) + expect(handled_errors).to eq(['exist? failed']) + end + + it 'returns nil from #write' do + expect(cache.write('key', :value)).to be_nil + expect(handled_errors).to eq(['write failed']) + end + + it 'returns nil from #delete' do + expect(cache.delete('key')).to be_nil + expect(handled_errors).to eq(['delete failed']) + end + + it 'returns the block value from #fetch when the provider fails before the block' do + calls = 0 + + expect(cache.fetch('key') { calls += 1 }).to eq(1) + expect(calls).to eq(1) + expect(handled_errors).to eq(['fetch failed']) + end + + it 'does not call the block again when the provider fails after the block' do + provider = Class.new(::ActiveSupport::Cache::MemoryStore) do + def fetch(*) + yield + raise ::IOError, 'write after fetch failed' + end + end.new + cache = ::ActiveRemote::Cached::Cache.new(provider) + calls = 0 + + expect(cache.fetch('key') { calls += 1 }).to eq(1) + expect(calls).to eq(1) + expect(handled_errors).to eq(['write after fetch failed']) + end + + it 'raises an error from the #fetch block, and does not handle it' do + cache = ::ActiveRemote::Cached::Cache.new(::ActiveSupport::Cache::MemoryStore.new) + not_found = ::ActiveRemote::RemoteRecordNotFound + + expect { cache.fetch('key') { raise not_found } }.to raise_error(not_found) + expect(handled_errors).to be_empty + end + + it 'raises an error from the #fetch block when the provider fails before the block' do + not_found = ::ActiveRemote::RemoteRecordNotFound + + expect { cache.fetch('key') { raise not_found } }.to raise_error(not_found) + expect(handled_errors).to eq(['fetch failed']) + end + + it 'does not pass the error handling options to the provider' do + provider = ::ActiveSupport::Cache::MemoryStore.new + cache = ::ActiveRemote::Cached::Cache.new(provider) + + expect(provider).to receive(:fetch).with('key', { :skip_nil => true }).and_call_original + + cache.fetch('key', :handle_cache_error => true, :cache_error_proc => lambda {}) { :value } + end + + it 'handles the error without a :cache_error_proc' do + ::ActiveRemote::Cached.default_options_overwrite(:handle_cache_error => true) + + expect(cache.read('key')).to be_nil + end + end + + context 'when :handle_cache_error is false' do + it 'raises the error and does not call :cache_error_proc' do + ::ActiveRemote::Cached.default_options( + :handle_cache_error => false, + :cache_error_proc => lambda { |error| handled_errors << error.message } + ) + + expect { cache.read('key') }.to raise_error(::IOError, 'read failed') + expect(handled_errors).to be_empty + end + end + end + describe '#enable_nested_caching!' do it 'writes to the cache provider only until nested caching is enabled' do cache.write('key', 'value') diff --git a/spec/active_remote/cached_spec.rb b/spec/active_remote/cached_spec.rb index b39051e..bed7bc8 100644 --- a/spec/active_remote/cached_spec.rb +++ b/spec/active_remote/cached_spec.rb @@ -164,6 +164,36 @@ class DurationChildClass < DurationOptionClass; end end end + describe 'a cached finder when the cache provider fails' do + let(:failing_provider) do + Class.new(HashCache) do + def fetch(*) + raise ::IOError, 'cache is down' + end + end.new + end + + before do + ::ActiveRemote::Cached.cache(failing_provider) + end + + it 'raises the error by default' do + expect { ConfigurationClass.cached_find_by_guid(:guid) }.to raise_error(::IOError, 'cache is down') + end + + it 'calls the finder and the error proc when :handle_cache_error is true' do + errors = [] + ::ActiveRemote::Cached.default_options( + :handle_cache_error => true, + :cache_error_proc => lambda { |error| errors << error } + ) + + expect(ConfigurationClass.cached_find_by_guid(:guid)).to eq(:find_result) + expect(ConfigurationClass.cached_search_by_guid(:guid)).to eq([:search_result]) + expect(errors.map(&:message)).to eq(['cache is down', 'cache is down']) + end + end + describe 'RUBY_AND_ACTIVE_SUPPORT_VERSION' do it 'prefixes every cache key' do expect(::ActiveRemote::Cached.cache).to receive(:fetch).with( From 1e6c78428e986202c05cef269f4b032cd5575e93 Mon Sep 17 00:00:00 2001 From: John Bolliger Date: Wed, 30 Sep 2026 16:57:21 -0700 Subject: [PATCH 2/5] Bump to 1.4.0 and document the upgrade Co-Authored-By: Claude Opus 5.5 (1M context) --- README.md | 17 +++++++++++++++++ lib/active_remote/cached/version.rb | 2 +- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 43c8f44..f157efa 100644 --- a/README.md +++ b/README.md @@ -194,6 +194,23 @@ CI runs this matrix on Ruby 3.1, Ruby 3.4, JRuby 9.4, and JRuby 10.0. `active_remote` 8.0 requires Ruby 3.2 or later. CI does not run that version on Ruby 3.1 or JRuby 9.4. +## Upgrading to 1.4.0 + +### Cache error handling + +1.4.0 adds `:handle_cache_error` and `:cache_error_proc` (see "Cache errors"). +Both are off by default, so an app that does not set them has no change. + +An app on the internal `0.3.0.rc2` release can move to 1.4.0 and keep its +initializer. 1.4.0 does not add the rest of that release: + +- `0.3.0.rc2` `fetch` called `read`, then `write`. 1.4.0 keeps the provider + `fetch`, so `:race_condition_ttl` now works. Redis keeps each entry for + 5 more minutes. +- `0.3.0.rc2` passed only known options to the cache provider. 1.4.0 passes + every option except the two error options, as 1.3.0 does. +- Every cache key changes (see "Upgrading to 1.2.0"). + ## Upgrading to 1.3.0 ### default_options merges diff --git a/lib/active_remote/cached/version.rb b/lib/active_remote/cached/version.rb index 51c952e..5de00e0 100644 --- a/lib/active_remote/cached/version.rb +++ b/lib/active_remote/cached/version.rb @@ -2,6 +2,6 @@ module ActiveRemote module Cached - VERSION = '1.3.0' + VERSION = '1.4.0' end end From 1b38a6f0277689b54a4779dbad79a4e2feecb87e Mon Sep 17 00:00:00 2001 From: John Bolliger Date: Wed, 30 Sep 2026 17:12:20 -0700 Subject: [PATCH 3/5] Prove an RPC error is never cached, and document the fallback An error from the fetch block (the RPC call) goes to the caller and leaves the cache empty, with :handle_cache_error on or off. The README now says that a cache outage sends each finder call to the remote service, which increases its load until the cache comes back. Co-Authored-By: Claude Opus 5.5 (1M context) --- README.md | 7 +++++-- spec/active_remote/cached/cache_spec.rb | 10 ++++++---- spec/active_remote/cached_spec.rb | 15 +++++++++++++++ 3 files changed, 26 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index f157efa..c788b61 100644 --- a/README.md +++ b/README.md @@ -103,7 +103,9 @@ Without `:expires_in`, a cached finder writes an entry that never expires. #### Cache errors By default, an error from the cache provider goes to the caller. To make a -cache error act as a cache miss, set `:handle_cache_error`: +cache error act as a cache miss, set `:handle_cache_error`. A cache outage then +does not stop the finders: each call goes to the remote service. This +increases the load on that service until the cache comes back. ```ruby # config/initializers/active_remote_cached.rb @@ -121,7 +123,8 @@ ActiveRemote::Cached.default_options( The library reads these two options from `default_options` only, and does not pass them to the cache provider. An error from the `fetch` block (for example, `ActiveRemote::RemoteRecordNotFound` from a bang finder, or an RPC error) always -goes to the caller. The block runs at most once for each `fetch`. +goes to the caller, and the library never caches it. The block runs at most +once for each `fetch`. `ActiveSupport::Cache::RedisCacheStore` already catches Redis connection errors and sends them to its own `:error_handler`. These options also catch the errors diff --git a/spec/active_remote/cached/cache_spec.rb b/spec/active_remote/cached/cache_spec.rb index efe7021..c02ec45 100644 --- a/spec/active_remote/cached/cache_spec.rb +++ b/spec/active_remote/cached/cache_spec.rb @@ -206,12 +206,14 @@ def fetch(*) expect(handled_errors).to eq(['write after fetch failed']) end - it 'raises an error from the #fetch block, and does not handle it' do - cache = ::ActiveRemote::Cached::Cache.new(::ActiveSupport::Cache::MemoryStore.new) - not_found = ::ActiveRemote::RemoteRecordNotFound + it 'raises an error from the #fetch block, does not handle it, and does not cache it' do + provider = ::ActiveSupport::Cache::MemoryStore.new + cache = ::ActiveRemote::Cached::Cache.new(provider) + rpc_error = ::ActiveRemote::ActiveRemoteError - expect { cache.fetch('key') { raise not_found } }.to raise_error(not_found) + expect { cache.fetch('key') { raise rpc_error, 'rpc failed' } }.to raise_error(rpc_error) expect(handled_errors).to be_empty + expect(provider.exist?('key')).to eq(false) end it 'raises an error from the #fetch block when the provider fails before the block' do diff --git a/spec/active_remote/cached_spec.rb b/spec/active_remote/cached_spec.rb index bed7bc8..08714ef 100644 --- a/spec/active_remote/cached_spec.rb +++ b/spec/active_remote/cached_spec.rb @@ -164,6 +164,21 @@ class DurationChildClass < DurationOptionClass; end end end + describe 'a cached finder when the RPC call fails' do + it 'raises the error and does not cache it, with or without :handle_cache_error' do + provider = HashCache.new + ::ActiveRemote::Cached.cache(provider) + allow(ConfigurationClass).to receive(:find).and_raise(::ActiveRemote::ActiveRemoteError, 'rpc failed') + + [false, true].each do |handle| + ::ActiveRemote::Cached.default_options(:handle_cache_error => handle) + + expect { ConfigurationClass.cached_find_by_guid(:guid) }.to raise_error(::ActiveRemote::ActiveRemoteError) + expect(provider).to be_empty + end + end + end + describe 'a cached finder when the cache provider fails' do let(:failing_provider) do Class.new(HashCache) do From 427e6375cf5e5387581cd696c68e80a7e962d5ef Mon Sep 17 00:00:00 2001 From: John Bolliger Date: Wed, 30 Sep 2026 17:18:29 -0700 Subject: [PATCH 4/5] Share one failsafe for the cache provider rescues delete, exist?, read and write now use one failsafe helper in place of four copies of the same rescue. FetchBlockResult tracks a run flag and builds the provider block itself, so fetch no longer needs the block && block_result guard. Co-Authored-By: Claude Opus 5.5 (1M context) --- lib/active_remote/cached/cache.rb | 57 ++++++++++++++++--------------- 1 file changed, 29 insertions(+), 28 deletions(-) diff --git a/lib/active_remote/cached/cache.rb b/lib/active_remote/cached/cache.rb index 16d74ae..23f7d29 100644 --- a/lib/active_remote/cached/cache.rb +++ b/lib/active_remote/cached/cache.rb @@ -29,11 +29,10 @@ def initialize(new_cache_provider) end def delete(*args) - nested_cache_provider.delete(*args) - super - rescue StandardError => e - handle_or_reraise_cache_error(e) - nil + failsafe do + nested_cache_provider.delete(*args) + super + end end def enable_nested_caching! @@ -45,10 +44,7 @@ def nested_caching? end def exist?(*args) - nested_cache_provider.exist?(*args) || super - rescue StandardError => e - handle_or_reraise_cache_error(e) - false + failsafe(:returning => false) { nested_cache_provider.exist?(*args) || super } end # An error from the block (the RPC call) always goes to the caller. Only @@ -58,7 +54,7 @@ def exist?(*args) def fetch(name, options = {}, &block) block_result = FetchBlockResult.new(block) provider_options = provider_fetch_options(options) - fetch_value = provider_fetch(name, provider_options, block && block_result) + fetch_value = provider_fetch(name, provider_options, &block_result.to_block) delete(name) if delete_after_fetch?(fetch_value, options, provider_options) @@ -71,18 +67,14 @@ def fetch(name, options = {}, &block) end def read(*args) - nested_cache_provider.read(*args) || super - rescue StandardError => e - handle_or_reraise_cache_error(e) - nil + failsafe { nested_cache_provider.read(*args) || super } end def write(*args) - nested_cache_provider.write(*args) - super - rescue StandardError => e - handle_or_reraise_cache_error(e) - nil + failsafe do + nested_cache_provider.write(*args) + super + end end private @@ -97,34 +89,43 @@ def initialize(block) end def value - run unless defined?(@value) + run unless @ran raise @error if @error @value end + # The block to give the provider: nil when fetch got no block, so the + # provider gets no block either. A proc, because the provider yields + # the key and #value takes no argument. + def to_block + proc { value } if @block + end + def raised?(error) - !@error.nil? && @error.equal?(error) + @error.equal?(error) end private def run + @ran = true @value = @block&.call rescue StandardError => e - @value = nil @error = e end end private_constant :FetchBlockResult - # Without a block, the provider gets no block, as before. - def provider_fetch(name, options, block_result) - provider_block = block_result && proc { block_result.value } + def provider_fetch(name, options, &block) + nested_cache_provider.fetch(name, options) { cache_provider.fetch(name, options, &block) } + end - nested_cache_provider.fetch(name, options) do - cache_provider.fetch(name, options, &provider_block) - end + def failsafe(returning: nil) + yield + rescue StandardError => e + handle_or_reraise_cache_error(e) + returning end def handle_or_reraise_cache_error(error) From 353d17a17381a92b8ae4b7be401973eb823455c4 Mon Sep 17 00:00:00 2001 From: John Bolliger Date: Thu, 1 Oct 2026 14:27:20 -0700 Subject: [PATCH 5/5] Fix the cache error handling findings from the code review - The nested cache and the cache provider each get their own failsafe, so a handled error in the nested cache no longer skips the provider. - The cleanup delete in fetch never raises, so a failed delete no longer discards the value from the remote service. - An error from :cache_error_proc is written to stderr and does not replace the result. - A provider error whose cause chain holds the block error is treated as the block error, so it is not reported as a cache error. - The README says the two options apply only in default_options. Co-Authored-By: Claude Opus 5.5 (1M context) --- README.md | 23 +++++-- lib/active_remote/cached/cache.rb | 61 +++++++++++----- spec/active_remote/cached/cache_spec.rb | 92 +++++++++++++++++++++++++ 3 files changed, 156 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index c788b61..d8c5948 100644 --- a/README.md +++ b/README.md @@ -118,10 +118,16 @@ ActiveRemote::Cached.default_options( | Option | Default | Description | |---|---|---| | `:handle_cache_error` | not set (off) | When true, a cache error does not go to the caller. `read` and `write` return nil, `exist?` returns false, `delete` returns nil, and `fetch` calls the block without the cache. | -| `:cache_error_proc` | not set | A callable that receives the cache error. It runs only when `:handle_cache_error` is true. | +| `:cache_error_proc` | not set | A callable that receives the cache error. It runs only when `:handle_cache_error` is true. If the proc raises, the library writes a warning to stderr, and the call continues. | -The library reads these two options from `default_options` only, and does not -pass them to the cache provider. An error from the `fetch` block (for example, +These two options apply only in `ActiveRemote::Cached.default_options`. A value +in a finder declaration or in a finder call has no effect, and the library does +not pass it to the cache provider. + +With nested caching, the nested cache and the cache provider each handle their +own errors. An error in the nested cache does not skip the cache provider. + +An error from the `fetch` block (for example, `ActiveRemote::RemoteRecordNotFound` from a bang finder, or an RPC error) always goes to the caller, and the library never caches it. The block runs at most once for each `fetch`. @@ -202,7 +208,16 @@ version on Ruby 3.1 or JRuby 9.4. ### Cache error handling 1.4.0 adds `:handle_cache_error` and `:cache_error_proc` (see "Cache errors"). -Both are off by default, so an app that does not set them has no change. +Both are off by default. + +### The cleanup delete in fetch no longer raises + +When `fetch` gets a nil or empty value (without `:allow_nil` or +`:allow_empty`), it deletes the entry. Before 1.4.0, an error from that delete +went to the caller, and the caller lost the value from the remote service. +In 1.4.0, `fetch` ignores that error and returns the value. The nil or empty +entry stays in the cache until its TTL ends. This applies with or without +`:handle_cache_error`. An app on the internal `0.3.0.rc2` release can move to 1.4.0 and keep its initializer. 1.4.0 does not add the rest of that release: diff --git a/lib/active_remote/cached/cache.rb b/lib/active_remote/cached/cache.rb index 23f7d29..ad5c940 100644 --- a/lib/active_remote/cached/cache.rb +++ b/lib/active_remote/cached/cache.rb @@ -28,11 +28,11 @@ def initialize(new_cache_provider) super(@cache_provider) end + # The nested cache and the cache provider each get their own failsafe, so + # a handled error in one does not skip the other. def delete(*args) - failsafe do - nested_cache_provider.delete(*args) - super - end + failsafe { nested_cache_provider.delete(*args) } + failsafe { super } end def enable_nested_caching! @@ -44,7 +44,8 @@ def nested_caching? end def exist?(*args) - failsafe(:returning => false) { nested_cache_provider.exist?(*args) || super } + failsafe(:returning => false) { nested_cache_provider.exist?(*args) } || + failsafe(:returning => false) { super } end # An error from the block (the RPC call) always goes to the caller. Only @@ -56,25 +57,23 @@ def fetch(name, options = {}, &block) provider_options = provider_fetch_options(options) fetch_value = provider_fetch(name, provider_options, &block_result.to_block) - delete(name) if delete_after_fetch?(fetch_value, options, provider_options) + delete_quietly(name) if delete_after_fetch?(fetch_value, options, provider_options) fetch_value rescue StandardError => e - raise if block_result.raised?(e) - - handle_or_reraise_cache_error(e) + # #value raises the block error again, so a block error goes to the + # caller as it was raised. + handle_or_reraise_cache_error(e) unless block_result.raised?(e) block_result.value end def read(*args) - failsafe { nested_cache_provider.read(*args) || super } + failsafe { nested_cache_provider.read(*args) } || failsafe { super } end def write(*args) - failsafe do - nested_cache_provider.write(*args) - super - end + failsafe { nested_cache_provider.write(*args) } + failsafe { super } end private @@ -102,8 +101,12 @@ def to_block proc { value } if @block end + # True for the block error itself, and for an error that a provider + # raised while it rescued the block error (Ruby sets it as the cause). def raised?(error) - @error.equal?(error) + return false if @error.nil? || error.nil? + + error.equal?(@error) || raised?(error.cause) end private @@ -117,8 +120,26 @@ def run end private_constant :FetchBlockResult + # An error from the nested cache is handled here, and the cache provider + # is still used. An error from the cache provider or the block goes to + # #fetch. def provider_fetch(name, options, &block) - nested_cache_provider.fetch(name, options) { cache_provider.fetch(name, options, &block) } + provider_result = FetchBlockResult.new(proc { cache_provider.fetch(name, options, &block) }) + + begin + nested_cache_provider.fetch(name, options, &provider_result.to_block) + rescue StandardError => e + handle_or_reraise_cache_error(e) unless provider_result.raised?(e) + provider_result.value + end + end + + # Removes a nil or empty value after #fetch. If the delete fails, the + # value stays until its TTL ends, so the error never fails the #fetch. + def delete_quietly(name) + delete(name) + rescue StandardError + nil end def failsafe(returning: nil) @@ -131,8 +152,16 @@ def failsafe(returning: nil) def handle_or_reraise_cache_error(error) raise error unless ::ActiveRemote::Cached.default_options[:handle_cache_error] + call_cache_error_proc(error) + end + + # A handled cache error must not fail the call, so an error from the proc + # (for example, a notifier that is down) is only reported. + def call_cache_error_proc(error) error_proc = ::ActiveRemote::Cached.default_options[:cache_error_proc] error_proc.call(error) if error_proc.respond_to?(:call) + rescue StandardError => e + warn("ActiveRemote::Cached ignored an error from :cache_error_proc: #{e.class}: #{e.message}") end # :skip_nil tells the provider not to write a nil at all, which saves a diff --git a/spec/active_remote/cached/cache_spec.rb b/spec/active_remote/cached/cache_spec.rb index c02ec45..4bd50a9 100644 --- a/spec/active_remote/cached/cache_spec.rb +++ b/spec/active_remote/cached/cache_spec.rb @@ -232,6 +232,35 @@ def fetch(*) cache.fetch('key', :handle_cache_error => true, :cache_error_proc => lambda {}) { :value } end + it 'returns the block value when :cache_error_proc raises' do + ::ActiveRemote::Cached.default_options(:cache_error_proc => lambda { |_| raise 'notifier down' }) + provider = Class.new(::ActiveSupport::Cache::MemoryStore) do + def fetch(*) + yield + raise ::IOError, 'write after fetch failed' + end + end.new + cache = ::ActiveRemote::Cached::Cache.new(provider) + + expect { expect(cache.fetch('key') { :value }).to eq(:value) } + .to output(/ignored an error from :cache_error_proc: RuntimeError: notifier down/).to_stderr + end + + it 'does not report a block error that the provider wraps in a new error' do + provider = Class.new(::ActiveSupport::Cache::MemoryStore) do + def fetch(*) + yield + rescue StandardError => e + raise ::IOError, "wrapped #{e.class}" + end + end.new + cache = ::ActiveRemote::Cached::Cache.new(provider) + not_found = ::ActiveRemote::RemoteRecordNotFound + + expect { cache.fetch('key') { raise not_found } }.to raise_error(not_found) + expect(handled_errors).to be_empty + end + it 'handles the error without a :cache_error_proc' do ::ActiveRemote::Cached.default_options_overwrite(:handle_cache_error => true) @@ -239,7 +268,70 @@ def fetch(*) end end + context 'with nested caching' do + let(:backing_provider) { ::ActiveSupport::Cache::MemoryStore.new } + let(:cache) do + ::ActiveRemote::Cached::Cache.new(backing_provider).tap(&:enable_nested_caching!) + end + let(:nested_provider) { cache.send(:nested_cache_provider) } + + before do + ::ActiveRemote::Cached.default_options( + :handle_cache_error => true, + :cache_error_proc => lambda { |error| handled_errors << error.message } + ) + %i[delete exist? fetch read write].each do |method_name| + allow(nested_provider).to receive(method_name).and_raise(::IOError, "nested #{method_name} failed") + end + end + + it 'still writes to and deletes from the cache provider when the nested cache fails' do + cache.write('key', 'value') + expect(backing_provider.read('key')).to eq('value') + + cache.delete('key') + expect(backing_provider.exist?('key')).to eq(false) + expect(handled_errors).to eq(['nested write failed', 'nested delete failed']) + end + + it 'still reads from the cache provider when the nested cache fails' do + backing_provider.write('key', 'value') + + expect(cache.read('key')).to eq('value') + expect(cache.exist?('key')).to eq(true) + end + + it 'still fetches through the cache provider when the nested cache fails' do + calls = 0 + + expect(cache.fetch('key') { calls += 1 }).to eq(1) + expect(cache.fetch('key') { calls += 1 }).to eq(1) + expect(calls).to eq(1) + expect(handled_errors).to eq(['nested fetch failed', 'nested fetch failed']) + end + + it 'raises a block error from inside the nested fetch, and does not report it' do + allow(nested_provider).to receive(:fetch).and_call_original + not_found = ::ActiveRemote::RemoteRecordNotFound + + expect { cache.fetch('key') { raise not_found } }.to raise_error(not_found) + expect(handled_errors).to be_empty + end + end + context 'when :handle_cache_error is false' do + it 'returns the fetched value when the cleanup delete fails' do + ::ActiveRemote::Cached.default_options(:handle_cache_error => false) + provider = Class.new(::ActiveSupport::Cache::MemoryStore) do + def delete(*) + raise ::IOError, 'delete failed' + end + end.new + cache = ::ActiveRemote::Cached::Cache.new(provider) + + expect(cache.fetch('key') { [] }).to eq([]) + end + it 'raises the error and does not call :cache_error_proc' do ::ActiveRemote::Cached.default_options( :handle_cache_error => false,