Add option to disable memory cache TTL extending

[dstranz]

Use case:

Let's assume that we are having an image that is valid for a limited time. For example, USD/EUR rating chart that should be reloaded every 3 minutes. So we are using .memoryCacheExpiration(.seconds(3 * 60)) option.

It's not worth to cache it on disk, because we will replace it every 3 minutes. So we are using .cacheMemoryOnly option.

What will be the expected result?

After reloading the chart view we will get from Kingfisher cached image if not older than 3 minutes or a fresh one.

What is actual behavior?

After reloading the chart view we will get from Kingfisher cached image if the time between last image load isn't longer than 3 minutes or a fresh one.

Proposed change

In many cases actual behavior is correct. Why not extending TTL time for frequently used user avatar or media that will be the same every time we will load it?

But in other cases when the image should be not older than given TTL time we should add an option to disable extending functionality.

That's the reason why I've added a new option to Kingfisher called .memoryCacheExpirationNotExtendable.

[dstranz]

I haven't got any problems with tests locally. They also pass on most of travis builds but not on all, so they are not reliable.

@onevcat Do you know what will a be better way to test cache TTL extending - probably without delay?

[onevcat]

@dstranz

Thanks for the proposal and implementation!

The CI sometimes does not work well due to some limitation on infrastructure. I will check it later, for both the use case and the tests.

[RomanPodymov]

Hello @dstranz
Try gem "cocoapods", "~> 1.7.1". I used it in #1199 and all checks have passed.

[dstranz]

Thank you for the suggestion @RomanPodymov.

It looks that it's not the only cause of failure.

IMO the idea to test cache that way, with delays doesn't work well on CI because (probably) there are some slow downs and cache time elapsed before the last delayed block.

https://travis-ci.org/onevcat/Kingfisher/jobs/540614120
✅ testCacheImageExtendingExpirationTask passed
🔴 testCacheImageExpirationTask failed

https://travis-ci.org/onevcat/Kingfisher/jobs/540614130
🔴 testCacheImageExtendingExpirationTask failed
✅ testCacheImageExpirationTask passed

I need to figure out better way to test cache with/without memoryCacheExpirationNotExtendable option.

[onevcat]

It's good to have this option. However, maybe we could make it more generic. Currently, we are extending the expiration duration according to the original .memoryCacheExpiration setting. Let's make it something like .memoryCacheAccessExtendingExpiration(StorageExpiration) so we can control it better. For example, in your case, you send a . memoryCacheAccessExtendingExpiration(.seconds(0)) to prevent the extending, while others can choose . memoryCacheAccessExtendingExpiration(.days(3)) to extend it by 3 days or .memoryCacheAccessExtendingExpiration(.expired) to make it expired immediately.

And maybe we could do the same thing to the disk cache, so we can solve #1152 as well.

[dstranz]

@onevcat Please take a look at the implementation of .memoryCacheAccessExtendingExpiration(StorageExpiration).

In case of not extending I use .memoryCacheAccessExtendingExpiration(.never).

[onevcat]

@dstranz Great!

In case of not extending I use .memoryCacheAccessExtendingExpiration(.never).

I can understand your concerns here. However, I am not feeling good to have different meanings on .never: in the .memoryCacheExpiration it means "not expiring at all" but in .memoryCacheAccessExtendingExpiration now it is "not extending". This would definitely brings confusion and issues in future, even you have written great documentation on it.

Maybe we would need another enum besides of StorageExpiration (something like ExpirationExtending), or maybe we need to reconsider the name of .memoryCacheAccessExtendingExpiration if we want to keep using StorageExpiration as the associated value type. I personally prefer the former. Do you have any better idea?

[dstranz]

OK, we could use .memoryCacheAccessExtendingExpiration(.seconds(0)) as "not extend" case. I understand your reason to not use .never but in such case without special treatment it will change TTL to 0 seconds and expire cache instead of not extending. Of course, we could add there guard expiration.timeInterval != 0 to change default behavior.

I had something similar to ExpirationExtending yesterday, but later removed that. What do you think about such cases:

enum ExpirationExtending {
case .none
case .cacheTime
case .expirationTime(_ expiration: StorageExpiration)
}

Default value in Options will be .cacheTime as before change.

[onevcat]

Yes. I think your ExpirationExtending definition is so nice!

[dstranz]

@onevcat I've just pushed the code with ExpirationExtending.

[onevcat]

CI machines are quite slow and it is not safe to judge a duration like 0.05s. I suggest make it 0.1, 0.3, 0.5s, which can improve the CI stability. Or instead of checking whether it is retrieved from memory cache or not, you can check the expiration duration after the access to confirm whether the time is extended, which might be also stable than a timer.

[dstranz]

Or instead of checking whether it is retrieved from memory cache or not, you can check the expiration duration after the access to confirm whether the time is extended, which might be also stable than a timer.

I've just rewritten both failing tests and then see your comment. Looks like we had similar idea. I'm only not sure if it's the best way to obtain expiration date:

let cacheKey = result.value!.source.cacheKey as NSString
let expirationTime1 = ImageCache.default.memoryStorage.storage.object(forKey: cacheKey)?.estimatedExpiration

It's good to have this option. However, maybe we could make it more generic.

I will look at that when tests will pass for current solution 😉

[onevcat]

All green! Thanks a lot for that, @dstranz It is awesome.