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.

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

Jump to bottom

Memory cache off-by-one error #396

Merged
merged 4 commits into from
Feb 13, 2018
Merged

Memory cache off-by-one error #396

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
615e3bb
Fixes an off by one in the handling of the in memory cache, which als…
Feb 12, 2018
2e9ad5f
Fixed InfoCache getter bypassing size restrictions of the RAM cache a…
Feb 12, 2018
96e54e9
Minor effeciency improvement - only read from disk once in the case w…
Feb 12, 2018
a02fcca
per conversation with @bcail in #396 - Avoiding writing to disk on ev…
Feb 12, 2018
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
14 changes: 7 additions & 7 deletions loris/img_info.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,8 +372,7 @@ def get(self, request):
info_and_lastmod = (info, lastmod)
logger.debug('Info for %s read from file system', request)
# into mem:
self._dict[request.url] = info_and_lastmod

self.__setitem__(request, info)
Copy link
Contributor

Choose a reason for hiding this comment

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

setitem writes the content to the file system as well as memory, doesn't it? Don't we only want this information to go into memory in this get method?

Maybe we should split setitem up into two methods, one for writing to disk and one for putting in memory?

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, I see your reasoning here. Writing on every get() would be a lot of writing to disk.

In this case I would be more inclined to tack a kwarg into __setitem__ for this purpose. The __setitem__ and __getitem__ dict-style interface is what is meant to be presented to "the outside world" from this class, correct?

If that is the case (and the cache is meant to be swappable in the wider code base by purpose-built implementations which expose the same interface), I would think that this case would be the only one where bootstrapping something into RAM that already exists on the filesystem would be required - all other cases would require being written both to disk and into RAM, both within the class and in calling code.

return info_and_lastmod

def has_key(self, request):
Expand Down Expand Up @@ -409,11 +408,12 @@ def __setitem__(self, request, info):
logger.debug('Created %s', icc_fp)

# into mem
lastmod = datetime.utcfromtimestamp(os.path.getmtime(info_fp))
with self._lock:
while len(self._dict) >= self.size:
self._dict.popitem(last=False)
self._dict[request.url] = (info,lastmod)
if self.size > 0:
lastmod = datetime.utcfromtimestamp(os.path.getmtime(info_fp))
with self._lock:
self._dict[request.url] = (info,lastmod)
while len(self._dict) > self.size:
self._dict.popitem(last=False)

def __delitem__(self, request):
with self._lock:
Expand Down
24 changes: 24 additions & 0 deletions tests/img_info_t.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,30 @@ def test_empty_cache_has_zero_size(self):
cache = img_info.InfoCache(root=self.SRC_IMAGE_CACHE)
assert len(cache) == 0

def test_cache_limit(self):
cache = img_info.InfoCache(root=self.SRC_IMAGE_CACHE, size=2)
self.app.info_cache = cache
request_uris = [
'/%s/%s' % (self.test_jp2_color_id,'info.json'),
'/%s/%s' % (self.test_jpeg_id,'info.json'),
'/%s/%s' % (self.test_png_id,'info.json'),
'/%s/%s' % (self.test_jp2_gray_id,'info.json')
]
for x in request_uris:
resp = self.client.get(x)

# Check we only cache two
assert len(self.app.info_cache) == 2

def test_no_cache(self):
cache = img_info.InfoCache(root=self.SRC_IMAGE_CACHE, size=0)
self.app.info_cache = cache
request_uri = '/%s/%s' % (self.test_jp2_color_id,'info.json')
resp = self.client.get(request_uri)
print(self.app.info_cache._dict)

assert len(self.app.info_cache) == 0

def test_deleting_cache_item_removes_color_profile_fp(self):
# First assemble the cache
cache, req = self._cache_with_request()
Expand Down