Skip to content

Commit 823c2c3

Browse files
lutovichnedtwigg
authored andcommitted
Fix resource leak in Maven plugin
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.
1 parent b695622 commit 823c2c3

File tree

4 files changed

+86
-18
lines changed

4 files changed

+86
-18
lines changed

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

+33-6
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,14 @@
1717

1818
import static com.diffplug.spotless.MoreIterables.toNullHostileList;
1919
import static com.diffplug.spotless.MoreIterables.toSortedSet;
20+
import static java.util.Comparator.comparing;
2021

2122
import java.io.File;
2223
import java.io.IOException;
2324
import java.io.Serializable;
25+
import java.nio.file.Files;
26+
import java.security.MessageDigest;
27+
import java.security.NoSuchAlgorithmException;
2428
import java.util.Arrays;
2529
import java.util.Collection;
2630
import java.util.Collections;
@@ -42,7 +46,7 @@ public final class FileSignature implements Serializable {
4246

4347
private final String[] filenames;
4448
private final long[] filesizes;
45-
private final long[] lastModified;
49+
private final byte[][] fileHashes;
4650

4751
/** Method has been renamed to {@link FileSignature#signAsSet}.
4852
* In case no sorting and removal of duplicates is required,
@@ -77,21 +81,21 @@ public static FileSignature signAsSet(File... files) throws IOException {
7781

7882
/** Creates file signature insensitive to the order of the files. */
7983
public static FileSignature signAsSet(Iterable<File> files) throws IOException {
80-
return new FileSignature(toSortedSet(files));
84+
return new FileSignature(toSortedSet(files, comparing(File::getName)));
8185
}
8286

8387
private FileSignature(final List<File> files) throws IOException {
84-
this.files = files;
88+
this.files = validateInputFiles(files);
8589

8690
filenames = new String[this.files.size()];
8791
filesizes = new long[this.files.size()];
88-
lastModified = new long[this.files.size()];
92+
fileHashes = new byte[this.files.size()][];
8993

9094
int i = 0;
9195
for (File file : this.files) {
92-
filenames[i] = file.getCanonicalPath();
96+
filenames[i] = file.getName();
9397
filesizes[i] = file.length();
94-
lastModified[i] = file.lastModified();
98+
fileHashes[i] = hash(file);
9599
++i;
96100
}
97101
}
@@ -119,4 +123,27 @@ public static String pathNativeToUnix(String pathNative) {
119123
public static String pathUnixToNative(String pathUnix) {
120124
return LineEnding.nativeIsWin() ? pathUnix.replace('/', '\\') : pathUnix;
121125
}
126+
127+
private static List<File> validateInputFiles(List<File> files) {
128+
for (File file : files) {
129+
if (!file.isFile()) {
130+
throw new IllegalArgumentException(
131+
"File signature can only be created for existing regular files, given: "
132+
+ file);
133+
}
134+
}
135+
return files;
136+
}
137+
138+
private static byte[] hash(File file) throws IOException {
139+
MessageDigest messageDigest;
140+
try {
141+
messageDigest = MessageDigest.getInstance("SHA-256");
142+
} catch (NoSuchAlgorithmException e) {
143+
throw new IllegalStateException("SHA-256 digest algorithm not available", e);
144+
}
145+
byte[] fileContent = Files.readAllBytes(file.toPath());
146+
messageDigest.update(fileContent);
147+
return messageDigest.digest();
148+
}
122149
}

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

+10-4
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2016 DiffPlug
2+
* Copyright 2016-2020 DiffPlug
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -20,6 +20,7 @@
2020
import java.util.ArrayList;
2121
import java.util.Collection;
2222
import java.util.Collections;
23+
import java.util.Comparator;
2324
import java.util.Iterator;
2425
import java.util.List;
2526

@@ -37,18 +38,23 @@ static <T> List<T> toNullHostileList(Iterable<T> input) {
3738
return shallowCopy;
3839
}
3940

40-
/** Sorts "raw" and removes duplicates, throwing on null elements. */
41+
/** Sorts "raw" using {@link Comparator#naturalOrder()} and removes duplicates, throwing on null elements. */
4142
static <T extends Comparable<T>> List<T> toSortedSet(Iterable<T> raw) {
43+
return toSortedSet(raw, Comparator.naturalOrder());
44+
}
45+
46+
/** Sorts "raw" and removes duplicates, throwing on null elements. */
47+
static <T> List<T> toSortedSet(Iterable<T> raw, Comparator<T> comparator) {
4248
List<T> toBeSorted = toNullHostileList(raw);
4349
// sort it
44-
Collections.sort(toBeSorted);
50+
Collections.sort(toBeSorted, comparator);
4551
// remove any duplicates (normally there won't be any)
4652
if (toBeSorted.size() > 1) {
4753
Iterator<T> iter = toBeSorted.iterator();
4854
T last = iter.next();
4955
while (iter.hasNext()) {
5056
T next = iter.next();
51-
if (next.compareTo(last) == 0) {
57+
if (comparator.compare(next, last) == 0) {
5258
iter.remove();
5359
} else {
5460
last = next;

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

+23-7
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2016 DiffPlug
2+
* Copyright 2016-2020 DiffPlug
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -16,10 +16,13 @@
1616
package com.diffplug.spotless.maven;
1717

1818
import static com.diffplug.common.base.Strings.isNullOrEmpty;
19+
import static java.nio.charset.StandardCharsets.UTF_8;
1920

2021
import java.io.File;
22+
import java.security.MessageDigest;
23+
import java.security.NoSuchAlgorithmException;
24+
import java.util.Base64;
2125
import java.util.Objects;
22-
import java.util.UUID;
2326

2427
import org.codehaus.plexus.resource.ResourceManager;
2528
import org.codehaus.plexus.resource.loader.FileResourceCreationException;
@@ -63,16 +66,29 @@ public File locateFile(String path) {
6366
}
6467
}
6568

66-
private static String tmpOutputFileName(String path) {
67-
String extension = FileUtils.extension(path);
68-
return TMP_RESOURCE_FILE_PREFIX + UUID.randomUUID() + '.' + extension;
69-
}
70-
7169
public File getBaseDir() {
7270
return baseDir;
7371
}
7472

7573
public File getBuildDir() {
7674
return buildDir;
7775
}
76+
77+
private static String tmpOutputFileName(String path) {
78+
String extension = FileUtils.extension(path);
79+
byte[] pathHash = hash(path);
80+
String pathBase64 = Base64.getEncoder().encodeToString(pathHash);
81+
return TMP_RESOURCE_FILE_PREFIX + pathBase64 + '.' + extension;
82+
}
83+
84+
private static byte[] hash(String value) {
85+
MessageDigest messageDigest;
86+
try {
87+
messageDigest = MessageDigest.getInstance("SHA-256");
88+
} catch (NoSuchAlgorithmException e) {
89+
throw new IllegalStateException("SHA-256 digest algorithm not available", e);
90+
}
91+
messageDigest.update(value.getBytes(UTF_8));
92+
return messageDigest.digest();
93+
}
7894
}

testlib/src/test/java/com/diffplug/spotless/FileSignatureTest.java

+20-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2016 DiffPlug
2+
* Copyright 2016-2020 DiffPlug
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -16,11 +16,13 @@
1616
package com.diffplug.spotless;
1717

1818
import static org.assertj.core.api.Assertions.assertThat;
19+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
1920

2021
import java.io.File;
2122
import java.io.IOException;
2223
import java.util.ArrayList;
2324
import java.util.Collection;
25+
import java.util.Collections;
2426
import java.util.List;
2527

2628
import org.junit.Test;
@@ -48,6 +50,23 @@ public void testFromSet() throws IOException {
4850
assertThat(outputFiles).containsExactlyElementsOf(expectedFiles);
4951
}
5052

53+
@Test
54+
public void testFromDirectory() {
55+
File dir = new File(rootFolder(), "dir");
56+
assertThatThrownBy(() -> FileSignature.signAsList(dir))
57+
.isInstanceOf(IllegalArgumentException.class);
58+
}
59+
60+
@Test
61+
public void testFromFilesAndDirectory() throws IOException {
62+
File dir = new File(rootFolder(), "dir");
63+
List<File> files = getTestFiles(inputPaths);
64+
files.add(dir);
65+
Collections.shuffle(files);
66+
assertThatThrownBy(() -> FileSignature.signAsList(files))
67+
.isInstanceOf(IllegalArgumentException.class);
68+
}
69+
5170
private List<File> getTestFiles(final String[] paths) throws IOException {
5271
final List<File> result = new ArrayList<>(paths.length);
5372
for (String path : paths) {

0 commit comments

Comments
 (0)