Re: [PR] Fix thread-safety issue in DefaultModelValidator (#11618) [maven]

["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