Does this Caching function work how I think it does?

Question

I've tentatively written this method:

public static Func<T> WeakCacheFor<T>( Func<T> provider ) where T: class
{
    var cache = new WeakReference(null);
    return () => {
        var x = (T)cache.Target;
        if( x == null )
        {
            x = provider();
            cache.Target = x;
        }
        return x;
    };
}

So a bit of background:

I have some long-winded legacy methods which look a bit like this:

var id = GetDatabaseId();
if(a){
    var data = GetLoader().Init(id).GetData(); // expensive!
    // do stuff with data
}
if(b){
    // don't load data
}
... lots more variations, some contain GetLoader().Init(id).GetData(); some don't....

My potential solution is to do this:

var id = GetDatabaseId();
var loadData = WeakCacheFor(() => GetLoader().Init(id).GetData());
if(a){
    var data = loadData();
    // do stuff with data
}
if(b){
    // don't load data
}
... lots more variations, some contain loadData(); some don't....

My thoughts about this:

• I don't need to cache beyond the scope of this method call, so it's fine if the GC collects it as soon as the method returns
• If the code takes a path which doesn't need to load the data, the hit won't be incurred
• If it does need the data it will be cached in the weak reference if it's needed again.
• If the GC does collect midway, it won't matter as it will just get re-loaded.

My questions:

1. Will this actually work? - Is there anything that I've missed in the WeakCacheFor method that might cause a strong reference to inadvertenly be held?
2. Am I being too clever for my own good? - Should I just incur the hit and cache the data in a normal local variable even if it's not needed?

I suspect I may be being too clever, but even if I am, does this seem to anyone else like a solution which can be usefully applied in other situations??

Update: Modified function because apparently you can't trust .IsAlive

Update: I realized that the returned Func will go out of scope at the end of the method, so I don't need a weakref at all and a normal ref will work just fine. I Was suffering from a case of "can't see the forest for the trees" I think.

Answer 1

I don't see any point in using a weak reference. Once you have loaded the data there is hardly any reason to throw it away until you are sure that it's not useful any more.

What you are implementing is a variation of the lazy loading pattern. Stick to the simple pattern and just use a regular reference to the item:

public static Func<T> LazyLoad<T>(Func<T> provider) where T : class {
   T item = null;
   return () => {
      if (item == null) {
         item = provider();
      }
      return item;
   };
}

(And a small tip: You are using the var keyword way too much.)

Answer 2

Couple of general comments:

1. Your solution (as well as my) is not thread safe.
2. It is not designed to work with IDisposable objects.

For the following discussion consider:

foo = WeakCacheFor<Foo>(() => CreateFoo());

Case # 1: Using foo as a long living variable (e.g. a member of the long living class, or a global variable)

Your solution makes sence here. The variable will be create when needed, and destroyed when system frees resources during the GC.

But notice, that if foo is time-expensive but memory-cheap, than probably it makes sence to use singleton pattern instead and load it once for the duration of the application?

Case # 2. Using foo as a local variable.

In this case it is better to use singleton pattern, I guess. Consider such example:

static void Main(string[] args)
{
    MethodA(5, 7);
    MethodA(8, 9);
}

static void MethodA(int a, int b)
{
    var foo = WeakCacheFor<Foo>(() => new Foo());

    if (a > 3)
    {
        Use(foo);

        if (a * b == 35)
        {
            GC.Collect(); // Simulate GC
            Use(foo);
        }
        else if(b % 6 == 2)
        {
            Use(foo);
        }
    }
}

The foo will be created 3 times. And 2 times if you comment the "Simulate GC" line. Also you can't use this for IDisposable classess.

Now let's try a singleton:

static void MethodA(int a, int b)
{
    using (var foo = new MySingleton<Foo>(() => new Foo()))
    {
        if (a > 3)
        {
            Use(foo);

            if (a * b == 35)
            {
                GC.Collect(); // Simulate GC
                Use(foo);
            }
            else if (b % 6 == 2)
            {
                Use(foo);
            }
        }
    }
}

As you can see, the code almost did not change, but now we get only 2 calls to foo ctor and IDisposable support.

A rough singleton implementation:

class MySingleton<T> : IDisposable
    where T : class
{
    private T _value;
    private Func<T> _provider;

    public MySingleton(Func<T> provider)
    {
        _provider = provider;
    }

    public T Get()
    {
        if (_value == null)
        {
            _value = _provider();
        }

        return _value;
    }

    #region IDisposable Members

    public void Dispose()
    {
        if(_value == null)
            return;

        IDisposable disposable = _value as IDisposable;

        if(disposable != null)
            disposable.Dispose();
    }

    #endregion
}

And the rest of the code:

class Foo : IDisposable
{
    public void Dispose() {}
}

static void Use(MySingleton<Foo> foo)
{
    foo.Get();
}

static void Use(Func<Foo> foo)
{
    foo();
}