Loading...

cfwheels@googlegroups.com

[Prev] Thread [Next]  |  [Prev] Date [Next]

[cfwheels] Re: EXPIRESAT, imageTags and wheels caching Michael Mon Feb 20 17:00:52 2012

Hi Per,

We are running 1.1.7

-Michael

On Feb 19, 2:41 pm, Per Djurner <[EMAIL PROTECTED]> wrote:
> I'll try and look into this further.
> What version of Wheels are you running?
> Thanks for the detailed report.
>
> / Per
>
>
>
>
>
>
>
> On Sat, Feb 18, 2012 at 1:17 AM, Michael <[EMAIL PROTECTED]> wrote:
> > Hi,
>
> > I'm not much of a contributor to the group, but I like wheels, so as
> > always I'll point out problems as I find them. Normally, I like to
> > offer a solution before pointing stuff out, this particular problem is
> > more of a challenge and I don't have a good one yet.
>
> > We have been getting the following error in our site error list over
> > the past couple of weeks:
> > "Element EXPIRESAT is undefined in a CFML structure referenced as part
> > of an expression."
>
> > The stack trace has not been particularly clear, and the error has
> > been occuring on random pages, but always where I am creating an
> > imageTag. I dug into the issue, and I know the exact cause, but there
> > are a number of subtle errors and a fix seems risky due to the
> > coreness of this code.
>
> > This is how it happens:
> > 1.  Someone uses an imageTag in a view:
> > #imageTag(source=request.crateImage, alt="", class="FloatLeft")#
>
> > 2. /wheels/view/assets.cfm -> imageTag gets invoked.
>
> > 3. in imageTag, application.wheels.cacheImages is checked for
> > existence, if it exists, the cache is checked:
> > loc.returnValue = $doubleCheckedLock(name=loc.lockName,
> > condition="$getFromCache", execute="$addImageTagToCache",
> > conditionArgs=loc.conditionArgs, executeArgs=loc.executeArgs);
>
> > 4. /wheels/global/cfml.cfm -> $doubleCheckedLock is invoked. The
> > condition "getFromCache" is executed, and returns false since the
> > imageTag doesn't exist.
>
> > 5. $doubleCheckedLock locks the arguments.name (loc.lockName above).
>
> > 6. Again, the condition "getFromCache" is executed, but this time
> > inside the lock. Again, it returns false, since imageTag doesnt exist.
>
> > 7. Next, the execute function ($addImageTagToCache) is executed, since
> > the getFromCache failed. $addImageTagToCache calls the standard
> > $addToCache in /wheels/global/internal.cfm
>
> > 8. Now, addToCache determines that the item should be cached and
> > starts building the struct. This is where it gets interesting:
> > line of code 581: application.wheels.cache[arguments.category]
> > [arguments.key] = {};
> > line of code 582: application.wheels.cache[arguments.category]
> > [arguments.key].expiresAt = ...
>
> > 9. line #581 executes and completes. The struct is created. At this
> > moment another thread pre-empts. It has already #1, #2, and #3 above
> > for the same image (remember, loc.lockName is based on the imageTag's
> > argument set, which can be oft repeated.)
>
> > 10. Thread #2 executes the first step of $doubleCheckedLock which is
> > to determine if the cache entry exists. It does this by once again
> > invoking the getFromCache condition
>
> > 11. Line 599 in /wheels/global/internal.cfm is hit. Remember that line
> > 582 in thread #1 has not yet run, so there is no expiresAt in the
> > struct.
> > 599: if (Now() > application.wheels.cache[arguments.category]
> > [arguments.key].expiresAt)
>
> > 12. CF Exception is thrown.
>
> > So there are a lot of morals to learn in this story. The first and
> > most obvious is that $doubleCheckedLock is flawed and is not a valid
> > synchronization technique. You would only notice errors in larger
> > applications, but the ability to say that CFWheels can scale is a key
> > feature for uptake.
>
> > My first reaction would be to just fix the doubleCheckedLock function
> > by removing the unlocked condition check, but it is used extensively
> > in the application for caching imageTags, pages, partials, actions,
> > models and controllers. The fix for doubleCheckedLock is to not be
> > double checked, and that would probably result in a performance hit.
>
> > The other issue I have with the way doubleCheckedLock is written is
> > the over-reliance on generic coding. It looks cool, and works fine
> > most of the time, but is particularly incorrect when used with
> > getFromCache as the condition that is invoked.
>
> > <cfset loc.returnValue = $invoke(method=arguments.condition,
> > invokeArgs=arguments.conditionArgs)>
>
> > Hypothetically speaking, you would expect this call to have exactly
> > two output pathways:
> > Input=condition, Output=true (if exists), false (otherwise)
>
> > However, getFromCache abuses the genericness, and actually has 5
> > pathways:
> > Input=condition
> > Output #1: false (if no object in cache)
> > Output #2: false (object was in cached, but was subsequently removed)
> > Output #3: a simple value other than false (if a simple type was
> > cached)
> > Output #4: false (if someone happened to cache the value of "false"
> > for a key)
> > Output #5: the duplicate of an object that was cached
>
> > An astute eye will note that for the doubleCheckedLock algorithm to
> > even have a hope of being threadsafe, only output #1 is valid. All of
> > the other 4 pathways are not only referencing the supposedly protected
> > value, but in some cases are even modifying it (ie removing from
> > cache)!
>
> > As hesitant as I am to say it, it is true that the site error could be
> > fixed simply by adding a structKeyExists to line 599 of /wheels/global/
> > internal.cfm:
> > if (StructKeyExists(application.wheels.cache[arguments.category]
> > [arguments.key], "expiresAt") && Now() >
> > application.wheels.cache[arguments.category][arguments.key].expiresAt)
>
> > However, this avoids the point that getFromCache should not be used
> > with doubleCheckedLock as well as the point that doubleCheckedLock
> > should not be used at all.
>
> > My recommendation to the wheels team:
> > 1. add an isInCache function. If it needs to check expiresAt, fine,
> > but be sure to structkeyexists check it first. Function should purely
> > return true or false. Use isInCache as the condition for all
> > doubleCheckedLock calls. Because it is simplified, it should be a lot
> > faster and be safer to call from within a lock.
> > 2. change getFromCache to not remove stuff in the cache. Call
> > isInCache to see if there is something to return. If there is, return
> > it. Otherwise dont return anything. People don't expect a get call to
> > perform a write/delete. They expect it to be read only.
> > 3. put the cull behavior into addToCache. In the beginning of
> > addToCache, check to see if the expired item exists in the cache, and
> > if so, then cull it. This way, all writing behavior is isolated in
> > just one place.
> > 4. Once isInCache is implemented as the condition for all
> > doubleCheckedLock calls, change it to something safer. My attempt at
> > an improved implementation is below. It is thread safe at least, since
> > all references to the cache are locked, and it is faster because the
> > lock is only required to confirm existence if the object already
> > exists in cache.
>
> > <cffunction name="$doubleCheckedLock" returntype="any" access="public"
> > output="false">
> >        <cfargument name="name" type="string" required="true">
> >        <cfargument name="condition" type="string" required="true">
> >        <cfargument name="execute" type="string" required="true">
> >        <cfargument name="conditionArgs" type="struct" required="false"
> > default="#StructNew()#">
> >        <cfargument name="executeArgs" type="struct" required="false"
> > default="#StructNew()#">
> >        <cfargument name="timeout" type="numeric" required="false"
> > default="30">
> >        <cfset var loc = {}>
> >        <cfset loc.returnValue = false />
>
> >        <cflock name="#arguments.name#" timeout="#arguments.timeout#">
> >                <cfset loc.returnValue = $invoke(method=arguments.condition,
> > invokeArgs=arguments.conditionArgs)>
> >        </cflock>
>
> >        <cfif IsBoolean(loc.returnValue) AND NOT loc.returnValue>
> >                <cflock name="#arguments.name#"
> > timeout="#arguments.timeout#">
> >                        <cfset loc.returnValue =
> > $invoke(method=arguments.condition,
> > invokeArgs=arguments.conditionArgs)>
> >                        <cfif IsBoolean(loc.returnValue) AND NOT
> > loc.returnValue>
> >                                <cfset loc.returnValue =
> > $invoke(method=arguments.execute,
> > invokeArgs=arguments.executeArgs)>
> >                        </cfif>
> >                </cflock>
> >        </cfif>
> >        <cfreturn loc.returnValue>
> > </cffunction>
>
> > --
> > You received this message because you are subscribed to the Google Groups
> > "ColdFusion on Wheels" group.
> > To post to this group, send email to [EMAIL PROTECTED]
> > To unsubscribe from this group, send email to
> > [EMAIL PROTECTED]
> > For more options, visit this group at
> >http://groups.google.com/group/cfwheels?hl=en.

-- 
You received this message because you are subscribed to the Google Groups 
"ColdFusion on Wheels" group.
To post to this group, send email to [EMAIL PROTECTED]
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at 
http://groups.google.com/group/cfwheels?hl=en.