Fix #12605: DefaultProjectBuildingHelper synchronized bottleneck in parallel builds - #12630
Fix #12605: DefaultProjectBuildingHelper synchronized bottleneck in parallel builds#12630elharo wants to merge 2 commits into
Conversation
…arallel builds Remove the synchronized keyword from createProjectRealm() and fix the check-then-act race in DefaultProjectRealmCache.put() by using ConcurrentHashMap.putIfAbsent() instead of containsKey() + put(). The projectRealmCache already uses ConcurrentHashMap and is thread-safe for get/createKey operations. The only non-thread-safe portion was the put() method's check-then-act pattern, which is now atomic. This allows multiple threads to set up extension realms for different projects concurrently rather than serializing through a global lock. Fixes gh-12605
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
The PR correctly identifies that DefaultProjectRealmCache.put() had a non-atomic check-then-act pattern and fixes it with putIfAbsent(). However, removing synchronized from createProjectRealm() exposes a TOCTOU race at the caller level that can crash parallel builds.
Issue: Race condition in createProjectRealm() (high severity)
The get() → create realm → put() sequence (lines 223-258) is not atomic. In a parallel multi-module build where two modules share the same extensions:
- Both threads call
projectRealmCache.get(key)→ both getnull - Both enter the
if (record == null)block - Both call
classRealmManager.createProjectRealm()creating duplicate ClassRealms - One thread's
putIfAbsent()succeeds; the other throwsIllegalStateException - The ISE is not caught by the caller at
DefaultProjectBuilder.java:1274(which only catchesPluginResolutionException | PluginManagerException | PluginVersionResolutionException) → build crash
The old synchronized prevented this race entirely.
Secondary issue: ClassRealm resource leak (medium severity)
The ClassRealm created by the losing thread is never registered in the cache, so DefaultProjectRealmCache.flush() never disposes it.
Suggested approaches
- Replace the get-create-put pattern with a
computeIfAbsent-style approach that atomically populates the cache - Or use a finer-grained lock (e.g., striped by key) to serialize same-key access while allowing different keys to proceed concurrently
- Or change
put()to return the existing record instead of throwing (matchingConcurrentHashMap.putIfAbsentsemantics) and have the caller dispose the redundant realm
Test coverage
The new test validates the cache-level atomicity (good), but does not cover the higher-level race in createProjectRealm() that this PR introduces.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Removes a global synchronization bottleneck during parallel builds and hardens project realm caching against concurrent duplicate insertions.
Changes:
- Removed
synchronizedfromDefaultProjectBuildingHelper.createProjectRealm()to avoid serializing parallel module builds. - Made
DefaultProjectRealmCache.put()atomic by switching toputIfAbsent()and throwing on duplicates. - Added a concurrency-focused unit test to validate only one concurrent
put()succeeds for the same key.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| impl/maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuildingHelper.java | Removes synchronized to eliminate a global lock in parallel builds. |
| impl/maven-core/src/main/java/org/apache/maven/project/DefaultProjectRealmCache.java | Fixes a check-then-act race by using putIfAbsent() and duplicate detection. |
| impl/maven-core/src/test/java/org/apache/maven/project/DefaultProjectRealmCacheTest.java | Adds a concurrent test to assert duplicate put() behavior under contention. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| for (int i = 0; i < threadCount; i++) { | ||
| futures.add(executor.submit(() -> { | ||
| barrier.await(); |
|
|
||
| int successCount = 0; |
|
|
||
| int threadCount = 10; | ||
| CyclicBarrier barrier = new CyclicBarrier(threadCount); | ||
| ExecutorService executor = Executors.newFixedThreadPool(threadCount); | ||
| List<Future<Boolean>> futures = new ArrayList<>(); | ||
|
|
||
| for (int i = 0; i < threadCount; i++) { | ||
| futures.add(executor.submit(() -> { | ||
| barrier.await(); | ||
| try { | ||
| cache.put(key, mock(ClassRealm.class), mock(DependencyFilter.class)); | ||
| return true; | ||
| } catch (IllegalStateException e) { | ||
| return false; | ||
| } | ||
| })); | ||
| } | ||
|
|
||
| int successCount = 0; | ||
| for (Future<Boolean> f : futures) { | ||
| if (f.get()) { | ||
| successCount++; | ||
| } |
| CacheRecord record = new CacheRecord(projectRealm, extensionArtifactFilter); | ||
|
|
||
| cache.put(key, record); | ||
| CacheRecord existing = cache.putIfAbsent(key, record); | ||
|
|
||
| if (existing != null) { | ||
| throw new IllegalStateException("Duplicate project realm for extensions " + key); | ||
| } |
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Re-reviewed after new commit — the TOCTOU race from our previous review remains unaddressed. The new commit (df63754) only fixes checkstyle/style issues in the test file and introduces two new issues.
1. TOCTOU race condition still present (high severity)
The critical race in createProjectRealm() is unchanged: removing synchronized without replacing it with an equivalent concurrency mechanism means IllegalStateException will be thrown under contention in parallel builds. See our previous review for the full analysis and suggested approaches.
2. Compilation error introduced (high severity)
The checkstyle commit removed the org.eclipse.aether.graph.DependencyFilter import along with an unused local variable, but DependencyFilter.class is still used inline in mock(DependencyFilter.class) at line 52. Since DependencyFilter is from org.eclipse.aether.graph (not the test's own package), this will fail to compile.
3. Typo (low severity)
The renamed loop variable futre at line 62 should be future.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
Fixes #12605
Problem
DefaultProjectBuildingHelper.createProjectRealm()was declaredsynchronized, making it a global lock during multi-module parallel builds. Every project needing extension realm setup had to acquire this lock serially.Additionally,
DefaultProjectRealmCache.put()had a check-then-act race condition:containsKey()followed byput()are not atomic, allowing two threads to both pass the check and overwrite each other's entry.Fix
synchronizedfromcreateProjectRealm()— theprojectRealmCacheusesConcurrentHashMapand is already thread-safe forget/createKey.put()race — replaced the non-atomiccontainsKey()+put()withputIfAbsent(), which atomically inserts only if absent and throwsIllegalStateExceptionif the key already exists.Testing
Added
DefaultProjectRealmCacheTestthat verifies concurrentput()calls with the same key result in exactly one successful insertion.