Only return the cached value if it exists. #247
Conversation
| @@ -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 | |||
There was a problem hiding this comment.
Maybe you can use an instance variable in the test class?
def test_class.call_count # or use an instance_variable_set or whatever
@call_count ||= 0
end
test_class.cache_with_timeout...
|
In the other PR you mentioned a test for 5, raise, raise, etc. Do you think you still need that? |
|
👍 on the code...just need to clean up the specs. |
| end | ||
| end | ||
|
|
||
| test_class.flapping_method(true) # initializes cache to 0 |
There was a problem hiding this comment.
Add an expectation here as well that it's 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 |
There was a problem hiding this comment.
You might want to throw another call after this one to show that it's cached since technically this line is a fresh invocation.
| 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 |
There was a problem hiding this comment.
Same here...you might want a second call to verify the caching.
There was a problem hiding this comment.
haha, I contemplated these too...
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 ManageIQ#244
jrafanie
force-pushed
the
cache_invalidation_is_harder
branch
from
f8bb265 to
59aab09
Compare
|
Checked commit jrafanie@59aab09 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 lib/gems/pending/util/extensions/miq-module.rb
spec/util/extensions/miq-module_spec.rb
|
| expect(test_class.flapping_method).to eq(0) # returns from cache | ||
|
|
||
| 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 |
There was a problem hiding this comment.
In the other PR you mentioned a test for 5, raise, raise, etc. Do you think you still need that?
@Fryguy No. This expectation shows that we are actually setting a new value and returning it, NOT the previously cached value, 0. So, I think this test encompasses the desired test cases.
Only return the cached value if it exists. (cherry picked from commit 6bdd644) https://bugzilla.redhat.com/show_bug.cgi?id=1473787
|
Fine backport details: |
|
@jrafanie The BZs referenced here is for Embedded Ansible. Since that's not available in Euwe branch, I assume this bug will show up in some different way... Can you create a BZ that describes the problem in Euwe? |
|
@simaishi Thanks! I'll remove the euwe label. While it's possible for this bug on euwe, it's very unlikely without the embedded ansible thread worker. |
If the cache was cleared and an exception was raised when trying to set
the new
cache[:value], we'd end up setting thecache[:timeout]butnot the new value. Any subsequent call would return a nil from
cache[:value]becausecache[:timeout]exists, makingcachenon-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 thecachedoesn't get partially set. This is not required but makes mefeel better.
https://bugzilla.redhat.com/show_bug.cgi?id=1469307
https://bugzilla.redhat.com/show_bug.cgi?id=1468898
This replaces #244