[BUG] GetOrSetAsync overload setting Options aren't being respected

[celluj34]

Describe the bug When you use the overload of GetOrSetAsync in which you can set the Options, they aren't being respected. I have tried setting Duration and the Skip* propreties, but they still only use the default settings. If you use the Action<FusionCacheEntryOptions> setupAction parameter, then they are correctly respected.

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

• use GetOrSetAsync with the Func<FusionCacheFactoryExecutionContext, CancellationToken, Task<TValue?>> signature (adaptive caching)
• set context.Options.Duration to a specific value (e.g. TimeSpan.FromSeconds(10))
  • I notice that other options like SkipDistributedCache are also not respected
• create any value
• see that Duration is set to the default value (30 seconds?)

return await _cache.GetOrSetAsync(key,
   async (context, _) =>
   {
       // not reflected anywhere
       context.Options.SetSkipDistributedCache(true, true).SetSkipDistributedCacheReadWhenStale(true);

       try
       {
           // not reflected anywhere
           context.Options.Duration = TimeSpan.FromSeconds(10);

           await using var connection = _databaseConnectionFactory.GetConnection();

           await connection.CommandBuilder($"SELECT 1").ExecuteScalarAsync<int>();

           return true;
       }
       catch (Exception ex)
       {
           // not reflected anywhere
           context.Options.Duration = TimeSpan.FromTicks(1);

           return false;
       }
   },
   options =>
   {
       // reflected properly
       options.SetSkipDistributedCache(true, true).SetSkipDistributedCacheReadWhenStale(true);
   }) ?? false;

Expected behavior I expect the Option properties set in the value factory to affect the Duration and other options, and the setupAction to override any values set.

Versions I've encountered this issue on:

• FusionCache version: 0.18
• .NET version: 6
• OS version: W10
• others (eg: if applicable, the Redis/Memcached/etc version and if onprem/cloud/etc): n/a

[celluj34]

Upon further debugging it looks like the setupAction is being run first, which is surprising but I haven't read 100% of the documentation. Then it makes sense that the Duration isn't populated in the setup action.

Those settings (namely SkipDistributedCache and SkipDistributedCacheReadWhenStale) need to exist first to affect how it reads from the distributed cache. Is there perhaps some documentation which explains which settings need to exist first or don't take affect when set inside the value factory?

[jodydonetti]

Hi @celluj34 and thanks for using FusionCache!

Upon further debugging it looks like the setupAction is being run first,

Yes, exactly. That is because every method call needs some entry options, and there are different ways (and so, overloads of each method) to do that: 1) don't pass anything -> the FusionCacheOptions.DefaultEntryOptions will be used, as-is 2) directly pass an instance of FusionCacheEntryOptions -> these will be used directly 3) pass a setupAction -> the aforementioned FusionCacheOptions.DefaultEntryOptions will be used as a starting point, duplicated and then the specified setupAction will be applied to further configure them (so you can use the defaults and change only what you need) 4) pass a simple TimeSpan -> the same as the previous point, but automatically changing only the Duration (which may be common for a lot of people)

In any case, when a core method is called (eg: GetOrSet(), Set(), etc) all these different ways to specify entry options are resolved back to a FusionCacheEntryOptions instance, because in the end that is what is needed.

Hope this helps clarify things.

but I haven't read 100% of the documentation.

Try taking a look here.

Those settings (namely SkipDistributedCache and SkipDistributedCacheReadWhenStale) need to exist first to affect how it reads from the distributed cache.

I'm not sure I understand what you mean, but anyway: any option you specify (in any of the ways mentioned above) will be used. Then, inside your factory (where you go get your data from the db or whatever) you can further change some of those options, if you like, to obtain adaptive caching.

Of course it should be noted that changing some options there doesn't make any sense, when you think about it. Example: if inside the factory you set SkipDistributedCacheReadWhenStale = true , the "Read" phase is already happened, so that train is gone for good. I hope I'm being able to explain myself here.

Is there perhaps some documentation which explains which settings need to exist first

No, because all of them are set in a way or another anyway, either via defaults, via a lambda, etc...

or don't take affect when set inside the value factory?

In general you can change any of them inside the factory as long as it makes sense. Another example: it doesn't make sense to change the FactorySoftTimeout inside the factory while the factory is running, right?

Now that you mention it, one idea can be to add a note for each option to specify if it supports adaptive caching. Although some of them may be obviously unsupported, others may not be as immediately clear. What do you think?

Let me know if all is clear now.

[celluj34]

Thanks for taking the time to discuss each of my points, I think you answered most of my questions!

Those settings (namely SkipDistributedCache and SkipDistributedCacheReadWhenStale) need to exist first to affect how it reads from the distributed cache.

I'm not sure I understand what you mean, but anyway: any option you specify (in any of the ways mentioned above) will be used. Then, inside your factory (where you go get your data from the db or whatever) you can further change some of those options, if you like, to obtain adaptive caching.

