[MGNLGROOVY-68] Changes to a Groovy class in config aren't picked up Created: 26/Feb/13  Updated: 26/Jun/18  Resolved: 05/Jan/16

Status: Closed
Project: Magnolia Groovy Module
Component/s: None
Affects Version/s: 1.2.4, 2.2
Fix Version/s: 2.4.3

Type: Bug Priority: Major
Reporter: Teresa Miyar Assignee: Federico Grilli
Resolution: Fixed Votes: 0
Labels: pm
Remaining Estimate: 0d
Time Spent: 7.5h
Original Estimate: Not Specified

Issue Links:
dependency
depends upon MAGNOLIA-6483 Introduce ReloadableClassFactory inte... Closed
relation
is related to MAGNOLIA-6378 Allow yaml based configuration to mak... Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Release notes required:
Yes
Date of First Response:
Epic Link: Light Development 1.0
Sprint: Basel 27
Story Points: 5

 Description   

I have created a groovy script, to be used as a model class for a component. It runs, but if i decide to make any change, changes wont be considered unless I restart the server.



 Comments   
Comment by Magnolia International [ 26/Feb/13 ]

Had the same issue in the past, and thought I reported it in Jira. IIRC, this isn't specific to the Groovy module, but due to a cache in content2bean, the class definition is never reloaded. A workaround that doesn't require a restart is to modify your template definition to force content2bean to reload it and thus reload the class.

Comment by Teresa Miyar [ 27/Feb/13 ]

what is the best way to do that? Some push pop method?Components.newInstance?

Comment by Magnolia International [ 27/Feb/13 ]

What ? No, just edit your template definition via AdminCentral, it will get reloaded!?

Comment by Federico Grilli [ 13/Oct/14 ]

Discussed with Greg the solution of forcing the config reload upon saving the groovy source and he didn't like as it would create an unwanted dependency between two different workspaces/systems. Your changes not being picked up automatically is the expected behaviour, meaning that a change in the scripts workspace where your Groovy class source lives should not automatically trigger a change in a different workspace, i.e. the config workspace in this case. Therefore, a reload of the configuration node using your Groovy class needs to be triggered manually, e.g. by modifying your template definition to force node2bean to reload it and thus reload the class itself. This has been added to groovy module doc too.

Comment by Federico Grilli [ 15/Oct/14 ]

Discussed about it further with Phippu and his take is that from a user perspective it is best to go ahead with my proposed solution. Can be further discussed in arch meeting if necessary.

Comment by Magnolia International [ 16/Oct/14 ]

