From 59aab09a52b63113698ee042c91719e0518901a3 Mon Sep 17 00:00:00 2001
From: Joe Rafaniello <jrafanie@redhat.com>
Date: Tue, 18 Jul 2017 17:29:33 -0400
Subject: [PATCH] Only return the cached value if it exists.

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 https://github.com/ManageIQ/manageiq-gems-pending/pull/244
---
 .../pending/util/extensions/miq-module.rb     |  5 ++--
 spec/util/extensions/miq-module_spec.rb       | 27 +++++++++++++++++++
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/lib/gems/pending/util/extensions/miq-module.rb b/lib/gems/pending/util/extensions/miq-module.rb
index ade1c2e45..e04810cda 100644
--- a/lib/gems/pending/util/extensions/miq-module.rb
+++ b/lib/gems/pending/util/extensions/miq-module.rb
@@ -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
 
diff --git a/spec/util/extensions/miq-module_spec.rb b/spec/util/extensions/miq-module_spec.rb
index 9ecb93703..c72e2c58c 100644
--- a/spec/util/extensions/miq-module_spec.rb
+++ b/spec/util/extensions/miq-module_spec.rb
@@ -41,6 +41,33 @@
       expect(test_module.default).to eq(value)
     end
 
+    it 'clears the cache even if the block raises an exception' do
+      test_class.define_singleton_method(:call_count)  { @call_count ||= 0 }
+      test_class.define_singleton_method(:call_count=) { |val| @call_count = val }
+      test_class.cache_with_timeout(:flapping_method) do
+        begin
+          if test_class.call_count.even?
+            test_class.call_count
+          else
+            raise "OOPS"
+          end
+        ensure
+          test_class.call_count += 1
+        end
+      end
+
+      expect(test_class.flapping_method(true)).to eq(0) # initializes cache to 0
+      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
+      expect(test_class.flapping_method).to eq(2)       # returns from cache
+
+      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
+      expect(test_class.flapping_method).to eq(4)       # returns from cache
+    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)