From 9b9565dbbdb23046728d63fccb3564a77de6e85f Mon Sep 17 00:00:00 2001 From: jglrxavpok Date: Tue, 2 Feb 2021 12:44:36 +0100 Subject: [PATCH 01/12] Early loading of Mixin and code modifiers + System property to disable early loading if necessary --- .../java/net/minestom/server/Bootstrap.java | 3 ++ .../server/extensions/ExtensionManager.java | 37 ++++++++++++++++++- .../MinestomRootClassLoader.java | 9 +++++ .../improveextensions/DisableEarlyLoad.java | 34 +++++++++++++++++ .../MixinIntoMinestomCore.java | 37 +++++++++++++++++++ .../MixinIntoMinestomCoreLauncher.java | 15 ++++++++ .../mixins/InstanceContainerMixin.java | 19 ++++++++++ .../disableearlyload/extension.json | 6 +++ .../disableearlyload/mixins.minestomcore.json | 10 +++++ .../improveextensions/extension.json | 6 +++ .../mixins.minestomcore.json | 10 +++++ 11 files changed, 185 insertions(+), 1 deletion(-) create mode 100644 src/test/java/improveextensions/DisableEarlyLoad.java create mode 100644 src/test/java/improveextensions/MixinIntoMinestomCore.java create mode 100644 src/test/java/improveextensions/MixinIntoMinestomCoreLauncher.java create mode 100644 src/test/java/improveextensions/mixins/InstanceContainerMixin.java create mode 100644 src/test/resources/improveextensions/disableearlyload/extension.json create mode 100644 src/test/resources/improveextensions/disableearlyload/mixins.minestomcore.json create mode 100644 src/test/resources/improveextensions/extension.json create mode 100644 src/test/resources/improveextensions/mixins.minestomcore.json diff --git a/src/main/java/net/minestom/server/Bootstrap.java b/src/main/java/net/minestom/server/Bootstrap.java index 5939923f7..3e5b26455 100644 --- a/src/main/java/net/minestom/server/Bootstrap.java +++ b/src/main/java/net/minestom/server/Bootstrap.java @@ -1,5 +1,6 @@ package net.minestom.server; +import net.minestom.server.extensions.ExtensionManager; import net.minestom.server.extras.selfmodification.MinestomRootClassLoader; import net.minestom.server.extras.selfmodification.mixins.MixinCodeModifier; import net.minestom.server.extras.selfmodification.mixins.MixinServiceMinestom; @@ -22,6 +23,8 @@ public final class Bootstrap { startMixin(args); MinestomRootClassLoader.getInstance().addCodeModifier(new MixinCodeModifier()); + ExtensionManager.loadCodeModifiersEarly(); + MixinServiceMinestom.gotoPreinitPhase(); // ensure extensions are loaded when starting the server Class serverClass = classLoader.loadClass("net.minestom.server.MinecraftServer"); diff --git a/src/main/java/net/minestom/server/extensions/ExtensionManager.java b/src/main/java/net/minestom/server/extensions/ExtensionManager.java index e7f81be86..edfdfec97 100644 --- a/src/main/java/net/minestom/server/extensions/ExtensionManager.java +++ b/src/main/java/net/minestom/server/extensions/ExtensionManager.java @@ -27,6 +27,8 @@ import java.util.zip.ZipFile; public class ExtensionManager { + public final static String DISABLE_EARLY_LOAD_SYSTEM_KEY = "minestom.extension.disable_early_load"; + public final static Logger LOGGER = LoggerFactory.getLogger(ExtensionManager.class); private final static String INDEV_CLASSES_FOLDER = "minestom.extension.indevfolder.classes"; @@ -469,7 +471,7 @@ public class ExtensionManager { } @NotNull - public Map getExtensionLoaders() { + public Map getExtensionLoaders() { return new HashMap<>(extensionLoaders); } @@ -485,6 +487,10 @@ public class ExtensionManager { return; } MinestomRootClassLoader modifiableClassLoader = (MinestomRootClassLoader) cl; + setupCodeModifiers(extensions, modifiableClassLoader); + } + + private void setupCodeModifiers(@NotNull List extensions, MinestomRootClassLoader modifiableClassLoader) { LOGGER.info("Start loading code modifiers..."); for (DiscoveredExtension extension : extensions) { try { @@ -664,4 +670,33 @@ public class ExtensionManager { public void shutdown() { this.extensionList.forEach(this::unload); } + + /** + * Loads code modifiers early, that is before MinecraftServer.init() is called. + */ + public static void loadCodeModifiersEarly() { + // allow users to disable early code modifier load + if("true".equalsIgnoreCase(System.getProperty(DISABLE_EARLY_LOAD_SYSTEM_KEY))) { + return; + } + LOGGER.info("Early load of code modifiers from extensions."); + ExtensionManager manager = new ExtensionManager(); + + // discover extensions that are present + List discovered = manager.discoverExtensions(); + + // setup extension class loaders, so that Mixin can load the json configuration file correctly + for(DiscoveredExtension e : discovered) { + manager.setupClassLoader(e); + } + + // setup code modifiers and mixins + manager.setupCodeModifiers(discovered, MinestomRootClassLoader.getInstance()); + + // setup is done, remove all extension classloaders + for(MinestomExtensionClassLoader extensionLoader : manager.getExtensionLoaders().values()) { + MinestomRootClassLoader.getInstance().removeChildInHierarchy(extensionLoader); + } + LOGGER.info("Early load of code modifiers from extensions done!"); + } } diff --git a/src/main/java/net/minestom/server/extras/selfmodification/MinestomRootClassLoader.java b/src/main/java/net/minestom/server/extras/selfmodification/MinestomRootClassLoader.java index bcb9d4232..c80df5528 100644 --- a/src/main/java/net/minestom/server/extras/selfmodification/MinestomRootClassLoader.java +++ b/src/main/java/net/minestom/server/extras/selfmodification/MinestomRootClassLoader.java @@ -65,6 +65,11 @@ public class MinestomRootClassLoader extends HierarchyClassLoader { // TODO: priorities? private final List modifiers = new LinkedList<>(); + /** + * List of already loaded code modifier class names. This prevents loading the same class twice. + */ + private final Set alreadyLoadedCodeModifiers = new HashSet<>(); + private MinestomRootClassLoader(ClassLoader parent) { super("Minestom Root ClassLoader", extractURLsFromClasspath(), parent); asmClassLoader = newChild(new URL[0]); @@ -246,6 +251,9 @@ public class MinestomRootClassLoader extends HierarchyClassLoader { } public void loadModifier(File[] originFiles, String codeModifierClass) { + if(alreadyLoadedCodeModifiers.contains(codeModifierClass)) { + return; + } URL[] urls = new URL[originFiles.length]; try { for (int i = 0; i < originFiles.length; i++) { @@ -258,6 +266,7 @@ public class MinestomRootClassLoader extends HierarchyClassLoader { synchronized (modifiers) { LOGGER.warn("Added Code modifier: {}", modifier); addCodeModifier(modifier); + alreadyLoadedCodeModifiers.add(codeModifierClass); } } } catch (MalformedURLException | ClassNotFoundException | InvocationTargetException | InstantiationException | IllegalAccessException | NoSuchMethodException e) { diff --git a/src/test/java/improveextensions/DisableEarlyLoad.java b/src/test/java/improveextensions/DisableEarlyLoad.java new file mode 100644 index 000000000..a36ad8182 --- /dev/null +++ b/src/test/java/improveextensions/DisableEarlyLoad.java @@ -0,0 +1,34 @@ +package improveextensions; + +import net.minestom.server.MinecraftServer; +import net.minestom.server.extensions.Extension; +import net.minestom.server.instance.InstanceContainer; +import net.minestom.server.world.DimensionType; +import org.junit.jupiter.api.Assertions; +import org.opentest4j.AssertionFailedError; + +import java.util.UUID; + +/** + * Extensions should be able to use Mixins for classes loaded very early by Minestom (InstanceContainer for instance) + */ +public class DisableEarlyLoad extends Extension { + + @Override + public void initialize() { + // force load of InstanceContainer class + InstanceContainer c = new InstanceContainer(UUID.randomUUID(), DimensionType.OVERWORLD, null); + System.out.println(c.toString()); + try { + Assertions.assertFalse(MixinIntoMinestomCore.success, "InstanceContainer must NOT have been mixed in with improveextensions.InstanceContainerMixin, because early loading has been disabled"); + } catch (AssertionFailedError e) { + e.printStackTrace(); + } + MinecraftServer.stopCleanly(); + } + + @Override + public void terminate() { + getLogger().info("Terminate extension"); + } +} diff --git a/src/test/java/improveextensions/MixinIntoMinestomCore.java b/src/test/java/improveextensions/MixinIntoMinestomCore.java new file mode 100644 index 000000000..131734e91 --- /dev/null +++ b/src/test/java/improveextensions/MixinIntoMinestomCore.java @@ -0,0 +1,37 @@ +package improveextensions; + +import net.minestom.server.MinecraftServer; +import net.minestom.server.extensions.Extension; +import net.minestom.server.instance.InstanceContainer; +import net.minestom.server.world.DimensionType; +import org.junit.jupiter.api.Assertions; +import org.opentest4j.AssertionFailedError; + +import java.util.UUID; + +/** + * Extensions should be able to use Mixins for classes loaded very early by Minestom (InstanceContainer for instance) + */ +public class MixinIntoMinestomCore extends Extension { + + public static boolean success = false; + + @Override + public void initialize() { + // force load of InstanceContainer class + InstanceContainer c = new InstanceContainer(UUID.randomUUID(), DimensionType.OVERWORLD, null); + System.out.println(c.toString()); + try { + Assertions.assertTrue(success, "InstanceContainer must have been mixed in with improveextensions.InstanceContainerMixin"); + Assertions.assertEquals(1, MinecraftServer.getExtensionManager().getExtensionLoaders().size(), "Only one extension classloader (this extension's) must be active."); + } catch (AssertionFailedError e) { + e.printStackTrace(); + } + MinecraftServer.stopCleanly(); + } + + @Override + public void terminate() { + getLogger().info("Terminate extension"); + } +} diff --git a/src/test/java/improveextensions/MixinIntoMinestomCoreLauncher.java b/src/test/java/improveextensions/MixinIntoMinestomCoreLauncher.java new file mode 100644 index 000000000..eb1a436a4 --- /dev/null +++ b/src/test/java/improveextensions/MixinIntoMinestomCoreLauncher.java @@ -0,0 +1,15 @@ +package improveextensions; + +import net.minestom.server.Bootstrap; + +// To launch with VM arguments: + +// To test early Mixin injections: +// -Dminestom.extension.indevfolder.classes=build/classes/java/test/ -Dminestom.extension.indevfolder.resources=build/resources/test/improveextensions +// To test disabling early Mixin injections: +// -Dminestom.extension.disable_early_load=true -Dminestom.extension.indevfolder.classes=build/classes/java/test/ -Dminestom.extension.indevfolder.resources=build/resources/test/improveextensions/disableearlyload +public class MixinIntoMinestomCoreLauncher { + public static void main(String[] args) { + Bootstrap.bootstrap("demo.MainDemo", args); + } +} diff --git a/src/test/java/improveextensions/mixins/InstanceContainerMixin.java b/src/test/java/improveextensions/mixins/InstanceContainerMixin.java new file mode 100644 index 000000000..f34ec9bcd --- /dev/null +++ b/src/test/java/improveextensions/mixins/InstanceContainerMixin.java @@ -0,0 +1,19 @@ +package improveextensions.mixins; + +import improveextensions.MixinIntoMinestomCore; +import net.minestom.server.instance.InstanceContainer; +import org.spongepowered.asm.mixin.Mixin; +import org.spongepowered.asm.mixin.injection.At; +import org.spongepowered.asm.mixin.injection.Inject; +import org.spongepowered.asm.mixin.injection.callback.CallbackInfo; + +@Mixin(InstanceContainer.class) +public class InstanceContainerMixin { + + @Inject(method = "", at = @At("RETURN"), require = 1) + private void constructorHead(CallbackInfo ci) { + System.out.println("Mixin into InstanceContainerMixin"); + MixinIntoMinestomCore.success = true; + } + +} diff --git a/src/test/resources/improveextensions/disableearlyload/extension.json b/src/test/resources/improveextensions/disableearlyload/extension.json new file mode 100644 index 000000000..c26927c24 --- /dev/null +++ b/src/test/resources/improveextensions/disableearlyload/extension.json @@ -0,0 +1,6 @@ +{ + "entrypoint": "improveextensions.DisableEarlyLoad", + "name": "DisableEarlyLoad", + "codeModifiers": [], + "mixinConfig": "mixins.minestomcore.json" +} \ No newline at end of file diff --git a/src/test/resources/improveextensions/disableearlyload/mixins.minestomcore.json b/src/test/resources/improveextensions/disableearlyload/mixins.minestomcore.json new file mode 100644 index 000000000..7c8c03fe2 --- /dev/null +++ b/src/test/resources/improveextensions/disableearlyload/mixins.minestomcore.json @@ -0,0 +1,10 @@ +{ + "required": true, + "minVersion": "0.8", + "package": "improveextensions.mixins", + "target": "@env(DEFAULT)", + "compatibilityLevel": "JAVA_11", + "mixins": [ + "InstanceContainerMixin" + ] +} \ No newline at end of file diff --git a/src/test/resources/improveextensions/extension.json b/src/test/resources/improveextensions/extension.json new file mode 100644 index 000000000..abbe3eb02 --- /dev/null +++ b/src/test/resources/improveextensions/extension.json @@ -0,0 +1,6 @@ +{ + "entrypoint": "improveextensions.MixinIntoMinestomCore", + "name": "MixinIntoMinestomCore", + "codeModifiers": [], + "mixinConfig": "mixins.minestomcore.json" +} \ No newline at end of file diff --git a/src/test/resources/improveextensions/mixins.minestomcore.json b/src/test/resources/improveextensions/mixins.minestomcore.json new file mode 100644 index 000000000..7c8c03fe2 --- /dev/null +++ b/src/test/resources/improveextensions/mixins.minestomcore.json @@ -0,0 +1,10 @@ +{ + "required": true, + "minVersion": "0.8", + "package": "improveextensions.mixins", + "target": "@env(DEFAULT)", + "compatibilityLevel": "JAVA_11", + "mixins": [ + "InstanceContainerMixin" + ] +} \ No newline at end of file From 2b5d67a3ca2415ebf447a63c4e974ea68811a2e0 Mon Sep 17 00:00:00 2001 From: jglrxavpok Date: Tue, 2 Feb 2021 14:49:19 +0100 Subject: [PATCH 02/12] Don't crash the server if Mixin can't be loaded due to identical file names inside the JAR file. But please spam the console. --- .../java/net/minestom/server/Bootstrap.java | 19 +++++++++++++++++-- .../server/extensions/ExtensionManager.java | 10 ++++++++-- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/src/main/java/net/minestom/server/Bootstrap.java b/src/main/java/net/minestom/server/Bootstrap.java index 3e5b26455..2913af492 100644 --- a/src/main/java/net/minestom/server/Bootstrap.java +++ b/src/main/java/net/minestom/server/Bootstrap.java @@ -7,6 +7,7 @@ import net.minestom.server.extras.selfmodification.mixins.MixinServiceMinestom; import org.spongepowered.asm.launch.MixinBootstrap; import org.spongepowered.asm.launch.platform.CommandLineOptions; import org.spongepowered.asm.mixin.Mixins; +import org.spongepowered.asm.service.ServiceNotAvailableError; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; @@ -21,7 +22,12 @@ public final class Bootstrap { try { ClassLoader classLoader = MinestomRootClassLoader.getInstance(); startMixin(args); - MinestomRootClassLoader.getInstance().addCodeModifier(new MixinCodeModifier()); + try { + MinestomRootClassLoader.getInstance().addCodeModifier(new MixinCodeModifier()); + } catch (RuntimeException e) { + e.printStackTrace(); + System.err.println("Failed to add MixinCodeModifier, mixins will not be injected. Check the log entries above to debug."); + } ExtensionManager.loadCodeModifiersEarly(); @@ -47,7 +53,16 @@ public final class Bootstrap { // hacks required to pass custom arguments Method start = MixinBootstrap.class.getDeclaredMethod("start"); start.setAccessible(true); - if (!((boolean) start.invoke(null))) { + try { + if (!((boolean) start.invoke(null))) { + return; + } + } catch (ServiceNotAvailableError e) { + e.printStackTrace(); + System.err.println("Failed to load Mixin, see error above."); + System.err.println("It is possible you simply have two files with identical names inside your server jar. " + + "Check your META-INF/services directory inside your Minestom implementation and merge files with identical names inside META-INF/services."); + return; } diff --git a/src/main/java/net/minestom/server/extensions/ExtensionManager.java b/src/main/java/net/minestom/server/extensions/ExtensionManager.java index edfdfec97..9a98c27c8 100644 --- a/src/main/java/net/minestom/server/extensions/ExtensionManager.java +++ b/src/main/java/net/minestom/server/extensions/ExtensionManager.java @@ -13,6 +13,7 @@ import org.jetbrains.annotations.Nullable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.spongepowered.asm.mixin.Mixins; +import org.spongepowered.asm.service.ServiceNotAvailableError; import java.io.*; import java.lang.reflect.Constructor; @@ -499,8 +500,13 @@ public class ExtensionManager { } if (!extension.getMixinConfig().isEmpty()) { final String mixinConfigFile = extension.getMixinConfig(); - Mixins.addConfiguration(mixinConfigFile); - LOGGER.info("Found mixin in extension {}: {}", extension.getName(), mixinConfigFile); + try { + Mixins.addConfiguration(mixinConfigFile); + LOGGER.info("Found mixin in extension {}: {}", extension.getName(), mixinConfigFile); + } catch (ServiceNotAvailableError e) { + MinecraftServer.getExceptionManager().handleException(e); + LOGGER.error("Could not load Mixin configuration: "+mixinConfigFile); + } } } catch (Exception e) { MinecraftServer.getExceptionManager().handleException(e); From eadd4a2b399359559dd6be3feac2278ff2e05908 Mon Sep 17 00:00:00 2001 From: jglrxavpok Date: Wed, 3 Feb 2021 17:11:06 +0100 Subject: [PATCH 03/12] Automatically unload GlobalEventHandler callbacks --- .../net/minestom/server/entity/Entity.java | 7 +++ .../server/event/GlobalEventHandler.java | 8 +++ .../server/event/handler/EventHandler.java | 60 +++++++++++++++++++ .../server/extensions/ExtensionManager.java | 10 +++- .../MinestomExtensionClassLoader.java | 41 ++++++++++++- .../MinestomRootClassLoader.java | 25 ++++++++ .../minestom/server/instance/Instance.java | 8 +++ .../UnloadCallbacksExtension.java | 57 ++++++++++++++++++ .../unloadcallbacks/extension.json | 4 ++ 9 files changed, 215 insertions(+), 5 deletions(-) create mode 100644 src/test/java/improveextensions/unloadcallbacks/UnloadCallbacksExtension.java create mode 100644 src/test/resources/improveextensions/unloadcallbacks/extension.json diff --git a/src/main/java/net/minestom/server/entity/Entity.java b/src/main/java/net/minestom/server/entity/Entity.java index 171c1cae2..298df01d5 100644 --- a/src/main/java/net/minestom/server/entity/Entity.java +++ b/src/main/java/net/minestom/server/entity/Entity.java @@ -120,6 +120,7 @@ public abstract class Entity implements Viewable, EventHandler, DataContainer, P // Events private final Map, Collection> eventCallbacks = new ConcurrentHashMap<>(); + private final Map>> extensionCallbacks = new ConcurrentHashMap<>(); protected Metadata metadata = new Metadata(this); @@ -640,6 +641,12 @@ public abstract class Entity implements Viewable, EventHandler, DataContainer, P return eventCallbacks; } + @NotNull + @Override + public Collection> getExtensionCallbacks(String extension) { + return extensionCallbacks.computeIfAbsent(extension, e -> new CopyOnWriteArrayList<>()); + } + /** * Each entity has an unique id (server-wide) which will change after a restart. * diff --git a/src/main/java/net/minestom/server/event/GlobalEventHandler.java b/src/main/java/net/minestom/server/event/GlobalEventHandler.java index dd9da088c..69c2d12b4 100644 --- a/src/main/java/net/minestom/server/event/GlobalEventHandler.java +++ b/src/main/java/net/minestom/server/event/GlobalEventHandler.java @@ -6,6 +6,7 @@ import org.jetbrains.annotations.NotNull; import java.util.Collection; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArrayList; /** * Object containing all the global event listeners. @@ -14,10 +15,17 @@ public final class GlobalEventHandler implements EventHandler { // Events private final Map, Collection> eventCallbacks = new ConcurrentHashMap<>(); + private final Map>> extensionCallbacks = new ConcurrentHashMap<>(); @NotNull @Override public Map, Collection> getEventCallbacksMap() { return eventCallbacks; } + + @NotNull + @Override + public Collection> getExtensionCallbacks(String extension) { + return extensionCallbacks.computeIfAbsent(extension, e -> new CopyOnWriteArrayList<>()); + } } diff --git a/src/main/java/net/minestom/server/event/handler/EventHandler.java b/src/main/java/net/minestom/server/event/handler/EventHandler.java index 73a583ff4..37a4acc90 100644 --- a/src/main/java/net/minestom/server/event/handler/EventHandler.java +++ b/src/main/java/net/minestom/server/event/handler/EventHandler.java @@ -6,12 +6,16 @@ import net.minestom.server.event.CancellableEvent; import net.minestom.server.event.Event; import net.minestom.server.event.EventCallback; import net.minestom.server.event.GlobalEventHandler; +import net.minestom.server.extensions.ExtensionManager; +import net.minestom.server.extras.selfmodification.MinestomExtensionClassLoader; +import net.minestom.server.extras.selfmodification.MinestomRootClassLoader; import net.minestom.server.instance.Instance; import net.minestom.server.utils.validate.Check; import org.jetbrains.annotations.NotNull; import java.util.Collection; import java.util.Map; +import java.util.Optional; import java.util.concurrent.CopyOnWriteArraySet; import java.util.stream.Stream; @@ -28,6 +32,38 @@ public interface EventHandler { @NotNull Map, Collection> getEventCallbacksMap(); + /** + * Gets a {@link Collection} containing all the listeners assigned to a specific extension (represented by its name). + * Used to unload all callbacks when the extension is unloaded + * + * @return a {@link Collection} with all the listeners + */ + @NotNull + Collection> getExtensionCallbacks(String extension); + + /** + * Tries to know which extension created this callback, based on the classloader of the callback + * @param callback the callback to get the extension of + * @return Optional.empty() if no extension has been found, Optional.of(<name>) with 'name' being the extension name + */ + static Optional getExtensionOwningCallback(@NotNull EventCallback callback) { + ClassLoader cl = callback.getClass().getClassLoader(); + if(cl instanceof MinestomExtensionClassLoader) { + return Optional.of(((MinestomExtensionClassLoader) cl).getExtensionName()); + } else if(System.getProperty(ExtensionManager.INDEV_CLASSES_FOLDER) != null) { // in a dev environment, the extension will always be loaded with the root classloader + StackTraceElement[] stackTrace = Thread.currentThread().getStackTrace(); + // 0 -> getStackTrace + // 1 -> getExtensionOwningCallback + // 2 -> add/remove EventCallback + // 3 -> Potentially the extension + if(stackTrace.length >= 4) { + StackTraceElement potentialExtensionCall = stackTrace[3]; + System.out.println(potentialExtensionCall.getClassLoaderName()); + } + } + return Optional.empty(); + } + /** * Adds a new event callback for the specified type {@code eventClass}. * @@ -37,6 +73,9 @@ public interface EventHandler { * @return true if the callback collection changed as a result of the call */ default boolean addEventCallback(@NotNull Class eventClass, @NotNull EventCallback eventCallback) { + Optional extensionSource = getExtensionOwningCallback(eventCallback); + extensionSource.ifPresent(s -> getExtensionCallbacks(s).add(eventCallback)); + Collection callbacks = getEventCallbacks(eventClass); return callbacks.add(eventCallback); } @@ -51,6 +90,9 @@ public interface EventHandler { */ default boolean removeEventCallback(@NotNull Class eventClass, @NotNull EventCallback eventCallback) { Collection callbacks = getEventCallbacks(eventClass); + Optional extensionSource = getExtensionOwningCallback(eventCallback); + extensionSource.ifPresent(s -> getExtensionCallbacks(s).remove(eventCallback)); + return callbacks.remove(eventCallback); } @@ -126,6 +168,24 @@ public interface EventHandler { } } + /** + * Remove all event callbacks owned by the given extension + * @param extension the extension to remove callbacks from + */ + default void removeCallbacksOwnedByExtension(String extension) { + Collection> extensionCallbacks = getExtensionCallbacks(extension); + for(EventCallback callback : extensionCallbacks) { + + // try to remove this callback from all callback collections + // we do this because we do not have information about the event class at this point + for(Collection eventCallbacks : getEventCallbacksMap().values()) { + eventCallbacks.remove(callback); + } + } + + extensionCallbacks.clear(); + } + private void runEvent(@NotNull Collection eventCallbacks, @NotNull E event) { for (EventCallback eventCallback : eventCallbacks) { eventCallback.run(event); diff --git a/src/main/java/net/minestom/server/extensions/ExtensionManager.java b/src/main/java/net/minestom/server/extensions/ExtensionManager.java index 9a98c27c8..bd0ab2359 100644 --- a/src/main/java/net/minestom/server/extensions/ExtensionManager.java +++ b/src/main/java/net/minestom/server/extensions/ExtensionManager.java @@ -32,8 +32,8 @@ public class ExtensionManager { public final static Logger LOGGER = LoggerFactory.getLogger(ExtensionManager.class); - private final static String INDEV_CLASSES_FOLDER = "minestom.extension.indevfolder.classes"; - private final static String INDEV_RESOURCES_FOLDER = "minestom.extension.indevfolder.resources"; + public final static String INDEV_CLASSES_FOLDER = "minestom.extension.indevfolder.classes"; + public final static String INDEV_RESOURCES_FOLDER = "minestom.extension.indevfolder.resources"; private final static Gson GSON = new Gson(); private final Map extensionLoaders = new HashMap<>(); @@ -432,7 +432,7 @@ public class ExtensionManager { @NotNull public MinestomExtensionClassLoader newClassLoader(@NotNull DiscoveredExtension extension, @NotNull URL[] urls) { MinestomRootClassLoader root = MinestomRootClassLoader.getInstance(); - MinestomExtensionClassLoader loader = new MinestomExtensionClassLoader(extension.getName(), urls, root); + MinestomExtensionClassLoader loader = new MinestomExtensionClassLoader(extension.getName(), extension.getEntrypoint(), urls, root); if (extension.getDependencies().length == 0) { // orphaned extension, we can insert it directly root.addChild(loader); @@ -523,6 +523,10 @@ public class ExtensionManager { private void unload(Extension ext) { ext.preTerminate(); ext.terminate(); + // remove callbacks for this extension + MinecraftServer.getGlobalEventHandler().removeCallbacksOwnedByExtension(ext.getDescription().getName()); + // TODO: more callback types + ext.postTerminate(); ext.unload(); diff --git a/src/main/java/net/minestom/server/extras/selfmodification/MinestomExtensionClassLoader.java b/src/main/java/net/minestom/server/extras/selfmodification/MinestomExtensionClassLoader.java index 6afbee103..24557d18b 100644 --- a/src/main/java/net/minestom/server/extras/selfmodification/MinestomExtensionClassLoader.java +++ b/src/main/java/net/minestom/server/extras/selfmodification/MinestomExtensionClassLoader.java @@ -10,9 +10,34 @@ public class MinestomExtensionClassLoader extends HierarchyClassLoader { */ private final MinestomRootClassLoader root; - public MinestomExtensionClassLoader(String name, URL[] urls, MinestomRootClassLoader root) { - super(name, urls, root); + /** + * Main of the main class of the extension linked to this classloader + */ + private final String mainClassName; + + public MinestomExtensionClassLoader(String extensionName, String mainClassName, URL[] urls, MinestomRootClassLoader root) { + super(extensionName, urls, root); this.root = root; + this.mainClassName = mainClassName; + } + + /** + * Returns the name of the extension linked to this classloader + * @return the name of the extension linked to this classloader + */ + public String getExtensionName() { + // simply calls ClassLoader#getName as the extension name is used to name this classloader + // this method is simply for ease-of-use + return getName(); + } + + /** + * Returns the main class name linked to the extension responsible for this classloader. + * Used by the root classloader to let extensions load themselves in a dev environment. + * @return the main class name linked to the extension responsible for this classloader + */ + public String getMainClassName() { + return mainClassName; } @Override @@ -72,4 +97,16 @@ public class MinestomExtensionClassLoader extends HierarchyClassLoader { super.finalize(); System.err.println("Class loader "+getName()+" finalized."); } + + /** + * Is the given class name the name of the entry point of one the extensions from this classloader chain? + * @param name the class name to check + * @return whether the given class name the name of the entry point of one the extensions from this classloader chain + * @see MinestomRootClassLoader#loadBytes(String, boolean) for more information + */ + protected boolean isMainExtensionClass(String name) { + if(mainClassName.equals(name)) + return true; + return children.stream().anyMatch(c -> c.isMainExtensionClass(name)); + } } diff --git a/src/main/java/net/minestom/server/extras/selfmodification/MinestomRootClassLoader.java b/src/main/java/net/minestom/server/extras/selfmodification/MinestomRootClassLoader.java index c80df5528..421566d5f 100644 --- a/src/main/java/net/minestom/server/extras/selfmodification/MinestomRootClassLoader.java +++ b/src/main/java/net/minestom/server/extras/selfmodification/MinestomRootClassLoader.java @@ -1,6 +1,7 @@ package net.minestom.server.extras.selfmodification; import net.minestom.server.MinecraftServer; +import net.minestom.server.extensions.ExtensionManager; import org.jetbrains.annotations.NotNull; import org.objectweb.asm.ClassReader; import org.objectweb.asm.ClassWriter; @@ -13,6 +14,7 @@ import java.io.IOException; import java.io.InputStream; import java.lang.reflect.InvocationTargetException; import java.net.MalformedURLException; +import java.net.URISyntaxException; import java.net.URL; import java.net.URLClassLoader; import java.util.HashSet; @@ -65,6 +67,12 @@ public class MinestomRootClassLoader extends HierarchyClassLoader { // TODO: priorities? private final List modifiers = new LinkedList<>(); + /** + * Whether Minestom detected that it is running in a dev environment. + * Determined by the existence of the system property {@link ExtensionManager#INDEV_CLASSES_FOLDER} + */ + private boolean inDevEnvironment = false; + /** * List of already loaded code modifier class names. This prevents loading the same class twice. */ @@ -73,6 +81,7 @@ public class MinestomRootClassLoader extends HierarchyClassLoader { private MinestomRootClassLoader(ClassLoader parent) { super("Minestom Root ClassLoader", extractURLsFromClasspath(), parent); asmClassLoader = newChild(new URL[0]); + inDevEnvironment = System.getProperty(ExtensionManager.INDEV_CLASSES_FOLDER) != null; } public static MinestomRootClassLoader getInstance() { @@ -184,6 +193,22 @@ public class MinestomRootClassLoader extends HierarchyClassLoader { if (name == null) throw new ClassNotFoundException(); String path = name.replace(".", "/") + ".class"; + + if(inDevEnvironment) { + // check if the class to load is the entry point of the extension + boolean isMainExtensionClass = false; + for(MinestomExtensionClassLoader c : children) { + if(c.isMainExtensionClass(name)) { + isMainExtensionClass = true; + break; + } + } + if(isMainExtensionClass) { // entry point of the extension, force load through extension classloader + throw new ClassNotFoundException("The class "+name+" is the entry point of an extension. " + + "Because we are in a dev environment, we force its load through its extension classloader, " + + "even though the root classloader has access."); + } + } InputStream input = getResourceAsStream(path); if (input == null) { throw new ClassNotFoundException("Could not find resource " + path); diff --git a/src/main/java/net/minestom/server/instance/Instance.java b/src/main/java/net/minestom/server/instance/Instance.java index 86155d927..6ebbdfd00 100644 --- a/src/main/java/net/minestom/server/instance/Instance.java +++ b/src/main/java/net/minestom/server/instance/Instance.java @@ -41,6 +41,7 @@ import org.jetbrains.annotations.Nullable; import java.util.*; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.CopyOnWriteArraySet; import java.util.function.Consumer; @@ -79,6 +80,7 @@ public abstract class Instance implements BlockModifier, EventHandler, DataConta private long lastTickAge = System.currentTimeMillis(); private final Map, Collection> eventCallbacks = new ConcurrentHashMap<>(); + private final Map>> extensionCallbacks = new ConcurrentHashMap<>(); // Entities present in this instance protected final Set entities = new CopyOnWriteArraySet<>(); @@ -862,6 +864,12 @@ public abstract class Instance implements BlockModifier, EventHandler, DataConta return eventCallbacks; } + @NotNull + @Override + public Collection> getExtensionCallbacks(String extension) { + return extensionCallbacks.computeIfAbsent(extension, e -> new CopyOnWriteArrayList<>()); + } + // UNSAFE METHODS (need most of time to be synchronized) /** diff --git a/src/test/java/improveextensions/unloadcallbacks/UnloadCallbacksExtension.java b/src/test/java/improveextensions/unloadcallbacks/UnloadCallbacksExtension.java new file mode 100644 index 000000000..0eaf4fc8c --- /dev/null +++ b/src/test/java/improveextensions/unloadcallbacks/UnloadCallbacksExtension.java @@ -0,0 +1,57 @@ +package improveextensions.unloadcallbacks; + +import net.minestom.server.MinecraftServer; +import net.minestom.server.event.EventCallback; +import net.minestom.server.event.GlobalEventHandler; +import net.minestom.server.event.handler.EventHandler; +import net.minestom.server.event.instance.InstanceTickEvent; +import net.minestom.server.extensions.Extension; +import net.minestom.server.utils.time.TimeUnit; +import org.junit.jupiter.api.Assertions; +import org.opentest4j.AssertionFailedError; + +public class UnloadCallbacksExtension extends Extension { + + private boolean ticked = false; + private final EventCallback callback = this::onTick; + + private void onTick(InstanceTickEvent e) { + ticked = true; + } + + @Override + public void initialize() { + GlobalEventHandler globalEvents = MinecraftServer.getGlobalEventHandler(); + // this callback will be automatically removed when unloading the extension + globalEvents.addEventCallback(InstanceTickEvent.class, callback); + + try { + Assertions.assertTrue(EventHandler.getExtensionOwningCallback(callback).isPresent()); + Assertions.assertEquals("UnloadCallbacksExtension", EventHandler.getExtensionOwningCallback(callback).get()); + } catch (AssertionFailedError e) { + e.printStackTrace(); + System.exit(-1); + } + + MinecraftServer.getSchedulerManager().buildTask(() -> { + // unload self + MinecraftServer.getExtensionManager().unloadExtension(getDescription().getName()); + }).delay(1L, TimeUnit.SECOND).schedule(); + } + + @Override + public void terminate() { + ticked = false; + + // TODO: set to transient task to avoid losing the task on termination + MinecraftServer.getSchedulerManager().buildTask(() -> { + // Make sure callback is disabled + try { + Assertions.assertFalse(ticked, "ticked should be false because the callback has been unloaded"); + } catch (AssertionFailedError e) { + e.printStackTrace(); + } + MinecraftServer.stopCleanly(); + }).delay(1L, TimeUnit.SECOND).schedule(); + } +} diff --git a/src/test/resources/improveextensions/unloadcallbacks/extension.json b/src/test/resources/improveextensions/unloadcallbacks/extension.json new file mode 100644 index 000000000..e1f8cafb0 --- /dev/null +++ b/src/test/resources/improveextensions/unloadcallbacks/extension.json @@ -0,0 +1,4 @@ +{ + "entrypoint": "improveextensions.unloadcallbacks.UnloadCallbacksExtension", + "name": "UnloadCallbacksExtension" +} \ No newline at end of file From 4f39498ef5e090e27f96425f9eecbc7411066639 Mon Sep 17 00:00:00 2001 From: jglrxavpok Date: Wed, 3 Feb 2021 17:12:20 +0100 Subject: [PATCH 04/12] Remove debug code no longer relevant --- .../minestom/server/event/handler/EventHandler.java | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/main/java/net/minestom/server/event/handler/EventHandler.java b/src/main/java/net/minestom/server/event/handler/EventHandler.java index 37a4acc90..c7454e45b 100644 --- a/src/main/java/net/minestom/server/event/handler/EventHandler.java +++ b/src/main/java/net/minestom/server/event/handler/EventHandler.java @@ -50,16 +50,6 @@ public interface EventHandler { ClassLoader cl = callback.getClass().getClassLoader(); if(cl instanceof MinestomExtensionClassLoader) { return Optional.of(((MinestomExtensionClassLoader) cl).getExtensionName()); - } else if(System.getProperty(ExtensionManager.INDEV_CLASSES_FOLDER) != null) { // in a dev environment, the extension will always be loaded with the root classloader - StackTraceElement[] stackTrace = Thread.currentThread().getStackTrace(); - // 0 -> getStackTrace - // 1 -> getExtensionOwningCallback - // 2 -> add/remove EventCallback - // 3 -> Potentially the extension - if(stackTrace.length >= 4) { - StackTraceElement potentialExtensionCall = stackTrace[3]; - System.out.println(potentialExtensionCall.getClassLoaderName()); - } } return Optional.empty(); } From c8e311855f932c7a445c3176b0629d5ef84523dd Mon Sep 17 00:00:00 2001 From: jglrxavpok Date: Wed, 3 Feb 2021 17:51:14 +0100 Subject: [PATCH 05/12] Auto-unschedule and cancel tasks from extensions --- .../server/event/handler/EventHandler.java | 20 +----- .../server/extensions/ExtensionManager.java | 4 +- .../MinestomRootClassLoader.java | 25 +++++-- .../server/timer/SchedulerManager.java | 65 +++++++++++++++++-- .../java/net/minestom/server/timer/Task.java | 34 +++++++++- .../minestom/server/timer/TaskBuilder.java | 26 +++++++- .../UnloadCallbacksExtension.java | 36 ++++++++-- 7 files changed, 174 insertions(+), 36 deletions(-) diff --git a/src/main/java/net/minestom/server/event/handler/EventHandler.java b/src/main/java/net/minestom/server/event/handler/EventHandler.java index c7454e45b..88dbe7745 100644 --- a/src/main/java/net/minestom/server/event/handler/EventHandler.java +++ b/src/main/java/net/minestom/server/event/handler/EventHandler.java @@ -6,11 +6,8 @@ import net.minestom.server.event.CancellableEvent; import net.minestom.server.event.Event; import net.minestom.server.event.EventCallback; import net.minestom.server.event.GlobalEventHandler; -import net.minestom.server.extensions.ExtensionManager; -import net.minestom.server.extras.selfmodification.MinestomExtensionClassLoader; import net.minestom.server.extras.selfmodification.MinestomRootClassLoader; import net.minestom.server.instance.Instance; -import net.minestom.server.utils.validate.Check; import org.jetbrains.annotations.NotNull; import java.util.Collection; @@ -41,19 +38,6 @@ public interface EventHandler { @NotNull Collection> getExtensionCallbacks(String extension); - /** - * Tries to know which extension created this callback, based on the classloader of the callback - * @param callback the callback to get the extension of - * @return Optional.empty() if no extension has been found, Optional.of(<name>) with 'name' being the extension name - */ - static Optional getExtensionOwningCallback(@NotNull EventCallback callback) { - ClassLoader cl = callback.getClass().getClassLoader(); - if(cl instanceof MinestomExtensionClassLoader) { - return Optional.of(((MinestomExtensionClassLoader) cl).getExtensionName()); - } - return Optional.empty(); - } - /** * Adds a new event callback for the specified type {@code eventClass}. * @@ -63,7 +47,7 @@ public interface EventHandler { * @return true if the callback collection changed as a result of the call */ default boolean addEventCallback(@NotNull Class eventClass, @NotNull EventCallback eventCallback) { - Optional extensionSource = getExtensionOwningCallback(eventCallback); + Optional extensionSource = MinestomRootClassLoader.findExtensionObjectOwner(eventCallback); extensionSource.ifPresent(s -> getExtensionCallbacks(s).add(eventCallback)); Collection callbacks = getEventCallbacks(eventClass); @@ -80,7 +64,7 @@ public interface EventHandler { */ default boolean removeEventCallback(@NotNull Class eventClass, @NotNull EventCallback eventCallback) { Collection callbacks = getEventCallbacks(eventClass); - Optional extensionSource = getExtensionOwningCallback(eventCallback); + Optional extensionSource = MinestomRootClassLoader.findExtensionObjectOwner(eventCallback); extensionSource.ifPresent(s -> getExtensionCallbacks(s).remove(eventCallback)); return callbacks.remove(eventCallback); diff --git a/src/main/java/net/minestom/server/extensions/ExtensionManager.java b/src/main/java/net/minestom/server/extensions/ExtensionManager.java index bd0ab2359..d46802aaf 100644 --- a/src/main/java/net/minestom/server/extensions/ExtensionManager.java +++ b/src/main/java/net/minestom/server/extensions/ExtensionManager.java @@ -524,7 +524,9 @@ public class ExtensionManager { ext.preTerminate(); ext.terminate(); // remove callbacks for this extension - MinecraftServer.getGlobalEventHandler().removeCallbacksOwnedByExtension(ext.getDescription().getName()); + String extensionName = ext.getDescription().getName(); + MinecraftServer.getGlobalEventHandler().removeCallbacksOwnedByExtension(extensionName); + MinecraftServer.getSchedulerManager().removeExtensionTasksOnUnload(extensionName); // TODO: more callback types ext.postTerminate(); diff --git a/src/main/java/net/minestom/server/extras/selfmodification/MinestomRootClassLoader.java b/src/main/java/net/minestom/server/extras/selfmodification/MinestomRootClassLoader.java index 421566d5f..a2427f810 100644 --- a/src/main/java/net/minestom/server/extras/selfmodification/MinestomRootClassLoader.java +++ b/src/main/java/net/minestom/server/extras/selfmodification/MinestomRootClassLoader.java @@ -1,6 +1,7 @@ package net.minestom.server.extras.selfmodification; import net.minestom.server.MinecraftServer; +import net.minestom.server.event.EventCallback; import net.minestom.server.extensions.ExtensionManager; import org.jetbrains.annotations.NotNull; import org.objectweb.asm.ClassReader; @@ -17,10 +18,7 @@ import java.net.MalformedURLException; import java.net.URISyntaxException; import java.net.URL; import java.net.URLClassLoader; -import java.util.HashSet; -import java.util.LinkedList; -import java.util.List; -import java.util.Set; +import java.util.*; /** * Class Loader that can modify class bytecode when they are loaded @@ -313,4 +311,23 @@ public class MinestomRootClassLoader extends HierarchyClassLoader { public List getModifiers() { return modifiers; } + + /** + * Tries to know which extension created this object, based on the classloader of the object. This can only check that the class of the object has been loaded + * by an extension. + * + * While not perfect, this should detect any callback created extension code. + * It is possible this current version of the implementation might struggle with callbacks created through external + * libraries, but as libraries are loaded separately for each extension, this *should not*(tm) be a problem. + * + * @param obj the object to get the extension of + * @return Optional.empty() if no extension has been found, Optional.of(<name>) with 'name' being the extension name + */ + public static Optional findExtensionObjectOwner(@NotNull Object obj) { + ClassLoader cl = obj.getClass().getClassLoader(); + if(cl instanceof MinestomExtensionClassLoader) { + return Optional.of(((MinestomExtensionClassLoader) cl).getExtensionName()); + } + return Optional.empty(); + } } diff --git a/src/main/java/net/minestom/server/timer/SchedulerManager.java b/src/main/java/net/minestom/server/timer/SchedulerManager.java index 40d3f6f00..f47c80a08 100644 --- a/src/main/java/net/minestom/server/timer/SchedulerManager.java +++ b/src/main/java/net/minestom/server/timer/SchedulerManager.java @@ -8,11 +8,11 @@ import net.minestom.server.utils.thread.MinestomThread; import org.jetbrains.annotations.NotNull; import java.util.Collection; -import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; -import java.util.concurrent.ScheduledExecutorService; -import java.util.concurrent.TimeUnit; +import java.util.List; +import java.util.Map; +import java.util.concurrent.*; import java.util.concurrent.atomic.AtomicInteger; +import java.util.stream.Collectors; /** * An object which manages all the {@link Task}'s. @@ -40,6 +40,16 @@ public final class SchedulerManager { // All the registered shutdown tasks (task id = task) protected final Int2ObjectMap shutdownTasks; + /** + * Tasks scheduled through extensions + */ + private final Map> extensionTasks = new ConcurrentHashMap<>(); + + /** + * Shutdown tasks scheduled through extensions + */ + private final Map> extensionShutdownTasks = new ConcurrentHashMap<>(); + /** * Default constructor */ @@ -169,4 +179,51 @@ public final class SchedulerManager { public ScheduledExecutorService getTimerExecutionService() { return timerExecutionService; } + + /** + * Called when a Task from an extension is scheduled. + * @param owningExtension the name of the extension which scheduled the task + * @param task the task that has been scheduled + */ + void onScheduleFromExtension(String owningExtension, Task task) { + List scheduledForThisExtension = extensionTasks.computeIfAbsent(owningExtension, s -> new CopyOnWriteArrayList<>()); + scheduledForThisExtension.add(task); + } + + /** + * Called when a Task from an extension is scheduled for server shutdown. + * @param owningExtension the name of the extension which scheduled the task + * @param task the task that has been scheduled + */ + void onScheduleShutdownFromExtension(String owningExtension, Task task) { + List scheduledForThisExtension = extensionShutdownTasks.computeIfAbsent(owningExtension, s -> new CopyOnWriteArrayList<>()); + scheduledForThisExtension.add(task); + } + + /** + * Unschedules all non-transient tasks ({@link Task#isTransient()}) from this scheduler. Tasks are allowed to complete + * @param extension the name of the extension to unschedule tasks from + * @see Task#isTransient() + */ + public void removeExtensionTasksOnUnload(String extension) { + List scheduledForThisExtension = extensionTasks.get(extension); + if(scheduledForThisExtension != null) { + List toCancel = scheduledForThisExtension.stream() + .filter(t -> !t.isTransient()) + .collect(Collectors.toList()); + toCancel.forEach(Task::cancel); + scheduledForThisExtension.removeAll(toCancel); + } + + + List shutdownScheduledForThisExtension = extensionShutdownTasks.get(extension); + if(shutdownScheduledForThisExtension != null) { + List toCancel = shutdownScheduledForThisExtension.stream() + .filter(t -> !t.isTransient()) + .collect(Collectors.toList()); + toCancel.forEach(Task::cancel); + shutdownScheduledForThisExtension.removeAll(toCancel); + shutdownTasks.values().removeAll(toCancel); + } + } } diff --git a/src/main/java/net/minestom/server/timer/Task.java b/src/main/java/net/minestom/server/timer/Task.java index b92acd984..a97d1ca1c 100644 --- a/src/main/java/net/minestom/server/timer/Task.java +++ b/src/main/java/net/minestom/server/timer/Task.java @@ -3,6 +3,7 @@ package net.minestom.server.timer; import it.unimi.dsi.fastutil.ints.Int2ObjectMap; import net.minestom.server.MinecraftServer; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; import java.util.Objects; import java.util.concurrent.ScheduledFuture; @@ -27,6 +28,16 @@ public class Task implements Runnable { private final long delay; // Repeat value for the task execution private final long repeat; + + /** Extension which owns this task, or null if none */ + private final String owningExtension; + /** + * If this task is owned by an extension, should it survive the unloading of said extension? + * May be useful for delay tasks, but it can prevent the extension classes from being unloaded, and preventing a full + * reload of that extension. + */ + private final boolean isTransient; + // Task completion/execution private ScheduledFuture future; // The thread of the task @@ -41,13 +52,15 @@ public class Task implements Runnable { * @param delay The time to delay * @param repeat The time until the repetition */ - public Task(@NotNull SchedulerManager schedulerManager, @NotNull Runnable runnable, boolean shutdown, long delay, long repeat) { + public Task(@NotNull SchedulerManager schedulerManager, @NotNull Runnable runnable, boolean shutdown, long delay, long repeat, boolean isTransient, @Nullable String owningExtension) { this.schedulerManager = schedulerManager; this.runnable = runnable; this.shutdown = shutdown; this.id = shutdown ? this.schedulerManager.getShutdownCounterIdentifier() : this.schedulerManager.getCounterIdentifier(); this.delay = delay; this.repeat = repeat; + this.isTransient = isTransient; + this.owningExtension = owningExtension; } /** @@ -87,6 +100,9 @@ public class Task implements Runnable { * Sets up the task for correct execution. */ public void schedule() { + if(owningExtension != null) { + this.schedulerManager.onScheduleFromExtension(owningExtension, this); + } this.future = this.repeat == 0L ? this.schedulerManager.getTimerExecutionService().schedule(this, this.delay, TimeUnit.MILLISECONDS) : this.schedulerManager.getTimerExecutionService().scheduleAtFixedRate(this, this.delay, this.repeat, TimeUnit.MILLISECONDS); @@ -150,6 +166,22 @@ public class Task implements Runnable { return id == task.id; } + /** + * If this task is owned by an extension, should it survive the unloading of said extension? + * May be useful for delay tasks, but it can prevent the extension classes from being unloaded, and preventing a full + * reload of that extension. + */ + public boolean isTransient() { + return isTransient; + } + + /** + * Extension which owns this task, or null if none + */ + public String getOwningExtension() { + return owningExtension; + } + @Override public int hashCode() { return Objects.hash(id); diff --git a/src/main/java/net/minestom/server/timer/TaskBuilder.java b/src/main/java/net/minestom/server/timer/TaskBuilder.java index a127513e2..f937e6294 100644 --- a/src/main/java/net/minestom/server/timer/TaskBuilder.java +++ b/src/main/java/net/minestom/server/timer/TaskBuilder.java @@ -1,6 +1,7 @@ package net.minestom.server.timer; import it.unimi.dsi.fastutil.ints.Int2ObjectMap; +import net.minestom.server.extras.selfmodification.MinestomRootClassLoader; import net.minestom.server.utils.time.TimeUnit; import org.jetbrains.annotations.NotNull; @@ -18,10 +19,18 @@ public class TaskBuilder { private final Runnable runnable; // True if the task planned for the application shutdown private final boolean shutdown; + /** Extension which owns this task, or null if none */ + private final String owningExtension; // Delay value for the task execution private long delay; // Repeat value for the task execution private long repeat; + /** + * If this task is owned by an extension, should it survive the unloading of said extension? + * May be useful for delay tasks, but it can prevent the extension classes from being unloaded, and preventing a full + * reload of that extension. + */ + private boolean isTransient; /** * Creates a task builder. @@ -46,6 +55,8 @@ public class TaskBuilder { this.schedulerManager = schedulerManager; this.runnable = runnable; this.shutdown = shutdown; + this.isTransient = false; + this.owningExtension = MinestomRootClassLoader.findExtensionObjectOwner(runnable).orElse(null); } /** @@ -96,6 +107,14 @@ public class TaskBuilder { return this; } + /** If this task is owned by an extension, should it survive the unloading of said extension? + * May be useful for delay tasks, but it can prevent the extension classes from being unloaded, and preventing a full + * reload of that extension. */ + public TaskBuilder makeTransient() { + isTransient = true; + return this; + } + /** * Schedules this {@link Task} for execution. * @@ -108,12 +127,17 @@ public class TaskBuilder { this.runnable, this.shutdown, this.delay, - this.repeat); + this.repeat, + this.isTransient, + this.owningExtension); if (this.shutdown) { Int2ObjectMap shutdownTasks = this.schedulerManager.shutdownTasks; synchronized (shutdownTasks) { shutdownTasks.put(task.getId(), task); } + if(owningExtension != null) { + this.schedulerManager.onScheduleShutdownFromExtension(owningExtension, task); + } } else { Int2ObjectMap tasks = this.schedulerManager.tasks; synchronized (tasks) { diff --git a/src/test/java/improveextensions/unloadcallbacks/UnloadCallbacksExtension.java b/src/test/java/improveextensions/unloadcallbacks/UnloadCallbacksExtension.java index 0eaf4fc8c..767994736 100644 --- a/src/test/java/improveextensions/unloadcallbacks/UnloadCallbacksExtension.java +++ b/src/test/java/improveextensions/unloadcallbacks/UnloadCallbacksExtension.java @@ -6,17 +6,21 @@ import net.minestom.server.event.GlobalEventHandler; import net.minestom.server.event.handler.EventHandler; import net.minestom.server.event.instance.InstanceTickEvent; import net.minestom.server.extensions.Extension; +import net.minestom.server.extras.selfmodification.MinestomRootClassLoader; import net.minestom.server.utils.time.TimeUnit; import org.junit.jupiter.api.Assertions; import org.opentest4j.AssertionFailedError; public class UnloadCallbacksExtension extends Extension { - private boolean ticked = false; + private boolean ticked1 = false; + private boolean ticked2 = false; + private boolean tickedScheduledNonTransient = false; + private boolean tickedScheduledTransient = false; private final EventCallback callback = this::onTick; private void onTick(InstanceTickEvent e) { - ticked = true; + ticked1 = true; } @Override @@ -24,10 +28,22 @@ public class UnloadCallbacksExtension extends Extension { GlobalEventHandler globalEvents = MinecraftServer.getGlobalEventHandler(); // this callback will be automatically removed when unloading the extension globalEvents.addEventCallback(InstanceTickEvent.class, callback); + // this one too + globalEvents.addEventCallback(InstanceTickEvent.class, e -> ticked2 = true); + + // this callback will be cancelled + MinecraftServer.getSchedulerManager().buildTask(() -> { + tickedScheduledNonTransient = true; + }).repeat(100L, TimeUnit.MILLISECOND).schedule(); + + // this callback will NOT be cancelled + MinecraftServer.getSchedulerManager().buildTask(() -> { + tickedScheduledTransient = true; + }).repeat(100L, TimeUnit.MILLISECOND).makeTransient().schedule(); try { - Assertions.assertTrue(EventHandler.getExtensionOwningCallback(callback).isPresent()); - Assertions.assertEquals("UnloadCallbacksExtension", EventHandler.getExtensionOwningCallback(callback).get()); + Assertions.assertTrue(MinestomRootClassLoader.findExtensionObjectOwner(callback).isPresent()); + Assertions.assertEquals("UnloadCallbacksExtension", MinestomRootClassLoader.findExtensionObjectOwner(callback).get()); } catch (AssertionFailedError e) { e.printStackTrace(); System.exit(-1); @@ -41,17 +57,23 @@ public class UnloadCallbacksExtension extends Extension { @Override public void terminate() { - ticked = false; + ticked1 = false; + ticked2 = false; + tickedScheduledNonTransient = false; + tickedScheduledTransient = false; // TODO: set to transient task to avoid losing the task on termination MinecraftServer.getSchedulerManager().buildTask(() -> { // Make sure callback is disabled try { - Assertions.assertFalse(ticked, "ticked should be false because the callback has been unloaded"); + Assertions.assertFalse(ticked1, "ticked1 should be false because the callback has been unloaded"); + Assertions.assertFalse(ticked2, "ticked2 should be false because the callback has been unloaded"); + Assertions.assertFalse(tickedScheduledNonTransient, "tickedScheduledNonTransient should be false because the callback has been unloaded"); + Assertions.assertTrue(tickedScheduledTransient, "tickedScheduledNonTransient should be true because the callback has NOT been unloaded"); } catch (AssertionFailedError e) { e.printStackTrace(); } MinecraftServer.stopCleanly(); - }).delay(1L, TimeUnit.SECOND).schedule(); + }).delay(1L, TimeUnit.SECOND).makeTransient().schedule(); } } From c9d2edef3a535f7d0bb2cf6bb0051ced9f22ada7 Mon Sep 17 00:00:00 2001 From: jglrxavpok Date: Wed, 3 Feb 2021 19:35:59 +0100 Subject: [PATCH 06/12] Tests for shutdown and delay tasks --- .../MinestomRootClassLoader.java | 2 +- .../UnloadCallbacksExtension.java | 20 +++++++++++++++---- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/main/java/net/minestom/server/extras/selfmodification/MinestomRootClassLoader.java b/src/main/java/net/minestom/server/extras/selfmodification/MinestomRootClassLoader.java index a2427f810..5813ea20a 100644 --- a/src/main/java/net/minestom/server/extras/selfmodification/MinestomRootClassLoader.java +++ b/src/main/java/net/minestom/server/extras/selfmodification/MinestomRootClassLoader.java @@ -316,7 +316,7 @@ public class MinestomRootClassLoader extends HierarchyClassLoader { * Tries to know which extension created this object, based on the classloader of the object. This can only check that the class of the object has been loaded * by an extension. * - * While not perfect, this should detect any callback created extension code. + * While not perfect, this should detect any callback created via extension code. * It is possible this current version of the implementation might struggle with callbacks created through external * libraries, but as libraries are loaded separately for each extension, this *should not*(tm) be a problem. * diff --git a/src/test/java/improveextensions/unloadcallbacks/UnloadCallbacksExtension.java b/src/test/java/improveextensions/unloadcallbacks/UnloadCallbacksExtension.java index 767994736..2c44089a8 100644 --- a/src/test/java/improveextensions/unloadcallbacks/UnloadCallbacksExtension.java +++ b/src/test/java/improveextensions/unloadcallbacks/UnloadCallbacksExtension.java @@ -3,7 +3,6 @@ package improveextensions.unloadcallbacks; import net.minestom.server.MinecraftServer; import net.minestom.server.event.EventCallback; import net.minestom.server.event.GlobalEventHandler; -import net.minestom.server.event.handler.EventHandler; import net.minestom.server.event.instance.InstanceTickEvent; import net.minestom.server.extensions.Extension; import net.minestom.server.extras.selfmodification.MinestomRootClassLoader; @@ -11,6 +10,8 @@ import net.minestom.server.utils.time.TimeUnit; import org.junit.jupiter.api.Assertions; import org.opentest4j.AssertionFailedError; +import java.util.concurrent.atomic.AtomicBoolean; + public class UnloadCallbacksExtension extends Extension { private boolean ticked1 = false; @@ -62,18 +63,29 @@ public class UnloadCallbacksExtension extends Extension { tickedScheduledNonTransient = false; tickedScheduledTransient = false; - // TODO: set to transient task to avoid losing the task on termination + AtomicBoolean executedDelayTaskAfterTerminate = new AtomicBoolean(false); + // because terminate is called just before unscheduling and removing event callbacks, + // the following task will never be executed, because it is not transient MinecraftServer.getSchedulerManager().buildTask(() -> { - // Make sure callback is disabled + executedDelayTaskAfterTerminate.set(true); + }).delay(100L, TimeUnit.MILLISECOND).schedule(); + + // this shutdown tasks will not be executed because it is not transient + MinecraftServer.getSchedulerManager().buildShutdownTask(() -> Assertions.fail("This shutdown task should be unloaded when the extension is")).schedule(); + + MinecraftServer.getSchedulerManager().buildTask(() -> { + // Make sure callbacks are disabled try { Assertions.assertFalse(ticked1, "ticked1 should be false because the callback has been unloaded"); Assertions.assertFalse(ticked2, "ticked2 should be false because the callback has been unloaded"); Assertions.assertFalse(tickedScheduledNonTransient, "tickedScheduledNonTransient should be false because the callback has been unloaded"); Assertions.assertTrue(tickedScheduledTransient, "tickedScheduledNonTransient should be true because the callback has NOT been unloaded"); + Assertions.assertFalse(executedDelayTaskAfterTerminate.get(), "executedDelayTaskAfterTerminate should be false because the callback has been unloaded before executing"); + System.out.println("All tests passed."); } catch (AssertionFailedError e) { e.printStackTrace(); } - MinecraftServer.stopCleanly(); + MinecraftServer.stopCleanly(); // TODO: fix deadlock which happens because stopCleanly waits on completion of scheduler tasks }).delay(1L, TimeUnit.SECOND).makeTransient().schedule(); } } From a63e9462c20d8646ae09b53624eb1dd2bb52ddcf Mon Sep 17 00:00:00 2001 From: jglrxavpok Date: Wed, 3 Feb 2021 20:47:01 +0100 Subject: [PATCH 07/12] All EventHandler remove extension callback when the extension unloads --- .../server/event/handler/EventHandler.java | 13 ++++++- .../minestom/server/extensions/Extension.java | 37 ++++++++++++++++++ .../server/extensions/ExtensionManager.java | 3 +- .../server/extensions/IExtensionObserver.java | 14 +++++++ .../server/timer/SchedulerManager.java | 15 ++++++- .../UnloadCallbacksExtension.java | 39 +++++++++++++++++-- 6 files changed, 112 insertions(+), 9 deletions(-) create mode 100644 src/main/java/net/minestom/server/extensions/IExtensionObserver.java diff --git a/src/main/java/net/minestom/server/event/handler/EventHandler.java b/src/main/java/net/minestom/server/event/handler/EventHandler.java index 88dbe7745..767424de5 100644 --- a/src/main/java/net/minestom/server/event/handler/EventHandler.java +++ b/src/main/java/net/minestom/server/event/handler/EventHandler.java @@ -6,6 +6,7 @@ import net.minestom.server.event.CancellableEvent; import net.minestom.server.event.Event; import net.minestom.server.event.EventCallback; import net.minestom.server.event.GlobalEventHandler; +import net.minestom.server.extensions.IExtensionObserver; import net.minestom.server.extras.selfmodification.MinestomRootClassLoader; import net.minestom.server.instance.Instance; import org.jetbrains.annotations.NotNull; @@ -19,7 +20,7 @@ import java.util.stream.Stream; /** * Represents an element which can have {@link Event} listeners assigned to it. */ -public interface EventHandler { +public interface EventHandler extends IExtensionObserver { /** * Gets a {@link Map} containing all the listeners assigned to a specific {@link Event} type. @@ -48,7 +49,10 @@ public interface EventHandler { */ default boolean addEventCallback(@NotNull Class eventClass, @NotNull EventCallback eventCallback) { Optional extensionSource = MinestomRootClassLoader.findExtensionObjectOwner(eventCallback); - extensionSource.ifPresent(s -> getExtensionCallbacks(s).add(eventCallback)); + extensionSource.ifPresent(name -> { + MinecraftServer.getExtensionManager().getExtension(name).observe(this); + getExtensionCallbacks(name).add(eventCallback); + }); Collection callbacks = getEventCallbacks(eventClass); return callbacks.add(eventCallback); @@ -166,4 +170,9 @@ public interface EventHandler { } } + @Override + default void onExtensionUnload(String extensionName) { + removeCallbacksOwnedByExtension(extensionName); + } + } diff --git a/src/main/java/net/minestom/server/extensions/Extension.java b/src/main/java/net/minestom/server/extensions/Extension.java index 82caa6063..094916ed4 100644 --- a/src/main/java/net/minestom/server/extensions/Extension.java +++ b/src/main/java/net/minestom/server/extensions/Extension.java @@ -1,10 +1,18 @@ package net.minestom.server.extensions; +import net.minestom.server.event.handler.EventHandler; import org.jetbrains.annotations.NotNull; import org.slf4j.Logger; +import java.lang.ref.WeakReference; import java.util.ArrayList; +import java.util.Collections; import java.util.List; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentSkipListSet; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.function.Consumer; public abstract class Extension { // Set by reflection @@ -14,6 +22,14 @@ public abstract class Extension { @SuppressWarnings("unused") private Logger logger; + /** + * Observers that will be notified of events related to this extension. + * Kept as WeakReference because entities can be observer, but could become candidate to be garbage-collected while + * this extension holds a reference to it. A WeakReference makes sure this extension does not prevent the memory + * from being cleaned up. + */ + private Set> observers = Collections.newSetFromMap(new ConcurrentHashMap<>()); + protected Extension() { } @@ -55,6 +71,27 @@ public abstract class Extension { return logger; } + /** + * Adds a new observer to this extension. + * Will be kept as a WeakReference. + * @param observer + */ + public void observe(IExtensionObserver observer) { + observers.add(new WeakReference<>(observer)); + } + + /** + * Calls some action on all valid observers of this extension + * @param action code to execute on each observer + */ + public void triggerChange(Consumer action) { + for(WeakReference weakObserver : observers) { + IExtensionObserver observer = weakObserver.get(); + if(observer != null) { + action.accept(observer); + } + } + } public static class ExtensionDescription { private final String name; diff --git a/src/main/java/net/minestom/server/extensions/ExtensionManager.java b/src/main/java/net/minestom/server/extensions/ExtensionManager.java index d46802aaf..8f8230421 100644 --- a/src/main/java/net/minestom/server/extensions/ExtensionManager.java +++ b/src/main/java/net/minestom/server/extensions/ExtensionManager.java @@ -525,8 +525,7 @@ public class ExtensionManager { ext.terminate(); // remove callbacks for this extension String extensionName = ext.getDescription().getName(); - MinecraftServer.getGlobalEventHandler().removeCallbacksOwnedByExtension(extensionName); - MinecraftServer.getSchedulerManager().removeExtensionTasksOnUnload(extensionName); + ext.triggerChange(observer -> observer.onExtensionUnload(extensionName)); // TODO: more callback types ext.postTerminate(); diff --git a/src/main/java/net/minestom/server/extensions/IExtensionObserver.java b/src/main/java/net/minestom/server/extensions/IExtensionObserver.java new file mode 100644 index 000000000..c9f86aa3d --- /dev/null +++ b/src/main/java/net/minestom/server/extensions/IExtensionObserver.java @@ -0,0 +1,14 @@ +package net.minestom.server.extensions; + +/** + * Observes events related to extensions + */ +public interface IExtensionObserver { + + /** + * Called before unloading an extension (that is, right after Extension#terminate and right before Extension#unload) + * @param extensionName the name of the extension that is being unloaded + */ + void onExtensionUnload(String extensionName); + +} diff --git a/src/main/java/net/minestom/server/timer/SchedulerManager.java b/src/main/java/net/minestom/server/timer/SchedulerManager.java index f47c80a08..33e9ca2e8 100644 --- a/src/main/java/net/minestom/server/timer/SchedulerManager.java +++ b/src/main/java/net/minestom/server/timer/SchedulerManager.java @@ -4,6 +4,8 @@ import it.unimi.dsi.fastutil.ints.Int2ObjectMap; import it.unimi.dsi.fastutil.ints.Int2ObjectOpenHashMap; import it.unimi.dsi.fastutil.objects.ObjectCollection; import net.minestom.server.MinecraftServer; +import net.minestom.server.extensions.Extension; +import net.minestom.server.extensions.IExtensionObserver; import net.minestom.server.utils.thread.MinestomThread; import org.jetbrains.annotations.NotNull; @@ -24,7 +26,7 @@ import java.util.stream.Collectors; *

* Shutdown tasks are built with {@link #buildShutdownTask(Runnable)} and are executed, as the name implies, when the server stops. */ -public final class SchedulerManager { +public final class SchedulerManager implements IExtensionObserver { private static boolean instanced; // A counter for all normal tasks @@ -188,6 +190,9 @@ public final class SchedulerManager { void onScheduleFromExtension(String owningExtension, Task task) { List scheduledForThisExtension = extensionTasks.computeIfAbsent(owningExtension, s -> new CopyOnWriteArrayList<>()); scheduledForThisExtension.add(task); + + Extension ext = MinecraftServer.getExtensionManager().getExtension(owningExtension); + ext.observe(this); } /** @@ -198,6 +203,9 @@ public final class SchedulerManager { void onScheduleShutdownFromExtension(String owningExtension, Task task) { List scheduledForThisExtension = extensionShutdownTasks.computeIfAbsent(owningExtension, s -> new CopyOnWriteArrayList<>()); scheduledForThisExtension.add(task); + + Extension ext = MinecraftServer.getExtensionManager().getExtension(owningExtension); + ext.observe(this); } /** @@ -226,4 +234,9 @@ public final class SchedulerManager { shutdownTasks.values().removeAll(toCancel); } } + + @Override + public void onExtensionUnload(String extensionName) { + removeExtensionTasksOnUnload(extensionName); + } } diff --git a/src/test/java/improveextensions/unloadcallbacks/UnloadCallbacksExtension.java b/src/test/java/improveextensions/unloadcallbacks/UnloadCallbacksExtension.java index 2c44089a8..832b1d25d 100644 --- a/src/test/java/improveextensions/unloadcallbacks/UnloadCallbacksExtension.java +++ b/src/test/java/improveextensions/unloadcallbacks/UnloadCallbacksExtension.java @@ -1,11 +1,15 @@ package improveextensions.unloadcallbacks; import net.minestom.server.MinecraftServer; +import net.minestom.server.entity.type.monster.EntityZombie; import net.minestom.server.event.EventCallback; import net.minestom.server.event.GlobalEventHandler; +import net.minestom.server.event.entity.EntityTickEvent; import net.minestom.server.event.instance.InstanceTickEvent; import net.minestom.server.extensions.Extension; import net.minestom.server.extras.selfmodification.MinestomRootClassLoader; +import net.minestom.server.instance.Instance; +import net.minestom.server.utils.Position; import net.minestom.server.utils.time.TimeUnit; import org.junit.jupiter.api.Assertions; import org.opentest4j.AssertionFailedError; @@ -18,6 +22,8 @@ public class UnloadCallbacksExtension extends Extension { private boolean ticked2 = false; private boolean tickedScheduledNonTransient = false; private boolean tickedScheduledTransient = false; + private boolean zombieTicked = false; + private boolean instanceTicked = false; private final EventCallback callback = this::onTick; private void onTick(InstanceTickEvent e) { @@ -32,6 +38,19 @@ public class UnloadCallbacksExtension extends Extension { // this one too globalEvents.addEventCallback(InstanceTickEvent.class, e -> ticked2 = true); + Instance instance = MinecraftServer.getInstanceManager().getInstances().stream().findFirst().orElseThrow(); + + // add an event callback on an instance + instance.addEventCallback(InstanceTickEvent.class, e -> instanceTicked = true); + instance.loadChunk(0,0); + + // add an event callback on an entity + EntityZombie zombie = new EntityZombie(new Position(8,64,8) /* middle of chunk */); + zombie.addEventCallback(EntityTickEvent.class, e -> { + zombieTicked = true; + }); + zombie.setInstance(instance); + // this callback will be cancelled MinecraftServer.getSchedulerManager().buildTask(() -> { tickedScheduledNonTransient = true; @@ -58,10 +77,20 @@ public class UnloadCallbacksExtension extends Extension { @Override public void terminate() { - ticked1 = false; - ticked2 = false; - tickedScheduledNonTransient = false; - tickedScheduledTransient = false; + new Thread(() -> { + try { + // wait for complete termination of this extension + Thread.sleep(10); + ticked1 = false; + ticked2 = false; + tickedScheduledNonTransient = false; + tickedScheduledTransient = false; + instanceTicked = false; + zombieTicked = false; + } catch (InterruptedException e) { + e.printStackTrace(); + } + }).start(); AtomicBoolean executedDelayTaskAfterTerminate = new AtomicBoolean(false); // because terminate is called just before unscheduling and removing event callbacks, @@ -79,6 +108,8 @@ public class UnloadCallbacksExtension extends Extension { Assertions.assertFalse(ticked1, "ticked1 should be false because the callback has been unloaded"); Assertions.assertFalse(ticked2, "ticked2 should be false because the callback has been unloaded"); Assertions.assertFalse(tickedScheduledNonTransient, "tickedScheduledNonTransient should be false because the callback has been unloaded"); + Assertions.assertFalse(zombieTicked, "zombieTicked should be false because the callback has been unloaded"); + Assertions.assertFalse(instanceTicked, "instanceTicked should be false because the callback has been unloaded"); Assertions.assertTrue(tickedScheduledTransient, "tickedScheduledNonTransient should be true because the callback has NOT been unloaded"); Assertions.assertFalse(executedDelayTaskAfterTerminate.get(), "executedDelayTaskAfterTerminate should be false because the callback has been unloaded before executing"); System.out.println("All tests passed."); From 2a96beb819395ddbf53512c2cb92987643a30eec Mon Sep 17 00:00:00 2001 From: jglrxavpok Date: Wed, 3 Feb 2021 20:56:36 +0100 Subject: [PATCH 08/12] Extensions will be unloaded when server is stopped --- .../net/minestom/server/MinecraftServer.java | 1 + .../minestom/server/extensions/Extension.java | 2 +- .../server/extensions/ExtensionManager.java | 13 +++++++ .../MixinIntoMinestomCoreLauncher.java | 3 ++ .../UnloadExtensionOnStop.java | 37 +++++++++++++++++++ .../unloadonstop/extension.json | 4 ++ 6 files changed, 59 insertions(+), 1 deletion(-) create mode 100644 src/test/java/improveextensions/unloadextensiononstop/UnloadExtensionOnStop.java create mode 100644 src/test/resources/improveextensions/unloadonstop/extension.json diff --git a/src/main/java/net/minestom/server/MinecraftServer.java b/src/main/java/net/minestom/server/MinecraftServer.java index be57cb1ff..7022daf31 100644 --- a/src/main/java/net/minestom/server/MinecraftServer.java +++ b/src/main/java/net/minestom/server/MinecraftServer.java @@ -784,6 +784,7 @@ public final class MinecraftServer { public static void stopCleanly() { stopping = true; LOGGER.info("Stopping Minestom server."); + extensionManager.unloadAllExtensions(); updateManager.stop(); schedulerManager.shutdown(); connectionManager.shutdown(); diff --git a/src/main/java/net/minestom/server/extensions/Extension.java b/src/main/java/net/minestom/server/extensions/Extension.java index 094916ed4..ea626d966 100644 --- a/src/main/java/net/minestom/server/extensions/Extension.java +++ b/src/main/java/net/minestom/server/extensions/Extension.java @@ -24,7 +24,7 @@ public abstract class Extension { /** * Observers that will be notified of events related to this extension. - * Kept as WeakReference because entities can be observer, but could become candidate to be garbage-collected while + * Kept as WeakReference because entities can be observers, but could become candidate to be garbage-collected while * this extension holds a reference to it. A WeakReference makes sure this extension does not prevent the memory * from being cleaned up. */ diff --git a/src/main/java/net/minestom/server/extensions/ExtensionManager.java b/src/main/java/net/minestom/server/extensions/ExtensionManager.java index 8f8230421..a588190d6 100644 --- a/src/main/java/net/minestom/server/extensions/ExtensionManager.java +++ b/src/main/java/net/minestom/server/extensions/ExtensionManager.java @@ -710,4 +710,17 @@ public class ExtensionManager { } LOGGER.info("Early load of code modifiers from extensions done!"); } + + /** + * Unloads all extensions + */ + public void unloadAllExtensions() { + // copy names, as the extensions map will be modified via the calls to unload + Set extensionNames = new HashSet<>(extensions.keySet()); + for(String ext : extensionNames) { + if(extensions.containsKey(ext)) { // is still loaded? Because extensions can depend on one another, it might have already been unloaded + unloadExtension(ext); + } + } + } } diff --git a/src/test/java/improveextensions/MixinIntoMinestomCoreLauncher.java b/src/test/java/improveextensions/MixinIntoMinestomCoreLauncher.java index eb1a436a4..3d7c3a26b 100644 --- a/src/test/java/improveextensions/MixinIntoMinestomCoreLauncher.java +++ b/src/test/java/improveextensions/MixinIntoMinestomCoreLauncher.java @@ -8,6 +8,9 @@ import net.minestom.server.Bootstrap; // -Dminestom.extension.indevfolder.classes=build/classes/java/test/ -Dminestom.extension.indevfolder.resources=build/resources/test/improveextensions // To test disabling early Mixin injections: // -Dminestom.extension.disable_early_load=true -Dminestom.extension.indevfolder.classes=build/classes/java/test/ -Dminestom.extension.indevfolder.resources=build/resources/test/improveextensions/disableearlyload + +// To test extension termination when the server quits: +// -Dminestom.extension.indevfolder.classes=build/classes/java/test/ -Dminestom.extension.indevfolder.resources=build/resources/test/improveextensions/unloadonstop public class MixinIntoMinestomCoreLauncher { public static void main(String[] args) { Bootstrap.bootstrap("demo.MainDemo", args); diff --git a/src/test/java/improveextensions/unloadextensiononstop/UnloadExtensionOnStop.java b/src/test/java/improveextensions/unloadextensiononstop/UnloadExtensionOnStop.java new file mode 100644 index 000000000..7491c2e8a --- /dev/null +++ b/src/test/java/improveextensions/unloadextensiononstop/UnloadExtensionOnStop.java @@ -0,0 +1,37 @@ +package improveextensions.unloadextensiononstop; + +import net.minestom.server.MinecraftServer; +import net.minestom.server.entity.type.monster.EntityZombie; +import net.minestom.server.event.EventCallback; +import net.minestom.server.event.GlobalEventHandler; +import net.minestom.server.event.entity.EntityTickEvent; +import net.minestom.server.event.instance.InstanceTickEvent; +import net.minestom.server.extensions.Extension; +import net.minestom.server.extras.selfmodification.MinestomRootClassLoader; +import net.minestom.server.instance.Instance; +import net.minestom.server.utils.Position; +import net.minestom.server.utils.time.TimeUnit; +import org.junit.jupiter.api.Assertions; +import org.opentest4j.AssertionFailedError; + +import java.util.concurrent.atomic.AtomicBoolean; + +public class UnloadExtensionOnStop extends Extension { + + private boolean terminated = false; + + @Override + public void initialize() { + MinecraftServer.getSchedulerManager().buildShutdownTask(() -> { + Assertions.assertTrue(terminated, "Extension should have been terminated on shutdown."); + System.out.println("All tests passed."); + }).makeTransient().schedule(); + + MinecraftServer.getSchedulerManager().buildTask(MinecraftServer::stopCleanly).makeTransient().delay(1L, TimeUnit.SECOND).schedule(); + } + + @Override + public void terminate() { + terminated = true; + } +} diff --git a/src/test/resources/improveextensions/unloadonstop/extension.json b/src/test/resources/improveextensions/unloadonstop/extension.json new file mode 100644 index 000000000..c3424d6d5 --- /dev/null +++ b/src/test/resources/improveextensions/unloadonstop/extension.json @@ -0,0 +1,4 @@ +{ + "entrypoint": "improveextensions.unloadextensiononstop.UnloadExtensionOnStop", + "name": "UnloadExtensionOnStop" +} \ No newline at end of file From ee158c0deafeb6ded77496b32f5fde9f07f2f2ac Mon Sep 17 00:00:00 2001 From: jglrxavpok Date: Thu, 4 Feb 2021 11:57:43 +0100 Subject: [PATCH 09/12] Use Nullable string instead of Optional in MinestomRootClassLoader#findExtensionObjectOwner --- .../server/event/handler/EventHandler.java | 16 +++++++++------- .../MinestomRootClassLoader.java | 17 ++++++++++------- .../net/minestom/server/timer/TaskBuilder.java | 2 +- .../UnloadCallbacksExtension.java | 4 ++-- 4 files changed, 22 insertions(+), 17 deletions(-) diff --git a/src/main/java/net/minestom/server/event/handler/EventHandler.java b/src/main/java/net/minestom/server/event/handler/EventHandler.java index 767424de5..5bb0bf40a 100644 --- a/src/main/java/net/minestom/server/event/handler/EventHandler.java +++ b/src/main/java/net/minestom/server/event/handler/EventHandler.java @@ -48,11 +48,11 @@ public interface EventHandler extends IExtensionObserver { * @return true if the callback collection changed as a result of the call */ default boolean addEventCallback(@NotNull Class eventClass, @NotNull EventCallback eventCallback) { - Optional extensionSource = MinestomRootClassLoader.findExtensionObjectOwner(eventCallback); - extensionSource.ifPresent(name -> { - MinecraftServer.getExtensionManager().getExtension(name).observe(this); - getExtensionCallbacks(name).add(eventCallback); - }); + String extensionSource = MinestomRootClassLoader.findExtensionObjectOwner(eventCallback); + if(extensionSource != null) { + MinecraftServer.getExtensionManager().getExtension(extensionSource).observe(this); + getExtensionCallbacks(extensionSource).add(eventCallback); + }; Collection callbacks = getEventCallbacks(eventClass); return callbacks.add(eventCallback); @@ -68,8 +68,10 @@ public interface EventHandler extends IExtensionObserver { */ default boolean removeEventCallback(@NotNull Class eventClass, @NotNull EventCallback eventCallback) { Collection callbacks = getEventCallbacks(eventClass); - Optional extensionSource = MinestomRootClassLoader.findExtensionObjectOwner(eventCallback); - extensionSource.ifPresent(s -> getExtensionCallbacks(s).remove(eventCallback)); + String extensionSource = MinestomRootClassLoader.findExtensionObjectOwner(eventCallback); + if(extensionSource != null) { + getExtensionCallbacks(extensionSource).remove(eventCallback); + }; return callbacks.remove(eventCallback); } diff --git a/src/main/java/net/minestom/server/extras/selfmodification/MinestomRootClassLoader.java b/src/main/java/net/minestom/server/extras/selfmodification/MinestomRootClassLoader.java index 5813ea20a..4f6e1cf81 100644 --- a/src/main/java/net/minestom/server/extras/selfmodification/MinestomRootClassLoader.java +++ b/src/main/java/net/minestom/server/extras/selfmodification/MinestomRootClassLoader.java @@ -1,9 +1,9 @@ package net.minestom.server.extras.selfmodification; import net.minestom.server.MinecraftServer; -import net.minestom.server.event.EventCallback; import net.minestom.server.extensions.ExtensionManager; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; import org.objectweb.asm.ClassReader; import org.objectweb.asm.ClassWriter; import org.objectweb.asm.tree.ClassNode; @@ -15,10 +15,12 @@ import java.io.IOException; import java.io.InputStream; import java.lang.reflect.InvocationTargetException; import java.net.MalformedURLException; -import java.net.URISyntaxException; import java.net.URL; import java.net.URLClassLoader; -import java.util.*; +import java.util.HashSet; +import java.util.LinkedList; +import java.util.List; +import java.util.Set; /** * Class Loader that can modify class bytecode when they are loaded @@ -321,13 +323,14 @@ public class MinestomRootClassLoader extends HierarchyClassLoader { * libraries, but as libraries are loaded separately for each extension, this *should not*(tm) be a problem. * * @param obj the object to get the extension of - * @return Optional.empty() if no extension has been found, Optional.of(<name>) with 'name' being the extension name + * @return null if no extension has been found, otherwise the extension name */ - public static Optional findExtensionObjectOwner(@NotNull Object obj) { + @Nullable + public static String findExtensionObjectOwner(@NotNull Object obj) { ClassLoader cl = obj.getClass().getClassLoader(); if(cl instanceof MinestomExtensionClassLoader) { - return Optional.of(((MinestomExtensionClassLoader) cl).getExtensionName()); + return ((MinestomExtensionClassLoader) cl).getExtensionName(); } - return Optional.empty(); + return null; } } diff --git a/src/main/java/net/minestom/server/timer/TaskBuilder.java b/src/main/java/net/minestom/server/timer/TaskBuilder.java index f937e6294..98636ce88 100644 --- a/src/main/java/net/minestom/server/timer/TaskBuilder.java +++ b/src/main/java/net/minestom/server/timer/TaskBuilder.java @@ -56,7 +56,7 @@ public class TaskBuilder { this.runnable = runnable; this.shutdown = shutdown; this.isTransient = false; - this.owningExtension = MinestomRootClassLoader.findExtensionObjectOwner(runnable).orElse(null); + this.owningExtension = MinestomRootClassLoader.findExtensionObjectOwner(runnable); } /** diff --git a/src/test/java/improveextensions/unloadcallbacks/UnloadCallbacksExtension.java b/src/test/java/improveextensions/unloadcallbacks/UnloadCallbacksExtension.java index 832b1d25d..a0815d861 100644 --- a/src/test/java/improveextensions/unloadcallbacks/UnloadCallbacksExtension.java +++ b/src/test/java/improveextensions/unloadcallbacks/UnloadCallbacksExtension.java @@ -62,8 +62,8 @@ public class UnloadCallbacksExtension extends Extension { }).repeat(100L, TimeUnit.MILLISECOND).makeTransient().schedule(); try { - Assertions.assertTrue(MinestomRootClassLoader.findExtensionObjectOwner(callback).isPresent()); - Assertions.assertEquals("UnloadCallbacksExtension", MinestomRootClassLoader.findExtensionObjectOwner(callback).get()); + Assertions.assertNotNull(MinestomRootClassLoader.findExtensionObjectOwner(callback)); + Assertions.assertEquals("UnloadCallbacksExtension", MinestomRootClassLoader.findExtensionObjectOwner(callback)); } catch (AssertionFailedError e) { e.printStackTrace(); System.exit(-1); From 9ce7a08d12eeb750f9743b189822f3af59c3699a Mon Sep 17 00:00:00 2001 From: jglrxavpok Date: Thu, 4 Feb 2021 19:11:43 +0100 Subject: [PATCH 10/12] Extension can check if their code modifiers (including Mixins) loaded correctly --- .../extensions/DiscoveredExtension.java | 18 +++++++++++ .../minestom/server/extensions/Extension.java | 26 +++++++++++++--- .../server/extensions/ExtensionManager.java | 22 ++++++++++--- .../MinestomRootClassLoader.java | 24 +++++++++----- .../MixinIntoMinestomCoreLauncher.java | 3 ++ .../MissingCodeModifiersExtension.java | 31 +++++++++++++++++++ .../missingmodifiers/extension.json | 8 +++++ 7 files changed, 116 insertions(+), 16 deletions(-) create mode 100644 src/test/java/improveextensions/missingmodifiers/MissingCodeModifiersExtension.java create mode 100644 src/test/resources/improveextensions/missingmodifiers/extension.json diff --git a/src/main/java/net/minestom/server/extensions/DiscoveredExtension.java b/src/main/java/net/minestom/server/extensions/DiscoveredExtension.java index 0e4641621..6e6715f56 100644 --- a/src/main/java/net/minestom/server/extensions/DiscoveredExtension.java +++ b/src/main/java/net/minestom/server/extensions/DiscoveredExtension.java @@ -23,6 +23,8 @@ final class DiscoveredExtension { private String[] codeModifiers; private String[] dependencies; private ExternalDependencies externalDependencies; + private List missingCodeModifiers = new LinkedList<>(); + private boolean failedToLoadMixin = false; transient List files = new LinkedList<>(); transient LoadStatus loadStatus = LoadStatus.LOAD_SUCCESS; transient private File originalJar; @@ -138,6 +140,22 @@ final class DiscoveredExtension { } + public void addMissingCodeModifier(String codeModifierClass) { + missingCodeModifiers.add(codeModifierClass); + } + + public void setFailedToLoadMixinFlag() { + failedToLoadMixin = true; + } + + public List getMissingCodeModifiers() { + return missingCodeModifiers; + } + + public boolean hasFailedToLoadMixin() { + return failedToLoadMixin; + } + enum LoadStatus { LOAD_SUCCESS("Actually, it did not fail. This message should not have been printed."), MISSING_DEPENDENCIES("Missing dependencies, check your logs."), diff --git a/src/main/java/net/minestom/server/extensions/Extension.java b/src/main/java/net/minestom/server/extensions/Extension.java index ea626d966..b2029014d 100644 --- a/src/main/java/net/minestom/server/extensions/Extension.java +++ b/src/main/java/net/minestom/server/extensions/Extension.java @@ -5,10 +5,7 @@ import org.jetbrains.annotations.NotNull; import org.slf4j.Logger; import java.lang.ref.WeakReference; -import java.util.ArrayList; -import java.util.Collections; -import java.util.List; -import java.util.Set; +import java.util.*; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentSkipListSet; import java.util.concurrent.CopyOnWriteArrayList; @@ -93,11 +90,20 @@ public abstract class Extension { } } + /** + * If this extension registers code modifiers and/or mixins, are they loaded correctly? + */ + public boolean areCodeModifiersAllLoadedCorrectly() { + return !getDescription().failedToLoadMixin && getDescription().getMissingCodeModifiers().isEmpty(); + } + public static class ExtensionDescription { private final String name; private final String version; private final List authors; private final List dependents = new ArrayList<>(); + private final List missingCodeModifiers = new LinkedList<>(); + private final boolean failedToLoadMixin; private final DiscoveredExtension origin; ExtensionDescription(@NotNull String name, @NotNull String version, @NotNull List authors, @NotNull DiscoveredExtension origin) { @@ -105,6 +111,8 @@ public abstract class Extension { this.version = version; this.authors = authors; this.origin = origin; + failedToLoadMixin = origin.hasFailedToLoadMixin(); + missingCodeModifiers.addAll(origin.getMissingCodeModifiers()); } @NotNull @@ -131,5 +139,15 @@ public abstract class Extension { DiscoveredExtension getOrigin() { return origin; } + + @NotNull + public List getMissingCodeModifiers() { + return missingCodeModifiers; + } + + @NotNull + public boolean hasFailedToLoadMixin() { + return failedToLoadMixin; + } } } diff --git a/src/main/java/net/minestom/server/extensions/ExtensionManager.java b/src/main/java/net/minestom/server/extensions/ExtensionManager.java index a588190d6..1625d3d0a 100644 --- a/src/main/java/net/minestom/server/extensions/ExtensionManager.java +++ b/src/main/java/net/minestom/server/extensions/ExtensionManager.java @@ -13,6 +13,8 @@ import org.jetbrains.annotations.Nullable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.spongepowered.asm.mixin.Mixins; +import org.spongepowered.asm.mixin.throwables.MixinError; +import org.spongepowered.asm.mixin.throwables.MixinException; import org.spongepowered.asm.service.ServiceNotAvailableError; import java.io.*; @@ -496,20 +498,32 @@ public class ExtensionManager { for (DiscoveredExtension extension : extensions) { try { for (String codeModifierClass : extension.getCodeModifiers()) { - modifiableClassLoader.loadModifier(extension.files.toArray(new File[0]), codeModifierClass); + boolean loaded = modifiableClassLoader.loadModifier(extension.files.toArray(new URL[0]), codeModifierClass); + if(!loaded) { + extension.addMissingCodeModifier(codeModifierClass); + } } if (!extension.getMixinConfig().isEmpty()) { final String mixinConfigFile = extension.getMixinConfig(); try { Mixins.addConfiguration(mixinConfigFile); LOGGER.info("Found mixin in extension {}: {}", extension.getName(), mixinConfigFile); - } catch (ServiceNotAvailableError e) { - MinecraftServer.getExceptionManager().handleException(e); + } catch (ServiceNotAvailableError | MixinError | MixinException e) { + if(MinecraftServer.getExceptionManager() != null) { + MinecraftServer.getExceptionManager().handleException(e); + } else { + e.printStackTrace(); + } LOGGER.error("Could not load Mixin configuration: "+mixinConfigFile); + extension.setFailedToLoadMixinFlag(); } } } catch (Exception e) { - MinecraftServer.getExceptionManager().handleException(e); + if(MinecraftServer.getExceptionManager() != null) { + MinecraftServer.getExceptionManager().handleException(e); + } else { + e.printStackTrace(); + } LOGGER.error("Failed to load code modifier for extension in files: " + extension.files .stream() diff --git a/src/main/java/net/minestom/server/extras/selfmodification/MinestomRootClassLoader.java b/src/main/java/net/minestom/server/extras/selfmodification/MinestomRootClassLoader.java index 4f6e1cf81..b1572aae3 100644 --- a/src/main/java/net/minestom/server/extras/selfmodification/MinestomRootClassLoader.java +++ b/src/main/java/net/minestom/server/extras/selfmodification/MinestomRootClassLoader.java @@ -275,15 +275,17 @@ public class MinestomRootClassLoader extends HierarchyClassLoader { return URLClassLoader.newInstance(urls, this); } - public void loadModifier(File[] originFiles, String codeModifierClass) { + /** + * Loads a code modifier. + * @param urls + * @param codeModifierClass + * @return whether the modifier has been loaded. Returns 'true' even if the code modifier is already loaded before calling this method + */ + public boolean loadModifier(URL[] urls, String codeModifierClass) { if(alreadyLoadedCodeModifiers.contains(codeModifierClass)) { - return; + return true; } - URL[] urls = new URL[originFiles.length]; try { - for (int i = 0; i < originFiles.length; i++) { - urls[i] = originFiles[i].toURI().toURL(); - } URLClassLoader loader = newChild(urls); Class modifierClass = loader.loadClass(codeModifierClass); if (CodeModifier.class.isAssignableFrom(modifierClass)) { @@ -294,9 +296,15 @@ public class MinestomRootClassLoader extends HierarchyClassLoader { alreadyLoadedCodeModifiers.add(codeModifierClass); } } - } catch (MalformedURLException | ClassNotFoundException | InvocationTargetException | InstantiationException | IllegalAccessException | NoSuchMethodException e) { - MinecraftServer.getExceptionManager().handleException(e); + return true; + } catch (ClassNotFoundException | InvocationTargetException | InstantiationException | IllegalAccessException | NoSuchMethodException e) { + if(MinecraftServer.getExceptionManager() != null) { + MinecraftServer.getExceptionManager().handleException(e); + } else { + e.printStackTrace(); + } } + return false; } public void addCodeModifier(CodeModifier modifier) { diff --git a/src/test/java/improveextensions/MixinIntoMinestomCoreLauncher.java b/src/test/java/improveextensions/MixinIntoMinestomCoreLauncher.java index 3d7c3a26b..8079a1730 100644 --- a/src/test/java/improveextensions/MixinIntoMinestomCoreLauncher.java +++ b/src/test/java/improveextensions/MixinIntoMinestomCoreLauncher.java @@ -11,6 +11,9 @@ import net.minestom.server.Bootstrap; // To test extension termination when the server quits: // -Dminestom.extension.indevfolder.classes=build/classes/java/test/ -Dminestom.extension.indevfolder.resources=build/resources/test/improveextensions/unloadonstop + +// To test report of failure when a mixin configuration cannot be loaded, or code modifiers are missing +// -Dminestom.extension.indevfolder.classes=build/classes/java/test/ -Dminestom.extension.indevfolder.resources=build/resources/test/improveextensions/missingmodifiers public class MixinIntoMinestomCoreLauncher { public static void main(String[] args) { Bootstrap.bootstrap("demo.MainDemo", args); diff --git a/src/test/java/improveextensions/missingmodifiers/MissingCodeModifiersExtension.java b/src/test/java/improveextensions/missingmodifiers/MissingCodeModifiersExtension.java new file mode 100644 index 000000000..3101ab76a --- /dev/null +++ b/src/test/java/improveextensions/missingmodifiers/MissingCodeModifiersExtension.java @@ -0,0 +1,31 @@ +package improveextensions.missingmodifiers; + +import net.minestom.server.MinecraftServer; +import net.minestom.server.extensions.Extension; +import org.junit.jupiter.api.Assertions; +import org.opentest4j.AssertionFailedError; + +public class MissingCodeModifiersExtension extends Extension { + + @Override + public void initialize() { + // force load of InstanceContainer class + try { + Assertions.assertFalse(areCodeModifiersAllLoadedCorrectly(), "Mixin configuration could not be loaded and code modifiers are unavailable, the failure should be reported"); + Assertions.assertTrue(getDescription().hasFailedToLoadMixin(), "Mixin configuration does not exist and should not be loaded"); + Assertions.assertEquals(1, getDescription().getMissingCodeModifiers().size(), "Code modifier does not exist, it should be reported as missing"); + Assertions.assertEquals("InvalidCodeModifierClass", getDescription().getMissingCodeModifiers().get(0)); + System.out.println("All tests passed."); + } catch (AssertionFailedError e) { + e.printStackTrace(); + } + MinecraftServer.stopCleanly(); + } + + @Override + public void terminate() { + getLogger().info("Terminate extension"); + } + + +} diff --git a/src/test/resources/improveextensions/missingmodifiers/extension.json b/src/test/resources/improveextensions/missingmodifiers/extension.json new file mode 100644 index 000000000..d5db684db --- /dev/null +++ b/src/test/resources/improveextensions/missingmodifiers/extension.json @@ -0,0 +1,8 @@ +{ + "entrypoint": "improveextensions.missingmodifiers.MissingCodeModifiersExtension", + "name": "MissingCodeModifiersExtension", + "codeModifiers": [ + "InvalidCodeModifierClass" + ], + "mixinConfig": "mixins.$invalid$.json" +} \ No newline at end of file From 6addd63396bed51e9a0f1e46bcbfb3d9153d4860 Mon Sep 17 00:00:00 2001 From: jglrxavpok Date: Thu, 4 Feb 2021 20:40:12 +0100 Subject: [PATCH 11/12] Fix extension subdependencies not being loaded + Test that J9 modules are supported --- build.gradle | 3 ++ .../server/extensions/ExtensionManager.java | 21 ++++++--- ...inestomCoreWithJava9ModuleOnClasspath.java | 43 +++++++++++++++++++ .../j9modules/extension.json | 14 ++++++ .../j9modules/mixins.minestomcore.json | 10 +++++ 5 files changed, 86 insertions(+), 5 deletions(-) create mode 100644 src/test/java/improveextensions/MixinIntoMinestomCoreWithJava9ModuleOnClasspath.java create mode 100644 src/test/resources/improveextensions/j9modules/extension.json create mode 100644 src/test/resources/improveextensions/j9modules/mixins.minestomcore.json diff --git a/build.gradle b/build.gradle index 3585753cc..facce88d5 100644 --- a/build.gradle +++ b/build.gradle @@ -105,6 +105,9 @@ dependencies { testImplementation 'org.junit.jupiter:junit-jupiter-api:5.6.2' testRuntimeOnly('org.junit.jupiter:junit-jupiter-engine:5.6.2') + // Only here to ensure J9 module support for extensions and our classloaders + testCompileOnly "org.mockito:mockito-core:2.28.2" + // Netty api 'io.netty:netty-handler:4.1.58.Final' api 'io.netty:netty-codec:4.1.58.Final' diff --git a/src/main/java/net/minestom/server/extensions/ExtensionManager.java b/src/main/java/net/minestom/server/extensions/ExtensionManager.java index 1625d3d0a..3643d7660 100644 --- a/src/main/java/net/minestom/server/extensions/ExtensionManager.java +++ b/src/main/java/net/minestom/server/extensions/ExtensionManager.java @@ -2,6 +2,7 @@ package net.minestom.server.extensions; import com.google.gson.Gson; import net.minestom.dependencies.DependencyGetter; +import net.minestom.dependencies.ResolvedDependency; import net.minestom.dependencies.maven.MavenRepository; import net.minestom.server.MinecraftServer; import net.minestom.server.extras.selfmodification.MinestomExtensionClassLoader; @@ -402,13 +403,13 @@ public class ExtensionManager { for (var artifact : externalDependencies.artifacts) { var resolved = getter.get(artifact, dependenciesFolder); - addDependencyFile(resolved.getContentsLocation(), ext); + addDependencyFile(resolved, ext); LOGGER.trace("Dependency of extension {}: {}", ext.getName(), resolved); } for (var dependencyName : ext.getDependencies()) { var resolved = getter.get(dependencyName, dependenciesFolder); - addDependencyFile(resolved.getContentsLocation(), ext); + addDependencyFile(resolved, ext); LOGGER.trace("Dependency of extension {}: {}", ext.getName(), resolved); } } catch (Exception e) { @@ -420,9 +421,19 @@ public class ExtensionManager { } } - private void addDependencyFile(URL dependency, DiscoveredExtension extension) { - extension.files.add(dependency); - LOGGER.trace("Added dependency {} to extension {} classpath", dependency.toExternalForm(), extension.getName()); + private void addDependencyFile(ResolvedDependency dependency, DiscoveredExtension extension) { + URL location = dependency.getContentsLocation(); + extension.files.add(location); + LOGGER.trace("Added dependency {} to extension {} classpath", location.toExternalForm(), extension.getName()); + + // recurse to add full dependency tree + if(!dependency.getSubdependencies().isEmpty()) { + LOGGER.trace("Dependency {} has subdependencies, adding...", location.toExternalForm()); + for(ResolvedDependency sub : dependency.getSubdependencies()) { + addDependencyFile(sub, extension); + } + LOGGER.trace("Dependency {} has had its subdependencies added.", location.toExternalForm()); + } } /** diff --git a/src/test/java/improveextensions/MixinIntoMinestomCoreWithJava9ModuleOnClasspath.java b/src/test/java/improveextensions/MixinIntoMinestomCoreWithJava9ModuleOnClasspath.java new file mode 100644 index 000000000..2de1ba459 --- /dev/null +++ b/src/test/java/improveextensions/MixinIntoMinestomCoreWithJava9ModuleOnClasspath.java @@ -0,0 +1,43 @@ +package improveextensions; + +import net.minestom.server.MinecraftServer; +import net.minestom.server.extensions.Extension; +import net.minestom.server.instance.InstanceContainer; +import net.minestom.server.world.DimensionType; +import org.junit.jupiter.api.Assertions; +import org.opentest4j.AssertionFailedError; + +import java.util.List; +import java.util.UUID; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Extensions should be able to use Mixins for classes loaded very early by Minestom (InstanceContainer for instance) + */ +public class MixinIntoMinestomCoreWithJava9ModuleOnClasspath extends Extension { + + @Override + public void initialize() { + // use Mockito only to ensure J9 modules on the classpath are supported + List mockedList = mock(List.class); + when(mockedList.get(0)).thenReturn("Test"); + // force load of InstanceContainer class + InstanceContainer c = new InstanceContainer(UUID.randomUUID(), DimensionType.OVERWORLD, null); + System.out.println(c.toString()); + try { + Assertions.assertTrue(MixinIntoMinestomCore.success, "InstanceContainer must have been mixed in with improveextensions.InstanceContainerMixin"); + Assertions.assertEquals(1, MinecraftServer.getExtensionManager().getExtensionLoaders().size(), "Only one extension classloader (this extension's) must be active."); + Assertions.assertEquals("Test", mockedList.get(0)); + } catch (AssertionFailedError e) { + e.printStackTrace(); + } + MinecraftServer.stopCleanly(); + } + + @Override + public void terminate() { + getLogger().info("Terminate extension"); + } +} diff --git a/src/test/resources/improveextensions/j9modules/extension.json b/src/test/resources/improveextensions/j9modules/extension.json new file mode 100644 index 000000000..ba409d029 --- /dev/null +++ b/src/test/resources/improveextensions/j9modules/extension.json @@ -0,0 +1,14 @@ +{ + "entrypoint": "improveextensions.MixinIntoMinestomCoreWithJava9ModuleOnClasspath", + "name": "MixinIntoMinestomCoreWithJava9ModuleOnClasspath", + "codeModifiers": [], + "mixinConfig": "mixins.minestomcore.json", + "externalDependencies": { + "repositories": [ + {"name": "JCentral", "url": "https://jcenter.bintray.com/"} + ], + "artifacts": [ + "org.mockito:mockito-core:2.28.2" + ] + } +} \ No newline at end of file diff --git a/src/test/resources/improveextensions/j9modules/mixins.minestomcore.json b/src/test/resources/improveextensions/j9modules/mixins.minestomcore.json new file mode 100644 index 000000000..7c8c03fe2 --- /dev/null +++ b/src/test/resources/improveextensions/j9modules/mixins.minestomcore.json @@ -0,0 +1,10 @@ +{ + "required": true, + "minVersion": "0.8", + "package": "improveextensions.mixins", + "target": "@env(DEFAULT)", + "compatibilityLevel": "JAVA_11", + "mixins": [ + "InstanceContainerMixin" + ] +} \ No newline at end of file From e40186a2aad5eca86a8cc9dd408bc1629ff5d267 Mon Sep 17 00:00:00 2001 From: jglrxavpok Date: Wed, 17 Feb 2021 17:37:54 +0100 Subject: [PATCH 12/12] Periodically remove extension observers (every minute at the moment) --- .../minestom/server/extensions/Extension.java | 20 +++++++++--- .../server/extensions/ExtensionManager.java | 31 +++++++++++++------ 2 files changed, 37 insertions(+), 14 deletions(-) diff --git a/src/main/java/net/minestom/server/extensions/Extension.java b/src/main/java/net/minestom/server/extensions/Extension.java index b2029014d..fdd626b77 100644 --- a/src/main/java/net/minestom/server/extensions/Extension.java +++ b/src/main/java/net/minestom/server/extensions/Extension.java @@ -1,14 +1,13 @@ package net.minestom.server.extensions; -import net.minestom.server.event.handler.EventHandler; import org.jetbrains.annotations.NotNull; import org.slf4j.Logger; +import java.lang.ref.Reference; +import java.lang.ref.ReferenceQueue; import java.lang.ref.WeakReference; import java.util.*; import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentSkipListSet; -import java.util.concurrent.CopyOnWriteArrayList; import java.util.function.Consumer; public abstract class Extension { @@ -26,6 +25,7 @@ public abstract class Extension { * from being cleaned up. */ private Set> observers = Collections.newSetFromMap(new ConcurrentHashMap<>()); + private ReferenceQueue observerReferenceQueue = new ReferenceQueue<>(); protected Extension() { @@ -74,7 +74,7 @@ public abstract class Extension { * @param observer */ public void observe(IExtensionObserver observer) { - observers.add(new WeakReference<>(observer)); + observers.add(new WeakReference<>(observer, observerReferenceQueue)); } /** @@ -97,6 +97,18 @@ public abstract class Extension { return !getDescription().failedToLoadMixin && getDescription().getMissingCodeModifiers().isEmpty(); } + /** + * Removes all expired reference to observers + * + * @see #observers + */ + public void cleanupObservers() { + Reference ref; + while((ref = observerReferenceQueue.poll()) != null) { + observers.remove(ref); + } + } + public static class ExtensionDescription { private final String name; private final String version; diff --git a/src/main/java/net/minestom/server/extensions/ExtensionManager.java b/src/main/java/net/minestom/server/extensions/ExtensionManager.java index 3643d7660..b4f672283 100644 --- a/src/main/java/net/minestom/server/extensions/ExtensionManager.java +++ b/src/main/java/net/minestom/server/extensions/ExtensionManager.java @@ -8,6 +8,7 @@ import net.minestom.server.MinecraftServer; import net.minestom.server.extras.selfmodification.MinestomExtensionClassLoader; import net.minestom.server.extras.selfmodification.MinestomRootClassLoader; import net.minestom.server.ping.ResponseDataConsumer; +import net.minestom.server.utils.time.TimeUnit; import net.minestom.server.utils.validate.Check; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -124,6 +125,13 @@ public class ExtensionManager { MinecraftServer.getExceptionManager().handleException(e); } } + + // periodically cleanup observers + MinecraftServer.getSchedulerManager().buildTask(() -> { + for(Extension ext : extensionList) { + ext.cleanupObservers(); + } + }).repeat(1L, TimeUnit.MINUTE).schedule(); } private void setupClassLoader(@NotNull DiscoveredExtension discoveredExtension) { @@ -241,16 +249,19 @@ public class ExtensionManager { @NotNull private List discoverExtensions() { List extensions = new LinkedList<>(); - for (File file : extensionFolder.listFiles()) { - if (file.isDirectory()) { - continue; - } - if (!file.getName().endsWith(".jar")) { - continue; - } - DiscoveredExtension extension = discoverFromJar(file); - if (extension != null && extension.loadStatus == DiscoveredExtension.LoadStatus.LOAD_SUCCESS) { - extensions.add(extension); + File[] fileList = extensionFolder.listFiles(); + if(fileList != null) { + for (File file : fileList) { + if (file.isDirectory()) { + continue; + } + if (!file.getName().endsWith(".jar")) { + continue; + } + DiscoveredExtension extension = discoverFromJar(file); + if (extension != null && extension.loadStatus == DiscoveredExtension.LoadStatus.LOAD_SUCCESS) { + extensions.add(extension); + } } }