4
\$\begingroup\$

I am using following code to maintain some static information. The problem I see with it is that, if the information retrieved using GeKeysFromCache is modified without using lock keyword it may lead to exceptions in a multithreaded environment. Is there a way to improve this implementation?

public class CacheHelper
{
    private static ConcurrentDictionary<string, List<InstanceField>> m_InstancesCached = new ConcurrentDictionary<string, List<InstanceField>>();
    private static readonly Object _entriesLock = new object();

    public List<InstanceField> GeKeysFromCache(string cacheKey)
    {
        return m_InstancesCached.GetOrAdd(cacheKey, (key) =>
        {
            return new List<InstanceField>();
        });
    }

    public void AddKeysToCache(string cacheKey, List<InstanceField> inputs)
    {
        m_InstancesCached.AddOrUpdate(cacheKey, inputs, (key, oldValue) =>
        {
            lock (_entriesLock)
            {
                oldValue.AddRange(inputs);
                return oldValue;
            }
        });
    }
}

Test Code: Without lock keyword it fails

public class ThreadSafeCachingTest
{
    private static readonly Object _entriesLock = new object();
    public ThreadSafeCachingTest()
    {
    }

    public void Invoke()
    {
        var thread1Completed = false;
        var thread2Completed = false;
        var thread3Completed = false;
        var threadList = new List<Thread>();

        var instanceName = "MyInstance";
        var beforeCachingData = (new CacheHelper()).GeKeysFromCache(instanceName);

        threadList.Add(new Thread(() =>
        {
            //lock (_entriesLock)
            //{
            var instanceFields1 = (new CacheHelper()).GeKeysFromCache(instanceName);
            instanceFields1 = instanceFields1 ?? new List<InstanceField>();

            for (int i = 0; i < 10000; i++)
            {
                instanceFields1.Add(new InstanceField() { FieldName = "FieldName-" + i.ToString() });
            }
            thread1Completed = true;
            //}
        }));


        threadList.Add(new Thread(() =>
        {
            //lock (_entriesLock)
            //{
            var instanceFields2 = (new CacheHelper()).GeKeysFromCache(instanceName);
            instanceFields2 = instanceFields2 ?? new List<InstanceField>();

            for (int i = 10000; i < 20000; i++)
            {
                instanceFields2.Add(new InstanceField() { FieldName = "FieldName-" + i.ToString() });
            }
            thread2Completed = true;
            //}
        }));

        threadList.Add(new Thread(() =>
        {
            //lock (_entriesLock)
            //{
            while (!(thread1Completed && thread2Completed && thread3Completed))
            {
                var instanceFields3 = (new CacheHelper()).GeKeysFromCache(instanceName);
                if (instanceFields3.Count > 0)
                {
                    instanceFields3.RemoveAt(0);
                }
                else
                {
                    thread3Completed = true;
                }
                //}
            }
        }));

        foreach (Thread t in threadList) t.Start();
        foreach (Thread t in threadList) t.Join();

        var afterCachingData = (new CacheHelper()).GeKeysFromCache(instanceName);
    }
}

public class InstanceField
{
    public string FieldName { get; set; }
}
\$\endgroup\$
2
  • \$\begingroup\$ I've never used a ConcurrentDictionary, but presumably all its methods are thread safe. So are you actually asking about making modifications to a List<InstanceField> in a thread-safe way? \$\endgroup\$
    – 404
    Mar 22, 2017 at 12:11
  • \$\begingroup\$ @eurotrash Yes. \$\endgroup\$ Mar 22, 2017 at 12:50

2 Answers 2

2
\$\begingroup\$
public List<InstanceField> GeKeysFromCache(string cacheKey)
{
    var resultSet = m_InstancesCached.GetOrAdd(cacheKey, (key) =>
   {
       return new List<InstanceField>();
   });

    return resultSet.ToList();
}

Why so many returns and when you can simply do this with only one return and without the lambda:

return m_InstancesCached.GetOrAdd(cacheKey, new List<string>()).ToList();
\$\endgroup\$
1
\$\begingroup\$

What I was missing was something very trivial, adding a ToList() to GeKeysFromCache makes it ThreadSafe

public class CacheHelper
{
    private static ConcurrentDictionary<string, List<InstanceField>> m_InstancesCached = new ConcurrentDictionary<string, List<InstanceField>>();
    private static readonly Object _entriesLock = new object();

    public List<InstanceField> GeKeysFromCache(string cacheKey)
    {
        var resultSet = m_InstancesCached.GetOrAdd(cacheKey, (key) =>
       {
           return new List<InstanceField>();
       });

        lock (_entriesLock)
        {
            return resultSet.ToList();
        }
    }

    public void AddKeysToCache(string cacheKey, List<InstanceField> inputs)
    {
        m_InstancesCached.AddOrUpdate(cacheKey, inputs, (key, oldValue) =>
        {
            lock (_entriesLock)
            {
                oldValue.AddRange(inputs);
                return oldValue;
            }
        });
    }
}
\$\endgroup\$
0

Your Answer

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

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