Skip to content

Commit d6bba18

Browse files
committed
Fix thread safety and add compile output caching control
Thread Safety Improvements: - Replace HashMap with ConcurrentHashMap for thread-safe access - Implement per-project isolation for attachedResourcesPathsById - Implement per-project counters to prevent race conditions - Remove clear() pattern that caused races in multi-threaded builds Each project now has isolated state (project key → resources map), preventing cross-module contamination and race conditions in `mvn -T` builds. Configuration Property: - Add saveCompileOutputs property (default: true) - Allows users to control compile-phase caching behavior - Provides opt-out for users wanting reduced I/O during development - Default fixes JPMS module descriptor restoration bug Addresses reviewer feedback on PR apache#394: 1. Thread safety concern with shared HashMap 2. Performance/design concerns about compile-phase caching
1 parent 19f4a0a commit d6bba18

File tree

2 files changed

+36
-9
lines changed

2 files changed

+36
-9
lines changed

src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java

Lines changed: 30 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -137,11 +137,17 @@ public class CacheControllerImpl implements CacheController {
137137
/**
138138
* A map dedicated to store the base path of resources stored to the cache which are not original artifacts
139139
* (ex : generated source basedir).
140-
* Used to link the resource to its path on disk
140+
* Outer map: project key → (classifier → path mapping)
141+
* Used to link the resource to its path on disk.
142+
* Thread-safe for concurrent access in multi-threaded Maven builds.
141143
*/
142-
private final Map<String, Path> attachedResourcesPathsById = new HashMap<>();
144+
private final Map<String, Map<String, Path>> attachedResourcesPathsByProject = new ConcurrentHashMap<>();
143145

144-
private int attachedResourceCounter = 0;
146+
/**
147+
* Counter for attached resources, isolated per project for thread safety.
148+
* Map: project key → counter value
149+
*/
150+
private final Map<String, Integer> attachedResourceCounterByProject = new ConcurrentHashMap<>();
145151
// CHECKSTYLE_OFF: ParameterNumber
146152
@Inject
147153
public CacheControllerImpl(
@@ -498,8 +504,12 @@ public void save(
498504
final MavenProject project = context.getProject();
499505
final MavenSession session = context.getSession();
500506
try {
501-
attachedResourcesPathsById.clear();
502-
attachedResourceCounter = 0;
507+
// Get or create this project's isolated maps for thread safety in multi-threaded builds
508+
final String projectKey = getVersionlessProjectKey(project);
509+
final Map<String, Path> projectResourcePaths =
510+
attachedResourcesPathsByProject.computeIfAbsent(projectKey, k -> new ConcurrentHashMap<>());
511+
// Reset counter for this project (no race condition - each project has its own counter)
512+
attachedResourceCounterByProject.put(projectKey, 0);
503513

504514
final HashFactory hashFactory = cacheConfig.getHashFactory();
505515
final HashAlgorithm algorithm = hashFactory.createAlgorithm();
@@ -641,7 +651,12 @@ private Artifact artifactDto(
641651
dto.setFileSize(Files.size(file));
642652

643653
// Get the relative path of any extra zip directory added to the cache
644-
Path relativePath = attachedResourcesPathsById.get(projectArtifact.getClassifier());
654+
final String projectKey = getVersionlessProjectKey(project);
655+
final Map<String, Path> projectResourcePaths =
656+
attachedResourcesPathsByProject.get(projectKey);
657+
Path relativePath = projectResourcePaths != null
658+
? projectResourcePaths.get(projectArtifact.getClassifier())
659+
: null;
645660
if (relativePath == null) {
646661
// If the path was not a member of this map, we are in presence of an original artifact.
647662
// we get its location on the disk
@@ -946,11 +961,17 @@ private void attachDirIfNotEmpty(
946961
throws IOException {
947962
if (Files.isDirectory(candidateSubDir) && hasFiles(candidateSubDir)) {
948963
final Path relativePath = project.getBasedir().toPath().relativize(candidateSubDir);
949-
attachedResourceCounter++;
950-
final String classifier = attachedOutputType.getClassifierPrefix() + attachedResourceCounter;
964+
// Get this project's isolated counter and map
965+
final String projectKey = getVersionlessProjectKey(project);
966+
final int counter = attachedResourceCounterByProject.merge(projectKey, 1, Integer::sum);
967+
final String classifier = attachedOutputType.getClassifierPrefix() + counter;
951968
boolean success = zipAndAttachArtifact(project, candidateSubDir, classifier, glob);
952969
if (success) {
953-
attachedResourcesPathsById.put(classifier, relativePath);
970+
final Map<String, Path> projectResourcePaths =
971+
attachedResourcesPathsByProject.get(projectKey);
972+
if (projectResourcePaths != null) {
973+
projectResourcePaths.put(classifier, relativePath);
974+
}
954975
LOGGER.debug("Attached directory: {}", candidateSubDir);
955976
}
956977
}

src/main/mdo/build-cache-config.mdo

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,12 @@ under the License.
200200
<defaultValue>false</defaultValue>
201201
<description>Enable the cache storing ability only if a build went through the clean lifecycle.</description>
202202
</field>
203+
<field>
204+
<name>saveCompileOutputs</name>
205+
<type>boolean</type>
206+
<defaultValue>true</defaultValue>
207+
<description>Enable saving of compile-phase outputs (classes, test-classes, etc.) to cache. When enabled (default), compile-only builds can be cached and restored, fixing JPMS module descriptor restoration issues. Set to false to restore pre-fix behavior and reduce I/O during active development.</description>
208+
</field>
203209
<field>
204210
<name>multiModule</name>
205211
<association>

0 commit comments

Comments
 (0)