From af2f778f2075fae3ba0f3e48f7ca4d19302aa6f0 Mon Sep 17 00:00:00 2001 From: Sterling Greene Date: Fri, 30 Aug 2019 16:47:28 -0400 Subject: [PATCH 1/6] Reproduce OOM with out-of-process worker API --- .../WorkerExecutorIntegrationTest.groovy | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/subprojects/workers/src/integTest/groovy/org/gradle/workers/internal/WorkerExecutorIntegrationTest.groovy b/subprojects/workers/src/integTest/groovy/org/gradle/workers/internal/WorkerExecutorIntegrationTest.groovy index 37f588696e7a..2ec3e6da5e81 100644 --- a/subprojects/workers/src/integTest/groovy/org/gradle/workers/internal/WorkerExecutorIntegrationTest.groovy +++ b/subprojects/workers/src/integTest/groovy/org/gradle/workers/internal/WorkerExecutorIntegrationTest.groovy @@ -190,6 +190,36 @@ class WorkerExecutorIntegrationTest extends AbstractWorkerExecutorIntegrationTes assertDifferentDaemonsWereUsed("runInDaemon", "startNewDaemon") } + @Issue("https://github.com/gradle/gradle/issues/10411") + def "does not leak project state across multiple builds"() { + fixture.withWorkActionClassInBuildSrc() + + buildFile << """ + ext.memoryHog = new byte[1024*1024*150] // ~100MB + + tasks.withType(WorkerTask) { task -> + isolationMode = IsolationMode.PROCESS + // Force a new daemon to be used + additionalForkOptions = { + it.systemProperty("foobar", task.name) + } + } + task startDaemon1(type: WorkerTask) + task startDaemon2(type: WorkerTask) + task startDaemon3(type: WorkerTask) + """ + + when: + succeeds("startDaemon1") + succeeds("startDaemon2") + succeeds("startDaemon3") + + then: + assertDifferentDaemonsWereUsed("startDaemon1", "startDaemon2") + assertDifferentDaemonsWereUsed("startDaemon2", "startDaemon3") + assertDifferentDaemonsWereUsed("startDaemon1", "startDaemon3") + } + def "starts a new worker daemon when there are no idle compatible worker daemons available"() { blockingServer.start() blockingServer.expectConcurrent("runInDaemon", "startNewDaemon") From 31932788936d6bfc5f1fa28bde40147c2a5d8d0c Mon Sep 17 00:00:00 2001 From: Sterling Greene Date: Thu, 29 Aug 2019 16:51:31 -0400 Subject: [PATCH 2/6] Use a non-decorated fork options when creating fork options for worker daemon WorkerDaemonClients are retained across builds, so any object associated with it must not contain project state since this will be kept around until the worker expires. - Introduce a newDecoratedJavaForkOptions which creates a decorated JavaForkOptionsInternal - Replace calls to the decorated version of the method with non-decorated versions where appropriate --- .../java/org/gradle/api/tasks/JavaExec.java | 8 +++++++- .../internal/DefaultExecActionFactory.java | 17 ++++++++++++----- .../process/internal/DefaultJavaExecAction.java | 5 +++-- .../process/internal/JavaExecHandleBuilder.java | 4 ++-- .../internal/JavaForkOptionsFactory.java | 1 + .../org/gradle/process/internal/JvmOptions.java | 4 ++-- .../internal/JavaExecHandleBuilderTest.groovy | 2 +- .../java/org/gradle/api/tasks/testing/Test.java | 2 +- .../internal/DaemonForkOptionsBuilder.java | 2 +- .../internal/DefaultProcessWorkerSpec.java | 4 ++-- .../internal/DefaultWorkerConfiguration.java | 6 +++--- .../workers/internal/DefaultWorkerExecutor.java | 4 ++-- .../DefaultWorkerConfigurationTest.groovy | 2 +- .../DefaultWorkerExecutorParallelTest.groovy | 9 +++++++-- .../internal/DefaultWorkerExecutorTest.groovy | 4 ++-- 15 files changed, 47 insertions(+), 27 deletions(-) diff --git a/subprojects/core/src/main/java/org/gradle/api/tasks/JavaExec.java b/subprojects/core/src/main/java/org/gradle/api/tasks/JavaExec.java index c1b252c90187..d99943d24c65 100644 --- a/subprojects/core/src/main/java/org/gradle/api/tasks/JavaExec.java +++ b/subprojects/core/src/main/java/org/gradle/api/tasks/JavaExec.java @@ -29,6 +29,7 @@ import org.gradle.process.JavaExecSpec; import org.gradle.process.JavaForkOptions; import org.gradle.process.ProcessForkOptions; +import org.gradle.process.internal.DslExecActionFactory; import org.gradle.process.internal.ExecActionFactory; import org.gradle.process.internal.JavaExecAction; @@ -100,7 +101,7 @@ public class JavaExec extends ConventionTask implements JavaExecSpec { private final JavaExecAction javaExecHandleBuilder; public JavaExec() { - javaExecHandleBuilder = getExecActionFactory().newJavaExecAction(); + javaExecHandleBuilder = getDslExecActionFactory().newDecoratedJavaExecAction(); } @Inject @@ -108,6 +109,11 @@ protected ExecActionFactory getExecActionFactory() { throw new UnsupportedOperationException(); } + @Inject + protected DslExecActionFactory getDslExecActionFactory() { + throw new UnsupportedOperationException(); + } + @TaskAction public void exec() { setMain(getMain()); // make convention mapping work (at least for 'main'... diff --git a/subprojects/core/src/main/java/org/gradle/process/internal/DefaultExecActionFactory.java b/subprojects/core/src/main/java/org/gradle/process/internal/DefaultExecActionFactory.java index 790d06191faf..dd3462a7f302 100644 --- a/subprojects/core/src/main/java/org/gradle/process/internal/DefaultExecActionFactory.java +++ b/subprojects/core/src/main/java/org/gradle/process/internal/DefaultExecActionFactory.java @@ -92,6 +92,11 @@ public ExecAction newExecAction() { return new DefaultExecAction(fileResolver, executor, buildCancellationToken); } + @Override + public JavaForkOptionsInternal newDecoratedJavaForkOptions() { + throw new UnsupportedOperationException(); + } + @Override public JavaForkOptionsInternal newJavaForkOptions() { return new DefaultJavaForkOptions(fileResolver, fileCollectionFactory, new DefaultJavaDebugOptions()); @@ -99,7 +104,9 @@ public JavaForkOptionsInternal newJavaForkOptions() { @Override public JavaForkOptionsInternal immutableCopy(JavaForkOptionsInternal options) { - JavaForkOptionsInternal copy = newJavaForkOptions(); + // NOTE: We do not want/need a decorated version of JavaForkOptions or JavaDebugOptions because + // these immutable instances are held across builds and will retain classloaders/services in the decorated object + JavaForkOptionsInternal copy = new DefaultJavaForkOptions(fileResolver, new DefaultFileCollectionFactory(fileResolver, null), new DefaultJavaDebugOptions()); options.copyTo(copy); return new ImmutableJavaForkOptions(copy); } @@ -111,7 +118,7 @@ public JavaExecAction newDecoratedJavaExecAction() { @Override public JavaExecAction newJavaExecAction() { - return new DefaultJavaExecAction(fileResolver, fileCollectionFactory, executor, buildCancellationToken, this); + return new DefaultJavaExecAction(fileResolver, fileCollectionFactory, executor, buildCancellationToken, newJavaForkOptions()); } @Override @@ -121,7 +128,7 @@ public ExecHandleBuilder newExec() { @Override public JavaExecHandleBuilder newJavaExec() { - return new JavaExecHandleBuilder(fileResolver, fileCollectionFactory, executor, buildCancellationToken, this); + return new JavaExecHandleBuilder(fileResolver, fileCollectionFactory, executor, buildCancellationToken, newJavaForkOptions()); } @Override @@ -166,11 +173,11 @@ public ExecAction newDecoratedExecAction() { @Override public JavaExecAction newDecoratedJavaExecAction() { - return instantiator.newInstance(DefaultJavaExecAction.class, fileResolver, fileCollectionFactory, executor, buildCancellationToken, this); + return instantiator.newInstance(DefaultJavaExecAction.class, fileResolver, fileCollectionFactory, executor, buildCancellationToken, newDecoratedJavaForkOptions()); } @Override - public JavaForkOptionsInternal newJavaForkOptions() { + public JavaForkOptionsInternal newDecoratedJavaForkOptions() { JavaDebugOptions javaDebugOptions = objectFactory.newInstance(DefaultJavaDebugOptions.class, objectFactory); return instantiator.newInstance(DefaultJavaForkOptions.class, fileResolver, fileCollectionFactory, javaDebugOptions); } diff --git a/subprojects/core/src/main/java/org/gradle/process/internal/DefaultJavaExecAction.java b/subprojects/core/src/main/java/org/gradle/process/internal/DefaultJavaExecAction.java index fac8e1332f65..3838b752b6bf 100644 --- a/subprojects/core/src/main/java/org/gradle/process/internal/DefaultJavaExecAction.java +++ b/subprojects/core/src/main/java/org/gradle/process/internal/DefaultJavaExecAction.java @@ -20,6 +20,7 @@ import org.gradle.api.internal.file.FileResolver; import org.gradle.initialization.BuildCancellationToken; import org.gradle.process.ExecResult; +import org.gradle.process.JavaForkOptions; import java.util.concurrent.Executor; @@ -27,8 +28,8 @@ * Use {@link ExecActionFactory} or {@link DslExecActionFactory} instead. */ public class DefaultJavaExecAction extends JavaExecHandleBuilder implements JavaExecAction { - public DefaultJavaExecAction(FileResolver fileResolver, FileCollectionFactory fileCollectionFactory, Executor executor, BuildCancellationToken buildCancellationToken, JavaForkOptionsFactory javaForkOptionsFactory) { - super(fileResolver, fileCollectionFactory, executor, buildCancellationToken, javaForkOptionsFactory); + public DefaultJavaExecAction(FileResolver fileResolver, FileCollectionFactory fileCollectionFactory, Executor executor, BuildCancellationToken buildCancellationToken, JavaForkOptions javaOptions) { + super(fileResolver, fileCollectionFactory, executor, buildCancellationToken, javaOptions); } @Override diff --git a/subprojects/core/src/main/java/org/gradle/process/internal/JavaExecHandleBuilder.java b/subprojects/core/src/main/java/org/gradle/process/internal/JavaExecHandleBuilder.java index 698db8760f5f..bf25a957278b 100644 --- a/subprojects/core/src/main/java/org/gradle/process/internal/JavaExecHandleBuilder.java +++ b/subprojects/core/src/main/java/org/gradle/process/internal/JavaExecHandleBuilder.java @@ -48,10 +48,10 @@ public class JavaExecHandleBuilder extends AbstractExecHandleBuilder implements private final JavaForkOptions javaOptions; private final List argumentProviders = new ArrayList(); - public JavaExecHandleBuilder(FileResolver fileResolver, FileCollectionFactory fileCollectionFactory, Executor executor, BuildCancellationToken buildCancellationToken, JavaForkOptionsFactory javaForkOptionsFactory) { + public JavaExecHandleBuilder(FileResolver fileResolver, FileCollectionFactory fileCollectionFactory, Executor executor, BuildCancellationToken buildCancellationToken, JavaForkOptions javaOptions) { super(fileResolver, executor, buildCancellationToken); this.fileCollectionFactory = fileCollectionFactory; - this.javaOptions = javaForkOptionsFactory.newJavaForkOptions(); + this.javaOptions = javaOptions; executable(javaOptions.getExecutable()); } diff --git a/subprojects/core/src/main/java/org/gradle/process/internal/JavaForkOptionsFactory.java b/subprojects/core/src/main/java/org/gradle/process/internal/JavaForkOptionsFactory.java index 43cc96b71c40..0f313ea94139 100644 --- a/subprojects/core/src/main/java/org/gradle/process/internal/JavaForkOptionsFactory.java +++ b/subprojects/core/src/main/java/org/gradle/process/internal/JavaForkOptionsFactory.java @@ -17,6 +17,7 @@ package org.gradle.process.internal; public interface JavaForkOptionsFactory { + JavaForkOptionsInternal newDecoratedJavaForkOptions(); JavaForkOptionsInternal newJavaForkOptions(); JavaForkOptionsInternal immutableCopy(JavaForkOptionsInternal options); diff --git a/subprojects/core/src/main/java/org/gradle/process/internal/JvmOptions.java b/subprojects/core/src/main/java/org/gradle/process/internal/JvmOptions.java index 26599be5b45e..2f03c2046083 100644 --- a/subprojects/core/src/main/java/org/gradle/process/internal/JvmOptions.java +++ b/subprojects/core/src/main/java/org/gradle/process/internal/JvmOptions.java @@ -322,7 +322,7 @@ public void copyTo(JavaForkOptions target) { target.setSystemProperties(mutableSystemProperties); target.setMinHeapSize(minHeapSize); target.setMaxHeapSize(maxHeapSize); - target.setBootstrapClasspath(getBootstrapClasspath()); + target.bootstrapClasspath(getBootstrapClasspath().getFiles()); target.setEnableAssertions(assertionsEnabled); copyDebugOptionsTo(target.getDebugOptions()); target.systemProperties(immutableSystemProperties); @@ -335,7 +335,7 @@ public JvmOptions createCopy() { target.setMinHeapSize(minHeapSize); target.setMaxHeapSize(maxHeapSize); if (bootstrapClasspath != null) { - target.setBootstrapClasspath(getBootstrapClasspath()); + target.setBootstrapClasspath(getBootstrapClasspath().getFiles()); } target.setEnableAssertions(assertionsEnabled); copyDebugOptionsTo(target.getDebugOptions()); diff --git a/subprojects/core/src/test/groovy/org/gradle/process/internal/JavaExecHandleBuilderTest.groovy b/subprojects/core/src/test/groovy/org/gradle/process/internal/JavaExecHandleBuilderTest.groovy index a43bfca4f409..d7569a1161f0 100644 --- a/subprojects/core/src/test/groovy/org/gradle/process/internal/JavaExecHandleBuilderTest.groovy +++ b/subprojects/core/src/test/groovy/org/gradle/process/internal/JavaExecHandleBuilderTest.groovy @@ -30,7 +30,7 @@ import java.util.concurrent.Executor import static java.util.Arrays.asList class JavaExecHandleBuilderTest extends Specification { - JavaExecHandleBuilder builder = new JavaExecHandleBuilder(TestFiles.resolver(), TestFiles.fileCollectionFactory(), Mock(Executor), new DefaultBuildCancellationToken(), TestFiles.execFactory()) + JavaExecHandleBuilder builder = new JavaExecHandleBuilder(TestFiles.resolver(), TestFiles.fileCollectionFactory(), Mock(Executor), new DefaultBuildCancellationToken(), TestFiles.execFactory().newJavaForkOptions()) FileCollectionFactory fileCollectionFactory = new DefaultFileCollectionFactory() diff --git a/subprojects/testing-jvm/src/main/java/org/gradle/api/tasks/testing/Test.java b/subprojects/testing-jvm/src/main/java/org/gradle/api/tasks/testing/Test.java index 1c66c3f7c191..840b68063e2e 100644 --- a/subprojects/testing-jvm/src/main/java/org/gradle/api/tasks/testing/Test.java +++ b/subprojects/testing-jvm/src/main/java/org/gradle/api/tasks/testing/Test.java @@ -152,7 +152,7 @@ public class Test extends AbstractTestTask implements JavaForkOptions, PatternFi public Test() { patternSet = getFileResolver().getPatternSetFactory().create(); - forkOptions = getForkOptionsFactory().newJavaForkOptions(); + forkOptions = getForkOptionsFactory().newDecoratedJavaForkOptions(); forkOptions.setEnableAssertions(true); } diff --git a/subprojects/workers/src/main/java/org/gradle/workers/internal/DaemonForkOptionsBuilder.java b/subprojects/workers/src/main/java/org/gradle/workers/internal/DaemonForkOptionsBuilder.java index b4cfb76e3fa2..0fc6ddbc8bea 100644 --- a/subprojects/workers/src/main/java/org/gradle/workers/internal/DaemonForkOptionsBuilder.java +++ b/subprojects/workers/src/main/java/org/gradle/workers/internal/DaemonForkOptionsBuilder.java @@ -28,7 +28,7 @@ public class DaemonForkOptionsBuilder { public DaemonForkOptionsBuilder(JavaForkOptionsFactory forkOptionsFactory) { this.forkOptionsFactory = forkOptionsFactory; - javaForkOptions = forkOptionsFactory.newJavaForkOptions(); + this.javaForkOptions = forkOptionsFactory.newJavaForkOptions(); } public DaemonForkOptionsBuilder keepAliveMode(KeepAliveMode keepAliveMode) { diff --git a/subprojects/workers/src/main/java/org/gradle/workers/internal/DefaultProcessWorkerSpec.java b/subprojects/workers/src/main/java/org/gradle/workers/internal/DefaultProcessWorkerSpec.java index efef6a5b626b..de61679b0f79 100644 --- a/subprojects/workers/src/main/java/org/gradle/workers/internal/DefaultProcessWorkerSpec.java +++ b/subprojects/workers/src/main/java/org/gradle/workers/internal/DefaultProcessWorkerSpec.java @@ -31,9 +31,9 @@ public class DefaultProcessWorkerSpec extends DefaultClassLoaderWorkerSpec imple protected final JavaForkOptions forkOptions; @Inject - public DefaultProcessWorkerSpec(JavaForkOptionsFactory javaForkOptionsFactory, ObjectFactory objectFactory) { + public DefaultProcessWorkerSpec(JavaForkOptions forkOptions, ObjectFactory objectFactory) { super(IsolationMode.PROCESS, objectFactory); - this.forkOptions = javaForkOptionsFactory.newJavaForkOptions(); + this.forkOptions = forkOptions; getForkOptions().setEnvironment(Maps.newHashMap()); } diff --git a/subprojects/workers/src/main/java/org/gradle/workers/internal/DefaultWorkerConfiguration.java b/subprojects/workers/src/main/java/org/gradle/workers/internal/DefaultWorkerConfiguration.java index bd9109c59f2c..9022aaa059c6 100644 --- a/subprojects/workers/src/main/java/org/gradle/workers/internal/DefaultWorkerConfiguration.java +++ b/subprojects/workers/src/main/java/org/gradle/workers/internal/DefaultWorkerConfiguration.java @@ -21,7 +21,7 @@ import org.gradle.api.ActionConfiguration; import org.gradle.api.internal.DefaultActionConfiguration; import org.gradle.process.JavaForkOptions; -import org.gradle.process.internal.JavaForkOptionsFactory; +import org.gradle.process.internal.JavaForkOptionsInternal; import org.gradle.util.GUtil; import org.gradle.workers.ClassLoaderWorkerSpec; import org.gradle.workers.ForkMode; @@ -40,8 +40,8 @@ public class DefaultWorkerConfiguration extends DefaultActionConfiguration imple private String displayName; private List classpath = Lists.newArrayList(); - public DefaultWorkerConfiguration(JavaForkOptionsFactory forkOptionsFactory) { - this.forkOptions = forkOptionsFactory.newJavaForkOptions(); + public DefaultWorkerConfiguration(JavaForkOptionsInternal forkOptions) { + this.forkOptions = forkOptions; } @Override diff --git a/subprojects/workers/src/main/java/org/gradle/workers/internal/DefaultWorkerExecutor.java b/subprojects/workers/src/main/java/org/gradle/workers/internal/DefaultWorkerExecutor.java index 5a6c1ceb22f6..3ffc05635b8e 100644 --- a/subprojects/workers/src/main/java/org/gradle/workers/internal/DefaultWorkerExecutor.java +++ b/subprojects/workers/src/main/java/org/gradle/workers/internal/DefaultWorkerExecutor.java @@ -124,7 +124,7 @@ public WorkQueue classLoaderIsolation(Action action) { @Override public WorkQueue processIsolation(Action action) { - DefaultProcessWorkerSpec spec = instantiator.newInstance(DefaultProcessWorkerSpec.class); + DefaultProcessWorkerSpec spec = instantiator.newInstance(DefaultProcessWorkerSpec.class, forkOptionsFactory.newDecoratedJavaForkOptions()); File defaultWorkingDir = spec.getForkOptions().getWorkingDir(); File workingDirectory = workerDirectoryProvider.getWorkingDirectory(); action.execute(spec); @@ -140,7 +140,7 @@ public WorkQueue processIsolation(Action action) { @Override public void submit(Class actionClass, Action configAction) { - DefaultWorkerConfiguration configuration = new DefaultWorkerConfiguration(forkOptionsFactory); + DefaultWorkerConfiguration configuration = new DefaultWorkerConfiguration(forkOptionsFactory.newDecoratedJavaForkOptions()); configAction.execute(configuration); Action parametersAction = new Action() { diff --git a/subprojects/workers/src/test/groovy/org/gradle/workers/internal/DefaultWorkerConfigurationTest.groovy b/subprojects/workers/src/test/groovy/org/gradle/workers/internal/DefaultWorkerConfigurationTest.groovy index ef2ce7255f54..5fc511c6cd92 100644 --- a/subprojects/workers/src/test/groovy/org/gradle/workers/internal/DefaultWorkerConfigurationTest.groovy +++ b/subprojects/workers/src/test/groovy/org/gradle/workers/internal/DefaultWorkerConfigurationTest.groovy @@ -23,7 +23,7 @@ import org.gradle.workers.WorkerConfiguration import spock.lang.Specification class DefaultWorkerConfigurationTest extends Specification { - WorkerConfiguration workerConfiguration = new DefaultWorkerConfiguration(TestFiles.execFactory()) + WorkerConfiguration workerConfiguration = new DefaultWorkerConfiguration(TestFiles.execFactory().newJavaForkOptions()) def "can accurately adapt to/from ForkMode"() { when: diff --git a/subprojects/workers/src/test/groovy/org/gradle/workers/internal/DefaultWorkerExecutorParallelTest.groovy b/subprojects/workers/src/test/groovy/org/gradle/workers/internal/DefaultWorkerExecutorParallelTest.groovy index d64f67875e73..e0d31f52fa9c 100644 --- a/subprojects/workers/src/test/groovy/org/gradle/workers/internal/DefaultWorkerExecutorParallelTest.groovy +++ b/subprojects/workers/src/test/groovy/org/gradle/workers/internal/DefaultWorkerExecutorParallelTest.groovy @@ -32,8 +32,8 @@ import org.gradle.process.internal.worker.child.WorkerDirectoryProvider import org.gradle.test.fixtures.concurrent.ConcurrentSpec import org.gradle.util.UsesNativeServices import org.gradle.workers.WorkAction -import org.gradle.workers.WorkerExecutionException import org.gradle.workers.WorkParameters +import org.gradle.workers.WorkerExecutionException import org.junit.Rule import org.junit.rules.TemporaryFolder import spock.lang.Unroll @@ -68,7 +68,7 @@ class DefaultWorkerExecutorParallelTest extends ConcurrentSpec { _ * executionQueueFactory.create() >> executionQueue _ * instantiator.newInstance(DefaultWorkerSpec) >> { args -> new DefaultWorkerSpec() } _ * instantiator.newInstance(DefaultClassLoaderWorkerSpec) >> { args -> new DefaultClassLoaderWorkerSpec(objectFactory) } - _ * instantiator.newInstance(DefaultProcessWorkerSpec) >> { args -> new DefaultProcessWorkerSpec(forkOptionsFactory, objectFactory) } + _ * instantiator.newInstance(DefaultProcessWorkerSpec, _) >> { args -> new DefaultProcessWorkerSpec(args[1][0], objectFactory) } _ * instantiator.newInstance(DefaultWorkerExecutor.DefaultWorkQueue, _, _) >> { args -> new DefaultWorkerExecutor.DefaultWorkQueue(args[1][0], args[1][1]) } workerExecutor = new DefaultWorkerExecutor(workerDaemonFactory, workerInProcessFactory, workerNoIsolationFactory, forkOptionsFactory, buildOperationWorkerRegistry, buildOperationExecutor, asyncWorkerTracker, workerDirectoryProvider, executionQueueFactory, classLoaderStructureProvider, actionExecutionSpecFactory, instantiator) _ * actionExecutionSpecFactory.newIsolatedSpec(_, _, _, _) >> Mock(IsolatedParametersActionExecutionSpec) @@ -139,6 +139,11 @@ class DefaultWorkerExecutorParallelTest extends ConcurrentSpec { this.delegate = delegate } + @Override + JavaForkOptionsInternal newDecoratedJavaForkOptions() { + return newJavaForkOptions() + } + @Override JavaForkOptionsInternal newJavaForkOptions() { def forkOptions = delegate.newJavaForkOptions() diff --git a/subprojects/workers/src/test/groovy/org/gradle/workers/internal/DefaultWorkerExecutorTest.groovy b/subprojects/workers/src/test/groovy/org/gradle/workers/internal/DefaultWorkerExecutorTest.groovy index 16a0b4e5b954..8032e6cccc4d 100644 --- a/subprojects/workers/src/test/groovy/org/gradle/workers/internal/DefaultWorkerExecutorTest.groovy +++ b/subprojects/workers/src/test/groovy/org/gradle/workers/internal/DefaultWorkerExecutorTest.groovy @@ -72,7 +72,7 @@ class DefaultWorkerExecutorTest extends Specification { _ * instantiator.newInstance(AdapterWorkParameters) >> parameters _ * instantiator.newInstance(DefaultWorkerSpec) >> { args -> new DefaultWorkerSpec() } _ * instantiator.newInstance(DefaultClassLoaderWorkerSpec) >> { args -> new DefaultClassLoaderWorkerSpec(objectFactory) } - _ * instantiator.newInstance(DefaultProcessWorkerSpec) >> { args -> new DefaultProcessWorkerSpec(forkOptionsFactory, objectFactory) } + _ * instantiator.newInstance(DefaultProcessWorkerSpec) >> { args -> new DefaultProcessWorkerSpec(forkOptionsFactory.newJavaForkOptions(), objectFactory) } _ * instantiator.newInstance(DefaultWorkerExecutor.DefaultWorkQueue, _, _) >> { args -> new DefaultWorkerExecutor.DefaultWorkQueue(args[1][0], args[1][1]) } workerExecutor = new DefaultWorkerExecutor(workerDaemonFactory, inProcessWorkerFactory, noIsolationWorkerFactory, forkOptionsFactory, buildOperationWorkerRegistry, buildOperationExecutor, asyncWorkTracker, workerDirectoryProvider, executionQueueFactory, classLoaderStructureProvider, actionExecutionSpecFactory, instantiator) _ * actionExecutionSpecFactory.newIsolatedSpec(_, _, _, _) >> Mock(IsolatedParametersActionExecutionSpec) @@ -80,7 +80,7 @@ class DefaultWorkerExecutorTest extends Specification { def "worker configuration fork property defaults to AUTO"() { given: - WorkerConfiguration configuration = new DefaultWorkerConfiguration(forkOptionsFactory) + WorkerConfiguration configuration = new DefaultWorkerConfiguration(forkOptionsFactory.newJavaForkOptions()) expect: configuration.isolationMode == IsolationMode.AUTO From d1c8ee67944c294f6214392f0e46eebd474bc3dc Mon Sep 17 00:00:00 2001 From: Sterling Greene Date: Sun, 1 Sep 2019 17:06:06 -0400 Subject: [PATCH 3/6] Add test for legacy worker API as well --- .../WorkerExecutorIntegrationTest.groovy | 2 +- ...kerExecutorLegacyApiIntegrationTest.groovy | 35 +++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/subprojects/workers/src/integTest/groovy/org/gradle/workers/internal/WorkerExecutorIntegrationTest.groovy b/subprojects/workers/src/integTest/groovy/org/gradle/workers/internal/WorkerExecutorIntegrationTest.groovy index 2ec3e6da5e81..4ed68534de0f 100644 --- a/subprojects/workers/src/integTest/groovy/org/gradle/workers/internal/WorkerExecutorIntegrationTest.groovy +++ b/subprojects/workers/src/integTest/groovy/org/gradle/workers/internal/WorkerExecutorIntegrationTest.groovy @@ -195,7 +195,7 @@ class WorkerExecutorIntegrationTest extends AbstractWorkerExecutorIntegrationTes fixture.withWorkActionClassInBuildSrc() buildFile << """ - ext.memoryHog = new byte[1024*1024*150] // ~100MB + ext.memoryHog = new byte[1024*1024*150] // ~150MB tasks.withType(WorkerTask) { task -> isolationMode = IsolationMode.PROCESS diff --git a/subprojects/workers/src/integTest/groovy/org/gradle/workers/internal/WorkerExecutorLegacyApiIntegrationTest.groovy b/subprojects/workers/src/integTest/groovy/org/gradle/workers/internal/WorkerExecutorLegacyApiIntegrationTest.groovy index 897066a788fe..e3aee7b8ed95 100644 --- a/subprojects/workers/src/integTest/groovy/org/gradle/workers/internal/WorkerExecutorLegacyApiIntegrationTest.groovy +++ b/subprojects/workers/src/integTest/groovy/org/gradle/workers/internal/WorkerExecutorLegacyApiIntegrationTest.groovy @@ -217,6 +217,41 @@ class WorkerExecutorLegacyApiIntegrationTest extends AbstractIntegrationSpec { isolationMode << ISOLATION_MODES } + @Issue("https://github.com/gradle/gradle/issues/10411") + def "does not leak project state across multiple builds"() { + buildFile << """ + ${legacyWorkerTypeAndTask} + + ext.memoryHog = new byte[1024*1024*150] // ~150MB + + tasks.withType(WorkerTask) { task -> + isolationMode = IsolationMode.PROCESS + displayName = "Test Work" + + text = "foo" + arrayOfThings = ["foo", "bar", "baz"] + listOfThings = ["foo", "bar", "baz"] + + // Force a new daemon to be used + workerConfiguration = { + forkOptions { options -> + options.with { + systemProperty("foobar", task.name) + } + } + } + } + task startDaemon1(type: WorkerTask) + task startDaemon2(type: WorkerTask) + task startDaemon3(type: WorkerTask) + """ + + expect: + succeeds("startDaemon1") + succeeds("startDaemon2") + succeeds("startDaemon3") + } + @Issue("https://github.com/gradle/gradle/issues/10323") def "can use a Properties object as a parameter"() { buildFile << """ From 56e7edf4213d2a18b8fe9e61c9e28d449d9f5364 Mon Sep 17 00:00:00 2001 From: Sterling Greene Date: Sun, 1 Sep 2019 17:31:32 -0400 Subject: [PATCH 4/6] Fix typo --- .../java/org/gradle/process/internal/JvmOptions.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/subprojects/core/src/main/java/org/gradle/process/internal/JvmOptions.java b/subprojects/core/src/main/java/org/gradle/process/internal/JvmOptions.java index 2f03c2046083..064046ac414f 100644 --- a/subprojects/core/src/main/java/org/gradle/process/internal/JvmOptions.java +++ b/subprojects/core/src/main/java/org/gradle/process/internal/JvmOptions.java @@ -251,10 +251,10 @@ public void systemProperty(String name, Object value) { } public FileCollection getBootstrapClasspath() { - return internalGetBootstrapCLasspath(); + return internalGetBootstrapClasspath(); } - private ConfigurableFileCollection internalGetBootstrapCLasspath() { + private ConfigurableFileCollection internalGetBootstrapClasspath() { if (bootstrapClasspath == null) { bootstrapClasspath = fileCollectionFactory.configurableFiles("bootstrap classpath"); } @@ -262,15 +262,15 @@ private ConfigurableFileCollection internalGetBootstrapCLasspath() { } public void setBootstrapClasspath(FileCollection classpath) { - internalGetBootstrapCLasspath().setFrom(classpath); + internalGetBootstrapClasspath().setFrom(classpath); } public void setBootstrapClasspath(Object... classpath) { - internalGetBootstrapCLasspath().setFrom(classpath); + internalGetBootstrapClasspath().setFrom(classpath); } public void bootstrapClasspath(Object... classpath) { - internalGetBootstrapCLasspath().from(classpath); + internalGetBootstrapClasspath().from(classpath); } public String getMinHeapSize() { From e0bb87b58a7f6e23e9e58e9293656ac5b3e52703 Mon Sep 17 00:00:00 2001 From: Sterling Greene Date: Sun, 1 Sep 2019 18:16:16 -0400 Subject: [PATCH 5/6] Fix tests --- .../api/internal/tasks/util/DefaultJavaForkOptionsTest.groovy | 2 +- .../org/gradle/workers/internal/DefaultProcessWorkerSpec.java | 1 - .../gradle/workers/internal/DefaultWorkerExecutorTest.groovy | 4 ++-- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/subprojects/core/src/test/groovy/org/gradle/api/internal/tasks/util/DefaultJavaForkOptionsTest.groovy b/subprojects/core/src/test/groovy/org/gradle/api/internal/tasks/util/DefaultJavaForkOptionsTest.groovy index b4d82fbdb2b0..e1fc3731f742 100755 --- a/subprojects/core/src/test/groovy/org/gradle/api/internal/tasks/util/DefaultJavaForkOptionsTest.groovy +++ b/subprojects/core/src/test/groovy/org/gradle/api/internal/tasks/util/DefaultJavaForkOptionsTest.groovy @@ -355,7 +355,7 @@ class DefaultJavaForkOptionsTest extends Specification { 1 * target.setSystemProperties([key: 12]) 1 * target.setMinHeapSize('64m') 1 * target.setMaxHeapSize('1g') - 1 * target.setBootstrapClasspath(options.bootstrapClasspath) + 1 * target.bootstrapClasspath(_) 1 * target.setEnableAssertions(false) 1 * target.getDebugOptions() >> new DefaultJavaDebugOptions() diff --git a/subprojects/workers/src/main/java/org/gradle/workers/internal/DefaultProcessWorkerSpec.java b/subprojects/workers/src/main/java/org/gradle/workers/internal/DefaultProcessWorkerSpec.java index de61679b0f79..3ce37690c01e 100644 --- a/subprojects/workers/src/main/java/org/gradle/workers/internal/DefaultProcessWorkerSpec.java +++ b/subprojects/workers/src/main/java/org/gradle/workers/internal/DefaultProcessWorkerSpec.java @@ -20,7 +20,6 @@ import org.gradle.api.Action; import org.gradle.api.model.ObjectFactory; import org.gradle.process.JavaForkOptions; -import org.gradle.process.internal.JavaForkOptionsFactory; import org.gradle.workers.ClassLoaderWorkerSpec; import org.gradle.workers.IsolationMode; import org.gradle.workers.ProcessWorkerSpec; diff --git a/subprojects/workers/src/test/groovy/org/gradle/workers/internal/DefaultWorkerExecutorTest.groovy b/subprojects/workers/src/test/groovy/org/gradle/workers/internal/DefaultWorkerExecutorTest.groovy index 8032e6cccc4d..5bfbf57f2768 100644 --- a/subprojects/workers/src/test/groovy/org/gradle/workers/internal/DefaultWorkerExecutorTest.groovy +++ b/subprojects/workers/src/test/groovy/org/gradle/workers/internal/DefaultWorkerExecutorTest.groovy @@ -72,7 +72,7 @@ class DefaultWorkerExecutorTest extends Specification { _ * instantiator.newInstance(AdapterWorkParameters) >> parameters _ * instantiator.newInstance(DefaultWorkerSpec) >> { args -> new DefaultWorkerSpec() } _ * instantiator.newInstance(DefaultClassLoaderWorkerSpec) >> { args -> new DefaultClassLoaderWorkerSpec(objectFactory) } - _ * instantiator.newInstance(DefaultProcessWorkerSpec) >> { args -> new DefaultProcessWorkerSpec(forkOptionsFactory.newJavaForkOptions(), objectFactory) } + _ * instantiator.newInstance(DefaultProcessWorkerSpec, _) >> { args -> new DefaultProcessWorkerSpec(args[1][0], objectFactory) } _ * instantiator.newInstance(DefaultWorkerExecutor.DefaultWorkQueue, _, _) >> { args -> new DefaultWorkerExecutor.DefaultWorkQueue(args[1][0], args[1][1]) } workerExecutor = new DefaultWorkerExecutor(workerDaemonFactory, inProcessWorkerFactory, noIsolationWorkerFactory, forkOptionsFactory, buildOperationWorkerRegistry, buildOperationExecutor, asyncWorkTracker, workerDirectoryProvider, executionQueueFactory, classLoaderStructureProvider, actionExecutionSpecFactory, instantiator) _ * actionExecutionSpecFactory.newIsolatedSpec(_, _, _, _) >> Mock(IsolatedParametersActionExecutionSpec) @@ -111,7 +111,7 @@ class DefaultWorkerExecutorTest extends Specification { } def "can convert javaForkOptions to daemonForkOptions"() { - WorkerSpec configuration = new DefaultProcessWorkerSpec(forkOptionsFactory, objectFactory) + WorkerSpec configuration = new DefaultProcessWorkerSpec(forkOptionsFactory.newJavaForkOptions(), objectFactory) given: configuration.forkOptions { options -> From db0625bcad4a2834ebcc8e4b71582e3506809ad4 Mon Sep 17 00:00:00 2001 From: Sterling Greene Date: Tue, 3 Sep 2019 16:34:06 -0400 Subject: [PATCH 6/6] Make sure the memory leak tests for workers use separate daemons --- .../workers/internal/WorkerExecutorIntegrationTest.groovy | 1 + .../internal/WorkerExecutorLegacyApiIntegrationTest.groovy | 2 ++ 2 files changed, 3 insertions(+) diff --git a/subprojects/workers/src/integTest/groovy/org/gradle/workers/internal/WorkerExecutorIntegrationTest.groovy b/subprojects/workers/src/integTest/groovy/org/gradle/workers/internal/WorkerExecutorIntegrationTest.groovy index 4ed68534de0f..0c3ab0e8a895 100644 --- a/subprojects/workers/src/integTest/groovy/org/gradle/workers/internal/WorkerExecutorIntegrationTest.groovy +++ b/subprojects/workers/src/integTest/groovy/org/gradle/workers/internal/WorkerExecutorIntegrationTest.groovy @@ -193,6 +193,7 @@ class WorkerExecutorIntegrationTest extends AbstractWorkerExecutorIntegrationTes @Issue("https://github.com/gradle/gradle/issues/10411") def "does not leak project state across multiple builds"() { fixture.withWorkActionClassInBuildSrc() + executer.withBuildJvmOpts('-Xms256m', '-Xmx512m').requireIsolatedDaemons().requireDaemon() buildFile << """ ext.memoryHog = new byte[1024*1024*150] // ~150MB diff --git a/subprojects/workers/src/integTest/groovy/org/gradle/workers/internal/WorkerExecutorLegacyApiIntegrationTest.groovy b/subprojects/workers/src/integTest/groovy/org/gradle/workers/internal/WorkerExecutorLegacyApiIntegrationTest.groovy index e3aee7b8ed95..2c61c241cdc7 100644 --- a/subprojects/workers/src/integTest/groovy/org/gradle/workers/internal/WorkerExecutorLegacyApiIntegrationTest.groovy +++ b/subprojects/workers/src/integTest/groovy/org/gradle/workers/internal/WorkerExecutorLegacyApiIntegrationTest.groovy @@ -219,6 +219,8 @@ class WorkerExecutorLegacyApiIntegrationTest extends AbstractIntegrationSpec { @Issue("https://github.com/gradle/gradle/issues/10411") def "does not leak project state across multiple builds"() { + executer.withBuildJvmOpts('-Xms256m', '-Xmx512m').requireIsolatedDaemons().requireDaemon() + buildFile << """ ${legacyWorkerTypeAndTask}