Uploaded image for project: 'Groovy'
  1. Groovy
  2. GROOVY-3980

GroovyScriptingEngine doesn't recognize changes in the source of checked class only when dependencies got changed

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.7.0
    • Fix Version/s: 1.7.1, 1.8-beta-1
    • Component/s: class generator
    • Labels:
      None
    • Environment:
      Groovy 1.7.0-all, JDK 1.5, Win
    • Flags:
      Patch

      Description

      Expected behaviour from my point of view it that GroovyScriptEngine should refresh class everytime its source code or source code of any of classes this class references changes. Such behaviour was implemented in 1.6.5. When upgrading to 1.7.0 our test began to fail - class is refreshed only in case of referenced classes source code gets changed, but when I directly change the source of the class I am directly loading, I receive old version - no refresh occurs.

      I can prove this by the set of tests that pass on 1.6.5 but fails on 1.7.0. Precisely the proving test is:
      com.fg.scripting.groovy.GroovyScriptingEngineReloadSupportTest#testReloadGroovyClass

      Before running tests, please inspect com.fg.scripting.groovy.AbstractGroovyTest#setUp and modify the path.

      1. v2_3980_v17x_Patch.txt
        2 kB
        Roshan Dawrani
      2. patch-GroovyScriptEngine.patch
        0.6 kB
        Jan Novotný
      3. lib_groovy-1.7.0.zip
        48 kB
        Jan Novotný
      4. 3980_v17x_Patch.txt
        3 kB
        Roshan Dawrani

        Activity

        Hide
        roshandawrani Roshan Dawrani added a comment -

        Thanks, Paul

        Show
        roshandawrani Roshan Dawrani added a comment - Thanks, Paul
        Hide
        paulk Paul King added a comment -

        Nice work!

        Show
        paulk Paul King added a comment - Nice work!
        Hide
        roshandawrani Roshan Dawrani added a comment -

        Fixed.

        You are welcome, Jan.

        Show
        roshandawrani Roshan Dawrani added a comment - Fixed. You are welcome, Jan.
        Hide
        roshandawrani Roshan Dawrani added a comment -

        Patch v2, right? I will go ahead and apply then?

        Show
        roshandawrani Roshan Dawrani added a comment - Patch v2, right? I will go ahead and apply then?
        Hide
        blackdrag Jochen Theodorou added a comment -

        seems I had the right intention and then forgot about it later in the implementation. Thanks Roshan, that patch looks good to me

        Show
        blackdrag Jochen Theodorou added a comment - seems I had the right intention and then forgot about it later in the implementation. Thanks Roshan, that patch looks good to me
        Hide
        roshandawrani Roshan Dawrani added a comment -

        Jochen, here is another patch that adds the script itself also to its dependencies.

        The counts in the DepenendencyTest had to be incremented by 1 due to this extra dependency that is now there.

        Show
        roshandawrani Roshan Dawrani added a comment - Jochen, here is another patch that adds the script itself also to its dependencies. The counts in the DepenendencyTest had to be incremented by 1 due to this extra dependency that is now there.
        Hide
        novoj Jan Novotný added a comment -

        Thank you for solving the issue Roshan - I clearly missed the point. It was a way harder that I initially thought.

        Show
        novoj Jan Novotný added a comment - Thank you for solving the issue Roshan - I clearly missed the point. It was a way harder that I initially thought.
        Hide
        roshandawrani Roshan Dawrani added a comment -

        No, the script did not have itself as a dependency.

        It's not that something is always getting recompiled - then the issue would not have been there, as you say.

        The main script was always getting recompiled if I tried the patch originally provided. On original 1.7.0, you are right that main entry was not getting recompiled and that was the problem in the issue reported here.

        Show
        roshandawrani Roshan Dawrani added a comment - No, the script did not have itself as a dependency. It's not that something is always getting recompiled - then the issue would not have been there, as you say. The main script was always getting recompiled if I tried the patch originally provided. On original 1.7.0, you are right that main entry was not getting recompiled and that was the problem in the issue reported here.
        Hide
        blackdrag Jochen Theodorou added a comment -

        I thought the problem is that something is not recompiled... When it is always recompiled, then this makes no sense at all for me.

        Checking the entry itself was not needed before, becaue the script has itself as dependency - at least I thought so. I guess that this might not always be the case

        Show
        blackdrag Jochen Theodorou added a comment - I thought the problem is that something is not recompiled... When it is always recompiled, then this makes no sense at all for me. Checking the entry itself was not needed before, becaue the script has itself as dependency - at least I thought so. I guess that this might not always be the case
        Hide
        roshandawrani Roshan Dawrani added a comment -

        Attaching the patch I propose.

        It's getting rid of the failure "testReloadGroovyClass(com.fg.scripting.groovy.GroovyScriptingEngineReloadSupportTest)"

        Show
        roshandawrani Roshan Dawrani added a comment - Attaching the patch I propose. It's getting rid of the failure "testReloadGroovyClass(com.fg.scripting.groovy.GroovyScriptingEngineReloadSupportTest)"
        Hide
        roshandawrani Roshan Dawrani added a comment -

        I have looked into the issue a bit and also the patch, and I think the patch is wrong. It is wrong because it is comparing a ScriptCacheEntry's modified time with current system time, which will always be true and hence the script will always get re-compiled, even when there are no changes to the script.

        So, say, a script was first loaded by GSE at time T1 and next calls are made at T1 + t1(System.currentTimeMillis), T1 + t2, T1 + t3, and there is no change in the script file at all, but because T1 < T1 + t1(or t2 or t3) will always give true, it will keep getting re-compiled.

        The idea is correct though - that in addition to looking at an entries dependencies, it should also look at the entry itself.

        I will get back after looking a bit more.

        Show
        roshandawrani Roshan Dawrani added a comment - I have looked into the issue a bit and also the patch, and I think the patch is wrong. It is wrong because it is comparing a ScriptCacheEntry's modified time with current system time, which will always be true and hence the script will always get re-compiled, even when there are no changes to the script. So, say, a script was first loaded by GSE at time T1 and next calls are made at T1 + t1(System.currentTimeMillis), T1 + t2, T1 + t3, and there is no change in the script file at all, but because T1 < T1 + t1(or t2 or t3) will always give true, it will keep getting re-compiled. The idea is correct though - that in addition to looking at an entries dependencies, it should also look at the entry itself. I will get back after looking a bit more.
        Hide
        novoj Jan Novotný added a comment -

        Yes, I am sure 1.7.0 is used - it's the explicit dependency set in pom.xml - try this:

        mvn dependency:tree

        This should print the dependent JARS.

        Moreover, I've tested my patch on latest sources checked out from Subversion and test passes. Spring shouldn't be involved in this test as I use directly GroovyScriptEngine.

        Show
        novoj Jan Novotný added a comment - Yes, I am sure 1.7.0 is used - it's the explicit dependency set in pom.xml - try this: mvn dependency:tree This should print the dependent JARS. Moreover, I've tested my patch on latest sources checked out from Subversion and test passes. Spring shouldn't be involved in this test as I use directly GroovyScriptEngine.
        Hide
        roshandawrani Roshan Dawrani added a comment -

        Please ignore my last couple of comments. I forgot about your patch - you must have tested it on 1.7.0, so it must be 1.7.0 on the mvn classpath. Sorry.

        Show
        roshandawrani Roshan Dawrani added a comment - Please ignore my last couple of comments. I forgot about your patch - you must have tested it on 1.7.0, so it must be 1.7.0 on the mvn classpath. Sorry.
        Hide
        roshandawrani Roshan Dawrani added a comment -

        Or will the groovy version be taken from pom of your test code, which is 1.7.0?

        Sorry I don't know maven much - just wanted to make sure that it is 1.7.0 that is getting used by test, as there is not much point in debugging it if it is using 1.5.6.

        Can I make maven output the runtime classpath it is using?

        Show
        roshandawrani Roshan Dawrani added a comment - Or will the groovy version be taken from pom of your test code, which is 1.7.0? Sorry I don't know maven much - just wanted to make sure that it is 1.7.0 that is getting used by test, as there is not much point in debugging it if it is using 1.5.6. Can I make maven output the runtime classpath it is using?
        Hide
        roshandawrani Roshan Dawrani added a comment -

        One confusion. You pom shows that you are using spring 2.5.6 and spring/2.5.6/spring-2.5.6.pom shows that it depends on groovy 1.5.6.

        I think, after 1.5.6 (especially in 1.7.0), GroovyScriptEngine has undergone a lot of changes in the (reloading) logic it uses to determine if sources/their dependencies have changed.

        Will it be possible for you to try it with a version of spring that uses a newer groovy version and see if you still face this issue?

        Changes to groovy 1.5.x line are not happening now anyway.

        Show
        roshandawrani Roshan Dawrani added a comment - One confusion. You pom shows that you are using spring 2.5.6 and spring/2.5.6/spring-2.5.6.pom shows that it depends on groovy 1.5.6. I think, after 1.5.6 (especially in 1.7.0), GroovyScriptEngine has undergone a lot of changes in the (reloading) logic it uses to determine if sources/their dependencies have changed. Will it be possible for you to try it with a version of spring that uses a newer groovy version and see if you still face this issue? Changes to groovy 1.5.x line are not happening now anyway.
        Hide
        novoj Jan Novotný added a comment -

        It's for my own good
        For this issue only test: testReloadGroovyClass(com.fg.scripting.groovy.GroovyScriptingEngineReloadSupportTest) is interesting. The second test is reported in another issue (GROOVY-3981), documenting incompatibility with Spring Framework.

        Show
        novoj Jan Novotný added a comment - It's for my own good For this issue only test: testReloadGroovyClass(com.fg.scripting.groovy.GroovyScriptingEngineReloadSupportTest) is interesting. The second test is reported in another issue ( GROOVY-3981 ), documenting incompatibility with Spring Framework.
        Hide
        roshandawrani Roshan Dawrani added a comment -

        I am now able to run the tests. Following are found to be failing -

        testReloadGroovyClass(com.fg.scripting.groovy.GroovyScriptingEngineReloadSupportTest)
        testReloadLoadGroovyClass(com.fg.scripting.groovy.SpringGroovyReloadSupportTest)

        Thanks for quickly assisting me in trying to reproduce the issue locally.

        Show
        roshandawrani Roshan Dawrani added a comment - I am now able to run the tests. Following are found to be failing - testReloadGroovyClass(com.fg.scripting.groovy.GroovyScriptingEngineReloadSupportTest) testReloadLoadGroovyClass(com.fg.scripting.groovy.SpringGroovyReloadSupportTest) Thanks for quickly assisting me in trying to reproduce the issue locally.
        Hide
        novoj Jan Novotný added a comment -

        I excluded problematic libraries and configured compiler plugin. Try it please and sorry not making it on the first strike. Nice example of environment differences.

        Show
        novoj Jan Novotný added a comment - I excluded problematic libraries and configured compiler plugin. Try it please and sorry not making it on the first strike. Nice example of environment differences.
        Hide
        novoj Jan Novotný added a comment -

        Javax libraries are not on public repositories due to licensing issues. They are not necessary for the test, but are probably in Log4J transitive dependencies. I will try to configure it better.

        Show
        novoj Jan Novotný added a comment - Javax libraries are not on public repositories due to licensing issues. They are not necessary for the test, but are probably in Log4J transitive dependencies. I will try to configure it better.
        Hide
        roshandawrani Roshan Dawrani added a comment -

        Still no luck - first compilation of java files was failing with the message that "annotation/generics not supported. Try -source 1.5". I configured the maven-compiler-plugin to use 1.5 source version.

        After that downloads of javax/mail, javax/jms artifacts is not going through for me.

        Show
        roshandawrani Roshan Dawrani added a comment - Still no luck - first compilation of java files was failing with the message that "annotation/generics not supported. Try -source 1.5". I configured the maven-compiler-plugin to use 1.5 source version. After that downloads of javax/mail, javax/jms artifacts is not going through for me.
        Hide
        novoj Jan Novotný added a comment -

        Can you please try this one?

        Show
        novoj Jan Novotný added a comment - Can you please try this one?
        Hide
        novoj Jan Novotný added a comment -

        Sorry, I forget to remove our company parent pom. I will replace Zip with correct version. Give me some time please.

        Show
        novoj Jan Novotný added a comment - Sorry, I forget to remove our company parent pom. I will replace Zip with correct version. Give me some time please.
        Hide
        roshandawrani Roshan Dawrani added a comment -

        I wanted to run the given tests to see what was happening in the reloading, but am unable to do it as it fails with

        [INFO] Failed to resolve artifact.
        
        GroupId: com.fg
        ArtifactId: fg-default-jdk15-library
        Version: 1.12
        
        Reason: Unable to download the artifact from any repository
        
          com.fg:fg-default-jdk15-library:pom:1.12
        

        What do I need to do to run the tests?

        Show
        roshandawrani Roshan Dawrani added a comment - I wanted to run the given tests to see what was happening in the reloading, but am unable to do it as it fails with [INFO] Failed to resolve artifact. GroupId: com.fg ArtifactId: fg-default-jdk15-library Version: 1.12 Reason: Unable to download the artifact from any repository com.fg:fg-default-jdk15-library:pom:1.12 What do I need to do to run the tests?

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 1h
              1h
              Remaining:
              Remaining Estimate - 1h
              1h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development