6
\$\begingroup\$

I have this simple cache mechanism for a repeated task in service. It uses static variable to store the information. Please suggest on how is it and if it could be better.

Premise: I need to verify transaction from stores. There can be lots of transactions happening repeatedly from multiple stores. Thus, this simple cache manager for getting the store information. I want it to be simple yet effective and fast.

StoreCacheManager.cs

public class StoreCacheManager
{
    private static List<StoreCacheInformation> _merchantStores = new List<StoreCacheInformation>();
    private DBEntities _db;
    private TimeSpan _cacheTime = new TimeSpan(1, 0, 0);//1 Hour
    public TimeSpan CacheTimeSpan { get { return _cacheTime; } }
    public StoreCacheManager(DBEntities db)
    {
        _db = db;
    }

    public async Task<StoreCacheInformation> Get(int storeId)
    {
        if (_merchantStores.Any())
        {
            var store =
                _merchantStores.FirstOrDefault(i => i.StoreID == storeId);
            if (store != null)
            {
                // Check if Cache time has expired
                if (store.CacheDateTimeUtc.Add(_cacheTime) < DateTime.UtcNow)
                {
                    lock (_merchantStores)
                    {
                        _merchantStores.Remove(store);
                    }
                }
                else
                {
                    return store;
                }
            }
        }
        return await GetAndCache(storeId);
    }

    private async Task<StoreCacheInformation> GetAndCache(int storeId)
    {
        var store = await GetStoreInfo(storeId);
        if (store != null)
        {
            lock (_merchantStores)
            {
                _merchantStores.Add(store);
            }
        }
        return store;
    }
    private async Task<StoreCacheInformation> GetStoreInfo(int storeId)
    {
        var storeInfo = await _db.Stores.Where(i => i.StoreID == storeId).Select(i => new StoreCacheInformation()
        {
            CountryCode = i.CountryObj.CountryCode,
            MerchantID = i.VendorOrgID ?? 0,
            TelephoneCode = i.CountryObj.TelephoneCountryCode,
            StoreID = storeId,
            //todo: deviceId and Token

        }).FirstOrDefaultAsync();
        if (storeInfo != null)
        {
            storeInfo.CacheDateTimeUtc = DateTime.UtcNow;
        }
        return storeInfo;
    }
}

StoreCacheInformation.cs

public class StoreCacheInformation
{
    public int MerchantID { get; set; }
    public int StoreID { get; set; }
    public string TelephoneCode { get; set; }
    public string CountryCode { get; set; }
    public DateTime CacheDateTimeUtc { get; set; }
    public string DeviceId { get; set; }
    public string AuthToken { get; set; }
}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Wouldn't a ConcurrentDictionary be better for this purpose? \$\endgroup\$
    – BCdotWEB
    Jul 11 '16 at 9:33
  • \$\begingroup\$ i actually never heard of it :P i will look it up \$\endgroup\$
    – Ruchan
    Jul 11 '16 at 9:54
  • \$\begingroup\$ I have rolled back the last edit. Please see What to do when someone answers. \$\endgroup\$
    – Mast
    Jul 11 '16 at 11:06
  • \$\begingroup\$ Use MemoryCache - it's a .NET built in, just build a short wrapper around it. \$\endgroup\$ Jul 11 '16 at 13:57
6
\$\begingroup\$

Concurrency

If you want to implement it thread-safe, you have also lock the whole transaction. For instance: store != null assumes that there is no item in cache. Imagine that after that check another thread added one. That would result in a cache where the same item is cached twice.

Conside to use a thread-safe collection (e.g. ConcurrentDictionary) instead of using locking.

.Net Framework already provides a thread-safe cache: MemoryCache.

I am not very familiar with the entity framework, but as far as I know is the DbContext not thread-safe. Therefore it is not a good idea to use a single instance of it in multithreaded environments.


Code Style

  • _merchantStores, _db and _cacheTime should be read-only.
  • Methods that return a Task should be called xxxAsync
  • The property setter in StoreCacheInformation should be private or at least internal. Otherview external code may modify the state of the cached items.
  • For many cached items, it is better to use a dictionary instead of list.
