You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "abhu85 (via GitHub)" <gi...@apache.org> on 2026/02/23 18:35:29 UTC
[PR] Fix thread-safety issue in DefaultModelValidator (#11618) [maven]
abhu85 opened a new pull request, #11734:
URL: https://github.com/apache/maven/pull/11734
## Summary
- Fix thread-safety issue where concurrent model validation causes `ClassCastException`
- Aligns compat layer with Maven 4 implementation which already uses thread-safe collections
- Adds regression test for concurrent validation
## Problem
`DefaultModelValidator` is a `@Singleton` with a shared mutable `HashSet` (`validIds`). When multiple threads validate models concurrently (e.g., using breadth-first dependency collector with `aether.dependencyCollector.impl=bf`), concurrent read/write operations on the HashSet cause:
```
java.lang.ClassCastException: class java.util.HashMap$Node cannot
be cast to class java.util.HashMap$TreeNode
at java.util.HashMap$TreeNode.putTreeVal(HashMap.java:2079)
at java.util.HashMap.putVal(HashMap.java:634)
at java.util.HashSet.add(HashSet.java:220)
at o.a.m.model.validation.DefaultModelValidator.validateId(DefaultModelValidator.java:1145)
```
This affects users of:
- Clojure tools-deps
- Leiningen with `aether.dependencyCollector.impl=bf`
- NetBeans IDE
## Fix Approach
Replace `new HashSet<>()` with `ConcurrentHashMap.newKeySet()` - identical to the solution already implemented in Maven 4's `maven-impl` module (lines 296, 298).
## Files Changed
| File | Change |
|------|--------|
| `compat/maven-model-builder/.../DefaultModelValidator.java` | Replace `HashSet` → `ConcurrentHashMap.newKeySet()` + explanatory comment |
| `compat/maven-model-builder/.../DefaultModelValidatorTest.java` | Add concurrent validation test (10 threads × 100 models) |
## Test Plan
- [x] New test `testConcurrentValidation()` added
- [ ] `mvn -pl compat/maven-model-builder test`
- [ ] `mvn -Prun-its verify` (optional, if reviewer deems necessary)
## Compatibility
- **Backwards compatible**: `ConcurrentHashMap.newKeySet()` provides same `Set` interface
- **No behavioral change**: Same validation logic, just thread-safe storage
- **Aligns with Maven 4**: Mirrors the implementation in `maven-impl` module
## Risk Assessment
**Low risk**:
- Minimal diff (3 lines of production code including comment)
- Same fix pattern already proven in Maven 4
- Thread-safe collection is a drop-in replacement
Fixes #11618
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix thread-safety issue in DefaultModelValidator (#11618) [maven]
Posted by "k15tfu (via GitHub)" <gi...@apache.org>.
k15tfu commented on PR #11734:
URL: https://github.com/apache/maven/pull/11734#issuecomment-5158052289
@gnodet Hi, would it be possible to backport this fix to 3.9.x or 3.10.x?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix thread-safety issue in DefaultModelValidator (#11618) [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet merged PR #11734:
URL: https://github.com/apache/maven/pull/11734
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix thread-safety issue in DefaultModelValidator (#11618) [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #11734:
URL: https://github.com/apache/maven/pull/11734#discussion_r3284349112
##########
compat/maven-model-builder/src/test/java/org/apache/maven/model/validation/DefaultModelValidatorTest.java:
##########
@@ -902,4 +902,61 @@ void profileActivationPropertyWithProjectExpression() throws Exception {
+ "${project.version} expressions are not supported during profile activation.",
result.getWarnings().get(1));
}
+
+ /**
+ * Validates thread-safety of DefaultModelValidator during concurrent model validation.
+ *
+ * <p>This test addresses GitHub issue #11618 where concurrent access to a shared
+ * {@code HashSet} in {@code DefaultModelValidator} could cause {@code ClassCastException}.
Review Comment:
Nit: the rest of the file uses top-level imports. These fully-qualified inline references work, but adding proper imports would be more consistent with the existing style:
```java
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicReference;
```
##########
compat/maven-model-builder/src/test/java/org/apache/maven/model/validation/DefaultModelValidatorTest.java:
##########
@@ -902,4 +902,61 @@ void profileActivationPropertyWithProjectExpression() throws Exception {
+ "${project.version} expressions are not supported during profile activation.",
result.getWarnings().get(1));
}
+
+ /**
+ * Validates thread-safety of DefaultModelValidator during concurrent model validation.
+ *
+ * <p>This test addresses GitHub issue #11618 where concurrent access to a shared
+ * {@code HashSet} in {@code DefaultModelValidator} could cause {@code ClassCastException}.
+ * The underlying issue occurs when multiple threads access a non-thread-safe {@code HashSet}
+ * (backed by {@code HashMap}) during internal restructuring operations.
+ *
+ * <p>The fix replaces {@code HashSet} with {@code ConcurrentHashMap.newKeySet()} to provide
+ * thread-safe concurrent access without external synchronization.
+ *
+ * @see <a href="https://github.com/apache/maven/issues/11618">GitHub #11618</a>
+ */
+ @Test
+ void testConcurrentValidation() throws Exception {
+ int threadCount = 10;
+ int iterationsPerThread = 100;
+ java.util.concurrent.CountDownLatch startLatch = new java.util.concurrent.CountDownLatch(1);
+ java.util.concurrent.CountDownLatch doneLatch = new java.util.concurrent.CountDownLatch(threadCount);
+ java.util.concurrent.atomic.AtomicReference<Throwable> failure = new java.util.concurrent.atomic.AtomicReference<>();
+
+ // Create multiple threads that will validate models concurrently
+ for (int t = 0; t < threadCount; t++) {
+ final int threadId = t;
+ new Thread(() -> {
+ try {
+ startLatch.await(); // Wait for all threads to be ready
+ for (int i = 0; i < iterationsPerThread; i++) {
+ Model model = new Model();
Review Comment:
Minor: the thread is created with `new Thread(...).start()` but never joined or named. `doneLatch.await(30, ...)` handles termination, but if a thread hangs, the unnamed daemon-less threads will keep the JVM alive after the test times out. Consider `Thread.ofVirtual()` or at minimum setting the thread as daemon / giving it a name for debuggability. Not a blocker.
##########
compat/maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java:
##########
@@ -89,7 +90,9 @@ public class DefaultModelValidator implements ModelValidator {
private static final String EMPTY = "";
- private final Set<String> validIds = new HashSet<>();
+ // Thread-safe set required because class is @Singleton and validIds is accessed concurrently
+ // See: https://github.com/apache/maven/issues/11618
Review Comment:
The fix itself is correct. As a pre-existing observation (not introduced by this PR): `validIds` grows unboundedly across all validations for the lifetime of the singleton — every unique groupId/artifactId/version ever validated is cached forever. In a long-running embedded scenario (IDE, daemon), this is a slow memory leak. The `maven-impl` module has the same pattern. Might be worth a follow-up issue to scope the cache per-session or use a bounded/weak-ref structure, but that's out of scope here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix thread-safety issue in DefaultModelValidator (#11618) [maven]
Posted by "abhu85 (via GitHub)" <gi...@apache.org>.
abhu85 commented on PR #11734:
URL: https://github.com/apache/maven/pull/11734#issuecomment-3992948200
Hi maintainers 👋
Friendly ping on this PR - it's been open for about a week. This is a minimal fix (3 lines of production code) for the thread-safety issue in
DefaultModelValidator that causes ClassCastException during concurrent model validation (#11618).
The fix replaces HashSet with ConcurrentHashMap.newKeySet() - the same approach already used in Maven 4's maven-impl module.
I notice the mergeable state shows as "unstable" - happy to rebase if needed. Would appreciate a review when you have a moment. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix thread-safety issue in DefaultModelValidator (#11618) [maven]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #11734:
URL: https://github.com/apache/maven/pull/11734#issuecomment-4726714439
@gnodet Please assign appropriate label to PR according to the type of change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org