Redis Cache Data source code - Retrieve/Set/Invalidate actions

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(retryStrategy);
}

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

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

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

var dataContainer = new CacheDataContainer();

var data = RetrieveCacheObject(fullCacheKey);

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

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

if (data != null)
{
if (DateTime.UtcNow > data.LastUpda

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

You can simplify this statement lambda to expression lambda:

private static readonly Lazy LazyConnection = new Lazy(() =>
    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(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(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 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.

StackExchange Code Review Q#83262, answer score: 2