Concurrency issue in MemoryCache
Closed this issue · 0 comments
madejejej commented
Hi!
We're running DeviceDetector in Sidekiq and under high load we sometimes are able to get NoMethodError: undefined method '[]' for nil:NilClass
from:
"/webapps/api/vendor/bundle/ruby/2.6.0/gems/device_detector-1.0.1/lib/device_detector/os.rb" line 11 in short_name
"/webapps/api/vendor/bundle/ruby/2.6.0/gems/device_detector-1.0.1/lib/device_detector.rb" line 66 in device_type
That's basically a MemoryCache#get_or_set
call which returns nil.
Here's the code for reference:
device_detector/lib/device_detector/memory_cache.rb
Lines 30 to 39 in f30a385
My belief what happens is:
- The cache is full and it will be purged on the next write
- Thread 1 In line 33 we check if the given key K1 exists
- Thread 2 in the meantime calls
cache.set('k2', 1)
and purges the cache. K1 was an old key and got purged - Thread 1 tries to
get(K1)
and returns, but there is no such key now so it returnsnil
Here's a script that can replicate it:
require 'device_detector'
cache = DeviceDetector::MemoryCache.new(max_cache_keys: 3)
# fill the cache 100% capacity
cache.set(1, 1)
cache.set(2, 2)
cache.set(3, 3)
# now on, the oldest keys should be purged on each write
sampler = (1..4).to_a
threads = 20.times.map do |thread_id|
Thread.new do
1_000_000.times do |i|
val = sampler.sample
ret = cache.get_or_set(val, val)
puts "[Thread #{thread_id}] BOOM" if ret.nil?
end
end
end
threads.each(&:join)
A simple solution would be to remove the check key?(string_key)
and just get
the value and return it if it's non-nil.