Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.

share|improve this question

migrated from stackoverflow.com Aug 12 at 14:56

This question came from our site for professional and enthusiast programmers.

2 Answers 2

up vote 11 down vote accepted

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)
share|improve this answer
    
Good tip on locking this. I'll make that change as you suggested. Would it be enough to lock (_value) or should I have a dummy object instead? –  Yuck Aug 12 at 15:20
    
If often you have a few threads and a long expiration time compared to the time it takes to create the resource, moving the check outside the lock may make it slower rather than faster. –  Ben Aaronson Aug 12 at 15:21
    
Ah okay, never mind, you added in the double-check thing :) –  Ben Aaronson Aug 12 at 15:22
    
@BenAaronson Yeah, I gave it a second thought, and I think it's much better to encapsulate the thread-safety strategy along with the refreshing. The double-checked locking will also avoid a perf hit 99% of the time. –  dcastro Aug 12 at 15:28
1  
@Yuck the problem is that _value is not immutable. Use a -private- and -readonly- field. Created a dedicated object for this is a good practice –  dcastro Aug 12 at 15:32

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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