Deprecate API methods added by 'Close Plugin Class Loaders on Disable' (#6737)

This commit is contained in:
Jason Penilla 2021-10-06 23:00:32 -05:00
parent 16088745e1
commit 20feb576da
3 changed files with 44 additions and 80 deletions

View file

@ -13,7 +13,7 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000
} catch (Throwable ex) {
server.getLogger().log(Level.SEVERE, "Error occurred while enabling " + plugin.getDescription().getFullName() + " (Is it up to date?)", ex);
+ // Paper start - Disable plugins that fail to load
+ disablePlugin(jPlugin);
+ this.server.getPluginManager().disablePlugin(jPlugin);
+ return;
+ // Paper end
}

View file

@ -6,6 +6,12 @@ Subject: [PATCH] Close Plugin Class Loaders on Disable
This should close more memory leaks from /reload and disabling plugins,
by closing the class loader and the jar file.
Note: This patch is no longer necessary as upstream now also closes
PluginClassLoaders on disable. This patch is now only to keep around
API methods it previously added, and to add back the log message when a
PluginClassLoader fails to disable, as upstream decided to ignore the
exception.
diff --git a/src/main/java/org/bukkit/plugin/PluginLoader.java b/src/main/java/org/bukkit/plugin/PluginLoader.java
index 0000000000000000000000000000000000000000..0000000000000000000000000000000000000000 100644
--- a/src/main/java/org/bukkit/plugin/PluginLoader.java
@ -14,15 +20,18 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000
* @param plugin Plugin to disable
*/
public void disablePlugin(@NotNull Plugin plugin);
+
+ // Paper start - close Classloader on disable
+ /**
+ * Disables the specified plugin
+ * <p>
+ * Attempting to disable a plugin that is not enabled will have no effect
+ * This method is no longer useful as upstream has
+ * made it so plugin classloaders are always closed on disable.
+ * Use {@link #disablePlugin(Plugin)} instead.
+ *
+ * @param plugin Plugin to disable
+ * @param closeClassloader if the classloader for the Plugin should be closed
+ * @param closeClassloader unused
+ * @deprecated Classloader is always closed by upstream now.
+ */
+ @Deprecated(forRemoval = true)
+ // provide default to allow other PluginLoader implementations to work
+ default public void disablePlugin(@NotNull Plugin plugin, boolean closeClassloader) {
+ disablePlugin(plugin);
@ -39,14 +48,18 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000
+ // Paper start - close Classloader on disable
+ /**
+ * Disables the specified plugin
+ * <p>
+ * Attempting to disable a plugin that is not enabled will have no effect
+ * This method is no longer useful as upstream has
+ * made it so plugin classloaders are always closed on disable.
+ * Use {@link #disablePlugin(Plugin)} instead.
+ *
+ * @param plugin Plugin to disable
+ * @param closeClassloader if the classloader for the Plugin should be closed
+ * @param closeClassloader unused
+ * @deprecated Classloader is always closed by upstream now.
+ */
+ public void disablePlugin(@NotNull Plugin plugin, boolean closeClassloader);
+ @Deprecated(forRemoval = true)
+ public default void disablePlugin(@NotNull Plugin plugin, boolean closeClassloader) {
+ this.disablePlugin(plugin);
+ }
+ // Paper end - close Classloader on disable
+
/**
@ -57,85 +70,36 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000
--- a/src/main/java/org/bukkit/plugin/SimplePluginManager.java
+++ b/src/main/java/org/bukkit/plugin/SimplePluginManager.java
@@ -0,0 +0,0 @@ public final class SimplePluginManager implements PluginManager {
@Override
public void disablePlugins() {
+ disablePlugins(false);
+ }
+
+ public void disablePlugins(boolean closeClassloaders) {
+ // Paper end - close Classloader on disable
Plugin[] plugins = getPlugins();
for (int i = plugins.length - 1; i >= 0; i--) {
- disablePlugin(plugins[i]);
+ disablePlugin(plugins[i], closeClassloaders); // Paper - close Classloader on disable
}
}
+ // Paper start
+ /**
+ * This method is no longer useful as upstream has
+ * made it so plugin classloaders are always closed on disable.
+ * Use {@link #disablePlugins()} instead.
+ *
+ * @param closeClassloaders unused
+ * @deprecated Classloader is always closed by upstream now.
+ */
+ @Deprecated(forRemoval = true)
+ public void disablePlugins(boolean closeClassloaders) {
+ this.disablePlugins();
+ }
+ // Paper end
+
@Override
public void disablePlugin(@NotNull final Plugin plugin) {
+ disablePlugin(plugin, false);
+ }
+
+ @Override
+ public void disablePlugin(@NotNull final Plugin plugin, boolean closeClassloader) {
+ // Paper end - close Classloader on disable
if (plugin.isEnabled()) {
try {
- plugin.getPluginLoader().disablePlugin(plugin);
+ plugin.getPluginLoader().disablePlugin(plugin, closeClassloader); // Paper - close Classloader on disable
} catch (Throwable ex) {
handlePluginException("Error occurred (in the plugin loader) while disabling "
+ plugin.getDescription().getFullName() + " (Is it up to date?)", ex, plugin); // Paper
@@ -0,0 +0,0 @@ public final class SimplePluginManager implements PluginManager {
@Override
public void clearPlugins() {
synchronized (this) {
- disablePlugins();
+ disablePlugins(true); // Paper - close Classloader on disable
plugins.clear();
lookupNames.clear();
dependencyGraph = GraphBuilder.directed().build();
diff --git a/src/main/java/org/bukkit/plugin/java/JavaPluginLoader.java b/src/main/java/org/bukkit/plugin/java/JavaPluginLoader.java
index 0000000000000000000000000000000000000000..0000000000000000000000000000000000000000 100644
--- a/src/main/java/org/bukkit/plugin/java/JavaPluginLoader.java
+++ b/src/main/java/org/bukkit/plugin/java/JavaPluginLoader.java
@@ -0,0 +0,0 @@ public final class JavaPluginLoader implements PluginLoader {
} catch (Throwable ex) {
server.getLogger().log(Level.SEVERE, "Error occurred while enabling " + plugin.getDescription().getFullName() + " (Is it up to date?)", ex);
// Paper start - Disable plugins that fail to load
- disablePlugin(jPlugin);
+ server.getPluginManager().disablePlugin(jPlugin, true); // Paper - close Classloader on disable - She's dead jim
return;
// Paper end
}
@@ -0,0 +0,0 @@ public final class JavaPluginLoader implements PluginLoader {
@Override
public void disablePlugin(@NotNull Plugin plugin) {
+ // Paper start - close Classloader on disable
+ disablePlugin(plugin, false); // Retain old behavior unless requested
+ }
+
+ public void disablePlugin(@NotNull Plugin plugin, boolean closeClassloader) {
+ // Paper end - close Class Loader on disable
Validate.isTrue(plugin instanceof JavaPlugin, "Plugin is not associated with this PluginLoader");
if (plugin.isEnabled()) {
@@ -0,0 +0,0 @@ public final class JavaPluginLoader implements PluginLoader {
loader.close();
} catch (IOException ex) {
//
+ this.server.getLogger().log(Level.WARNING, "Error closing the PluginClassLoader for '" + plugin.getDescription().getFullName() + "'", ex); // Paper - log exception
}
+ // Paper start - close Class Loader on disable
+ try {
+ if (closeClassloader) {
+ loader.close();
+ }
+ } catch (IOException e) {
+ server.getLogger().log(Level.WARNING, "Error closing the Plugin Class Loader for " + plugin.getDescription().getFullName());
+ e.printStackTrace();
+ }
+ // Paper end
}
}
}

View file

@ -38,14 +38,14 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000
List<Command> pluginCommands = PluginCommandYamlParser.parse(plugin);
@@ -0,0 +0,0 @@ public final class SimplePluginManager implements PluginManager {
}
// Paper end
@Override
- public void disablePlugin(@NotNull final Plugin plugin, boolean closeClassloader) {
+ public synchronized void disablePlugin(@NotNull final Plugin plugin, boolean closeClassloader) { // Paper - synchronize
// Paper end - close Classloader on disable
- public void disablePlugin(@NotNull final Plugin plugin) {
+ public synchronized void disablePlugin(@NotNull final Plugin plugin) { // Paper - synchronize
if (plugin.isEnabled()) {
try {
plugin.getPluginLoader().disablePlugin(plugin);
@@ -0,0 +0,0 @@ public final class SimplePluginManager implements PluginManager {
defaultPerms.get(false).clear();
}