Merge pull request #3395 from bdurand/fix_file_store_cleanup

Fix ActiveSupport::Cache::FileStore.cleanup to actually work.

2 files changed, 52 insertions, 3 deletions

diff --git a/activesupport/lib/active_support/cache/file_store.rb b/activesupport/lib/active_support/cache/file_store.rb
index b431041b76..bc5d94b5a7 100644
--- a/activesupport/lib/active_support/cache/file_store.rb
+++ b/activesupport/lib/active_support/cache/file_store.rb
@@ -26,11 +26,26 @@ module ActiveSupport
         FileUtils.rm_r(root_dirs.collect{|f| File.join(cache_path, f)})
       end
 
+      # Cleanup the cache by removing old entries. By default this will delete entries
+      # that haven't been accessed in one day. You can change this behavior by passing
+      # in a +not_accessed_in+ option. Any entry not accessed in that number of seconds
+      # in the past will be deleted. Alternatively, you can pass in +:expired_only+ with
+      # +true+ to only delete expired entries.
       def cleanup(options = nil)
         options = merged_options(options)
-        each_key(options) do |key|
-          entry = read_entry(key, options)
-          delete_entry(key, options) if entry && entry.expired?
+        expired_only = options[:expired_only]
+        timestamp = Time.now - (options[:not_accessed_in] || 1.day.to_i)
+        search_dir(cache_path) do |fname|
+          if expired_only
+            key = file_path_key(fname)
+            entry = read_entry(key, options)
+            delete_entry(key, options) if entry && entry.expired?
+          else
+            if File.atime(fname) <= timestamp
+              key = file_path_key(fname)
+              delete_entry(key, options)
+            end
+          end
         end
       end
 
diff --git a/activesupport/test/caching_test.rb b/activesupport/test/caching_test.rb
index cb5362525f..1b2f6d061c 100644
--- a/activesupport/test/caching_test.rb
+++ b/activesupport/test/caching_test.rb
@@ -557,6 +557,29 @@ class FileStoreTest < ActiveSupport::TestCase
     key = @cache_with_pathname.send(:key_file_path, "views/index?id=1")
     assert_equal "views/index?id=1", @cache_with_pathname.send(:file_path_key, key)
   end
+
+  def test_cleanup_with_not_accessed_in
+    @cache.write(1, "aaaaaaaaaa")
+    @cache.write(2, "bbbbbbbbbb")
+    @cache.write(3, "cccccccccc")
+    sleep(2)
+    @cache.read(2)
+    @cache.cleanup(:not_accessed_in => 1)
+    assert_equal false, @cache.exist?(1)
+    assert_equal true, @cache.exist?(2)
+    assert_equal false, @cache.exist?(3)
+  end
+
+  def test_cleanup_with_expired_only
+    @cache.write(1, "aaaaaaaaaa", :expires_in => 0.001)
+    @cache.write(2, "bbbbbbbbbb")
+    @cache.write(3, "cccccccccc", :expires_in => 0.001)
+    sleep(0.002)
+    @cache.cleanup(:expired_only => 0.001)
+    assert_equal false, @cache.exist?(1)
+    assert_equal true, @cache.exist?(2)
+    assert_equal false, @cache.exist?(3)
+  end
   
   # Because file systems have a maximum filename size, filenames > max size should be split in to directories
   # If filename is 'AAAAB', where max size is 4, the returned path should be AAAA/B
@@ -646,6 +669,17 @@ class MemoryStoreTest < ActiveSupport::TestCase
     assert_equal true, @cache.exist?(2)
     assert_equal false, @cache.exist?(1)
   end
+
+  def test_cleanup_removes_expired_entries
+    @cache.write(1, "aaaaaaaaaa", :expires_in => 0.001)
+    @cache.write(2, "bbbbbbbbbb")
+    @cache.write(3, "cccccccccc", :expires_in => 0.001)
+    sleep(0.002)
+    @cache.cleanup
+    assert_equal false, @cache.exist?(1)
+    assert_equal true, @cache.exist?(2)
+    assert_equal false, @cache.exist?(3)
+  end
 end
 
 uses_memcached 'memcached backed store' do