ZiggyCreatures / FusionCache

FusionCache is an easy to use, fast and robust hybrid cache with advanced resiliency features.
MIT License
1.91k stars 97 forks source link

[BUG] FailSafe seems to ignore Duration when enabled #115

Closed suhrab closed 1 year ago

suhrab commented 1 year ago

Describe the bug When FailSafe is enabled it seems that Duration is ignored. Method GetOrSetAsync(). Please check out gist link below IsFailSafeEnabled=true Duration=1hour FailSafeMaxDuration=10seconds FailSafeThrottleDuration=5seconds The cached item expires in 10 seconds. Though Duration is 1hour. And cache factory does not throw

To Reproduce https://gist.github.com/suhrab/dab0a8081066cb51b130c744d93b4a86 "cacheFactoryGood fired" will be printed to Console 3 times. Though only single time expected - Duration is 1hours

I've encountered this issue on

jodydonetti commented 1 year ago

Hi @suhrab and thanks for using FusionCache!

You kind of have a point here but the thing is more nuanced, let me quickly explain.

In the Options docs you can see that it says this about Duration:

image

And about FailSafeMaxDuration it says this:

image

So the thing is this: Duration is intended as "after how much time the cache entry should be considered stale".

When NOT USING fail-safe, Duration corresponds to the actual physical cache entry expiration (and so after Duration passed the entry is effectively removed from the cache), nothing more to think about.

When USING fail-safe instead, the actual cache entry physical expiration is the FailSafeMaxDuration and the Duration becomes a metadata that is used to trigger a data refresh to the database (or whatever): this is useful because, with fail-safe, you can reuse a stale data in case something goes wrong while going to the database.

Now the problem here is what happens when the user (you in this case) specifies that the Duration is higher than the FailSafeMaxDuration: what you are basically saying is "I want this value to be considered valid for 1 hour, but also please remove it after 10 sec". Of course if it is removed after 10 sec, nothing more can happen after that.

Now, how can we make this better?

I think there are 2 ways to handle this:

  1. throw an exception when fail-safe is enabled and FailSafeMaxDuration is lower than Duration, since it doesn't make sense
  2. normalize the actual physical duration, so that if you enable fail-safe and say that Duration is 1 hour and FailSafeMaxDuration is 10 sec, the actual physical duration becomes the higher of the 2 (in this case 1 hour), and maybe log something to let the user know about this

I would go with the 2nd option, but I'd like to know what you think about all of this.

Let me know!

jodydonetti commented 1 year ago

PS: there's a new version (v0.19.0) coming out this weekend so, if you are quick to answer, this change may go directly into it 😉

jodydonetti commented 1 year ago

Hi @suhrab , I've implemented the 2nd option thanks to some additional checks and a normalization, and it seems to work very well.

As said it is also logging in case a normalization occurs, and the default logging level in case of options normalization is Warning so people won't be surprised, like in your case (at least if they look at their logs).

Of course that can be changed thanks to a new FusionCacheOptions option called IncoherentOptionsNormalizationLogLevel, which I may use in the future for more normalization behaviors like this.

I've also added some tests of course to verify the correct behavior and avoid future regressions.

I think I'll release the new version between today and tomorrow.

Hope this helps.

jodydonetti commented 1 year ago

The new v0.19.0 has been released, which includes this 🎉

suhrab commented 1 year ago

Hey Jody. I like more the option 2 you did. "the higher of the 2" is good. Great news. Have fun with your project. You are very friendly and helpful. Good job!