Skip to content

[FSSDK-11158] update: add remove method in LRU Cache for CMAB service #366

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[FSSDK-11158] update: add remove method in LRU Cache for CMAB service #366

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
de61931
update: Extend LRUCache with remove method and corresponding tests
FarhanAnjum-opti Jul 1, 2025
01e3a9f
update: Clean up whitespace in LRUCache implementation and tests
FarhanAnjum-opti Jul 1, 2025
40d0571
update: Extend copyright notice to include 2025
FarhanAnjum-opti Jul 1, 2025
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
15 changes: 14 additions & 1 deletion lib/optimizely/odp/lru_cache.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# frozen_string_literal: true

#
# Copyright 2022, Optimizely and contributors
# Copyright 2022-2025, Optimizely and contributors
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -91,6 +91,19 @@ def peek(key)

@cache_mutex.synchronize { @map[key]&.value }
end

# Remove the element associated with the provided key from the cache
#
# @param key - The key to remove

def remove(key)
return if @capacity <= 0

@cache_mutex.synchronize do
@map.delete(key)
Copy link
Preview

Copilot AI Jul 9, 2025

Choose a reason for hiding this comment

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

The remove method deletes the entry from the map but does not unlink the corresponding node from the internal doubly linked list, which can lead to stale nodes and incorrect eviction behavior. Consider retrieving the node before deletion and performing proper list unlinking (updating prev and next pointers) within the synchronized block.

Suggested change
@map.delete(key)
element = @map.delete(key)
return nil unless element
# Unlink the node from the doubly linked list
if element.prev
element.prev.next = element.next
end
if element.next
element.next.prev = element.prev
end

Copilot uses AI. Check for mistakes.

end
nil
end
end

class CacheElement
Expand Down
82 changes: 81 additions & 1 deletion spec/odp/lru_cache_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# frozen_string_literal: true

#
# Copyright 2022, Optimizely and contributors
# Copyright 2022-2025, Optimizely and contributors
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -149,4 +149,84 @@
cache.save('cow', 'crate')
expect(cache.lookup('cow')).to eq 'crate'
end

it 'should remove existing key' do
cache = Optimizely::LRUCache.new(3, 1000)

cache.save('1', 100)
cache.save('2', 200)
cache.save('3', 300)

expect(cache.lookup('1')).to eq 100
expect(cache.lookup('2')).to eq 200
expect(cache.lookup('3')).to eq 300

cache.remove('2')

expect(cache.lookup('1')).to eq 100
expect(cache.lookup('2')).to be_nil
expect(cache.lookup('3')).to eq 300
end

it 'should handle removing non-existent key' do
cache = Optimizely::LRUCache.new(3, 1000)
cache.save('1', 100)
cache.save('2', 200)

cache.remove('3') # Doesn't exist

expect(cache.lookup('1')).to eq 100
expect(cache.lookup('2')).to eq 200
end

it 'should handle removing from zero sized cache' do
cache = Optimizely::LRUCache.new(0, 1000)
cache.save('1', 100)
cache.remove('1')

expect(cache.lookup('1')).to be_nil
end

it 'should handle removing and adding back a key' do
cache = Optimizely::LRUCache.new(3, 1000)
cache.save('1', 100)
cache.save('2', 200)
cache.save('3', 300)

cache.remove('2')
cache.save('2', 201)

expect(cache.lookup('1')).to eq 100
expect(cache.lookup('2')).to eq 201
expect(cache.lookup('3')).to eq 300
end

it 'should handle thread safety' do
max_size = 100
cache = Optimizely::LRUCache.new(max_size, 1000)

(1..max_size).each do |i|
cache.save(i.to_s, i * 100)
end

threads = []
(1..(max_size / 2)).each do |i|
thread = Thread.new do
cache.remove(i.to_s)
end
threads << thread
end

threads.each(&:join)

(1..max_size).each do |i|
if i <= max_size / 2
expect(cache.lookup(i.to_s)).to be_nil
else
expect(cache.lookup(i.to_s)).to eq(i * 100)
end
end

expect(cache.instance_variable_get('@map').size).to eq(max_size / 2)
Copy link
Preview

Copilot AI Jul 9, 2025

Choose a reason for hiding this comment

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

[nitpick] The test inspects the private @map instance variable directly. It’s more maintainable to expose a public method (e.g., size or count) on LRUCache for checking the number of entries, rather than relying on internal state.

Suggested change
expect(cache.instance_variable_get('@map').size).to eq(max_size / 2)
expect(cache.size).to eq(max_size / 2)

Copilot uses AI. Check for mistakes.

end
end