Improve plugin load and unload ordering by RhysB · Pull Request #188 · retromcorg/Project-Poseidon

RhysB · 2026-04-07T09:41:00Z

📌 Pull Request

Description

Improve plugin load, enable, and disable ordering by introducing a plugin load order planner.

Behaviour:

Plans the plugin load order utilizing the plugin.yml information.
Makes the load order deterministic to prevent OS/Java types from effecting load order.
Priority hard dependencies over soft dependencies where possible.
Disable plugins in the reverse order.

Related Issues / Discussions

Fixes issues discussed in the RetroMC Discord with Zavdav and RobertWesner

Fixes #
Related to #

Motivation & Context

Fixes several bugs regarding plugin loading, enabling, and disabling.

Type of Change

Please tick all that apply:

🐛 Bug fix (non-breaking change that fixes an issue)
⚡ Performance improvement
🧩 New feature (non-breaking)
🔧 Refactor / cleanup (no functional changes)
🔥 Crash / exploit fix
📚 Documentation update
❗ Breaking change (may affect plugins or server behavior)

Testing Performed

Compiled successfully with mvn clean package
Tested on a Beta 1.7.3 server
Existing functionality verified
Edge cases considered

Test details:

- Tested on the RetroMC development server with approx 70 plugins.
- PR resolves load issues we have previously had with some of our plugins.

Compatibility Considerations

No known compatibility impact
Potential plugin impact (described below)
Network / protocol (Netcode) behavior changed
Configuration change required

Compatibility Notes

This PR shouldn't break compatibility of plugins with correct plugin.yml files.

Checklist

Please confirm the following:

No unnecessary formatting or whitespace-only changes
Changes are compatible with Minecraft Beta 1.7.3
No dependencies have been added without discussion with the RetroMC team

RobertWesner

Works, but the code has some minor edges that we might want to change.

RobertWesner · 2026-04-07T18:03:53Z

src/main/java/org/bukkit/plugin/SimplePluginManager.java

