loris-imageserver / loris

Loris IIIF Image Server
Other
209 stars 87 forks source link

Memory cache off-by-one error #396

Closed bnbalsamo closed 6 years ago

bnbalsamo commented 6 years ago

Current handling of the in memory cache in InfoCache has an off-by-one in the while loop condition. In cases where size > 0 the cache would (harmlessly, I believe) just be one smaller than stated in the size parameter. In cases where size == 0 we were getting StopIteration exceptions in production, though in my own testing I could only get KeyError: 'dictionary is empty'

This PR changes the while loop condition to strictly >, rather than >=

bcail commented 6 years ago

@bnbalsamo thanks for the PR. Could you add a test for your fix?

bnbalsamo commented 6 years ago

Sorry for multiple commits here - let me know if you'd prefer I squash them and re-open a PR.

My initial assumption was incorrect, there wasn't an off by one error in the loop condition, but the case where RAM cache size == 0 was broken.

To fix this the logic for RAM cache addition is now

add to cache --> trim cache instead of trim cache --> add to cache

There was also a particularly pesky bypassing of the RAM cache size logic in the getter - that now calls __setitem___ internally instead of handling the ._dict itself.

The final commit (which occurred to me in the middle of writing this comment) minimizes disk reads in the event there is no RAM cache.

There are also now tests for limited RAM cache sizes and no RAM cache.

One potential byproduct of this change is that the previously a call to InfoCache.get() would always deposit the request record in RAM, making subsequent calls to InfoCache.get() fast, it is now the case that calls to .get() can repeatedly be reading from disk (even within the same thread), if cache size is set to 0.

I'm not familiar enough with the entirety of the code base to guess whether or not someone relied on repeated calls to InfoCache.get() to be "RAM-fast" instead of "Disk-fast", but figured I should give a heads up, just in case.

bcail commented 6 years ago

@bnbalsamo thanks. Merging.