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] GetOrSetAsync is not handling concurrent operations/threads #281

Closed oasaleh closed 3 months ago

oasaleh commented 3 months ago

Describe the bug

I'm calling a method from multiple threads. This method gets the cache and then looks in it for a value (the same value for all the threads.) Even though tokenCacheKey and acquireTokenFunc passed are the same for all the threads, the factory function (the 2nd param for GetOrSetAsync) is being called for every thread.

To Reproduce

    private async Task<string> GetTokenInternalAsync(string tokenCacheKey,
        Func<Task<AuthenticationResult>> acquireTokenFunc)
    {
        IFusionCache cache = cacheProvider.GetCache(Constants.TokenCacheName);
        bool fromCache = true;
        TimeSpan expirationDuration = TimeSpan.FromDays(1); // To avoid compiler complaining of unassigned variable

        // Try to get the token from the cache
        string token = await cache.GetOrSetAsync(
            tokenCacheKey,
            async _ =>
            {
                logger.LogInformation("{token} not found in cache. Acquiring a new token.", tokenCacheKey);
                // If the token is not in the cache, acquire a new one
                AuthenticationResult result = await acquireTokenFunc();
                // Calculate the expiration duration based on the token's ExpiresOn property
                expirationDuration = result.ExpiresOn - DateTime.UtcNow;

                logger.LogInformation($"New token acquired. Expiration duration set to {expirationDuration}.");

                fromCache = false;
                return result.AccessToken;
            },
            options => options.SetDuration(expirationDuration)
        );

        if (fromCache)
        {
            logger.LogInformation("{token} retrieved from cache.", tokenCacheKey);
        }

        return token;
    }

Expected behavior

For the same tokenCacheKey, the constructor should have only been invoked once (and accordingly, acquireTokenFunc() should have been invoked once as well.)

Versions

I've encountered this issue on:

jodydonetti commented 3 months ago

Hi @oasaleh , I'll look into this, but can you give me your DefaultEntryOptions setup please?

oasaleh commented 3 months ago

@jodydonetti, thank you. Here's my setup in Startup.cs.

        services.AddFusionCache(Constants.TokenCacheName).WithDefaultEntryOptions(opt =>
        {
            opt.Duration = TimeSpan.MaxValue;
        });

Also, I'm initiating the concurrency using await Parallel.ForEachAsync.

oasaleh commented 3 months ago

I found the problem. I can't do this inside the method: IFusionCache cache = cacheProvider.GetCache(Constants.TokenCacheName);. It must be done in the constructor instead. Now it's working!

jodydonetti commented 3 months ago

I'm glad it works now!

One thing though, to avoid surprising for you. In your factory you are doing:

// Calculate the expiration duration based on the token's ExpiresOn property
expirationDuration = result.ExpiresOn - DateTime.UtcNow;

This does nothing, meaning it's just changing a variable declared outside. If you want to do adaptive caching you need to change the context's options, like this:

// Calculate the expiration duration based on the token's ExpiresOn property
ctx.Options.Duration = result.ExpiresOn - DateTime.UtcNow;

Hope this helps.

oasaleh commented 3 months ago

This does nothing, meaning it's just changing a variable declared outside.

But I am using it in my options. options => options.SetDuration(expirationDuration) This is not being taken into account?

jodydonetti commented 3 months ago

Oh yeah absolutely it is, but when calling the GetOrSet method, not after the execution of the factory.

Let's try to clear up the flow:

So there's no way to change a variable used beore to affect options already calculated. The way to change some options in the factory is via Adaptive Caching as I highlighted, by using ctx.Options.Duration = ....

Hope this helps!