Cleanup ActiveSupport::Cache::ThreadSafety module and add test coverage

activesupport/lib/active_support/cache.rb

@@ -21,7 +21,7 @@ def self.expand_cache_key(key, namespace = nil)
       expanded_cache_key = namespace ? "#{namespace}/" : ""
 
       if ENV["RAILS_CACHE_ID"] || ENV["RAILS_APP_VERSION"]
-        expanded_cache_key << "#{ENV["RAILS_CACHE_ID"] || ENV["RAILS_APP_VERSION"]}/" 
+        expanded_cache_key << "#{ENV["RAILS_CACHE_ID"] || ENV["RAILS_APP_VERSION"]}/"
       end
 
       expanded_cache_key << case
@@ -40,13 +40,8 @@ def self.expand_cache_key(key, namespace = nil)
     class Store
       cattr_accessor :logger
 
-      def initialize
-      end
-
       def threadsafe!
-        @mutex = Mutex.new
-        self.class.send :include, ThreadSafety
-        self
+        extend ThreadSafety
       end
 
       # Pass <tt>:force => true</tt> to force a cache miss.
@@ -110,29 +105,24 @@ def decrement(key, amount = 1)
           nil
         end
       end
-      
+
       private
         def log(operation, key, options)
           logger.debug("Cache #{operation}: #{key}#{options ? " (#{options.inspect})" : ""}") if logger && !@logger_off
         end
     end
 
-
     module ThreadSafety #:nodoc:
-      def read(key, options = nil) #:nodoc:
-        @mutex.synchronize { super }
+      def self.extended(object) #:nodoc:
+        object.instance_variable_set(:@mutex, Mutex.new)
       end
 
-      def write(key, value, options = nil) #:nodoc:
-        @mutex.synchronize { super }
-      end
-
-      def delete(key, options = nil) #:nodoc:
-        @mutex.synchronize { super }
-      end
-
-      def delete_matched(matcher, options = nil) #:nodoc:
-        @mutex.synchronize { super }
+      %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

activesupport/test/caching_test.rb

@@ -70,3 +70,70 @@ 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