(groovy) 02/02: GROOVY-12163: fix concurrent modification of ExpandoMetaClass mixinClasses (tweak test for improved robustness and less cost and fix a misleading comment)

[pa...@apache.org]

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

paulk-asert pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/groovy.git

commit 535be21d26e5073717db93fc055a09e7437837fa
Author: Paul King <pa...@asert.com.au>
AuthorDate: Tue Jul 14 07:50:17 2026 +1000

    GROOVY-12163: fix concurrent modification of ExpandoMetaClass mixinClasses (tweak test for improved robustness and less cost and fix a misleading comment)
---
 .../groovy/reflection/MixinInMetaClass.java        |  7 ++-
 .../ExpandoMetaClassMixinConcurrencyTest.groovy    | 62 +++++++++++++++-------
 2 files changed, 49 insertions(+), 20 deletions(-)

diff --git a/src/main/java/org/codehaus/groovy/reflection/MixinInMetaClass.java b/src/main/java/org/codehaus/groovy/reflection/MixinInMetaClass.java
index 50cdfec685..c031b91549 100644
--- a/src/main/java/org/codehaus/groovy/reflection/MixinInMetaClass.java
+++ b/src/main/java/org/codehaus/groovy/reflection/MixinInMetaClass.java
@@ -223,11 +223,16 @@ public class MixinInMetaClass {
 
     /**
      * Returns the hash code based on the mixin class.
+     * <p>
+     * {@code ExpandoMetaClass} dedupes mixins (GROOVY-11775) through a
+     * {@link java.util.concurrent.CopyOnWriteArraySet}, which compares with
+     * {@link #equals} alone, so this must stay consistent with it rather than
+     * being relied upon for that deduplication.
      *
      * @return the hash code
      */
     @Override
     public int hashCode() {
-        return mixinClass.hashCode(); // GROOVY-11775
+        return mixinClass.hashCode();
     }
 }
diff --git a/src/test/groovy/groovy/lang/ExpandoMetaClassMixinConcurrencyTest.groovy b/src/test/groovy/groovy/lang/ExpandoMetaClassMixinConcurrencyTest.groovy
index e76cddffa1..68dcb79d39 100644
--- a/src/test/groovy/groovy/lang/ExpandoMetaClassMixinConcurrencyTest.groovy
+++ b/src/test/groovy/groovy/lang/ExpandoMetaClassMixinConcurrencyTest.groovy
@@ -20,45 +20,56 @@ package groovy.lang
 
 import org.codehaus.groovy.reflection.MixinInMetaClass
 import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.Timeout
 
 import java.util.concurrent.CopyOnWriteArrayList
 import java.util.concurrent.CyclicBarrier
+import java.util.concurrent.TimeUnit
 import java.util.concurrent.atomic.AtomicBoolean
 
+import static org.junit.jupiter.api.Assertions.assertNotNull
 import static org.junit.jupiter.api.Assertions.assertTrue
 
 final class ExpandoMetaClassMixinConcurrencyTest {
 
+    private static final int CATEGORY_COUNT = 40
+    private static final int PRE_MIXED_COUNT = 8
+    private static final int READER_COUNT = 3
+    private static final int TIMEOUT_SECONDS = 30
+
     static class Target {}
 
     // Adding a mixin (addMixinClass) mutates the backing mixin set while method
     // dispatch iterates it (findMixinMethod). The set must tolerate that; a plain
-    // LinkedHashSet threw ConcurrentModificationException from the dispatching thread.
+    // LinkedHashSet threw ConcurrentModificationException in the dispatching thread.
     @Test
+    @Timeout(60)
     void mixinAddIsSafeAgainstConcurrentDispatch() {
         def loader = new GroovyClassLoader()
         // distinct category classes so every mixin is a fresh structural add
-        def categories = (0..<160).collect { i ->
-            loader.parseClass("class Cat_${i} { def catMethod_${i}() { 'x' } }", "Cat_${i}.groovy")
+        def categories = (0..<CATEGORY_COUNT).collect { i ->
+            loader.parseClass("class Cat_$i { def catMethod_$i() { 'x' } }", "Cat_${i}.groovy")
         }
 
         def emc = new ExpandoMetaClass(Target, false, true)
         emc.initialize()
 
-        // pre-grow the set so each iteration spans a wide window
-        (0..<40).each { MixinInMetaClass.mixinClassesToMetaClass(emc, [categories[it]]) }
+        // pre-populate so the readers always have something to iterate over
+        categories[0..<PRE_MIXED_COUNT].each { MixinInMetaClass.mixinClassesToMetaClass(emc, [it]) }
 
         def errors = new CopyOnWriteArrayList<Throwable>()
         def stop = new AtomicBoolean(false)
-        def emptyArgs = new Class[0]
-        def barrier = new CyclicBarrier(5)
+        def noArgs = new Class[0]
+        def barrier = new CyclicBarrier(READER_COUNT + 1)
 
-        def readers = (1..4).collect {
+        def readers = (1..READER_COUNT).collect {
             Thread.start {
                 try {
-                    barrier.await()
-                    while (!stop.get()) {
-                        emc.findMixinMethod('noSuchMixinMethod', emptyArgs)
+                    barrier.await(TIMEOUT_SECONDS, TimeUnit.SECONDS)
+                    // findMixinMethod never blocks, so the interrupt flag is the only
+                    // way out if the writer dies without setting stop
+                    while (!stop.get() && !Thread.currentThread().isInterrupted()) {
+                        emc.findMixinMethod('noSuchMixinMethod', noArgs)
                     }
                 } catch (Throwable t) {
                     errors << t
@@ -69,9 +80,9 @@ final class ExpandoMetaClassMixinConcurrencyTest {
         }
         def writer = Thread.start {
             try {
-                barrier.await()
-                for (int i = 40; i < categories.size() && !stop.get(); i++) {
-                    MixinInMetaClass.mixinClassesToMetaClass(emc, [categories[i]])
+                barrier.await(TIMEOUT_SECONDS, TimeUnit.SECONDS)
+                categories[PRE_MIXED_COUNT..<CATEGORY_COUNT].each {
+                    MixinInMetaClass.mixinClassesToMetaClass(emc, [it])
                 }
             } catch (Throwable t) {
                 errors << t
@@ -80,11 +91,24 @@ final class ExpandoMetaClassMixinConcurrencyTest {
             }
         }
 
-        writer.join()
-        stop.set(true)
-        readers*.join()
+        def threads = [writer, *readers]
+        threads.each { it.join(TimeUnit.SECONDS.toMillis(TIMEOUT_SECONDS)) }
+        def stuck = threads.findAll { it.alive }
+        if (!stuck.isEmpty()) {
+            stop.set(true) // release the readers, which spin rather than block
+            stuck*.interrupt()
+            stuck.each { it.join(TimeUnit.SECONDS.toMillis(1)) }
+        }
+        assertTrue(stuck.isEmpty(), "threads still running after ${TIMEOUT_SECONDS}s: $stuck")
 
-        assertTrue(errors.isEmpty(),
-                "concurrent mixin add during dispatch iteration threw: ${errors.isEmpty() ? '' : errors.first()}")
+        if (!errors.isEmpty()) {
+            throw new AssertionError(
+                    "concurrent mixin add during dispatch iteration threw: ${errors.first()}", errors.first())
+        }
+
+        // every concurrent add must still be observable, i.e. no lost updates
+        (0..<CATEGORY_COUNT).each { i ->
+            assertNotNull(emc.findMixinMethod("catMethod_$i", noArgs), "mixin $i was lost")
+        }
     }
 }