Wierd Caching

Monday, January 30, 2006

Here is a (greatly simplified) piece of code that has been happily running on 13 production servers for one year.

private void DoWork()
{
    DataObject data = Cache[
"DataObject"] as DataObject;
    
if (data == null)
    {
        InitializeCache();
        data = Cache[
"DataObject"] as DataObject;
    }
            
    data.ToString();
}

private void InitializeCache()
{
    Cache[
"DataObject"] = new DataObject();
}

The above code has a flaw. We can’t depend on a cached object still being in the cache when we are ready to use it. The code assumes it can place an object in the cache and (almost) immediately pull it back out. Even with the flaw, the code never caused a problem in production environments that log all exceptions. 

The weird part is that the data.ToString() line occasionally throws a NullReferenceException on one (and only one) developer machine. How odd that the code fails on a developer machine serving a single request at a time. It’s good the weirdness occurred, because it pointed out a problem. The fix is to return a reference from the initialize cache method, and not to depend on the ASP.NET cache to keep the object alive.


Comments
Scott Monday, January 30, 2006
Could you explain the flaw a little more for those of us that are intellectually challenged, like myself. :) I'm not seeing a bug. Is there a race condition when you create the cache and when the items in the cache are available? Because if there is that seems like a pretty big bug in ASP.NET. I've never used the cache in quite that way before. I've always created accessor methods for getting things out of the cache rather than having my calling code determine whether or not it should initialize the cache.
scott Monday, January 30, 2006
No problem, Scott.

It is a race condition, but let's just try this code:

1: Cache["DataObject"] = new DataObject();
2: DataObject data;
3: data = Cache["DataObject"] as DataObject;
4: data.ToString();

A lot can happen between line 1 and line 3. It's theoretically possible that another thread might be caching lots of objects and bump our DataObject out of the cache.
scott Monday, January 30, 2006
Does that make the flaw clear?
a Monday, January 30, 2006
you're saying to add a line that says return x in the function InitializeCache ?
Quaser Monday, January 30, 2006
Yes he is, presumeable like this:

private DataObject InitializeCache()
{
DataObject data = new DataObject();
Cache["DataObject"] = data;
return data
}
scott Monday, January 30, 2006
Yes, a better version would look like:

private void DoWork()
{
DataObject data = Cache["DataObject"]
as DataObject;
if (data == null)
{
data = CreateAndCacheData();
}
data.ToString();
}

private DataObject CreateAndCacheData()
{
DataObject result = new DataObject();
Cache["DataObject"] = result;
return result;
}

The original code was returning a reference indirectly through the Cache, but the Cache doesn't guarantee it will hold onto an object forever, and a few nanoseconds is a long time on a computer.

A better version returns the reference directly to the client.

Scott Monday, January 30, 2006
hmmm, interesting. I was thinking mostly along threading issues and that using the lock or some kind syncLock object would do the trick. But your statement "the Cache doesn't guarantee it will hold onto an object forever" gives me pause.

What good is a cache that doesn't, well, cache? I think of the cache as a bank. As long as I go to the bank during business hours (e.g. during the running time of the application), I should be able to withdraw any money I've put in there.

Or are you saying that other threads could remove/invalidate your cached object in between the putting and the getting in the above example, resulting in an exception. If that's what you mean, then a lock statement would go a little ways towards solving that problem I think.
scott Monday, January 30, 2006
A lock would be useful here too, but don't forget the cache itself can "automatically remove seldom used or unimportant items". I think it would be a rare case where the cache removed an object we put in on the previous line of code, but like I say, there just isn't a guarantee, so it's better to be safe and return the reference explicitly.
a Tuesday, January 31, 2006
Thanks for the replies guys.
Joshua Flanagan Thursday, February 2, 2006
One thing that will help: instead of adding the item to the cache using the indexer property, use the explicit Add or Insert method that takes a CacheItemPriorty parameter. Set the CacheItemPriority to NotRemovable.
That will at least help avoid the case where your item gets evicted from the cache to make room for other data. It will only be removed based on the rules you defined (a time limit, or a dependency). That makes the cache more predictable.
Of course you don't want to use that setting in every case, because the Cache should be allowed to evict most data when it needs the memory.
Comments are now closed.
by K. Scott Allen K.Scott Allen
My Pluralsight Courses
The Podcast!