ZiggyCreatures / FusionCache

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

[BUG] AllowTimedOutFactoryBackgroundCompletion issue with FailSafe #332

Open Coelho04 opened 1 day ago

Coelho04 commented 1 day ago

Describe the bug

Hi there, first, thank you for all your hard work and this amazing framework. I think I found an issue when having the AllowTimedOutFactoryBackgroundCompletion and having a FaillSafe Defined

Having something like this

return await cache.GetOrSetAsync<IReadOnlyList<Product>>(cacheKey, async (context, token) =>
            {
                var result = await this.GetProductsAsync(request, token);

                if (!result.IsSuccess)
                {
                    logger.LogWarning(result.Error, "Failed to get brands from Commerce API.");
                    return context.Fail("Failed to get brands from Commerce API.");
                }

                if (result.Value == null || !result.Value.Any())
                {
                    logger.NoBrandsAvailable(request);

                    return context.Fail("Failed to get Products.");
                }

                return result.Value;
            },
            MaybeValue<IReadOnlyList<Product>>.FromValue([]),
            cancellationToken);

When you have a synthetic timeout what happens is that the the task will be completed in the background the issue is when returning a context.Fail("Failed to get Products."); it will not take into consideration that the factory can also fail and by doing so it's setting a null value in the cache and instead of setting the logical expiration date with the FailSafeThrottleDuration + Jitter is setting with duration defined in the settings + jitter, for instance, 1h.

To Reproduce

To reproduce this just use something like this:

return await cache.GetOrSetAsync<IReadOnlyList<Product>>(cacheKey, async (context, token) =>
            {
                var result = await this.GetProductsAsync(request, token);

                if (!result.IsSuccess)
                {
                    logger.LogWarning(result.Error, "Failed to get brands from Commerce API.");
                    return context.Fail("Failed to get brands from Commerce API.");
                }

                if (result.Value == null || !result.Value.Any())
                {
                    logger.NoBrandsAvailable(request);

                    return context.Fail("Failed to get Products.");
                }

                return result.Value;
            },
            MaybeValue<IReadOnlyList<Product>>.FromValue([]),
            cancellationToken);

Expected behavior

I was expecting that even when running in the background if the factory fails it would still respect the fail-safe and set the expiration time accordingly

Versions

I've encountered this issue on:

jodydonetti commented 1 day ago

Hi there, first, thank you for all your hard work and this amazing framework.

Thanks, I'm really glad you are liking it 🙂

I think I found an issue when having the AllowTimedOutFactoryBackgroundCompletion and having a FaillSafe Defined [...] I was expecting that even when running in the background if the factory fails it would still respect the fail-safe and set the expiration time accordingly

Damn, you are right! I'm currently handling the "soft fail" (eg: fail without throwing an exception) in the "normal flow" but not in the "background completion flow", good catch!

I'll fix it in the upcoming v2.

Thanks!