Issue when executing Mustache object several times with different scopes

[Mariano Campo]

Hi,

I think there might be an issue when executing the same Mustache object twice with different scopes. (I'm using version 0.7.2)

The first time I pass an empty map, so when it tries to replace a variable placeholder, it doesn't find it and leaves a blank as expected. But when I call it again with a map that contains the desired variable, it still doesn't find it.

This test should make it clear:

@Test

public void test() throws IOException  {

ByteArrayOutputStream out;

String template = "Value: {{value}}";

Mustache mustache = new DefaultMustacheFactory().compile(new StringReader(template), "test");

Map<String, String> emptyMap = new HashMap<String, String>();

Map<String, String> map = new HashMap<String, String>();

map.put("value", "something");

//execute mustache with an empty map, lookup will fail

out = new ByteArrayOutputStream();

mustache.execute(new OutputStreamWriter(out), emptyMap).flush();

assertEquals("Value: ", out.toString());

//execute mustache with a proper map, lookup shouldn't fail

out = new ByteArrayOutputStream();

mustache.execute(new OutputStreamWriter(out), map).flush();

assertEquals("Value: something", out.toString());

}

I think the problem might be in the variable lookup logic in DefaultCode, specifically in the use of the "notFound" variable (seems that once it changes to true, it never gets reseted).

I couldn't look more into this, so any help would be much appreciated.

Thanks,

Mariano.

[Pohl Longsine]

On Tuesday, April 17, 2012 7:43:51 AM UTC-5, Mariano Campo wrote:

I think the problem might be in the variable lookup logic in DefaultCode, specifically in the use of the "notFound" variable (seems that once it changes to true, it never gets reseted).

The notFound variable is local to that method. I'm only now becoming familiar with the code, but it looks that on the first execution, the DefaultCode instance asks its ObjectHandler to find a Wrapper for the tag name in the list of scopes, but gets null since ReflectionObjectHandler only returns a wrapper for a Map if a non-null value exists for the given key.  So DefaultCode creates a raw instance of GuardedWrapper, and puts it in its local list of cached wrappers.   Finally, the DefaultCode, considers the tag name to have not been found in any of the scopes, and so it correctly returns a null value for the tag, but before doing so it leaves its 'wrapper' instance variable null.

On the second execution, DefaultCode's getWrapper() method looks in the cache and finds the GuardedWrapper instance, and believes the tag to have been found (since it is agnostic to exactly what kind of wrapper was in the cache).  DefaultCode then tries to invoke call() on the wrapper, but this wrapper is unaware of the existence of a Map anywhere in the chain of scopes.  Instead, this wrapper's sole purpose is to assert that the number of scope objects (and their types) have not changed.   Since they haven't, it does not throw a GuardException, which would have caused the searching process to begin again.  So, instead, the GuardedWrapper returns null, without ever having consulted the Map.

On the second execution, the  

[Sam]

This is a bug in the guard mechanism in the current stable release.
This should be fixed in the 0.7.3-SNAPSHOT. If you find that it works
for you, I have some additional tests to catch this, then I will push
the 0.7.3 release.

Thanks,
Sam

[Pohl Longsine]

Nice work on the guard mechanism.  I like the use of Predicate.

[Sam]

Thanks! I wrote it and rewrote about 5 times before I was happy with
it. There is likely some room for optimization / simplification but I
needed to first get it to a place where I handle all the gnarly edge
cases.

Sam

[Matt Cheely]

I'm still seeing this issues in the latest 0.7.3-SNAPSHOT.

Also, after looking over the code (and I'm admittedly still familiarizing myself with it), I have to wonder if there are other issues with DefaultCode storing a wrapper object in a field on the class, namely that if I have two concurrent requests that are rendering the same Mustache with different types of backing data (Map vs SomeClass), what's to prevent ugly race conditions around that wrapper field?

Or am I just not using the tool in the intended way? Is the idea that Mustache objects (and the DefaultMustacheFactory by extension) aren't intended to be re-used or used concurrently and should be created from scratch with each new request.

[Mariano Campo]

I'm also still seeing this on the 0.7.3-SNAPSHOT from sonatype.

[Pohl Longsine]

I think Matt's concern is worthy of a moment of reflection.  I had a similar first impression with it, but I managed to convince myself that the design is safe.   Please poke holes in my thinking here.

The purpose of the Wrapper instance variable in DefaultCode is to provide a fast-path for obtaining a reference to a Method that can be invoked on the backing logic through reflection.  Prior to invocation, though, the calling thread needs to pass through a series of guards that examine the nature of the scopes array.

Pretty much all of a calling-thread's state is managed on its own call-stack.  The only stageful overlap appears to be the reference to the Wrapper instance held by each DefaultCode instance.  The reference to the Wrapper instance can never be in an intermediate state.  For every calling thread, this reference is either null, or it is a reference to some concrete instance of Wrapper.  Currently, every such instance of Wrapper is also a GuardedWrapper, since nothing else implements Wrapper.

The call() method of GuardedWrapper always does a guardedCall(Object[] scopes) internally.   This scopes parameter is going to be different for each calling thread, and this array of objects will either pass the guards and return a value, or it will fail a guard test, causing that thread's stack to unwind back to the catch block in DefaultCode's get_recurse() method, which resets the wrapper instance to null and recurses again to conduct a new search for a new Wrapper to cache.   This search, operating only on the calling thread's call stack, returns a Wrapper tailored for its own Object[] of scopes.

The potential for a race, then, happens when two threads compete to assign different & conflicting instances of Wrapper to the "wrapper" field at the same time.   Both of these assignments will happen, and so long as the guard mechanism is correct, it does not matter the order in which they happen.   But since guardCall() only applies a wrapper's predicates against the scopes on the present thread's call stack, there is no potential for confusing different backing data on different threads.

So I think you're use case is safe.

[Sam]

I think that it is thread safe as your analysis bears out, but I think
that there is enough potential for issues with this edge case that I
want to avoid it entirely. I've deployed another snapshot that takes a
far more aggressive deoptimization path - it basically gives up on
caching wrappers for any DefaultCode that ever fails guard check. For
the best performance, you should probably not change the types of
underlying objects between mustache instances.

Matt, I'm still concerned that there may be a case where the guards
fail. Can you send me a small example that doesn't work for you? Once
I am sure that it is entirely safe we can revisit reoptimizing the
failed guard case.

Thanks,
Sam

[Matt Cheely]

Sam,

The last time I checked, Mariano's test case above failed when added to the set of tests in the most recent code in master on github, and is representative of the behavior I'm seeing in one of our applications. I don't have a test case for (potential) concurrency issues as I didn't nail anything specific down, and that sort of thing is hard to reproduce. However, I was thinking about the concurrency stuff a bit today while running some errands, and I had an idea:

When executing a template, if instead of passing the original Writer  (which is obviously not shared beween thread) between Codes, you passed something like a RenderContext that wrapped up the writer and any mutable state (like cached guards) that was related to the current rendering pass. Then you could keep the caching, make all fields on Codes final, and I think be very confident about thread safety. Just an idea I wanted to get out while it was fresh. If you'd be interested in this sort of approach, I'd be happy to put in a bit of time on an implementation, seems like it might be fun to do.

I don't think there's any issue with changing the backing objects on mustache instances reducing performance due to cache misses, but I think it's worth pointing out that the use case could potentially be hard to avoid. Given that the defaultMustacheFactory caches instances, it's hard to get a new instance for something like a little utilitarian partial for a common UI element if you "decide" you have to change the type of backing object. Also, in some more complex application environments, you may not always have access to data in the same way. As an example, at Lulu we have to display summary information about books all the way from the creation process to when they're shipped to a customer. Even though the book might be displayed with the same UI and the same template in several places, the internal representation of the book changes many times along the way.

While I'm rambling, let me also say thanks for the excellent mustache implementation for java, and also for introducing the template inheritance mechanism and proposing it as part of the spec. I think it's a really nice mechanism that fits well with the simple & logic-less approach of mustache while adding a ton of value. I'm looking forward to updating our templates to use it.

Cheers,
-Matt

[Sam]

> The last time I checked, Mariano's test case above failed when added to the
> set of tests in the most recent code in master on github, and is

Unfortunately, I failed to push and only committed locally. The change
should now be in master. I would love it if you guys sent me a pull
request with your test.

> representative of the behavior I'm seeing in one of our applications. I
> don't have a test case for (potential) concurrency issues as I didn't nail
> anything specific down, and that sort of thing is hard to reproduce.
> However, I was thinking about the concurrency stuff a bit today while
> running some errands, and I had an idea:
>
> When executing a template, if instead of passing the original Writer
> (which is obviously not shared beween thread) between Codes, you passed
> something like a RenderContext that wrapped up the writer and any mutable
> state (like cached guards) that was related to the current rendering pass.
> Then you could keep the caching, make all fields on Codes final, and I
> think be very confident about thread safety. Just an idea I wanted to get
> out while it was fresh. If you'd be interested in this sort of approach,
> I'd be happy to put in a bit of time on an implementation, seems like it
> might be fun to do.

An interesting idea. I'm not sure how it would graph back onto the
Codes efficiently. It might make sense to just have a different
ObjectHandler that doesn't do the caching / guarding at all. It is
slower, but not complicated.

> I don't think there's any issue with changing the backing objects on
> mustache instances reducing performance due to cache misses, but I think
> it's worth pointing out that the use case could potentially be hard to
> avoid. Given that the defaultMustacheFactory caches instances, it's hard to
> get a new instance for something like a little utilitarian partial for a

Creating a default mustache factory is cheap, you can just use another
one and recompile. But, I see what you mean. I will reconsider whether
having that at the top level makes sense. It may make more sense to
cache that at a lower level.

> common UI element if you "decide" you have to change the type of backing
> object. Also, in some more complex application environments, you may not
> always have access to data in the same way. As an example, at Lulu we have
> to display summary information about books all the way from the creation
> process to when they're shipped to a customer. Even though the book might
> be displayed with the same UI and the same template in several places, the
> internal representation of the book changes many times along the way.

This is another case where it might make more sense to just use a
simpler, less aggressive object handler. I'll probably add one to make
this use case more natural and easier to reason about.

> While I'm rambling, let me also say thanks for the excellent mustache
> implementation for java, and also for introducing the template inheritance
> mechanism and proposing it as part of the spec. I think it's a really nice
> mechanism that fits well with the simple & logic-less approach of mustache
> while adding a ton of value. I'm looking forward to updating our templates
> to use it.

Thanks! I appreciate all the feedback and the use cases I haven't
thought about.

Sam

[Matt Cheely]

> I don't think there's any issue with changing the backing objects on
> mustache instances reducing performance due to cache misses, but I think
> it's worth pointing out that the use case could potentially be hard to
> avoid. Given that the defaultMustacheFactory caches instances, it's hard to
> get a new instance for something like a little utilitarian partial for a

Creating a default mustache factory is cheap, you can just use another
one and recompile. But, I see what you mean. I will reconsider whether
having that at the top level makes sense. It may make more sense to
cache that at a lower level.

 

I think I worded that poorly. I actually quite like the caching of the parsed templates - why read & parse the same template file multiple times? I just wanted to contrast/combine that behavior with the changing domain models.

[Sam]

On Apr 19, 10:23 am, Matt Cheely <matt.che...@gmail.com> wrote:
> I think I worded that poorly. I actually quite like the caching of the
> parsed templates - why read & parse the same template file multiple times?
> I just wanted to contrast/combine that behavior with the changing domain
> models.

The big issue is the cache doesn't really buy you much if you are only
doing one rendering pass with it. You are probably better off not
caching at all at that point since there is a fairly large cost for
the guards and doing the initial lookups. For most real world cases
the guards won't be breaking for every call site and you will probably
still be getting a reasonably big benefit caching. That said, if you
are using drastically different contexts at different places in your
program, I would just make a new factory for each use case. The
overhead is pretty low. Compiling Twitter's not trivial tweet.mustache
included in the tests can be done 4k/s in a single thread.

Sam

[Pohl Longsine]

Shouldn't it be possible to turn each Object[] scopes that you see into a ScopeKey object, such that the equals() method returns true (when comparing two ScopeKey instances) iff the scope array has the same length and the same types in the same order?

Given such a key, you could have a ConcurrentMap<ScopeKey,Wrapper> ivar in each DefaultCode instance.

A production system that oscillates between several kinds of scope arrays would eventually fill each cache so that every DefaultCode would always have an O(1) lookup for a wrapper.

I suppose that has a pathological case if someone makes a system that never repeats the same scope types, though.  Hmm.

[Sam Pullara]

This doesn't protect you against map keys changing. This was the
original version of guards before fixing them with predicates. I have
added hashCode/equals to all the Predicates and this could be used but
I'm not sure if it is worth calculating the guard to look it up vs
just making a new wrapper.

Sam

[Sam]

Ok, I made a breakthrough on how to optimize even this case -- the key
insight was to test previously successful wrappers before generating a
new one. You should get very little slow down when changing objects,
even if you switch between them very often. There is a new included
benchmark that cycles between 3 different objects on each call and
that only has about a 2.5x slow down from the best case scenario (vs
50x that I had with the earlier implementation that passed your
tests). I'd love a code review by whoever wants to look at it. If it
passes muster, I am going to ship 0.7.3 ASAP.

Thanks,
Sam

[Matt Cheely]

I took a look at the code and added some comments to some of the commits on github. I do still see a possible concurrency issue around cachedWrapper, but I'd love to have somebody prove me wrong on that.

[Pohl Longsine]

The latest looks great, especially after you added the get/createAndGet idiom in DefaultCode.   Bring 0.7.3 on.