Skip to content

Commit

Permalink
Fix resource leak in Maven plugin
Browse files Browse the repository at this point in the history
The resource leak increased the number of threads with every execution of
Exclipse-based formatter step in a Maven module. It happened because the
`SpotlessCache` wasn't properly utilized. Every cache key was based on
randomized file names because Maven plugin relocates config files into
the target directory with a random file name. This resulted in all cache
accesses being cache misses.

This commit fixes the problem by:

 * making the Maven plugin use non-random file names for output files.
   `FileLocator` is changed to construct names based on a hash of the
   input path. Input path can be a URL, file in a JAR, or a regular
   local file

 * changing `FileSignature` (used for `SpotlessCache` keys) to use filenames
   instead of paths and file hashes instead of last modified timestamps

These two changes allow `FileSignature` to uniquely identify a set of files
by their content and not take file paths into account. As a result, created
keys for `SpotlessCache` are properly comparable which results in lots of
cache hits and decreased number of created threads.

Changed `FileCignature` only accepts files, not directories. NPM formatters
changed to have their signatures based on package.json file instead of the
whole node_modules directory.
  • Loading branch information
lutovich committed Jun 27, 2020
1 parent 5e40de5 commit 52a7a6c
Show file tree
Hide file tree
Showing 7 changed files with 153 additions and 33 deletions.
39 changes: 33 additions & 6 deletions lib/src/main/java/com/diffplug/spotless/FileSignature.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,14 @@

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.IOException;
import java.io.Serializable;
import java.nio.file.Files;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
Expand All @@ -42,7 +46,7 @@ public final class FileSignature implements Serializable {

private final String[] filenames;
private final long[] filesizes;
private final long[] lastModified;
private final byte[][] fileHashes;

/** Method has been renamed to {@link FileSignature#signAsSet}.
* In case no sorting and removal of duplicates is required,
Expand Down Expand Up @@ -77,21 +81,21 @@ 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));
return new FileSignature(toSortedSet(files, comparing(File::getName)));
}

private FileSignature(final List<File> files) throws IOException {
this.files = files;
this.files = validateInputFiles(files);

filenames = new String[this.files.size()];
filesizes = new long[this.files.size()];
lastModified = new long[this.files.size()];
fileHashes = new byte[this.files.size()][];

int i = 0;
for (File file : this.files) {
filenames[i] = file.getCanonicalPath();
filenames[i] = file.getName();
filesizes[i] = file.length();
lastModified[i] = file.lastModified();
fileHashes[i] = hash(file);
++i;
}
}
Expand Down Expand Up @@ -119,4 +123,27 @@ public static String pathNativeToUnix(String pathNative) {
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;
}

private static byte[] hash(File file) throws IOException {
MessageDigest messageDigest;
try {
messageDigest = MessageDigest.getInstance("SHA-256");
} catch (NoSuchAlgorithmException e) {
throw new IllegalStateException("SHA-256 digest algorithm not available", e);
}
byte[] fileContent = Files.readAllBytes(file.toPath());
messageDigest.update(fileContent);
return messageDigest.digest();
}
}
22 changes: 17 additions & 5 deletions lib/src/main/java/com/diffplug/spotless/MoreIterables.java
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -20,14 +20,17 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.Comparator;
import java.util.Iterator;
import java.util.List;

final class MoreIterables {
// prevent direct instantiation
private MoreIterables() {}

/** Returns a shallow copy of input elements, throwing on null elements. */
/**
* Returns a shallow copy of input elements, throwing on null elements.
*/
static <T> List<T> toNullHostileList(Iterable<T> input) {
requireElementsNonNull(input);
List<T> shallowCopy = (input instanceof Collection)
Expand All @@ -37,18 +40,27 @@ 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;
Expand Down
43 changes: 43 additions & 0 deletions lib/src/main/java/com/diffplug/spotless/npm/NodeServerLayout.java
Original file line number Diff line number Diff line change
@@ -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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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;
Expand Down Expand Up @@ -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();
}
}
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -16,11 +16,13 @@
package com.diffplug.spotless;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import java.io.File;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.List;

import org.junit.Test;
Expand Down Expand Up @@ -48,6 +50,23 @@ public void testFromSet() throws IOException {
assertThat(outputFiles).containsExactlyElementsOf(expectedFiles);
}

@Test
public void testFromDirectory() {
File dir = new File(rootFolder(), "dir");
assertThatThrownBy(() -> FileSignature.signAsList(dir))
.isInstanceOf(IllegalArgumentException.class);
}

@Test
public void testFromFilesAndDirectory() throws IOException {
File dir = new File(rootFolder(), "dir");
List<File> files = getTestFiles(inputPaths);
files.add(dir);
Collections.shuffle(files);
assertThatThrownBy(() -> FileSignature.signAsList(files))
.isInstanceOf(IllegalArgumentException.class);
}

private List<File> getTestFiles(final String[] paths) throws IOException {
final List<File> result = new ArrayList<>(paths.length);
for (String path : paths) {
Expand Down

0 comments on commit 52a7a6c

Please sign in to comment.