[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: |
|
||||||||||||||||
| 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:
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:
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 ? |
| 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
After 4.5
|
| Comment by Philipp Bärfuss [ 31/Dec/15 ] |
|
What we could try is the following:
|
| 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). |