Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
18 changes: 15 additions & 3 deletions lib/fast_cache/cache.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,18 @@ class Cache
# the cache.
# @param [Integer] expire_interval Number of cache operations between
# calls to {#expire!}.
def initialize(max_size, ttl, expire_interval = 100)
# @yield [Object] If a block is given, each time an object is removed
# from the cache, it will be yielded to the block. This
# is useful for cleaning up resources used by objects
# stored in the cache.
def initialize(max_size, ttl, expire_interval = 100, &cleanup)
@max_size = max_size
@ttl = ttl.to_f
@expire_interval = expire_interval
@op_count = 0
@data = {}
@expires_at = {}
@cleanup = cleanup
end

# Retrieves a value from the cache, if available and not expired, or
Expand Down Expand Up @@ -84,6 +89,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 +109,7 @@ def empty?
#
# @return [self]
def clear
@data.each_value {|entry| @cleanup.call(entry.value)} if @cleanup
@data.clear
@expires_at.clear
self
Expand Down Expand Up @@ -176,6 +183,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 +202,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 +214,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 +223,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