Skip to content

Commit 8c488bf

Browse files
agrbergclaude
andcommitted
Make MemoryAdapter thread-safe with Monitor
The MemoryAdapter used a plain Hash with no synchronization, causing race conditions in multi-threaded servers (Puma, Sidekiq). fetch did a non-atomic exist?-then-write, allowing concurrent threads to duplicate work or corrupt state. Changes: - Wrap all cache operations with Monitor#synchronize - Make fetch atomic (inline key check + write under one lock) - Add frozen_string_literal pragma - Update tests to assert cache state directly instead of mocking internal write calls Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 9ec5616 commit 8c488bf

2 files changed

Lines changed: 31 additions & 17 deletions

File tree

Lines changed: 22 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,42 +1,50 @@
1+
# frozen_string_literal: true
2+
3+
require 'monitor'
4+
15
module Cacheable
26
module CacheAdapters
37
class MemoryAdapter
48
def initialize
9+
@monitor = Monitor.new
510
clear
611
end
712

813
def read(key)
9-
cache[key]
14+
@monitor.synchronize { @cache[key] }
1015
end
1116

1217
def write(key, value)
13-
cache[key] = value
18+
@monitor.synchronize { @cache[key] = value }
1419
end
1520

1621
def exist?(key)
17-
cache.key?(key)
22+
@monitor.synchronize { @cache.key?(key) }
1823
end
1924

25+
# NOTE: yield is intentionally called inside the lock to prevent thundering herd — only one thread
26+
# computes a missing value while others wait. This is acceptable for a simple in-memory adapter;
27+
# production use cases needing high concurrency should use a real cache backend via CacheAdapter.
2028
def fetch(key, _options = {})
21-
return read(key) if exist?(key)
29+
@monitor.synchronize do
30+
return @cache[key] if @cache.key?(key)
2231

23-
write(key, yield)
32+
@cache[key] = yield
33+
end
2434
end
2535

26-
def delete(key) # rubocop:disable Naming/PredicateMethod -- mimics the ActiveSupport::Cache::Store#delete interface and isn't a predicate
27-
return false unless exist?(key)
36+
def delete(key)
37+
@monitor.synchronize do
38+
return false unless @cache.key?(key)
2839

29-
cache.delete key
30-
true
40+
@cache.delete(key)
41+
true
42+
end
3143
end
3244

3345
def clear
34-
@cache = {}
46+
@monitor.synchronize { @cache = {} }
3547
end
36-
37-
private
38-
39-
attr_reader :cache
4048
end
4149
end
4250
end

spec/cacheable/cacheable_spec.rb

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -366,8 +366,10 @@
366366
cacheable(*local_variable_so_class_eval_works)
367367
end
368368

369-
expect(described_class.cache_adapter).to receive(:write).twice.and_call_original
370369
2.times { cache_methods.each { |method| cacheable_object.send(method) } }
370+
cache_methods.each do |method|
371+
expect(described_class.cache_adapter.exist?([method])).to be true
372+
end
371373
end
372374

373375
it 'uses the same options for cacheable methods declared on a single line' do
@@ -377,8 +379,10 @@
377379
cacheable(*local_variable_so_class_eval_works, unless: proc { true })
378380
end
379381

380-
expect(described_class.cache_adapter).not_to receive(:write)
381382
2.times { cache_methods.each { |method| cacheable_object.send(method) } }
383+
cache_methods.each do |method|
384+
expect(described_class.cache_adapter.exist?([method])).to be false
385+
end
382386
end
383387

384388
it 'can take strings' do
@@ -388,8 +392,10 @@
388392
cacheable(*cache_methods_as_strings)
389393
end
390394

391-
expect(described_class.cache_adapter).to receive(:write).twice.and_call_original
392395
2.times { cache_methods.each { |method| cacheable_object.send(method) } }
396+
cache_methods_as_strings.each do |method|
397+
expect(described_class.cache_adapter.exist?([method])).to be true
398+
end
393399
end
394400

395401
it 'can take strings before the method is defined' do

0 commit comments

Comments
 (0)