Register one script change listener per script instead of per error

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 {

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

src/main/java/org/cyclops/integratedscripting/evaluate/EvaluationExceptionResolutionHelpers.java

@@ -1,13 +1,19 @@
 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;
 
 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,60 +23,119 @@ public class EvaluationExceptionResolutionHelpers {
     // Holds weak references of created EvaluationExceptions
     private static final ReferenceQueue<? super EvaluationException> 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<Pair<Integer, Path>, ScriptExceptionsListener> LISTENERS = Maps.newHashMap();
+
     /**
      * Indicate that the given EvaluationException must be resolved when the given script is changed.
      * @param evaluationException An evaluation exception.
      * @param disk A script disk.
      * @param path A script path.
      * @return The given exception.
      */
-    public static EvaluationException resolveOnScriptChange(EvaluationException evaluationException, int disk, Path path) {
-        Wrapper<IScriptingData.IDiskScriptsChangeListener> 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<Integer, Path> 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;
     }
 
     /**
      * 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<Integer, Path> key;
+        private final Set<EvaluationExceptionReference> exceptions = Sets.newHashSet();
+
+        public ScriptExceptionsListener(Pair<Integer, Path> key) {
+            this.key = key;
+        }
+
+        public Pair<Integer, Path> 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<EvaluationExceptionReference> 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<EvaluationException> {
 
-        private final int disk;
-        private final Wrapper<IScriptingData.IDiskScriptsChangeListener> listener;
+        private final ScriptExceptionsListener listener;
 
         public EvaluationExceptionReference(
                 EvaluationException referent,
                 ReferenceQueue<? super EvaluationException> queue,
-                int disk,
-                Wrapper<IScriptingData.IDiskScriptsChangeListener> 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;
         }
     }
 

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<IScriptingData.IDiskScriptsChangeListener> 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<EvaluationException> 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<EvaluationException> 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<EvaluationException> exceptions = Lists.newArrayList();
+        List<Boolean> 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<IScriptingData.IDiskScriptsChangeListener> 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));
+    }
+}