Skip to content

Commit

Permalink
Remove --add-opens for test workers
Browse files Browse the repository at this point in the history 
* 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 equivilent
* Verify users can opt back into previous behavior by adding --add-opens flags manually to test jvm args
* Improved multi-jdk support for TestNG Tests
* Add release notes and upgrade guide

Addressed action items
  • Loading branch information
@jvandort
jvandort committed Jun 22, 2022
1 parent 0ed952d commit 4b1028e
Show file tree
Hide file tree
Showing 37 changed files with 511 additions and 155 deletions.
32 changes: 24 additions & 8 deletions ...rojects/base-services/src/main/java/org/gradle/internal/classloader/ClassLoaderUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

/**
Expand All @@ -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.
Expand All @@ -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);
}
Expand Down Expand Up @@ -120,6 +123,7 @@ private interface ClassDefiner {
private interface ClassLoaderPackagesFetcher {
Package[] getPackages(ClassLoader classLoader);

@Nullable
Package getPackage(ClassLoader classLoader, String name);
}

Expand Down Expand Up @@ -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);
Expand All @@ -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);
}
}
}
18 changes: 18 additions & 0 deletions subprojects/core/src/integTest/groovy/org/gradle/api/ApplyPluginIntegSpec.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
16 changes: 16 additions & 0 deletions .../src/integTest/groovy/org/gradle/testfixtures/ProjectBuilderEndUserIntegrationTest.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
"""
}

Expand Down
32 changes: 21 additions & 11 deletions ...ts/core/src/main/java/org/gradle/process/internal/worker/DefaultWorkerProcessBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
13 changes: 6 additions & 7 deletions ...ts/core/src/main/java/org/gradle/process/internal/worker/DefaultWorkerProcessFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
}

Expand All @@ -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;
}
Expand Down
18 changes: 18 additions & 0 deletions subprojects/core/src/main/java/org/gradle/process/internal/worker/WorkerProcessBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down
28 changes: 10 additions & 18 deletions ...ternal/worker/child/ApplicationClassesInSystemClassLoaderWorkerImplementationFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand All @@ -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");
Expand Down Expand Up @@ -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<>();
Expand Down Expand Up @@ -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);
}
}

0 comments on commit 4b1028e

Please sign in to comment.