Locking considerations for populating ConcurrentDictionary with database values ​​in C #

Do I need to use lock (lockObj) {} when I populate my ConcurrentDictionary here? For a little background this will be used in an MVC application, although I suspect the scripting question is relevant to any multithreaded application.

Searching stackoverflow, I didn't find this exact scenario. The first time a value is requested from the GetOptionById value, it can be called by two separate threads.

1) Would it be considered best practice to have List objects evaluate to a private static object that you have locked, in the hopes of not calling the database multiple times before filling the ConcurrentDictionary?

2) Is this (# 1 above) even necessary, or is the ConcurrentDictionary smart enough to handle this on its own? Thanks in advance for any input.

public class MyOptions
{
    static string GetOptionById(int id)
    {
        if (options == null || options.Count <= 0)
            FillOptionList();
        return options[id];
    }

    static void FillOptionList()
    {
        List<MyBusinessObject> objects = DataAccessLayer.GetList();
        foreach (MyBusinessObject obj in objects)
            options.TryAdd(obj.Id, obj.Name);
    }

    private static ConcurrentDictionary<int, string> options = new ConcurrentDictionary<int, string>();
}

      

EDIT: Thanks everyone for your input, would this be a safer approach?

    public static string OptionById(int id)
    {
        if (!options.ContainsKey(id))
        {
            //perhaps this is a new option and we need to reload the list
            FillOptionsOrReturn(true /*force the fill*/);
            return (!options.ContainsKey(id)) ? "Option not found" : options[id];
        }
        else
            return options[id];
    }

    private static void FillOptionsOrReturn(bool forceFill = false)
    {
        List<MyBusinessClass> objectsFromDb = null;
        lock (lockObj)
        {
            if (forceFill || options == null || options.Keys.Count <= 0)
                reasons = DataAccessLayer.GetList();
        }
        if (objectsFromDb != null)
        {
            foreach (MyBusinessClass myObj in objectsFromDb)
                options.TryAdd(myObj.id, myObj.name);
        }
    }

    private static ConcurrentDictionary<int, string> options = new ConcurrentDictionary<int, string>();
    private static object lockObj = new object();

      

+3


source to share


4 answers


Do I need to use lock (lockObj) {} when I populate my ConcurrentDictionary here?

No, the methods of this data structure are already thread safe.

1) Would it be considered a best practice to make the value of List objects private static that you block in the hope of not naming the database multiple times before populating the ConcurrentDictionary?

Maybe, especially if GetList

it wasn't thread safe by itself. Other than what you suggest will fail. This instance is List<MyBusinessObject>

returned from GetList

, so you can't block what doesn't already exist. Instead, you will create a separate object for locking purposes only.



2) Is this (# 1 above) even necessary, or is the ConcurrentDictionary smart enough to handle this on its own?

No, there is no magic that would go on, anyway the caller GetList

would be performed sequentially.

By the way, yours GetOptionById

has a race condition. if

More than one stream could enter the block at the same time . Your code may try to initialize the dictionary more than once.

+1


source


What you definitely have is not safe. Consider:

Threads X and Y simultaneously call GetOptionById

at about the same time. X indicates that he needs to fill the dictionary, and starts to do so. The first result is returned and added to the dictionary.

Y then indicates that there is an entry and assumes that the dictionary is complete, so it picks the one it is interested in, which is probably not the one that was already loaded.



This looks like a good candidate for use Lazy<T>

... you can choose the appropriate options there so that only one thread can populate the dictionary at a time - the second thread will wait until the first one finishes before continuing. So "populating the dictionary" effectively becomes atomic.

If you never need to update the dictionary after the first download, you may be even better off with Lazy<Dictionary<string, string>>

- it's safe to have multiple readers if there are no writers. I believe it Lazy<T>

will handle memory barriers appropriately.

+3


source


There are problems like this in your code. If they are acceptable then you are fine, and if not then you need locks.

  • For multiple threads, one can figure out that it is options

    null and recreate the dictionary. This will cause it to be repeatedly filled.
  • It is possible to read a stream from a dictionary while some, but not all, elements are added.
+2


source


ConcurrentDictionary

only provide thread safety for accessing list items. On the other hand, your method FillOptionList

may be called multiple times from different threads, everyone will happily insert values ​​into the collection in turn.

What needs to be blocked to avoid this is not the collection itself, but a state check inside GetOptionById.

+1


source







All Articles