Caching in MemberValueResolver does not take into account that classes may be loaded by different classloaders

[erdi]

I'm specifically referring to this cache.

Basically, if a method is cached in this cache and then the same method on the same class but loaded by a different classloader is being resolved then there is an IllegalArgumentException being thrown here.

This is an issue when using Handlebars with Ratpack in hot reload mode. When using that mode Ratpack reloads application when a change in source files is detected but reuses classes of itself and it's dependencies including Handlebars.java. When an application class is being used as the rendering context the issue occurs.

There are two issues here. First could be solved by changing the key of that cache to be based on a value type which would consist of a class instance and the name instead of a String as it currently is. This would leave use with possible memory leaks in hot reload mode. To solve that, there should be also a public method exposed which would allow to clear a cache of a MemberValueResolver instance.

I'm happy to provide a self contained sample not involving ratpack that exposes the issue and/or a PR if you are interested. I would like to know if you're interested in it before putting any effort in.

[jknack]

I see the problem...what if you provide your own MethodResolver without cache while running in Ratpack dev mode?

Don't love/want to add a .clear method, it seems odd.

btw, I invited you to try my micro web framework: http://jooby.org (a Ratpack alternative)

[erdi]

Thanks but I'll keep on using Ratpack.

I have a workaround for the issue but it means that everybody will need to apply something to get it working. We could hardcode the list of resolvers when creating a context like this:

Context.newBuilder(template.model).resolver(MapValueResolver.INSTANCE, beanValueResolver).build()

but it would mean that if you change the default list in ValueResolver.VALUE_RESOLVERS in a future release we would not pick it up.

If you're not willing to add a method to clear the cache (may I ask why such a method feels odd to you?) will you at least fix the cache to work properly with classes loaded by different classloaders?

[jknack]

this is very specific issue, not a common issue. cache is an internal thing... doesn't feel right to expose it.

what if you change the methodvalueresolver in way that let you turn off cache at creation time? I can accept a pull request for that.

[erdi]

If the cache is an internal thing that the user should not be aware of then I would suggest you fix it to handle classes loaded by more than one classloader. It's not hard - simply create a value type consisting of the class and the member name, implement equals() and hashCode()using Objects.equals() and Objects.hash() and use that as the key. I'm happy to provide a PR with that. And I would argue that using more than one classloader in a app is not an uncommon thing.

Letting me turn off the caching buys me nothing. I'm already working around the issue by wrapping my models in Context manually with the code provided in my comment above and managing value resolvers myself. The thing is that I don't want to do it because this should be internal and I shouldn't have to know what the default value resolvers are (as defined in ValueResolver.VALUE_RESOLVERS) - that's an implementation detail. I should be able to pass a model to Template.apply() and it should just work - now I had to replicate what Context.wrap() does which is private and hence internal. I want to be able to update Handlebars in the future without having to revisit this piece of code to check that you haven't added any default value resolvers.

[jknack]

about to become an endless discussion and I don't want that :)

Send your pull request with your cache changes. Add the .clean method with a good javadoc about what it does and/or an example of when is required.

Cool?

[erdi]

Cool. I will provide PR (hopefully over the weekend) with tests and fixes for the cache improvement. I think that when we have in then there will be no need for adding the clear() method.

[jknack]

nice!

[erdi]

A fix available in PR #406.