CODEWITHSUNDEEP
asp.net-mvc-3cachingthread-safety
dotnet-provoke
dotnet-provoke

Reputation: 1608

Thread safe caching

I am trying to analyze what problem i might be having with unsafe threading in my code.

In my mvc3 webapplication i try to the following:

// Caching code
public static class CacheExtensions
{
    public static T GetOrStore<T>(this Cache cache, string key, Func<T> generator)
    {
        var result = cache[key];
        if(result == null)
        {
          result = generator();
          lock(sync) {
              cache[key] = result;
          }
        }
    return (T)result;
    }
}

Using the caching like this:

// Using the cached stuff
public class SectionViewData 
{
    public IEnumerable<Product> Products {get;set;}
    public IEnumerable<SomethingElse> SomethingElse {get;set;}
}

private void Testing() 
{
    var cachedSection = HttpContext.Current.Cache.GetOrStore("Some Key", 0 => GetSectionViewData());

    // Threading problem?
    foreach(var product in cachedSection.Products)
    {
         DosomestuffwithProduct...
    }
}

private SectionViewData GetSectionViewData() 
{
    SectionViewData viewData = new SectionViewData();
    viewData.Products = CreateProductList();
    viewData.SomethingElse = CreateSomethingElse();

    return viewData;
}

Could i run inte problem with the IEnumerable? I dont have much experience with threading problems. The cachedSection would not get touched if some other thread adds a new value to cache right? To me this would work!

Should i cache Products and SomethingElse indivually? Would that be better than caching the whole SectionViewData??

Upvotes: 1

Views: 1112

Answers (2)

Joachim Isaksson
Joachim Isaksson

Reputation: 181097

Threading is hard;

In your GetOrStore method, the get/generator sequence is entirely unsynchronized, so any nymber of threads can get null from the cache and run the generator function at the same time. This may - or may not - be a problem.

Your lock statement only locks the setter of cache[string], which is already thread safe and doesn't need to be "extra locked".

The variation of double-checked locking in the cache is suspect, I'd try to get rid of it. Since the thread that never enters the lock() section can get result without a memory barrier, result may not be entirely constructed by the time the thread gets it.

Enumerating the cached IEnumrators is safe as long as nothing modifies them at the same time. If GetSectionViewData() returns an object with immutable (as in non changing) collections, you're safe.

Upvotes: 1

albattran
albattran

Reputation: 1907

Your code is missing parts like how would Products be populated? Only in GetSectionViewData? If so, then I don't see a major problem with your code. There is however a chance that two threads generate the same data(CachedSection) for the same key, it shouldn't create a threading problem except that you are doing the work twice, so if this was an expensive operation I would change the code so it only generates it once per key. If it is not expensive, it works fine as is.

IEnumerable for Products is not touched (assuming you create it separately per thread, but the enumerator on the cache is modified for each insert operation, hence it is not thread safe. So if you are using this I would be careful about that.

Upvotes: 0

Related Questions