[Webmacro-devel] Bug in caching of { } declarations (Was Re: Bug in repeating invocations of #eval, )

[Alex F. <al...@fi...>]

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 <al...@fi...> 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...

Alex

<snip>


-- 
Alex Fiennes
email: al...@fi...
mobile:  +44 (0)7813 832662
office: +44 (0)1875 823 310