[BUG] SkipDistributedCacheReadWhenStale also skips on memory cache miss

[angularsen]

Describe the bug

Distributed cache is skipped on memory cache miss when setting SkipDistributedCacheReadWhenStale = true in FusionCacheOptions. Expected only stale cache to be skipped.

To Reproduce

Dependencies: XUnit, FluentAssertions

public class FusionCacheTests
{
    [Fact]
    public async Task SkipDistributedCacheReadWhenStale_True_DoesNotSkipOnMemoryCacheMiss()
    {
        const string cacheKey = "key";
        var fusionCache1 = new FusionCache(Options.Create(new FusionCacheOptions
        {
            DefaultEntryOptions = { SkipDistributedCacheReadWhenStale = true }
        }));

        var fusionCache2 = new FusionCache(Options.Create(new FusionCacheOptions
        {
            DefaultEntryOptions = { SkipDistributedCacheReadWhenStale = true }
        }));

        var fakeDistributedCache = new FakeDistributedCache();
        fusionCache1.SetupDistributedCache(fakeDistributedCache, new FusionCacheSystemTextJsonSerializer());
        fusionCache2.SetupDistributedCache(fakeDistributedCache, new FusionCacheSystemTextJsonSerializer());

        // Set cache via the first fusion cache instance.
        await fusionCache1.SetAsync(cacheKey, new TestCacheEntry("irrelevant"));
        fakeDistributedCache.GetAsync(cacheKey).Should().NotBeNull("it should now be stored in distributed cache");
        (await fusionCache1.TryGetAsync<TestCacheEntry>(cacheKey)).HasValue.Should().BeTrue("cache hit in fusionCache1's memory");

        // The second fusion cache instance should still read from distributed cache on memory cache miss.
        (await fusionCache2.TryGetAsync<TestCacheEntry>(cacheKey)).HasValue.Should().BeTrue("cache miss in fusionCache2's memory, but should have cache hit in distributed cache");
    }

    private record TestCacheEntry(string Value);

    private class FakeDistributedCache : IDistributedCache
    {
        private readonly ConcurrentDictionary<string, byte[]> _cache = new();
        public byte[]? Get(string key)
        {
            return _cache.GetValueOrDefault(key);
        }

        public Task<byte[]?> GetAsync(string key, CancellationToken token = new CancellationToken())
        {
            return Task.FromResult(_cache.GetValueOrDefault(key));
        }

        public void Refresh(string key) { throw new System.NotImplementedException(); }

        public Task RefreshAsync(string key, CancellationToken token = new CancellationToken()) { throw new System.NotImplementedException(); }

        public void Remove(string key)
        {
            _cache.Remove(key, out _);
        }

        public Task RemoveAsync(string key, CancellationToken token = new CancellationToken())
        {
            _cache.Remove(key, out _);
            return Task.CompletedTask;
        }

        public void Set(string key, byte[] value, DistributedCacheEntryOptions options)
        {
            _cache.AddOrUpdate(key, value, (key, existing) => value);
        }

        public Task SetAsync(string key, byte[] value, DistributedCacheEntryOptions options, CancellationToken token = new CancellationToken())
        {
            _cache.AddOrUpdate(key, value, (key, existing) => value);
            return Task.CompletedTask;
        }
    }
}

Expected behavior Skip distributed cache only on stale value in memory cache, don't skip on missing value in memory cache.

Versions I've encountered this issue on:

• FusionCache 0.24.0
• .NET 7.0
• Windows 11

Screenshots None.

Additional context Not sure if this is a bug or as intended, but it was not intuitive for me based on the name SkipDistributedCacheReadWhenStale and its xmldoc:

When a 2nd layer (distributed cache) is used and a cache entry in the 1st layer (memory cache) is found but is stale, a read is done on the distributed cache: the reason is that in a multi-node environment another node may have updated the cache entry, so we may found a newer version of it.

There are situations though, like in a mobile app with a SQLite 2nd layer, where the 2nd layer is not really "distributed" but just "out of process" (to ease cold starts): in situations like this noone can have updated the 2nd layer, so we can skip that extra read for a perf boost (of course the write part will still be done).

TL/DR: if your 2nd level is not "distributed" but only "out of process", setting this to true can give you a nice performance boost.

Our use case was a Blazor web app, where we store order details in browser local storage to resume after redirecting to a payment method provider and back again. There is no central storage available.

We configure FusionCache to use browser local storage as distributed cache to persist the order, but setting SkipDistributedCacheReadWhenStale = true resulted in not reading from browser local storage due to a miss in memory cache. This is then due to Blazor creating scoped resources per circuit, which in practice is for each full page load. So when redirecting back from the payment method provider to our app, a new FusionCache instance and memory cache instance is created.

A more common use case is the "cold start" scenario in xmldoc, getting cached values persisted to disk on startup or after restarting a .NET application, using disk as distributed cache.

[jodydonetti]

Hi @angularsen the behavior should be the one you expected, so it seems like you hit a bug: maybe it's because of one of the changes I've made recently, but it's strange that the tests didn't catch that.

I'll be able to take a look at that after Christmas, will let you know.

Thanks for pointing that out!

[angularsen]

Great! Thanks for the speedy response. No rush on my end, have a great holiday!

[jodydonetti]

Hi @angularsen , upon further inspection it seems like we were both kinda right 😅

Basically in the "get or set" code path the check was correct, whereas in the "get only" code path (eg: TryGet/GetOrDefault) the check was not correct.

I tried to keep them as "aligned" as possible, but this escaped my efforts.

Now it's fixed in the new branch, and with the next release will be fixed.

Thanks for reporting it!

[jodydonetti]

While we are here, a couple of notes.

First is a curiosity: why did you create a custom IDistributedCache implementation that works only in memory, instead of using the existing one MemoryDistributedCache? I was wondering if you had a particular reason.

Also, watch out for a couple of things in this line:

fakeDistributedCache.GetAsync(cacheKey).Should().NotBeNull("it should now be stored in distributed cache");

First, since you are calling the async version (fakeDistributedCache.GetAsync(cacheKey)) and you are not using the await keyword, what you are in fact testing is the returned Task<T> which will always be non null, and not the returned cached value itself.

Second, I see you are using the same cacheKey to directly access the distributed cache, and typically the cache key is modified when talking to that (read here for more) so in theory it would always fail.

Hope this helps.

[angularsen]

Oh yeah, you are absolutely right about the two mistakes in the test code. I just threw it together real quick and the results matched what I saw in production code so I thought I got it right 😓😄

What you say about Get vs GetOrSet makes sense to me, we rarely use Get-path and I guess most other people also use it less frequently, which could explain why it took time to discover.

No good reason to not use MemoryDistributedCached, I could not think of any ready to go implementation in the heat of the moment so I just wrote my own for this piece of test code.

Thanks for the swift investigation and pointing out the mistakes, much appreciated 🙇 Will give the new version a go soon.

[jodydonetti]

Hi @angularsen , I just released v0.25.0-preview1 🎉

This version includes the fix related to this issue, so if you can give it a try it would be great, thanks!

[angularsen]

Perfect, will give it a go soon when I'm done wrapping up another small project.

[jodydonetti]

Hi @angularsen , I just release v0.25.0 🥳

[angularsen]

Perfect thank you, haven't had time to try it yet but will do.

[angularsen]

It took me a while to get back to this, but I've tried v0.26 and this fix seems to work. Thanks!