Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add cleanup block to FastCache::Cache, with RSpec tests. #3

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions lib/fast_cache/cache.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ def initialize(max_size, ttl, expire_interval = 100)
@op_count = 0
@data = {}
@expires_at = {}
@cleanup = Proc.new if block_given?
Copy link
Contributor

Choose a reason for hiding this comment

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

Please provide the block as an &param. This type of defaulting is one of the obscure features of Ruby that saves a few characters at the expense of readability.

end

# Retrieves a value from the cache, if available and not expired, or
Expand Down Expand Up @@ -84,6 +85,7 @@ def delete(key)
entry = @data.delete(key)
if entry
@expires_at.delete(entry)
@cleanup.call(entry.value) if @cleanup
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the semantics of returning a value that has been cleaned up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. As implemented, the value will still be returned here after the cleanup block has been called. I think that's ok, as long as the user is aware that's the case -- if they are invalidating the object in some way with their cleanup block, they should not treat the returned value here as something usable. (But, might still be useful for identity comparison or something like that.)

An alternative, I suppose, could be to return nil here if there is a cleanup block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or, we could change it to not call the cleanup block here? That would seem inconsistent to me, though.

entry.value
else
nil
Expand All @@ -103,6 +105,7 @@ def empty?
#
# @return [self]
def clear
@data.values.map(&:value).map(&@cleanup) if @cleanup
Copy link
Contributor

Choose a reason for hiding this comment

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

This operation is unnecessarily inefficient:

  1. values has to traverse the hash and create an array
  2. map traverses the array and creates another array
  3. map traverses that array and creates another array that is ignored

What you want here is a single each_value loop.

@data.clear
@expires_at.clear
self
Expand Down Expand Up @@ -176,6 +179,7 @@ def get(key)
if found
if entry.expires_at <= t
@expires_at.delete(entry)
@cleanup.call(entry.value) if @cleanup
return false, nil
else
@data[key] = entry
Expand All @@ -194,7 +198,9 @@ def store(key, val)
end

def store_entry(key, entry)
@data.delete(key)
found = true
old_entry = @data.delete(key) { found = false }
@cleanup.call(old_entry.value) if @cleanup && found
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not check for a nil default value return from delete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's been a while since I wrote this, but if I recall correctly, the reason for this was to properly handle the case where a nil or false value was found in (and deleted from) the cache.

@data[key] = entry
@expires_at[entry] = key
shrink_if_needed
Expand All @@ -204,6 +210,7 @@ def shrink_if_needed
if @data.length > @max_size
entry = delete(@data.shift)
@expires_at.delete(entry)
@cleanup.call(entry.value) if @cleanup
end
end

Expand All @@ -212,7 +219,8 @@ def check_expired(t)
while (key_value_pair = @expires_at.first) &&
(entry = key_value_pair.first).expires_at <= t
key = @expires_at.delete(entry)
@data.delete(key)
entry = @data.delete(key)
@cleanup.call(entry.value) if @cleanup
end
end
end
Expand Down
45 changes: 43 additions & 2 deletions spec/lib/fast_cache/cache_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,19 @@
end
end

context 'non-empty cache' do
shared_context :non_empty_cache do
let(:cache) { described_class.new(3, 60, 1) }
before do
@cache = described_class.new(3, 60, 1)
@cache = cache
@cache[:a] = 1
@cache[:b] = 2
@cache[:c] = 3
end
subject { @cache }
end

context 'non-empty cache' do
include_context :non_empty_cache

its(:empty?) { should be_false }
its(:length) { should eq 3 }
Expand Down Expand Up @@ -112,6 +117,42 @@
subject[:e].should eq 6
end
end

describe 'cleanup block' do
let(:cleanups) { [] }
let(:ttl) { 60 }
let(:cache) { described_class.new(3, ttl, 1) do |obj|
cleanups << obj
end }
it 'is called when the cache is cleared' do
subject.clear
cleanups.should =~ [1,2,3]
end
it 'is called when an item is deleted' do
subject.delete(:a).should eq 1
cleanups.should =~ [1]
end
it 'is called when an existing item is replaced' do
subject[:a] = 11
cleanups.should =~ [1]
end
it 'is called when an item is removed when full' do
subject[:d] = 4
cleanups.should =~ [1]
end

context 'with immediate expiration' do
let(:ttl) { 0 }
it 'is called when items are expired' do
subject.expire!
cleanups.should =~ [1,2,3]
end
it 'is called when item access triggers expiration' do
subject[:a].should be_nil
cleanups.should =~ [1,2,3]
end
end
end
end

describe 'TTL behaviors' do
Expand Down