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.

Sign up for GitHub

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

Jump to bottom

Fix bug when gradle clean spotlessCheck was called on parallel build #501

Merged
merged 3 commits into from
Jan 1, 2020
Merged

Fix bug when gradle clean spotlessCheck was called on parallel build #501

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8c86815
Added SpotlessCache.clearOnce, and used it to fix #243
nedtwigg Dec 30, 2019
4091e3f
A better solution.
nedtwigg Dec 31, 2019
1243372
Update changelog.
nedtwigg Dec 31, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 28 additions & 1 deletion lib/src/main/java/com/diffplug/spotless/SpotlessCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
import java.util.Map;
import java.util.Objects;

import javax.annotation.Nullable;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;

/**
Expand Down Expand Up @@ -73,7 +75,12 @@ static SpotlessCache instance() {
return instance;
}

/** Closes all cached classloaders. */
/**
* Closes all cached classloaders.
*
* @deprecated because it is [dangerous](https://github.com/diffplug/spotless/issues/243#issuecomment-564323856), replacement is {@link #clearOnce(Object)}.
*/
@Deprecated
public static void clear() {
List<URLClassLoader> toDelete;
synchronized (instance) {
Expand All @@ -89,5 +96,25 @@ public static void clear() {
}
}

private static volatile Object lastClear;

/**
* Closes all cached classloaders iff `key` is not `.equals()` to the last call to `clearOnce()`.
* If `key` is null, the clear will always happen (as though null != null).
*/
public static boolean clearOnce(@Nullable Object key) {
synchronized (instance) {
if (key == null) {
lastClear = null;
} else if (key.equals(lastClear)) {
// only clear once
return false;
}
lastClear = key;
}
clear();
return true;
}

private static final SpotlessCache instance = new SpotlessCache();
}
3 changes: 2 additions & 1 deletion plugin-gradle/CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@

### Version 3.27.0-SNAPSHOT - TBD ([javadoc](https://diffplug.github.io/spotless/javadoc/snapshot/), [snapshot](https://oss.sonatype.org/content/repositories/snapshots/com/diffplug/spotless/spotless-plugin-gradle/))

* Added method `FormatExtension.createIndependentTask(String taskName)` which allows creating a Spotless task outside of the `check`/`apply` lifecycle. See [javadoc](https://github.com/diffplug/spotless/blob/91ed7203994e52058ea6c2e0f88d023ed290e433/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java#L613-L639) for details.
* Added method `FormatExtension.createIndependentTask(String taskName)` which allows creating a Spotless task outside of the `check`/`apply` lifecycle. See [javadoc](https://github.com/diffplug/spotless/blob/91ed7203994e52058ea6c2e0f88d023ed290e433/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java#L613-L639) for details. ([#500](https://github.com/diffplug/spotless/pull/500))
* Running clean and spotlessCheck during a parallel build could cause exceptions, fixed by ([#501](https://github.com/diffplug/spotless/pull/501)).

### Version 3.26.1 - November 27th 2019 ([javadoc](https://diffplug.github.io/spotless/javadoc/spotless-plugin-gradle/3.26.1/), [jcenter](https://bintray.com/diffplug/opensource/spotless-plugin-gradle/3.26.1))

Expand Down
9 changes: 8 additions & 1 deletion plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,14 @@ public void apply(Project project) {

// clear spotless' cache when the user does a clean
Task clean = project.getTasks().getByName(BasePlugin.CLEAN_TASK_NAME);
clean.doLast(unused -> SpotlessCache.clear());
clean.doLast(unused -> {
// resolution for: https://github.com/diffplug/spotless/issues/243#issuecomment-564323856
// project.getRootProject() is consistent across every project, so only of one the clears will
// actually happen (as desired)
//
// we use System.identityHashCode() to avoid a memory leak by hanging on to the reference directly
SpotlessCache.clearOnce(System.identityHashCode(project.getRootProject()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nedtwigg Reading the source code for SpotlessCache::clearOnce, I'm unclear on why passing project.getRootProject() to the method would cause the method to hang onto it; AFAICT the method never stores the reference inside a field or something similar. 🤔

So I was wondering if you could help me understand why the call to System::identityHashCode is needed here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spotless/lib/src/main/java/com/diffplug/spotless/SpotlessCache.java

Line 113 in f73660f

lastClear = key;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a better way to spell it, but I think the javadoc is the clearest explanation.

if (key == null || !key.equals(lastClear)) {
	lastClear = key;
} else {
	// only clear once
	return false;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, that lastClear = key line is what I'd missed! Thank you @nedtwigg. :)

});

project.afterEvaluate(unused -> {
// Add our check task as a dependency on the global check task
Expand Down