3
\$\begingroup\$

This is my RedisDataSource code. What I am mainly interested in is how the retry policy could be improved and if using WindowsAzure Trainsient Fault Handling this case (using Redis Azure Cache) makes sense.

   using System;
    using System.Threading.Tasks;
    using AB.Common.Helpers;
    using AB.SiteCaching.CacheContainer;
    using Microsoft.Practices.EnterpriseLibrary.WindowsAzure.TransientFaultHandling.Cache;
    using Microsoft.Practices.TransientFaultHandling;
    using Newtonsoft.Json;
    using StackExchange.Redis;

namespace AB.SiteCaching.Providers
{
    public class RedisDataSource : ICacheProvider
    {
        private readonly IDatabase _cache;
        private readonly RetryPolicy _retryPolicy;

        public string CacheRegion { get; set; }

        public RedisDataSource()
        {
            _cache = Connection.GetDatabase();
            var retryStrategy = new FixedInterval(3, TimeSpan.FromSeconds(2));
            _retryPolicy = new RetryPolicy<CacheTransientErrorDetectionStrategy>(retryStrategy);
        }

        private static readonly Lazy<ConnectionMultiplexer> LazyConnection = new Lazy<ConnectionMultiplexer>(() =>
        {
            return ConnectionMultiplexer.Connect("CONNECTIONSTRINGTHATWILLCOMEFROMWEBCONFIG");
        });

        public static ConnectionMultiplexer Connection
        {
            get
            {
                return LazyConnection.Value;
            }
        }

        public T RetrieveCached<T>(string key, Func<T> onNotCached, TimeSpan timeOut) where T : class
        {
            string fullCacheKey = string.Format("{0}_{1}", key, CacheRegion);

            var dataContainer = new CacheDataContainer<T>();

            var data = RetrieveCacheObject<T>(fullCacheKey);

            var getTask = new Task(() =>
            {
                var cached = onNotCached();

                dataContainer.CachedData = cached;
                StoreCacheObject(fullCacheKey, dataContainer, timeOut);
            });

            if (data != null)
            {
                if (DateTime.UtcNow > data.LastUpdated.AddMinutes(5))
                {
                    if (data.IsDirty == null) data.IsDirty = DateTime.UtcNow;

                    if (data.IsDirty != null && dataContainer.RequestSent == null)
                    {
                        dataContainer.RequestSent = DateTime.UtcNow;
                        getTask.Start();
                    }
                }

            }
            else
            {
                getTask.Start();
                getTask.Wait();
                data = RetrieveCacheObject<T>(fullCacheKey);
            }

            return data.CachedData;
        }

        public async Task<T> RetrieveCachedAsync<T>(string key, Func<Task<T>> onNotCached, TimeSpan timeOut) where T : class
        {
            string fullCacheKey = string.Format("{0}_{1}", key, CacheRegion);

            var dataContainer = new CacheDataContainer<T>();

            var data = RetrieveCacheObject<T>(fullCacheKey);

            if (data != null)
            {
                if (DateTime.UtcNow > data.LastUpdated.AddMinutes(5))
                {
                    if (data.IsDirty == null) data.IsDirty = DateTime.UtcNow;

                    if (data.IsDirty != null && dataContainer.RequestSent == null)
                    {
                        dataContainer.RequestSent = DateTime.UtcNow;
                        var cached = await onNotCached();
                        dataContainer.CachedData = cached;
                        StoreCacheObject(fullCacheKey, dataContainer, timeOut);
                    }
                }

            }
            else
            {
                var cached = await onNotCached();
                dataContainer.CachedData = cached;
                StoreCacheObject(fullCacheKey, dataContainer, timeOut);
                data = RetrieveCacheObject<T>(fullCacheKey);
            }

            return data.CachedData;
        }

        /// <summary>
        /// Retrieve cached object from Redis
        /// </summary>
        /// <typeparam name="T"></typeparam>
        /// <param name="fullCacheKey">Cache Key</param>
        /// <returns></returns>
        private CacheDataContainer<T> RetrieveCacheObject<T>(string fullCacheKey) where T : class
        {
            CacheDataContainer<T> data = null;
            try
            {
                var dataCache = _cache;
                var cachedString = _retryPolicy.ExecuteAction(() => dataCache.StringGet(fullCacheKey));
                if (cachedString.HasValue)
                {
                    return JsonConvert.DeserializeObject<CacheDataContainer<T>>(cachedString);
                }

            }
            catch (RedisException ex)
            {
                LoggingHelper.Log(ex);
            }

            return data;
        }

        /// <summary>
        /// Store cache object in Redis cache
        /// </summary>
        /// <typeparam name="T"></typeparam>
        /// <param name="fullCacheKey"></param>
        /// <param name="cached"></param>
        /// <param name="timeOut"></param>
        private void StoreCacheObject<T>(string fullCacheKey, CacheDataContainer<T> cached, TimeSpan timeOut)
        {
            var dataCache = _cache;
            if (dataCache != null)
            {
                _retryPolicy.ExecuteAction(() =>
                {
                    cached.LastUpdated = DateTime.UtcNow;
                    cached.IsDirty = null;
                    cached.RequestSent = null;

                    var serialized = JsonConvert.SerializeObject(cached);

                    dataCache.StringSet(fullCacheKey, serialized, timeOut);
                });
            }

        }

