-
Notifications
You must be signed in to change notification settings - Fork 63
Improve plugin load and unload ordering #188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,303 @@ | ||
| package com.legacyminecraft.poseidon; | ||
|
|
||
| import org.bukkit.Server; | ||
| import org.bukkit.plugin.InvalidDescriptionException; | ||
| import org.bukkit.plugin.InvalidPluginException; | ||
| import org.bukkit.plugin.PluginDescriptionFile; | ||
| import org.yaml.snakeyaml.error.YAMLException; | ||
|
|
||
| import java.io.File; | ||
| import java.io.IOException; | ||
| import java.io.InputStream; | ||
| import java.util.*; | ||
| import java.util.jar.JarEntry; | ||
| import java.util.jar.JarFile; | ||
| import java.util.logging.Level; | ||
| import java.util.regex.Matcher; | ||
| import java.util.regex.Pattern; | ||
|
|
||
| public final class PluginLoadPlanner { | ||
| private final Server server; | ||
| private final Set<Pattern> fileFilters; | ||
| private final File updateDirectory; | ||
|
|
||
| public PluginLoadPlanner(Server server, Set<Pattern> fileFilters, File updateDirectory) { | ||
| this.server = server; | ||
| this.fileFilters = fileFilters; | ||
| this.updateDirectory = updateDirectory; | ||
| } | ||
|
|
||
| // Generate a load order for plugins in a given directory based on dependencies. | ||
| public List<PlannedPlugin> plan(File directory, File[] files) { | ||
| if (files == null || files.length == 0) { | ||
| return Collections.emptyList(); | ||
| } | ||
|
|
||
| // Normalize filesystem enumeration so plugin order is not platform-dependent. | ||
| // Issue identified by RobertWesner | ||
| Arrays.sort(files, (left, right) -> left.getName().compareToIgnoreCase(right.getName())); | ||
|
|
||
| // Index plugin metadata up front so dependency decisions can be made before any plugin code runs. | ||
| LinkedHashMap<String, PluginCandidate> candidates = new LinkedHashMap<String, PluginCandidate>(); | ||
|
|
||
| // Loop through files in the directory and parse plugin descriptions, skipping duplicates and invalid plugins. | ||
| //TODO: We should figure out how we want to handle dupe plugins in Poseidon in the future. No reason exists for them and they just cause hard to trobleshoot issues | ||
| for (File file : files) { | ||
| PluginCandidate candidate = createCandidate(file); | ||
| if (candidate == null) { | ||
| continue; | ||
| } | ||
|
|
||
| PluginCandidate existing = candidates.get(candidate.name); | ||
| if (existing != null) { | ||
| server.getLogger().log(Level.SEVERE, "Could not load '" + file.getPath() + "' in folder '" + directory.getPath() + "': duplicate plugin name '" + candidate.name + "' also found in '" + existing.file.getPath() + "'"); | ||
| continue; | ||
| } | ||
|
|
||
| candidates.put(candidate.name, candidate); | ||
| } | ||
|
|
||
| List<PlannedPlugin> plan = new ArrayList<PlannedPlugin>(); | ||
|
|
||
|
|
||
| // Create a deterministic load order | ||
| Set<String> loadedNames = new LinkedHashSet<String>(); | ||
| LinkedHashSet<PluginCandidate> remaining = new LinkedHashSet<PluginCandidate>(candidates.values()); | ||
|
|
||
| // Iterate until no plugins remain or no progress can be made due to missing or circular dependencies. | ||
| while (!remaining.isEmpty()) { | ||
| List<PluginCandidate> ready = new ArrayList<PluginCandidate>(); | ||
|
|
||
| // First try to satisfy both hard dependencies | ||
| for (PluginCandidate candidate : remaining) { | ||
| if (hasMissingHardDependencies(candidate, candidates)) { | ||
| continue; | ||
| } | ||
|
|
||
| // Prefer loading after both hard and present soft dependencies when the graph allows it. | ||
| if (dependenciesLoaded(candidate.getHardDependencies(), loadedNames) | ||
| && dependenciesLoaded(candidate.getPresentSoftDependencies(candidates), loadedNames)) { | ||
| ready.add(candidate); | ||
| } | ||
| } | ||
|
|
||
| // If no plugin can satisfy every soft dependency, fall back to hard-dependency order. | ||
| boolean relaxedSoftDependencies = false; | ||
| if (ready.isEmpty()) { | ||
| for (PluginCandidate candidate : remaining) { | ||
| if (hasMissingHardDependencies(candidate, candidates)) { | ||
| continue; | ||
| } | ||
|
|
||
| // This preserves startup progress when soft dependencies form cycles or long chains. | ||
| if (dependenciesLoaded(candidate.getHardDependencies(), loadedNames)) { | ||
| ready.add(candidate); | ||
| } | ||
| } | ||
| relaxedSoftDependencies = !ready.isEmpty(); | ||
| } | ||
|
|
||
| if (ready.isEmpty()) { | ||
| // Anything left here either references a missing hard dependency or is part of a cycle. | ||
| break; | ||
| } | ||
|
|
||
| // Sort deterministically to ensure a stable load order | ||
| Collections.sort(ready, (left, right) -> { | ||
| int loadOrder = left.description.getLoad().compareTo(right.description.getLoad()); | ||
| if (loadOrder != 0) { | ||
| return loadOrder; | ||
| } | ||
|
|
||
| int nameOrder = left.name.compareToIgnoreCase(right.name); | ||
| if (nameOrder != 0) { | ||
| return nameOrder; | ||
| } | ||
|
|
||
| return left.file.getName().compareToIgnoreCase(right.file.getName()); | ||
| }); | ||
|
|
||
| for (PluginCandidate candidate : ready) { | ||
| // Tell the legacy loader to ignore soft dependencies only when the planner already relaxed them. | ||
| boolean ignoreSoftDependencies = relaxedSoftDependencies || candidate.hasMissingSoftDependencies(candidates); | ||
| plan.add(new PlannedPlugin(candidate.file, candidate.name, ignoreSoftDependencies)); | ||
| loadedNames.add(candidate.name); | ||
| remaining.remove(candidate); | ||
| } | ||
| } | ||
|
|
||
| // Print errors | ||
| for (PluginCandidate candidate : remaining) { | ||
| // If the candidate has missing hard dependencies, report them. Otherwise, report a circular or unresolved dependency chain. | ||
| if (hasMissingHardDependencies(candidate, candidates)) { | ||
| for (String dependency : candidate.getMissingHardDependencies(candidates)) { | ||
| server.getLogger().log(Level.SEVERE, "Could not load '" + candidate.file.getPath() + "' in folder '" + directory.getPath() + "': Unknown dependency " + dependency); | ||
| } | ||
| } else { | ||
| server.getLogger().log(Level.SEVERE, "Could not load '" + candidate.file.getPath() + "' in folder '" + directory.getPath() + "': circular or unresolved dependency chain"); | ||
| } | ||
| } | ||
|
|
||
| return plan; | ||
| } | ||
|
|
||
| private PluginCandidate createCandidate(File file) { | ||
| PluginDescriptionFile description; | ||
|
|
||
| try { | ||
| description = getPluginDescription(file); | ||
| } catch (InvalidPluginException ex) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this also could be a multi catch, and InvalidPluginException is missing the message (which imo can be omitted from both) and is using ex.getCause instead of ex |
||
| server.getLogger().log(Level.SEVERE, "Could not load '" + file.getPath() + "': ", ex.getCause()); | ||
| return null; | ||
| } catch (InvalidDescriptionException ex) { | ||
| server.getLogger().log(Level.SEVERE, "Could not load '" + file.getPath() + "': " + ex.getMessage(), ex); | ||
| return null; | ||
| } | ||
|
|
||
| if (description == null) { | ||
| // Non-plugin files in the directory are ignored by the registered file filters. | ||
| return null; | ||
| } | ||
|
|
||
| return new PluginCandidate(file, description); | ||
| } | ||
|
|
||
| private PluginDescriptionFile getPluginDescription(File file) throws InvalidPluginException, InvalidDescriptionException { | ||
| // Read plugin.yml first so ordering can be computed without instantiating plugin classes. | ||
| File descriptionSource = getEffectivePluginFile(file); // If plugin has an update, read description from the update file instead as it might have new dependencies. | ||
|
|
||
| for (Pattern filter : fileFilters) { | ||
| Matcher match = filter.matcher(descriptionSource.getName()); | ||
| if (!match.find()) { | ||
| continue; | ||
| } | ||
|
|
||
| JarFile jar = null; | ||
| InputStream stream = null; | ||
| try { | ||
| jar = new JarFile(descriptionSource); | ||
| JarEntry entry = jar.getJarEntry("plugin.yml"); | ||
|
|
||
| if (entry == null) { | ||
| throw new InvalidPluginException(new IOException("Jar does not contain plugin.yml")); | ||
| } | ||
|
|
||
| stream = jar.getInputStream(entry); | ||
| return new PluginDescriptionFile(stream); | ||
| } catch (IOException ex) { | ||
| throw new InvalidPluginException(ex); | ||
| } catch (YAMLException ex) { | ||
| throw new InvalidPluginException(ex); | ||
| } finally { | ||
| if (stream != null) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you could use try-with-resources since java7 on both stream and jar, which in my opinion is much cleaner than checking for being null in finally |
||
| try { | ||
| stream.close(); | ||
| } catch (IOException ignored) { | ||
| } | ||
| } | ||
| if (jar != null) { | ||
| try { | ||
| jar.close(); | ||
| } catch (IOException ignored) { | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| private File getEffectivePluginFile(File file) { | ||
| if (updateDirectory == null || !updateDirectory.isDirectory()) { | ||
| return file; | ||
| } | ||
|
|
||
| File updateFile = new File(updateDirectory, file.getName()); | ||
| if (updateFile.isFile()) { | ||
| // Return the update file instead for processing | ||
| return updateFile; | ||
| } | ||
|
|
||
| return file; | ||
| } | ||
|
|
||
| private boolean hasMissingHardDependencies(PluginCandidate candidate, Map<String, PluginCandidate> candidates) { | ||
| return !candidate.getMissingHardDependencies(candidates).isEmpty(); | ||
| } | ||
|
|
||
| private boolean dependenciesLoaded(Collection<String> dependencies, Set<String> loadedNames) { | ||
| for (String dependency : dependencies) { | ||
| if (!loadedNames.contains(dependency)) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| public static final class PlannedPlugin { | ||
| public final File file; | ||
| public final String name; | ||
| public final boolean ignoreSoftDependencies; | ||
|
|
||
| PlannedPlugin(File file, String name, boolean ignoreSoftDependencies) { | ||
| this.file = file; | ||
| this.name = name; | ||
| this.ignoreSoftDependencies = ignoreSoftDependencies; | ||
| } | ||
| } | ||
|
|
||
| private static final class PluginCandidate { | ||
| private final File file; | ||
| private final PluginDescriptionFile description; | ||
| private final String name; | ||
| private final List<String> hardDependencies; | ||
| private final List<String> softDependencies; | ||
|
|
||
| private PluginCandidate(File file, PluginDescriptionFile description) { | ||
| this.file = file; | ||
| this.description = description; | ||
| this.name = description.getName(); | ||
| this.hardDependencies = copyDependencies(description.getDepend()); | ||
| this.softDependencies = copyDependencies(description.getSoftDepend()); | ||
| } | ||
|
|
||
| private List<String> getHardDependencies() { | ||
| return hardDependencies; | ||
| } | ||
|
|
||
| private List<String> getMissingHardDependencies(Map<String, PluginCandidate> candidates) { | ||
| List<String> missing = new ArrayList<String>(); | ||
| for (String dependency : hardDependencies) { | ||
| if (!candidates.containsKey(dependency)) { | ||
| missing.add(dependency); | ||
| } | ||
| } | ||
| return missing; | ||
| } | ||
|
|
||
| private List<String> getPresentSoftDependencies(Map<String, PluginCandidate> candidates) { | ||
| List<String> present = new ArrayList<String>(); | ||
| for (String dependency : softDependencies) { | ||
| if (candidates.containsKey(dependency)) { | ||
| present.add(dependency); | ||
| } | ||
| } | ||
| return present; | ||
| } | ||
|
|
||
| private boolean hasMissingSoftDependencies(Map<String, PluginCandidate> candidates) { | ||
| // Missing soft dependencies should not block load, but present ones still influence ordering. | ||
| return getPresentSoftDependencies(candidates).size() != softDependencies.size(); | ||
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| private static List<String> copyDependencies(Object dependencies) { | ||
| if (dependencies == null) { | ||
| return Collections.emptyList(); | ||
| } | ||
|
|
||
| return new ArrayList<>((Collection<String>) dependencies); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -147,8 +147,9 @@ public void enablePlugins(PluginLoadOrder type) { | |||||
| Plugin[] plugins = pluginManager.getPlugins(); | ||||||
|
|
||||||
| for (Plugin plugin : plugins) { | ||||||
| if ((!plugin.isEnabled()) && (plugin.getDescription().getLoad() == type)) { | ||||||
| loadPlugin(plugin); | ||||||
| // Re-evaluate every disabled plugin on each phase so deferred dependencies can come alive later. | ||||||
| if ((!plugin.isEnabled()) && shouldAttemptEnable(plugin, type)) { | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| enablePlugin(plugin, type, new LinkedHashSet<String>()); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -163,6 +164,72 @@ public void disablePlugins() { | |||||
| pluginManager.disablePlugins(); | ||||||
| } | ||||||
|
|
||||||
| private boolean shouldAttemptEnable(Plugin plugin, PluginLoadOrder type) { | ||||||
| // Startup plugins are eligible during both passes. Postworld plugins only become eligible later. | ||||||
| //TODO Reevaluate if this should be allowed | ||||||
| return plugin.getDescription().getLoad().ordinal() <= type.ordinal(); | ||||||
| } | ||||||
|
|
||||||
| private boolean enablePlugin(Plugin plugin, PluginLoadOrder type, Set<String> enabling) { | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't fully understand why this complexity is needed. Is it just to ensure startup plugins get loaded first and remaining plugins get loaded afterwards? Why not just .filter() the plugin load order beforehand into those that run on startup and those who run post world load? How is a a startup plugin depending on postworld handled? |
||||||
| if (plugin.isEnabled()) { | ||||||
| return true; | ||||||
| } | ||||||
|
|
||||||
| if (!shouldAttemptEnable(plugin, type)) { | ||||||
| return false; | ||||||
| } | ||||||
|
|
||||||
| String pluginName = plugin.getDescription().getName(); | ||||||
| // Guard against recursive dependency loops during the current enable chain. | ||||||
| if (!enabling.add(pluginName)) { | ||||||
| getLogger().log(Level.SEVERE, "Circular plugin dependency detected while enabling " + plugin.getDescription().getFullName()); | ||||||
| return false; | ||||||
| } | ||||||
|
|
||||||
| try { | ||||||
| Object dependObject = plugin.getDescription().getDepend(); | ||||||
| if (dependObject instanceof Collection) { | ||||||
| for (Object dependencyNameObject : (Collection) dependObject) { | ||||||
| String dependencyName = String.valueOf(dependencyNameObject); | ||||||
| Plugin dependency = pluginManager.getPlugin(dependencyName); | ||||||
|
|
||||||
| if (dependency == null) { | ||||||
| getLogger().log(Level.SEVERE, "Could not enable " + plugin.getDescription().getFullName() + ": missing required dependency " + dependencyName); | ||||||
| return false; | ||||||
| } | ||||||
|
|
||||||
| // A dependency scheduled for a later phase will be retried when that phase runs. | ||||||
| if (!shouldAttemptEnable(dependency, type)) { | ||||||
| return false; | ||||||
| } | ||||||
|
|
||||||
| // Hard dependencies must be fully enabled before this plugin can start. | ||||||
| if (!enablePlugin(dependency, type, enabling)) { | ||||||
| return false; | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| Object softDependObject = plugin.getDescription().getSoftDepend(); | ||||||
| if (softDependObject instanceof Collection) { | ||||||
| for (Object dependencyNameObject : (Collection) softDependObject) { | ||||||
| String dependencyName = String.valueOf(dependencyNameObject); | ||||||
| Plugin dependency = pluginManager.getPlugin(dependencyName); | ||||||
| // Soft dependencies are enabled first when possible, but do not block startup. | ||||||
| if (dependency != null && !dependency.isEnabled() && shouldAttemptEnable(dependency, type)) { | ||||||
| enablePlugin(dependency, type, enabling); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // The actual enable call stays in one place so permission registration behavior is unchanged. | ||||||
| loadPlugin(plugin); | ||||||
| return plugin.isEnabled(); | ||||||
| } finally { | ||||||
| enabling.remove(pluginName); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| private void loadPlugin(Plugin plugin) { | ||||||
| try { | ||||||
| pluginManager.enablePlugin(plugin); | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just here to confirm it works, even if the ordering differs from mine, the dependencies are still maintained: