Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] AllowTimedOutFactoryBackgroundCompletion issue with FailSafe #332

Closed
Coelho04 opened this issue Nov 21, 2024 · 5 comments
Closed

[BUG] AllowTimedOutFactoryBackgroundCompletion issue with FailSafe #332

Coelho04 opened this issue Nov 21, 2024 · 5 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@Coelho04
Copy link

Coelho04 commented Nov 21, 2024

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:

  • fusion cache 1.4.1
  • .net 8
@jodydonetti jodydonetti self-assigned this Nov 21, 2024
@jodydonetti jodydonetti added the bug Something isn't working label Nov 21, 2024
@jodydonetti jodydonetti added this to the v2.0.0 milestone Nov 21, 2024
@jodydonetti
Copy link
Collaborator

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!

@jodydonetti
Copy link
Collaborator

Can confirm all of the above: I've fixed it and now there are tests to verify that a fail in a background factory (both a throw and a soft one like ctx.Fail()) are handled correctly.

The new v2 will be out soon, and that version will include this fix, too.

Thanks again!

@jodydonetti
Copy link
Collaborator

Hi all, v2.0.0-preview-3 is out 🥳

@jodydonetti
Copy link
Collaborator

Hi all, I still can't believe it but v2.0.0 is finally out 🎉
Now I can rest.

@bbehrens
Copy link

@jodydonetti huge congrats man! Beautiful work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants