Fix FileStore#cleanup to no longer rely on missing each_key method #12196

Merged
merged 1 commit into from Sep 12, 2013

Conversation

Projects
None yet
3 participants
Contributor

h-lame commented Sep 11, 2013

This is based on the work by @bdurand in #3395, but I just want it to work as it apparently used to and remove all expired entries rather than extend the functionality. So, we just replace each_key with some search_dir magic, rather than adding the :not_accessed_in option.

Contributor

h-lame commented Sep 11, 2013

Assuming this is ok to go in master, what's the process for getting it backported to 3.2.x as well?

Contributor

pftg commented Sep 11, 2013

Rails committers will backport if needed.

Please add Changelog and use 1.9 hash style in the code.

Contributor

pftg commented Sep 11, 2013

Also, I confirmed, that your tests shows bug with each_key

Contributor

h-lame commented Sep 11, 2013

@pftg - Thanks. I've rebased with your requested changes.

@pftg pftg and 1 other commented on an outdated diff Sep 11, 2013

activesupport/test/caching_test.rb
@@ -709,6 +709,18 @@ def test_log_exception_when_cache_read_fails
@cache.send(:read_entry, "winston", {})
assert @buffer.string.present?
end
+
+ def test_cleanup_removes_all_expired_entries
+ time = Time.now
+ @cache.write('foo', 'bar', expires_in: 10)
+ @cache.write('baz', 'quz')
+ @cache.write('quux', 'corge', expires_in: 20)
+ Time.stubs(:now).returns(time + 15)
+ @cache.cleanup
+ refute @cache.exist?('foo')
@pftg

pftg Sep 11, 2013

Contributor

do not use refute, instead use assert_not - just for rails conventions

@h-lame

h-lame Sep 11, 2013

Contributor

Ah, right! Thanks for spotting. It's been a while since I last did any rails bugs, I should re-read the contributors guide and conventions.

Contributor

pftg commented Sep 11, 2013

👍

@rafaelfranca rafaelfranca added a commit that referenced this pull request Sep 12, 2013

@rafaelfranca rafaelfranca Merge pull request #12196 from h-lame/fix-activesupport-cache-filesto…
…re-cleanup

Fix FileStore#cleanup to no longer rely on missing each_key method
cdc10c8

@rafaelfranca rafaelfranca merged commit cdc10c8 into rails:master Sep 12, 2013

1 check passed

default The Travis CI build passed
Details

@rafaelfranca rafaelfranca added a commit that referenced this pull request Sep 12, 2013

@rafaelfranca rafaelfranca Merge pull request #12196 from h-lame/fix-activesupport-cache-filesto…
…re-cleanup

Fix FileStore#cleanup to no longer rely on missing each_key method
Conflicts:
	activesupport/CHANGELOG.md
d107a84

@rafaelfranca rafaelfranca added a commit that referenced this pull request Sep 12, 2013

@rafaelfranca rafaelfranca Merge pull request #12196 from h-lame/fix-activesupport-cache-filesto…
…re-cleanup

Fix FileStore#cleanup to no longer rely on missing each_key method
Conflicts:
	activesupport/CHANGELOG.md
	activesupport/test/caching_test.rb
c539c68
Owner

rafaelfranca commented Sep 12, 2013

Thank you. Backported to all the supported branches

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment