Generic cached value class mimicking Lazy<T>

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 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 {
    private readonly TimeSpan _duration;
    private DateTime _expiration;
    private Lazy _value;
    private readonly Func _valueFactory;

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

            return _value.Value;
        }
    }

    public Cached(Func 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(() => _valueFactory());
            _expiration = DateTime.Now.Add(_duration);
        }
    }
}

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

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:

• move the lock up to the place where the expiration date is being checked.

• 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(_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's constructor:

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

StackExchange Code Review Q#59805, answer score: 13