diff --git a/README.md b/README.md index 97553c4..d8c5948 100644 --- a/README.md +++ b/README.md @@ -100,6 +100,42 @@ 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`. 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 +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. If the proc raises, the library writes a warning to stderr, and the call continues. | + +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`. + +`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) @@ -167,6 +203,32 @@ 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. + +### 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: + +- `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/cache.rb b/lib/active_remote/cached/cache.rb index 187098e..ad5c940 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) @@ -24,9 +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) - nested_cache_provider.delete(*args) - super + failsafe { nested_cache_provider.delete(*args) } + failsafe { super } end def enable_nested_caching! @@ -38,35 +44,131 @@ def nested_caching? end def exist?(*args) - nested_cache_provider.exist?(*args) || super + failsafe(:returning => false) { nested_cache_provider.exist?(*args) } || + failsafe(:returning => false) { super } 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_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 + # #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) - nested_cache_provider.read(*args) || super + failsafe { nested_cache_provider.read(*args) } || failsafe { super } end def write(*args) - nested_cache_provider.write(*args) - super + failsafe { nested_cache_provider.write(*args) } + failsafe { super } 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 @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 + + # 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) + return false if @error.nil? || error.nil? + + error.equal?(@error) || raised?(error.cause) + end + + private + + def run + @ran = true + @value = @block&.call + rescue StandardError => e + @error = e + end + 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) + 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) + yield + rescue StandardError => e + handle_or_reraise_cache_error(e) + returning + end + + 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 # 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/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 diff --git a/spec/active_remote/cached/cache_spec.rb b/spec/active_remote/cached/cache_spec.rb index 8defcf7..4bd50a9 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,220 @@ 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, 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 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 + 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 '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) + + expect(cache.read('key')).to be_nil + 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, + :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..08714ef 100644 --- a/spec/active_remote/cached_spec.rb +++ b/spec/active_remote/cached_spec.rb @@ -164,6 +164,51 @@ 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 + 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(