Groovy
  1. Groovy
  2. GROOVY-3294

Improved class loading for AST transformations

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.6-rc-1, 1.6-rc-2
    • Fix Version/s: 1.6-rc-3, 1.7-beta-1
    • Component/s: Compiler
    • Labels:
      None
    • Flags:
      Patch

      Description

      Currently, groovyc loads AST transformations with the same class loader that is also responsible for loading compile dependencies. This can lead to problems for compiler clients that provide a custom class loader for loading compile dependencies, as such a class loader might not be able to meet the requirements for loading AST transformations. The most prominent example is GMaven, which uses a custom class loader to prevent classpath pollution without having to fork groovyc. Unfortunately this also means that GMaven cannot compile any programs that make use of AST transformations.
      This patch allows for compiler clients to optionally provide a separate class loader for loading AST transformations, thereby decoupling the class loading of AST transformations from that of compile dependencies. This makes it possible for compiler clients such as GMaven (and possibly Gradle) to support AST transformations, and paves the way for further class loading improvements in future versions of groovyc.
      The patch is fairly small and tries to keep changes to the existing codebase to an absolute minimum. Apart from applying moderate changes to two transform-related classes, it only adds a few lines of code to class CompilationUnit. Existing compiler clients will see no change in behavior. Please consider this patch for inclusion in Groovy 1.6, or a 1.6 maintenance release. Also attached is the corresponding GMaven patch. Any feedback is welcome.

        Issue Links

          Activity

          Peter Niederwieser created issue -
          Guillaume Delcroix made changes -
          Field Original Value New Value
          Fix Version/s 1.6.1 [ 14852 ]
          Guillaume Delcroix made changes -
          Fix Version/s 1.7-beta-1 [ 14014 ]
          Hide
          Guillaume Delcroix added a comment -

          Isn't it related to a previous issue you had opened?

          Show
          Guillaume Delcroix added a comment - Isn't it related to a previous issue you had opened?
          Jason Dillon made changes -
          Link This issue relates to MGROOVY-174 [ MGROOVY-174 ]
          Hide
          Peter Niederwieser added a comment -

          In a sense yes, but this patch has a much smaller scope: the only thing it does is to allow compiler clients to optionally provide a custom class loader for loading AST transformations, just as they have always been able to provide a custom class loader for loading compile dependencies. As such it is a very natural extension of groovyc that both fixes immediate problems and simplifies future efforts such as GROOVY-3169, all without changing the class loading behavior of existing compiler clients.

          Show
          Peter Niederwieser added a comment - In a sense yes, but this patch has a much smaller scope: the only thing it does is to allow compiler clients to optionally provide a custom class loader for loading AST transformations, just as they have always been able to provide a custom class loader for loading compile dependencies. As such it is a very natural extension of groovyc that both fixes immediate problems and simplifies future efforts such as GROOVY-3169 , all without changing the class loading behavior of existing compiler clients.
          Hide
          Jochen Theodorou added a comment -

          there are some small adjustments to the patch I would like to see. SourceUnits may have a different ClassLoader then the CompileUnit or CompilationUnit. So I would like to see the transform loader in the same class.. Also I would probably name it to compilerExtensionLoader or better only extensionLoader. Also per default the laoder should be the same as what getClassLoader produces... these x==null?y:z checks I don't like very much.Then of course ASTTransformationCollectorCodeVisitor should not get the loader in the constructor, but request it from source as it does with the normal classloader.

          Show
          Jochen Theodorou added a comment - there are some small adjustments to the patch I would like to see. SourceUnits may have a different ClassLoader then the CompileUnit or CompilationUnit. So I would like to see the transform loader in the same class.. Also I would probably name it to compilerExtensionLoader or better only extensionLoader. Also per default the laoder should be the same as what getClassLoader produces... these x==null?y:z checks I don't like very much.Then of course ASTTransformationCollectorCodeVisitor should not get the loader in the constructor, but request it from source as it does with the normal classloader.
          Hide
          Peter Niederwieser added a comment -

          > SourceUnits may have a different ClassLoader then the CompileUnit or CompilationUnit. So I would like to see the transform loader in the same class.
          OK, so I should move transformLoader into ProcessingUnit and add another constructor there? Note that without further changes, initialization of transformLoader has to occur at construction time, because loading of global transforms is triggered from within CompilationUnit's constructor.

          > Also I would probably name it to compilerExtensionLoader or better only extensionLoader.
          I can do that, but "extension" already has an established meaning in the JVM (extension classpath, extension class loader).

          > Also per default the loader should be the same as what getClassLoader produces... these x==null?y:z checks I don't like very much.
          Neither do I, but the problem is that the regular class loader can also be set with ProcessingUnit.setClassLoader(), and I tried to support this form of initialization at least for local transforms. If that's not necessary then I can simplify the code.

          Show
          Peter Niederwieser added a comment - > SourceUnits may have a different ClassLoader then the CompileUnit or CompilationUnit. So I would like to see the transform loader in the same class. OK, so I should move transformLoader into ProcessingUnit and add another constructor there? Note that without further changes, initialization of transformLoader has to occur at construction time, because loading of global transforms is triggered from within CompilationUnit's constructor. > Also I would probably name it to compilerExtensionLoader or better only extensionLoader. I can do that, but "extension" already has an established meaning in the JVM (extension classpath, extension class loader). > Also per default the loader should be the same as what getClassLoader produces... these x==null?y:z checks I don't like very much. Neither do I, but the problem is that the regular class loader can also be set with ProcessingUnit.setClassLoader(), and I tried to support this form of initialization at least for local transforms. If that's not necessary then I can simplify the code.
          Hide
          Jochen Theodorou added a comment -

          the transforms loaded in the CompilationUnit are global ones. Of course CompilationUnit will still get the loader and that loader should be used to load the global transforms. IMHO ASTTransformationVisitor.addPhaseOperations should get that loader for this. As for the name "extensionLoader", you are right... then we should stick with compilerExtensionLoader.. or with astTransformationLoader.

          A for ProcessingUnit.setClassLoader()... so you mean if someone uses null for the ast transformation loader, then it should use whatever is used for the normal classloader even if that is changed...hmm.... hadn't thought about that aspect.... But I don' think it is needed, better have a more simple code.

          Show
          Jochen Theodorou added a comment - the transforms loaded in the CompilationUnit are global ones. Of course CompilationUnit will still get the loader and that loader should be used to load the global transforms. IMHO ASTTransformationVisitor.addPhaseOperations should get that loader for this. As for the name "extensionLoader", you are right... then we should stick with compilerExtensionLoader.. or with astTransformationLoader. A for ProcessingUnit.setClassLoader()... so you mean if someone uses null for the ast transformation loader, then it should use whatever is used for the normal classloader even if that is changed...hmm.... hadn't thought about that aspect.... But I don' think it is needed, better have a more simple code.
          Hide
          Peter Niederwieser added a comment -

          >A for ProcessingUnit.setClassLoader()... so you mean if someone uses null for the ast transformation loader, then it should use whatever is used for the normal classloader even if that is changed
          I meant:

          def unit = new CompilationUnit()
          unit.classLoader = someClassLoader
          unit.addSource(someFile)
          unit.compile()
          

          The initialization logic for ProcessingUnit/SourceUnit/CompilationUnit looks overly complex to me, with ProcessingUnit.setClassLoader() being especially evil. For example, if you call setClassLoader() after addSource(), the SourceUnit will end up with the "wrong" class loader. Should I just pretend that setClassLoader() doesn't exist?

          Show
          Peter Niederwieser added a comment - >A for ProcessingUnit.setClassLoader()... so you mean if someone uses null for the ast transformation loader, then it should use whatever is used for the normal classloader even if that is changed I meant: def unit = new CompilationUnit() unit.classLoader = someClassLoader unit.addSource(someFile) unit.compile() The initialization logic for ProcessingUnit/SourceUnit/CompilationUnit looks overly complex to me, with ProcessingUnit.setClassLoader() being especially evil. For example, if you call setClassLoader() after addSource(), the SourceUnit will end up with the "wrong" class loader. Should I just pretend that setClassLoader() doesn't exist?
          Hide
          Peter Niederwieser added a comment -

          One more thing that troubles me is that CompilationUnit.setSource(SourceUnit) always forces the CompilationUnit's class loader on the SourceUnit. Therefore, setting a class loader on the SourceUnit has no effect. Is this OK?

          Show
          Peter Niederwieser added a comment - One more thing that troubles me is that CompilationUnit.setSource(SourceUnit) always forces the CompilationUnit's class loader on the SourceUnit. Therefore, setting a class loader on the SourceUnit has no effect. Is this OK?
          Guillaume Delcroix made changes -
          Fix Version/s 1.6-rc-2 [ 13832 ]
          Fix Version/s 1.6.1 [ 14852 ]
          Hide
          Guillaume Delcroix added a comment -

          I'll let you apply this patch, and amend it as appropriate.

          Show
          Guillaume Delcroix added a comment - I'll let you apply this patch, and amend it as appropriate.
          Guillaume Delcroix made changes -
          Assignee Jochen Theodorou [ blackdrag ]
          Hide
          Jochen Theodorou added a comment -

          I now applied the patch, but with a few changes... Firstly I removed the reflective code you added. For a transform to work properly you need access to the compiler classes, so there is no point in excluding the annotation classes for the transform in th the compiler. Also I changed transform loader so it has to be a GroovyClassLoder. The loader used for transforms might be a GroovyClassLoader, but this loader has the ability compile scripts into classes on its own. But since we are currently in a compilation this must not happen. The result would be a deadlock or a cyclic compilation.... that means a file is to be compiled, which needs another file, which then is compiled, but depends on the first file. So if the compiler spawns a new compilation process outside the current one it would have to spawn new ones again and again. So a special loadClass method is used to avoid this cycle. The compiler then identifies classes to compile itself, instead of depending on GroovyClassLoader for this.

          Of course that means the gmaven patch has to be changed to use GroovyClassLoader instead of URLClassLoader, but that shouldn't be a problem.

          Show
          Jochen Theodorou added a comment - I now applied the patch, but with a few changes... Firstly I removed the reflective code you added. For a transform to work properly you need access to the compiler classes, so there is no point in excluding the annotation classes for the transform in th the compiler. Also I changed transform loader so it has to be a GroovyClassLoder. The loader used for transforms might be a GroovyClassLoader, but this loader has the ability compile scripts into classes on its own. But since we are currently in a compilation this must not happen. The result would be a deadlock or a cyclic compilation.... that means a file is to be compiled, which needs another file, which then is compiled, but depends on the first file. So if the compiler spawns a new compilation process outside the current one it would have to spawn new ones again and again. So a special loadClass method is used to avoid this cycle. The compiler then identifies classes to compile itself, instead of depending on GroovyClassLoader for this. Of course that means the gmaven patch has to be changed to use GroovyClassLoader instead of URLClassLoader, but that shouldn't be a problem.
          Hide
          Jochen Theodorou added a comment -

          I close this issue as fixed now.... should we keep GROOVY-3169 open?

          Show
          Jochen Theodorou added a comment - I close this issue as fixed now.... should we keep GROOVY-3169 open?
          Jochen Theodorou made changes -
          Resolution Fixed [ 1 ]
          Status Open [ 1 ] Closed [ 6 ]
          pnw made changes -
          Resolution Fixed [ 1 ]
          Status Closed [ 6 ] Reopened [ 4 ]
          Hide
          Peter Niederwieser added a comment -

          This patch makes local transforms work in the presence of a custom class loader. It also includes a thorough unit test.

          Show
          Peter Niederwieser added a comment - This patch makes local transforms work in the presence of a custom class loader. It also includes a thorough unit test.
          pnw made changes -
          Attachment transform_class_loading_fixed.patch [ 39573 ]
          Jochen Theodorou made changes -
          Fix Version/s 1.6.1 [ 14852 ]
          Affects Version/s 1.6-rc-2 [ 13832 ]
          Fix Version/s 1.6-rc-2 [ 13832 ]
          Jochen Theodorou made changes -
          Fix Version/s 1.6.1 [ 14852 ]
          Fix Version/s 1.6 [ 14913 ]
          Hide
          Jochen Theodorou added a comment -

          patch applied

          Show
          Jochen Theodorou added a comment - patch applied
          Jochen Theodorou made changes -
          Resolution Fixed [ 1 ]
          Status Reopened [ 4 ] Resolved [ 5 ]
          Paul King made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Cédric Champeau made changes -
          Link This issue is related to GROOVY-5044 [ GROOVY-5044 ]
          Mark Thomas made changes -
          Project Import Sun Apr 05 13:32:57 UTC 2015 [ 1428240777691 ]
          Mark Thomas made changes -
          Workflow jira [ 12732291 ] Default workflow, editable Closed status [ 12744136 ]
          Mark Thomas made changes -
          Patch Submitted Yes [ 10763 ]
          Flags Patch [ 10430 ]
          Mark Thomas made changes -
          Project Import Mon Apr 06 02:11:23 UTC 2015 [ 1428286283443 ]
          Mark Thomas made changes -
          Workflow jira [ 12973598 ] Default workflow, editable Closed status [ 12980764 ]
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Closed Closed
          3d 22h 56m 1 Jochen Theodorou 22/Jan/09 08:55
          Closed Closed Reopened Reopened
          4d 1m 1 pnw 26/Jan/09 08:56
          Reopened Reopened Resolved Resolved
          7d 2h 46m 1 Jochen Theodorou 02/Feb/09 11:43
          Resolved Resolved Closed Closed
          9d 15h 52m 1 Paul King 12/Feb/09 03:35

            People

            • Assignee:
              Jochen Theodorou
              Reporter:
              Peter Niederwieser
            • Votes:
              4 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development