        /// <summary>
        /// Invalidate a cache object in Redis
        /// </summary>
        public void Invalidate(string fullCacheKey)
        {
            _cache.SetRemove(fullCacheKey, RedisValue.Null);
        }

        public bool IsSet(string fullKey)
        {
            return _cache.KeyExists(fullKey);
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Just a small thing... in some places onNotCached is awaitable, and in others it's synchronous. Is that intentional? \$\endgroup\$ – Gigi Mar 6 '15 at 13:41
  • \$\begingroup\$ @Gigi It looks intentional to me. Having one overload for synchronous delegate and one for asynchronous delegate is not uncommon. \$\endgroup\$ – svick Mar 10 '15 at 23:55
  • \$\begingroup\$ @Gigi - yes, svick is correct. This is intentional. \$\endgroup\$ – Jakub Holovsky Mar 11 '15 at 1:42
  • \$\begingroup\$ Hi Jakub! I have a lot of issue with Azure Redis to save and read object. Is your code working with Azure Redis? What do you have in CacheDataContainer? Thank you in advance. \$\endgroup\$ – Enrico Mar 2 '18 at 12:49
2
\$\begingroup\$
private static readonly Lazy<ConnectionMultiplexer> LazyConnection = new Lazy<ConnectionMultiplexer>(() =>
{
    return ConnectionMultiplexer.Connect("CONNECTIONSTRINGTHATWILLCOMEFROMWEBCONFIG");
});

You can simplify this statement lambda to expression lambda:

private static readonly Lazy<ConnectionMultiplexer> LazyConnection = new Lazy<ConnectionMultiplexer>(() =>
    ConnectionMultiplexer.Connect("CONNECTIONSTRINGTHATWILLCOMEFROMWEBCONFIG"));

var getTask = new Task(() =>
{
    var cached = onNotCached();

    dataContainer.CachedData = cached;
    StoreCacheObject(fullCacheKey, dataContainer, timeOut);
});

if (data != null)
{
    if (DateTime.UtcNow > data.LastUpdated.AddMinutes(5))
    {
        if (data.IsDirty == null) data.IsDirty = DateTime.UtcNow;

        if (data.IsDirty != null && dataContainer.RequestSent == null)
        {
            dataContainer.RequestSent = DateTime.UtcNow;
            getTask.Start();
        }
    }

}
else
{
    getTask.Start();
    getTask.Wait();
    data = RetrieveCacheObject<T>(fullCacheKey);
}

You create a cold Task here, and then decide to either ignore it, start and wait for it or just start it.

Instead of that, I think it would be simpler to use a delegate here and create the Task only in the one case where you actually need it:

Action getAction = () =>
{
    var cached = onNotCached();

    dataContainer.CachedData = cached;
    StoreCacheObject(fullCacheKey, dataContainer, timeOut);
};

if (data != null)
{
    if (DateTime.UtcNow > data.LastUpdated.AddMinutes(5))
    {
        if (data.IsDirty == null) data.IsDirty = DateTime.UtcNow;

        if (data.IsDirty != null && dataContainer.RequestSent == null)
        {
            dataContainer.RequestSent = DateTime.UtcNow;
            Task.Run(getAction);
        }
    }

}
else
{
    getAction();
    data = RetrieveCacheObject<T>(fullCacheKey);
}

if (data.IsDirty == null) data.IsDirty = DateTime.UtcNow;

So IsDirty is a DateTime?? IsDirty indicates that it's bool, if it's something else, it should have different name, something like DirtyTime.


if (data.IsDirty != null && dataContainer.RequestSent == null)
{
    dataContainer.RequestSent = DateTime.UtcNow;
    getTask.Start();
}

…

if (data.IsDirty != null && dataContainer.RequestSent == null)
{
    dataContainer.RequestSent = DateTime.UtcNow;
    var cached = await onNotCached();
    dataContainer.CachedData = cached;
    StoreCacheObject(fullCacheKey, dataContainer, timeOut);
}

In the synchronous case, you're not waiting for update of dirty data. But in the asynchronous case, you are (asynchronously) waiting for it. Such asymmetry looks suspicious, is it intentional?


CacheDataContainer<T> data = null;
try
{
    …
    if (…)
    {
        return …;
    }

}
catch …

return data;

You're never reading or writing to data again. You could remove the variable and write just return null;.


Your XML comments seem pretty useless, pretty much just repeating the method name. If you want to have them, start with the most important parts, like the class itself. And if you don't want to comment every single self-explanatory parameter, don't, but then don't leave the empty comments there.

\$\endgroup\$

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.