(maven) branch maven-3.10.x updated: [3.10.x] Fix gh-11618: Make DefaultModelValidator thread-safe (#13303)

[cs...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

cstamas pushed a commit to branch maven-3.10.x
in repository https://gitbox.apache.org/repos/asf/maven.git


The following commit(s) were added to refs/heads/maven-3.10.x by this push:
     new bb5f9d767a [3.10.x] Fix gh-11618: Make DefaultModelValidator thread-safe (#13303)
bb5f9d767a is described below

commit bb5f9d767a4f5f1cc3f98e5f43daad9b081d3cb9
Author: Abhishek Chauhan <60...@users.noreply.github.com>
AuthorDate: Tue Oct 20 20:31:42 2026 +0530

    [3.10.x] Fix gh-11618: Make DefaultModelValidator thread-safe (#13303)
    
    Backport of #11734 (already on `master`, and on `maven-4.0.x` via #12284) to `maven-3.10.x`. A backport to the 3.x line was requested on #11734 (https://github.com/apache/maven/pull/11734#issuecomment-5158052289) and has not been answered.
    
    ### Problem
    
    `DefaultModelValidator` is a `@Singleton`, and its `validIds` cache is a plain `HashSet` shared by every validation that runs through it. Callers that validate models concurrently (the breadth-first dependency collector, `aether.dependencyCollector.impl=bf`, as used by Clojure tools-deps and Leiningen; IDE integrations such as NetBeans) can corrupt the underlying `HashMap`. #11618 reports a `ClassCastException` (`HashMap$Node cannot be cast to class HashMap$TreeNode`).
    
    ### Change
    
    Same as on `master` and `maven-4.0.x`:
    - `validIds` is `ConcurrentHashMap.newKeySet()`;
    - `validateId()` checks `id != null` before `contains()`, since a `ConcurrentHashMap` key set throws on a `null` lookup. A `null` id still reaches `validateStringNotEmpty()` and is reported as before.
    
    The regression test `testConcurrentValidation()` is the one from #11734, unchanged.
    
    ### Verification on `maven-3.10.x`
    
    - `mvn -pl maven-model-builder verify` (including spotless and RAT): BUILD SUCCESS, 205 tests (77 in `DefaultModelValidatorTest`), 0 failures.
    - `git diff --check` is clean.
    - I also stress-tested the singleton directly: 16 threads × 2000 unique models per round, 50 rounds, one shared `DefaultModelValidator`.
      - **Without the change:** 3 of 3 runs never finished and were killed after 60 s. A thread dump showed the threads spinning in `HashMap$TreeNode.balanceInsertion()` under `validateId()`. Short single-round runs threw the `ClassCastException` reported in #11618.
      - **With the change:** all 150 rounds complete in about 1 s with no failures.
    
    `testConcurrentValidation()` is probabilistic. In my runs a single iteration of that load hit the race about 1 time in 8, so it guards against the bug coming back but will not fail on every run against the unfixed code.
    
    - [x] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
    
    Refs #11618
---
 .../model/validation/DefaultModelValidator.java    |  7 ++-
 .../validation/DefaultModelValidatorTest.java      | 64 ++++++++++++++++++++++
 2 files changed, 69 insertions(+), 2 deletions(-)

diff --git a/maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java b/maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java
index bb8389d28d..687cdd58c1 100644
--- a/maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java
+++ b/maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java
@@ -34,6 +34,7 @@
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.function.Consumer;
 import java.util.function.Supplier;
 import java.util.regex.Matcher;
@@ -92,7 +93,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
+    private final Set<String> validIds = ConcurrentHashMap.newKeySet();
 
     private ModelVersionProcessor versionProcessor;
 
@@ -1159,7 +1162,7 @@ private boolean validateId(
             String id,
             String sourceHint,
             InputLocationTracker tracker) {
-        if (validIds.contains(id)) {
+        if (id != null && validIds.contains(id)) {
             return true;
         }
         if (!validateStringNotEmpty(prefix, fieldName, problems, severity, version, id, sourceHint, tracker)) {
diff --git a/maven-model-builder/src/test/java/org/apache/maven/model/validation/DefaultModelValidatorTest.java b/maven-model-builder/src/test/java/org/apache/maven/model/validation/DefaultModelValidatorTest.java
index a72946ed5e..e662a9d540 100644
--- a/maven-model-builder/src/test/java/org/apache/maven/model/validation/DefaultModelValidatorTest.java
+++ b/maven-model-builder/src/test/java/org/apache/maven/model/validation/DefaultModelValidatorTest.java
@@ -21,6 +21,9 @@
 import java.io.InputStream;
 import java.util.List;
 import java.util.Properties;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.UnaryOperator;
 
 import org.apache.maven.model.Model;
@@ -975,4 +978,65 @@ public void testOkParentRelativePath1() throws Exception {
         SimpleProblemCollector result = validateRaw("raw-model/ok-parent-relativePath1.xml");
         assertViolations(result, 0, 0, 0);
     }
+
+    /**
+     * 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
+    public void testConcurrentValidation() throws Exception {
+        int threadCount = 50;
+        int iterationsPerThread = 1000;
+        CountDownLatch startLatch = new CountDownLatch(1);
+        CountDownLatch doneLatch = new CountDownLatch(threadCount);
+        AtomicReference<Throwable> failure = new AtomicReference<>();
+
+        // Create multiple threads that will validate models concurrently
+        for (int t = 0; t < threadCount; t++) {
+            final int threadId = t;
+            Thread thread = new Thread(() -> {
+                try {
+                    startLatch.await(); // Wait for all threads to be ready
+                    for (int i = 0; i < iterationsPerThread; i++) {
+                        Model model = new Model();
+                        model.setModelVersion("4.0.0");
+                        model.setGroupId("test.group" + threadId);
+                        model.setArtifactId("test-artifact-" + threadId + "-" + i);
+                        model.setVersion("1.0.0");
+
+                        SimpleProblemCollector problems = new SimpleProblemCollector(model);
+                        validator.validateEffectiveModel(model, new DefaultModelBuildingRequest(), problems);
+                    }
+                } catch (Throwable e) {
+                    failure.compareAndSet(null, e);
+                } finally {
+                    doneLatch.countDown();
+                }
+            });
+            thread.setName("validator-test-" + threadId);
+            thread.setDaemon(true);
+            thread.start();
+        }
+
+        // Start all threads simultaneously
+        startLatch.countDown();
+
+        // Wait for all threads to complete
+        assertTrue(doneLatch.await(30, TimeUnit.SECONDS), "Threads did not complete in time");
+
+        // Check if any thread encountered an error
+        if (failure.get() != null) {
+            throw new AssertionError(
+                    "Concurrent validation failed: " + failure.get().getMessage(), failure.get());
+        }
+    }
 }