From 701b1cb2e5f4ba3c2708413fceb01d13a6c7fcbc Mon Sep 17 00:00:00 2001 From: LeoDog896 Date: Tue, 23 Mar 2021 11:35:52 -0400 Subject: [PATCH] Move to DiscoveredExtension vs ExtensionDescription For those who are wondering why I replaced some streams: https://stackoverflow.com/questions/16635398/java-8-iterable-foreach-vs-foreach-loop --- .../extensions/DiscoveredExtension.java | 6 +- .../minestom/server/extensions/Extension.java | 85 ++++++------------- .../ExtensionDependencyResolver.java | 10 ++- .../server/extensions/ExtensionManager.java | 84 ++++++++++-------- .../demo/commands/ReloadExtensionCommand.java | 2 +- .../MissingCodeModifiersExtension.java | 6 +- .../UnloadCallbacksExtension.java | 2 +- 7 files changed, 86 insertions(+), 109 deletions(-) diff --git a/src/main/java/net/minestom/server/extensions/DiscoveredExtension.java b/src/main/java/net/minestom/server/extensions/DiscoveredExtension.java index 6e6715f56..28945fbfd 100644 --- a/src/main/java/net/minestom/server/extensions/DiscoveredExtension.java +++ b/src/main/java/net/minestom/server/extensions/DiscoveredExtension.java @@ -10,7 +10,7 @@ import java.net.URL; import java.util.LinkedList; import java.util.List; -final class DiscoveredExtension { +public final class DiscoveredExtension { public final static Logger LOGGER = LoggerFactory.getLogger(DiscoveredExtension.class); @@ -23,7 +23,7 @@ final class DiscoveredExtension { private String[] codeModifiers; private String[] dependencies; private ExternalDependencies externalDependencies; - private List missingCodeModifiers = new LinkedList<>(); + private final List missingCodeModifiers = new LinkedList<>(); private boolean failedToLoadMixin = false; transient List files = new LinkedList<>(); transient LoadStatus loadStatus = LoadStatus.LOAD_SUCCESS; @@ -77,7 +77,7 @@ final class DiscoveredExtension { } @Nullable - File getOriginalJar() { + public File getOriginalJar() { return originalJar; } diff --git a/src/main/java/net/minestom/server/extensions/Extension.java b/src/main/java/net/minestom/server/extensions/Extension.java index fdd626b77..1b6316043 100644 --- a/src/main/java/net/minestom/server/extensions/Extension.java +++ b/src/main/java/net/minestom/server/extensions/Extension.java @@ -13,7 +13,7 @@ import java.util.function.Consumer; public abstract class Extension { // Set by reflection @SuppressWarnings("unused") - private ExtensionDescription description; + private DiscoveredExtension origin; // Set by reflection @SuppressWarnings("unused") private Logger logger; @@ -24,8 +24,13 @@ public abstract class Extension { * 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<>()); - private ReferenceQueue observerReferenceQueue = new ReferenceQueue<>(); + protected final Set> observers = Collections.newSetFromMap(new ConcurrentHashMap<>()); + protected final ReferenceQueue observerReferenceQueue = new ReferenceQueue<>(); + + /** + * List of extensions that depend on this extension. + */ + protected final Set dependents = new HashSet<>(); protected Extension() { @@ -59,10 +64,14 @@ public abstract class Extension { } @NotNull - public ExtensionDescription getDescription() { - return description; + public DiscoveredExtension getOrigin() { + return origin; } + /** + * Gets the logger for the extension + * @return The logger for the extension + */ @NotNull protected Logger getLogger() { return logger; @@ -71,7 +80,8 @@ public abstract class Extension { /** * Adds a new observer to this extension. * Will be kept as a WeakReference. - * @param observer + * + * @param observer The observer to add */ public void observe(IExtensionObserver observer) { observers.add(new WeakReference<>(observer, observerReferenceQueue)); @@ -82,9 +92,9 @@ public abstract class Extension { * @param action code to execute on each observer */ public void triggerChange(Consumer action) { - for(WeakReference weakObserver : observers) { + for (WeakReference weakObserver : observers) { IExtensionObserver observer = weakObserver.get(); - if(observer != null) { + if (observer != null) { action.accept(observer); } } @@ -94,7 +104,7 @@ 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(); + return !getOrigin().hasFailedToLoadMixin() && getOrigin().getMissingCodeModifiers().isEmpty(); } /** @@ -109,57 +119,10 @@ public abstract class Extension { } } - 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) { - this.name = name; - this.version = version; - this.authors = authors; - this.origin = origin; - failedToLoadMixin = origin.hasFailedToLoadMixin(); - missingCodeModifiers.addAll(origin.getMissingCodeModifiers()); - } - - @NotNull - public String getName() { - return name; - } - - @NotNull - public String getVersion() { - return version; - } - - @NotNull - public List getAuthors() { - return authors; - } - - @NotNull - public List getDependents() { - return dependents; - } - - @NotNull - DiscoveredExtension getOrigin() { - return origin; - } - - @NotNull - public List getMissingCodeModifiers() { - return missingCodeModifiers; - } - - @NotNull - public boolean hasFailedToLoadMixin() { - return failedToLoadMixin; - } + /** + * @return A modifiable list of dependents. + */ + public Set getDependents() { + return dependents; } } diff --git a/src/main/java/net/minestom/server/extensions/ExtensionDependencyResolver.java b/src/main/java/net/minestom/server/extensions/ExtensionDependencyResolver.java index 71d1ce4bb..300cdc4c1 100644 --- a/src/main/java/net/minestom/server/extensions/ExtensionDependencyResolver.java +++ b/src/main/java/net/minestom/server/extensions/ExtensionDependencyResolver.java @@ -37,11 +37,13 @@ public class ExtensionDependencyResolver implements DependencyResolver { // B depends on A (A<-B) // When loading B, with no deep conversion, Ext will not be added to the list of dependencies (because it is not a direct dependency) // But when trying to call/access code from extension A, the parts dependent on Ext won't be inside B's dependencies, triggering a ClassNotFoundException - List deps = new LinkedList<>(); - for (URL u : ext.files) { - deps.add(new ResolvedDependency(u.toExternalForm(), u.toExternalForm(), "", u, new LinkedList<>())); + List dependencies = new LinkedList<>(); + + for (URL url : ext.files) { + dependencies.add(new ResolvedDependency(url.toExternalForm(), url.toExternalForm(), "", url, new LinkedList<>())); } - return new ResolvedDependency(ext.getName(), ext.getName(), ext.getVersion(), ext.files.get(0), deps); + + return new ResolvedDependency(ext.getName(), ext.getName(), ext.getVersion(), ext.files.get(0), dependencies); } throw new UnresolvedDependencyException("No extension named " + extensionName); } diff --git a/src/main/java/net/minestom/server/extensions/ExtensionManager.java b/src/main/java/net/minestom/server/extensions/ExtensionManager.java index b4f672283..260fabaaf 100644 --- a/src/main/java/net/minestom/server/extensions/ExtensionManager.java +++ b/src/main/java/net/minestom/server/extensions/ExtensionManager.java @@ -24,7 +24,6 @@ import java.lang.reflect.Constructor; import java.lang.reflect.Field; import java.lang.reflect.InvocationTargetException; import java.net.URL; -import java.net.URLClassLoader; import java.util.*; import java.util.concurrent.CopyOnWriteArrayList; import java.util.stream.Collectors; @@ -148,12 +147,6 @@ public class ExtensionManager { // Create ExtensionDescription (authors, version etc.) final String extensionName = discoveredExtension.getName(); String mainClass = discoveredExtension.getEntrypoint(); - Extension.ExtensionDescription extensionDescription = new Extension.ExtensionDescription( - extensionName, - discoveredExtension.getVersion(), - Arrays.asList(discoveredExtension.getAuthors()), - discoveredExtension - ); MinestomExtensionClassLoader loader = extensionLoaders.get(extensionName.toLowerCase()); @@ -207,9 +200,9 @@ public class ExtensionManager { // Set extension description try { - Field descriptionField = Extension.class.getDeclaredField("description"); - descriptionField.setAccessible(true); - descriptionField.set(extension, extensionDescription); + Field originField = Extension.class.getDeclaredField("origin"); + originField.setAccessible(true); + originField.set(extension, discoveredExtension); } catch (IllegalAccessException e) { // We made it accessible, should not occur } catch (NoSuchFieldException e) { @@ -236,7 +229,7 @@ public class ExtensionManager { if (dep == null) { LOGGER.warn("Dependency {} of {} is null? This means the extension has been loaded without its dependency, which could cause issues later.", dependency, discoveredExtension.getName()); } else { - dep.getDescription().getDependents().add(discoveredExtension.getName()); + dep.getDependents().add(discoveredExtension.getName()); } } @@ -306,14 +299,16 @@ public class ExtensionManager { } } - @Nullable + @NotNull private List generateLoadOrder(@NotNull List discoveredExtensions) { // Do some mapping so we can map strings to extensions. Map extensionMap = new HashMap<>(); Map> dependencyMap = new HashMap<>(); + for (DiscoveredExtension discoveredExtension : discoveredExtensions) { extensionMap.put(discoveredExtension.getName().toLowerCase(), discoveredExtension); } + for (DiscoveredExtension discoveredExtension : discoveredExtensions) { List dependencies = Arrays.stream(discoveredExtension.getDependencies()) @@ -323,7 +318,7 @@ public class ExtensionManager { if (dependencyExtension == null) { // attempt to see if it is not already loaded (happens with dynamic (re)loading) if (extensions.containsKey(dependencyName.toLowerCase())) { - return extensions.get(dependencyName.toLowerCase()).getDescription().getOrigin(); + return extensions.get(dependencyName.toLowerCase()).getOrigin(); } else { LOGGER.error("Extension {} requires an extension called {}.", discoveredExtension.getName(), dependencyName); LOGGER.error("However the extension {} could not be found.", dependencyName); @@ -356,12 +351,17 @@ public class ExtensionManager { ) { // Get all "loadable" (not actually being loaded!) extensions and put them in the sorted list. for (Map.Entry> entry : loadableExtensions) { + // Add to sorted list. sortedList.add(entry.getKey()); + // Remove to make the next iterations a little bit quicker (hopefully) and to find cyclic dependencies. dependencyMap.remove(entry.getKey()); + // Remove this dependency from all the lists (if they include it) to make way for next level of extensions. - dependencyMap.forEach((key, dependencyList) -> dependencyList.remove(entry.getKey())); + for (var dependencies : dependencyMap.values()) { + dependencies.remove(entry.getKey()); + } } } @@ -387,46 +387,56 @@ public class ExtensionManager { private void loadDependencies(List extensions) { List allLoadedExtensions = new LinkedList<>(extensions); - extensionList.stream().map(ext -> ext.getDescription().getOrigin()).forEach(allLoadedExtensions::add); + + for (Extension extension : extensionList) + allLoadedExtensions.add(extension.getOrigin()); + ExtensionDependencyResolver extensionDependencyResolver = new ExtensionDependencyResolver(allLoadedExtensions); - for (DiscoveredExtension ext : extensions) { + + for (DiscoveredExtension discoveredExtension : extensions) { try { DependencyGetter getter = new DependencyGetter(); - DiscoveredExtension.ExternalDependencies externalDependencies = ext.getExternalDependencies(); + DiscoveredExtension.ExternalDependencies externalDependencies = discoveredExtension.getExternalDependencies(); List repoList = new LinkedList<>(); for (var repository : externalDependencies.repositories) { + if (repository.name == null) { throw new IllegalStateException("Missing 'name' element in repository object."); } + if (repository.name.isEmpty()) { throw new IllegalStateException("Invalid 'name' element in repository object."); } + if (repository.url == null) { throw new IllegalStateException("Missing 'url' element in repository object."); } + if (repository.url.isEmpty()) { throw new IllegalStateException("Invalid 'url' element in repository object."); } + repoList.add(new MavenRepository(repository.name, repository.url)); } + getter.addMavenResolver(repoList); getter.addResolver(extensionDependencyResolver); - for (var artifact : externalDependencies.artifacts) { + for (String artifact : externalDependencies.artifacts) { var resolved = getter.get(artifact, dependenciesFolder); - addDependencyFile(resolved, ext); - LOGGER.trace("Dependency of extension {}: {}", ext.getName(), resolved); + addDependencyFile(resolved, discoveredExtension); + LOGGER.trace("Dependency of extension {}: {}", discoveredExtension.getName(), resolved); } - for (var dependencyName : ext.getDependencies()) { + for (String dependencyName : discoveredExtension.getDependencies()) { var resolved = getter.get(dependencyName, dependenciesFolder); - addDependencyFile(resolved, ext); - LOGGER.trace("Dependency of extension {}: {}", ext.getName(), resolved); + addDependencyFile(resolved, discoveredExtension); + LOGGER.trace("Dependency of extension {}: {}", discoveredExtension.getName(), resolved); } } catch (Exception e) { - ext.loadStatus = DiscoveredExtension.LoadStatus.MISSING_DEPENDENCIES; - LOGGER.error("Failed to load dependencies for extension {}", ext.getName()); - LOGGER.error("Extension '{}' will not be loaded", ext.getName()); + discoveredExtension.loadStatus = DiscoveredExtension.LoadStatus.MISSING_DEPENDENCIES; + LOGGER.error("Failed to load dependencies for extension {}", discoveredExtension.getName()); + LOGGER.error("Extension '{}' will not be loaded", discoveredExtension.getName()); LOGGER.error("This is the exception", e); } } @@ -560,7 +570,7 @@ public class ExtensionManager { ext.preTerminate(); ext.terminate(); // remove callbacks for this extension - String extensionName = ext.getDescription().getName(); + String extensionName = ext.getOrigin().getName(); ext.triggerChange(observer -> observer.onExtensionUnload(extensionName)); // TODO: more callback types @@ -570,10 +580,10 @@ public class ExtensionManager { // remove as dependent of other extensions // this avoids issues where a dependent extension fails to reload, and prevents the base extension to reload too for (Extension e : extensionList) { - e.getDescription().getDependents().remove(ext.getDescription().getName()); + e.getDependents().remove(ext.getOrigin().getName()); } - String id = ext.getDescription().getName().toLowerCase(); + String id = ext.getOrigin().getName().toLowerCase(); // remove from loaded extensions extensions.remove(id); extensionList.remove(ext); @@ -595,19 +605,19 @@ public class ExtensionManager { throw new IllegalArgumentException("Extension " + extensionName + " is not currently loaded."); } - File originalJar = ext.getDescription().getOrigin().getOriginalJar(); + File originalJar = ext.getOrigin().getOriginalJar(); if (originalJar == null) { LOGGER.error("Cannot reload extension {} that is not from a .jar file!", extensionName); return; } LOGGER.info("Reload extension {} from jar file {}", extensionName, originalJar.getAbsolutePath()); - List dependents = new LinkedList<>(ext.getDescription().getDependents()); // copy dependents list + List dependents = new LinkedList<>(ext.getDependents()); // copy dependents list List originalJarsOfDependents = new LinkedList<>(); for (String dependentID : dependents) { Extension dependentExt = extensions.get(dependentID.toLowerCase()); - File dependentOriginalJar = dependentExt.getDescription().getOrigin().getOriginalJar(); + File dependentOriginalJar = dependentExt.getOrigin().getOriginalJar(); originalJarsOfDependents.add(dependentOriginalJar); if (dependentOriginalJar == null) { LOGGER.error("Cannot reload extension {} that is not from a .jar file!", dependentID); @@ -696,7 +706,7 @@ public class ExtensionManager { if (ext == null) { throw new IllegalArgumentException("Extension " + extensionName + " is not currently loaded."); } - List dependents = new LinkedList<>(ext.getDescription().getDependents()); // copy dependents list + List dependents = new LinkedList<>(ext.getDependents()); // copy dependents list for (String dependentID : dependents) { Extension dependentExt = extensions.get(dependentID.toLowerCase()); @@ -715,7 +725,9 @@ public class ExtensionManager { * Shutdowns all the extensions by unloading them. */ public void shutdown() { - this.extensionList.forEach(this::unload); + for (Extension extension : getExtensions()) { + extension.unload(); + } } /** @@ -733,7 +745,7 @@ public class ExtensionManager { List discovered = manager.discoverExtensions(); // setup extension class loaders, so that Mixin can load the json configuration file correctly - for(DiscoveredExtension e : discovered) { + for (DiscoveredExtension e : discovered) { manager.setupClassLoader(e); } @@ -741,7 +753,7 @@ public class ExtensionManager { manager.setupCodeModifiers(discovered, MinestomRootClassLoader.getInstance()); // setup is done, remove all extension classloaders - for(MinestomExtensionClassLoader extensionLoader : manager.getExtensionLoaders().values()) { + for (MinestomExtensionClassLoader extensionLoader : manager.getExtensionLoaders().values()) { MinestomRootClassLoader.getInstance().removeChildInHierarchy(extensionLoader); } LOGGER.info("Early load of code modifiers from extensions done!"); diff --git a/src/test/java/demo/commands/ReloadExtensionCommand.java b/src/test/java/demo/commands/ReloadExtensionCommand.java index a62981004..198a2cf55 100644 --- a/src/test/java/demo/commands/ReloadExtensionCommand.java +++ b/src/test/java/demo/commands/ReloadExtensionCommand.java @@ -25,7 +25,7 @@ public class ReloadExtensionCommand extends Command { static { ReloadExtensionCommand.extensionsName = MinecraftServer.getExtensionManager().getExtensions() .stream() - .map(extension -> extension.getDescription().getName()) + .map(extension -> extension.getOrigin().getName()) .toArray(String[]::new); } diff --git a/src/test/java/improveextensions/missingmodifiers/MissingCodeModifiersExtension.java b/src/test/java/improveextensions/missingmodifiers/MissingCodeModifiersExtension.java index 3101ab76a..f8d8679b0 100644 --- a/src/test/java/improveextensions/missingmodifiers/MissingCodeModifiersExtension.java +++ b/src/test/java/improveextensions/missingmodifiers/MissingCodeModifiersExtension.java @@ -12,9 +12,9 @@ public class MissingCodeModifiersExtension extends Extension { // 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)); + Assertions.assertTrue(getOrigin().hasFailedToLoadMixin(), "Mixin configuration does not exist and should not be loaded"); + Assertions.assertEquals(1, getOrigin().getMissingCodeModifiers().size(), "Code modifier does not exist, it should be reported as missing"); + Assertions.assertEquals("InvalidCodeModifierClass", getOrigin().getMissingCodeModifiers().get(0)); System.out.println("All tests passed."); } catch (AssertionFailedError e) { e.printStackTrace(); diff --git a/src/test/java/improveextensions/unloadcallbacks/UnloadCallbacksExtension.java b/src/test/java/improveextensions/unloadcallbacks/UnloadCallbacksExtension.java index 5289438a6..a2a74fcb3 100644 --- a/src/test/java/improveextensions/unloadcallbacks/UnloadCallbacksExtension.java +++ b/src/test/java/improveextensions/unloadcallbacks/UnloadCallbacksExtension.java @@ -72,7 +72,7 @@ public class UnloadCallbacksExtension extends Extension { MinecraftServer.getSchedulerManager().buildTask(() -> { // unload self - MinecraftServer.getExtensionManager().unloadExtension(getDescription().getName()); + MinecraftServer.getExtensionManager().unloadExtension(getOrigin().getName()); }).delay(1L, TimeUnit.SECOND).schedule(); }