Skip to content

Commit

Permalink
Fix memory leaks in gradle daemon (#1198)
Browse files Browse the repository at this point in the history
  • Loading branch information
nedtwigg authored May 10, 2022
2 parents 2e9b09c + 628c5fc commit 49099f3
Show file tree
Hide file tree
Showing 11 changed files with 101 additions and 49 deletions.
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

0 comments on commit 49099f3

Please sign in to comment.