125

For starters let me just throw it out there that I know the code below is not thread safe (correction: might be). What I am struggling with is finding an implementation that is and one that I can actually get to fail under test. I am refactoring a large WCF project right now that needs some (mostly) static data cached and its populated from a SQL database. It needs to expire and "refresh" at least once a day which is why I am using MemoryCache.

I know that the code below should not be thread safe but I cannot get it to fail under heavy load and to complicate matters a google search shows implementations both ways (with and without locks combined with debates whether or not they are necessary.

Could someone with knowledge of MemoryCache in a multi threaded environment let me definitively know whether or not I need to lock where appropriate so that a call to remove (which will seldom be called but its a requirement) will not throw during retrieval/repopulation.

public class MemoryCacheService : IMemoryCacheService
{
    private const string PunctuationMapCacheKey = "punctuationMaps";
    private static readonly ObjectCache Cache;
    private readonly IAdoNet _adoNet;

    static MemoryCacheService()
    {
        Cache = MemoryCache.Default;
    }

    public MemoryCacheService(IAdoNet adoNet)
    {
        _adoNet = adoNet;
    }

    public void ClearPunctuationMaps()
    {
        Cache.Remove(PunctuationMapCacheKey);
    }

    public IEnumerable GetPunctuationMaps()
    {
        if (Cache.Contains(PunctuationMapCacheKey))
        {
            return (IEnumerable) Cache.Get(PunctuationMapCacheKey);
        }

        var punctuationMaps = GetPunctuationMappings();

        if (punctuationMaps == null)
        {
            throw new ApplicationException("Unable to retrieve punctuation mappings from the database.");
        }

        if (punctuationMaps.Cast<IPunctuationMapDto>().Any(p => p.UntaggedValue == null || p.TaggedValue == null))
        {
            throw new ApplicationException("Null values detected in Untagged or Tagged punctuation mappings.");
        }

        // Store data in the cache
        var cacheItemPolicy = new CacheItemPolicy
        {
            AbsoluteExpiration = DateTime.Now.AddDays(1.0)
        };

        Cache.AddOrGetExisting(PunctuationMapCacheKey, punctuationMaps, cacheItemPolicy);

        return punctuationMaps;
    }

    //Go oldschool ADO.NET to break the dependency on the entity framework and need to inject the database handler to populate cache
    private IEnumerable GetPunctuationMappings()
    {
        var table = _adoNet.ExecuteSelectCommand("SELECT [id], [TaggedValue],[UntaggedValue] FROM [dbo].[PunctuationMapper]", CommandType.Text);
        if (table != null && table.Rows.Count != 0)
        {
            return AutoMapper.Mapper.DynamicMap<IDataReader, IEnumerable<PunctuationMapDto>>(table.CreateDataReader());
        }

        return null;
    }
}
2
  • ObjectCache is thread safe, I don't think your class can fail. msdn.microsoft.com/en-us/library/… You might be going to the database at the same time but that will only use more cpu than needed.
    – the_lotus
    Nov 22, 2013 at 16:47
  • 2
    While ObjectCache is thread safe, the implementations of it may not be. Thus the MemoryCache question.
    – Haney
    Nov 22, 2013 at 16:49

8 Answers 8

94

The default MS-provided MemoryCache is entirely thread safe. Any custom implementation that derives from MemoryCache may not be thread safe. If you're using plain MemoryCache out of the box, it is thread safe. Browse the source code of my open source distributed caching solution to see how I use it (MemCache.cs):

https://github.com/haneytron/dache/blob/master/Dache.CacheHost/Storage/MemCache.cs

5
  • 2
    David, Just to confirm, in the very simple example class I have above, the call to .Remove() is in fact thread safe if a different thread is in the process of calling Get()? I suppose I should just use reflector and dig deeper but there is a lot of conflicting info out there. Nov 22, 2013 at 18:20
  • 15
    It is thread safe, but prone to race conditions... If your Get happens before your Remove, the data will be returned on the Get. If the Remove happens first, it will not. This is a lot like dirty reads on a database.
    – Haney
    Nov 22, 2013 at 21:25
  • 14
    worth to mention (as I commented in another answer below), dotnet core implementation is currently NOT entirely thread safe. specifically the GetOrCreate method. there is an issue on that on github
    – AmitE
    Mar 25, 2018 at 13:09
  • Do the memory cache throw an exception "Out of Memory" while running threads using console? Nov 25, 2019 at 14:06
  • how to solve dirty reads problem then ?
    – kuldeep
    Jan 27 at 8:19
73

While MemoryCache is indeed thread safe as other answers have specified, it does have a common multi threading issue - if 2 threads try to Get from (or check Contains) the cache at the same time, then both will miss the cache and both will end up generating the result and both will then add the result to the cache.

Often this is undesirable - the second thread should wait for the first to complete and use its result rather than generating results twice.

This was one of the reasons I wrote LazyCache - a friendly wrapper on MemoryCache that solves these sorts of issues. It is also available on Nuget.

5
  • 3
    "it does have a common multi threading issue" That's why you should use atomic methods like AddOrGetExisting, instead of implementing custom logic around Contains. The AddOrGetExisting method of MemoryCache is atomic and threadsafe referencesource.microsoft.com/System.Runtime.Caching/R/… Sep 23, 2020 at 9:14
  • 11
    Yes AddOrGetExisting is thread safe. But it assumes you already have a reference to the object that would be added to the cache. Normally you don't want AddOrGetExisting you want "GetExistingOrGenerateThisAndCacheIt" which is what LazyCache gives you. Sep 23, 2020 at 12:00
  • yes, agreed on the "if you don't already have the obejct" point Sep 24, 2020 at 8:57
  • 2
    The AddOrGetExisting is not atomic in fact. Please see github.com/dotnet/runtime/issues/36499
    – Andrew
    Jan 1, 2021 at 11:21
  • 1
    @Andrew you referenced AddOrCreate from .NET Core. I talking about AddOrGetExisting and even linked to the source code which has a lock in it Mar 23, 2021 at 10:56
29

As others have stated, MemoryCache is indeed thread safe. The thread safety of the data stored within it however, is entirely up to your using's of it.

To quote Reed Copsey from his awesome post regarding concurrency and the ConcurrentDictionary<TKey, TValue> type. Which is of course applicable here.

If two threads call this [GetOrAdd] simultaneously, two instances of TValue can easily be constructed.

You can imagine that this would be especially bad if TValue is expensive to construct.

To work your way around this, you can leverage Lazy<T> very easily, which coincidentally is very cheap to construct. Doing this ensures if we get into a multithreaded situation, that we're only building multiple instances of Lazy<T> (which is cheap).

GetOrAdd() (GetOrCreate() in the case of MemoryCache) will return the same, singular Lazy<T> to all threads, the "extra" instances of Lazy<T> are simply thrown away.

Since the Lazy<T> doesn't do anything until .Value is called, only one instance of the object is ever constructed.

Now for some code! Below is an extension method for IMemoryCache which implements the above. It arbitrarily is setting SlidingExpiration based on a int seconds method param. But this is entirely customizable based on your needs.

Note this is specific to .netcore2.0 apps

public static T GetOrAdd<T>(this IMemoryCache cache, string key, int seconds, Func<T> factory)
{
    return cache.GetOrCreate<T>(key, entry => new Lazy<T>(() =>
    {
        entry.SlidingExpiration = TimeSpan.FromSeconds(seconds);

        return factory.Invoke();
    }).Value);
}

To call:

IMemoryCache cache;
var result = cache.GetOrAdd("someKey", 60, () => new object());

To perform this all asynchronously, I recommend using Stephen Toub's excellent AsyncLazy<T> implementation found in his article on MSDN. Which combines the builtin lazy initializer Lazy<T> with the promise Task<T>:

public class AsyncLazy<T> : Lazy<Task<T>>
{
    public AsyncLazy(Func<T> valueFactory) :
        base(() => Task.Factory.StartNew(valueFactory))
    { }
    public AsyncLazy(Func<Task<T>> taskFactory) :
        base(() => Task.Factory.StartNew(() => taskFactory()).Unwrap())
    { }
}   

Now the async version of GetOrAdd():

public static Task<T> GetOrAddAsync<T>(this IMemoryCache cache, string key, int seconds, Func<Task<T>> taskFactory)
{
    return cache.GetOrCreateAsync<T>(key, async entry => await new AsyncLazy<T>(async () =>
    { 
        entry.SlidingExpiration = TimeSpan.FromSeconds(seconds);

        return await taskFactory.Invoke();
    }).Value);
}

And finally, to call:

IMemoryCache cache;
var result = await cache.GetOrAddAsync("someKey", 60, async () => new object());
8
  • 4
    I tried it, and it doens't seem to work (dot net core 2.0). each GetOrCreate will create a new Lazy instance, and the cache is updated with the new Lazy, and so, the Value get's evaluated\created multiple times (in a multiple thread env).
    – AmitE
    Mar 23, 2018 at 14:55
  • 3
    from the looks of it, .netcore 2.0 MemoryCache.GetOrCreate isn't thread safe in the same way ConcurrentDictionary is
    – AmitE
    Mar 23, 2018 at 17:28
  • Probably a silly question, but are you certain it was the Value that was created multiple times and not the Lazy? If so, how did you validate this?
    – pim
    Mar 24, 2018 at 11:42
  • 4
    I used a factory function that prints to screen when it was used + generate a random number, and start 10 threads all trying to GetOrCreate using the same key and that factory. the result, factory was used 10 times when using with memory cache (saw the prints) + each time the GetOrCreate returned a different value! I run the same test using ConcurrentDicionary and saw the factory being used only once, and got the same value always. I found a closed issue on it in github, I just wrote a comment there that it should be re-opened
    – AmitE
    Mar 25, 2018 at 13:05
  • 1
    This is all correct, and is similar to the implementation inside LazyCache which handles this code for you. In addition, it has some extra locking tricks to make sure your delegate only runs once in all scenarios and other features like callbacks, setting sizing and cancellation tokens all still work. github.com/alastairtree/lazycache Sep 23, 2020 at 12:07
11

Check out this link: http://msdn.microsoft.com/en-us/library/system.runtime.caching.memorycache(v=vs.110).aspx

Go to the very bottom of the page (or search for the text "Thread Safety").

You will see:

^ Thread Safety

This type is thread safe.

4
  • 7
    I stopped trusting MSDN's definition of "Thread Safe" on its own quite a while ago based on personal experience. Here is a good read: link Nov 22, 2013 at 20:20
  • 3
    That post is slightly different than the link I provided above. The distinction is very important in that the link I provided did not offer any caveats to the declaration of thread safety. I also have personal experience using MemoryCache.Default in very high volume (millions of cache hits per minute) with no threading problems yet. Nov 22, 2013 at 20:46
  • I think what they mean is that read and write operations take place atomically .Briefly, while thread A tries to read current value,it always read completed write operations ,not in the middle of writing data to memory by any thread.When thread A tries to write to memory no intervention could be made by any thread.That's my understanding but there are many questions/articles about that which don't lead to a complete conclusion like following. stackoverflow.com/questions/3137931/msdn-what-is-thread-safety
    – erhan355
    Jan 27, 2020 at 9:44
  • "Thread safe" / race conditions can be interpreted on many levels. An API formally could be thread-safe in the sense that it will not throw unexpected errors or add duplicate records when being called from parallel threads, but it does not necessarily mean that you can safely use it in typical use-cases without any concurrency control. For example, cache stampeding race condition is totally possible even when using completely "thread-safe" core libraries and APIs. Jul 15, 2022 at 12:21
4

As mentioned by @AmitE at the answer of @pimbrouwers, his example is not working as demonstrated here:

class Program
{
    static async Task Main(string[] args)
    {
        var cache = new MemoryCache(new MemoryCacheOptions());

        var tasks = new List<Task>();
        var counter = 0;

        for (int i = 0; i < 10; i++)
        {
            var loc = i;
            tasks.Add(Task.Run(() =>
            {
                var x = GetOrAdd(cache, "test", TimeSpan.FromMinutes(1), () => Interlocked.Increment(ref counter));
                Console.WriteLine($"Interation {loc} got {x}");
            }));
        }

        await Task.WhenAll(tasks);
        Console.WriteLine("Total value creations: " + counter);
        Console.ReadKey();
    }

    public static T GetOrAdd<T>(IMemoryCache cache, string key, TimeSpan expiration, Func<T> valueFactory)
    {
        return cache.GetOrCreate(key, entry =>
        {
            entry.SetSlidingExpiration(expiration);
            return new Lazy<T>(valueFactory, LazyThreadSafetyMode.ExecutionAndPublication);
        }).Value;
    }
}

Output:

Interation 6 got 8
Interation 7 got 6
Interation 2 got 3
Interation 3 got 2
Interation 4 got 10
Interation 8 got 9
Interation 5 got 4
Interation 9 got 1
Interation 1 got 5
Interation 0 got 7
Total value creations: 10

It seems like GetOrCreate returns always the created entry. Luckily, that's very easy to fix:

public static T GetOrSetValueSafe<T>(IMemoryCache cache, string key, TimeSpan expiration,
    Func<T> valueFactory)
{
    if (cache.TryGetValue(key, out Lazy<T> cachedValue))
        return cachedValue.Value;

    cache.GetOrCreate(key, entry =>
    {
        entry.SetSlidingExpiration(expiration);
        return new Lazy<T>(valueFactory, LazyThreadSafetyMode.ExecutionAndPublication);
    });

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

That works as expected:

Interation 4 got 1
Interation 9 got 1
Interation 1 got 1
Interation 8 got 1
Interation 0 got 1
Interation 6 got 1
Interation 7 got 1
Interation 2 got 1
Interation 5 got 1
Interation 3 got 1
Total value creations: 1
1
  • 4
    This doesn't work either. Just try it many times and you'll see that sometimes the value is not always 1.
    – mlessard
    Dec 16, 2018 at 2:25
3

Just uploaded sample library to address issue for .Net 2.0.

Take a look on this repo:

RedisLazyCache

I'm using Redis cache but it also failover or just Memorycache if Connectionstring is missing.

It's based on LazyCache library that guarantees single execution of callback for write in an event of multi threading trying to load and save data specially if the callback are very expensive to execute.

3
  • Please share answer only, other information can be shared as comment Apr 28, 2018 at 0:14
  • 1
    @WaelAbbas. I tried but it seems I need 50 reputations first. :D. Although it's not a direct answer to the OP question (can be answered by Yes/No with some explanation why), my reply is for a possible solution for the No answer. Apr 30, 2018 at 16:32
  • The implementation is flawed. Basically, once a key enters the concurrent dictionary, it can never be removed. Otherwise there is the possibility of having two semaphores active for one key, therefore allowing 2 threads to work on the same key. Jun 18 at 8:13
2

The cache is threadsafe, but like others have stated, its possible that GetOrAdd will call the func multiple types if call from multiple types.

Here is my minimal fix on that

private readonly SemaphoreSlim _cacheLock = new SemaphoreSlim(1);

and

await _cacheLock.WaitAsync();
var data = await _cache.GetOrCreateAsync(key, entry => ...);
_cacheLock.Release();
5
  • I Think its a Nice solution, but if I have multiple methods changing different caches, if I use the lock they will get locked without need! In this case we should have multiple _cacheLocks, I think it would be better if the cachelock could also have a Key!
    – Daniel
    Apr 28, 2020 at 23:59
  • There are many ways of solving it, one is generics the semaphore will be unique per instance of MyCache<T>. Then you can register it AddSingleton(typeof(IMyCache<>), typeof(MyCache<>));
    – Anders
    Apr 29, 2020 at 7:39
  • I probably wouldnt make the entire cache a singleton that could lead to trouble if you need to invoke other transient types. So maybe have a semaphore store ICacheLock<T> which is singleton
    – Anders
    Apr 29, 2020 at 7:41
  • The problem with this version is that if you have 2 different things to cache at the same time then you have to wait for the first one to finish generating before you can check the cache for the second. It is more efficient for them both to be able to check the cache (and generate) at the same if the keys are different. LazyCache uses a combination of Lazy<T> and striped locking to ensure your items cache as fast as possible, and only generate once per key. See github.com/alastairtree/lazycache Sep 23, 2020 at 12:14
  • This approach is problematic in async methods. Acquiring the semaphore happens on the first part of an async task, and releasing it happens on the continuation of that task. Semaphores are not re-entrant meaning you can't "lock" multiple times on the same thread. If you run many tasks in parallel, and all of the "lock" the semaphore, it is very possible that their first part is executed on the same thread (many tasks doesn't necessarily mean many threads). Which means of course that your "wait" part will block. And since it is blocked, no release will be called, leading to a deadlock of tasks. Feb 6 at 13:12
0

I know this is a old thread, but I have stumbled on the same problem and wanted to post my solution:

class FooProvider
{
    private readonly SemaphoreSlim _lock = new(1, 1);
    private readonly IMemoryCache _cache;

    public async ValueTask<Foo> GetFooAsync(string key)
    {
        var entry = _cache.Get<Foo>(key);
        if (entry is not null)
            return entry;

        _lock.Wait(cancellationToken);
        try
        {
            entry = _cache.Get<Foo>(key);
            if (entry is null)
            {
                entry = await CreateFooAsync();
                _cache.Set(key, entry, TimeSpan.FromSeconds(30));
            }
        }
        finally
        {
            _lock.Release();
        }
    }
}

the class is basically first checking if there's data in the cache. If it's missing, it will use a global lock, re-check if there's data (another thread might have entered it before), and if not, will call the async factory.

This will block other readers, even if they are trying to pull data for another key. A solution would be to keep a ConcurrentDictionary<string, SemaphoreSlim>, for example.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.