Discussion: Is DefaultCache's Synchronization Mechanism Necessary or Overly Aggressive?

[rorydpayne]

I'm running into issues that appear to relate to the synchronization logic in DefaultCache. Specifically, I'm seeing clusters of requests fail with the following error:

System.Exception: Failed to obtain cache lock for: 'Duende.IdentityServer.Services.DefaultCache'

I’ve noticed this has been discussed before:

#1155
#191
#659
#720

While the root cause of the lock acquisition failure remains unclear to me, I wanted to step back and question whether the locking mechanism itself is necessary in most cases.

From what I can tell, the locking in DefaultCache.GetOrAddAsync is not about protecting access to the cache itself since MemoryCache is already thread-safe. Rather it is to synchronize the execution of the get delegate when a cache miss occurs. That makes sense in principle, but in practice, it introduces a level of serialization that may not be necessary, especially when the underlying store can safely handle concurrent calls for different keys.

In my use case, the inner store used by the cache is backed by an RDBMS (via Duende EF stores). That data access layer already provides its own isolation guarantees. Introducing a coarse-grained lock at the DefaultCache layer seems not only redundant, but also potentially detrimental to performance and scalability - especially under high concurrency where the locked resource isn't actually shared (e.g., different clients being resolved simultaneously).

So in general, I'm wondering if this kind of synchronization might be better handled at the level of the store implementation, rather than enforced within the caching layer itself. Since the actual store implementations are not part of this library, that would argue for either removing the lock entirely, or at least making it optional or more fine-grained depending on the store's needs. This would avoid enforcing a global synchronization strategy that may not be appropriate or necessary for all setups.

I’d love to hear thoughts on the original rationale for the current locking approach, and whether there’s openness to making it optional or store-specific. Is this something that could be revisited?

Thanks for your time and all the work that goes into IdentityServer!

[khalidabuhakmeh]

Hello @rorydpayne,

We've noticed some customers running into this issue in production environments, and some of your observations are correct. The current DefaultCache does lock, and in most cases, this is OK, but in high-load situations, you may intermittently see these kinds of exceptions.

While not an "official" recommendation, we have spiked a HybridCache implementation that seems to resolve these kinds of exceptions for some customers that you may want to try in your current setup.

https://github.com/khalidabuhakmeh/IdentityServerHybridCache

Here is the implementation if you want to try it out.

using System.Diagnostics;
using Duende.IdentityServer.Services;
using Microsoft.Extensions.Caching.Hybrid;

namespace HybridCacheSample.Caches;

public class HybridCache<T>(HybridCache cache, ILogger<HybridCache<T>> logger) : ICache<T>
    where T : class
{
    private static string GetKey(string key) =>
        $"/{typeof(T).FullName}/{key}";
    
    public async Task<T?> GetAsync(string key)
    {
        using var activity = Activity.Current?.Source.StartActivity();

        // this is a get, so expire immediately
        var result = await cache.GetOrCreateAsync<T?>(GetKey(key),
            _ => default,
            new HybridCacheEntryOptions
            {
                Expiration = TimeSpan.Zero,
                LocalCacheExpiration = TimeSpan.Zero,
                Flags = HybridCacheEntryFlags.DisableLocalCache | HybridCacheEntryFlags.DisableDistributedCache
            });
        
        return result;
    }

    public async Task<T> GetOrAddAsync(string key, TimeSpan duration, Func<Task<T>> factory)
    {
        using var activity = Activity.Current?.Source.StartActivity();
        
        logger.LogInformation("Get or Add for {Type}: {Key}", typeof(T).FullName, key);
        
        return await cache.GetOrCreateAsync<Func<Task<T>>, T>(
            GetKey(key), 
            factory, 
            async (fact, _) =>
            {
                logger.LogInformation("creating for {Type}: {Key}", typeof(T).FullName, key);
                return await fact();
            });
    }

    public async Task SetAsync(string key, T item, TimeSpan expiration)
    {
        using var activity = Activity.Current?.Source.StartActivity();
        logger.LogInformation("cache set for {Type}: {Key}", typeof(T).FullName, key);
        
        await cache.SetAsync(GetKey(key), item, new()
        {
            Expiration = expiration,
            LocalCacheExpiration = expiration
        });
    }

    public async Task RemoveAsync(string key)
    {
        using var activity = Activity.Current?.Source.StartActivity();
        logger.LogInformation("cache remove for {Type}: {Key}", typeof(T).FullName, key);
        
        await cache.RemoveAsync(GetKey(key));
    }
}

[rorydpayne]

Hello @khalidabuhakmeh,

Thanks for your reply. This is exactly what I've implemented as a workaround already and so far so good. Comforting to know we're not the only ones skipping the lock even if it's not an "official recommendation" as you say.

Thought I'd raise it here anyway to open a discussion up. I guess if you have been spiking an approach using this HybridCache, then there is perhaps some openness to making this configurable in the future?

I appreciate that by enabling in-memory caching, there is elegance to avoiding multiple reads of the same entity from the underlying store. Maybe some kind of striped lock would be an improvement.

[maartenba]

We're definitely looking at some improvements to locking (and caching in general), but some of these may be breaking changes where we are discussing when the type of change would be appropriate.

I'd be curious to hear - where did you run into caching issues? Is there a specific endpoint or action where you ran into this?