From dd20828e73b393c52371f6f351fb9f644cd637ce Mon Sep 17 00:00:00 2001
From: Ryan Ernst <ryan@iernst.net>
Date: Sat, 23 Nov 2024 05:49:53 -0800
Subject: [PATCH] Store plugin loader separately from instance (#117393)

Stable plugins do not have a plugin instance, so their loader cannot be
retrieved by looking at the instance class (which is a placeholder).
This commit adds back the class loader of each plugin to the
LoadedPlugin record so that it can be closed by tests.

closes #117220
---
 .../elasticsearch/plugins/PluginsService.java   |  4 ++--
 .../plugins/PluginsServiceTests.java            | 17 +++++++++++------
 .../plugins/MockPluginsService.java             |  2 +-
 3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/server/src/main/java/org/elasticsearch/plugins/PluginsService.java b/server/src/main/java/org/elasticsearch/plugins/PluginsService.java
index cfdb7aaf0b509..6ef3cd17ba2e9 100644
--- a/server/src/main/java/org/elasticsearch/plugins/PluginsService.java
+++ b/server/src/main/java/org/elasticsearch/plugins/PluginsService.java
@@ -62,7 +62,7 @@ public StablePluginsRegistry getStablePluginRegistry() {
      * @param descriptor Metadata about the plugin, usually loaded from plugin properties
      * @param instance The constructed instance of the plugin's main class
      */
-    record LoadedPlugin(PluginDescriptor descriptor, Plugin instance) {
+    record LoadedPlugin(PluginDescriptor descriptor, Plugin instance, ClassLoader classLoader) {
 
         LoadedPlugin {
             Objects.requireNonNull(descriptor);
@@ -426,7 +426,7 @@ We need to pass a name though so that we can show that a plugin was loaded (via
                 }
                 plugin = loadPlugin(pluginClass, settings, configPath);
             }
-            loadedPlugins.put(name, new LoadedPlugin(pluginBundle.plugin, plugin));
+            loadedPlugins.put(name, new LoadedPlugin(pluginBundle.plugin, plugin, pluginLayer.pluginClassLoader()));
         } finally {
             privilegedSetContextClassLoader(cl);
         }
diff --git a/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java b/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java
index b84f1d2c7635c..015bc72747bf2 100644
--- a/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java
+++ b/server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java
@@ -466,7 +466,8 @@ public void testExtensiblePlugin() {
             List.of(
                 new PluginsService.LoadedPlugin(
                     new PluginDescriptor("extensible", null, null, null, null, classname, null, List.of(), false, false, false, false),
-                    extensiblePlugin
+                    extensiblePlugin,
+                    null
                 )
             )
         );
@@ -480,7 +481,8 @@ public void testExtensiblePlugin() {
             List.of(
                 new PluginsService.LoadedPlugin(
                     new PluginDescriptor("extensible", null, null, null, null, classname, null, List.of(), false, false, false, false),
-                    extensiblePlugin
+                    extensiblePlugin,
+                    null
                 ),
                 new PluginsService.LoadedPlugin(
                     new PluginDescriptor(
@@ -497,7 +499,8 @@ public void testExtensiblePlugin() {
                         false,
                         false
                     ),
-                    testPlugin
+                    testPlugin,
+                    null
                 )
             )
         );
@@ -885,20 +888,22 @@ static final class Loader extends ClassLoader {
     // We can use the direct ClassLoader from the plugin because tests do not use any parent SPI ClassLoaders.
     static void closePluginLoaders(PluginsService pluginService) {
         for (var lp : pluginService.plugins()) {
-            if (lp.instance().getClass().getClassLoader() instanceof URLClassLoader urlClassLoader) {
+            if (lp.classLoader() instanceof URLClassLoader urlClassLoader) {
                 try {
                     PrivilegedOperations.closeURLClassLoader(urlClassLoader);
                 } catch (IOException unexpected) {
                     throw new UncheckedIOException(unexpected);
                 }
-            }
-            if (lp.instance().getClass().getClassLoader() instanceof UberModuleClassLoader loader) {
+            } else if (lp.classLoader() instanceof UberModuleClassLoader loader) {
                 try {
                     PrivilegedOperations.closeURLClassLoader(loader.getInternalLoader());
                 } catch (Exception e) {
                     throw new RuntimeException(e);
                 }
+            } else {
+                throw new AssertionError("Cannot close unexpected classloader " + lp.classLoader());
             }
+
         }
     }
 
diff --git a/test/framework/src/main/java/org/elasticsearch/plugins/MockPluginsService.java b/test/framework/src/main/java/org/elasticsearch/plugins/MockPluginsService.java
index d51b2cfb450bc..9e96396493bdf 100644
--- a/test/framework/src/main/java/org/elasticsearch/plugins/MockPluginsService.java
+++ b/test/framework/src/main/java/org/elasticsearch/plugins/MockPluginsService.java
@@ -72,7 +72,7 @@ protected void addServerExportsService(Map<String, List<ModuleQualifiedExportsSe
             if (logger.isTraceEnabled()) {
                 logger.trace("plugin loaded from classpath [{}]", pluginInfo);
             }
-            pluginsLoaded.add(new LoadedPlugin(pluginInfo, plugin));
+            pluginsLoaded.add(new LoadedPlugin(pluginInfo, plugin, MockPluginsService.class.getClassLoader()));
         }
         loadExtensions(pluginsLoaded);
         this.classpathPlugins = List.copyOf(pluginsLoaded);