Groovy
  1. Groovy
  2. GROOVY-3702

Groovlet: depending groovy files are not recompiled

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.6.4
    • Fix Version/s: 1.7-rc-1
    • Component/s: Compiler, Groovlet / GSP
    • Labels:
      None
    • Environment:
      Linux Ubuntu 8.04.3 LTS, Kernel 2.6.24-16-virtual, JVM 1.6.0_14-b08, Apache Tomcat 6.0.20

      Description

      If you change file b.groovy which is used by a.groovy, file b.groovy is not recompiled. Both files are in the same (root) directory of a web application.

      Example:
      a.groovy

      println "<html><body>";
      println "a: test<br/>";
      def b = new b();
      println b.test();
      println "</body></html>";
      

      b.groovy

      public class b {
          def String test() {
              return "b: test<br/>";
          }
      }
      

      Calling a.groovy by a URL like this http://localhost:8080/groovy/a.groovy will show this output:

      a: test
      b: test
      

      If in file b.groovy the return value is changed to "b: test2<br/>" you will not see the change when calling a.groovy again.

      1. 3702_v17x.txt
        2 kB
        Roshan Dawrani

        Issue Links

          Activity

          Guido Schoepp created issue -
          Hide
          Guido Schoepp added a comment -

          I just tested this with several groovy-jars (1.6.4, 1.6.3, 1.6.2, 1.6.1, 1.5.4, 1.5.1) and on another Linux host (Ubuntu 8.04.3 LTS, Kernel 2.6.24-22-server) with JVM 1.6.0_07-b06 and Tomcat 6.0.14. The result is the same.

          Show
          Guido Schoepp added a comment - I just tested this with several groovy-jars (1.6.4, 1.6.3, 1.6.2, 1.6.1, 1.5.4, 1.5.1) and on another Linux host (Ubuntu 8.04.3 LTS, Kernel 2.6.24-22-server) with JVM 1.6.0_07-b06 and Tomcat 6.0.14. The result is the same.
          Hide
          Jochen Theodorou added a comment -

          could you test Groovy-1.7-beta-1? This release contains a new GroovyScriptEngine, which I think is used here.

          Show
          Jochen Theodorou added a comment - could you test Groovy-1.7-beta-1? This release contains a new GroovyScriptEngine, which I think is used here.
          Hide
          Guido Schoepp added a comment -

          I just tested it with groovy-all-1.7-beta-1.jar but it doesn't help.

          The result is strange. After stopping and starting Tomcat the first call of http://localhost:8080/groovy/a.groovy results in the following error:
          org.codehaus.groovy.control.ErrorCollector.failIfErrors(ErrorCollector.java:296)
          org.codehaus.groovy.control.MultipleCompilationErrorsException: startup failed:
          /localhost/ingaapps/a.groovy: 5: unable to resolve class b
          @ line 3, column 9.

          After manually calling http://localhost:8080/groovy/b.groovy a following call of a.groovy shows the expected result. But after changing the return value in b.groovy the file will still not be recompiled.

          Show
          Guido Schoepp added a comment - I just tested it with groovy-all-1.7-beta-1.jar but it doesn't help. The result is strange. After stopping and starting Tomcat the first call of http://localhost:8080/groovy/a.groovy results in the following error: org.codehaus.groovy.control.ErrorCollector.failIfErrors(ErrorCollector.java:296) org.codehaus.groovy.control.MultipleCompilationErrorsException: startup failed: /localhost/ingaapps/a.groovy: 5: unable to resolve class b @ line 3, column 9. After manually calling http://localhost:8080/groovy/b.groovy a following call of a.groovy shows the expected result. But after changing the return value in b.groovy the file will still not be recompiled.
          Hide
          Guido Schoepp added a comment -

          b.groovy is recompiled as expected with groovy-all-1.0.jar

          Show
          Guido Schoepp added a comment - b.groovy is recompiled as expected with groovy-all-1.0.jar
          Hide
          Guillaume Delcroix added a comment -

          Hi Guido,

          This should now really be fixed in Groovy 1.7-beta-2.
          Can you please confirm this?

          Show
          Guillaume Delcroix added a comment - Hi Guido, This should now really be fixed in Groovy 1.7-beta-2. Can you please confirm this?
          Hide
          Guido Schoepp added a comment -

          Hi Guillaume,

          I just tested 1.7-beta-2. On the first call of http://localhost:8080/groovy/a.groovy everything seems ok. But if you try to access http://localhost:8080/groovy/a.groovy a second time the following error occurs:

          INFO: GroovyServlet Error:  script: '/a.groovy':  Script not found, sending 404.
          
          Show
          Guido Schoepp added a comment - Hi Guillaume, I just tested 1.7-beta-2. On the first call of http://localhost:8080/groovy/a.groovy everything seems ok. But if you try to access http://localhost:8080/groovy/a.groovy a second time the following error occurs: INFO: GroovyServlet Error: script: '/a.groovy': Script not found, sending 404.
          Hide
          Guillaume Delcroix added a comment -

          Guido, could you please attach a zip of a minimalistic webapp that fails for you?
          Because I've just tried creating a small app, with 1.7-beta-2, using the Groovlet servlet, with two scripts a.groovy and b.groovy in WEB-INF/groovy, and tried accessing and modifying a and b, and all the cases I've tried just worked out of the box.

          Show
          Guillaume Delcroix added a comment - Guido, could you please attach a zip of a minimalistic webapp that fails for you? Because I've just tried creating a small app, with 1.7-beta-2, using the Groovlet servlet, with two scripts a.groovy and b.groovy in WEB-INF/groovy, and tried accessing and modifying a and b, and all the cases I've tried just worked out of the box.
          Hide
          Guillaume Delcroix added a comment -

          I tried in Jetty (actually, Google App Engine's Jetty), and it worked great.
          Now I've just tried in Tomcat, and it fails as you explained.

          Show
          Guillaume Delcroix added a comment - I tried in Jetty (actually, Google App Engine's Jetty), and it worked great. Now I've just tried in Tomcat, and it fails as you explained.
          Guillaume Delcroix made changes -
          Field Original Value New Value
          Fix Version/s 1.7-rc-1 [ 14666 ]
          Paul King made changes -
          Link This issue depends upon GROOVY-2212 [ GROOVY-2212 ]
          Hide
          Paul King added a comment -

          Possible related or duplicate issue

          Show
          Paul King added a comment - Possible related or duplicate issue
          Paul King made changes -
          Link This issue is related to GROOVY-2212 [ GROOVY-2212 ]
          Hide
          Paul King added a comment -

          Possible related or duplicate issue

          Show
          Paul King added a comment - Possible related or duplicate issue
          Paul King made changes -
          Link This issue depends upon GROOVY-2212 [ GROOVY-2212 ]
          Hide
          Roshan Dawrani added a comment -

          I looked into it and this issue is still there and hasn't benefited from GROOVY-3888.

          GROOVY-3888 has only removed the 404 issue mentioned above and allowed the original problem to surface with 1.7-rc too. The original issue is there because of the GSE#isSourceNewer() logic, which I doubt was tested with Tomcat (which has different behavior from Jetty, for which it works).

          So, the issue is that when a script, say '/a.groovy' is accessed for 2nd time and control comes in GSE#isSourceNewer(), it does the following to get the current timestamp of the script so it can compare it with previously cached timestamp (when it last compiled it):

          URLConnection conn = rc.getResourceConnection("/a.groovy");
          File file = new File(conn.getURL().getPath());
          long lastMod = file.lastModified();
          

          Now the problem is that Jetty and Tomcat behave differently with these URL paths. While Jetty gives back URL as file:/C:/jetty-6.0.1/webapps/g3702/a.groovy (full, real path), tomcat gives it back as "jndi:/localhost/g3702/a.groovy" (a logical path)

          Because Tomcat's is a logical path, file.exists() is always false and file.lastModified() always 0, so it doesn't pick up the newer timestamp (which is only available through a real file, that exists).

          I couldn't think of a clean solution to get the real path back from AbstractHttpServlet in addition to the URLConnection because call is made through groovy.util.ResourceConnector, which is on the public API, and it didn't want to pollute it with something hackish.

          It's 2 AM here now. If someone wants to take it further, it's welcomed

          Show
          Roshan Dawrani added a comment - I looked into it and this issue is still there and hasn't benefited from GROOVY-3888 . GROOVY-3888 has only removed the 404 issue mentioned above and allowed the original problem to surface with 1.7-rc too. The original issue is there because of the GSE#isSourceNewer() logic, which I doubt was tested with Tomcat (which has different behavior from Jetty, for which it works). So, the issue is that when a script, say '/a.groovy' is accessed for 2nd time and control comes in GSE#isSourceNewer(), it does the following to get the current timestamp of the script so it can compare it with previously cached timestamp (when it last compiled it): URLConnection conn = rc.getResourceConnection( "/a.groovy" ); File file = new File(conn.getURL().getPath()); long lastMod = file.lastModified(); Now the problem is that Jetty and Tomcat behave differently with these URL paths. While Jetty gives back URL as file:/C:/jetty-6.0.1/webapps/g3702/a.groovy (full, real path), tomcat gives it back as "jndi:/localhost/g3702/a.groovy" (a logical path) Because Tomcat's is a logical path, file.exists() is always false and file.lastModified() always 0, so it doesn't pick up the newer timestamp (which is only available through a real file, that exists). I couldn't think of a clean solution to get the real path back from AbstractHttpServlet in addition to the URLConnection because call is made through groovy.util.ResourceConnector, which is on the public API, and it didn't want to pollute it with something hackish. It's 2 AM here now. If someone wants to take it further, it's welcomed
          Hide
          Jochen Theodorou added a comment -

          I assume asking the connection would work. So maybe we should test the URL for having the file protocol and if that is the case use the old logic, but if not, then use the connection to know if the modification time is newer... Something like this can be found in GCL

          Show
          Jochen Theodorou added a comment - I assume asking the connection would work. So maybe we should test the URL for having the file protocol and if that is the case use the old logic, but if not, then use the connection to know if the modification time is newer... Something like this can be found in GCL
          Hide
          Roshan Dawrani added a comment -

          Ok, with the GCL like way of checking lastModified timestamp - based on URL protocol, isSourceNewer() is working fine now, but now it has stuck at another point in GSE - at GSE#createCompilationUnit(), where it adds JNDI based paths like /localhost/g3702/b.groovy also with file protocol, and then it fails again, because "file://localhost/g3702/b.groovy" is not a valid source pointer. So, 404s again.

          And then in GSE#createCompilationUnit(), dependency cache is just string paths, so cannot even know if it is supposed to be "file" protocol based or, say, "jndi". Right now, it simply hard-codes "file" protocol there.

          I think someone who is better/more fully aware of this whole dependency caching logic needs to re-visit it considering the sources will not always be "file" protocol based - and make changes at all relevant places in GSE.

          How about maintaining dependency cache only in terms of real "file" protocol based paths only? Then even non-file protocol based like "jndi:/localhost/g3702/b.groovy" will first become like "file://c:/x/y/g3702/b.groovy".

          Show
          Roshan Dawrani added a comment - Ok, with the GCL like way of checking lastModified timestamp - based on URL protocol, isSourceNewer() is working fine now, but now it has stuck at another point in GSE - at GSE#createCompilationUnit(), where it adds JNDI based paths like /localhost/g3702/b.groovy also with file protocol, and then it fails again, because "file://localhost/g3702/b.groovy" is not a valid source pointer. So, 404s again. And then in GSE#createCompilationUnit(), dependency cache is just string paths, so cannot even know if it is supposed to be "file" protocol based or, say, "jndi". Right now, it simply hard-codes "file" protocol there. I think someone who is better/more fully aware of this whole dependency caching logic needs to re-visit it considering the sources will not always be "file" protocol based - and make changes at all relevant places in GSE. How about maintaining dependency cache only in terms of real "file" protocol based paths only? Then even non-file protocol based like "jndi:/localhost/g3702/b.groovy" will first become like "file://c:/x/y/g3702/b.groovy".
          Hide
          Jochen Theodorou added a comment -

          how do we transform the path?

          Show
          Jochen Theodorou added a comment - how do we transform the path?
          Hide
          Roshan Dawrani added a comment -

          I am attaching a patch that transforms the paths initially in AbstractHttpServlet itself to real paths so that further down in GSE we don't deal with both "file:" and non-"file:" URLs like "jndi:".

          With this change the fix of GROOVY-3888 is not required anymore and I have reverted it in this patch. Also, GSE does not need to check the lastModified timestamp in a GCL-like manner - it can continue to assume that URLs are "file:" based - because now AbstractHttpServlet ensures that.

          I have tested both use-cases of GROOVY-3702, GROOVY-3888 with this patch on both Tomcat (6.0.20) and Jetty (6.0.1).

          Show
          Roshan Dawrani added a comment - I am attaching a patch that transforms the paths initially in AbstractHttpServlet itself to real paths so that further down in GSE we don't deal with both "file:" and non-"file:" URLs like "jndi:". With this change the fix of GROOVY-3888 is not required anymore and I have reverted it in this patch. Also, GSE does not need to check the lastModified timestamp in a GCL-like manner - it can continue to assume that URLs are "file:" based - because now AbstractHttpServlet ensures that. I have tested both use-cases of GROOVY-3702 , GROOVY-3888 with this patch on both Tomcat (6.0.20) and Jetty (6.0.1).
          Roshan Dawrani made changes -
          Attachment 3702_v17x.txt [ 45962 ]
          Hide
          Jochen Theodorou added a comment -

          te patch looks good to me... can you test if that does fix GROOVY-2212 as well?

          Show
          Jochen Theodorou added a comment - te patch looks good to me... can you test if that does fix GROOVY-2212 as well?
          Hide
          Roshan Dawrani added a comment -

          Yes, this patch is fixing GROOVY-2212 as well. I again verified on both Tomcat (6.0.20) and Jetty (6.0.1). I guess Jetty was ok earlier also, but Tomcat run was also fine now.

          For cross-checking, I reverted my patch and checked that GROOVY-2212 was going back to having its issues (script not picking up the changes made after Tomcat restart) - and it was having the issue for the same reason that we discussed before - that timestamp was coming as 0 from 'new File("/localhost/a/b/groovy").lastModified' since the path is non-existent physically. Since the patch transforms this path to real-path, timestamp comes alright and scripts get loaded alright.

          Show
          Roshan Dawrani added a comment - Yes, this patch is fixing GROOVY-2212 as well. I again verified on both Tomcat (6.0.20) and Jetty (6.0.1). I guess Jetty was ok earlier also, but Tomcat run was also fine now. For cross-checking, I reverted my patch and checked that GROOVY-2212 was going back to having its issues (script not picking up the changes made after Tomcat restart) - and it was having the issue for the same reason that we discussed before - that timestamp was coming as 0 from 'new File("/localhost/a/b/groovy").lastModified' since the path is non-existent physically. Since the patch transforms this path to real-path, timestamp comes alright and scripts get loaded alright.
          Hide
          Roshan Dawrani added a comment -

          I have checked-in the patch now. GROOVY-3888, GROOVY-3702, and GROOVY-2212 were all found fixed on Tomcat/Jetty in my local testing.

          It will be nice if someone can independently verify the same.

          Also on Google App Engine?

          Show
          Roshan Dawrani added a comment - I have checked-in the patch now. GROOVY-3888 , GROOVY-3702 , and GROOVY-2212 were all found fixed on Tomcat/Jetty in my local testing. It will be nice if someone can independently verify the same. Also on Google App Engine?
          Hide
          Guillaume Delcroix added a comment -

          I haven't double checked on Tomcat and Jetty, but just on Jetty for Google App Engine, both in the local development mode and once deployed on Google's cloud, and the latest code in trunks works perfectly for me. Well done.

          Show
          Guillaume Delcroix added a comment - I haven't double checked on Tomcat and Jetty, but just on Jetty for Google App Engine, both in the local development mode and once deployed on Google's cloud, and the latest code in trunks works perfectly for me. Well done.
          Hide
          Roshan Dawrani added a comment -

          Thanks.

          So, I will double-check all 3 of them once more on Jetty/Tomcat. If they work fine, is it ok if I mark them fixed?

          Also, since the GSE's re-implementation with all the dependency checking is only there in trunk, are these issues only trunk specific? Or some of them may be present in 1.6.6 too? I haven't had a look at 1.6.6 yet.

          Show
          Roshan Dawrani added a comment - Thanks. So, I will double-check all 3 of them once more on Jetty/Tomcat. If they work fine, is it ok if I mark them fixed? Also, since the GSE's re-implementation with all the dependency checking is only there in trunk, are these issues only trunk specific? Or some of them may be present in 1.6.6 too? I haven't had a look at 1.6.6 yet.
          Hide
          Guillaume Delcroix added a comment -

          I think the issue can safely be marked as fixed thanks to your fixes.
          Groovy 1.6 should be fine, as we haven't touched it much (if at all) with regards to these GSE re-implementations.
          You can have a look at 1.6 to be certain, but I don't think it's needed.

          Show
          Guillaume Delcroix added a comment - I think the issue can safely be marked as fixed thanks to your fixes. Groovy 1.6 should be fine, as we haven't touched it much (if at all) with regards to these GSE re-implementations. You can have a look at 1.6 to be certain, but I don't think it's needed.
          Hide
          Roshan Dawrani added a comment -

          GROOVY-3702 and GROOVY-2212 are both failing on 1.6.6 - on both Jetty and Tomcat - both having the same issue - that if a.groovy depends on b.groovy and

          1) if a.groovy changes on the fly, its changes are seen

          2) if b.groovy changes, a.groovy is not re-compiled (as it does in trunk considering that one of files that it depends upon - b.groovy - has changed)

          But to achieve this, I guess, the whole trunk's version of GSE has to be ported back before my tiny fixes will have any effect

          Let me know what is the call on that? Do we want to port it and have this dependency management logic available in 1.6.6 as well? Or is it a feature that groovy will have from 1.7-rc-1 onwards and ot earlier?

          Show
          Roshan Dawrani added a comment - GROOVY-3702 and GROOVY-2212 are both failing on 1.6.6 - on both Jetty and Tomcat - both having the same issue - that if a.groovy depends on b.groovy and 1) if a.groovy changes on the fly, its changes are seen 2) if b.groovy changes, a.groovy is not re-compiled (as it does in trunk considering that one of files that it depends upon - b.groovy - has changed) But to achieve this, I guess, the whole trunk's version of GSE has to be ported back before my tiny fixes will have any effect Let me know what is the call on that? Do we want to port it and have this dependency management logic available in 1.6.6 as well? Or is it a feature that groovy will have from 1.7-rc-1 onwards and ot earlier?
          Hide
          Guillaume Delcroix added a comment -

          Groovy 1.6 had some limitations regarding reloading anyway. The rewrite was just deemed for 1.7.
          So I think we should encourage people to move to 1.7 once it's released in final version.

          Show
          Guillaume Delcroix added a comment - Groovy 1.6 had some limitations regarding reloading anyway. The rewrite was just deemed for 1.7. So I think we should encourage people to move to 1.7 once it's released in final version.
          Hide
          Roshan Dawrani added a comment -

          So, you are confirming that these GSE re-implementation and then these fixes don't have to be ported to 1.6.x and the fixes will be available from 1.7-rc-1 onwards?

          If that is the case, I can mark them fixed. I re-tested them once more and trunk fixes look alright on both Tomcat/Jetty.

          Show
          Roshan Dawrani added a comment - So, you are confirming that these GSE re-implementation and then these fixes don't have to be ported to 1.6.x and the fixes will be available from 1.7-rc-1 onwards? If that is the case, I can mark them fixed. I re-tested them once more and trunk fixes look alright on both Tomcat/Jetty.
          Hide
          Guillaume Delcroix added a comment -

          Yes, I confirm the GSE re-implementation and the fixes don't have to be back-ported to 1.6.x.
          You can mark the issue as fixed, yes.

          Show
          Guillaume Delcroix added a comment - Yes, I confirm the GSE re-implementation and the fixes don't have to be back-ported to 1.6.x. You can mark the issue as fixed, yes.
          Roshan Dawrani made changes -
          Assignee Roshan Dawrani [ roshandawrani ]
          Hide
          Roshan Dawrani added a comment -

          Fixed.

          Show
          Roshan Dawrani added a comment - Fixed.
          Roshan Dawrani made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Hide
          Roshan Dawrani added a comment -

          Sorry. Wasn't trying to irritate you

          Show
          Roshan Dawrani added a comment - Sorry. Wasn't trying to irritate you
          Hide
          Guillaume Delcroix added a comment -

          You're never irritating me, don't worry!
          My answers were probably not clearly explained, sorry if I confused you in any way.

          Show
          Guillaume Delcroix added a comment - You're never irritating me, don't worry! My answers were probably not clearly explained, sorry if I confused you in any way.
          Hide
          Roshan Dawrani added a comment -

          Guillaume, now the issue that you mailed (about groovlet re-loading with Jetty 7) is also taken care of.

          All previous 3 JIRAs + Jetty 7 issue from your mail - all I re-tested with Jetty 7, Jetty 6 and Tomcat 6.

          Show
          Roshan Dawrani added a comment - Guillaume, now the issue that you mailed (about groovlet re-loading with Jetty 7) is also taken care of. All previous 3 JIRAs + Jetty 7 issue from your mail - all I re-tested with Jetty 7, Jetty 6 and Tomcat 6.
          Hide
          Guillaume Delcroix added a comment -

          Excellent, Roshan.
          I think these were the remaining issues we had with our new GSE implementation and the related usage of it with our servlets.
          So we're all good now.
          Thanks a lot for your excellent work, as usual.

          Show
          Guillaume Delcroix added a comment - Excellent, Roshan. I think these were the remaining issues we had with our new GSE implementation and the related usage of it with our servlets. So we're all good now. Thanks a lot for your excellent work, as usual.
          Paul King made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Mark Thomas made changes -
          Project Import Sun Apr 05 13:32:57 UTC 2015 [ 1428240777691 ]
          Mark Thomas made changes -
          Workflow jira [ 12732685 ] Default workflow, editable Closed status [ 12744285 ]
          Mark Thomas made changes -
          Project Import Mon Apr 06 02:11:23 UTC 2015 [ 1428286283443 ]
          Mark Thomas made changes -
          Workflow jira [ 12970270 ] Default workflow, editable Closed status [ 12978018 ]
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Resolved Resolved
          88d 18h 28m 1 Roshan Dawrani 22/Nov/09 05:21
          Resolved Resolved Closed Closed
          17d 12h 54m 1 Paul King 09/Dec/09 18:16

            People

            • Assignee:
              Roshan Dawrani
              Reporter:
              Guido Schoepp
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development