How to ensure that a long running method is not executed in parallel by multiple tasks?-CodePudding

I have a basic Dictionary based cache in a class.

private HashSet<string> _myCache = new HashSet<string>();

private void BuildMyCache()
        {
          
           lock(_cacheLock)
            {

                try
                {
                    _cacheRebuildInProgress = true;
                    _favouritesCache.Clear();
                    //Populate the _myCache hashset here by querying the database
                                              
                        }
                    }
                    _cacheRebuildInProgress = false;
                }
                catch (Exception ex)
                {
                    _cacheRebuildInProgress = false;
                }
            }
        }

and then I have this method in the same class which may be called by multiple tasks, all running at the same time.

public bool ValueExistsInMyCache(string valueToSearch)
        {
            while(true)
            {
                if(!_cacheRebuildInProgress)
                {
                    if (_myCache.Count == 0 || stopWatch.ElapsedMilliseconds / 1000 / 60 >= _cacheSettings.TimespanMinutes)
                    {
                        BuildMyCache();
                        stopWatch.Restart();                       
                    }
                    break;
                }
                else
                {
                    Thread.Sleep(1000);
                }
                
            }
           
            var result = _myCache.Contains(valueToSearch);
            return result;
        }

So the way I see this working ideally is:

1- Multiple tasks might try to read from my cache at the same time. That's fine

2- If the cache is being rebuilt, I'd like the rebuild method to be called only once, making any subsequent task to wait while it is rebuilt and then search the cache that was rebuilt

The question is, does the above code accomplish 2, and also I should consider using a caching library for this simple use case and save myself the headache of having to deal with hard to debug situations later because I might get how I've written this wrong.

Thanks

CodePudding user response：

It sounds like you want a read/write lock. You allow multiple readers at the same time, but you only want to allow one writer (the thing rebuilding the cache) at a time, and the writer can't operate while any of the readers are reading.

private readonly ReaderWriterLockSlim _myCacheLock = new();
private readonly HashSet<string> _myCache = new HashSet<string>();

private void BuildMyCache()
{
    _myCacheLock.EnterWriteLock();
    try
    {
        _myCache.Clear();
        //Populate the _myCache hashset here by querying the database
    }
    finally
    {
        _myCacheLock.ExitWriteLock();
    }
}

public bool ValueExistsInMyCache(string valueToSearch)
{
    _myCacheLock.EnterReadLock();
    try
    {
        return _myCache.Contains(valueToSearch);
    }
    finally
    {
        _myCacheLock.ExitReadLock();
    }
}

That said, it might be easier to built a new cache into a new HashSet<string>, and only write that to _myCache when it's done. This has the advantage that readers aren't blocked while a new cache is being rebuilt: they'll just continue using the old cache until the new one is ready.

You'll still need a lock around reads/writes of the _myCache field as reading threads are allowed to simply ignore changes to this field otherwise.

private readonly object _myCacheLock = new();
private HashSet<string> _myCache = new HashSet<string>();

private void BuildMyCache()
{
    var newCache = new HashSet<string>();
    // Populate newCache here by querying the database
    lock (_myCacheLock)
    {
        _myCache = newCache;
    }
}

public bool ValueExistsInMyCache(string valueToSearch)
{
    HashSet<string> myCache;
    lock (_myCacheLock)
    {
        myCache = _myCache;
    }
    return myCache.Contains(valueToSearch);
}

Note that this approach won't stop multiple concurrent calls to BuildMyCache from all building the cache in parallel. This is "safe", but probably wasteful if this is likely to happen.

CodePudding user response：

What you've created yourself is a spinlock with an expiring cache. The latter is what the other answers seem to have not taken into account.

You may get your solution to work but it's probably not going to be the most performant or stable and it's best to not reinvent the wheel in general.

Have a look at IMemoryCache with it's extension methods. You will have to add Microsoft.Extensions.Caching.Abstractions and Microsoft.Extensions.Caching.Memory nuget packages.

Once you've injected your memory cache object though, it's as simple as calling the GetOrCreate extension method and it will lock appropriately as well as ensure that your expiry date is upheld.

public bool ValueExistsInMyCache(string valueToSearch)
{
    var hashSet = cache.GetOrCreate("myCache", entry =>
    {
        entry.AbsoluteExpirationRelativeToNow = TimeSpan.FromMinutes(_cacheSettings.TimespanMinutes);
        var hashset = new HashSet<string>();
        //populateHashset here
        return hashset;
    });

    return hashSet.Contains(valueToSearch);
}

The above code is a quick example - you could further refactor it to not need the hashSet anymore but I'll leave that up to you :-)

CodePudding user response：

You could use Interlocked.CompareExchange.

if(Interlocked.CompareExchange(ref myField, 1, 0) == 0){

}

That should ensure that only one caller succeeds in the check, and all other fail. If you want to run the check multiple times you obviously need to reset the field, and using lock or memoryBarrier to ensure the write is not moved around by the compiler or CPU.

Also note that HashSet<string> is not thread safe, so if there is any chance of concurrent access (with exception of concurrent reads), all accesses need to be inside a lock, or possibly some other synchronization mechanism. The posted example code looks unsafe to me. A concurrentDictionary might be a much safer option to use. That sleep also looks like a poor design, if some other thread is updating the cache it might be a better option to wait for that to be done using some kind of event.