Skip to content

Commit

Permalink
Only return the cached value if it exists.
Browse files Browse the repository at this point in the history
If the cache was cleared and an exception was raised when trying to set
the new `cache[:value]`, we'd end up setting the `cache[:timeout]` but
not the new value.  Any subsequent call would return a nil from
`cache[:value]` because `cache[:timeout]` exists, making `cache` non-empty.

The new code only returns from the cache if the :value key exists in the
`cache`.

Additionally, for consistency, we only set the `cache[:timeout]`
and `cache[:value]` after we've calculated these values, therefore the
`cache` doesn't get partially set.  This is not required but makes me
feel better.

https://bugzilla.redhat.com/show_bug.cgi?id=1469307
https://bugzilla.redhat.com/show_bug.cgi?id=1468898

This replaces #244
  • Loading branch information
jrafanie committed Jul 19, 2017
1 parent f571790 commit f8bb265
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 2 deletions.
5 changes: 3 additions & 2 deletions lib/gems/pending/util/extensions/miq-module.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,15 @@ def cache_with_timeout(method, timeout = nil, &block)

old_timeout = cache[:timeout]
cache.clear if force_reload || (old_timeout && Time.now.utc > old_timeout)
break cache[:value] unless cache.empty?
break cache[:value] if cache.key?(:value)

new_timeout = timeout || 300 # 5 minutes
new_timeout = new_timeout.call if new_timeout.kind_of?(Proc)
new_timeout = Time.now.utc + new_timeout
new_value = block.call

cache[:timeout] = new_timeout
cache[:value] = block.call
cache[:value] = new_value
end
end

Expand Down
24 changes: 24 additions & 0 deletions spec/util/extensions/miq-module_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,30 @@
expect(test_module.default).to eq(value)
end

it 'clears the cache even if the block raises an exception' do
$call_count = 0
test_class.cache_with_timeout(:flapping_method) do
begin
if $call_count.even?
$call_count
else
raise "OOPS"
end
ensure
$call_count += 1
end
end

test_class.flapping_method(true) # initializes cache to 0
expect(test_class.flapping_method).to eq(0) # returns cached 0

test_class.flapping_method(true) rescue nil # blows up, clears cache for next request
expect(test_class.flapping_method).to eq(2) # sets new value since cache is cleared

test_class.flapping_method(true) rescue nil # blows up, clears cache for next request
expect(test_class.flapping_method).to eq(4) # sets new value since cache is cleared
end

it 'will not return the cached value when passed force_reload = true' do
value = test_class.default(true)
expect(test_class.default(true)).not_to eq(value)
Expand Down

0 comments on commit f8bb265

Please sign in to comment.