7
\$\begingroup\$

Here I want to lock when populating a particular cache object, without blocking other calls to the same method requesting Foos for other barIds. I realise the MemoryCache will be thread safe, but if two or more concurrent calls come in it seems like it would be better if only one of them populates the cache and locks the others out while this is done.

using System.Runtime.Caching;
using Microsoft.Practices.Unity;

public class FooCacheService : IFooService
{
        private static readonly ConcurrentDictionary<string, object> FooServiceCacheLocks = new ConcurrentDictionary<string, object>();

        [Dependency("Explicit")]
        public IFooService ExplicitService { get; set; }

        public IEnumerable<Foo> GetFoosForBar(int barId)
        {
            string key = barId.ToString();

            object lockObject = FooServiceCacheLocks.GetOrAdd(key, new object());

            object cached = MemoryCache.Default[key];

            if (cached == null)
            {
                lock (lockObject)
                {
                    cached = MemoryCache.Default[key];

                    if (cached == null)
                    {
                        IEnumerable<Foo> foosForBar = this.ExplicitService.GetFoosForBar(barId);

                        MemoryCache.Default.Add(key, foosForBar, DateTime.Now.AddMinutes(5));

                        return foosForBar;
                    }
                }
            }

            return (IEnumerable<Foo>)cached;
        }
}

Assume GetFoosForBar is a CPU/IO intensive process and takes a short while to complete. Does this seem like a reasonable way to achieve this? I'm surprised that System.Runtime.Caching does not provide any support for this as default - or have I overlooked something?

Improve this question
\$\endgroup\$
8
\$\begingroup\$

What I don't like about your approach is that the lock objects are never removed from FooServiceCacheLocks, even when the object is removed from the cache.

One way to simplify your code would be to combine MemoryCache with Lazy: instances of Lazy are cheap (as long as you don't access its Value) so you can create more of them than needed. With that your code would look something like this:

public IEnumerable<Foo> GetFoosForBar(int barId)
{
    var newLazy = new Lazy<IEnumerable<Foo>>(
        () => this.ExplicitService.GetFoosForBar(barId));

    var lazyFromCache = (Lazy<IEnumerable<Foo>>)MemoryCache.Default.AddOrGetExisting(
        barId.ToString(), newLazy, DateTime.Now.AddMinutes(5));

    return (lazyFromCache ?? newLazy).Value;
}
Improve this answer
\$\endgroup\$
  • \$\begingroup\$ Is the OP's locking approach (two null checks with a lock in the middle) poor or just out of favour? I am currently using that approach in my projects. I am aware of the Lazy class but do not know enough of it to use it in my work. \$\endgroup\$ – Prabu Oct 1 '13 at 23:38
  • \$\begingroup\$ @pwee167 Like I said, the locking approach here means lock objects are never removed, which is basically a memory leak. And it's unnecessarily complicated. And I think that if you don't know enough about something interesting, that's a great opportunity to learn. You should probably start with the documentation, Lazy is actually not that complicated. \$\endgroup\$ – svick Oct 2 '13 at 0:54
  • \$\begingroup\$ I am reading about the Lazy class as I am typing this! I understand your point regarding storing the locks in the ConcurrentDictionary FooServiceCacheLocks and then not removing them once the corresponding cache item being removed. What my question was in reference to the locking strategy there after, and whether that was sound in design. I.e. If I only had one lock to control access to a piece of code, would the double null check with a lock in between be still good? \$\endgroup\$ – Prabu Oct 2 '13 at 1:13
  • \$\begingroup\$ @pwee167 This is called double-checked locking and it can be tricky to get it right. If possible, just lock and then check; that's somewhat less efficient, but easier to get right. But I think the way it's used here is correct. \$\endgroup\$ – svick Oct 2 '13 at 1:35
  • \$\begingroup\$ Thanks, that's what I was missing: I wondered why you could pass a lambda expression to ConcurrentDictionary's GetOrAdd method, but there was no equivalent for the MemoryCache object - didn't realise you could combine it with Lazy. There is one small amendment though to your code to make it work - from the MS docco: Return value: If a cache entry with the same key exists, the existing cache entry; otherwise, null. so you need if (lazyFromCache == null) { return newLazy.value; } \$\endgroup\$ – SilverlightFox Oct 2 '13 at 10:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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