Merge pull request #20999 Remove --add-opens for test workers

Fixes: #19771

* Update ClassLoaderUtils to use public JDK APIs when listing a ClassLoader's package
* Disable adding any --add-opens flags to test worker processes
* Add tests to verify production code and test code behavior is equivalent
* Verify users can opt back into previous behavior by adding --add-opens flags manually to test jvm args
* Rework TestNG version coverage to align it closer to how Groovy versions are selected during tests

TODO:
- [x] Add Release Notes Entry
- [x] Add Upgrade Guide Entry

Co-authored-by: Justin Van Dort <jvandort@gradle.com>

subprojects/base-services/src/main/java/org/gradle/internal/classloader/ClassLoaderUtils.java

@@ -36,7 +36,7 @@ public abstract class ClassLoaderUtils {
 
     static {
         CLASS_DEFINER = JavaVersion.current().isJava9Compatible() ? new LookupClassDefiner() : new ReflectionClassDefiner();
-        CLASS_LOADER_PACKAGES_FETCHER = JavaVersion.current().isJava9Compatible() ? new LookupPackagesFetcher() : new ReflectionPackagesFetcher();
+        CLASS_LOADER_PACKAGES_FETCHER = JavaVersion.current().isJava9Compatible() ? new Java9PackagesFetcher() : new ReflectionPackagesFetcher();
     }
 
     /**
@@ -51,6 +51,8 @@ public static void tryClose(@Nullable ClassLoader classLoader) {
         CompositeStoppable.stoppable(classLoader).stop();
     }
 
+    // Used by the Gradle Play Framework Plugin. See:
+    // https://github.com/gradle/playframework/blob/master/src/main/java/org/gradle/playframework/tools/internal/run/PlayWorkerServer.java#L72
     public static void disableUrlConnectionCaching() {
         // fix problems in updating jar files by disabling default caching of URL connections.
         // URLConnection default caching should be disabled since it causes jar file locking issues and JVM crashes in updating jar files.
@@ -69,6 +71,7 @@ static Package[] getPackages(ClassLoader classLoader) {
         return CLASS_LOADER_PACKAGES_FETCHER.getPackages(classLoader);
     }
 
+    @Nullable
     static Package getPackage(ClassLoader classLoader, String name) {
         return CLASS_LOADER_PACKAGES_FETCHER.getPackage(classLoader, name);
     }
@@ -120,6 +123,7 @@ private interface ClassDefiner {
     private interface ClassLoaderPackagesFetcher {
         Package[] getPackages(ClassLoader classLoader);
 
+        @Nullable
         Package getPackage(ClassLoader classLoader, String name);
     }
 
@@ -188,8 +192,8 @@ private static class LookupClassDefiner extends AbstractClassLoaderLookuper impl
         @SuppressWarnings("unchecked")
         public <T> Class<T> defineDecoratorClass(Class<?> decoratedClass, ClassLoader classLoader, String className, byte[] classBytes) {
             try {
-                // Lookup.defineClass can only define a class into same classloader as the lookup object
-                // we have to use the fallback defineClass() if they're not same, which is the case of ManagedProxyClassGenerator
+                // Lookup.defineClass can only define a class into same classloader as the lookup object.
+                // We have to use the fallback defineClass() if they're not same, which is the case of ManagedProxyClassGenerator
                 if (decoratedClass.getClassLoader() == classLoader) {
                     MethodHandles.Lookup lookup = MethodHandles.privateLookupIn(decoratedClass, baseLookup);
                     return (Class) lookup.defineClass(classBytes);
@@ -216,24 +220,36 @@ public Package[] getPackages(ClassLoader classLoader) {
             return GET_PACKAGES_METHOD.invoke(classLoader);
         }
 
+        @Nullable
         @Override
         public Package getPackage(ClassLoader classLoader, String name) {
             return GET_PACKAGE_METHOD.invoke(classLoader, name);
         }
     }
 
-    private static class LookupPackagesFetcher extends AbstractClassLoaderLookuper implements ClassLoaderPackagesFetcher {
-        private MethodType getPackagesMethodType = MethodType.methodType(Package[].class, new Class<?>[]{});
-        private MethodType getDefinedPackageMethodType = MethodType.methodType(Package.class, new Class<?>[]{String.class});
+    private static class Java9PackagesFetcher implements ClassLoaderPackagesFetcher {
 
         @Override
         public Package[] getPackages(ClassLoader classLoader) {
-            return invoke(classLoader, "getPackages", getPackagesMethodType);
+            return new ClassLoader(classLoader) {
+                @Override
+                protected Package[] getPackages() {
+                    return super.getPackages();
+                }
+            }.getPackages();
         }
 
+        @Nullable
         @Override
         public Package getPackage(ClassLoader classLoader, String name) {
-            return invoke(classLoader, "getPackage", getDefinedPackageMethodType, name);
+            return new ClassLoader(classLoader) {
+                // TODO: This is deprecated for a reason. We should consider migrating if possible.
+                @Override
+                @SuppressWarnings("deprecation")
+                protected Package getPackage(String name) {
+                    return super.getPackage(name);
+                }
+            }.getPackage(name);
         }
     }
 }

subprojects/core/src/integTest/groovy/org/gradle/api/ApplyPluginIntegSpec.groovy

@@ -107,6 +107,24 @@ class ApplyPluginIntegSpec extends AbstractIntegrationSpec {
 
         and:
         buildFile << junitBasedBuildScript()
+        buildFile << """
+            // Needed when using ProjectBuilder
+            class AddOpensArgProvider implements CommandLineArgumentProvider {
+                private final Test test;
+                public AddOpensArgProvider(Test test) {
+                    this.test = test;
+                }
+                @Override
+                Iterable<String> asArguments() {
+                    return test.javaVersion.isCompatibleWith(JavaVersion.VERSION_1_9)
+                        ? ["--add-opens=java.base/java.lang=ALL-UNNAMED"]
+                        : []
+                }
+            }
+            tasks.withType(Test).configureEach {
+                jvmArgumentProviders.add(new AddOpensArgProvider(it))
+            }
+        """
 
         expect:
         executer.withArgument("--info")

subprojects/core/src/integTest/groovy/org/gradle/testfixtures/ProjectBuilderEndUserIntegrationTest.groovy

@@ -44,6 +44,22 @@ class ProjectBuilderEndUserIntegrationTest extends AbstractIntegrationSpec {
             testLogging.exceptionFormat = 'full'
         }
 
+        // Needed when using ProjectBuilder
+        class AddOpensArgProvider implements CommandLineArgumentProvider {
+            private final Test test;
+            public AddOpensArgProvider(Test test) {
+                this.test = test;
+            }
+            @Override
+            Iterable<String> asArguments() {
+                return test.javaVersion.isCompatibleWith(JavaVersion.VERSION_1_9)
+                    ? ["--add-opens=java.base/java.lang=ALL-UNNAMED"]
+                    : []
+            }
+        }
+        tasks.withType(Test).configureEach {
+            jvmArgumentProviders.add(new AddOpensArgProvider(it))
+        }
         """
     }
 

subprojects/core/src/main/java/org/gradle/process/internal/worker/DefaultWorkerProcessBuilder.java

@@ -19,7 +19,9 @@
 import org.gradle.api.Action;
 import org.gradle.api.logging.LogLevel;
 import org.gradle.internal.id.IdGenerator;
+import org.gradle.internal.jvm.JpmsConfiguration;
 import org.gradle.internal.jvm.Jvm;
+import org.gradle.internal.jvm.inspection.JvmVersionDetector;
 import org.gradle.internal.logging.events.OutputEventListener;
 import org.gradle.internal.remote.Address;
 import org.gradle.internal.remote.ConnectionAcceptor;
@@ -60,23 +62,27 @@ public class DefaultWorkerProcessBuilder implements WorkerProcessBuilder {
     private final Set<File> applicationModulePath = new LinkedHashSet<>();
 
     private final MemoryManager memoryManager;
+    private final JvmVersionDetector jvmVersionDetector;
+
     private Action<? super WorkerProcessContext> action;
     private LogLevel logLevel = LogLevel.LIFECYCLE;
     private String baseName = "Gradle Worker";
-    private File gradleUserHomeDir;
     private int connectTimeoutSeconds;
     private List<URL> implementationClassPath;
     private List<URL> implementationModulePath;
     private boolean shouldPublishJvmMemoryInfo;
 
-    DefaultWorkerProcessBuilder(JavaExecHandleFactory execHandleFactory, MessagingServer server, IdGenerator<Long> idGenerator, ApplicationClassesInSystemClassLoaderWorkerImplementationFactory workerImplementationFactory, OutputEventListener outputEventListener, MemoryManager memoryManager) {
+    private boolean useLegacyAddOpens = true;
+
+    DefaultWorkerProcessBuilder(JavaExecHandleFactory execHandleFactory, MessagingServer server, IdGenerator<Long> idGenerator, ApplicationClassesInSystemClassLoaderWorkerImplementationFactory workerImplementationFactory, OutputEventListener outputEventListener, MemoryManager memoryManager, JvmVersionDetector jvmVersionDetector) {
         this.javaCommand = execHandleFactory.newJavaExec();
         this.javaCommand.setExecutable(Jvm.current().getJavaExecutable());
         this.server = server;
         this.idGenerator = idGenerator;
         this.workerImplementationFactory = workerImplementationFactory;
         this.outputEventListener = outputEventListener;
         this.memoryManager = memoryManager;
+        this.jvmVersionDetector = jvmVersionDetector;
     }
 
     public int getConnectTimeoutSeconds() {
@@ -174,14 +180,6 @@ public WorkerProcessBuilder setLogLevel(LogLevel logLevel) {
         return this;
     }
 
-    public File getGradleUserHomeDir() {
-        return gradleUserHomeDir;
-    }
-
-    public void setGradleUserHomeDir(File gradleUserHomeDir) {
-        this.gradleUserHomeDir = gradleUserHomeDir;
-    }
-
     @Override
     public void setImplementationClasspath(List<URL> implementationClassPath) {
         this.implementationClassPath = implementationClassPath;
@@ -197,6 +195,13 @@ public void enableJvmMemoryInfoPublishing(boolean shouldPublish) {
         this.shouldPublishJvmMemoryInfo = shouldPublish;
     }
 
+    @Deprecated
+    @Override
+    public WorkerProcessBuilder setUseLegacyAddOpens(boolean useLegacyAddOpens) {
+        this.useLegacyAddOpens = useLegacyAddOpens;
+        return this;
+    }
+
     @Override
     public WorkerProcess build() {
         final WorkerJvmMemoryStatus memoryStatus = shouldPublishJvmMemoryInfo ? new WorkerJvmMemoryStatus() : null;
@@ -234,7 +239,12 @@ public void run() {
         JavaExecHandleBuilder javaCommand = getJavaCommand();
         javaCommand.setDisplayName(displayName);
 
-        workerImplementationFactory.prepareJavaCommand(id, displayName, this, implementationClassPath, implementationModulePath, localAddress, javaCommand, shouldPublishJvmMemoryInfo);
+        boolean java9Compatible = jvmVersionDetector.getJavaVersion(javaCommand.getExecutable()).isJava9Compatible();
+        if (java9Compatible && useLegacyAddOpens) {
+            javaCommand.jvmArgs(JpmsConfiguration.GRADLE_WORKER_JPMS_ARGS);
+        }
+
+        workerImplementationFactory.prepareJavaCommand(id, displayName, this, implementationClassPath, implementationModulePath, localAddress, javaCommand, shouldPublishJvmMemoryInfo, java9Compatible);
 
         javaCommand.args("'" + displayName + "'");
         if (javaCommand.getMaxHeapSize() == null) {

subprojects/core/src/main/java/org/gradle/process/internal/worker/DefaultWorkerProcessFactory.java

@@ -36,8 +36,8 @@ public class DefaultWorkerProcessFactory implements WorkerProcessFactory {
     private final LoggingManager loggingManager;
     private final MessagingServer server;
     private final IdGenerator<Long> idGenerator;
-    private final File gradleUserHomeDir;
     private final JavaExecHandleFactory execHandleFactory;
+    private final JvmVersionDetector jvmVersionDetector;
     private final OutputEventListener outputEventListener;
     private final ApplicationClassesInSystemClassLoaderWorkerImplementationFactory workerImplementationFactory;
     private final MemoryManager memoryManager;
@@ -49,10 +49,10 @@ public DefaultWorkerProcessFactory(LoggingManager loggingManager, MessagingServe
         this.loggingManager = loggingManager;
         this.server = server;
         this.idGenerator = idGenerator;
-        this.gradleUserHomeDir = gradleUserHomeDir;
         this.execHandleFactory = execHandleFactory;
+        this.jvmVersionDetector = jvmVersionDetector;
         this.outputEventListener = outputEventListener;
-        this.workerImplementationFactory = new ApplicationClassesInSystemClassLoaderWorkerImplementationFactory(classPathRegistry, temporaryFileProvider, jvmVersionDetector, gradleUserHomeDir);
+        this.workerImplementationFactory = new ApplicationClassesInSystemClassLoaderWorkerImplementationFactory(classPathRegistry, temporaryFileProvider, gradleUserHomeDir);
         this.memoryManager = memoryManager;
     }
 
@@ -70,18 +70,17 @@ public WorkerProcessBuilder create(Action<? super WorkerProcessContext> workerAc
 
     @Override
     public <IN, OUT> SingleRequestWorkerProcessBuilder<IN, OUT> singleRequestWorker(Class<? extends RequestHandler<? super IN, ? extends OUT>> workerImplementation) {
-        return new DefaultSingleRequestWorkerProcessBuilder<IN, OUT>(workerImplementation, newWorkerProcessBuilder(), outputEventListener);
+        return new DefaultSingleRequestWorkerProcessBuilder<>(workerImplementation, newWorkerProcessBuilder(), outputEventListener);
     }
 
     @Override
     public <IN, OUT> MultiRequestWorkerProcessBuilder<IN, OUT> multiRequestWorker(Class<? extends RequestHandler<? super IN, ? extends OUT>> workerImplementation) {
-        return new DefaultMultiRequestWorkerProcessBuilder<IN, OUT>(workerImplementation, newWorkerProcessBuilder(), outputEventListener);
+        return new DefaultMultiRequestWorkerProcessBuilder<>(workerImplementation, newWorkerProcessBuilder(), outputEventListener);
     }
 
     private DefaultWorkerProcessBuilder newWorkerProcessBuilder() {
-        DefaultWorkerProcessBuilder builder = new DefaultWorkerProcessBuilder(execHandleFactory, server, idGenerator, workerImplementationFactory, outputEventListener, memoryManager);
+        DefaultWorkerProcessBuilder builder = new DefaultWorkerProcessBuilder(execHandleFactory, server, idGenerator, workerImplementationFactory, outputEventListener, memoryManager, jvmVersionDetector);
         builder.setLogLevel(loggingManager.getLevel());
-        builder.setGradleUserHomeDir(gradleUserHomeDir);
         builder.setConnectTimeoutSeconds(connectTimeoutSeconds);
         return builder;
     }

subprojects/core/src/main/java/org/gradle/process/internal/worker/WorkerProcessBuilder.java

@@ -59,6 +59,24 @@ public interface WorkerProcessBuilder extends WorkerProcessSettings {
 
     void enableJvmMemoryInfoPublishing(boolean shouldPublish);
 
+    /**
+     * Specify whether the {@code --add-opens} command line args specified by
+     * {@link org.gradle.internal.jvm.JpmsConfiguration#GRADLE_WORKER_JPMS_ARGS}
+     * should be used for the process-to-build.
+     * <p>
+     * Note: This option will be removed in Gradle 8.0 so that no workers use
+     * these args by default. We are leaving these args enabled by default for
+     * most worker types in order to reduce breaking changes for those using
+     * reflection already in their workers already. In 8.0, we will remove these
+     * args entirely from all workers.
+     *
+     * @see <a href="https://github.com/gradle/gradle/issues/21013">8.0 Removal Issue</a>
+     *
+     * @param useLegacyAddOpens Whether to use the add-opens args.
+     */
+    @Deprecated
+    WorkerProcessBuilder setUseLegacyAddOpens(boolean useLegacyAddOpens);
+
     /**
      * Creates the worker process. The process is not started until {@link WorkerProcess#start()} is called.
      *

subprojects/core/src/main/java/org/gradle/process/internal/worker/child/ApplicationClassesInSystemClassLoaderWorkerImplementationFactory.java

@@ -17,14 +17,11 @@
 package org.gradle.process.internal.worker.child;
 
 import com.google.common.base.Joiner;
-import org.gradle.api.JavaVersion;
 import org.gradle.api.UncheckedIOException;
 import org.gradle.api.internal.ClassPathRegistry;
 import org.gradle.api.internal.file.temp.TemporaryFileProvider;
 import org.gradle.api.logging.LogLevel;
 import org.gradle.internal.io.StreamByteBuffer;
-import org.gradle.internal.jvm.JpmsConfiguration;
-import org.gradle.internal.jvm.inspection.JvmVersionDetector;
 import org.gradle.internal.process.ArgWriter;
 import org.gradle.internal.remote.Address;
 import org.gradle.internal.remote.internal.inet.MultiChoiceAddress;
@@ -69,22 +66,25 @@
  *     (ActionExecutionWorker + worker action implementation)
  * </pre>
  */
-public class ApplicationClassesInSystemClassLoaderWorkerImplementationFactory implements WorkerImplementationFactory {
+public class ApplicationClassesInSystemClassLoaderWorkerImplementationFactory {
     private final ClassPathRegistry classPathRegistry;
     private final TemporaryFileProvider temporaryFileProvider;
-    private final JvmVersionDetector jvmVersionDetector;
     private final File gradleUserHomeDir;
 
-    public ApplicationClassesInSystemClassLoaderWorkerImplementationFactory(ClassPathRegistry classPathRegistry, TemporaryFileProvider temporaryFileProvider,
-                                                                            JvmVersionDetector jvmVersionDetector, File gradleUserHomeDir) {
+    public ApplicationClassesInSystemClassLoaderWorkerImplementationFactory(
+        ClassPathRegistry classPathRegistry,
+        TemporaryFileProvider temporaryFileProvider,
+        File gradleUserHomeDir
+    ) {
         this.classPathRegistry = classPathRegistry;
         this.temporaryFileProvider = temporaryFileProvider;
-        this.jvmVersionDetector = jvmVersionDetector;
         this.gradleUserHomeDir = gradleUserHomeDir;
     }
 
-    @Override
-    public void prepareJavaCommand(long workerId, String displayName, WorkerProcessBuilder processBuilder, List<URL> implementationClassPath, List<URL> implementationModulePath, Address serverAddress, JavaExecHandleBuilder execSpec, boolean publishProcessInfo) {
+    /**
+     * Configures the Java command that will be used to launch the child process.
+     */
+    public void prepareJavaCommand(long workerId, String displayName, WorkerProcessBuilder processBuilder, List<URL> implementationClassPath, List<URL> implementationModulePath, Address serverAddress, JavaExecHandleBuilder execSpec, boolean publishProcessInfo, boolean useOptionsFile) {
         Collection<File> applicationClasspath = processBuilder.getApplicationClasspath();
         Set<File> applicationModulePath = processBuilder.getApplicationModulePath();
         LogLevel logLevel = processBuilder.getLogLevel();
@@ -98,8 +98,6 @@ public void prepareJavaCommand(long workerId, String displayName, WorkerProcessB
             execSpec.getMainModule().set("gradle.worker");
         }
         execSpec.getMainClass().set("worker." + GradleWorkerMain.class.getName());
-
-        boolean useOptionsFile = shouldUseOptionsFile(execSpec);
         if (useOptionsFile) {
             // Use an options file to pass across application classpath
             File optionsFile = temporaryFileProvider.createTemporaryFile("gradle-worker-classpath", "txt");
@@ -170,11 +168,6 @@ public void prepareJavaCommand(long workerId, String displayName, WorkerProcessB
         execSpec.setStandardInput(buffer.getInputStream());
     }
 
-    private boolean shouldUseOptionsFile(JavaExecHandleBuilder execSpec) {
-        JavaVersion executableVersion = jvmVersionDetector.getJavaVersion(execSpec.getExecutable());
-        return executableVersion != null && executableVersion.isJava9Compatible();
-    }
-
     private List<String> writeOptionsFile(boolean runAsModule, Collection<File> workerMainClassPath, Collection<URL> implementationModulePath, Collection<File> applicationClasspath, Set<File> applicationModulePath, File optionsFile) {
         List<File> classpath = new ArrayList<>();
         List<File> modulePath = new ArrayList<>();
@@ -205,7 +198,6 @@ private List<String> writeOptionsFile(boolean runAsModule, Collection<File> work
         if (!classpath.isEmpty()) {
             argumentList.addAll(Arrays.asList("-cp", Joiner.on(File.pathSeparator).join(classpath)));
         }
-        argumentList.addAll(JpmsConfiguration.GRADLE_WORKER_JPMS_ARGS);
         return ArgWriter.argsFileGenerator(optionsFile, ArgWriter.javaStyleFactory()).transform(argumentList);
     }
 }