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 <[email protected]> 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") + } } }