+            } catch (UnknownDependencyException ex) {
+                server.getLogger().log(Level.SEVERE, "Could not load '" + plannedPlugin.file.getPath() + "' in folder '" + directory.getPath() + "': " + ex.getMessage(), ex);
+            } catch (InvalidPluginException ex) {
+                server.getLogger().log(Level.SEVERE, "Could not load '" + plannedPlugin.file.getPath() + "' in folder '" + directory.getPath() + "': ", ex.getCause());
+            } catch (InvalidDescriptionException ex) {
+                server.getLogger().log(Level.SEVERE, "Could not load '" + plannedPlugin.file.getPath() + "' in folder '" + directory.getPath() + "': " + ex.getMessage(), ex);


multi-catch would be beneficial here, as all catches are (almost¹) exactly the same

¹ InvalidDescriptionException is missing the message but has the colon and using ex.getCause() instead of ex

in my opinion you don't need to attach the previous exception message, the cause exists for that purpose, the information is practically included twice.

Suggested change

} catch (UnknownDependencyException ex) {

server.getLogger().log(Level.SEVERE, "Could not load '" + plannedPlugin.file.getPath() + "' in folder '" + directory.getPath() + "': " + ex.getMessage(), ex);

} catch (InvalidPluginException ex) {

server.getLogger().log(Level.SEVERE, "Could not load '" + plannedPlugin.file.getPath() + "' in folder '" + directory.getPath() + "': ", ex.getCause());

} catch (InvalidDescriptionException ex) {

server.getLogger().log(Level.SEVERE, "Could not load '" + plannedPlugin.file.getPath() + "' in folder '" + directory.getPath() + "': " + ex.getMessage(), ex);

} catch (UnknownDependencyException | InvalidPluginException | InvalidDescriptionException ex) {

server.getLogger().log(Level.SEVERE, "Could not load '" + plannedPlugin.file.getPath() + "' in folder '" + directory.getPath() + "'.", ex);

RobertWesner · 2026-04-07T18:12:43Z

src/main/java/org/bukkit/plugin/SimplePluginManager.java

            try {
                plugin.getPluginLoader().enablePlugin(plugin);
+                // Record successful enables so shutdown can run in strict reverse dependency order.
+                if (plugin.isEnabled() && !enableOrder.contains(plugin)) {


In what scenario could the second condition !enableOrder.contains(plugin) be relevant?
It could only ever reach here when its not already enabled.
Sounds like its a defensive check that will never actually happen ^^

RobertWesner · 2026-04-07T18:21:16Z

src/main/java/org/bukkit/craftbukkit/CraftServer.java

+        return plugin.getDescription().getLoad().ordinal() <= type.ordinal();
+    }
+
+    private boolean enablePlugin(Plugin plugin, PluginLoadOrder type, Set<String> enabling) {


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?

RobertWesner · 2026-04-07T18:25:19Z

src/main/java/com/legacyminecraft/poseidon/PluginLoadPlanner.java

+            } catch (YAMLException ex) {
+                throw new InvalidPluginException(ex);
+            } finally {
+                if (stream != null) {


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

RobertWesner · 2026-04-07T18:26:22Z

src/main/java/com/legacyminecraft/poseidon/PluginLoadPlanner.java

+
+        try {
+            description = getPluginDescription(file);
+        } catch (InvalidPluginException ex) {


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

RobertWesner · 2026-04-07T18:40:32Z

src/main/java/com/legacyminecraft/poseidon/PluginLoadPlanner.java

+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+public final class PluginLoadPlanner {


Just here to confirm it works, even if the ordering differs from mine, the dependencies are still maintained:

A ABC B C D E F G H B depends on D C depends on D D depends on A H depends on B ABC depends on A, B, C

RobertWesner · 2026-04-07T18:43:46Z

src/main/java/org/bukkit/plugin/SimplePluginManager.java

    private final Map<Boolean, Set<Permission>> defaultPerms = new LinkedHashMap<Boolean, Set<Permission>>();
    private final Map<String, Map<Permissible, Boolean>> permSubs = new HashMap<String, Map<Permissible, Boolean>>();
    private final Map<Boolean, Map<Permissible, Boolean>> defSubs = new HashMap<Boolean, Map<Permissible, Boolean>>();
+    private final List<Plugin> enableOrder = new ArrayList<Plugin>();


Why do we have the generic types on all new? It can be inferred by compiler (and IDE). Just stylistic choice?

RobertWesner · 2026-04-07T18:45:26Z

src/main/java/org/bukkit/craftbukkit/CraftServer.java

-            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)) {


Suggested change

if ((!plugin.isEnabled()) && shouldAttemptEnable(plugin, type)) {

if (!plugin.isEnabled() && shouldAttemptEnable(plugin, type)) {

Improve plugin load and unload ordering

6d48781

RobertWesner requested changes Apr 7, 2026

View reviewed changes

RobertWesner assigned RhysB Apr 7, 2026

RobertWesner mentioned this pull request Apr 7, 2026

Ensure file loading order respects dependencies. #187

Closed

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve plugin load and unload ordering#188

Improve plugin load and unload ordering#188
RhysB wants to merge 1 commit intomasterfrom
load-order

RhysB commented Apr 7, 2026

Uh oh!

RobertWesner left a comment

Uh oh!

RobertWesner Apr 7, 2026

Uh oh!

RobertWesner Apr 7, 2026

Uh oh!

RobertWesner Apr 7, 2026

Uh oh!

RobertWesner Apr 7, 2026

Uh oh!

RobertWesner Apr 7, 2026

Uh oh!

RobertWesner Apr 7, 2026

Uh oh!

RobertWesner Apr 7, 2026

Uh oh!

RobertWesner Apr 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	if ((!plugin.isEnabled()) && shouldAttemptEnable(plugin, type)) {
	if (!plugin.isEnabled() && shouldAttemptEnable(plugin, type)) {

Conversation

RhysB commented Apr 7, 2026

📌 Pull Request

Description

Related Issues / Discussions

Motivation & Context

Type of Change

Testing Performed

Compatibility Considerations

Compatibility Notes

Checklist

Uh oh!

RobertWesner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants