diff --git a/src/main/java/org/cyclops/integratedscripting/IntegratedScripting.java b/src/main/java/org/cyclops/integratedscripting/IntegratedScripting.java index 534c5f4f1..aa8bdc318 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.gametest.GameTestsAdvancements; @@ -125,6 +126,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..84f04941a --- /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.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.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.hamcrest.MatcherAssert.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; + + @BeforeEach + public void beforeEach() { + 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; + } + + @AfterEach + public void afterEach() { + 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)); + } +}