onevcat / Kingfisher

A lightweight, pure-Swift library for downloading and caching images from the web.
MIT License
23.13k stars 2.64k forks source link

Disk Cache can grow significantly larger than 'sizeLimit' #2253

Closed hotngui closed 3 months ago

hotngui commented 4 months ago

Check List

Thanks for considering to open an issue. Before you submit your issue, please confirm these boxes are checked.

Issue Description

I was doing some stress testing on devices whose disks are almost full. I noticed that disk cache can grow significantly larger than what I specify via the ImageCache.default.diskStorage.config.sizeLimit until the app is backgrounded or terminated.

What

I took a look at the source code, but could not find anywhere that checks the disk space used by the KF cache against the sizeLimit to keep the cache from growing too large while the app is still in the foreground.

Am I missing some configuration flag or something?

Reproduce

This can be tricky as you need to have a device whose disk is almost full - from another app that is not using the OS cache sandbox since that would not work correctly.

onevcat commented 3 months ago

The current behavior is intentionally designed to check and remove cache files that exceed the size limit only when the app enters the background.

This approach prevents performance issues that could arise from checking the file size every time a new image is cached, which could significantly impact performance.

If needed, you can manually invoke the removeSizeExceededValues method on the disk cache at any time to remove files that exceed the specified size limit.

hotngui commented 3 months ago

Yes, I understand the current behavior was intentional and that it's not practical to check each time. I was thinking more around the idea of call removeSizeExceededValues when you did receive an error on attempting to write to the disk cache or something.

I was not able to find a central location where I could attempt to catch the error myself; or a closure I could register to be called only when that error got thrown. Perhaps I missed something in the code?

I work on several apps that can be heavy on images in long user sessions, so having the cache grow with limits during a single session can be an issue.

onevcat commented 3 months ago

Currently, there actually is an error defined in Kingfisher when writing disk cache failed, as the CacheErrorReason.cannotCreateCacheFile, which is thrown in the ImageCache as in CacheStoreResult.diskCacheResult. However, since when using the KingfisherManager and the view extension methods, the cache process happens in an async way (by default, after the image setting completion handler is called) and will not affect the UI layer, this error is ignored (case 1, case 2).

If you need to implement a way to inspect this, maybe you can try to add a delegate to expose this error out and perform a clean up work there.

Or easier, maybe you can just set up a timer (say perform it every 5 or 10 minutes) to clean it regularly before an issue may happen.

hotngui commented 3 months ago

Or easier, maybe you can just set up a timer (say perform it every 5 or 10 minutes) to clean it regularly before an issue may happen Yeah, this is what I was thinking as well. But wanted to make sure I was not missing a feature that already existed before going that route as it does feel a bit janky.