Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix memory leaks in gradle daemon #1198

Merged
merged 9 commits into from
May 10, 2022
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (
## [Unreleased]
### Fixed
* Update the `black` version regex to fix `19.10b0` and earlier. (fixes [#1195](https://github.com/diffplug/spotless/issues/1195), regression introduced in `2.25.0`)
* `GitAttributesLineEndings$RelocatablePolicy` and `FormatterStepImpl` now null-out their initialization lambdas after their state has been calculated, which allows GC to collect variables which were incidentally captured but not needed in the calculated state. ([#1198](https://github.com/diffplug/spotless/pull/1198))

## [2.25.2] - 2022-05-03
### Changes
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@
*/
package com.diffplug.spotless.extra;

import static com.diffplug.spotless.extra.LibExtraPreconditions.requireElementsNonNull;

import java.io.File;
import java.io.FileInputStream;
import java.io.IOException;
Expand Down Expand Up @@ -78,8 +76,8 @@ public static LineEnding.Policy create(File projectDir, Supplier<Iterable<File>>
static class RelocatablePolicy extends LazyForwardingEquality<CachedEndings> implements LineEnding.Policy {
private static final long serialVersionUID = 5868522122123693015L;

final transient File projectDir;
final transient Supplier<Iterable<File>> toFormat;
transient File projectDir;
transient Supplier<Iterable<File>> toFormat;

RelocatablePolicy(File projectDir, Supplier<Iterable<File>> toFormat) {
this.projectDir = Objects.requireNonNull(projectDir, "projectDir");
Expand All @@ -88,8 +86,13 @@ static class RelocatablePolicy extends LazyForwardingEquality<CachedEndings> imp

@Override
protected CachedEndings calculateState() throws Exception {
Runtime runtime = new RuntimeInit(projectDir, toFormat.get()).atRuntime();
return new CachedEndings(projectDir, runtime, toFormat.get());
Runtime runtime = new RuntimeInit(projectDir).atRuntime();
// LazyForwardingEquality guarantees that this will only be called once, and keeping toFormat
// causes a memory leak, see https://github.com/diffplug/spotless/issues/1194
CachedEndings state = new CachedEndings(projectDir, runtime, toFormat.get());
projectDir = null;
toFormat = null;
return state;
}

@Override
Expand Down Expand Up @@ -146,8 +149,7 @@ static class RuntimeInit {
final @Nullable File workTree;

@SuppressFBWarnings("SIC_INNER_SHOULD_BE_STATIC_ANON")
RuntimeInit(File projectDir, Iterable<File> toFormat) {
requireElementsNonNull(toFormat);
RuntimeInit(File projectDir) {
/////////////////////////////////
// USER AND SYSTEM-WIDE VALUES //
/////////////////////////////////
Expand Down
32 changes: 32 additions & 0 deletions lib/src/main/java/com/diffplug/spotless/DelegateFormatterStep.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* Copyright 2022 DiffPlug
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.diffplug.spotless;

import java.util.Objects;

/** Superclass of all compound FormatterSteps necessary for {@link com.diffplug.spotless.LazyForwardingEquality#unlazy(java.lang.Object)}. */
abstract class DelegateFormatterStep implements FormatterStep {
protected final FormatterStep delegateStep;

DelegateFormatterStep(FormatterStep delegateStep) {
this.delegateStep = Objects.requireNonNull(delegateStep);
}

@Override
public final String getName() {
return delegateStep.getName();
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2016-2021 DiffPlug
* Copyright 2016-2022 DiffPlug
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -22,27 +22,19 @@

import javax.annotation.Nullable;

final class FilterByContentPatternFormatterStep implements FormatterStep {
private final FormatterStep delegateStep;
final class FilterByContentPatternFormatterStep extends DelegateFormatterStep {
final Pattern contentPattern;

FilterByContentPatternFormatterStep(FormatterStep delegateStep, String contentPattern) {
this.delegateStep = Objects.requireNonNull(delegateStep);
super(delegateStep);
this.contentPattern = Pattern.compile(Objects.requireNonNull(contentPattern));
}

@Override
public String getName() {
return delegateStep.getName();
}

@Override
public @Nullable String format(String raw, File file) throws Exception {
Objects.requireNonNull(raw, "raw");
Objects.requireNonNull(file, "file");

Matcher matcher = contentPattern.matcher(raw);

if (matcher.find()) {
return delegateStep.format(raw, file);
} else {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2016 DiffPlug
* Copyright 2016-2022 DiffPlug
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -20,20 +20,14 @@

import javax.annotation.Nullable;

final class FilterByFileFormatterStep implements FormatterStep {
private final FormatterStep delegateStep;
final class FilterByFileFormatterStep extends DelegateFormatterStep {
private final SerializableFileFilter filter;

FilterByFileFormatterStep(FormatterStep delegateStep, SerializableFileFilter filter) {
this.delegateStep = Objects.requireNonNull(delegateStep);
super(delegateStep);
this.filter = Objects.requireNonNull(filter);
}

@Override
public String getName() {
return delegateStep.getName();
}

@Override
public @Nullable String format(String raw, File file) throws Exception {
Objects.requireNonNull(raw, "raw");
Expand Down
10 changes: 7 additions & 3 deletions lib/src/main/java/com/diffplug/spotless/FormatterStepImpl.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2016-2020 DiffPlug
* Copyright 2016-2022 DiffPlug
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -39,7 +39,7 @@ abstract class FormatterStepImpl<State extends Serializable> extends Strict<Stat
final transient String name;

/** Transient because only the state matters. */
final transient ThrowingEx.Supplier<State> stateSupplier;
transient ThrowingEx.Supplier<State> stateSupplier;

FormatterStepImpl(String name, ThrowingEx.Supplier<State> stateSupplier) {
this.name = Objects.requireNonNull(name);
Expand All @@ -53,7 +53,11 @@ public String getName() {

@Override
protected State calculateState() throws Exception {
return stateSupplier.get();
// LazyForwardingEquality guarantees that this will only be called once, and keeping toFormat
// causes a memory leak, see https://github.com/diffplug/spotless/issues/1194
State state = stateSupplier.get();
stateSupplier = null;
return state;
}

static final class Standard<State extends Serializable> extends FormatterStepImpl<State> {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2016 DiffPlug
* Copyright 2016-2022 DiffPlug
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -111,4 +111,18 @@ static byte[] toBytes(Serializable obj) {
}
return byteOutput.toByteArray();
}

/** Ensures that the lazy state has been evaluated. */
public static void unlazy(Object in) {
if (in instanceof LazyForwardingEquality) {
((LazyForwardingEquality<?>) in).state();
} else if (in instanceof DelegateFormatterStep) {
unlazy(((DelegateFormatterStep) in).delegateStep);
} else if (in instanceof Iterable) {
Iterable<Object> cast = (Iterable<Object>) in;
for (Object c : cast) {
unlazy(c);
}
}
}
}
3 changes: 3 additions & 0 deletions plugin-gradle/CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@
We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (starting after version `3.27.0`).

## [Unreleased]
### Added
* `FormatExtension.createIndependentApplyTaskLazy`, with same functionality as `createIndependentApplyTaskLazy` but returning `TaskProvider` ([#1198](https://github.com/diffplug/spotless/pull/1198))
### Fixed
* Update the `black` version regex to fix `19.10b0` and earlier. (fixes [#1195](https://github.com/diffplug/spotless/issues/1195), regression introduced in `6.5.0`)
* Improved daemon memory consumption ([#1198](https://github.com/diffplug/spotless/pull/1198) fixes [#1194](https://github.com/diffplug/spotless/issues/1194))

## [6.5.2] - 2022-05-03
### Changes
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.gradle.api.file.ConfigurableFileTree;
import org.gradle.api.file.FileCollection;
import org.gradle.api.plugins.BasePlugin;
import org.gradle.api.tasks.TaskProvider;

import com.diffplug.common.base.Preconditions;
import com.diffplug.spotless.FormatExceptionPolicyStrict;
Expand Down Expand Up @@ -769,6 +770,11 @@ protected Project getProject() {
return spotless.project;
}

/** Eager version of {@link #createIndependentApplyTaskLazy(String)} */
public SpotlessApply createIndependentApplyTask(String taskName) {
return createIndependentApplyTaskLazy(taskName).get();
}

/**
* Creates an independent {@link SpotlessApply} for (very) unusual circumstances.
*
Expand All @@ -782,19 +788,19 @@ protected Project getProject() {
*
* NOTE: does not respect the rarely-used <a href="https://github.com/diffplug/spotless/blob/b7f8c551a97dcb92cc4b0ee665448da5013b30a3/plugin-gradle/README.md#can-i-apply-spotless-to-specific-files">{@code spotlessFiles} property</a>.
*/
public SpotlessApply createIndependentApplyTask(String taskName) {
public TaskProvider<SpotlessApply> createIndependentApplyTaskLazy(String taskName) {
Preconditions.checkArgument(!taskName.endsWith(SpotlessExtension.APPLY), "Task name must not end with " + SpotlessExtension.APPLY);
// create and setup the task
SpotlessTaskImpl spotlessTask = spotless.project.getTasks().create(taskName + SpotlessTaskService.INDEPENDENT_HELPER, SpotlessTaskImpl.class);
spotlessTask.init(spotless.getRegisterDependenciesTask().getTaskService());
setupTask(spotlessTask);
// clean removes the SpotlessCache, so we have to run after clean
spotlessTask.mustRunAfter(BasePlugin.CLEAN_TASK_NAME);
TaskProvider<SpotlessTaskImpl> spotlessTask = spotless.project.getTasks().register(taskName + SpotlessTaskService.INDEPENDENT_HELPER, SpotlessTaskImpl.class, task -> {
task.init(spotless.getRegisterDependenciesTask().getTaskService());
setupTask(task);
// clean removes the SpotlessCache, so we have to run after clean
task.mustRunAfter(BasePlugin.CLEAN_TASK_NAME);
});
// create the apply task
SpotlessApply applyTask = spotless.project.getTasks().create(taskName, SpotlessApply.class);
applyTask.init(spotlessTask);
applyTask.dependsOn(spotlessTask);

TaskProvider<SpotlessApply> applyTask = spotless.project.getTasks().register(taskName, SpotlessApply.class, task -> {
task.dependsOn(spotlessTask);
task.init(spotlessTask.get());
});
return applyTask;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,12 +127,10 @@ private static Provisioner forConfigurationContainer(Project project, Configurat
if (!projName.isEmpty()) {
projName = projName + "/";
}
logger.error(
"You need to add a repository containing the '{}' artifact in '{}build.gradle'.\n" +
throw new GradleException(String.format(
"You need to add a repository containing the '%s' artifact in '%sbuild.gradle'.%n" +
"E.g.: 'repositories { mavenCentral() }'",
mavenCoords, projName,
e);
throw e;
mavenCoords, projName), e);
}
};
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2021 DiffPlug
* Copyright 2021-2022 DiffPlug
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -26,6 +26,7 @@
import org.gradle.api.Task;

import com.diffplug.spotless.FileSignature;
import com.diffplug.spotless.LazyForwardingEquality;

class JvmLocalCache {
private static GradleException cacheIsStale() {
Expand Down Expand Up @@ -53,6 +54,11 @@ static class LiveCacheKeyImpl<T> implements LiveCache<T>, Serializable {

@Override
public void set(T value) {
if (value instanceof LazyForwardingEquality) {
// whenever we cache an instance of LazyForwardingEquality, we want to make sure that we give it
// a chance to null-out its initialization lambda (see https://github.com/diffplug/spotless/issues/1194#issuecomment-1120744842)
LazyForwardingEquality.unlazy((LazyForwardingEquality<?>) value);
}
daemonState.put(internalKey, value);
}

Expand Down