Skip to content

Commit

Permalink
Store plugin loader separately from instance (#117393)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
rjernst authored Nov 23, 2024
1 parent 8fe13f5 commit dd20828
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
)
);
Expand All @@ -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(
Expand All @@ -497,7 +499,8 @@ public void testExtensiblePlugin() {
false,
false
),
testPlugin
testPlugin,
null
)
)
);
Expand Down Expand Up @@ -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());
}

}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit dd20828

Please sign in to comment.