ZiggyCreatures / FusionCache

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

[BUG] Value is stored for longer than FailSafeMaxDuration #212

Closed F2 closed 3 months ago

F2 commented 4 months ago

First of all, thank you for this library! The best .NET caching library out there. 👍

Describe the bug

According to the Options docs, the field FailSafeMaxDuration is the amount of time a cached entry will be available when using fail-safe.

This is also backed by the FailSafe docs. That example describes a Duration of 5 minutes, a FailSafeMaxDuration of 2 hours, and a FailSafeThrottleDuration of 1 minute, and concludes:

a total 2 hours is passed (FailSafeMaxDuration): the value is actually deleted from the cache, like, for real

However, using the same example with smaller durations (see below), it seems that the cache is never deleted when the factory consistently fails.

To Reproduce

Here's a MRE (Minimal Reproducible Example) of the issue:

using ZiggyCreatures.Caching.Fusion;

var cache = new FusionCache(new FusionCacheOptions());
var entryOptions = new FusionCacheEntryOptions
{
    Duration = TimeSpan.FromSeconds(5),
    IsFailSafeEnabled = true,
    FailSafeMaxDuration = TimeSpan.FromSeconds(10),
    FailSafeThrottleDuration = TimeSpan.FromSeconds(2),
};

await cache.SetAsync("key", "value", entryOptions);

for (var i = 0; i < 60; i++)
{
    var value = await cache.GetOrSetAsync<string>("key", _ => throw new Exception(), entryOptions);
    Console.WriteLine($"[{i}s] Got value: {value}");

    await Task.Delay(1000);
}

Expected behavior

It should not be possible to get the value after 10 seconds. It should throw an exception.

Actual behavior

It keeps getting the value for forever. image

Note, setting FailSafeThrottleDuration to a super high value makes it behave slightly better. In my example, it would throw an exception after 15 seconds. Still, according to documentation, it should happen after 10 seconds. And besides, using a throttle is a good idea.

Versions

I've encountered this issue on:

Additional information

As an aside, in the FailSafe docs it says:

That makes it sound like if Duration is 5 seconds and FailSafeMaxDuration is 10 seconds, the total time the value would be kept around is 5+10 seconds. But that's inconsistent with the rest of the docs, including some of your answers, stating it will take the highest value of the two.

jodydonetti commented 4 months ago

Hi @F2 and thanks for using FusionCache.

Sorry for the delay but I was at the MVP Global Summit and was not able to look into this. Anyway interesting, you seem to be right at first glance: now that I'm back I'll look into this asap and will let you know.

Thanks!

jodydonetti commented 3 months ago

I'm looking into this, will update soon.

jodydonetti commented 3 months ago

Update: ok I think I've found the culprit, working on the best way to make the change considering all the edge cases.

jodydonetti commented 3 months ago

Hi @F2 , here's an update: I've made some changes and I can confirm it now works as intended! I also added some tests to make sure it's strictly verified from now on to avoid any future regressions.

Will push the new version soon and I'll update you when that happen.

Thanks again!

jodydonetti commented 3 months ago

Hi, I just release v1.1.0 🥳

F2 commented 2 months ago

@jodydonetti Thank you so much. I can confirm it works in my code. 🤩

jodydonetti commented 2 months ago

image