Skip to content
This repository
Browse code

Fix deletion of empty directories:

1. When comparing the directory to delete against the top level
   cache_path, use File.realpath to make sure we aren't comparing two
   unequal strings that point to the same path. This occurs, for
   example, when cache_path has a trailing slash, which it does in the
   default Rails configuration. Since the input to
   delete_empty_directories never has a trailing slash, the comparison
   will never be true and the top level cache directory (and above) may
   be deleted. However…

2. File.delete raises EPERM when trying to delete a directory, so no
   directories have ever been deleted. Changing the code to Dir.delete
   fixes that.
  • Loading branch information...
commit b8837066dcaa389c00aaf22565bb6988ded3932e 1 parent 202041e
Charles Jones chuckbjones authored
2  activesupport/CHANGELOG.md
Source Rendered
... ... @@ -1,5 +1,7 @@
1 1 ## Rails 4.0.0.beta1 (February 25, 2013) ##
2 2
  3 +* Fix deletion of empty directories in ActiveSupport::Cache::FileStore. *Charles Jones*
  4 +
3 5 * Prevent `DateTime#change` from truncating the second fraction, when seconds
4 6 do not need to be changed.
5 7
4 activesupport/lib/active_support/cache/file_store.rb
@@ -150,9 +150,9 @@ def file_path_key(path)
150 150
151 151 # Delete empty directories in the cache.
152 152 def delete_empty_directories(dir)
153   - return if dir == cache_path
  153 + return if File.realpath(dir) == File.realpath(cache_path)
154 154 if Dir.entries(dir).reject {|f| EXCLUDED_DIRS.include?(f)}.empty?
155   - File.delete(dir) rescue nil
  155 + Dir.delete(dir) rescue nil
156 156 delete_empty_directories(File.dirname(dir))
157 157 end
158 158 end
12 activesupport/test/caching_test.rb
@@ -672,6 +672,18 @@ def test_delete_matched_when_cache_directory_does_not_exist
672 672 end
673 673 end
674 674
  675 + def test_delete_does_not_delete_empty_parent_dir
  676 + sub_cache_dir = File.join(cache_dir, 'subdir/')
  677 + sub_cache_store = ActiveSupport::Cache::FileStore.new(sub_cache_dir)
  678 + assert_nothing_raised(Exception) do
  679 + assert sub_cache_store.write('foo', 'bar')
  680 + assert sub_cache_store.delete('foo')
  681 + end
  682 + assert File.exist?(cache_dir), "Parent of top level cache dir was deleted!"
  683 + assert File.exist?(sub_cache_dir), "Top level cache dir was deleted!"
  684 + assert Dir.entries(sub_cache_dir).reject {|f| ActiveSupport::Cache::FileStore::EXCLUDED_DIRS.include?(f)}.empty?
  685 + end
  686 +
675 687 def test_log_exception_when_cache_read_fails
676 688 File.expects(:exist?).raises(StandardError, "failed")
677 689 @cache.send(:read_entry, "winston", {})

0 comments on commit b883706

Please sign in to comment.
Something went wrong with that request. Please try again.