Conditional thread locking

Question

Scenario

I want to download resources. I don't want a resource to be downloaded more that once. If thread a downloads resource 1 it should be cached, and thread b should wait and use the cached resource 1 if it attempts to download resource 1 at the same time. If thread c wants to download resource 2, it should not be influenced by thread a and b.

Attempt

I have tried to implement the scenario below:

using System;
using System.Collections.Generic;
using System.Threading;

namespace ConsoleApplication1
{
    class ConditionalThreadLockingProgram
    {
        private static readonly object _lockObject = new object();
        private static readonly Dictionary<int, string> Locks =
            new Dictionary<int, string>();
        private static readonly Dictionary<int, string> Resources =
            new Dictionary<int, string>();

        public static string GetLock(int resourceId)
        {
            lock (_lockObject)
            {
                if (Locks.ContainsKey(resourceId))
                {
                    return Locks[resourceId];
                }
                return Locks[resourceId] = string.Format(
                    "Lock #{0}",
                    resourceId
                );
            }
        }

        public static void FetchResource(object resourceIdObject)
        {
            var resourceId = (int)resourceIdObject;
            var currentLock = GetLock(resourceId);
            lock (currentLock)
            {
                if (Resources.ContainsKey(resourceId))
                {
                    Console.WriteLine(
                        "Thread {0} got cached: {1}",
                        Thread.CurrentThread.Name,
                        Resources[resourceId]
                    );
                    return;
                }
                Thread.Sleep(2000);
                Console.WriteLine(
                    "Thread {0} downloaded: {1}",
                    Thread.CurrentThread.Name,
                    Resources[resourceId] = string.Format(
                        "Resource #{0}",
                        resourceId
                    )
                );
            }
        }

        static void Main(string[] args)
        {
            new Thread(FetchResource) { Name = "a" }.Start(1);
            new Thread(FetchResource) { Name = "b" }.Start(1);
            new Thread(FetchResource) { Name = "c" }.Start(2);
            Console.ReadLine();
        }
    }
}

Question

Does it work? Any issues?

I would maybe suggesting using the SyncRoot of the dictionary instead of creating your own lock. GetLock will use (((IDictionary)Locks).SyncRoot) and FetchResouce will use (((IDictionary)Resources).SyncRoot) — d.moncada, Feb 18 '13 at 23:33
If you are looking for feedback on working code, it may be more appropriate to post your question to http://codereview.stackexchange.com/. — dgvid, Feb 18 '13 at 23:34
moncadad: Thank you for your suggestion. Some say that it may be dangerous to use the `SyncRoot` property: http://stackoverflow.com/questions/327654/hashtable-to-dictionary-syncroot — knut, Feb 18 '13 at 23:42
dgvid: thank you for telling me about codereview.stackexchange.com. Never used that site before. I feel that this question is about the concept of using different lock objects inside the same method. I feel that this is a generic question that need an example to clearify. — knut, Feb 18 '13 at 23:44
Don't lock on strings, ever. Because of string interning you virtually never know whether or not you're locking on the same instance of a string or not. It results in implementation defined behavior. — Servy, Feb 19 '13 at 00:54

Chris Gessler · Answer 1 · 2013-02-19T00:06:49.313

C# now contains Lazy, Concurrent Collections and MemoryCache - Add a reference to System.Runtime.Caching for MemoryCache.

Here's what I would do - no extra locking needed and the Lazy implementation takes care of the race condition.

/// <summary>
/// Summary description for ResourceFactory
/// </summary>
public static class ResourceFactory
{
    private const string _cacheKeyFormat = "AppResource[{0}]";

    private static readonly ObjectCache _cache = MemoryCache.Default;

    private static readonly CacheItemPolicy _policy = new CacheItemPolicy() 
    { 
        SlidingExpiration = TimeSpan.FromMinutes(Int32.Parse(ConfigurationManager.AppSettings["AppResourceTimeout"] ?? "20")), 
        RemovedCallback = new CacheEntryRemovedCallback(AppResourceRemovedCallback) 
    };

    private static void AppResourceRemovedCallback(CacheEntryRemovedArguments args)
    {
        // item was removed from cache
    }

    #region Extensions to make ObjectCache work with Lazy

    public static TValue GetOrAdd<TKey, TValue>(this ObjectCache @this, TKey key, Func<TKey, TValue> valueFactory, CacheItemPolicy policy)
    {
        Lazy<TValue> lazy = new Lazy<TValue>(() => valueFactory(key), true);
        return ((Lazy<TValue>)@this.AddOrGetExisting(key.ToString(), lazy, policy) ?? lazy).Value;
    }

    public static TValue GetOrAdd<TKey, TParam1, TValue>(this ObjectCache @this, TKey key, TParam1 param1, Func<TKey, TParam1, TValue> valueFactory, CacheItemPolicy policy)
    {
        Lazy<TValue> lazy = new Lazy<TValue>(() => valueFactory(key, param1), true);
        return ((Lazy<TValue>)@this.AddOrGetExisting(key.ToString(), lazy, policy) ?? lazy).Value;
    }

    #endregion


    public static AppResourceEntity GetResourceById(int resourceId)
    {
        #region sanity checks

        if (resourceId < 0) throw new ArgumentException("Invalid parameter", "resourceId");

        #endregion

        string key = string.Format(_cacheKeyFormat, resourceId);

        AppResourceEntity resource = _cache.GetOrAdd(
            key, 
            resourceId, 
            (k, r) =>
            {
                return AppResourceDataLayer.GetResourceById(r);
            }, 
            _policy
        );

        return resource;
    }
}

Conditional thread locking

Scenario

Attempt

Question

1 Answers1