From ee5367d0b3617a6d75b4bb262cf48dbb0f39cab1 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 18:26:25 +0000 Subject: [PATCH] Register one script change listener per script instead of per error Every created script error registered its own script change listener, to resolve that error when the script changes. Those listeners were only removed once their exception was collected AND a new exception factory was created, which does not happen while a script variable holds on to its value, so a script that keeps failing accumulated a listener per failed evaluation. Removing them afterwards was quadratic on top of that, because each one was removed separately from a list: 10000 errors left 10000 listeners behind, which took 28ms to expunge. Exceptions are now collected per script, behind a single listener that resolves all of them when that script changes, and drops them one by one as they are collected. Registering a new exception expunges the collected ones first, so failing scripts no longer accumulate them at all. These listeners belong to the scripting data they were registered on, so they are also forgotten when the server stops. Mockito is bumped to 2.x, matching what newer branches already use, since 1.x uses cglib and cannot create mocks on the Java 17 toolchain. Related to #67 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JCW1HmWhLT27jh57t9d7B6 --- build.gradle | 2 +- .../IntegratedScripting.java | 2 + .../EvaluationExceptionResolutionHelpers.java | 109 ++++++++++++--- ...luationExceptionResolutionHelpersTest.java | 127 ++++++++++++++++++ 4 files changed, 217 insertions(+), 23 deletions(-) create mode 100644 src/test/java/org/cyclops/integratedscripting/evaluate/EvaluationExceptionResolutionHelpersTest.java diff --git a/build.gradle b/build.gradle index 16bf44cb5..ad0c639e2 100644 --- a/build.gradle +++ b/build.gradle @@ -122,7 +122,7 @@ dependencies { testAnnotationProcessor 'org.projectlombok:lombok:1.18.22' testImplementation "junit:junit:4.12" - testImplementation "org.mockito:mockito-core:1.+" + testImplementation "org.mockito:mockito-core:2.+" } minecraft { diff --git a/src/main/java/org/cyclops/integratedscripting/IntegratedScripting.java b/src/main/java/org/cyclops/integratedscripting/IntegratedScripting.java index 2bffdac29..2dbffebf3 100644 --- a/src/main/java/org/cyclops/integratedscripting/IntegratedScripting.java +++ b/src/main/java/org/cyclops/integratedscripting/IntegratedScripting.java @@ -39,6 +39,7 @@ import org.cyclops.integratedscripting.core.language.LanguageHandlerRegistry; import org.cyclops.integratedscripting.core.language.LanguageHandlers; import org.cyclops.integratedscripting.core.network.ScriptingData; +import org.cyclops.integratedscripting.evaluate.EvaluationExceptionResolutionHelpers; import org.cyclops.integratedscripting.evaluate.translation.ValueTranslatorRegistry; import org.cyclops.integratedscripting.evaluate.translation.ValueTranslators; import org.cyclops.integratedscripting.inventory.container.ContainerScriptingDriveConfig; @@ -116,6 +117,7 @@ protected void onServerStopping(ServerStoppingEvent event) { this.scriptingData.close(); } this.scriptingData = null; + EvaluationExceptionResolutionHelpers.reset(); } @SubscribeEvent diff --git a/src/main/java/org/cyclops/integratedscripting/evaluate/EvaluationExceptionResolutionHelpers.java b/src/main/java/org/cyclops/integratedscripting/evaluate/EvaluationExceptionResolutionHelpers.java index 398d33120..6c7fedda9 100644 --- a/src/main/java/org/cyclops/integratedscripting/evaluate/EvaluationExceptionResolutionHelpers.java +++ b/src/main/java/org/cyclops/integratedscripting/evaluate/EvaluationExceptionResolutionHelpers.java @@ -1,6 +1,9 @@ package org.cyclops.integratedscripting.evaluate; -import org.cyclops.cyclopscore.datastructure.Wrapper; +import com.google.common.collect.Lists; +import com.google.common.collect.Maps; +import com.google.common.collect.Sets; +import org.apache.commons.lang3.tuple.Pair; import org.cyclops.integrateddynamics.api.evaluate.EvaluationException; import org.cyclops.integratedscripting.api.network.IScriptingData; import org.cyclops.integratedscripting.core.network.ScriptingNetworkHelpers; @@ -8,6 +11,9 @@ import java.lang.ref.ReferenceQueue; import java.lang.ref.WeakReference; import java.nio.file.Path; +import java.util.Collection; +import java.util.Map; +import java.util.Set; /** * @author rubensworks @@ -17,6 +23,9 @@ public class EvaluationExceptionResolutionHelpers { // Holds weak references of created EvaluationExceptions private static final ReferenceQueue EVALUATION_EXCEPTION_REFERENCE_QUEUE = new ReferenceQueue<>(); + // Holds one script change listener per script, no matter how many exceptions must be resolved for it. + private static final Map, ScriptExceptionsListener> LISTENERS = Maps.newHashMap(); + /** * Indicate that the given EvaluationException must be resolved when the given script is changed. * @param evaluationException An evaluation exception. @@ -24,10 +33,21 @@ public class EvaluationExceptionResolutionHelpers { * @param path A script path. * @return The given exception. */ - public static EvaluationException resolveOnScriptChange(EvaluationException evaluationException, int disk, Path path) { - Wrapper listener = new Wrapper<>(); - listener.set(createListener(new EvaluationExceptionReference(evaluationException, EVALUATION_EXCEPTION_REFERENCE_QUEUE, disk, listener), disk, path)); - ScriptingNetworkHelpers.getScriptingData().addListener(disk, listener.get()); + public static synchronized EvaluationException resolveOnScriptChange(EvaluationException evaluationException, int disk, Path path) { + // Drop the exceptions that were collected since the last call, + // so that scripts that keep failing don't accumulate them. + expungeStaleEvaluationExceptions(); + + Pair key = Pair.of(disk, path); + ScriptExceptionsListener listener = LISTENERS.get(key); + if (listener == null) { + listener = new ScriptExceptionsListener(key); + LISTENERS.put(key, listener); + ScriptingNetworkHelpers.getScriptingData().addListener(disk, listener); + } + listener.addException(new EvaluationExceptionReference(evaluationException, + EVALUATION_EXCEPTION_REFERENCE_QUEUE, listener)); + return evaluationException; } @@ -35,42 +55,87 @@ public static EvaluationException resolveOnScriptChange(EvaluationException eval * Call this periodically to flush stale entries in * {@link EvaluationExceptionResolutionHelpers#EVALUATION_EXCEPTION_REFERENCE_QUEUE}. */ - public static void expungeStaleEvaluationExceptions() { + public static synchronized void expungeStaleEvaluationExceptions() { for (Object x; (x = EVALUATION_EXCEPTION_REFERENCE_QUEUE.poll()) != null; ) { - ((EvaluationExceptionReference) x).removeListener(); + EvaluationExceptionReference reference = (EvaluationExceptionReference) x; + reference.getListener().removeException(reference); } } - protected static IScriptingData.IDiskScriptsChangeListener createListener(EvaluationExceptionReference evaluationExceptionReference, int disk, Path path) { - return scriptPathRelative -> { - if (scriptPathRelative.equals(path)) { - EvaluationException exception = evaluationExceptionReference.get(); - if (exception != null) { - exception.resolve(); + /** + * Forget all pending exceptions and listeners. + * This must be called when the scripting data these listeners were registered on goes away. + */ + public static synchronized void reset() { + LISTENERS.clear(); + while (EVALUATION_EXCEPTION_REFERENCE_QUEUE.poll() != null) { + // Drop all pending references + } + } + + protected static synchronized void removeListener(ScriptExceptionsListener listener) { + if (LISTENERS.remove(listener.getKey()) != null) { + ScriptingNetworkHelpers.getScriptingData().removeListener(listener.getKey().getLeft(), listener); + } + } + + /** + * Resolves all exceptions that were created for one script once that script changes. + */ + public static class ScriptExceptionsListener implements IScriptingData.IDiskScriptsChangeListener { + + private final Pair key; + private final Set exceptions = Sets.newHashSet(); + + public ScriptExceptionsListener(Pair key) { + this.key = key; + } + + public Pair getKey() { + return key; + } + + public void addException(EvaluationExceptionReference reference) { + this.exceptions.add(reference); + } + + public void removeException(EvaluationExceptionReference reference) { + if (this.exceptions.remove(reference) && this.exceptions.isEmpty()) { + EvaluationExceptionResolutionHelpers.removeListener(this); + } + } + + @Override + public void onChange(Path scriptPathRelative) { + if (scriptPathRelative.equals(this.key.getRight())) { + Collection references = Lists.newArrayList(this.exceptions); + this.exceptions.clear(); + EvaluationExceptionResolutionHelpers.removeListener(this); + for (EvaluationExceptionReference reference : references) { + EvaluationException exception = reference.get(); + if (exception != null) { + exception.resolve(); + } } - ScriptingNetworkHelpers.getScriptingData().removeListener(disk, evaluationExceptionReference.listener.get()); } - }; + } } public static class EvaluationExceptionReference extends WeakReference { - private final int disk; - private final Wrapper listener; + private final ScriptExceptionsListener listener; public EvaluationExceptionReference( EvaluationException referent, ReferenceQueue queue, - int disk, - Wrapper listener + ScriptExceptionsListener listener ) { super(referent, queue); - this.disk = disk; this.listener = listener; } - public void removeListener() { - ScriptingNetworkHelpers.getScriptingData().removeListener(disk, listener.get()); + public ScriptExceptionsListener getListener() { + return listener; } } diff --git a/src/test/java/org/cyclops/integratedscripting/evaluate/EvaluationExceptionResolutionHelpersTest.java b/src/test/java/org/cyclops/integratedscripting/evaluate/EvaluationExceptionResolutionHelpersTest.java new file mode 100644 index 000000000..11a10757a --- /dev/null +++ b/src/test/java/org/cyclops/integratedscripting/evaluate/EvaluationExceptionResolutionHelpersTest.java @@ -0,0 +1,127 @@ +package org.cyclops.integratedscripting.evaluate; + +import com.google.common.collect.Lists; +import net.minecraft.network.chat.Component; +import org.cyclops.integrateddynamics.api.evaluate.EvaluationException; +import org.cyclops.integratedscripting.IntegratedScripting; +import org.cyclops.integratedscripting.api.evaluate.translation.IEvaluationExceptionFactory; +import org.cyclops.integratedscripting.api.network.IScriptingData; +import org.cyclops.integratedscripting.core.network.ScriptingData; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mockito; + +import java.nio.file.Path; +import java.util.List; +import java.util.concurrent.atomic.AtomicInteger; + +import static org.hamcrest.CoreMatchers.is; +import static org.junit.Assert.assertThat; + +/** + * @author rubensworks + */ +public class EvaluationExceptionResolutionHelpersTest { + + private static final Path PATH = Path.of("script.js"); + private static final Path PATH_OTHER = Path.of("other.js"); + + // Registered listeners outlive a test, so every test uses its own disks. + private static final AtomicInteger NEXT_DISK = new AtomicInteger(); + + private int disk; + private IntegratedScripting instanceOriginal; + private List listeners; + + @Before + public void before() { + this.disk = NEXT_DISK.getAndAdd(2); + this.listeners = Lists.newArrayList(); + + ScriptingData scriptingData = Mockito.mock(ScriptingData.class); + Mockito.doAnswer(invocation -> this.listeners.add(invocation.getArgument(1))) + .when(scriptingData).addListener(Mockito.anyInt(), Mockito.any()); + Mockito.doAnswer(invocation -> this.listeners.remove(invocation.getArgument(1))) + .when(scriptingData).removeListener(Mockito.anyInt(), Mockito.any()); + + IntegratedScripting mod = Mockito.mock(IntegratedScripting.class); + mod.scriptingData = scriptingData; + this.instanceOriginal = IntegratedScripting._instance; + IntegratedScripting._instance = mod; + } + + @After + public void after() { + IntegratedScripting._instance = this.instanceOriginal; + } + + @Test + public void testSingleListenerForManyExceptions() { + IEvaluationExceptionFactory factory = ScriptHelpers.getEvaluationExceptionFactory(this.disk, PATH, "member"); + + // Hold on to the exceptions, so that they can not be collected during this test. + List exceptions = Lists.newArrayList(); + for (int i = 0; i < 1000; i++) { + exceptions.add(factory.createError(Component.literal("error " + i))); + } + + assertThat(this.listeners.size(), is(1)); + } + + @Test + public void testListenerPerScript() { + List exceptions = Lists.newArrayList(); + for (int i = 0; i < 2; i++) { + exceptions.add(ScriptHelpers.getEvaluationExceptionFactory(this.disk, PATH, "member") + .createError(Component.literal("a"))); + exceptions.add(ScriptHelpers.getEvaluationExceptionFactory(this.disk, PATH_OTHER, "member") + .createError(Component.literal("b"))); + exceptions.add(ScriptHelpers.getEvaluationExceptionFactory(this.disk + 1, PATH, "member") + .createError(Component.literal("c"))); + } + + assertThat(this.listeners.size(), is(3)); + } + + @Test + public void testExceptionsAreResolvedOnScriptChange() { + IEvaluationExceptionFactory factory = ScriptHelpers.getEvaluationExceptionFactory(this.disk, PATH, "member"); + List exceptions = Lists.newArrayList(); + List resolved = Lists.newArrayList(); + for (int i = 0; i < 3; i++) { + EvaluationException exception = factory.createError(Component.literal("error " + i)); + int index = i; + resolved.add(false); + exception.addResolutionListeners(() -> resolved.set(index, true)); + exceptions.add(exception); + } + + // Changes to other scripts don't resolve anything. + List listenersBefore = Lists.newArrayList(this.listeners); + listenersBefore.forEach(listener -> listener.onChange(PATH_OTHER)); + assertThat(resolved, is(Lists.newArrayList(false, false, false))); + + listenersBefore.forEach(listener -> listener.onChange(PATH)); + assertThat(resolved, is(Lists.newArrayList(true, true, true))); + + // The listeners are not needed anymore once the script changed. + assertThat(this.listeners.isEmpty(), is(true)); + } + + @Test + public void testListenersAreRemovedAfterExceptionsAreCollected() throws InterruptedException { + IEvaluationExceptionFactory factory = ScriptHelpers.getEvaluationExceptionFactory(this.disk, PATH, "member"); + for (int i = 0; i < 100; i++) { + factory.createError(Component.literal("error " + i)); + } + assertThat(this.listeners.size(), is(1)); + + for (int i = 0; i < 20 && !this.listeners.isEmpty(); i++) { + System.gc(); + Thread.sleep(50); + EvaluationExceptionResolutionHelpers.expungeStaleEvaluationExceptions(); + } + assertThat(this.listeners.isEmpty(), is(true)); + } +}