mirror of
https://github.com/discourse/discourse.git
synced 2025-05-24 03:36:18 +08:00
FIX: race condition in Discourse.cache.fetch (#28124)
When using `Discourse.cache.fetch` with an expiry, there's a potential for a race condition due to how we read the data from redis. The code used to be ```ruby raw = redis.get(key) if !force entry = read_entry(key) if raw return entry if raw && !(entry == :__corrupt_cache__) ``` with `read_entry` defined as follow ```ruby def read_entry(key) if data = redis.get(key) Marshal.load(data) end rescue => e :__corrupt_cache__ end ``` If the value at "key" expired in redis between `raw = redis.get` and `entry = read_entry`, the `entry` variable would be `nil` despite `raw` having a value. We would then proceed to return `entry` (which is `nil`) thinking it had a value, when it didn't. The first `redis.get` can be skipped altogether and we can rely only on `read_entry` to read the data from redis. Thus avoiding the race condition and removing the double read operations. Internal ref - t/132507
This commit is contained in:
@ -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
|
||||
|
Reference in New Issue
Block a user