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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