Of course it should be noted that changing some options there doesn't make any sense, when you think about it. Example: if inside the factory you set SkipDistributedCacheReadWhenStale = true , the "Read" phase is already happened, so that train is gone for good. I hope I'm being able to explain myself here.

What I mean is something you mentioned, "it doesn't make sense to change the FactorySoftTimeout inside the factory while the factory is running", so why am I able to set that value while the factory is running? It makes total sense when you explain it like that, but it's not clear only from the documentation, if you don't take the time to think about it logically.

Now that you mention it, one idea can be to add a note for each option to specify if it supports adaptive caching. Although some of them may be obviously unsupported, others may not be as immediately clear. What do you think?

I think this would be a great change, at least from the C# doc perspective. I wonder if you could enforce it in code? Does something like this make sense:

// passed in when using the context for the value factory
// supports options only usable when using adaptive caching
public /*abstract*/ class FusionCacheEntryFactoryOptions {
    public Timespan Duration { get; set; }
}

// used when creating defaults, supports all options
public class FusionCacheEntryOptions : FusionCacheEntryFactoryOptions {
    public bool SkipDistributedCacheReadWhenStale { get; set; }
}

public class FusionCacheFactoryExecutionContext {
    public FusionCacheEntryFactoryOptions Options { get; set; }
}

This may or may not be feasible as it's definitely a breaking change, but would prevent setting options that have no effect.

[jodydonetti]

Thanks for taking the time to discuss each of my points, I think you answered most of my questions!

Thank you for contributing to the repo with your time!

What I mean is something you mentioned, "it doesn't make sense to change the FactorySoftTimeout inside the factory while the factory is running", so why am I able to set that value while the factory is running?

I created the FusionCacheEntryOptions class to hold the available options together (as is common with the other .NET caching abstractions, like MemoryCacheEntryOptions or DistributedCacheEntryOptions, so that users can feel at home).

Then, later on, the adaptive caching feature came along. While designing it, I felt it was overkill to create a separate class to "protect" users from setting things that could not possibly work during the factory execution.

This is also similar to the already existing way that the common MemoryCache operates, where you can change some options while the factory is running, even if changing some of those doesn't make sense.

It makes total sense when you explain it like that, but it's not clear only from the documentation

Ok, good point: I may add some notes in the xml docs (Intellisense & co) so that users can know which one can be set during adaptive caching and which cannot.

Also as said, I'll try to make that more clear and prominent in the online docs as well.

if you don't take the time to think about it logically

This is also a good point: even if something is logical, it may not always be immediately logical: people who code are not necessarily just exploring a new fancy library or just playing. More often than not they are solving real-world issues in real-world projects, so having something that can drive them towards a golden path is definitely something important to consider carefully.

I think this would be a great change, at least from the C# doc perspective. I wonder if you could enforce it in code? Does something like this make sense:

// passed in when using the context for the value factory
// supports options only usable when using adaptive caching
public /*abstract*/ class FusionCacheEntryFactoryOptions {
    public Timespan Duration { get; set; }
}

// used when creating defaults, supports all options
public class FusionCacheEntryOptions : FusionCacheEntryFactoryOptions {
    public bool SkipDistributedCacheReadWhenStale { get; set; }
}

public class FusionCacheFactoryExecutionContext {
    public FusionCacheEntryFactoryOptions Options { get; set; }
}

This may or may not be feasible as it's definitely a breaking change, but would prevent setting options that have no effect.

I'll think about such approach or a similar one, but honestly I don't know if going through the corresponding breaking change is worth it.

Another one may be to throw an exception when changing an option during the factory execution, when that option should not be changed (even though this seems a little excessive). Another one may be logging with a warning level.

Both of these though suffer from the fact that you may have some common code (think some custom ext methods) that modify a FusionCacheEntryOptions "in your own way" and that may throw when applied during factory execution, or in the case of the warning log may not be so apparent.

Let me think about it, I will let you know!

[celluj34]

This is also similar to the already existing way that the common MemoryCache operates, where you can change some options while the factory is running, even if changing some of those doesn't make sense.

Yep, this is exactly what I was doing and not thinking any further than that. "Of course it works that way, all the options are there!"

I'll think about such approach or a similar one, but honestly I don't know if going through the corresponding breaking change is worth it.

I totally understand! You know way more about your library than I do, so I trust you know what's best :)

I think I am satisfied, but I'll leave it up to you if you want to close this issue. Maybe there will be a few work items but as far as I am concerned what I posted originally isn't strictly a bug, just me not understanding. Thanks again!

[jodydonetti]

Hi @celluj34 , I've just updated the Options docs, and now it is specified which are supported in the adaptive cache scenario (with a 🧙‍♂️ icon).

Hope this helps.

[celluj34]

Yes, looks great, thank you!