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 `FileSignature` only accepts files, not directories.
  • Loading branch information
lutovich authored and nedtwigg committed Jun 27, 2020
1 parent b695622 commit 823c2c3
Show file tree
Hide file tree
Showing 4 changed files with 86 additions and 18 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();
}
}
14 changes: 10 additions & 4 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,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;

Expand All @@ -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;
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 823c2c3

Please sign in to comment.