How often have you seen (or written) code like the following:
<cffunction name="getProductInfo" returntype="struct"> <cfargument name="sku" type="string"> <cfif not structKeyExists(application.products,arguments.sku)> <cfset application.products[arguments.sku] = this.loadProductInfo(arguments.sku)> </cfif> <cfreturn application.products[arguments.sku]> </cffunction>
Typical "if it doesn't exist make one" test, right? Now, how many people know that under the right circumstances, this function can blow up with an error?
Here's the situation. A developer at a company where I consult created this system to cache product information for a shopping cart. It's not a cached query because the information comes from several sources, and contains things like coupon lookups and upsell information.
So far, so good. Now Joe, (not his real name) was a conscientious sort, and decided that he'd better clean up after himself since the system contained a LOT of products and he didn't want the cache getting too large.
There are several ways to handle this type of situation:
- Placing a simple limit on the size of the cache. This he tossed out because doing so would entail a performance hit on any non-cached item.
- Doing a timed cache where items expire after a certain period. This he didn't want to do because somewhere or somehow he'd have to loop the cache periodically and look for expired items. (And Joe had always had a bit of a problem remembering how to do date comparisons in CF.)
- Zap the cache periodically and start over.
<cfset application.products=structNew()>
Proud of his performance enhancements, and back on schedule, Joe went on to other things. But over the next few months, reports started trickling in of the system failing and of users losing their shopping carts. Not a lot of them, but a couple a month.
Going back over the error logs, we found nothing (the entire block of code was wrapped in a non-logging cfcatch "any", another no-no). Looking at the change logs, we focused in on recent additions and modifications to the system, and found the enhancements... and the problem.
Here's the error sequence:
- Scheduled event fires.
- Elsewhere, a page request calls getProductInfo, which looks for an item in the cache and sees it's there.
- Scheduled event clears the cache.
- Function tries to return the item it "knows" is in the cache, and blows up.
So even though the structKeyExists was atomic, and the struct assignment was atomic, the two of them together were not.
What to do? Well, one thing we could do was to implement a good caching system. Or tried wrapping everything in a convoluted chain of locks.
But in the meantime we immediately replaced the function with the following:
<cffunction name="getProductInfo" returntype="struct"> <cfargument name="sku" type="string"> <cfset product=0> <cftry> <cfset product=application.products[arguments.sku]> <cfcatch type="expression"> <cfset product=this.loadProductInfo(arguments.sku)> <cfset application.products[arguments.sku]=product> </cfcatch> </cftry> <cfreturn product> </cffunction>
This pattern is called " assumed existence". In it, instead of always checking to see if an item exists, we simply assume that it does and immediately try to return one. If it doesn't exist, the code fires an exception and in catching it we create the desired item, add it to the cache, and return it.
Do a loop timing test and you'll find that with a properly loaded cache it performs almost exactly the same as the first solution, since setting up the try/catch is balanced out by eliminating the structKeyExists done on every call to the function.
This routine is a good replacement for the standard "not structKeyExists" or "not isDefined" code sequence, and definitely should be considered when you're working with items in a cache that can be cleared or manipulated elsewhere in the system.
And Joe? He's busy doing a new caching system... but that's another story.
Comments