Async threadsafe Get from MemoryCache

Question

I have created a async cache that uses .NET MemoryCache underneath. This is the code:

public async Task<T> GetAsync(string key, Func<Task<T>> populator, TimeSpan expire, object parameters)
{
    if(parameters != null)
        key += JsonConvert.SerializeObject(parameters);

    if(!_cache.Contains(key))
    {
        var data = await populator();
        lock(_cache)
        {
            if(!_cache.Contains(key)) //Check again but locked this time
                _cache.Add(key, data, DateTimeOffset.Now.Add(expire));
        }
    }

    return (T)_cache.Get(key);
}

I think the only downside is that I need to do the await outside the lock so the populator isn't thread safe, but since the await can't reside inside a lock I guess this is the best way. Are there any pitfalls that I have missed?

Update: A version of Esers answer that is also threadsafe when another thread invalidates the cache:

public async Task<T> GetAsync(string key, Func<Task<T>> populator, TimeSpan expire, object parameters)
{
    if(parameters != null)
        key += JsonConvert.SerializeObject(parameters);

    var lazy = new Lazy<Task<T>>(populator, true);
    _cache.AddOrGetExisting(key, lazy, DateTimeOffset.Now.Add(expire));
    return ((Lazy<Task<T>>) _cache.Get(key)).Value;
}

It can however be slower because it creates Lazy instances that never will be executed and it uses Lazy in full threadsafe mode LazyThreadSafetyMode.ExecutionAndPublication

Update with new benchmark (Higher is better)

Lazy with lock      42535929
Lazy with GetOrAdd  41070320 (Only solution that is completely thread safe)
Semaphore           64573360

Answer 1

A simple solution would be to use SemaphoreSlim.WaitAsync() instead of a lock, and then you could get around the issue of awaiting inside a lock. Although, all other methods of MemoryCache are thread-safe.

private SemaphoreSlim semaphoreSlim = new SemaphoreSlim(1);
public async Task<T> GetAsync(
            string key, Func<Task<T>> populator, TimeSpan expire, object parameters)
{
    if (parameters != null)
        key += JsonConvert.SerializeObject(parameters);

    if (!_cache.Contains(key))
    {
        await semaphoreSlim.WaitAsync();
        try
        {
            if (!_cache.Contains(key))
            {
                var data = await populator();
                _cache.Add(key, data, DateTimeOffset.Now.Add(expire));
            }
        }
        finally
        {
            semaphoreSlim.Release();
        }
    }

    return (T)_cache.Get(key);
}

Answer 2

The current answers use the somewhat outdated System.Runtime.Caching.MemoryCache. They also contain subtle race conditions (see comments). Finally, not all of them allow the timeout to be dependent on the value to be cached.

Here's my attempt using the new Microsoft.Extensions.Caching.Memory (used by ASP.NET Core):

//Add NuGet package: Microsoft.Extensions.Caching.Memory    

using Microsoft.Extensions.Caching.Memory;
using Microsoft.Extensions.Primitives;

MemoryCache _cache = new MemoryCache(new MemoryCacheOptions());

public Task<T> GetOrAddAsync<T>(
        string key, Func<Task<T>> factory, Func<T, TimeSpan> expirationCalculator)
{    
    return _cache.GetOrCreateAsync(key, async cacheEntry => 
    {
        var cts = new CancellationTokenSource();
        cacheEntry.AddExpirationToken(new CancellationChangeToken(cts.Token));
        var value = await factory().ConfigureAwait(false);
        cts.CancelAfter(expirationCalculator(value));
        return value;
    });
}

Sample usage:

await GetOrAddAsync("foo", () => Task.Run(() => 42), i  => TimeSpan.FromMilliseconds(i)));