Federico, alternatives were proposed:

  • bake in some self-check in the class/classloader (sounds like we'd proxy a Class object, which is though)
  • objects such as TemplateDefinition are modified to hold something else than a Class, e.g a String or a ClassReference or something like that. Don't think that'd entail any config change.

Of course we need to do something about this from a user's perspective - I was merely suggesting that the current situation is explainable, not that it's practical.

Your solution introduce tight dependency between the groovy module and the config workspace. It won't work for objects not configured in the config workspace. (e.g objects configured elsewhere, components declared in a module descriptor as singletons, and components configured by file/code)

TBH I'm surprised there isn't a "built-in" solution in Groovy for this (read: there probably is); one should be able to keep a reference to a Groovy class object and expect it to be up to date at any point. So maybe there's a bug in our classloader, for example.

Comment by Magnolia International [ 16/Oct/14 ]

Re-reading my very first comment up here. Did anybody ever bothered verifying that was true ? If that's the case that'd mean there's a 3 approach to this, i.e fixing whatever that is in n2b ?

Comment by Federico Grilli [ 16/Oct/14 ]

@Greg I actually looked at N2B and afaics there's no class cache, so nothing that could be fixed there. As to the alternatives:

  • bake in some self-check: not sure I understand this. There already is a check in our groovy cl at MgnlGroovyClassLoader.isSourceNewer(URL, Class) which is called by loadClass(..). The problem with the configuration objects is that even if the groovy class is recompiled and reloaded by the cl, the config def object using it still keeps the old version
  • defs holding String or a ClassReference sounds like an API breaking change in some essential parts of Magnolia (e.g. RenderableDefinition) which perhaps is too much for the issue at hand.

Re "built-in" solution, I actually thought it would work as you say. I could investigate on a possible bug in our cl. What I already saw, as I said before, is that even forcing the groovy class to recompile and reload, the objects keeping a ref to it will keep on using the old version of it.

Finally, even if my solution creates this "dependency" and won't work in all use cases, I guess it still covers many (most?) of them.

Comment by Magnolia International [ 16/Oct/14 ]

If stuff gets broken AND this particular issue HAS TO be fixed NOW, then we can always take a 2-step approach (although "fix it later" never sounds promising).

Self-check: yeah i'm not sure myself either. IF imagine that it'd be OK to proxy the class objects (it seems fishy), it'd go something like this - with a typical interceptor proxy (the api call belows are imaginary, but I expect something similar in proxy/proxytoys etc)

ProxiedClass<T> {
 Class<T> theRealClass
 Object intercept(MethodCall methodAndArgs) {
   theRealClass = theRealClass.getClassLoader().checkForUpdate(theRealClass)
   invoke(theRealClass, methodAndArgs)
 }
}

If we need an "urgent" fix maybe instead of hiding that in the save action, we could have another big red fat button somewhere (ie another action in the groovy app?) that does what the current fix does ?

See also https://github.com/jmarranz/relproxy/

Comment by Federico Grilli [ 16/Oct/14 ]

After further discussion with Greg, given this is not an urgent issue (in spite its being definitely annoying), given that a workaround exists (manually trigger config reloading) and given that my solution is more of a hack than a proper fix, we agreed to keep this open and postpone it to a future release so to give more thought to it and find a more satisfying solution.

Comment by Philipp Bärfuss [ 10/Dec/15 ]

A new attempt might be made based on https://github.com/jmarranz/relproxy/

I stumbled over this by reading this http://www.innowhere.com/inexperiments/groovyex?itsnat_doc_name=groovyex

I am not suer but maybe this already fixed class reloading issue could impact our solution too: https://issues.apache.org/jira/browse/GROOVY-4975

Comment by Philipp Bärfuss [ 14/Dec/15 ]

What the relproxy provides is keeping an object reference which is reloaded if the underlaying class changes. Our use case is a bit simpler. We keep the reference to the class. and this class should be reloaded. So we would want to reload/proxy the class.

Comment by Federico Grilli [ 31/Dec/15 ]

I did some further investigation to see why this used to work before M4.5 and after that no longer works. Afaics, it stopped working when we introduced IoC

Before 4.5
--------------

  • info.magnolia.objectfactory.DefaultComponentProvider was the default ComponentProvider
  • object instances were created at info.magnolia.objectfactory.DefaultComponentProvider.newInstance(Class<T>, Object...) with the following call Classes.getClassFactory().newInstance(clazz, parameters);
  • this would eventually delegate to info.magnolia.module.groovy.support.classes.GroovyClassFactory.newInstance(..) which would return the recompiled class if modified

After 4.5
------------

  • info.magnolia.objectfactory.guice.GuiceComponentProvider is the new default ComponentProvider
    • it has no notion of ClassFactory, therefore hot recompiling of a groovy model class no longer works.
  • But why it works when I force reloading e.g. config containing a Groovy modelClass?
    • because there N2B is triggered, namely info.magnolia.jcr.node2bean.impl.Node2BeanTransformerImpl.convertPropertyValue(..) where the following call happens
      Class<?> clazz = Classes.getClassFactory().forName(..); which eventually delegates to info.magnolia.module.groovy.support.classes.GroovyClassFactory.forName(..) which would recompile the groovy source if modified. The recompiled class is finally assigned to the modelClass property so that when GuiceComponentProvider creates a new instance of it, it uses the latest recompiled class.
Comment by Philipp Bärfuss [ 31/Dec/15 ]

What we could try is the following:

  1. the GroovyClassFactory (despite the name a Magnolia class) returns a delegate proxy delegating all calls to the original class
  2. on the calls to newInstance() the proxy use same mechanism to check for re-compilation as the GroovyClassFactory uses
  3. node2bean (and also map2bean once MAGNOLIA-6378 is fixed) assigned the proxy. well without knowing about that fact
Comment by Philipp Bärfuss [ 04/Jan/16 ]

What I like about your approach of introducing ClassFactory.recompileIfNeeded is the fact that you make it an explicit feature and that the overhead should indeed be supper minimal. Well it just makes a call to a function returning the argument.

There is one drawback as far I can see. The definition bean containing the original class won't be updated. This means unless node2bean is re-triggered by other changes the recompile would happen each time because the passed class would always be considered 'older'.

Comment by Federico Grilli [ 05/Jan/16 ]

That shouldn't happen because what GroovyClassFactory.recompileIfNewer(..) eventually does is comparing the timestamp of the Groovy class with the last modified date of the corresponding source in repository. Compilation happens only if source is more recent than the class. True, however, that this check is done every time Magnolia instantiates a Groovy class and comes with a little overhead as you can see in the above table (in that example ~ 5ms compared to ~496ms of recompilation and ~0.3 ms when not doing the check).

Generated at Mon Feb 12 05:55:15 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.