Groovy
  1. Groovy
  2. GROOVY-3981

Spring 2.5.X refresh interval incompatibility

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Duplicate
    • Affects Version/s: 1.7.0
    • Fix Version/s: None
    • Component/s: class generator
    • Labels:
      None
    • Environment:
      Groovy 1.7.0, JDK 1.5, Spring 2.5.6, Win

      Description

      Version 1.7.0 corrupts Spring 2.5.X refresh interval behaviour. I include set of test proving that. If you run them with Groovy 1.6.5 (see comments containing Groovy 1.6.5, that are necessary to process before running with 1.6.5 - 1.7.0 introduces some changes breaking backward compatibility) all tests pass. With 1.7.0 two fails - one is described in another issue GROOVY-3980.

      This issue should concern with test: com.fg.scripting.groovy.SpringGroovyReloadSupportTest#testReloadLoadGroovyClass

      Note that before running tests, path in com.fg.scripting.groovy.AbstractGroovyTest#setUp must be modified to match your system.

      Spring configuration is so simple, that (I hope) cannot be wrong:

      <?xml version="1.0" encoding="utf-8"?>
      <beans xmlns="http://www.springframework.org/schema/beans"
      xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:lang="http://www.springframework.org/schema/lang"
      xsi:schemaLocation="http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans-2.0.xsd http://www.springframework.org/schema/lang http://www.springframework.org/schema/lang/spring-lang.xsd">

      <bean id="propertyPlaceholderConfigurer" class="org.springframework.beans.factory.config.PropertyPlaceholderConfigurer"/>

      <lang:groovy id="mainGroovyClass"
      refresh-check-delay="0"
      script-source="file:$

      {project.build.directory}/target/test-classes/com/fg/mock/MainGroovyClass.groovy"/>

      <lang:groovy id="referencingGroovyClass"
      refresh-check-delay="0"
      script-source="file:${project.build.directory}

      /target/test-classes/com/fg/mock/ReferencingGroovyClass.groovy">
      <lang:property name="mainGroovyClass" ref="mainGroovyClass"/>
      </lang:groovy>

      </beans>

      Bean refreshes to conform to actual source code occurs no more with Groovy 1.7.0.
      Regretfully I wasn't able to find out where problem lies, so no patch is included.

      1. 3633_v17x_Patch.txt
        2 kB
        Roshan Dawrani
      2. lib_groovy.1.7.0.zip
        28 kB
        Jan Novotný
      3. lib_groovy.1.7.0-spring-3.0.0.zip
        28 kB
        Jan Novotný
      4. lib_groovy.1.7.0-spring-3.0.0-second-wait.zip
        27 kB
        Jan Novotný
      5. v2_3633_v17x_Patch.txt
        0.4 kB
        Roshan Dawrani

        Issue Links

          Activity

          Hide
          Guillaume Delcroix added a comment -

          Have you tried with Spring 3?
          Spring 3 was upgraded to Groovy 1.7, AFAIK, but not Spring 2.5, still based on Groovy 1.6.

          Show
          Guillaume Delcroix added a comment - Have you tried with Spring 3? Spring 3 was upgraded to Groovy 1.7, AFAIK, but not Spring 2.5, still based on Groovy 1.6.
          Hide
          Guillaume Delcroix added a comment -

          More on Spring 3...
          We have to be careful: if ever we fix the issue you mention, we shouldn't break Spring 3's Groovy support!!!

          Show
          Guillaume Delcroix added a comment - More on Spring 3... We have to be careful: if ever we fix the issue you mention, we shouldn't break Spring 3's Groovy support!!!
          Hide
          Jan Novotný added a comment -

          I will try that on Spring 3.0 and will let you know. But for several companies (such as ours) upgrading to Spring 3.0 is not an option because of JDK15 requirements or necessity to update own libraries to adapt Spring 3.0 changes, which takes some time (depending on how tight integration was).

          I still hope, that Groovy will still support JDK14 though artifact groovy-all-jdk14-1.6.5 was damaged and not fully usable in older environments. Or will it not? I noticed that 1.7.0 port for JDK14 is still missing on Maven repositories ...

          Show
          Jan Novotný added a comment - I will try that on Spring 3.0 and will let you know. But for several companies (such as ours) upgrading to Spring 3.0 is not an option because of JDK15 requirements or necessity to update own libraries to adapt Spring 3.0 changes, which takes some time (depending on how tight integration was). I still hope, that Groovy will still support JDK14 though artifact groovy-all-jdk14-1.6.5 was damaged and not fully usable in older environments. Or will it not? I noticed that 1.7.0 port for JDK14 is still missing on Maven repositories ...
          Hide
          Jan Novotný added a comment -

          Refresh fails even with Spring 3.0.0.RELEASE from repo1/maven2 repository. I include zip with tests using new release of the Spring Framework and the test still fails. Is it even possible with the test coverage that is standard for Spring products? Isn't it a problem of my local environment? I hope not.

          Show
          Jan Novotný added a comment - Refresh fails even with Spring 3.0.0.RELEASE from repo1/maven2 repository. I include zip with tests using new release of the Spring Framework and the test still fails. Is it even possible with the test coverage that is standard for Spring products? Isn't it a problem of my local environment? I hope not.
          Hide
          Jan Novotný added a comment -

          Reuploaded ZIPs - there were dependencies on our company parent pom. These poms should be universal (I hope).

          Show
          Jan Novotný added a comment - Reuploaded ZIPs - there were dependencies on our company parent pom. These poms should be universal (I hope).
          Hide
          Jan Novotný added a comment -

          Corrected pom.xml files again so you can run it without problems - removed problematic log4j transitive dependencies and configured compiler plugin.

          Show
          Jan Novotný added a comment - Corrected pom.xml files again so you can run it without problems - removed problematic log4j transitive dependencies and configured compiler plugin.
          Hide
          Roshan Dawrani added a comment -

          Anyone knows which spring/groovy components are involved in spring's <lang:groovy> support? If I have to look a bit into this issue where groovy bean refresh is having some issue, which groovy components should I keep an eye on?

          GROOVY-3980 was clearly entering into groovy through GroovyScriptEngine, but in this one entry point from spring into groovy is not clear to me.

          Can anyone help with that please?

          Show
          Roshan Dawrani added a comment - Anyone knows which spring/groovy components are involved in spring's <lang:groovy> support? If I have to look a bit into this issue where groovy bean refresh is having some issue, which groovy components should I keep an eye on? GROOVY-3980 was clearly entering into groovy through GroovyScriptEngine, but in this one entry point from spring into groovy is not clear to me. Can anyone help with that please?
          Hide
          Roshan Dawrani added a comment -

          This is duplicate of GROOVY-3973.

          Even in this JIRA, the call is coming from spring's GroovyScriptFactory into GroovyClassLoader.parseClass(String, String) - exact same issue as reported in GROOVY-3973.

          Show
          Roshan Dawrani added a comment - This is duplicate of GROOVY-3973 . Even in this JIRA, the call is coming from spring's GroovyScriptFactory into GroovyClassLoader.parseClass(String, String) - exact same issue as reported in GROOVY-3973 .
          Hide
          Jan Novotný added a comment -

          As far as I know there is class org.springframework.scripting.groovy.GroovyScriptFactory that takes care of creating Groovy classes. It uses GroovyClassLoader to do that - so no GroovyScriptengine is used there.

          In general Spring should generate for each Groovy bean dynamic proxy - I think that it is always JdkDynamicAopProxy in case of Groovy bean. This proxy then delegates method call on its TargetSource, that is represented by RefreshableScriptTargetSource. This implementation asks GroovyScriptFactory to check whether Groovy instance needs refresh, but only in specified intervals that are defined on the <lang:groovy/> tag of the particular bean in Spring context (in our case 0).

          GroovyScriptFactory consults ScriptSource.isModified() and when this object returns true, new Groovy instance should be created through via freshTarget() method.

          What confuses me mosts that it seems that problem is not caused by the Groovy itself. Even if file gets changed in the test's modifyGroovyFile method, the:

          public final synchronized Object getTarget() {
          if ((refreshCheckDelayElapsed() && requiresRefresh()) || this.targetObject == null)

          { refresh(); }

          return this.targetObject;
          }

          in the AbstractRefreshableTarget source returns:

          true for refreshCheckDelayElapsed - ok, because there is zero set on the bean, so check should be triggered every time
          false for requiresRefresh - and this is the problem, bud I cannot undestand why it still returns fals even if file timestamp changed

          Moreover all those classes are Spring classes - so why this work with old Groovy and not the new version, when it seems that Groovy doesn't get involved in all of this (whole obsolete checking is happening in Spring classes)?

          I am still looking at that ... maybe I found something, but I doubt.

          Show
          Jan Novotný added a comment - As far as I know there is class org.springframework.scripting.groovy.GroovyScriptFactory that takes care of creating Groovy classes. It uses GroovyClassLoader to do that - so no GroovyScriptengine is used there. In general Spring should generate for each Groovy bean dynamic proxy - I think that it is always JdkDynamicAopProxy in case of Groovy bean. This proxy then delegates method call on its TargetSource, that is represented by RefreshableScriptTargetSource. This implementation asks GroovyScriptFactory to check whether Groovy instance needs refresh, but only in specified intervals that are defined on the <lang:groovy/> tag of the particular bean in Spring context (in our case 0). GroovyScriptFactory consults ScriptSource.isModified() and when this object returns true, new Groovy instance should be created through via freshTarget() method. What confuses me mosts that it seems that problem is not caused by the Groovy itself. Even if file gets changed in the test's modifyGroovyFile method, the: public final synchronized Object getTarget() { if ((refreshCheckDelayElapsed() && requiresRefresh()) || this.targetObject == null) { refresh(); } return this.targetObject; } in the AbstractRefreshableTarget source returns: true for refreshCheckDelayElapsed - ok, because there is zero set on the bean, so check should be triggered every time false for requiresRefresh - and this is the problem, bud I cannot undestand why it still returns fals even if file timestamp changed Moreover all those classes are Spring classes - so why this work with old Groovy and not the new version, when it seems that Groovy doesn't get involved in all of this (whole obsolete checking is happening in Spring classes)? I am still looking at that ... maybe I found something, but I doubt.
          Hide
          Roshan Dawrani added a comment -

          Yes, no GroovyScriptEngine in this case. The calls come from GroovyScriptFactory into GroovyClassLoader.

          I was about to start tracing Spring code from ClassPathXmlApplicationContext -> GroovyScriptFactory -> GroovyClassLoader, and I am sure the details you have provided will help me understand that much better. Thanks a lot.

          I am pretty sure that it is duplicate of GROOVY-3973 and the fix for GROOVY-3633 has affected your case too. I am still looking into it. May be I will try reverting the fix of GROOVY-3633 and see if your test start going through.

          I will write back as soon as I have more details to add.

          Show
          Roshan Dawrani added a comment - Yes, no GroovyScriptEngine in this case. The calls come from GroovyScriptFactory into GroovyClassLoader. I was about to start tracing Spring code from ClassPathXmlApplicationContext -> GroovyScriptFactory -> GroovyClassLoader, and I am sure the details you have provided will help me understand that much better. Thanks a lot. I am pretty sure that it is duplicate of GROOVY-3973 and the fix for GROOVY-3633 has affected your case too. I am still looking into it. May be I will try reverting the fix of GROOVY-3633 and see if your test start going through. I will write back as soon as I have more details to add.
          Hide
          Roshan Dawrani added a comment -

          Yes, if I just revert the fix of GROOVY-3633, your test starts going through. So, the origin of the issue and that it is duplicate of GROOVY-3973 is confirmed now.

          I think fix for GROOVY-3633 has to be changed.

          Show
          Roshan Dawrani added a comment - Yes, if I just revert the fix of GROOVY-3633 , your test starts going through. So, the origin of the issue and that it is duplicate of GROOVY-3973 is confirmed now. I think fix for GROOVY-3633 has to be changed.
          Hide
          Roshan Dawrani added a comment - - edited

          Hi Jochen,
          I wanted to propose changing the current fix of GROOVY-3633 and am attaching an alternative patch.

          The issue in GROOVY-3633(2nd part) was that if a cacheable GroovyCodeSource gets compiled and later recompile() is called on it, it removes the entry of that GroovyCodeSource from sourceCache and the recompiled class never gets cached again as the new GroovyCodeSource always has cacheable flag set as false.

          In the proposed patch, when we cache the class for a cacheable GroovyCodeSource, we make note of that in a separate cache and use that later in recompile() when it makes the new GroovyCodeSource - instead of having "false", it will again have "true", as needed by GROOVY-3633 - but done in a different way - and not hardcoding it "true" always.

          With this change, this regression on the spring bean refresh goes away.

          What do you think?

          Show
          Roshan Dawrani added a comment - - edited Hi Jochen, I wanted to propose changing the current fix of GROOVY-3633 and am attaching an alternative patch. The issue in GROOVY-3633 (2nd part) was that if a cacheable GroovyCodeSource gets compiled and later recompile() is called on it, it removes the entry of that GroovyCodeSource from sourceCache and the recompiled class never gets cached again as the new GroovyCodeSource always has cacheable flag set as false. In the proposed patch, when we cache the class for a cacheable GroovyCodeSource, we make note of that in a separate cache and use that later in recompile() when it makes the new GroovyCodeSource - instead of having "false", it will again have "true", as needed by GROOVY-3633 - but done in a different way - and not hardcoding it "true" always. With this change, this regression on the spring bean refresh goes away. What do you think?
          Hide
          Jochen Theodorou added a comment -

          so basically the problem is that a String does not get compiled again, because the source name is reused and the old one was cached. While I agree that making String sources cacheable by default might have been the wrong decision I don't like that map. You need it because after recompilation the information of being cacheable would be lost else? what happens if you disable caching for the string based version by default?

          Show
          Jochen Theodorou added a comment - so basically the problem is that a String does not get compiled again, because the source name is reused and the old one was cached. While I agree that making String sources cacheable by default might have been the wrong decision I don't like that map. You need it because after recompilation the information of being cacheable would be lost else? what happens if you disable caching for the string based version by default?
          Hide
          Roshan Dawrani added a comment -

          Here is another patch that just makes caching disabled in parseClass(String, String).

          This change is also helping with Spring reloading as Spring's GroovyScriptFactory only uses GCL#parseClass(String, String).

          So, as far as regression with Spring integration is concerned, even this patch v2 helps as much as the previous one.

          Show
          Roshan Dawrani added a comment - Here is another patch that just makes caching disabled in parseClass(String, String). This change is also helping with Spring reloading as Spring's GroovyScriptFactory only uses GCL#parseClass(String, String). So, as far as regression with Spring integration is concerned, even this patch v2 helps as much as the previous one.
          Hide
          Jochen Theodorou added a comment -

          I would like to hear Guillaume on this. I think that I can approve of this patch now. Looks much better than the map version.

          Show
          Jochen Theodorou added a comment - I would like to hear Guillaume on this. I think that I can approve of this patch now. Looks much better than the map version.
          Hide
          Roshan Dawrani added a comment -

          Ok, let's wait for Guillaume's inputs on it. I am not applying the patch right now.

          Show
          Roshan Dawrani added a comment - Ok, let's wait for Guillaume's inputs on it. I am not applying the patch right now.
          Hide
          Roshan Dawrani added a comment -

          Hi Jan, regarding testing reloading of groovy beans through spring, I have a suggestion. I think your reload tests need to change a bit to see consistent results.

          Spring calls GroovyClassLoader to re-parse the class only when RefreshableScriptTargetSource#requiresRefresh() returns true. This is based on ResourceScriptSource#isModified(), which compares the previous lastModified timestamp of the groovy script file with the current timestamp.

          Now in your tests, you first call original MainGroovyClass#sayHello(), then you modify the file and then call it again to check if Groovy is re-parsing the script. But you have many tests that modify the same file to test the reload the script file and intermittently it is possible that the timestamps it sees before and after modification are in the same millisecond value - and, intermittently, in those cases, Spring will not invoke GCL#parseClass() and reloading will not happen and the test will fail.

          So, the suggestion is that before you modify the script file during a test, you can introduce sleep of a few milli-seconds - to force the pre and post modification timestamps to have different values.

          What do you think?

          Show
          Roshan Dawrani added a comment - Hi Jan, regarding testing reloading of groovy beans through spring, I have a suggestion. I think your reload tests need to change a bit to see consistent results. Spring calls GroovyClassLoader to re-parse the class only when RefreshableScriptTargetSource#requiresRefresh() returns true. This is based on ResourceScriptSource#isModified(), which compares the previous lastModified timestamp of the groovy script file with the current timestamp. Now in your tests, you first call original MainGroovyClass#sayHello(), then you modify the file and then call it again to check if Groovy is re-parsing the script. But you have many tests that modify the same file to test the reload the script file and intermittently it is possible that the timestamps it sees before and after modification are in the same millisecond value - and, intermittently, in those cases, Spring will not invoke GCL#parseClass() and reloading will not happen and the test will fail. So, the suggestion is that before you modify the script file during a test, you can introduce sleep of a few milli-seconds - to force the pre and post modification timestamps to have different values. What do you think?
          Hide
          Roshan Dawrani added a comment -

          The previous comment is all about what happens within Spring's reloading/refresh logic. If following happen too fast (in the same milli-second), then Spring itself doesn't call groovy parseClass and then groovy cannot do anything :

          1) getBean(aGroovyScript) call 1
          2) modify the groovy script configured in spring context
          1) getBean(aGroovyScript) call 2

          Show
          Roshan Dawrani added a comment - The previous comment is all about what happens within Spring's reloading/refresh logic. If following happen too fast (in the same milli-second), then Spring itself doesn't call groovy parseClass and then groovy cannot do anything : 1) getBean(aGroovyScript) call 1 2) modify the groovy script configured in spring context 1) getBean(aGroovyScript) call 2
          Hide
          Jan Novotný added a comment -

          Yes Roshan, that's a good idea. I will alter the tests with adding a small waiting section before modifyFile. I tried to play with that, but because it didn't help with the issue I removed that again. But this may be the purpose why some of the tests fail on our company integration server, that is probably much more fast than my local machine where all test pass with 1.6.5 Groovy.

          Show
          Jan Novotný added a comment - Yes Roshan, that's a good idea. I will alter the tests with adding a small waiting section before modifyFile. I tried to play with that, but because it didn't help with the issue I removed that again. But this may be the purpose why some of the tests fail on our company integration server, that is probably much more fast than my local machine where all test pass with 1.6.5 Groovy.
          Hide
          Roshan Dawrani added a comment -

          After applying the fix being discussed here, when I ran the failing test (of this JIRA) individually, it runs successfully every single time, but when I run all 16 tests from your code together - intermittently one reload test will fail or another (my desktop is fast too ). Some digging up showed that both pre-post file modification, timestamps were coming as same. A delay in between should help there.

          Show
          Roshan Dawrani added a comment - After applying the fix being discussed here, when I ran the failing test (of this JIRA) individually, it runs successfully every single time, but when I run all 16 tests from your code together - intermittently one reload test will fail or another (my desktop is fast too ). Some digging up showed that both pre-post file modification, timestamps were coming as same. A delay in between should help there.
          Hide
          Jan Novotný added a comment -

          I've modified the tests so that there is a second wait time. Now even on the fastest machines the timestamp must change . Just in case you would like to test it again, Roshan.

          How long do you guess will take to get those bugfixes into the GA release? Which one will it be - 1.7.1?

          Show
          Jan Novotný added a comment - I've modified the tests so that there is a second wait time. Now even on the fastest machines the timestamp must change . Just in case you would like to test it again, Roshan. How long do you guess will take to get those bugfixes into the GA release? Which one will it be - 1.7.1?
          Hide
          Roshan Dawrani added a comment -

          Yes, Jan, the bug fixes will come out only in 1.7.1, which, I have heard from Guillaume, will happen within a month or so.

          In the meanwhile, you have the option to use 1.7.1 snapshot versions.

          Please note that fix for this JIRA (being done by changing GROOVY-3633 fix) is not in the codebase yet, as Jochen wanted Guillaume also to review it.

          Fix for GROOVY-3980 is already checked-in and is available in the snapshot.

          Show
          Roshan Dawrani added a comment - Yes, Jan, the bug fixes will come out only in 1.7.1, which, I have heard from Guillaume, will happen within a month or so. In the meanwhile, you have the option to use 1.7.1 snapshot versions. Please note that fix for this JIRA (being done by changing GROOVY-3633 fix) is not in the codebase yet, as Jochen wanted Guillaume also to review it. Fix for GROOVY-3980 is already checked-in and is available in the snapshot.
          Hide
          Roshan Dawrani added a comment -

          @Jan - this fix is in the 1.7.x codebase now (against GROOVY-3973) and should get released in 1.7.1.

          I tested with your new tests (with the delay) and they seem to be consistently passing now.

          @Jochen - I checked with Guillaume before checking-in the fix.

          Show
          Roshan Dawrani added a comment - @Jan - this fix is in the 1.7.x codebase now (against GROOVY-3973 ) and should get released in 1.7.1. I tested with your new tests (with the delay) and they seem to be consistently passing now. @Jochen - I checked with Guillaume before checking-in the fix.
          Hide
          Jan Novotný added a comment - - edited

          Thank you Roshan - even my internal integration server test fail issue was corrected by the advice you gave me. Looking forward to 1.7.1

          Show
          Jan Novotný added a comment - - edited Thank you Roshan - even my internal integration server test fail issue was corrected by the advice you gave me. Looking forward to 1.7.1

            People

            • Assignee:
              Roshan Dawrani
              Reporter:
              Jan Novotný
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development