Generic cached value class mimicking Lazy<T>

Question

Before I wrote this I searched and found a number of solutions that make use of a caching provider to handle a set of items. I felt that was too cumbersome of an approach and set out to create a class that could cache a single value, similar to the way Lazy<T> manages the initialization of a single value.

I'm having trouble determining the thread-safety of this implementation, specifically where lock statements should be used. Can any experts on working with multi-threaded applications spot where I may have gone wrong? Or is this approach sound?

public class Cached<T> {
    private readonly TimeSpan _duration;
    private DateTime _expiration;
    private Lazy<T> _value;
    private readonly Func<T> _valueFactory;

    public T Value {
        get {
            // Do I need locking while checking the expiration?
            if (DateTime.Now >= _expiration)
                RefreshValue();

            return _value.Value;
        }
    }

    public Cached(Func<T> valueFactory, TimeSpan duration) {
        _duration = duration;
        _valueFactory = valueFactory;

        RefreshValue();
    }

    private void RefreshValue() {
        // Is this sufficient while updating the expiration and value?
        lock (this) {
            _value = new Lazy<T>(() => _valueFactory());
            _expiration = DateTime.Now.Add(_duration);
        }
    }
}

I'm using Lazy<T> for the value getter because it's possible that the cached value will be (1) expensive to load and (2) not immediately needed.

Answer 1

First off, never lock on this. You never know who else is locking on that instance. Conversely, clients don't know that the cache is locking on itself either. For example, this seemingly innocent code would lead to a deadlock:

//Thread A
lock(cache)
lock(someObject)
{
    //do something
}

//Thread B
lock(someObject)
{
    //here, the cache will lock on itself.
    //the locks are accidentally taken in reverse order -> deadlock!
    var x = cache.Value;
}

That's why it's good practice to lock on a private readonly object. Moving on...

Your current implementation allows the value to be refreshed twice in a row. This is a problem if the resource is expensive to create (and resources worth caching usually are).

Two threads might check whether DateTime.Now >= _expiration, then both go into the RefreshValue method and execute the critical region in quick sucession. To solve this you can either:

1. move the lock up to the place where the expiration date is being checked.
2. move the expiration check inside the RefreshValue method.

I would go with option 2, and add a double-checked lock to avoid hitting the lock every single time:

private readonly object _refreshLock = new object();

private void RefreshValueIfNeeded()
{
    if (DateTime.Now >= _expiration)
    {
        lock (_refreshLock)
        {
            if (DateTime.Now >= _expiration)
            {
                _value = new Lazy<T>(_valueFactory);
                _expiration = DateTime.Now.Add(_duration);
            }
        }
    }
}


public T Value
{
    get
    {
        RefreshValueIfNeeded();
        return _value.Value;
    }
}

One more thing: you don't need to wrap the value factory into another delegate, you can just pass it directly to Lazy<T>'s constructor:

//change this
new Lazy<T>(() => _valueFactory())
//to
new Lazy<T>(_valueFactory)

Answer 2

What you could get is multiple threads calling RefreshValue, waiting on that lock, then one at a time they run the code inside the lock. That's inefficient but shouldn't lead to any actual bugs.

If you want to avoid this, you could put a lock on the expiration check. However, I'd suggest profiling this, because this will potentially be hit many times more than RefreshValue, and potentially may have a greater performance cost.

Another alternative might be to have the an initial expiration check as it is, then a second, identical one inside the RefreshValue lock, returning immediately if it hasn't expired. This will mean most calls will be able to go through without ever hitting the lock, and if multiple threads do end up entering RefreshValueone after the other, only one will actually do the expensive value calculation. Again, you should choose between this and one of the other two based on profiling with what you think are reasonable expiration intervals.