Thread safe re-initialization of concurrent dictionary

Question

I want to know if the following code is thread safe, which I assume it is not. And how I could possibly make it thread safe?

Basically I have a ConcurrentDictionary which acts as a cache for a database table. I want to query the DB every 10 seconds and update the db cache. There will be other threads querying this dictionary the whole time.

I can't just use TryAdd as there may also me elements which have been removed. So I decided instead of searching through the entire dictionary to possibly update, add or remove. I would just reinitialize the dictionary. Please do tell me if this is a silly idea.

My concern is that when I reinitialize the dictionary the querying threads will not longer by thread safe for the instance when the initialization takes place. For that reason I have used a lock for the dictionary when updating it, However I am not sure if this is correct as the object changes in the lock?

private static System.Timers.Timer updateTimer;
private static volatile Boolean _isBusyUpdating = false;
private static ConcurrentDictionary<int, string> _contactIdNames;

public Constructor()
{
    // Setup Timers for data updater         
    updateTimer = new System.Timers.Timer();
    updateTimer.Interval = new TimeSpan(0, 0, 10, 0).TotalMilliseconds;
    updateTimer.Elapsed += OnTimedEvent;
    // Start the timer
    updateTimer.Enabled = true;
}

private void OnTimedEvent(Object source, System.Timers.ElapsedEventArgs e)
{
    if (!_isBusyUpdating)
    {
        _isBusyUpdating = true;
        // Get new data values and update the list
        try
        {
            var tmp = new ConcurrentDictionary<int, string>();
            using (var db = new DBEntities())
            {
                foreach (var item in db.ContactIDs.Select(x => new { x.Qualifier, x.AlarmCode, x.Description }).AsEnumerable())
                {
                    int key = (item.Qualifier * 1000) + item.AlarmCode;
                    tmp.TryAdd(key, item.Description);
                }
            }
            if (_contactIdNames == null)
            {
                _contactIdNames = tmp;
            }
            else
            {
                lock (_contactIdNames)
                {
                    _contactIdNames = tmp;
                }
            }
        }
        catch (Exception e)
        {
            Debug.WriteLine("Error occurred in update ContactId db store", e);
        }
        _isBusyUpdating = false;
    }
}

    /// Use the dictionary from another Thread
    public int GetIdFromClientString(string Name)
    {
        try
        {
            int pk;
            if (_contactIdNames.TryGetValue(Name, out pk))
            {
                return pk;
            }
        }
        catch { }
        //If all else fails return -1
        return -1;
    }

Answer 1

You're right your code is not thread safe.

1. You need to lock _isBusyUpdating variable.
2. You need to lock _contactIdNames every time, not only when its not null.

Also this code is similar to singleton pattern and it has the same problem with initialization. You can solve it with Double checked locking. However you also need double checked locking when accessing entries.

In the case when you updating whole dictionary at once you need to lock current value every time when accessing. Otherwise you can access it while it's still changing and get error. So you either need to lock variable each time or use Interlocked.

As MSDN says volatile should do the trick with _isBusyUpdating, it should be thread safe.

If you don't want to keep track of _contactIdNames thread safety try to implement update of each entry on the same dictionary. The problem will be in difference detection between DB and current values (what entries have been removed or added, others can be simple rewritten), but not in thread safety, since ConcurrentDictionary is already thread safe.

Answer 2

You seem to be making a lot of work for yourself. Here's how I would tackle this task:

public class Constructor
{
    private volatile Dictionary<int, string> _contactIdNames;

    public Constructor()
    {
        Observable
            .Interval(TimeSpan.FromSeconds(10.0))
            .StartWith(-1)
            .Select(n =>
            {
                using (var db = new DBEntities())
                {
                    return db.ContactIDs.ToDictionary(
                        x => x.Qualifier * 1000 + x.AlarmCode,
                        x => x.Description);
                }
            })
            .Subscribe(x => _contactIdNames = x);
    }

    public string TryGetValue(int key)
    {
        string value = null;
        _contactIdNames.TryGetValue(key, out value);
        return value;
    }
}

I'm using Microsoft's Reactive Extensions (Rx) Framework - NuGet "Rx-Main" - for the timer to update the dictionary.

The Rx should be fairly straightforward. If you haven't seen it before in very simple terms it's like LINQ meets events.

If you don't like Rx then just go with your current timer model.

All this code does is create a new dictionary every 10 seconds from the DB. I'm just using a plain dictionary since it is only being created from one thread. Since reference assignment is atomic then you can just re-assign the dictionary when you like with complete thread-safety.

Multiple threads can safely read from a dictionary as long as the elements don't change.

Answer 3

I want to know if the following code is thread safe, which I assume it is not. And how I could possibly make it thread safe?

I believe it's not. First of all i'd create property for ConcurrentDictionary and check if update is underway inside get method, and if it is, i'd return the previous version of object :

    private object obj = new object();
    private ConcurrentDictionary<int, string> _contactIdNames;
    private ConcurrentDictionary<int, string> _contactIdNamesOld;
    private volatile bool _isBusyUpdating = false;

    public ConcurrentDictionary<int, string> ContactIdNames
    {
        get
        {
            if (!_isBusyUpdating) return _contactIdNames;

            return _contactIdNamesOld;
        }
        private set 
        {
            if(_isBusyUpdating) _contactIdNamesOld = 
                new ConcurrentDictionary<int, string>(_contactIdNames);

            _contactIdNames = value; 
        }
    }

And your method can be :

    private static void OnTimedEvent(Object source, System.Timers.ElapsedEventArgs e)
    {
        if (_isBusyUpdating) return;

        lock (obj)
        {

            _isBusyUpdating = true;
            // Get new data values and update the list

            try
            {
                ContactIdNames = new ConcurrentDictionary<int, string>();
                using (var db = new DBEntities())
                {
                    foreach (var item in db.ContactIDs.Select(x => new { x.Qualifier, x.AlarmCode, x.Description }).AsEnumerable())
                    {
                        int key = (item.Qualifier * 1000) + item.AlarmCode;
                        _contactIdNames.TryAdd(key, item.Description);
                    }
                }                    
            }
            catch (Exception e)
            {
                Debug.WriteLine("Error occurred in update ContactId db store", e);        
                _contactIdNames = _contactIdNamesOld;            
            }
            finally 
            {                   
                _isBusyUpdating = false;
            }
        }
    }

P.S.

My concern is that when I reinitialize the dictionary the querying threads will not longer by thread safe for the instance when the initialization takes place. For that reason I have used a lock for the dictionary when updating it, However I am not sure if this is correct as the object changes in the lock?

It's ConcurrentDictionary<T> type is threadsafe and not the instance of it, so even if you create new instance and change the reference to it - it's not something to worry about.