Fix concurrent updates to mapped model sources - #12766
Conversation
Store model sources in a concurrent key set so parallel model discovery cannot lose entries while preserving duplicate suppression.\n\nAdd coverage for concurrent updates, the null-group index, lookup behavior, and duplicate sources.
gnodet
left a comment
There was a problem hiding this comment.
APPROVE ✅
Clean, minimal fix that correctly replaces a thread-unsafe HashSet with ConcurrentHashMap.newKeySet() for the inner set values of a ConcurrentHashMap, accompanied by well-structured concurrency and deduplication tests.
Observations (non-blocking):
-
The one-line production change is correct and complete. The outer
ConcurrentHashMap.computeIfAbsentis atomic for creating the entry, and the innerConcurrentHashMap.newKeySet()makes the returnedSetthread-safe for concurrentaddand iteration. -
The
resolveReactorModelmethod iterates the set with a for-each loop. WithConcurrentHashMap.newKeySet(), this produces a weakly consistent iterator that will not throwConcurrentModificationException, which is the correct behavior for this use case. -
PathSource.equals/hashCode(based onPath) ensures that deduplication semantics are preserved with the newConcurrentHashMap-backed set, matching the previousHashSetbehavior. -
The
HashSetimport remains used elsewhere in the class (lines 987, 1519), so the import is still needed.
This review was generated by an AI agent (Claude Code) and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
Fixes #12597.
mappedSourcesuses aConcurrentHashMap, but its values were ordinaryHashSetinstances. Parallel model discovery could therefore lose sourceswhen multiple tasks updated the same group/artifact index.
Use
ConcurrentHashMap.newKeySet()for each mapped source set. This preservesduplicate suppression while making concurrent updates and iteration safe.
Regression coverage exercises concurrent writes to both the direct
group/artifact key and the secondary null-group key, verifies concurrent
collection semantics and multi-source lookup behavior, and preserves duplicate
suppression.
A backport to
maven-4.0.xmay be appropriate.Tests:
mvn -pl impl/maven-impl verify(552 tests passed, 4 skipped)mvn -Prun-its verifyunder Java 21 (all 1,051 Core ITs executed)(4 tests passed)
The full Core IT run reported two stale generated-file errors in toolchain
tests and one existing MNG-8181 log-format assertion. None exercises the
changed model-source code.
mvn verifyto make sure basic checks pass.