\$\endgroup\$
5
  • \$\begingroup\$ The DbContext is single instace per cache manager. I intend the scope of cache manager to be per request. I will look up the ConcurrentDictionary though. \$\endgroup\$
    – Ruchan
    Jul 11 '16 at 9:56
  • \$\begingroup\$ @Ruchan: Ok, than it is OK. I didn't know that it is an ASP.Net application.... In case that you don't want to use in the build-in cache (There is also one for ASP.Net), I added few additional points regaring to code style. \$\endgroup\$
    – JanDotNet
    Jul 11 '16 at 10:36
  • \$\begingroup\$ Thanks, I have updated the code with your suggestions. Also could you suggest a way to clear expired items? \$\endgroup\$
    – Ruchan
    Jul 11 '16 at 10:52
  • 1
    \$\begingroup\$ @Ruchan: Please do not edit your question. Otherwise the answers are not valid anymore. If you want to show the refactored version, you could add it as answer or post a new question if you want to get a review again. \$\endgroup\$
    – JanDotNet
    Jul 11 '16 at 11:01
  • 1
    \$\begingroup\$ Also could you suggest a way to clear expired items? Actually I would use one of the build-in caches, because they handle that kind of problem. If you want to implement your own solution you have to ask another one because I am not an ASP.Net developer ;) \$\endgroup\$
    – JanDotNet
    Jul 11 '16 at 11:07
0
\$\begingroup\$

Update

StoreCacheManager.cs

public class StoreCacheManager
{
    //private static List<StoreCacheInformation> _merchantStores = new List<StoreCacheInformation>();
    private static readonly ConcurrentDictionary<int, StoreCacheInformation> _merchantStores =
        new ConcurrentDictionary<int, StoreCacheInformation>();
    private readonly DBEntities _db;
    private readonly TimeSpan _cacheTime = new TimeSpan(1, 0, 0);//1 Hour
    public TimeSpan CacheTimeSpan { get { return _cacheTime; } }
    public StoreCacheManager(DBEntities db)
    {
        _db = db;

        var x = new System.Web.Caching.Cache();

    }

    public StoreCacheInformation Get(int storeId)
    {
        lock (_merchantStores)
        {
            if (_merchantStores.Any())
            {
                var store =
                    _merchantStores.FirstOrDefault(i => i.Key == storeId);
                if (store.Value != null)
                {
                    // Check if Cache time has expired
                    if (store.Value.CacheDateTimeUtc.Add(_cacheTime) < DateTime.UtcNow)
                    {
                       _merchantStores.Remove(storeId);
                    }
                    else
                    {
                        return store.Value;
                    }
                }
            }
            return GetAndCache(storeId);
        }
    }

    private StoreCacheInformation GetAndCache(int storeId)
    {
        var store = GetStoreInfo(storeId);
        if (store != null)
        {
            _merchantStores.TryAdd(storeId, store);
        }
        return store;
    }
    private StoreCacheInformation GetStoreInfo(int storeId)
    {
        var storeInfo = _db.Stores.Where(i => i.StoreID == storeId).Select(i => new StoreCacheInformation()
        {
            CountryCode = i.CountryObj.CountryCode,
            MerchantID = i.VendorOrgID ?? 0,
            TelephoneCode = i.CountryObj.TelephoneCountryCode,
            StoreID = storeId,
            //todo: deviceId and Token

        }).FirstOrDefault();
        if (storeInfo != null)
        {
            storeInfo.CacheDateTimeUtc = DateTime.UtcNow;
        }
        return storeInfo;
    }
}

Changes

  • added lock to the whole transaction. The async tasks had to go since it gives error like "await" can't be used with try...catch, lock
  • made properties readonly.
  • added extension to Remove method
\$\endgroup\$
2
  • 2
    \$\begingroup\$ You could also use TimeSpan.FromHours(1) to get rid of the comment ;) \$\endgroup\$
    – JanDotNet
    Jul 11 '16 at 11:11
  • 1
    \$\begingroup\$ If you are making changes based on the review. You should consider making your answer a wiki answer IMHO. \$\endgroup\$
    – Tolani
    Jul 13 '16 at 13:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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