Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
MemoryStore is the only "unsafe" store. Make it threadsafe by default.
  • Loading branch information
josh committed Aug 6, 2008
1 parent 7305650 commit e5b1ab7
Show file tree
Hide file tree
Showing 5 changed files with 44 additions and 99 deletions.
2 changes: 1 addition & 1 deletion actionpack/lib/action_view/helpers/asset_tag_helper.rb
Expand Up @@ -463,7 +463,7 @@ def image_tag(source, options = {})
end

private
COMPUTED_PUBLIC_PATHS = ActiveSupport::Cache::MemoryStore.new.silence!.threadsafe!
COMPUTED_PUBLIC_PATHS = ActiveSupport::Cache::MemoryStore.new.silence!

# Add the the extension +ext+ if not present. Return full URLs otherwise untouched.
# Prefix with <tt>/dir/</tt> if lacking a leading +/+. Account for relative URL
Expand Down
18 changes: 0 additions & 18 deletions activesupport/lib/active_support/cache.rb
Expand Up @@ -39,10 +39,6 @@ def self.expand_cache_key(key, namespace = nil)
class Store
cattr_accessor :logger

def threadsafe!
extend ThreadSafety
end

def silence!
@silence = true
self
Expand Down Expand Up @@ -115,20 +111,6 @@ def log(operation, key, options)
logger.debug("Cache #{operation}: #{key}#{options ? " (#{options.inspect})" : ""}") if logger && !@silence && !@logger_off
end
end

module ThreadSafety #:nodoc:
def self.extended(object) #:nodoc:
object.instance_variable_set(:@mutex, Mutex.new)
end

%w(read write delete delete_matched exist? increment decrement).each do |method|
module_eval <<-EOS, __FILE__, __LINE__
def #{method}(*args)
@mutex.synchronize { super }
end
EOS
end
end
end
end

Expand Down
55 changes: 43 additions & 12 deletions activesupport/lib/active_support/cache/memory_store.rb
Expand Up @@ -3,36 +3,67 @@ module Cache
class MemoryStore < Store
def initialize
@data = {}
@mutex = Mutex.new
end

def fetch(key, options = {})
@mutex.synchronize do
super
end
end

def read(name, options = nil)
super
@data[name]
@mutex.synchronize do
super
@data[name]
end
end

def write(name, value, options = nil)
super
@data[name] = value
@mutex.synchronize do
super
@data[name] = value
end
end

def delete(name, options = nil)
super
@data.delete(name)
@mutex.synchronize do
super
@data.delete(name)
end
end

def delete_matched(matcher, options = nil)
super
@data.delete_if { |k,v| k =~ matcher }
@mutex.synchronize do
super
@data.delete_if { |k,v| k =~ matcher }
end
end

def exist?(name,options = nil)
super
@data.has_key?(name)
@mutex.synchronize do
super
@data.has_key?(name)
end
end

def increment(key, amount = 1)
@mutex.synchronize do
super
end
end

def decrement(key, amount = 1)
@mutex.synchronize do
super
end
end

def clear
@data.clear
@mutex.synchronize do
@data.clear
end
end
end
end
end
end
67 changes: 0 additions & 67 deletions activesupport/test/caching_test.rb
Expand Up @@ -70,70 +70,3 @@ def test_fetch_with_forced_cache_miss
end
end
end

class ThreadSafetyCacheStoreTest < Test::Unit::TestCase
def setup
@cache = ActiveSupport::Cache.lookup_store(:memory_store).threadsafe!
@cache.write('foo', 'bar')

# No way to have mocha proxy to the original method
@mutex = @cache.instance_variable_get(:@mutex)
@mutex.instance_eval %(
def calls; @calls; end
def synchronize
@calls ||= 0
@calls += 1
yield
end
)
end

def test_read_is_synchronized
assert_equal 'bar', @cache.read('foo')
assert_equal 1, @mutex.calls
end

def test_write_is_synchronized
@cache.write('foo', 'baz')
assert_equal 'baz', @cache.read('foo')
assert_equal 2, @mutex.calls
end

def test_delete_is_synchronized
assert_equal 'bar', @cache.read('foo')
@cache.delete('foo')
assert_equal nil, @cache.read('foo')
assert_equal 3, @mutex.calls
end

def test_delete_matched_is_synchronized
assert_equal 'bar', @cache.read('foo')
@cache.delete_matched(/foo/)
assert_equal nil, @cache.read('foo')
assert_equal 3, @mutex.calls
end

def test_fetch_is_synchronized
assert_equal 'bar', @cache.fetch('foo') { 'baz' }
assert_equal 'fu', @cache.fetch('bar') { 'fu' }
assert_equal 3, @mutex.calls
end

def test_exist_is_synchronized
assert @cache.exist?('foo')
assert !@cache.exist?('bar')
assert_equal 2, @mutex.calls
end

def test_increment_is_synchronized
@cache.write('foo_count', 1)
assert_equal 2, @cache.increment('foo_count')
assert_equal 4, @mutex.calls
end

def test_decrement_is_synchronized
@cache.write('foo_count', 1)
assert_equal 0, @cache.decrement('foo_count')
assert_equal 4, @mutex.calls
end
end
1 change: 0 additions & 1 deletion railties/lib/initializer.rb
Expand Up @@ -776,7 +776,6 @@ def threadsafe!
self.dependency_loading = false
self.active_record.allow_concurrency = true
self.action_controller.allow_concurrency = true
self.to_prepare { Rails.cache.threadsafe! }
self
end

Expand Down

1 comment on commit e5b1ab7

@haberbyte
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea why, but this changeset breaks my app. Probably because I’m using i18n_yaml for translations with the new I18n API…it uses Rails caching mechanism…but now I don’t even get requests back from mongrel. They all magically seem to wait forever…
Not sure if this is a bug caused by this changeset, or a misuse of the cache in that plugin, but since this commit, it breaks…

Please sign in to comment.