jonnybot0 opened a new pull request, #1816: URL: https://github.com/apache/groovy/pull/1816
# The Context Following discussion on GROOVY-9541 and [a prior pull request](https://github.com/apache/groovy/pull/1814#issuecomment-1292252517), here's a proposal for how to address GROOVY-9541. One notable impact of the bug is that collector annotations like `@Immutable` and `@Canonical` won't work consistently due to the `ASTTransformationCollectorCodeVisitor#findCollectedAnnotations` method's dependence on `StaticTypeCheckingSupport#evaluateExpression`. # The Fix Basically, make a classloader a required parameter of `StaticTypeCheckingSupport#evaluateExpression`. In OSGi and other affected contexts, that should prevent Groovy from using the context classloader by default. As @eric-milles noted on the previous PR, each usage of `evaluateExpression` needs evaluated to determine which classloader should be passed. This has been done, and all usages in this repository have been updated. I've also kept the old method signature as a deprecated method for backward compatibility. # The Kludge One problem I ran into was that, with the fix in the first commit (b7d964b), the `compileTestGroovy` task fails with errors like this: ``` .../src/spec/test/testingguide/GroovyTestCaseExampleTests.groovy: Could not find class for Transformation Processor org.apache.groovy.test.transform.NotYetImplementedASTTransformation declared by groovy.test.NotYetImplemented ``` I tracked this down to the call to `ASTTransformationCollectorCodeVisitor#loadTransformClass` from the `findCollectedAnnotations` method on the same class. A lot of files, like `GroovyTestCaseExampleTests.groovy` were failing to compile because they couldn't load the `NotYetImplementedASTTransformation` annotation anymore. That baffled me, since I couldn't see how the `transformLoader` field used there would be affected by the change at line 186: ``` .map(exp -> evaluateExpression(exp, source.getConfiguration(), transformLoader)) ``` ...yet that was the one that, if changed to `null`, worked just fine. Based on that, it appeared that call to `evaluateExpression` was changing the state of the GroovyClassLoader in a way that could cause it to later fail to load classes that would be known otherwise. The workaround I used in the second commit was to create a new GroovyClassLoader with the one supplied as a parent. I don't entirely like it, as `new`ing up a classloader on every call to `evaluateExpression` is liable to lead to excessive garbage collection when all those ephemeral classloaders have to get thrown away. So really, I'm writing all this to seek advice on a better way to address the problem. Here's the full stack trace swallowed by `ASTTransformationCollectorCodeVisitor#loadTransformClass`'s catch clause (if you revert back to b7d964b8): ``` java.lang.ClassNotFoundException: org.apache.groovy.test.transform.NotYetImplementedASTTransformation at java.base/java.net.URLClassLoader.findClass(URLClassLoader.java:445) at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:587) at groovy.lang.GroovyClassLoader.loadClass(GroovyClassLoader.java:862) at org.codehaus.groovy.transform.ASTTransformationCollectorCodeVisitor.loadTransformClass(ASTTransformationCollectorCodeVisitor.java:220) at org.codehaus.groovy.transform.ASTTransformationCollectorCodeVisitor.lambda$addTransformsToClassNode$2(ASTTransformationCollectorCodeVisitor.java:257) at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197) at java.base/java.util.Spliterators$ArraySpliterator.forEachRemaining(Spliterators.java:992) at java.base/java.util.stream.Streams$ConcatSpliterator.forEachRemaining(Streams.java:734) at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509) at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499) at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150) at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173) at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:596) at org.codehaus.groovy.transform.ASTTransformationCollectorCodeVisitor.addTransformsToClassNode(ASTTransformationCollectorCodeVisitor.java:257) at org.codehaus.groovy.transform.ASTTransformationCollectorCodeVisitor.visitAnnotations(ASTTransformationCollectorCodeVisitor.java:112) at org.codehaus.groovy.ast.ClassCodeVisitorSupport.visitConstructorOrMethod(ClassCodeVisitorSupport.java:114) at org.codehaus.groovy.ast.ClassCodeVisitorSupport.visitMethod(ClassCodeVisitorSupport.java:110) at org.codehaus.groovy.ast.ClassNode.visitMethods(ClassNode.java:1143) at org.codehaus.groovy.ast.ClassNode.visitContents(ClassNode.java:1136) at org.codehaus.groovy.ast.ClassCodeVisitorSupport.visitClass(ClassCodeVisitorSupport.java:52) at org.codehaus.groovy.transform.ASTTransformationCollectorCodeVisitor.visitClass(ASTTransformationCollectorCodeVisitor.java:77) at org.codehaus.groovy.tools.javac.JavaAwareCompilationUnit.lambda$new$1(JavaAwareCompilationUnit.java:89) at org.codehaus.groovy.control.CompilationUnit$IPrimaryClassNodeOperation.doPhaseOperation(CompilationUnit.java:937) at org.codehaus.groovy.control.CompilationUnit.processPhaseOperations(CompilationUnit.java:692) at org.codehaus.groovy.control.CompilationUnit.compile(CompilationUnit.java:666) at org.codehaus.groovy.control.CompilationUnit.compile(CompilationUnit.java:647) at org.gradle.api.internal.tasks.compile.ApiGroovyCompiler.execute(ApiGroovyCompiler.java:270) at org.gradle.api.internal.tasks.compile.ApiGroovyCompiler.execute(ApiGroovyCompiler.java:64) at org.gradle.api.internal.tasks.compile.GroovyCompilerFactory$DaemonSideCompiler.execute(GroovyCompilerFactory.java:97) at org.gradle.api.internal.tasks.compile.GroovyCompilerFactory$DaemonSideCompiler.execute(GroovyCompilerFactory.java:76) at org.gradle.api.internal.tasks.compile.daemon.AbstractDaemonCompiler$CompilerWorkAction.execute(AbstractDaemonCompiler.java:135) at org.gradle.workers.internal.DefaultWorkerServer.execute(DefaultWorkerServer.java:63) at org.gradle.workers.internal.AbstractClassLoaderWorker$1.create(AbstractClassLoaderWorker.java:49) at org.gradle.workers.internal.AbstractClassLoaderWorker$1.create(AbstractClassLoaderWorker.java:43) at org.gradle.internal.classloader.ClassLoaderUtils.executeInClassloader(ClassLoaderUtils.java:100) at org.gradle.workers.internal.AbstractClassLoaderWorker.executeInClassLoader(AbstractClassLoaderWorker.java:43) at org.gradle.workers.internal.IsolatedClassloaderWorker.run(IsolatedClassloaderWorker.java:49) at org.gradle.workers.internal.IsolatedClassloaderWorker.run(IsolatedClassloaderWorker.java:30) at org.gradle.workers.internal.WorkerDaemonServer.run(WorkerDaemonServer.java:87) at org.gradle.workers.internal.WorkerDaemonServer.run(WorkerDaemonServer.java:56) at org.gradle.process.internal.worker.request.WorkerAction$1.call(WorkerAction.java:138) at org.gradle.process.internal.worker.child.WorkerLogEventListener.withWorkerLoggingProtocol(WorkerLogEventListener.java:41) at org.gradle.process.internal.worker.request.WorkerAction.run(WorkerAction.java:135) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:568) at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36) at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24) at org.gradle.internal.remote.internal.hub.MessageHubBackedObjectConnection$DispatchWrapper.dispatch(MessageHubBackedObjectConnection.java:182) at org.gradle.internal.remote.internal.hub.MessageHubBackedObjectConnection$DispatchWrapper.dispatch(MessageHubBackedObjectConnection.java:164) at org.gradle.internal.remote.internal.hub.MessageHub$Handler.run(MessageHub.java:414) at org.gradle.internal.concurrent.ExecutorPolicy$CatchAndRecordFailures.onExecute(ExecutorPolicy.java:64) at org.gradle.internal.concurrent.ManagedExecutorImpl$1.run(ManagedExecutorImpl.java:48) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) at java.base/java.lang.Thread.run(Thread.java:833) startup failed: ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
