Merge pull request #571 from lutovich/fix-mvn-resource-usage

Fix resource leak in Maven plugin

Author: nedtwigg

CHANGES.md

@@ -14,6 +14,10 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (
 * `LineEnding.GIT_ATTRIBUTES` now creates a policy whose serialized state can be relocated from one machine to another.  No user-visible change, but paves the way for remote build cache support in Gradle. ([#621](https://github.com/diffplug/spotless/pull/621))
 ### Added
 * `prettier` will now autodetect the parser (and formatter) to use based on the filename, unless you override this using `config` or `configFile` with the option `parser` or `filepath`. ([#620](https://github.com/diffplug/spotless/pull/620))
+### Changed
+* **BREAKING** `FileSignature` can no longer sign folders, only files.  Signatures are now based only on filename (not path), size, and a content hash.  It throws an error if a signature is attempted on a folder or on multiple files with different paths but the same filename - it never breaks silently.  This change does not break any of Spotless' internal logic, so it is unlikely to affect any of Spotless' consumers either. ([#571](https://github.com/diffplug/spotless/pull/571))
+  * This change allows the maven plugin to cache classloaders across subprojects when loading config resources from the classpath (fixes [#559](https://github.com/diffplug/spotless/issues/559)).
+  * This change also allows the gradle plugin to work with the remote buildcache (fixes [#280](https://github.com/diffplug/spotless/issues/280)).
 
 ## [1.34.1] - 2020-06-17
 ### Changed
@@ -162,7 +166,7 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (
   * Updated default npm package version of `prettier` from 1.13.4 to 1.16.4
   * Updated default npm package version of internally used typescript package from 2.9.2 to 3.3.3 and tslint package from 5.1.0 to 5.12.0 (both used by `tsfmt`)
 * Updated default eclipse-wtp from 4.7.3a to 4.7.3b ([#371](https://github.com/diffplug/spotless/pull/371)).
-* Configured `buìld-scan` plugin in build ([#356](https://github.com/diffplug/spotless/pull/356)).
+* Configured `build-scan` plugin in build ([#356](https://github.com/diffplug/spotless/pull/356)).
   * Runs on every CI build automatically.
   * Users need to opt-in on their local machine.
 * Default behavior of XML formatter changed to ignore external URIs ([#369](https://github.com/diffplug/spotless/issues/369)).

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

@@ -17,20 +17,26 @@
 
 import static com.diffplug.spotless.MoreIterables.toNullHostileList;
 import static com.diffplug.spotless.MoreIterables.toSortedSet;
+import static java.util.Comparator.comparing;
 
 import java.io.File;
+import java.io.FileInputStream;
 import java.io.IOException;
+import java.io.InputStream;
 import java.io.Serializable;
+import java.security.MessageDigest;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 
 /** Computes a signature for any needed files. */
 public final class FileSignature implements Serializable {
-	private static final long serialVersionUID = 1L;
+	private static final long serialVersionUID = 2L;
 
 	/*
 	 * Transient because not needed to uniquely identify a FileSignature instance, and also because
@@ -39,10 +45,7 @@ public final class FileSignature implements Serializable {
 	 */
 	@SuppressFBWarnings("SE_TRANSIENT_FIELD_NOT_RESTORED")
 	private final transient List<File> files;
-
-	private final String[] filenames;
-	private final long[] filesizes;
-	private final long[] lastModified;
+	private final Sig[] signatures;
 
 	/** Method has been renamed to {@link FileSignature#signAsSet}.
 	 * In case no sorting and removal of duplicates is required,
@@ -77,21 +80,30 @@ public static FileSignature signAsSet(File... files) throws IOException {
 
 	/** Creates file signature insensitive to the order of the files. */
 	public static FileSignature signAsSet(Iterable<File> files) throws IOException {
-		return new FileSignature(toSortedSet(files));
+		List<File> natural = toSortedSet(files);
+		List<File> onNameOnly = toSortedSet(files, comparing(File::getName));
+		if (natural.size() != onNameOnly.size()) {
+			StringBuilder builder = new StringBuilder();
+			builder.append("For these files:\n");
+			for (File file : files) {
+				builder.append("  " + file.getAbsolutePath() + "\n");
+			}
+			builder.append("a caching signature is being generated, which will be based only on their\n");
+			builder.append("names, not their full path (foo.txt, not C:\folder\foo.txt). Unexpectedly,\n");
+			builder.append("you have two files with different paths, but the same names.  You must\n");
+			builder.append("rename one of them so that all files have unique names.");
+			throw new IllegalArgumentException(builder.toString());
+		}
+		return new FileSignature(onNameOnly);
 	}
 
 	private FileSignature(final List<File> files) throws IOException {
-		this.files = files;
-
-		filenames = new String[this.files.size()];
-		filesizes = new long[this.files.size()];
-		lastModified = new long[this.files.size()];
+		this.files = validateInputFiles(files);
+		this.signatures = new Sig[this.files.size()];
 
 		int i = 0;
 		for (File file : this.files) {
-			filenames[i] = file.getCanonicalPath();
-			filesizes[i] = file.length();
-			lastModified[i] = file.lastModified();
+			signatures[i] = cache.sign(file);
 			++i;
 		}
 	}
@@ -120,6 +132,77 @@ public static String pathUnixToNative(String pathUnix) {
 		return LineEnding.nativeIsWin() ? pathUnix.replace('/', '\\') : pathUnix;
 	}
 
+	private static List<File> validateInputFiles(List<File> files) {
+		for (File file : files) {
+			if (!file.isFile()) {
+				throw new IllegalArgumentException(
+						"File signature can only be created for existing regular files, given: "
+								+ file);
+			}
+		}
+		return files;
+	}
+
+	/**
+	 * It is very common for a given set of files to be "signed" many times.  For example,
+	 * the jars which constitute any given formatter live in a central cache, but will be signed
+	 * over and over.  To save this I/O, we maintain a cache, invalidated by lastModified time.
+	 */
+	static final Cache cache = new Cache();
+
+	private static final class Cache {
+		Map<String, Sig> cache = new HashMap<>();
+
+		synchronized Sig sign(File fileInput) throws IOException {
+			String canonicalPath = fileInput.getCanonicalPath();
+			Sig sig = cache.computeIfAbsent(canonicalPath, ThrowingEx.<String, Sig> wrap(p -> {
+				MessageDigest digest = MessageDigest.getInstance("SHA-256");
+				File file = new File(p);
+				// calculate the size and content hash of the file
+				long size = 0;
+				byte[] buf = new byte[1024];
+				long lastModified;
+				try (InputStream input = new FileInputStream(file)) {
+					lastModified = file.lastModified();
+					int numRead;
+					while ((numRead = input.read(buf)) != -1) {
+						size += numRead;
+						digest.update(buf, 0, numRead);
+					}
+				}
+				return new Sig(file.getName(), size, digest.digest(), lastModified);
+			}));
+			long lastModified = fileInput.lastModified();
+			if (sig.lastModified != lastModified) {
+				cache.remove(canonicalPath);
+				return sign(fileInput);
+			} else {
+				return sig;
+			}
+		}
+	}
+
+	@SuppressFBWarnings("SE_TRANSIENT_FIELD_NOT_RESTORED")
+	private static final class Sig implements Serializable {
+		private static final long serialVersionUID = 6727302747168655222L;
+
+		@SuppressWarnings("unused")
+		final String name;
+		@SuppressWarnings("unused")
+		final long size;
+		@SuppressWarnings("unused")
+		final byte[] hash;
+		/** transient because state should be transferable from machine to machine. */
+		final transient long lastModified;
+
+		Sig(String name, long size, byte[] hash, long lastModified) {
+			this.name = name;
+			this.size = size;
+			this.hash = hash;
+			this.lastModified = lastModified;
+		}
+	}
+
 	/** Asserts that child is a subpath of root. and returns the subpath. */
 	public static String subpath(String root, String child) {
 		if (child.startsWith(root)) {

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2016 DiffPlug
+ * Copyright 2016-2020 DiffPlug
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -20,6 +20,7 @@
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.Iterator;
 import java.util.List;
 
@@ -37,18 +38,23 @@ static <T> List<T> toNullHostileList(Iterable<T> input) {
 		return shallowCopy;
 	}
 
-	/** Sorts "raw" and removes duplicates, throwing on null elements. */
+	/** Sorts "raw" using {@link Comparator#naturalOrder()} and removes duplicates, throwing on null elements. */
 	static <T extends Comparable<T>> List<T> toSortedSet(Iterable<T> raw) {
+		return toSortedSet(raw, Comparator.naturalOrder());
+	}
+
+	/** Sorts "raw" and removes duplicates, throwing on null elements. */
+	static <T> List<T> toSortedSet(Iterable<T> raw, Comparator<T> comparator) {
 		List<T> toBeSorted = toNullHostileList(raw);
 		// sort it
-		Collections.sort(toBeSorted);
+		Collections.sort(toBeSorted, comparator);
 		// remove any duplicates (normally there won't be any)
 		if (toBeSorted.size() > 1) {
 			Iterator<T> iter = toBeSorted.iterator();
 			T last = iter.next();
 			while (iter.hasNext()) {
 				T next = iter.next();
-				if (next.compareTo(last) == 0) {
+				if (comparator.compare(next, last) == 0) {
 					iter.remove();
 				} else {
 					last = next;

lib/src/main/java/com/diffplug/spotless/npm/NodeServerLayout.java

@@ -0,0 +1,43 @@
+/*
+ * Copyright 2020 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.npm;
+
+import java.io.File;
+
+class NodeServerLayout {
+
+	private final File nodeModulesDir;
+	private final File packageJsonFile;
+	private final File serveJsFile;
+
+	NodeServerLayout(File buildDir, String stepName) {
+		this.nodeModulesDir = new File(buildDir, "spotless-node-modules-" + stepName);
+		this.packageJsonFile = new File(nodeModulesDir, "package.json");
+		this.serveJsFile = new File(nodeModulesDir, "serve.js");
+	}
+
+	File nodeModulesDir() {
+		return nodeModulesDir;
+	}
+
+	File packageJsonFile() {
+		return packageJsonFile;
+	}
+
+	File serveJsFile() {
+		return serveJsFile;
+	}
+}

lib/src/main/java/com/diffplug/spotless/npm/NpmFormatterStepStateBase.java

@@ -44,7 +44,7 @@ abstract class NpmFormatterStepStateBase implements Serializable {
 	private static final long serialVersionUID = 1460749955865959948L;
 
 	@SuppressWarnings("unused")
-	private final FileSignature nodeModulesSignature;
+	private final FileSignature packageJsonSignature;
 
 	@SuppressFBWarnings("SE_TRANSIENT_FIELD_NOT_RESTORED")
 	public final transient File nodeModulesDir;
@@ -56,22 +56,26 @@ abstract class NpmFormatterStepStateBase implements Serializable {
 
 	private final String stepName;
 
-	protected NpmFormatterStepStateBase(String stepName, NpmConfig npmConfig, File buildDir, @Nullable File npm) throws IOException {
+	protected NpmFormatterStepStateBase(String stepName, NpmConfig npmConfig, File buildDir,
+			@Nullable File npm) throws IOException {
 		this.stepName = requireNonNull(stepName);
 		this.npmConfig = requireNonNull(npmConfig);
 		this.npmExecutable = resolveNpm(npm);
 
-		this.nodeModulesDir = prepareNodeServer(buildDir);
-		this.nodeModulesSignature = FileSignature.signAsList(this.nodeModulesDir);
+		NodeServerLayout layout = prepareNodeServer(buildDir);
+		this.nodeModulesDir = layout.nodeModulesDir();
+		this.packageJsonSignature = FileSignature.signAsList(layout.packageJsonFile());
 	}
 
-	private File prepareNodeServer(File buildDir) throws IOException {
-		File targetDir = new File(buildDir, "spotless-node-modules-" + stepName);
-		NpmResourceHelper.assertDirectoryExists(targetDir);
-		NpmResourceHelper.writeUtf8StringToFile(targetDir, "package.json", this.npmConfig.getPackageJsonContent());
-		NpmResourceHelper.writeUtf8StringToFile(targetDir, "serve.js", this.npmConfig.getServeScriptContent());
-		runNpmInstall(targetDir);
-		return targetDir;
+	private NodeServerLayout prepareNodeServer(File buildDir) throws IOException {
+		NodeServerLayout layout = new NodeServerLayout(buildDir, stepName);
+		NpmResourceHelper.assertDirectoryExists(layout.nodeModulesDir());
+		NpmResourceHelper.writeUtf8StringToFile(layout.packageJsonFile(),
+				this.npmConfig.getPackageJsonContent());
+		NpmResourceHelper
+				.writeUtf8StringToFile(layout.serveJsFile(), this.npmConfig.getServeScriptContent());
+		runNpmInstall(layout.nodeModulesDir());
+		return layout;
 	}
 
 	private void runNpmInstall(File npmProjectDir) throws IOException {

lib/src/main/java/com/diffplug/spotless/npm/NpmResourceHelper.java

@@ -28,9 +28,8 @@ private NpmResourceHelper() {
 		// no instance required
 	}
 
-	static void writeUtf8StringToFile(File targetDir, String fileName, String stringToWrite) throws IOException {
-		File packageJsonFile = new File(targetDir, fileName);
-		Files.write(packageJsonFile.toPath(), stringToWrite.getBytes(StandardCharsets.UTF_8));
+	static void writeUtf8StringToFile(File file, String stringToWrite) throws IOException {
+		Files.write(file.toPath(), stringToWrite.getBytes(StandardCharsets.UTF_8));
 	}
 
 	static void writeUtf8StringToOutputStream(String stringToWrite, OutputStream outputStream) throws IOException {

plugin-gradle/CHANGES.md

@@ -6,6 +6,7 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (
 
 ## [Unreleased]
 ### Added
+* Full support for the Gradle buildcache - previously only supported local, now supports remote too. Fixes [#566](https://github.com/diffplug/spotless/issues/566) and [#280](https://github.com/diffplug/spotless/issues/280), via changes in [#621](https://github.com/diffplug/spotless/pull/621) and [#571](https://github.com/diffplug/spotless/pull/571).
 * `prettier` will now autodetect the parser (and formatter) to use based on the filename, unless you override this using `config()` or `configFile()` with the option `parser` or `filepath`. ([#620](https://github.com/diffplug/spotless/pull/620))
 ### Fixed
 * LineEndings.GIT_ATTRIBUTES is now a bit more efficient, and paves the way for remote build cache support in Gradle. ([#621](https://github.com/diffplug/spotless/pull/621))

plugin-maven/CHANGES.md

@@ -4,7 +4,8 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (
 
 ## [Unreleased]
 ### Added
-* `prettier` will now autodetect the parser (and formatter) to use based on the filename, unless you override this using `config` or `configFile` with the option `parser` or `filepath`. ([#620](https://github.com/diffplug/spotless/pull/620))
+* `prettier` will now autodetect the parser (and formatter) to use based on the filename, unless you override this using `config` or `configFile` with the option `parser` or `filepath` ([#620](https://github.com/diffplug/spotless/pull/620)).
+* Huge speed improvement for multi-module projects thanks to improved cross-project classloader caching ([#571](https://github.com/diffplug/spotless/pull/571), fixes [#559](https://github.com/diffplug/spotless/issues/559)).
 
 ## [1.31.3] - 2020-06-17
 ### Changed

plugin-maven/src/main/java/com/diffplug/spotless/maven/FileLocator.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2016 DiffPlug
+ * Copyright 2016-2020 DiffPlug
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -16,10 +16,13 @@
 package com.diffplug.spotless.maven;
 
 import static com.diffplug.common.base.Strings.isNullOrEmpty;
+import static java.nio.charset.StandardCharsets.UTF_8;
 
 import java.io.File;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
+import java.util.Base64;
 import java.util.Objects;
-import java.util.UUID;
 
 import org.codehaus.plexus.resource.ResourceManager;
 import org.codehaus.plexus.resource.loader.FileResourceCreationException;
@@ -63,16 +66,29 @@ public File locateFile(String path) {
 		}
 	}
 
-	private static String tmpOutputFileName(String path) {
-		String extension = FileUtils.extension(path);
-		return TMP_RESOURCE_FILE_PREFIX + UUID.randomUUID() + '.' + extension;
-	}
-
 	public File getBaseDir() {
 		return baseDir;
 	}
 
 	public File getBuildDir() {
 		return buildDir;
 	}
+
+	private static String tmpOutputFileName(String path) {
+		String extension = FileUtils.extension(path);
+		byte[] pathHash = hash(path);
+		String pathBase64 = Base64.getEncoder().encodeToString(pathHash);
+		return TMP_RESOURCE_FILE_PREFIX + pathBase64 + '.' + extension;
+	}
+
+	private static byte[] hash(String value) {
+		MessageDigest messageDigest;
+		try {
+			messageDigest = MessageDigest.getInstance("SHA-256");
+		} catch (NoSuchAlgorithmException e) {
+			throw new IllegalStateException("SHA-256 digest algorithm not available", e);
+		}
+		messageDigest.update(value.getBytes(UTF_8));
+		return messageDigest.digest();
+	}
 }