diff --git a/lib/cache.rb b/lib/cache.rb index 313fc418a83..15cd79e6047 100644 --- a/lib/cache.rb +++ b/lib/cache.rb @@ -82,9 +82,13 @@ class Cache end key = normalize_key(name) - raw = redis.get(key) if !force - entry = read_entry(key) if raw - return entry if raw && !(entry == :__corrupt_cache__) + + if !force + if raw = redis.get(key) + entry = decode_entry(raw, key) + return entry if entry != :__corrupt_cache__ + end + end val = blk.call write_entry(key, val, expires_in: expires_in) @@ -99,10 +103,8 @@ class Cache Discourse.warn_exception(e, message: "Corrupt cache... skipping entry for key #{key}") end - def read_entry(key) - if data = redis.get(key) - Marshal.load(data) # rubocop:disable Security/MarshalLoad - end + def decode_entry(raw, key) + Marshal.load(raw) # rubocop:disable Security/MarshalLoad rescue => e # corrupt cache, this can happen if Marshal version # changes. Log it once so we can tell it is happening. @@ -112,6 +114,12 @@ class Cache :__corrupt_cache__ end + def read_entry(key) + if data = redis.get(key) + decode_entry(data, key) + end + end + def write_entry(key, value, expires_in: nil) dumped = Marshal.dump(value) expiry = expires_in || MAX_CACHE_AGE diff --git a/spec/lib/cache_spec.rb b/spec/lib/cache_spec.rb index 96d497ebeb4..4f08048a64d 100644 --- a/spec/lib/cache_spec.rb +++ b/spec/lib/cache_spec.rb @@ -121,5 +121,15 @@ RSpec.describe Cache do expect(cache.read("my_key")).to eq("bob") end end + + it "isn't prone to a race condition due to key expiring between GET calls" do + key = cache.normalize_key("my_key") + + # while this is not technically testing the race condition, it's + # ensuring we're only calling redis.get once, which is a good enough proxy + Discourse.redis.stubs(:get).with(key).returns(Marshal.dump("bob")).once + + expect(fetch_value).to eq("bob") + end end end