Skip to content

Commit 3dce563

Browse files
author
Vincent Potucek
committed
fix ConcurrentModificationException in expandWildcardImports 
1 parent 1104073 commit 3dce563

File tree

2 files changed

+188
-30
lines changed

2 files changed

+188
-30
lines changed

‎lib/src/main/java/com/diffplug/spotless/java/ExpandWildcardImportsStep.java‎

Lines changed: 31 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,17 @@
1515
*/
1616
package com.diffplug.spotless.java;
1717

18+
import static com.diffplug.spotless.JarState.from;
19+
import static com.diffplug.spotless.JarState.promise;
20+
import static java.util.List.copyOf;
21+
import static java.util.Objects.requireNonNull;
22+
1823
import java.io.File;
1924
import java.io.Serial;
2025
import java.io.Serializable;
2126
import java.lang.reflect.InvocationTargetException;
27+
import java.util.ArrayList;
2228
import java.util.Collection;
23-
import java.util.Objects;
2429
import java.util.Set;
2530

2631
import com.diffplug.spotless.FormatterFunc;
@@ -33,22 +38,23 @@ public final class ExpandWildcardImportsStep implements Serializable {
3338
private static final long serialVersionUID = 1L;
3439

3540
private static final String INCOMPATIBLE_ERROR_MESSAGE = "There was a problem interacting with Java-Parser; maybe you set an incompatible version?";
36-
private static final String MAVEN_COORDINATES = "com.github.javaparser:javaparser-symbol-solver-core";
37-
public static final String DEFAULT_VERSION = "3.27.1";
41+
private static final String MAVEN_COORDINATES = "com.github.javaparser:javaparser-symbol-solver-core:3.27.1";
3842

3943
private final Collection<File> typeSolverClasspath;
4044
private final JarState.Promised jarState;
4145

4246
private ExpandWildcardImportsStep(Collection<File> typeSolverClasspath, JarState.Promised jarState) {
43-
this.typeSolverClasspath = typeSolverClasspath;
47+
this.typeSolverClasspath = new ArrayList<>(typeSolverClasspath); // Create defensive copy of the classpath.
4448
this.jarState = jarState;
4549
}
4650

4751
public static FormatterStep create(Set<File> typeSolverClasspath, Provisioner provisioner) {
48-
Objects.requireNonNull(provisioner, "provisioner cannot be null");
52+
requireNonNull(provisioner);
53+
requireNonNull(typeSolverClasspath);
4954
return FormatterStep.create("expandwildcardimports",
50-
new ExpandWildcardImportsStep(typeSolverClasspath,
51-
JarState.promise(() -> JarState.from(MAVEN_COORDINATES + ":" + DEFAULT_VERSION, provisioner))),
55+
new ExpandWildcardImportsStep(
56+
new ArrayList<>(typeSolverClasspath), // Create a defensive copy of the classpath.
57+
promise(() -> from(MAVEN_COORDINATES, provisioner))),
5258
ExpandWildcardImportsStep::equalityState,
5359
State::toFormatter);
5460
}
@@ -57,31 +63,26 @@ private State equalityState() {
5763
return new State(typeSolverClasspath, jarState.get());
5864
}
5965

60-
private static class State implements Serializable {
61-
@Serial
62-
private static final long serialVersionUID = 1L;
63-
64-
private final Collection<File> typeSolverClasspath;
65-
private final JarState jarState;
66+
private record State(Collection<File> typeSolverClasspath, JarState jarState) implements Serializable {
6667

67-
public State(Collection<File> typeSolverClasspath, JarState jarState) {
68-
this.typeSolverClasspath = typeSolverClasspath;
69-
this.jarState = jarState;
70-
}
68+
@Serial
69+
private static final long serialVersionUID = 1L;
7170

72-
FormatterFunc toFormatter() {
73-
try {
74-
return (FormatterFunc) jarState
75-
.getClassLoader()
76-
.loadClass("com.diffplug.spotless.glue.javaparser.ExpandWildcardsFormatterFunc")
77-
.getConstructor(Collection.class)
78-
.newInstance(typeSolverClasspath);
79-
} catch (ClassNotFoundException | NoSuchMethodException | InvocationTargetException
80-
| InstantiationException | IllegalAccessException | NoClassDefFoundError cause) {
81-
throw new IllegalStateException(INCOMPATIBLE_ERROR_MESSAGE, cause);
71+
private State(Collection<File> typeSolverClasspath, JarState jarState) {
72+
this.typeSolverClasspath = copyOf(typeSolverClasspath); // Create immutable copy.
73+
this.jarState = jarState;
8274
}
83-
}
8475

76+
FormatterFunc toFormatter() {
77+
try {
78+
return (FormatterFunc) jarState
79+
.getClassLoader()
80+
.loadClass("com.diffplug.spotless.glue.javaparser.ExpandWildcardsFormatterFunc")
81+
.getConstructor(Collection.class)
82+
.newInstance(typeSolverClasspath);
83+
} catch (ClassNotFoundException | NoSuchMethodException | InvocationTargetException
84+
| InstantiationException | IllegalAccessException | NoClassDefFoundError cause) {
85+
throw new IllegalStateException(INCOMPATIBLE_ERROR_MESSAGE, cause);
86+
}
8587
}
86-
87-
}
88+
}}

‎testlib/src/test/java/com/diffplug/spotless/java/ExpandWildcardImportsStepTest.java‎

Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,15 +15,26 @@
1515
*/
1616
package com.diffplug.spotless.java;
1717

18+
import static org.assertj.core.api.Fail.fail;
19+
import static org.junit.jupiter.api.Assertions.assertEquals;
20+
1821
import java.io.File;
1922
import java.nio.charset.StandardCharsets;
2023
import java.nio.file.Files;
24+
import java.util.Collections;
25+
import java.util.HashSet;
2126
import java.util.Set;
27+
import java.util.concurrent.ConcurrentHashMap;
28+
import java.util.concurrent.ExecutorService;
29+
import java.util.concurrent.Executors;
30+
import java.util.concurrent.TimeUnit;
31+
import java.util.concurrent.atomic.AtomicInteger;
2232

2333
import org.junit.jupiter.api.Test;
2434

2535
import com.diffplug.spotless.FormatterStep;
2636
import com.diffplug.spotless.ResourceHarness;
37+
import com.diffplug.spotless.StepHarness;
2738
import com.diffplug.spotless.StepHarnessWithFile;
2839
import com.diffplug.spotless.TestProvisioner;
2940

@@ -39,4 +50,150 @@ void expandWildCardImports() throws Exception {
3950
StepHarnessWithFile.forStep(this, step).testResource("java/expandwildcardimports/JavaClassWithWildcardsUnformatted.test", "java/expandwildcardimports/JavaClassWithWildcardsFormatted.test");
4051
}
4152

53+
@Test
54+
void expandWildcardImports_concurrentAccess() throws Exception {
55+
// Test concurrent access to the formatter step
56+
newFile("src/foo/bar/baz/").mkdirs();
57+
Files.write(newFile("src/foo/bar/AnotherClassInSamePackage.java").toPath(), getTestResource("java/expandwildcardimports/AnotherClassInSamePackage.test").getBytes(StandardCharsets.UTF_8));
58+
Files.write(newFile("src/foo/bar/baz/AnotherImportedClass.java").toPath(), getTestResource("java/expandwildcardimports/AnotherImportedClass.test").getBytes(StandardCharsets.UTF_8));
59+
File dummyJar = new File(ResourceHarness.class.getResource("/java/expandwildcardimports/example-lib.jar").toURI());
60+
61+
// Create the step once
62+
FormatterStep step = ExpandWildcardImportsStep.create(
63+
Collections.synchronizedSet(new HashSet<>(Set.of(newFile("src"), dummyJar))),
64+
TestProvisioner.mavenCentral());
65+
66+
String testInput = getTestResource("java/expandwildcardimports/JavaClassWithWildcardsUnformatted.test");
67+
String expectedOutput = getTestResource("java/expandwildcardimports/JavaClassWithWildcardsFormatted.test");
68+
69+
// Test with multiple threads
70+
int threadCount = 10;
71+
ExecutorService executor = Executors.newFixedThreadPool(threadCount);
72+
AtomicInteger successCount = new AtomicInteger(0);
73+
74+
for (int i = 0; i < threadCount; i++) {
75+
executor.submit(() -> {
76+
try {
77+
StepHarness harness = StepHarness.forStep(step);
78+
harness.test(testInput, expectedOutput);
79+
successCount.incrementAndGet();
80+
} catch (Exception e) {
81+
e.printStackTrace();
82+
throw new RuntimeException(e);
83+
}
84+
});
85+
}
86+
87+
executor.shutdown();
88+
executor.awaitTermination(30, TimeUnit.SECONDS);
89+
90+
// All threads should succeed
91+
assertEquals(threadCount, successCount.get(), "All concurrent accesses should succeed");
92+
}
93+
94+
@Test
95+
void expandWildcardImports_withMutableClasspath() throws Exception {
96+
// Test with a classpath that could be modified concurrently
97+
newFile("src/foo/bar/baz/").mkdirs();
98+
Files.write(newFile("src/foo/bar/AnotherClassInSamePackage.java").toPath(), getTestResource("java/expandwildcardimports/AnotherClassInSamePackage.test").getBytes(StandardCharsets.UTF_8));
99+
Files.write(newFile("src/foo/bar/baz/AnotherImportedClass.java").toPath(), getTestResource("java/expandwildcardimports/AnotherImportedClass.test").getBytes(StandardCharsets.UTF_8));
100+
File dummyJar = new File(ResourceHarness.class.getResource("/java/expandwildcardimports/example-lib.jar").toURI());
101+
102+
// Use thread-safe collections for classpath
103+
Set<File> classpath = ConcurrentHashMap.newKeySet();
104+
classpath.add(newFile("src"));
105+
classpath.add(dummyJar);
106+
107+
FormatterStep step = ExpandWildcardImportsStep.create(classpath, TestProvisioner.mavenCentral());
108+
109+
String testInput = getTestResource("java/expandwildcardimports/JavaClassWithWildcardsUnformatted.test");
110+
String expectedOutput = getTestResource("java/expandwildcardimports/JavaClassWithWildcardsFormatted.test");
111+
112+
StepHarness.forStep(step).test(testInput, expectedOutput);
113+
114+
// Test that we can modify the original classpath without affecting the step
115+
// (the step should have taken a defensive copy)
116+
classpath.clear();
117+
classpath.add(newFile("different/path"));
118+
119+
// The step should still work with its original classpath
120+
StepHarness.forStep(step).test(testInput, expectedOutput);
121+
}
122+
123+
@Test
124+
void expandWildcardImports_emptyClasspath() throws Exception {
125+
// Test with empty classpath
126+
FormatterStep step = ExpandWildcardImportsStep.create(Collections.emptySet(), TestProvisioner.mavenCentral());
127+
128+
// Even with empty classpath, Java standard library imports should still be expanded
129+
String simpleCode = """
130+
package test;
131+
132+
import java.util.*;
133+
134+
public class Test {
135+
private List<String> items;
136+
}
137+
""";
138+
139+
String expectedOutput = """
140+
package test;
141+
142+
import java.util.List;
143+
144+
public class Test {
145+
private List<String> items;
146+
}
147+
""";
148+
149+
// The step should still expand java.util.* to java.util.List
150+
StepHarness.forStep(step).test(simpleCode, expectedOutput);
151+
}
152+
153+
@Test
154+
void expandWildcardImports_nullSafety() throws Exception {
155+
// Test null safety
156+
try {
157+
ExpandWildcardImportsStep.create(null, TestProvisioner.mavenCentral());
158+
fail("Should throw NullPointerException for null classpath");
159+
} catch (NullPointerException e) {
160+
// Expected
161+
}
162+
163+
try {
164+
ExpandWildcardImportsStep.create(Set.of(newFile("src")), null);
165+
fail("Should throw NullPointerException for null provisioner");
166+
} catch (NullPointerException e) {
167+
// Expected
168+
}
169+
}
170+
171+
@Test
172+
void expandWildcardImports_withLargeClasspath() throws Exception {
173+
// Test with a large classpath to ensure no performance issues
174+
newFile("src/foo/bar/baz/").mkdirs();
175+
Files.write(newFile("src/foo/bar/AnotherClassInSamePackage.java").toPath(), getTestResource("java/expandwildcardimports/AnotherClassInSamePackage.test").getBytes(StandardCharsets.UTF_8));
176+
Files.write(newFile("src/foo/bar/baz/AnotherImportedClass.java").toPath(), getTestResource("java/expandwildcardimports/AnotherImportedClass.test").getBytes(StandardCharsets.UTF_8));
177+
File dummyJar = new File(ResourceHarness.class.getResource("/java/expandwildcardimports/example-lib.jar").toURI());
178+
179+
// Create a large classpath
180+
Set<File> largeClasspath = ConcurrentHashMap.newKeySet();
181+
largeClasspath.add(newFile("src"));
182+
largeClasspath.add(dummyJar);
183+
184+
// Add many dummy directories
185+
for (int i = 0; i < 100; i++) {
186+
File dummyDir = newFile("dummy/dir/" + i + "/");
187+
dummyDir.mkdirs();
188+
largeClasspath.add(dummyDir);
189+
}
190+
191+
FormatterStep step = ExpandWildcardImportsStep.create(largeClasspath, TestProvisioner.mavenCentral());
192+
193+
String testInput = getTestResource("java/expandwildcardimports/JavaClassWithWildcardsUnformatted.test");
194+
String expectedOutput = getTestResource("java/expandwildcardimports/JavaClassWithWildcardsFormatted.test");
195+
196+
// Should handle large classpaths without issues
197+
StepHarness.forStep(step).test(testInput, expectedOutput);
198+
}
42199
}

0 commit comments

Comments
 (0)