Menu
▾
▴
Re: [Webmacro-devel] Bug in caching of { } declarations (Was Re: Bug in repeating invocations of #eval, )
|
From: Tim P. <ti...@pa...> - 2012-04-15 17:12:12
|
Hi Alex,
On 11 April 2012 00:05, Alex Fiennes wrote:
> All,
>
> I've been doing some more thinking about the problem defined below and I've
> got some updates to the situation...
>
> On 24 November 2011 17:44, Alex Fiennes wrote:
>>
>> Dear all,
>
>
> <snip>
>
>>
>> First off, I've found a bug. Lets consider the following block of code:
>>
>> #templet $test {
>> Original: $value <br />
>> #set $value = $value + 1
>> Incremented: $value <br />
>> }
>> #eval $test using { "value": 1 }
>>
>> You run this in a servlet and it does what you would expect, ie:
>>
>> Original: 1
>> Incremented: 2
>>
>> However if you reload the page shortly afterwards then you get the
>> slightly surprising:
>>
>> Original: 2
>> Incremented: 3
>>
>> which is much less what you would expect.
>>
>> I've been doing some initial poking around and from what I can see the
>> problem is roughly along the lines of:
>>
>> the parser compiles the {"value": 1} into a Map and then caches this
>> the EvalDirective passes this very same Map into the Context that is used
>> to evaluate the $test templet
>> the $test templet makes changes to it's Context which then changes the
>> cached compiled version of {"value": 1}
>> the next time we invoke $test, we pass in what we assume is the compiled
>> version of {"value": 1}, but it actually now contains {"value": 2}
>>
>> If one leaves a pause between reloads and the caching engine discards the
>> cached parsed templates then the value returns back to 1.
>>
>> Now obviously, the EvalDirective should be taking a shallow copy of the
>> params before setting this as the Map that is backing the Context that is
>> used to evaluate $test.
>> I've mocked this up in user space so that I can invoke:
>>
>> #eval $test using $Vars.copyMap({ "value": 1 })
>>
>> and I get the expected behaviour, ie the value doesn't increment between
>> reloads.
>>
>> So I'm going to patch EvalDirective to sort this out which brings me onto
>> my second point.
>
>
> The problem actually isn't with EvalDirective because I think that it is
> doing the right thing. I would like to be able to do something like this if
> I wanted to:
>
> #set $params = { }
> #eval $initParams1 using $params
> #eval $initParams2 using $params
> #eval $doWork using $params
>
>
> and have $doWork get the populated version of $params that has been
> customised by passing through the preceeding initialisation routines.
> Making EvalDirective clone it's using params breaks this as the
> initialisation never makes it out of the preceeding invocations.
>
> however, if I have a template that reads as follows:
>
> #set $b = { }
> #if ($b.i == null) { #set $b.i = 0 } #else { #set $b.i = $b.i+1 }
> Value: $b.i
>
> then I would expect the value of $b.i to be 0 because we initialise an empty
> array and then put the value in.
> however this is not the case. The parsed version of $b which is initially
> an empty Map is cached along with the rest of the parsed template and
> therefore the value of $b.i persists and is incremented each evaluation of
> the template until the cache decides to expire it. This is definitely not
> what we want to happen and therefore he cloning of the Map should occur when
> we want to use the parsed version of "{ }" and therefore we will always have
> a "fresh copy" of $b that reflects its state when it was declared.
>
> I've had a little look into MapBuilder and the code in question looks like
> this:
>
> public Object build(BuildContext pc)
> throws BuildException
> {
> Map<Object, Object> ret = new HashMap<Object, Object>(size());
> boolean isMacro = false;
> for (Iterator<Map.Entry<Object, Object>> itr = entrySet().iterator();
> itr.hasNext();) {
> Map.Entry<Object, Object> entry = itr.next();
> Object key = entry.getKey();
> Object value = entry.getValue();
> if (key instanceof Builder)
> key = ((Builder) key).build(pc);
> if (value instanceof Builder)
> value = ((Builder) value).build(pc);
> if (!isMacro)
> isMacro = key instanceof Macro || value instanceof Macro;
> ret.put(key, value);
> }
> if (isMacro)
> return new MapMacro(ret);
> else
> return ret;
> }
>
> What is interesting about this is that we have two different return types -
> the Map itself, or the MapMacro wrapper which when evaluated creates a new
> Map and populates it with the evaluated contents of the original parsed and
> cached Map above. This would imply that if you Map definition is not
> constant, ie contains Macros then we shouldn't be seeing the bug, and if we
> extend our test case to:
>
> #set $var = "variable"
> #set $b = { "var": $var }
> #if ($b.i == null) { #set $b.i = 1 } #else { #set $b.i = $b.i+1 }
> Value: $b.i
>
>
> then we do in fact not see an incrementing $b.i because the parser is
> evaluating the value of $var each time that $b is utilised and therefore the
> problem goes away.
>
> I am therefore proposing that in cases where MapBuilder doesn't have any
> Macros in then it returns a simpler wrapper like so:
>
> class MapClone
> implements Macro
> {
> private final Map<Object, Object> __map;
>
> MapClone(Map<Object,Object> map) {
> __map = map;
> }
>
> @Override
> public Object evaluate(Context context)
> throws PropertyException
> {
> return new HashMap<Object,Object>(__map);
> }
>
> @Override
> public void write(FastWriter out,
> Context context)
> throws PropertyException, IOException
> {
> out.write(__map.toString());
> }
> }
>
>
> which just creates a clean copy of the cached Map every time it is asked for
> it, and we return it as follows:
>
> if (isMacro)
> return new MapMacro(ret);
> else
> return new MapClone(ret);
>
>
> then it is no longer possible to reproduce any of the unexpected behaviour
> and state is no longer preserved between re-invocations of the Template.
>
> If anyone can see any flaws in the above then let me know - otherwise I will
> commit the updates and move forwards...
>
I cannot see a flaw, but that is not saying much.
Are you going to commit a failing test first and then a fix for that?
I do not currently use any of the fancy features of WM,
only its most basic functionality, so am not a central user.
Which repository are you committing to? (or are you still using CVS?)
cheers
Tim
--
Tim Pizey
http://pizey.net/~timp
|