Note that it is not guaranteed for the factory method to be called only once (see https://github.com/aspnet/Caching/issues/240).

Answer 3

Although there is an already accepted answer, I'll post a new one with Lazy<T> approach. Idea is: to minimize the duration of lock block, if the key doesn't exists in cache, put a Lazy<T> to cache. That way all threads using the same key at the same time will be waiting the same Lazy<T>'s value

public Task<T> GetAsync<T>(string key, Func<Task<T>> populator, TimeSpan expire, object parameters)
{
    if (parameters != null)
        key += JsonConvert.SerializeObject(parameters);

    lock (_cache)
    {
        if (!_cache.Contains(key))
        {
            var lazy = new Lazy<Task<T>>(populator, true);
            _cache.Add(key, lazy, DateTimeOffset.Now.Add(expire));
        }
    }

    return ((Lazy<Task<T>>)_cache.Get(key)).Value;
}

Version2

public Task<T> GetAsync<T>(string key, Func<Task<T>> populator, TimeSpan expire, object parameters)
{
    if (parameters != null)
        key += JsonConvert.SerializeObject(parameters);

    var lazy = ((Lazy<Task<T>>)_cache.Get(key));
    if (lazy != null) return lazy.Value;

    lock (_cache)
    {
        if (!_cache.Contains(key))
        {
            lazy = new Lazy<Task<T>>(populator, true);
            _cache.Add(key, lazy, DateTimeOffset.Now.Add(expire));
            return lazy.Value;
        }
        return ((Lazy<Task<T>>)_cache.Get(key)).Value;
    }
}

Version3

public Task<T> GetAsync<T>(string key, Func<Task<T>> populator, TimeSpan expire, object parameters)
{
    if (parameters != null)
        key += JsonConvert.SerializeObject(parameters);

    var task = (Task<T>)_cache.Get(key);
    if (task != null) return task;

    var value = populator();
    return 
     (Task<T>)_cache.AddOrGetExisting(key, value, DateTimeOffset.Now.Add(expire)) ?? value;
}

Answer 4

This thread is a bit old but I hit this recently and I thought I'd leave this answer hoping it helps.

With async, there are few things to keep in mind:

1. An "uber lock" approach is not quick as it blocks the factory operations on other keys when performing one on a key.
2. A "lock per key" (SemaphoreSlim) has 2 things going on: a. It's disposable, so disposing it after could race. b. Live with not disposing it.

I chose to solve it using a pool of locks. It's not required to have a lock per key, but just enough locks as the max active threads possible. I then assign the same lock to the key through hashing. The pool size is a function of ProcessorCount. The valueFactory is executed only once. Since multiple keys map to a lock (a key always maps to the same lock), operations for keys with the same hash will get synchronized. So this loses some parallelism, and this compromise may not work for all cases. I'm OK with this compromise. This is the approach that LazyCache, and FusionCache (one of its many approaches) use, among other things. So I'd use one of them, but it's good to know the trick though as it's pretty nifty.

private readonly SemaphoreSlimPool _lockPool = new SemaphoreSlimPool(1, 1);

private async Task<TValue> GetAsync(object key, Func<ICacheEntry, Task<TValue>> valueFactory)
{
    if (_cache.TryGetValue(key, out var value))
    {
        return value;
    }

    // key-specific lock so as to not block operations on other keys
    var lockForKey = _lockPool[key];
    await lockForKey.WaitAsync().ConfigureAwait(false);
    try
    {
        if (_cache.TryGetValue(key, out value))
        {
            return value;
        }

        value = await _cache.GetOrCreateAsync(key, valueFactory).ConfigureAwait(false);
        return value;
    }
    finally
    {
        lockForKey.Release();
    }
}

// Dispose SemaphoreSlimPool

And here's the SemaphoreSlimPool impl (source, nuget).

/// <summary>
/// Provides a pool of SemaphoreSlim objects for keyed usage.
/// </summary>
public class SemaphoreSlimPool : IDisposable
{
    /// <summary>
    /// Pool of SemaphoreSlim objects.
    /// </summary>
    private readonly SemaphoreSlim[] pool;

    /// <summary>
    /// Size of the pool.
    /// <para></para>
    /// Environment.ProcessorCount is not always correct so use more slots as buffer,
    /// with a minimum of 32 slots.
    /// </summary>
    private readonly int poolSize = Math.Max(Environment.ProcessorCount << 3, 32);

    private const int NoMaximum = int.MaxValue;

    /// <summary>
    /// Ctor.
    /// </summary>
    public SemaphoreSlimPool(int initialCount)
        : this(initialCount, NoMaximum)
    { }

    /// <summary>
    /// Ctor.
    /// </summary>
    public SemaphoreSlimPool(int initialCount, int maxCount)
    {
        pool = new SemaphoreSlim[poolSize];
        for (int i = 0; i < poolSize; i++)
        {
            pool[i] = new SemaphoreSlim(initialCount, maxCount);
        }
    }

    /// <inheritdoc cref="Get(object)" />
    public SemaphoreSlim this[object key] => Get(key);

    /// <summary>
    /// Returns a <see cref="SemaphoreSlim"/> from the pool that the <paramref name="key"/> maps to.
    /// </summary>
    /// <exception cref="ArgumentNullException"></exception>
    public SemaphoreSlim Get(object key)
    {
        _ = key ?? throw new ArgumentNullException(nameof(key));
        return pool[GetIndex(key)];
    }

    private uint GetIndex(object key)
    {
        return unchecked((uint)key.GetHashCode()) % (uint)poolSize;
    }

    private bool disposed = false;

    public void Dispose()
    {
        Dispose(true);
    }

    public void Dispose(bool disposing)
    {
        if (!disposed)
        {
            if (disposing)
            {
                if (pool != null)
                {
                    for (int i = 0; i < poolSize; i++)
                    {
                        pool[i].Dispose();
                    }
                }
            }

            disposed = true;
        }
    }
}

I've thrown quite a bit of threads on this with a lot of churn due to low ttl and it's not bombing out. So far it looks good to me, but I'd like to see if anyone can find bugs.