Thread safety and memory leak consideration on lookup collection with synchronization

Question

In a program I am working on (an ASP.NET application) I have an in-memory lookup collection that is static readonly (shared among all thread of the ASP.NET application).

I know this is not orthodox but that was made to have an in-memory version of a remote and slow DB, to improve performance of some lookup operations.

The collection is then accessed by ASP.NET pages that need to do the lookup, so it's accessed by many threads, but that's not an issue because it's guaranteed that those threads only read the collection.

Sometimes I need to refresh the collection content, to do so in a background thread (with a lock), I work on a temporary variable of the same type of static lookup collection.

When I finished all the work, I simply assign the temp variable to the static lookup collection, if meanwhile the "swapping" is completed someone accessed the old data, that's not an issue (reading old data is accepted as long sooner or later the data will be updated).

I think that there should be not any issued, but am a bit scared about race condition of a such situation. Another aspect that scare me a bit are memory leaks.

I start considering Interlock.Exchange, but not sure if fit my situation.

Here is the base class I use to handle the in-memory collection that represents lookup structure of a slow and remote database:

//In memory indexed collection to query slow table by key when table does not contain too much data and is somehow slow (eh remote system)
public class InMemoryStore<T,K> 
    where T : new()
    where K : struct
{
    protected object _instanceSync = new object();
    protected DateTime _lastRebuild = ConstantsEntry.MIN_DATE;
    protected TimeSpan _timeout = new TimeSpan(0, 15, 0);
    protected int _cutoff = 1000;

    protected ReadOnlyDictionary<K, T> _preparingData = null;
    protected ReadOnlyDictionary<K, T> _readyData = null;
    protected volatile bool _isSyncRunning = false;
    protected DateTime _syncronizationStart = ConstantsEntry.MIN_DATE;

    protected InMemoryStore(TimeSpan timeout, int cutoff)
    {
        _timeout = timeout;
        _cutoff = cutoff;
    }

    // Best effort query against the store using only key as search means
    public List<T> Query ( List<K> query)
    {
        List<T> result = new List<T>();
        var dictionaryRef = _readyData;

        if (dictionaryRef == null)
            return result;

        foreach (var key in query)
            if (dictionaryRef.TryGetValue(key, out T value))
                result.Add(value);

        return result;
    }

    // Standard logic to rebuild internal index, provide some extension point to customize memory constraints and table access
    public void Rebuild()
    {
        try
        {
            lock (_instanceSync)
            {
                if (_isSyncRunning && (DateTime.UtcNow - _syncronizationStart) < new TimeSpan(0,30,0) )
                    return;

                if (this.MemoryLimitExceeded(cutoff: _cutoff))
                {
                    _readyData = null;
                    _preparingData = null;
                }

                _preparingData = null;
                _isSyncRunning = true;
                _syncronizationStart = DateTime.UtcNow;

                Task.Run(() =>
                {
                    try
                    {
                        this.InternalRebuildLogic();

                        if (_preparingData != null)
                        {
                            _readyData = _preparingData
                            //or better --> Interlocked.Exchange<ReadOnlyDictionary<K, T>>(ref _readyData, _preparingData);

                            _preparingData = null;
                            _lastRebuild = DateTime.UtcNow;
                        }
                    }
                    catch (Exception threadErr) { }
                    finally
                    {
                        _isSyncRunning = false;
                    }
                });
            }
        }
        catch (Exception err) { }
    }

    
    //Extension point to execute custom index rebuild
    protected virtual void InternalRebuildLogic() {}

    // Check there is not too much item in the collection 
   
    protected virtual bool MemoryLimitExceeded (int cutoff) {}
}

Here is a concrete implementation :

public class ArticleInMemoryStore : InMemoryStore<tbl_ana_Article, int>
{
    protected ArticleInMemoryStore(TimeSpan timeout, int cutoff) : base(timeout, cutoff){}

    protected override bool MemoryLimitExceeded(int cutoff)
    {
        using ( var context = ServiceLocator.ConnectionProvider.Instance<ArticleDataContext>())
        {
            int entries = context.tbl_ana_Articles.Count();
            return entries > cutoff;
        }
    }

    protected override void InternalRebuildLogic()
    {
        using (var context = ServiceLocator.ConnectionProvider.Instance<ArticleDataContext>())
        {
            var dictionary = context.tbl_ana_Articles.ToList().ToDictionary(item => item.ID, item => item);
            _preparingData = new ReadOnlyDictionary<int,tbl_ana_Article>(dictionary);
        }
    }
}

I would like to have feedback on possible issues related to memory-leak and race-condition that i possibly have overlooked in such scenario.

For example I am asking myself if using Interlocked.Exchange is better than just variable assignment as I did.

Answer 1

I can see a problem here:

protected ReadOnlyDictionary<K, T> _readyData = null;

The _readyData field is not volatile, and is accessed inside the Query method without synchronization or explicit memory barrier:

var dictionaryRef = _readyData;

The problem is that without a fence the .NET Jitter is allowed to reorder instractions in a way that would allow the thread running the Query method to see a partially initialzed ReadOnlyDictionary<K, T> object. The probability of this happening is small, and depending on the hardware on which the program runs it might be zero. If you don't like gambling, my suggestion is to declare the field as volatile.

You can find answers that explain in more details the danger associated with the lack of a memory barrier in this question: The need for volatile modifier in double checked locking in .NET.